← Watch feed
wallet: Remove meaningless bool fallback in FundTransaction
Public commit record
73/100 · Adequate
What the developer wrote
Authored by MarcoFalke
wallet: Remove meaningless bool fallback in FundTransaction
The fallback was added in commit
af4fe7fd126eff2dd1942276ea91c8ab9dd717c6, more than a decade ago.
However, it is irrelevant, given that watch-only wallet is not a concept
anymore after commit 1337c72198a7d32935431d64e9e58c12f9003abc.
So remove the code.
Can be reviewed with the --ignore-all-space option.
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context
What changed, and why it matters
This commit removes an old backward-compatibility feature in Bitcoin Core's wallet RPC command `fundrawtransaction`. Previously, callers could pass a plain `true` or `false` as the second argument, which was silently ignored. Now, passing a boolean triggers a clear JSON type error, and callers must use the normal options object. This is a cleanup change, not a security fix, but it makes the API stricter and easier to reason about.
Recommended action
No urgent action. Operators or tools that still pass a bare boolean to `fundrawtransaction` must update to use the options object. Reviewers should confirm the removed behavior was indeed unused and that no other RPC relies on `.skip_type_check = true` for this argument.
Security signals we found
01
Removal of a no-op backward-compatibility code path that silently accepted arbitrary boolean values
02
Stricter RPC input validation: bare booleans now rejected with a JSON type error
03
No memory safety, cryptographic, or consensus changes observed
04
No explicit security relevance stated by the author
Evidence from the diff
The patch deletes the if (options.type() == UniValue::VBOOL) no-op branch in FundTransaction() and removes .skip_type_check = true from the options RPC argument definition. As a result, fundrawtransaction no longer accepts a bare boolean for its second parameter; it now enforces that options is an object. The functional test is updated to expect error -3 for a boolean and to remove the legacy includeWatching compatibility test for watch-only wallets.
Changed components
src/wallet/rpc/spend.cpptest/functional/wallet_fundrawtransaction.pyRPC method fundrawtransactionInspect captured patch +60 / −65
diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
index a42cec46..4404f81a 100644
--- a/src/wallet/rpc/spend.cpp
+++ b/src/wallet/rpc/spend.cpp
@@ -486,10 +486,7 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact
std::optional<unsigned int> change_position;
bool lockUnspents = false;
if (!options.isNull()) {
- if (options.type() == UniValue::VBOOL) {
- // backward compatibility bool only fallback, does nothing
- } else {
- RPCTypeCheckObj(options,
+ RPCTypeCheckObj(options,
{
{"add_inputs", UniValueType(UniValue::VBOOL)},
{"include_unsafe", UniValueType(UniValue::VBOOL)},
@@ -521,83 +518,82 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact
},
true, true);
- if (options.exists("add_inputs")) {
- coinControl.m_allow_other_inputs = options["add_inputs"].get_bool();
- }
-
- if (options.exists("changeAddress") || options.exists("change_address")) {
- const std::string change_address_str = (options.exists("change_address") ? options["change_address"] : options["changeAddress"]).get_str();
- CTxDestination dest = DecodeDestination(change_address_str);
+ if (options.exists("add_inputs")) {
+ coinControl.m_allow_other_inputs = options["add_inputs"].get_bool();
+ }
- if (!IsValidDestination(dest)) {
- throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Change address must be a valid bitcoin address");
- }
+ if (options.exists("changeAddress") || options.exists("change_address")) {
+ const std::string change_address_str = (options.exists("change_address") ? options["change_address"] : options["changeAddress"]).get_str();
+ CTxDestination dest = DecodeDestination(change_address_str);
- coinControl.destChange = dest;
+ if (!IsValidDestination(dest)) {
+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Change address must be a valid bitcoin address");
}
- if (options.exists("changePosition") || options.exists("change_position")) {
- int pos = (options.exists("change_position") ? options["change_position"] : options["changePosition"]).getInt<int>();
- if (pos < 0 || (unsigned int)pos > recipients.size()) {
- throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds");
- }
- change_position = (unsigned int)pos;
- }
+ coinControl.destChange = dest;
+ }
- if (options.exists("change_type")) {
- if (options.exists("changeAddress") || options.exists("change_address")) {
- throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both change address and address type options");
- }
- if (std::optional<OutputType> parsed = ParseOutputType(options["change_type"].get_str())) {
- coinControl.m_change_type.emplace(parsed.value());
- } else {
- throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown change type '%s'", options["change_type"].get_str()));
- }
+ if (options.exists("changePosition") || options.exists("change_position")) {
+ int pos = (options.exists("change_position") ? options["change_position"] : options["changePosition"]).getInt<int>();
+ if (pos < 0 || (unsigned int)pos > recipients.size()) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds");
}
+ change_position = (unsigned int)pos;
+ }
- if (options.exists("lockUnspents") || options.exists("lock_unspents")) {
- lockUnspents = (options.exists("lock_unspents") ? options["lock_unspents"] : options["lockUnspents"]).get_bool();
+ if (options.exists("change_type")) {
+ if (options.exists("changeAddress") || options.exists("change_address")) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both change address and address type options");
}
-
- if (options.exists("include_unsafe")) {
- coinControl.m_include_unsafe_inputs = options["include_unsafe"].get_bool();
+ if (std::optional<OutputType> parsed = ParseOutputType(options["change_type"].get_str())) {
+ coinControl.m_change_type.emplace(parsed.value());
+ } else {
+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown change type '%s'", options["change_type"].get_str()));
}
+ }
- if (options.exists("feeRate")) {
- if (options.exists("fee_rate")) {
- throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both fee_rate (" + CURRENCY_ATOM + "/vB) and feeRate (" + CURRENCY_UNIT + "/kvB)");
- }
- if (options.exists("conf_target")) {
- throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both conf_target and feeRate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.");
- }
- if (options.exists("estimate_mode")) {
- throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both estimate_mode and feeRate");
- }
- coinControl.m_feerate = CFeeRate(AmountFromValue(options["feeRate"]));
- coinControl.fOverrideFeeRate = true;
- }
+ if (options.exists("lockUnspents") || options.exists("lock_unspents")) {
+ lockUnspents = (options.exists("lock_unspents") ? options["lock_unspents"] : options["lockUnspents"]).get_bool();
+ }
- if (options.exists("replaceable")) {
- coinControl.m_signal_bip125_rbf = options["replaceable"].get_bool();
+ if (options.exists("include_unsafe")) {
+ coinControl.m_include_unsafe_inputs = options["include_unsafe"].get_bool();
+ }
+
+ if (options.exists("feeRate")) {
+ if (options.exists("fee_rate")) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both fee_rate (" + CURRENCY_ATOM + "/vB) and feeRate (" + CURRENCY_UNIT + "/kvB)");
+ }
+ if (options.exists("conf_target")) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both conf_target and feeRate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.");
+ }
+ if (options.exists("estimate_mode")) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both estimate_mode and feeRate");
}
+ coinControl.m_feerate = CFeeRate(AmountFromValue(options["feeRate"]));
+ coinControl.fOverrideFeeRate = true;
+ }
- if (options.exists("minconf")) {
- coinControl.m_min_depth = options["minconf"].getInt<int>();
+ if (options.exists("replaceable")) {
+ coinControl.m_signal_bip125_rbf = options["replaceable"].get_bool();
+ }
- if (coinControl.m_min_depth < 0) {
- throw JSONRPCError(RPC_INVALID_PARAMETER, "Negative minconf");
- }
+ if (options.exists("minconf")) {
+ coinControl.m_min_depth = options["minconf"].getInt<int>();
+
+ if (coinControl.m_min_depth < 0) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Negative minconf");
}
+ }
- if (options.exists("maxconf")) {
- coinControl.m_max_depth = options["maxconf"].getInt<int>();
+ if (options.exists("maxconf")) {
+ coinControl.m_max_depth = options["maxconf"].getInt<int>();
- if (coinControl.m_max_depth < coinControl.m_min_depth) {
- throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("maxconf can't be lower than minconf: %d < %d", coinControl.m_max_depth, coinControl.m_min_depth));
- }
+ if (coinControl.m_max_depth < coinControl.m_min_depth) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("maxconf can't be lower than minconf: %d < %d", coinControl.m_max_depth, coinControl.m_min_depth));
}
- SetFeeEstimateMode(wallet, coinControl, options["conf_target"], options["estimate_mode"], options["fee_rate"], override_min_fee);
}
+ SetFeeEstimateMode(wallet, coinControl, options["conf_target"], options["estimate_mode"], options["fee_rate"], override_min_fee);
}
if (options.exists("solving_data")) {
@@ -774,7 +770,6 @@ RPCMethod fundrawtransaction()
},
FundTxDoc()),
RPCArgOptions{
- .skip_type_check = true,
.oneline_description = "options",
}},
{"iswitness", RPCArg::Type::BOOL, RPCArg::DefaultHint{"depends on heuristic tests"}, "Whether the transaction hex is a serialized witness transaction.\n"
diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py
index 77ccac60..a35b2c80 100755
--- a/test/functional/wallet_fundrawtransaction.py
+++ b/test/functional/wallet_fundrawtransaction.py
@@ -295,6 +295,7 @@ class RawTransactionsTest(BitcoinTestFramework):
assert_equal(utx['txid'], dec_tx['vin'][0]['txid'])
assert_raises_rpc_error(-8, "Unknown named parameter foo", self.nodes[2].fundrawtransaction, rawtx, foo='bar')
+ assert_raises_rpc_error(-3, "JSON value of type bool is not of expected type object", self.nodes[2].fundrawtransaction, rawtx, True)
# reserveChangeKey was deprecated and is now removed
assert_raises_rpc_error(-8, "Unknown named parameter reserveChangeKey", lambda: self.nodes[2].fundrawtransaction(hexstring=rawtx, reserveChangeKey=True))
@@ -762,8 +763,7 @@ class RawTransactionsTest(BitcoinTestFramework):
}]
wwatch.importdescriptors(desc_import)
- # Backward compatibility test (2nd params is includeWatching)
- result = wwatch.fundrawtransaction(rawtx, True)
+ result = wwatch.fundrawtransaction(rawtx)
res_dec = self.nodes[0].decoderawtransaction(result["hex"])
assert_equal(len(res_dec["vin"]), 1)
assert_equal(res_dec["vin"][0]["txid"], self.watchonly_utxo['txid'])
Risk score
Our methodology →Why this scored 20/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.