From 4f4cd353194310a8562da6aae27c9c8eda8a4ff0 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 2 Sep 2024 18:01:36 -0300 Subject: [PATCH 1/2] rpc: decouple sendtoaddress 'subtractfeefromamount' boolean parsing The 'subtractfeefromamount' arg is only boolean for sendtoaddress(). Other commands should never provide it as a boolean. --- src/wallet/rpc/spend.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index bea9b2eec18..bfe6c8812fe 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -68,10 +68,7 @@ std::set InterpretSubtractFeeFromOutputInstructions(const UniValue& sffo_in { std::set sffo_set; if (sffo_instructions.isNull()) return sffo_set; - if (sffo_instructions.isBool()) { - if (sffo_instructions.get_bool()) sffo_set.insert(0); - return sffo_set; - } + for (const auto& sffo : sffo_instructions.getValues()) { if (sffo.isStr()) { for (size_t i = 0; i < destinations.size(); ++i) { @@ -310,10 +307,13 @@ RPCHelpMan sendtoaddress() UniValue address_amounts(UniValue::VOBJ); const std::string address = request.params[0].get_str(); address_amounts.pushKV(address, request.params[1]); - std::vector recipients = CreateRecipients( - ParseOutputs(address_amounts), - InterpretSubtractFeeFromOutputInstructions(request.params[4], address_amounts.getKeys()) - ); + + std::set sffo_set; + if (!request.params[4].isNull() && request.params[4].get_bool()) { + sffo_set.insert(0); + } + + std::vector recipients{CreateRecipients(ParseOutputs(address_amounts), sffo_set)}; const bool verbose{request.params[10].isNull() ? false : request.params[10].get_bool()}; return SendMoney(*pwallet, coin_control, recipients, mapValue, verbose); From cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 7 Sep 2024 12:15:42 -0300 Subject: [PATCH 2/2] RPC: improve SFFO arg parsing, error catching and coverage Following changes were made: 1) Catch and signal error for duplicate string destinations. 2) Catch and signal error for invalid value type. 3) Catch and signal error for string destination not found in tx outputs. 4) Improved 'InterpretSubtractFeeFromOutputInstructions()' code organization. 5) Added test coverage for all possible error failures. Also, fixed two PEP 8 warnings at the 'wallet_sendmany.py' file: - PEP 8: E302 expected 2 blank lines, found 1 at the SendmanyTest class declaration. - PEP 8: E303 too many blank lines (2) at skip_test_if_missing_module() and set_test_params() --- src/wallet/rpc/spend.cpp | 32 +++++++++++++++--------------- test/functional/wallet_sendmany.py | 18 +++++++++++++---- 2 files changed, 30 insertions(+), 20 deletions(-) diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index bfe6c8812fe..4ae1ba7143a 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -70,24 +70,24 @@ std::set InterpretSubtractFeeFromOutputInstructions(const UniValue& sffo_in if (sffo_instructions.isNull()) return sffo_set; for (const auto& sffo : sffo_instructions.getValues()) { + int pos{-1}; if (sffo.isStr()) { - for (size_t i = 0; i < destinations.size(); ++i) { - if (sffo.get_str() == destinations.at(i)) { - sffo_set.insert(i); - break; - } - } - } - if (sffo.isNum()) { - int pos = sffo.getInt(); - if (sffo_set.contains(pos)) - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, duplicated position: %d", pos)); - if (pos < 0) - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, negative position: %d", pos)); - if (pos >= int(destinations.size())) - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, position too large: %d", pos)); - sffo_set.insert(pos); + auto it = find(destinations.begin(), destinations.end(), sffo.get_str()); + if (it == destinations.end()) throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter 'subtract fee from output', destination %s not found in tx outputs", sffo.get_str())); + pos = it - destinations.begin(); + } else if (sffo.isNum()) { + pos = sffo.getInt(); + } else { + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter 'subtract fee from output', invalid value type: %s", uvTypeName(sffo.type()))); } + + if (sffo_set.contains(pos)) + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter 'subtract fee from output', duplicated position: %d", pos)); + if (pos < 0) + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter 'subtract fee from output', negative position: %d", pos)); + if (pos >= int(destinations.size())) + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter 'subtract fee from output', position too large: %d", pos)); + sffo_set.insert(pos); } return sffo_set; } diff --git a/test/functional/wallet_sendmany.py b/test/functional/wallet_sendmany.py index ad99100590b..e338ba7b649 100755 --- a/test/functional/wallet_sendmany.py +++ b/test/functional/wallet_sendmany.py @@ -5,17 +5,17 @@ """Test the sendmany RPC command.""" from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import assert_raises_rpc_error + class SendmanyTest(BitcoinTestFramework): # Setup and helpers def add_options(self, parser): self.add_wallet_options(parser) - def skip_test_if_missing_module(self): self.skip_if_no_wallet() - def set_test_params(self): self.num_nodes = 1 self.setup_clean_chain = True @@ -26,9 +26,19 @@ class SendmanyTest(BitcoinTestFramework): addr_3 = self.wallet.getnewaddress() self.log.info("Test using duplicate address in SFFO argument") - self.def_wallet.sendmany(dummy='', amounts={addr_1: 1, addr_2: 1}, subtractfeefrom=[addr_1, addr_1, addr_1]) + assert_raises_rpc_error(-8, "Invalid parameter 'subtract fee from output', duplicated position: 0", self.def_wallet.sendmany, dummy='', amounts={addr_1: 1, addr_2: 1}, subtractfeefrom=[addr_1, addr_1, addr_1]) self.log.info("Test using address not present in tx.vout in SFFO argument") - self.def_wallet.sendmany(dummy='', amounts={addr_1: 1, addr_2: 1}, subtractfeefrom=[addr_3]) + assert_raises_rpc_error(-8, f"Invalid parameter 'subtract fee from output', destination {addr_3} not found in tx outputs", self.def_wallet.sendmany, dummy='', amounts={addr_1: 1, addr_2: 1}, subtractfeefrom=[addr_3]) + self.log.info("Test using negative index in SFFO argument") + assert_raises_rpc_error(-8, "Invalid parameter 'subtract fee from output', negative position: -5", self.def_wallet.sendmany, dummy='', amounts={addr_1: 1, addr_2: 1}, subtractfeefrom=[-5]) + self.log.info("Test using an out of bounds index in SFFO argument") + assert_raises_rpc_error(-8, "Invalid parameter 'subtract fee from output', position too large: 5", self.def_wallet.sendmany, dummy='', amounts={addr_1: 1, addr_2: 1}, subtractfeefrom=[5]) + self.log.info("Test using an unexpected type in SFFO argument") + assert_raises_rpc_error(-8, "Invalid parameter 'subtract fee from output', invalid value type: bool", self.def_wallet.sendmany, dummy='', amounts={addr_1: 1, addr_2: 1}, subtractfeefrom=[False]) + self.log.info("Test duplicates in SFFO argument, mix string destinations with numeric indexes") + assert_raises_rpc_error(-8, "Invalid parameter 'subtract fee from output', duplicated position: 0", self.def_wallet.sendmany, dummy='', amounts={addr_1: 1, addr_2: 1}, subtractfeefrom=[0, addr_1]) + self.log.info("Test valid mixing of string destinations with numeric indexes in SFFO argument") + self.def_wallet.sendmany(dummy='', amounts={addr_1: 1, addr_2: 1}, subtractfeefrom=[0, addr_2]) def run_test(self): self.nodes[0].createwallet("activewallet")