Skip to content

Commit

Permalink
Merge bitcoin/bitcoin#30844: RPC: improve SFFO arg parsing, error cat…
Browse files Browse the repository at this point in the history 
…ching and coverage

cddcbaf RPC: improve SFFO arg parsing, error catching and coverage (furszy)
4f4cd35 rpc: decouple sendtoaddress 'subtractfeefromamount' boolean parsing (furszy)

Pull request description:

  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()

ACKs for top commit:
  achow101:
    ACK cddcbaf
  murchandamus:
    crACK cddcbaf
  naiyoma:
    TACK [https://github.com/bitcoin/bitcoin/pull/30844/commits/cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e](https://github.com/bitcoin/bitcoin/pull/30844/commits/cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e)
  ismaelsadeeq:
    Code review and Tested ACK cddcbaf

Tree-SHA512: c9c15582b81101a93987458d155394ff2c9ca42864624c034ee808a31c3a7d7f55105dea98e86fce17d3c7b2c1a6b5b77942da66b287f8b8881a60cde78c1a3c
  • Loading branch information
@achow101
achow101 committed Jan 29, 2025
2 parents 1e0c5bd + cddcbaf commit c7869cb
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 28 deletions.
48 changes: 24 additions & 24 deletions src/wallet/rpc/spend.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,29 +68,26 @@ std::set<int> InterpretSubtractFeeFromOutputInstructions(const UniValue& sffo_in
{
std::set<int> 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()) {
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<int>();
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<int>();
} 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;
}
Expand Down Expand Up @@ -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<CRecipient> recipients = CreateRecipients(
ParseOutputs(address_amounts),
InterpretSubtractFeeFromOutputInstructions(request.params[4], address_amounts.getKeys())
);

std::set<int> sffo_set;
if (!request.params[4].isNull() && request.params[4].get_bool()) {
sffo_set.insert(0);
}

std::vector<CRecipient> 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);
Expand Down
18 changes: 14 additions & 4 deletions test/functional/wallet_sendmany.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -26,9 +26,19 @@ def test_sffo_repeated_address(self):
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")
Expand Down

0 comments on commit c7869cb

Please sign in to comment.