From bcf55ba9fe9737139f9ffc021d26bd91f793c807 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 14 Sep 2022 16:04:57 -0400 Subject: [PATCH 1/3] Merge bitcoin/bitcoin#26053: rpc: bugfix, 'add_inputs' default value is true unless 'inputs' are provided b00fc44ca5cb938f18d31cde7feb4e1c968dcc2f test: add coverage for 'add_inputs' dynamic default value (furszy) ddbcfdf3d050061f4e8979a0e2bb63bba5a43c47 RPC: bugfix, 'add_inputs' default value is true unless 'inputs' are provided (furszy) Pull request description: This bugfix was meant to be in #25685, but decoupled it to try to make it part of 24.0 release. It's a truly misleading functionality. This PR doesn't change behavior in any way. Just fixes two invalid RPC help messages and adds test coverage for the current behavior. #### Description In both RPC commands `send()` and `walletcreatefundedpsbt` the help message says that `add_inputs` default value is false when it's actually dynamically set by the following statement: ```c++ coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; ``` Which means that, by default, `add_inputs` is true unless there is any pre-set input, in which case, the default is false. ACKs for top commit: achow101: ACK b00fc44ca5cb938f18d31cde7feb4e1c968dcc2f S3RK: ACK b00fc44ca5cb938f18d31cde7feb4e1c968dcc2f Tree-SHA512: 5c68a40d81c994e0ab6de0817db69c4d3dea3a9a64a60362531bf583b7a4c37d524b740905a3f3a89cdbf221913ff5b504746625adb8622788aea93a35bbcd40 --- src/wallet/rpc/spend.cpp | 4 +- test/functional/wallet_fundrawtransaction.py | 115 +++++++++++++++++++ 2 files changed, 117 insertions(+), 2 deletions(-) diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index de9385e82b5d..f44eedff2bc8 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -926,7 +926,7 @@ RPCHelpMan send() {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "", Cat>( { - {"add_inputs", RPCArg::Type::BOOL, RPCArg::Default{false}, "If inputs are specified, automatically include more if they are not enough."}, + {"add_inputs", RPCArg::Type::BOOL, RPCArg::DefaultHint{"false when \"inputs\" are specified, true otherwise"},"Automatically include coins from the wallet to cover the target amount.\n"}, {"include_unsafe", RPCArg::Type::BOOL, RPCArg::Default{false}, "Include inputs that are not safe to spend (unconfirmed transactions from outside keys and unconfirmed replacement transactions).\n" "Warning: the resulting transaction may become invalid if one of the unsafe inputs disappears.\n" "If that happens, you will need to fund the transaction with different inputs and republish it."}, @@ -1377,7 +1377,7 @@ RPCHelpMan walletcreatefundedpsbt() {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "", Cat>( { - {"add_inputs", RPCArg::Type::BOOL, RPCArg::Default{false}, "If inputs are specified, automatically include more if they are not enough."}, + {"add_inputs", RPCArg::Type::BOOL, RPCArg::DefaultHint{"false when \"inputs\" are specified, true otherwise"}, "Automatically include coins from the wallet to cover the target amount.\n"}, {"include_unsafe", RPCArg::Type::BOOL, RPCArg::Default{false}, "Include inputs that are not safe to spend (unconfirmed transactions from outside keys and unconfirmed replacement transactions).\n" "Warning: the resulting transaction may become invalid if one of the unsafe inputs disappears.\n" "If that happens, you will need to fund the transaction with different inputs and republish it."}, diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py index f97b54ecec09..92b5921378ff 100755 --- a/test/functional/wallet_fundrawtransaction.py +++ b/test/functional/wallet_fundrawtransaction.py @@ -103,6 +103,7 @@ def run_test(self): self.generate(self.nodes[2], 1) self.generate(self.nodes[0], 121) + self.test_add_inputs_default_value() self.test_change_position() self.test_simple() self.test_simple_two_coins() @@ -1021,6 +1022,120 @@ def test_external_inputs(self): self.nodes[2].unloadwallet("extfund") + def test_add_inputs_default_value(self): + self.log.info("Test 'add_inputs' default value") + + # Create and fund the wallet with 5 BTC + self.nodes[2].createwallet("test_preset_inputs") + wallet = self.nodes[2].get_wallet_rpc("test_preset_inputs") + addr1 = wallet.getnewaddress() + self.nodes[0].sendtoaddress(addr1, 5) + self.generate(self.nodes[0], 1) + + # Covered cases: + # 1. Default add_inputs value with no preset inputs (add_inputs=true): + # Expect: automatically add coins from the wallet to the tx. + # 2. Default add_inputs value with preset inputs (add_inputs=false): + # Expect: disallow automatic coin selection. + # 3. Explicit add_inputs=true and preset inputs (with preset inputs not-covering the target amount). + # Expect: include inputs from the wallet. + # 4. Explicit add_inputs=true and preset inputs (with preset inputs covering the target amount). + # Expect: only preset inputs are used. + # 5. Explicit add_inputs=true, no preset inputs (same as (1) but with an explicit set): + # Expect: include inputs from the wallet. + + # Case (1), 'send' command + # 'add_inputs' value is true unless "inputs" are specified, in such case, add_inputs=false. + # So, the wallet will automatically select coins and create the transaction if only the outputs are provided. + tx = wallet.send(outputs=[{addr1: 3}]) + assert tx["complete"] + + # Case (2), 'send' command + # Select an input manually, which doesn't cover the entire output amount and + # verify that the dynamically set 'add_inputs=false' value works. + + # Fund wallet with 2 outputs, 5 BTC each. + addr2 = wallet.getnewaddress() + source_tx = self.nodes[0].send(outputs=[{addr1: 5}, {addr2: 5}], options={"change_position": 0}) + self.generate(self.nodes[0], 1) + + # Select only one input. + options = { + "inputs": [ + { + "txid": source_tx["txid"], + "vout": 1 # change position was hardcoded to index 0 + } + ] + } + assert_raises_rpc_error(-4, "Insufficient funds", wallet.send, outputs=[{addr1: 8}], options=options) + + # Case (3), Explicit add_inputs=true and preset inputs (with preset inputs not-covering the target amount) + options["add_inputs"] = True + options["add_to_wallet"] = False + tx = wallet.send(outputs=[{addr1: 8}], options=options) + assert tx["complete"] + + # Case (4), Explicit add_inputs=true and preset inputs (with preset inputs covering the target amount) + options["inputs"].append({ + "txid": source_tx["txid"], + "vout": 2 # change position was hardcoded to index 0 + }) + tx = wallet.send(outputs=[{addr1: 8}], options=options) + assert tx["complete"] + # Check that only the preset inputs were added to the tx + decoded_psbt_inputs = self.nodes[0].decodepsbt(tx["psbt"])['tx']['vin'] + assert_equal(len(decoded_psbt_inputs), 2) + for input in decoded_psbt_inputs: + assert_equal(input["txid"], source_tx["txid"]) + + # Case (5), assert that inputs are added to the tx by explicitly setting add_inputs=true + options = {"add_inputs": True, "add_to_wallet": True} + tx = wallet.send(outputs=[{addr1: 8}], options=options) + assert tx["complete"] + + ################################################ + + # Case (1), 'walletcreatefundedpsbt' command + # Default add_inputs value with no preset inputs (add_inputs=true) + inputs = [] + outputs = {self.nodes[1].getnewaddress(): 8} + assert "psbt" in wallet.walletcreatefundedpsbt(inputs=inputs, outputs=outputs) + + # Case (2), 'walletcreatefundedpsbt' command + # Default add_inputs value with preset inputs (add_inputs=false). + inputs = [{ + "txid": source_tx["txid"], + "vout": 1 # change position was hardcoded to index 0 + }] + outputs = {self.nodes[1].getnewaddress(): 8} + assert_raises_rpc_error(-4, "Insufficient funds", wallet.walletcreatefundedpsbt, inputs=inputs, outputs=outputs) + + # Case (3), Explicit add_inputs=true and preset inputs (with preset inputs not-covering the target amount) + options["add_inputs"] = True + options["add_to_wallet"] = False + assert "psbt" in wallet.walletcreatefundedpsbt(outputs=[{addr1: 8}], inputs=inputs, options=options) + + # Case (4), Explicit add_inputs=true and preset inputs (with preset inputs covering the target amount) + inputs.append({ + "txid": source_tx["txid"], + "vout": 2 # change position was hardcoded to index 0 + }) + psbt_tx = wallet.walletcreatefundedpsbt(outputs=[{addr1: 8}], inputs=inputs, options=options) + # Check that only the preset inputs were added to the tx + decoded_psbt_inputs = self.nodes[0].decodepsbt(psbt_tx["psbt"])['tx']['vin'] + assert_equal(len(decoded_psbt_inputs), 2) + for input in decoded_psbt_inputs: + assert_equal(input["txid"], source_tx["txid"]) + + # Case (5), 'walletcreatefundedpsbt' command + # Explicit add_inputs=true, no preset inputs + options = { + "add_inputs": True + } + assert "psbt" in wallet.walletcreatefundedpsbt(inputs=[], outputs=outputs, options=options) + + self.nodes[2].unloadwallet("test_preset_inputs") def test_include_unsafe(self): self.log.info("Test fundrawtxn with unsafe inputs") From 3fe49c9366c4da6fd0ab2562b8b4c006bfd255e5 Mon Sep 17 00:00:00 2001 From: fanquake Date: Thu, 24 Aug 2023 11:34:42 +0100 Subject: [PATCH 2/3] Merge bitcoin/bitcoin#28230: rpc: Add MaybeArg() and Arg() default helper c00000df1605788acadceb90c22ae9f00db8a9dc rpc: Add MaybeArg() and Arg() default helper (MarcoFalke) Pull request description: Currently the RPC method implementations have many issues: * Default RPC argument values (and their optionality state) are duplicated in the documentation and the C++ code, with no checks to prevent them from going out of sync. * Getting an optional RPC argument is verbose, using a ternary operator, or worse, a multi-line `if`. Fix all issues by adding default helper that can be called via `self.Arg(0)`. The helper needs a few lines of code in the `src/rpc/util.h` header file. Everything else will be implemented in the cpp file once and if an RPC method needs it. There is also an `self.MaybeArg(0)` helper that works on any arg to return the argument, the default, or a falsy value. ACKs for top commit: ajtowns: reACK c00000df1605788acadceb90c22ae9f00db8a9dc stickies-v: re-ACK c00000df1605788acadceb90c22ae9f00db8a9dc TheCharlatan: re-ACK c00000df1605788acadceb90c22ae9f00db8a9dc Tree-SHA512: e7ddcab3faa319bc53edbdf3f89ce83389d2c4e571d5db42401620ff105e522a4a0669dad08e08cde5fd05c790aec3b806f63261a9100c2778865a489e57381e --- src/rpc/mining.cpp | 8 +++--- src/rpc/util.cpp | 46 +++++++++++++++++++++++++++++++ src/rpc/util.h | 58 +++++++++++++++++++++++++++++++++++++++ src/wallet/rpc/coins.cpp | 4 +-- src/wallet/rpc/wallet.cpp | 2 +- 5 files changed, 111 insertions(+), 7 deletions(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index ea13095ed37f..fb1ef58baa60 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -118,7 +118,7 @@ static RPCHelpMan getnetworkhashps() ChainstateManager& chainman = EnsureAnyChainman(request.context); LOCK(cs_main); - return GetNetworkHashPS(!request.params[0].isNull() ? request.params[0].getInt() : 120, !request.params[1].isNull() ? request.params[1].getInt() : -1, chainman.ActiveChain()); + return GetNetworkHashPS(self.Arg(0), self.Arg(1), chainman.ActiveChain()); }, }; } @@ -230,12 +230,12 @@ static RPCHelpMan generatetodescriptor() "\nGenerate 11 blocks to mydesc\n" + HelpExampleCli("generatetodescriptor", "11 \"mydesc\"")}, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - const int num_blocks{request.params[0].getInt()}; - const uint64_t max_tries{request.params[2].isNull() ? DEFAULT_MAX_TRIES : request.params[2].getInt()}; + const auto num_blocks{self.Arg(0)}; + const auto max_tries{self.Arg(2)}; CScript coinbase_script; std::string error; - if (!getScriptFromDescriptor(request.params[1].get_str(), coinbase_script, error)) { + if (!getScriptFromDescriptor(self.Arg(1), coinbase_script, error)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error); } diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 738a5dea162e..02be3a9b80a4 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -525,13 +525,59 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const if (request.mode == JSONRPCRequest::GET_HELP || !IsValidNumArgs(request.params.size())) { throw std::runtime_error(ToString()); } + CHECK_NONFATAL(m_req == nullptr); + m_req = &request; UniValue ret = m_fun(*this, request); + m_req = nullptr; if (gArgs.GetBoolArg("-rpcdoccheck", DEFAULT_RPC_DOC_CHECK)) { CHECK_NONFATAL(std::any_of(m_results.m_results.begin(), m_results.m_results.end(), [&ret](const RPCResult& res) { return res.MatchesType(ret); })); } return ret; } +using CheckFn = void(const RPCArg&); +static const UniValue* DetailMaybeArg(CheckFn* check, const std::vector& params, const JSONRPCRequest* req, size_t i) +{ + CHECK_NONFATAL(i < params.size()); + const UniValue& arg{CHECK_NONFATAL(req)->params[i]}; + const RPCArg& param{params.at(i)}; + if (check) check(param); + + if (!arg.isNull()) return &arg; + if (!std::holds_alternative(param.m_fallback)) return nullptr; + return &std::get(param.m_fallback); +} + +static void CheckRequiredOrDefault(const RPCArg& param) +{ + // Must use `Arg(i)` to get the argument or its default value. + const bool required{ + std::holds_alternative(param.m_fallback) && RPCArg::Optional::NO == std::get(param.m_fallback), + }; + CHECK_NONFATAL(required || std::holds_alternative(param.m_fallback)); +} + +#define TMPL_INST(check_param, ret_type, return_code) \ + template <> \ + ret_type RPCHelpMan::ArgValue(size_t i) const \ + { \ + const UniValue* maybe_arg{ \ + DetailMaybeArg(check_param, m_args, m_req, i), \ + }; \ + return return_code \ + } \ + void force_semicolon(ret_type) + +// Optional arg (without default). Can also be called on required args, if needed. +TMPL_INST(nullptr, std::optional, maybe_arg ? std::optional{maybe_arg->get_real()} : std::nullopt;); +TMPL_INST(nullptr, std::optional, maybe_arg ? std::optional{maybe_arg->get_bool()} : std::nullopt;); +TMPL_INST(nullptr, const std::string*, maybe_arg ? &maybe_arg->get_str() : nullptr;); + +// Required arg or optional arg with default value. +TMPL_INST(CheckRequiredOrDefault, int, CHECK_NONFATAL(maybe_arg)->getInt();); +TMPL_INST(CheckRequiredOrDefault, uint64_t, CHECK_NONFATAL(maybe_arg)->getInt();); +TMPL_INST(CheckRequiredOrDefault, const std::string&, CHECK_NONFATAL(maybe_arg)->get_str();); + bool RPCHelpMan::IsValidNumArgs(size_t num_args) const { size_t num_required_args = 0; diff --git a/src/rpc/util.h b/src/rpc/util.h index cbbd8831b283..5e113e992c13 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -5,6 +5,7 @@ #ifndef BITCOIN_RPC_UTIL_H #define BITCOIN_RPC_UTIL_H +#include #include #include #include @@ -13,14 +14,30 @@ #include