From 8956c0d20f6b207a269b3e62a3b53d6dd81527dc Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Wed, 17 May 2023 13:05:39 +0100 Subject: [PATCH 01/11] Merge bitcoin/bitcoin#27571: ci: Run iwyu on all src files ddddf4957b02c83ed9b6c46b35d8ae1e137889d2 ci: Run iwyu on all src files (MarcoFalke) Pull request description: This makes it easier to look at the CI output of a file without having to manually add it first to the list. ACKs for top commit: hebasto: ACK ddddf4957b02c83ed9b6c46b35d8ae1e137889d2 Tree-SHA512: 342b52838ae45ea343731c30058cdd5595d5ea5601a1f396de4466ccdd63f7ab07b3a193df3669e4dca7cb535557dcc98f866b3cf986b98176b20ecead123868 Co-authored-by: fanquake --- ci/dash/lint-tidy.sh | 45 +++++------------------- contrib/containers/ci/ci-slim.Dockerfile | 1 + 2 files changed, 9 insertions(+), 37 deletions(-) diff --git a/ci/dash/lint-tidy.sh b/ci/dash/lint-tidy.sh index e82444291d5e..3dbb26c16050 100755 --- a/ci/dash/lint-tidy.sh +++ b/ci/dash/lint-tidy.sh @@ -94,45 +94,16 @@ python3 "${CLANG_TIDY_CACHE_PY}" --show-stats 2>&1 || true cleanup_ctcache "${CTCACHE_DIR}" "${CTCACHE_MAXSIZE_MB}" echo "==========================" +# Filter out files by regex here, because regex may not be +# accepted in src/.bear-tidy-config +# Filter out: +# * qt qrc and moc generated files +# * secp256k1 +jq 'map(select(.file | test("src/qt/qrc_.*\\.cpp$|/moc_.*\\.cpp$|src/secp256k1/src/") | not))' ../compile_commands.json > tmp.json +mv tmp.json ../compile_commands.json + cd "${BASE_ROOT_DIR}/build-ci/dashcore-${BUILD_TARGET}" iwyu_tool.py \ - "src/common/init.cpp" \ - "src/common/url.cpp" \ - "src/compat" \ - "src/dbwrapper.cpp" \ - "src/init" \ - "src/kernel" \ - "src/node/chainstate.cpp" \ - "src/node/chainstatemanager_args.cpp" \ - "src/node/mempool_args.cpp" \ - "src/node/utxo_snapshot.cpp" \ - "src/node/validation_cache_args.cpp" \ - "src/node/minisketchwrapper.cpp" \ - "src/policy/feerate.cpp" \ - "src/policy/packages.cpp" \ - "src/policy/settings.cpp" \ - "src/primitives/transaction.cpp" \ - "src/random.cpp" \ - "src/rpc/fees.cpp" \ - "src/rpc/signmessage.cpp" \ - "src/test/fuzz/string.cpp" \ - "src/test/fuzz/txorphan.cpp" \ - "src/test/fuzz/util/" \ - "src/util/bip32.cpp" \ - "src/util/bytevectorhash.cpp" \ - "src/util/check.cpp" \ - "src/util/error.cpp" \ - "src/util/exception.cpp" \ - "src/util/hasher.cpp" \ - "src/util/message.cpp" \ - "src/util/moneystr.cpp" \ - "src/util/serfloat.cpp" \ - "src/util/spanparsing.cpp" \ - "src/util/string.cpp" \ - "src/util/strencodings.cpp" \ - "src/util/syserror.cpp" \ - "src/util/threadinterrupt.cpp" \ - "src/zmq" \ -p . "${MAKEJOBS}" \ -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \ -Xiwyu --max_line_length=160 \ diff --git a/contrib/containers/ci/ci-slim.Dockerfile b/contrib/containers/ci/ci-slim.Dockerfile index a9701c81d0ff..9aca0169d8d4 100644 --- a/contrib/containers/ci/ci-slim.Dockerfile +++ b/contrib/containers/ci/ci-slim.Dockerfile @@ -42,6 +42,7 @@ RUN set -ex; \ curl \ g++ \ git \ + jq \ libbz2-dev \ liblzma-dev \ libreadline-dev \ From e0288488de5d5dc13a279d86d2c4539e010486a5 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Fri, 19 May 2023 09:53:26 +0100 Subject: [PATCH 02/11] Merge bitcoin/bitcoin#27662: build: Bump minimum supported GCC to g++-9 fa953f15bfcf95df9aa9c91e1c4b56a205f4d1ae build: Bump minimum supported GCC to g++-9 (MarcoFalke) fa69955e741dd60dc6160e81cf223bbecd286806 ci: Bump centos:stream8 to centos:stream9 (MarcoFalke) fa6a755d9fb22cad3d7063b21a1c8a137ae981b2 ci: Document the false positive error for g++-9 (MarcoFalke) Pull request description: It is a bit frustrating to write valid C++ code only to realize that g++-8 fails to parse it later on. The only non-EOL operating system still shipping with g++-8 is CentOS Stream 8. I think it is reasonable for users of affected Linux distributions to: * Upgrade their operating system, or compiler to a supported version. * Alternatively, stay with a previous release of Bitcoin Core as long as it is supported. Fixes https://github.com/bitcoin/bitcoin/issues/27537 ACKs for top commit: hebasto: ACK fa953f15bfcf95df9aa9c91e1c4b56a205f4d1ae fanquake: ACK fa953f15bfcf95df9aa9c91e1c4b56a205f4d1ae Tree-SHA512: b9cf7e763d3071e1e008c5010de19601d4773afe46d58cf869d3f59285c53240c739a1cd7235a5525ede1bbdf6b6cb6fb091c8fc314864a28d5b27a400bb7632 Co-authored-by: fanquake --- src/qt/addresstablemodel.cpp | 5 ++--- src/txrequest.cpp | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index ce13a7701743..3d494871cf30 100644 --- a/src/qt/addresstablemodel.cpp +++ b/src/qt/addresstablemodel.cpp @@ -53,15 +53,14 @@ struct AddressTableEntryLessThan }; /* Determine address type from address purpose */ -static AddressTableEntry::Type translateTransactionType(wallet::AddressPurpose purpose, bool isMine) +constexpr AddressTableEntry::Type translateTransactionType(wallet::AddressPurpose purpose, bool isMine) { // "refund" addresses aren't shown, and change addresses aren't returned by getAddresses at all. switch (purpose) { case wallet::AddressPurpose::SEND: return AddressTableEntry::Sending; case wallet::AddressPurpose::RECEIVE: return AddressTableEntry::Receiving; case wallet::AddressPurpose::REFUND: return AddressTableEntry::Hidden; - // No default case to allow for compiler to warn - } + } // no default case, so the compiler can warn about missing cases assert(false); } diff --git a/src/txrequest.cpp b/src/txrequest.cpp index ae1beed5c634..06f3c7aa3d0a 100644 --- a/src/txrequest.cpp +++ b/src/txrequest.cpp @@ -66,7 +66,7 @@ struct Announcement { const bool m_preferred : 1; /** What state this announcement is in. - * This is a uint8_t instead of a State to silence a GCC warning in versions prior to 8.4 and 9.3. + * This is a uint8_t instead of a State to silence a GCC warning in versions prior to 9.3. * See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414 */ uint8_t m_state : 3 {static_cast(State::CANDIDATE_DELAYED)}; From 5c6a42bc04c61d0817eec984bb629449055039ac Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 29 May 2023 10:21:10 +0100 Subject: [PATCH 03/11] Merge bitcoin/bitcoin#27739: ci: Add missing set -e to 01_base_install.sh fa12558d21aa03c22407a1458a0345d8a7ab6a4b ci: Avoid leaking HOME var into CI pod (MarcoFalke) aaaa4326035c889a7be59cfe738986ae5581d5ac ci: Remove "default" test env (MarcoFalke) fa7a87bc7c3468024c6aa05cda23c575b1cf707a ci: Add missing set -e to 01_base_install.sh (MarcoFalke) Pull request description: Otherwise errors are silently ignored ACKs for top commit: TheCharlatan: ACK [fa12558](https://github.com/bitcoin/bitcoin/pull/27739/commits/fa12558d21aa03c22407a1458a0345d8a7ab6a4b) hebasto: ACK fa12558d21aa03c22407a1458a0345d8a7ab6a4b Tree-SHA512: dbf3f16302c83973b78f3a5e7793090bc9ac44fdf20d51a26b30a99a97369971661e9aed1cd810d80d49d60009651ca0a8aeb2bdc24198a143bf4fff0ec89901 Co-authored-by: fanquake --- ci/test/00_setup_env.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ci/test/00_setup_env.sh b/ci/test/00_setup_env.sh index e71b490a4018..21e8032576f8 100755 --- a/ci/test/00_setup_env.sh +++ b/ci/test/00_setup_env.sh @@ -6,6 +6,8 @@ export LC_ALL=C.UTF-8 +set -ex + # The root dir. # The ci system copies this folder. BASE_ROOT_DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )"/../../ >/dev/null 2>&1 && pwd ) @@ -52,7 +54,6 @@ export RUN_STDLIB_PATH_CHECK=${RUN_STDLIB_PATH_CHECK:-false} export TEST_RUNNER_TIMEOUT_FACTOR=${TEST_RUNNER_TIMEOUT_FACTOR:-4} export RUN_FUZZ_TESTS=${RUN_FUZZ_TESTS:-false} -export CONTAINER_NAME=${CONTAINER_NAME:-ci_unnamed} # Randomize test order. # See https://www.boost.org/doc/libs/1_71_0/libs/test/doc/html/boost_test/utf_reference/rt_param_reference/random.html export BOOST_TEST_RANDOM=${BOOST_TEST_RANDOM:-1} From b617006bfa44cd14e19d33dbcca7431214dbbd62 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 1 Jun 2023 15:19:37 -0400 Subject: [PATCH 04/11] partial Merge bitcoin/bitcoin#26485: RPC: Accept options as named-only parameters BACKPORT NOTE: missing changes in test/functional/wallet_taproot.py (sendall with named psbt=True instead of options={"psbt": True}); the file does not exist in Dash yet and the hunk comes with the Taproot backport 2cd28e9fef5dd743bcd70025196ee311fcfdcae4 rpc: Add check for unintended option/parameter name clashes (Ryan Ofsky) 95d7de0964620a3f7386a4adc5707559868abf84 test: Update python tests to use named parameters instead of options objects (Ryan Ofsky) 96233146dd31c1d99fd1619be4449944623ef750 RPC: Allow RPC methods accepting options to take named parameters (Ryan Ofsky) 702b56d2a8ce48bc3b66a2867d09fa11dcf12fc5 RPC: Add add OBJ_NAMED_PARAMS type (Ryan Ofsky) Pull request description: Allow RPC methods which take an `options` parameter (`importmulti`, `listunspent`, `fundrawtransaction`, `bumpfee`, `send`, `sendall`, `walletcreatefundedpsbt`, `simulaterawtransaction`), to accept the options as named parameters, without the need for nested JSON objects. This makes it possible to make calls like: ```sh src/bitcoin-cli -named bumpfee txid fee_rate=10 ``` instead of ```sh src/bitcoin-cli -named bumpfee txid options='{"fee_rate": 10}' ``` RPC help is also updated to show options as top level named arguments instead of as nested objects.
diff

```diff @@ -15,16 +15,17 @@ Arguments: 1. txid (string, required) The txid to be bumped -2. options (json object, optional) +2. options (json object, optional) Options object that can be used to pass named arguments, listed below. + +Named Arguments: - { - "conf_target": n, (numeric, optional, default=wallet -txconfirmtarget) Confirmation target in blocks +conf_target (numeric, optional, default=wallet -txconfirmtarget) Confirmation target in blocks - "fee_rate": amount, (numeric or string, optional, default=not set, fall back to wallet fee estimation) +fee_rate (numeric or string, optional, default=not set, fall back to wallet fee estimation) Specify a fee rate in sat/vB instead of relying on the built-in fee estimator. Must be at least 1.000 sat/vB higher than the current transaction fee rate. WARNING: before version 0.21, fee_rate was in BTC/kvB. As of 0.21, fee_rate is in sat/vB. - "replaceable": bool, (boolean, optional, default=true) Whether the new transaction should still be +replaceable (boolean, optional, default=true) Whether the new transaction should still be marked bip-125 replaceable. If true, the sequence numbers in the transaction will be left unchanged from the original. If false, any input sequence numbers in the original transaction that were less than 0xfffffffe will be increased to 0xfffffffe @@ -32,11 +33,10 @@ still be replaceable in practice, for example if it has unconfirmed ancestors which are replaceable). - "estimate_mode": "str", (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive): +estimate_mode (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive): "unset" "economical" "conservative" - } Result: { (json object) ```

**Review suggestion:** To understand this PR, it is probably easiest to review the commits in reverse order because the last commit shows the external API changes, the middle commit shows the internal API changes, and the first commit contains the low-level implementation. ACKs for top commit: achow101: ACK 2cd28e9fef5dd743bcd70025196ee311fcfdcae4 Tree-SHA512: 50f6e78fa622826dab3f810400d8c1a03a98a090b1f2fea79729c58ad8cff955554bd44c2a5975f62a526b900dda68981862fd7d7d05c17f94f5b5d847317436 Co-authored-by: Andrew Chow --- doc/release-notes-26485.md | 16 ++ src/rpc/client.cpp | 64 ++++++++ src/rpc/server.cpp | 46 +++++- src/rpc/server.h | 13 +- src/rpc/util.cpp | 85 ++++++++-- src/rpc/util.h | 24 ++- src/test/rpc_tests.cpp | 75 ++++++++- src/wallet/rpc/backup.cpp | 2 +- src/wallet/rpc/coins.cpp | 2 +- src/wallet/rpc/spend.cpp | 16 +- src/wallet/rpc/wallet.cpp | 2 +- test/functional/rpc_help.py | 4 +- test/functional/rpc_psbt.py | 12 +- test/functional/wallet_fundrawtransaction.py | 150 +++++++++--------- test/functional/wallet_import_rescan.py | 2 +- test/functional/wallet_keypool_hd.py | 14 +- test/functional/wallet_migration.py | 2 +- .../wallet_multisig_descriptor_psbt.py | 4 +- .../wallet_resendwallettransactions.py | 6 +- test/functional/wallet_sendall.py | 18 +-- test/functional/wallet_signer.py | 4 +- test/functional/wallet_timelock.py | 2 +- test/functional/wallet_watchonly.py | 4 +- 23 files changed, 413 insertions(+), 154 deletions(-) create mode 100644 doc/release-notes-26485.md diff --git a/doc/release-notes-26485.md b/doc/release-notes-26485.md new file mode 100644 index 000000000000..e17a36f49f97 --- /dev/null +++ b/doc/release-notes-26485.md @@ -0,0 +1,16 @@ +JSON-RPC +--- + +For RPC methods which accept `options` parameters ((`importmulti`, `listunspent`, `fundrawtransaction`, `send`, `sendall`, `walletcreatefundedpsbt`, `simulaterawtransaction`), it is now possible to pass the options as named parameters without the need for a nested object. (#8010) + +This means it is possible make calls like: + +```sh +src/dash-cli -named listunspent coinType=1 +``` + +instead of + +```sh +src/dash-cli -named listunspent query_options='{"coinType": 1}' +``` diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 1bef88730638..7fe8d09cb306 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -121,6 +121,12 @@ static const CRPCConvertParam vRPCConvertParams[] = { "listunspent", 2, "addresses" }, { "listunspent", 3, "include_unsafe" }, { "listunspent", 4, "query_options" }, + { "listunspent", 4, "minimumAmount" }, + { "listunspent", 4, "maximumAmount" }, + { "listunspent", 4, "maximumCount" }, + { "listunspent", 4, "minimumSumAmount" }, + { "listunspent", 4, "include_immature_coinbase" }, + { "listunspent", 4, "coinType" }, { "getblock", 1, "verbosity" }, { "getblock", 1, "verbose" }, { "getblockheader", 1, "verbose" }, @@ -152,10 +158,37 @@ static const CRPCConvertParam vRPCConvertParams[] = { "submitpackage", 0, "package" }, { "combinerawtransaction", 0, "txs" }, { "fundrawtransaction", 1, "options" }, + { "fundrawtransaction", 1, "add_inputs"}, + { "fundrawtransaction", 1, "include_unsafe"}, + { "fundrawtransaction", 1, "minconf"}, + { "fundrawtransaction", 1, "maxconf"}, + { "fundrawtransaction", 1, "changePosition"}, + { "fundrawtransaction", 1, "includeWatching"}, + { "fundrawtransaction", 1, "lockUnspents"}, + { "fundrawtransaction", 1, "fee_rate"}, + { "fundrawtransaction", 1, "feeRate"}, + { "fundrawtransaction", 1, "subtractFeeFromOutputs"}, + { "fundrawtransaction", 1, "input_sizes"}, + { "fundrawtransaction", 1, "conf_target"}, + { "fundrawtransaction", 1, "solving_data"}, + { "fundrawtransaction", 1, "use_cj"}, { "walletcreatefundedpsbt", 0, "inputs" }, { "walletcreatefundedpsbt", 1, "outputs" }, { "walletcreatefundedpsbt", 2, "locktime" }, { "walletcreatefundedpsbt", 3, "options" }, + { "walletcreatefundedpsbt", 3, "add_inputs"}, + { "walletcreatefundedpsbt", 3, "include_unsafe"}, + { "walletcreatefundedpsbt", 3, "minconf"}, + { "walletcreatefundedpsbt", 3, "maxconf"}, + { "walletcreatefundedpsbt", 3, "changePosition"}, + { "walletcreatefundedpsbt", 3, "includeWatching"}, + { "walletcreatefundedpsbt", 3, "lockUnspents"}, + { "walletcreatefundedpsbt", 3, "fee_rate"}, + { "walletcreatefundedpsbt", 3, "feeRate"}, + { "walletcreatefundedpsbt", 3, "subtractFeeFromOutputs"}, + { "walletcreatefundedpsbt", 3, "conf_target"}, + { "walletcreatefundedpsbt", 3, "solving_data"}, + { "walletcreatefundedpsbt", 3, "use_cj"}, { "walletcreatefundedpsbt", 4, "bip32derivs" }, { "walletprocesspsbt", 1, "sign" }, { "walletprocesspsbt", 3, "bip32derivs" }, @@ -182,12 +215,42 @@ static const CRPCConvertParam vRPCConvertParams[] = { "send", 1, "conf_target" }, { "send", 3, "fee_rate"}, { "send", 4, "options" }, + { "send", 4, "add_inputs"}, + { "send", 4, "include_unsafe"}, + { "send", 4, "minconf"}, + { "send", 4, "maxconf"}, + { "send", 4, "add_to_wallet"}, + { "send", 4, "change_position"}, + { "send", 4, "fee_rate"}, + { "send", 4, "include_watching"}, + { "send", 4, "inputs"}, + { "send", 4, "locktime"}, + { "send", 4, "lock_unspents"}, + { "send", 4, "psbt"}, + { "send", 4, "subtract_fee_from_outputs"}, + { "send", 4, "conf_target"}, + { "send", 4, "solving_data"}, + { "send", 4, "use_cj"}, { "sendall", 0, "recipients" }, { "sendall", 1, "conf_target" }, { "sendall", 3, "fee_rate"}, { "sendall", 4, "options" }, + { "sendall", 4, "add_to_wallet"}, + { "sendall", 4, "fee_rate"}, + { "sendall", 4, "include_watching"}, + { "sendall", 4, "inputs"}, + { "sendall", 4, "locktime"}, + { "sendall", 4, "lock_unspents"}, + { "sendall", 4, "psbt"}, + { "sendall", 4, "send_max"}, + { "sendall", 4, "minconf"}, + { "sendall", 4, "maxconf"}, + { "sendall", 4, "conf_target"}, + { "sendall", 4, "solving_data"}, + { "sendall", 4, "use_cj"}, { "simulaterawtransaction", 0, "rawtxs" }, { "simulaterawtransaction", 1, "options" }, + { "simulaterawtransaction", 1, "include_watchonly"}, { "importprivkey", 2, "rescan" }, { "importelectrumwallet", 1, "index" }, { "importaddress", 2, "rescan" }, @@ -195,6 +258,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "importpubkey", 2, "rescan" }, { "importmulti", 0, "requests" }, { "importmulti", 1, "options" }, + { "importmulti", 1, "rescan" }, { "importdescriptors", 0, "requests" }, { "listdescriptors", 0, "private" }, { "verifychain", 0, "checklevel" }, diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 440ac1a33df8..5dfe27db2756 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -409,7 +409,7 @@ std::string JSONRPCExecBatch(const JSONRPCRequest& jreq, const UniValue& vReq) * Process named arguments into a vector of positional arguments, based on the * passed-in specification for the RPC call's arguments. */ -static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, const std::vector& argNames) +static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, const std::vector>& argNames) { JSONRPCRequest out = in; out.params = UniValue(UniValue::VARR); @@ -434,7 +434,9 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c // "args" parameter, if present. int hole = 0; int initial_hole_size = 0; - for (const std::string &argNamePattern: argNames) { + const std::string* initial_param = nullptr; + UniValue options{UniValue::VOBJ}; + for (const auto& [argNamePattern, named_only]: argNames) { std::vector vargNames = SplitString(argNamePattern, '|'); auto fr = argsIn.end(); for (const std::string & argName : vargNames) { @@ -443,7 +445,22 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c break; } } - if (fr != argsIn.end()) { + + // Handle named-only parameters by pushing them into a temporary options + // object, and then pushing the accumulated options as the next + // positional argument. + if (named_only) { + if (fr != argsIn.end()) { + if (options.exists(fr->first)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Parameter " + fr->first + " specified multiple times"); + } + options.pushKVEnd(fr->first, *fr->second); + argsIn.erase(fr); + } + continue; + } + + if (!options.empty() || fr != argsIn.end()) { for (int i = 0; i < hole; ++i) { // Fill hole between specified parameters with JSON nulls, // but not at the end (for backwards compatibility with calls @@ -451,12 +468,26 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c out.params.push_back(UniValue()); } hole = 0; - out.params.push_back(*fr->second); - argsIn.erase(fr); + if (!initial_param) initial_param = &argNamePattern; } else { hole += 1; if (out.params.empty()) initial_hole_size = hole; } + + // If named input parameter "fr" is present, push it onto out.params. If + // options are present, push them onto out.params. If both are present, + // throw an error. + if (fr != argsIn.end()) { + if (!options.empty()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Parameter " + fr->first + " conflicts with parameter " + options.getKeys().front()); + } + out.params.push_back(*fr->second); + argsIn.erase(fr); + } + if (!options.empty()) { + out.params.push_back(std::move(options)); + options = UniValue{UniValue::VOBJ}; + } } // If leftover "args" param was found, use it as a source of positional // arguments and add named arguments after. This is a convenience for @@ -464,9 +495,8 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c // arguments as described in doc/JSON-RPC-interface.md#parameter-passing auto positional_args{argsIn.extract("args")}; if (positional_args && positional_args.mapped()->isArray()) { - const bool has_named_arguments{initial_hole_size < (int)argNames.size()}; - if (initial_hole_size < (int)positional_args.mapped()->size() && has_named_arguments) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Parameter " + argNames[initial_hole_size] + " specified twice both as positional and named argument"); + if (initial_hole_size < (int)positional_args.mapped()->size() && initial_param) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Parameter " + *initial_param + " specified twice both as positional and named argument"); } // Assign positional_args to out.params and append named_args after. UniValue named_args{std::move(out.params)}; diff --git a/src/rpc/server.h b/src/rpc/server.h index 73b362d50d89..b8348e4aa663 100644 --- a/src/rpc/server.h +++ b/src/rpc/server.h @@ -93,7 +93,7 @@ class CRPCCommand using Actor = std::function; //! Constructor taking Actor callback supporting multiple handlers. - CRPCCommand(std::string category, std::string name, Actor actor, std::vector args, intptr_t unique_id) + CRPCCommand(std::string category, std::string name, Actor actor, std::vector> args, intptr_t unique_id) : category(std::move(category)), name(std::move(name)), actor(std::move(actor)), argNames(std::move(args)), unique_id(unique_id) { @@ -113,7 +113,16 @@ class CRPCCommand std::string category; std::string name; Actor actor; - std::vector argNames; + //! List of method arguments and whether they are named-only. Incoming RPC + //! requests contain a "params" field that can either be an array containing + //! unnamed arguments or an object containing named arguments. The + //! "argNames" vector is used in the latter case to transform the params + //! object into an array. Each argument in "argNames" gets mapped to a + //! unique position in the array, based on the order it is listed, unless + //! the argument is a named-only argument with argNames[x].second set to + //! true. Named-only arguments are combined into a JSON object that is + //! appended after other arguments, see transformNamedArguments for details. + std::vector> argNames; intptr_t unique_id; }; diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 362ba3001e73..8bb5e2ff4967 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -367,7 +367,8 @@ struct Sections { case RPCArg::Type::NUM: case RPCArg::Type::AMOUNT: case RPCArg::Type::RANGE: - case RPCArg::Type::BOOL: { + case RPCArg::Type::BOOL: + case RPCArg::Type::OBJ_NAMED_PARAMS: { if (is_top_level_arg) return; // Nothing more to do for non-recursive types on first recursion auto left = indent; if (arg.m_opts.type_str.size() != 0 && push_name) { @@ -463,12 +464,32 @@ RPCHelpMan::RPCHelpMan(std::string name, std::string description, std::vector named_args; + // Map of parameter names and types just used to check whether the names are + // unique. Parameter names always need to be unique, with the exception that + // there can be pairs of POSITIONAL and NAMED parameters with the same name. + enum ParamType { POSITIONAL = 1, NAMED = 2, NAMED_ONLY = 4 }; + std::map param_names; + for (const auto& arg : m_args) { std::vector names = SplitString(arg.m_names, '|'); // Should have unique named arguments for (const std::string& name : names) { - CHECK_NONFATAL(named_args.insert(name).second); + auto& param_type = param_names[name]; + CHECK_NONFATAL(!(param_type & POSITIONAL)); + CHECK_NONFATAL(!(param_type & NAMED_ONLY)); + param_type |= POSITIONAL; + } + if (arg.m_type == RPCArg::Type::OBJ_NAMED_PARAMS) { + for (const auto& inner : arg.m_inner) { + std::vector inner_names = SplitString(inner.m_names, '|'); + for (const std::string& inner_name : inner_names) { + auto& param_type = param_names[inner_name]; + CHECK_NONFATAL(!(param_type & POSITIONAL) || inner.m_opts.also_positional); + CHECK_NONFATAL(!(param_type & NAMED)); + CHECK_NONFATAL(!(param_type & NAMED_ONLY)); + param_type |= inner.m_opts.also_positional ? NAMED : NAMED_ONLY; + } + } } // Default value type should match argument type only when defined if (arg.m_fallback.index() == 2) { @@ -631,12 +652,17 @@ bool RPCHelpMan::IsValidNumArgs(size_t num_args) const return num_required_args <= num_args && num_args <= m_args.size(); } -std::vector RPCHelpMan::GetArgNames() const +std::vector> RPCHelpMan::GetArgNames() const { - std::vector ret; + std::vector> ret; ret.reserve(m_args.size()); for (const auto& arg : m_args) { - ret.emplace_back(arg.m_names); + if (arg.m_type == RPCArg::Type::OBJ_NAMED_PARAMS) { + for (const auto& inner : arg.m_inner) { + ret.emplace_back(inner.m_names, /*named_only=*/true); + } + } + ret.emplace_back(arg.m_names, /*named_only=*/false); } return ret; } @@ -668,20 +694,31 @@ std::string RPCHelpMan::ToString() const // Arguments Sections sections; + Sections named_only_sections; for (size_t i{0}; i < m_args.size(); ++i) { const auto& arg = m_args.at(i); if (arg.m_opts.hidden) break; // Any arg that follows is also hidden - if (i == 0) ret += "\nArguments:\n"; - // Push named argument name and description sections.m_sections.emplace_back(::ToString(i + 1) + ". " + arg.GetFirstName(), arg.ToDescriptionString(/*is_named_arg=*/true)); sections.m_max_pad = std::max(sections.m_max_pad, sections.m_sections.back().m_left.size()); // Recursively push nested args sections.Push(arg); + + // Push named-only argument sections + if (arg.m_type == RPCArg::Type::OBJ_NAMED_PARAMS) { + for (const auto& arg_inner : arg.m_inner) { + named_only_sections.PushSection({arg_inner.GetFirstName(), arg_inner.ToDescriptionString(/*is_named_arg=*/true)}); + named_only_sections.Push(arg_inner); + } + } } + + if (!sections.m_sections.empty()) ret += "\nArguments:\n"; ret += sections.ToString(); + if (!named_only_sections.m_sections.empty()) ret += "\nNamed Arguments:\n"; + ret += named_only_sections.ToString(); // Result ret += m_results.ToDescriptionString(); @@ -695,17 +732,30 @@ std::string RPCHelpMan::ToString() const UniValue RPCHelpMan::GetArgMap() const { UniValue arr{UniValue::VARR}; + + auto push_back_arg_info = [&arr](const std::string& rpc_name, int pos, const std::string& arg_name, const RPCArg::Type& type) { + UniValue map{UniValue::VARR}; + map.push_back(rpc_name); + map.push_back(pos); + map.push_back(arg_name); + map.push_back(type == RPCArg::Type::STR || + type == RPCArg::Type::STR_HEX); + arr.push_back(map); + }; + for (int i{0}; i < int(m_args.size()); ++i) { const auto& arg = m_args.at(i); std::vector arg_names = SplitString(arg.m_names, '|'); for (const auto& arg_name : arg_names) { - UniValue map{UniValue::VARR}; - map.push_back(m_name); - map.push_back(i); - map.push_back(arg_name); - map.push_back(arg.m_type == RPCArg::Type::STR || - arg.m_type == RPCArg::Type::STR_HEX); - arr.push_back(map); + push_back_arg_info(m_name, i, arg_name, arg.m_type); + if (arg.m_type == RPCArg::Type::OBJ_NAMED_PARAMS) { + for (const auto& inner : arg.m_inner) { + std::vector inner_names = SplitString(inner.m_names, '|'); + for (const std::string& inner_name : inner_names) { + push_back_arg_info(m_name, i, inner_name, inner.m_type); + } + } + } } } return arr; @@ -734,6 +784,7 @@ static std::optional ExpectedType(RPCArg::Type type) return UniValue::VBOOL; } case Type::OBJ: + case Type::OBJ_NAMED_PARAMS: case Type::OBJ_USER_KEYS: { return UniValue::VOBJ; } @@ -807,6 +858,7 @@ std::string RPCArg::ToDescriptionString(bool is_named_arg) const break; } case Type::OBJ: + case Type::OBJ_NAMED_PARAMS: case Type::OBJ_USER_KEYS: { ret += "json object"; break; @@ -835,6 +887,7 @@ std::string RPCArg::ToDescriptionString(bool is_named_arg) const } // no default case, so the compiler can warn about missing cases } ret += ")"; + if (m_type == Type::OBJ_NAMED_PARAMS) ret += " Options object that can be used to pass named arguments, listed below."; ret += m_description.empty() ? "" : " " + m_description; return ret; } @@ -1081,6 +1134,7 @@ std::string RPCArg::ToStringObj(const bool oneline) const } return res + "...]"; case Type::OBJ: + case Type::OBJ_NAMED_PARAMS: case Type::OBJ_USER_KEYS: // Currently unused, so avoid writing dead code NONFATAL_UNREACHABLE(); @@ -1113,6 +1167,7 @@ std::string RPCArg::ToString(const bool oneline) const return GetFirstName(); } case Type::OBJ: + case Type::OBJ_NAMED_PARAMS: case Type::OBJ_USER_KEYS: { const std::string res = Join(m_inner, ",", [&](const RPCArg& i) { return i.ToStringObj(oneline); }); if (m_type == Type::OBJ) { diff --git a/src/rpc/util.h b/src/rpc/util.h index e97f935fa9bb..34629ca5d608 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -151,6 +151,15 @@ struct RPCArgOptions { std::string oneline_description{}; //!< Should be empty unless it is supposed to override the auto-generated summary line std::vector type_str{}; //!< Should be empty unless it is supposed to override the auto-generated type strings. Vector length is either 0 or 2, m_opts.type_str.at(0) will override the type of the value in a key-value pair, m_opts.type_str.at(1) will override the type in the argument description. bool hidden{false}; //!< For testing only + bool also_positional{false}; //!< If set allows a named-parameter field in an OBJ_NAMED_PARAM options object + //!< to have the same name as a top-level parameter. By default the RPC + //!< framework disallows this, because if an RPC request passes the value by + //!< name, it is assigned to top-level parameter position, not to the options + //!< position, defeating the purpose of using OBJ_NAMED_PARAMS instead OBJ for + //!< that option. But sometimes it makes sense to allow less-commonly used + //!< options to be passed by name only, and more commonly used options to be + //!< passed by name or position, so the RPC framework allows this as long as + //!< methods set the also_positional flag and read values from both positions. }; struct RPCArg { @@ -160,6 +169,13 @@ struct RPCArg { STR, NUM, BOOL, + OBJ_NAMED_PARAMS, //!< Special type that behaves almost exactly like + //!< OBJ, defining an options object with a list of + //!< pre-defined keys. The only difference between OBJ + //!< and OBJ_NAMED_PARAMS is that OBJ_NAMED_PARMS + //!< also allows the keys to be passed as top-level + //!< named parameters, as a more convenient way to pass + //!< options to the RPC method without nesting them. OBJ_USER_KEYS, //!< Special type where the user must set the keys e.g. to define multiple addresses; as opposed to e.g. an options object where the keys are predefined AMOUNT, //!< Special type representing a floating point amount (can be either NUM or STR) STR_HEX, //!< Special type that is a STR with only hex chars @@ -204,7 +220,7 @@ struct RPCArg { m_description{std::move(description)}, m_opts{std::move(opts)} { - CHECK_NONFATAL(type != Type::ARR && type != Type::OBJ && type != Type::OBJ_USER_KEYS); + CHECK_NONFATAL(type != Type::ARR && type != Type::OBJ && type != Type::OBJ_NAMED_PARAMS && type != Type::OBJ_USER_KEYS); } RPCArg( @@ -221,7 +237,7 @@ struct RPCArg { m_description{std::move(description)}, m_opts{std::move(opts)} { - CHECK_NONFATAL(type == Type::ARR || type == Type::OBJ || type == Type::OBJ_USER_KEYS); + CHECK_NONFATAL(type == Type::ARR || type == Type::OBJ || type == Type::OBJ_NAMED_PARAMS || type == Type::OBJ_USER_KEYS); } bool IsOptional() const; @@ -428,8 +444,8 @@ class RPCHelpMan UniValue GetArgMap() const; /** If the supplied number of args is neither too small nor too high */ bool IsValidNumArgs(size_t num_args) const; - - std::vector GetArgNames() const; + //! Return list of arguments and whether they are named-only. + std::vector> GetArgNames() const; const std::string m_name; diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index 0501a4f264fb..835bd1204caf 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -44,11 +44,11 @@ class RPCTestingSetup : public TestingSetup RPCTestingSetup() : TestingSetup(CBaseChainParams::MAIN, {"-deprecatedrpc=legacy_mn"}) {} - UniValue TransformParams(const UniValue& params, std::vector arg_names); + UniValue TransformParams(const UniValue& params, std::vector> arg_names); UniValue CallRPC(std::string args); }; -UniValue RPCTestingSetup::TransformParams(const UniValue& params, std::vector arg_names) +UniValue RPCTestingSetup::TransformParams(const UniValue& params, std::vector> arg_names) { UniValue transformed_params; CRPCTable table; @@ -89,7 +89,7 @@ BOOST_FIXTURE_TEST_SUITE(rpc_tests, RPCTestingSetup) BOOST_AUTO_TEST_CASE(rpc_namedparams) { - const std::vector arg_names{"arg1", "arg2", "arg3", "arg4", "arg5"}; + const std::vector> arg_names{{"arg1", false}, {"arg2", false}, {"arg3", false}, {"arg4", false}, {"arg5", false}}; // Make sure named arguments are transformed into positional arguments in correct places separated by nulls BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"arg2": 2, "arg4": 4})"), arg_names).write(), "[null,2,null,4]"); @@ -114,6 +114,28 @@ BOOST_AUTO_TEST_CASE(rpc_namedparams) BOOST_CHECK_EQUAL(TransformParams(JSON(R"([1,2,3,4,5,6,7,8,9,10])"), arg_names).write(), "[1,2,3,4,5,6,7,8,9,10]"); } +BOOST_AUTO_TEST_CASE(rpc_namedonlyparams) +{ + const std::vector> arg_names{{"arg1", false}, {"arg2", false}, {"opt1", true}, {"opt2", true}, {"options", false}}; + + // Make sure optional parameters are really optional. + BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"arg1": 1, "arg2": 2})"), arg_names).write(), "[1,2]"); + + // Make sure named-only parameters are passed as options. + BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"arg1": 1, "arg2": 2, "opt1": 10, "opt2": 20})"), arg_names).write(), R"([1,2,{"opt1":10,"opt2":20}])"); + + // Make sure options can be passed directly. + BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"arg1": 1, "arg2": 2, "options":{"opt1": 10, "opt2": 20}})"), arg_names).write(), R"([1,2,{"opt1":10,"opt2":20}])"); + + // Make sure options and named parameters conflict. + BOOST_CHECK_EXCEPTION(TransformParams(JSON(R"({"arg1": 1, "arg2": 2, "opt1": 10, "options":{"opt1": 10}})"), arg_names), UniValue, + HasJSON(R"({"code":-8,"message":"Parameter options conflicts with parameter opt1"})")); + + // Make sure options object specified through args array conflicts. + BOOST_CHECK_EXCEPTION(TransformParams(JSON(R"({"args": [1, 2, {"opt1": 10}], "opt2": 20})"), arg_names), UniValue, + HasJSON(R"({"code":-8,"message":"Parameter options specified twice both as positional and named argument"})")); +} + BOOST_AUTO_TEST_CASE(rpc_rawparams) { // Test raw transaction API argument handling @@ -487,6 +509,53 @@ BOOST_AUTO_TEST_CASE(rpc_getblockstats_calculate_percentiles_by_size) } } +// Make sure errors are triggered appropriately if parameters have the same names. +BOOST_AUTO_TEST_CASE(check_dup_param_names) +{ + enum ParamType { POSITIONAL, NAMED, NAMED_ONLY }; + auto make_rpc = [](std::vector> param_names) { + std::vector params; + std::vector options; + auto push_options = [&] { if (!options.empty()) params.emplace_back(RPCArg{strprintf("options%i", params.size()), RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "", std::move(options)}); }; + for (auto& [param_name, param_type] : param_names) { + if (param_type == POSITIONAL) { + push_options(); + params.emplace_back(std::move(param_name), RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "description"); + } else { + options.emplace_back(std::move(param_name), RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "description", RPCArgOptions{.also_positional = param_type == NAMED}); + } + } + push_options(); + return RPCHelpMan{"method_name", "description", params, RPCResults{}, RPCExamples{""}}; + }; + + // No errors if parameter names are unique. + make_rpc({{"p1", POSITIONAL}, {"p2", POSITIONAL}}); + make_rpc({{"p1", POSITIONAL}, {"p2", NAMED}}); + make_rpc({{"p1", POSITIONAL}, {"p2", NAMED_ONLY}}); + make_rpc({{"p1", NAMED}, {"p2", POSITIONAL}}); + make_rpc({{"p1", NAMED}, {"p2", NAMED}}); + make_rpc({{"p1", NAMED}, {"p2", NAMED_ONLY}}); + make_rpc({{"p1", NAMED_ONLY}, {"p2", POSITIONAL}}); + make_rpc({{"p1", NAMED_ONLY}, {"p2", NAMED}}); + make_rpc({{"p1", NAMED_ONLY}, {"p2", NAMED_ONLY}}); + + // Error if parameters names are duplicates, unless one parameter is + // positional and the other is named and .also_positional is true. + BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", POSITIONAL}}), NonFatalCheckError); + make_rpc({{"p1", POSITIONAL}, {"p1", NAMED}}); + BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", NAMED_ONLY}}), NonFatalCheckError); + make_rpc({{"p1", NAMED}, {"p1", POSITIONAL}}); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED}}), NonFatalCheckError); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED_ONLY}}), NonFatalCheckError); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", POSITIONAL}}), NonFatalCheckError); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED}}), NonFatalCheckError); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED_ONLY}}), NonFatalCheckError); + + // Make sure duplicate aliases are detected too. + BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p2|p1", NAMED_ONLY}}), NonFatalCheckError); +} + BOOST_AUTO_TEST_CASE(help_example) { // test different argument types diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 9e72c0dc3309..acd32c39f8df 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -1526,7 +1526,7 @@ RPCHelpMan importmulti() }, }, RPCArgOptions{.oneline_description="requests"}}, - {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "", + {"options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "", { {"rescan", RPCArg::Type::BOOL, RPCArg::Default{true}, "Scan the chain and mempool for wallet transactions after all imports."}, }, diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index cc03c59c79d3..3c09bab10cb1 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -560,7 +560,7 @@ RPCHelpMan listunspent() }, {"include_unsafe", RPCArg::Type::BOOL, RPCArg::Default{true}, "Include outputs that are not safe to spend\n" "See description of \"safe\" attribute below."}, - {"query_options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "JSON with query options", + {"query_options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "", { {"minimumAmount", RPCArg::Type::AMOUNT, RPCArg::Default{0}, "Minimum value of each UTXO in " + CURRENCY_UNIT + ""}, {"maximumAmount", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"unlimited"}, "Maximum value of each UTXO in " + CURRENCY_UNIT + ""}, diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index a39fa39e06ae..fa21a43c9660 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -477,9 +477,9 @@ RPCHelpMan settxfee() static std::vector FundTxDoc(bool solving_data = true) { std::vector args = { - {"conf_target", RPCArg::Type::NUM, RPCArg::DefaultHint{"wallet -txconfirmtarget"}, "Confirmation target in blocks"}, + {"conf_target", RPCArg::Type::NUM, RPCArg::DefaultHint{"wallet -txconfirmtarget"}, "Confirmation target in blocks", RPCArgOptions{.also_positional = true}}, {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, "The fee estimate mode, must be one of (case insensitive):\n" - "\"" + FeeModes("\"\n\"") + "\""}, + "\"" + FeeModes("\"\n\"") + "\"", RPCArgOptions{.also_positional = true}}, }; if (solving_data) { args.push_back({"solving_data", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "Keys and scripts needed for producing a final transaction with a dummy signature.\n" @@ -767,7 +767,7 @@ RPCHelpMan fundrawtransaction() "Only pay-to-pubkey, multisig, and P2SH versions thereof are currently supported for watch-only\n", { {"hexstring", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The hex string of the raw transaction"}, - {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "for backward compatibility: passing in a true instead of an object will result in {\"includeWatching\":true}", + {"options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "For backward compatibility: passing in a true instead of an object will result in {\"includeWatching\":true}", Cat>( { {"add_inputs", RPCArg::Type::BOOL, RPCArg::Default{true}, "For a transaction with existing inputs, automatically include more if they are not enough."}, @@ -985,7 +985,7 @@ RPCHelpMan send() {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, "The fee estimate mode, must be one of (case insensitive):\n" "\"" + FeeModes("\"\n\"") + "\""}, {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/B."}, - {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "", + {"options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "", Cat>( { {"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"}, @@ -997,7 +997,7 @@ RPCHelpMan send() {"add_to_wallet", RPCArg::Type::BOOL, RPCArg::Default{true}, "When false, returns a serialized transaction which will not be added to the wallet or broadcast"}, {"change_address", RPCArg::Type::STR, RPCArg::DefaultHint{"automatic"}, "The Dash address to receive the change"}, {"change_position", RPCArg::Type::NUM, RPCArg::DefaultHint{"random"}, "The index of the change output"}, - {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/B."}, + {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/B.", RPCArgOptions{.also_positional = true}}, {"include_watching", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Also select inputs which are watch only.\n" "Only solvable inputs can be used. Watch-only destinations are solvable if the public key and/or output script was imported,\n" "e.g. with 'importpubkey' or 'importmulti' with the 'pubkeys' or 'desc' field."}, @@ -1098,11 +1098,11 @@ RPCHelpMan sendall() "\"" + FeeModes("\"\n\"") + "\""}, {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/B."}, { - "options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "", + "options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "", Cat>( { {"add_to_wallet", RPCArg::Type::BOOL, RPCArg::Default{true}, "When false, returns the serialized transaction without broadcasting or adding it to the wallet"}, - {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/B."}, + {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/B.", RPCArgOptions{.also_positional = true}}, {"include_watching", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Also select inputs which are watch-only.\n" "Only solvable inputs can be used. Watch-only destinations are solvable if the public key and/or output script was imported,\n" "e.g. with 'importpubkey' or 'importmulti' with the 'pubkeys' or 'desc' field."}, @@ -1447,7 +1447,7 @@ RPCHelpMan walletcreatefundedpsbt() OutputsDoc(), RPCArgOptions{.skip_type_check = true}}, {"locktime", RPCArg::Type::NUM, RPCArg::Default{0}, "Raw locktime. Non-0 value also locktime-activates inputs"}, - {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "", + {"options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "", Cat>( { {"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"}, diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index e0a85beb6d8a..52f527f24a9e 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -1028,7 +1028,7 @@ RPCHelpMan simulaterawtransaction() {"rawtx", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, ""}, }, }, - {"options", RPCArg::Type::OBJ_USER_KEYS, RPCArg::Optional::OMITTED, "Options", + {"options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "", { {"include_watchonly", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Whether to include watch-only addresses (see RPC importaddress)"}, }, diff --git a/test/functional/rpc_help.py b/test/functional/rpc_help.py index 8f76302717eb..714115c640e0 100755 --- a/test/functional/rpc_help.py +++ b/test/functional/rpc_help.py @@ -87,8 +87,8 @@ def test_client_conversion_table(self): for argname, convert in converts_by_argname.items(): if all(convert) != any(convert): - # Only allow dummy to fail consistency check - assert argname == 'dummy', ('WARNING: conversion mismatch for argument named %s (%s)' % (argname, list(zip(all_methods_by_argname[argname], converts_by_argname[argname])))) + # Only allow dummy and psbt to fail consistency check + assert argname in ['dummy', "psbt"], ('WARNING: conversion mismatch for argument named %s (%s)' % (argname, list(zip(all_methods_by_argname[argname], converts_by_argname[argname])))) def test_categories(self): node = self.nodes[0] diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 1ad6ede8b65e..58490bf8724b 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -329,7 +329,7 @@ def run_test(self): self.log.info("Test various PSBT operations") # partially sign multisig things with node 1 - psbtx = wmulti.walletcreatefundedpsbt(inputs=[{"txid":txid,"vout":p2sh_pos}], outputs={self.nodes[1].getnewaddress():9.99}, options={'changeAddress': self.nodes[1].getrawchangeaddress()})['psbt'] + psbtx = wmulti.walletcreatefundedpsbt(inputs=[{"txid":txid,"vout":p2sh_pos}], outputs={self.nodes[1].getnewaddress():9.99}, changeAddress=self.nodes[1].getrawchangeaddress())['psbt'] walletprocesspsbt_out = self.nodes[1].walletprocesspsbt(psbtx) psbtx = walletprocesspsbt_out['psbt'] assert_equal(walletprocesspsbt_out['complete'], False) @@ -665,7 +665,7 @@ def test_psbt_input_keys(psbt_input, keys): psbt = wallet.walletcreatefundedpsbt( inputs=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": input_weight}], outputs={self.nodes[0].getnewaddress(): 15}, - options={"add_inputs": True} + add_inputs=True, ) signed = wallet.walletprocesspsbt(psbt["psbt"]) signed = self.nodes[0].walletprocesspsbt(signed["psbt"]) @@ -675,21 +675,21 @@ def test_psbt_input_keys(psbt_input, keys): psbt2 = wallet.walletcreatefundedpsbt( inputs=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": low_input_weight}], outputs={self.nodes[0].getnewaddress(): 15}, - options={"add_inputs": True} + add_inputs=True, ) assert_greater_than(psbt["fee"], psbt2["fee"]) # Increasing the weight should have a higher fee psbt2 = wallet.walletcreatefundedpsbt( inputs=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": high_input_weight}], outputs={self.nodes[0].getnewaddress(): 15}, - options={"add_inputs": True} + add_inputs=True, ) assert_greater_than(psbt2["fee"], psbt["fee"]) # The provided weight should override the calculated weight when solving data is provided psbt3 = wallet.walletcreatefundedpsbt( inputs=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": high_input_weight}], outputs={self.nodes[0].getnewaddress(): 15}, - options={'add_inputs': True, "solving_data":{"descriptors": [desc]}} + add_inputs=True, solving_data={"descriptors": [desc]}, ) assert_equal(psbt2["fee"], psbt3["fee"]) @@ -703,7 +703,7 @@ def test_psbt_input_keys(psbt_input, keys): psbt3 = wallet.walletcreatefundedpsbt( inputs=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": high_input_weight}], outputs={self.nodes[0].getnewaddress(): 15}, - options={"add_inputs": True} + add_inputs=True, ) assert_equal(psbt2["fee"], psbt3["fee"]) diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py index f6b5eaf948e3..4530ac92d051 100755 --- a/test/functional/wallet_fundrawtransaction.py +++ b/test/functional/wallet_fundrawtransaction.py @@ -146,7 +146,7 @@ def test_change_position(self): """Ensure setting changePosition in fundraw with an exact match is handled properly.""" self.log.info("Test fundrawtxn changePosition option") rawmatch = self.nodes[2].createrawtransaction([], {self.nodes[2].getnewaddress():500}) - rawmatch = self.nodes[2].fundrawtransaction(rawmatch, {"changePosition":1, "subtractFeeFromOutputs":[0]}) + rawmatch = self.nodes[2].fundrawtransaction(rawmatch, changePosition=1, subtractFeeFromOutputs=[0]) assert_equal(rawmatch["changepos"], -1) self.nodes[3].createwallet(wallet_name="wwatch", disable_private_keys=True) @@ -257,10 +257,10 @@ def test_invalid_option(self): dec_tx = self.nodes[2].decoderawtransaction(rawtx) assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) - assert_raises_rpc_error(-3, "Unexpected key foo", self.nodes[2].fundrawtransaction, rawtx, {'foo':'bar'}) + assert_raises_rpc_error(-8, "Unknown named parameter foo", self.nodes[2].fundrawtransaction, rawtx, foo='bar') # reserveChangeKey was deprecated and is now removed - assert_raises_rpc_error(-3, "Unexpected key reserveChangeKey", lambda: self.nodes[2].fundrawtransaction(hexstring=rawtx, options={'reserveChangeKey': True})) + assert_raises_rpc_error(-8, "Unknown named parameter reserveChangeKey", lambda: self.nodes[2].fundrawtransaction(hexstring=rawtx, reserveChangeKey=True)) def test_invalid_change_address(self): self.log.info("Test fundrawtxn with an invalid change address") @@ -272,7 +272,7 @@ def test_invalid_change_address(self): dec_tx = self.nodes[2].decoderawtransaction(rawtx) assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) - assert_raises_rpc_error(-5, "Change address must be a valid Dash address", self.nodes[2].fundrawtransaction, rawtx, {'changeAddress':'foobar'}) + assert_raises_rpc_error(-5, "Change address must be a valid Dash address", self.nodes[2].fundrawtransaction, rawtx, changeAddress='foobar') def test_valid_change_address(self): self.log.info("Test fundrawtxn with a provided change address") @@ -285,8 +285,8 @@ def test_valid_change_address(self): assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) change = self.nodes[2].getnewaddress() - assert_raises_rpc_error(-8, "changePosition out of bounds", self.nodes[2].fundrawtransaction, rawtx, {'changeAddress':change, 'changePosition':2}) - rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {'changeAddress': change, 'changePosition': 0}) + assert_raises_rpc_error(-8, "changePosition out of bounds", self.nodes[2].fundrawtransaction, rawtx, changeAddress=change, changePosition=2) + rawtxfund = self.nodes[2].fundrawtransaction(rawtx, changeAddress=change, changePosition=0) dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) out = dec_tx['vout'][0] assert_equal(change, out['scriptPubKey']['address']) @@ -307,7 +307,7 @@ def test_coin_selection(self): assert_equal("00", dec_tx['vin'][0]['scriptSig']['hex']) # Should fail without add_inputs: - assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False}) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, self.nodes[2].fundrawtransaction, rawtx, add_inputs=False) # add_inputs is enabled by default rawtxfund = self.nodes[2].fundrawtransaction(rawtx) @@ -337,8 +337,8 @@ def test_two_vin(self): assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) # Should fail without add_inputs: - assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False}) - rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {"add_inputs": True}) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, self.nodes[2].fundrawtransaction, rawtx, add_inputs=False) + rawtxfund = self.nodes[2].fundrawtransaction(rawtx, add_inputs=True) dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) matchingOuts = 0 @@ -369,8 +369,8 @@ def test_two_vin_two_vout(self): assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) # Should fail without add_inputs: - assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False}) - rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {"add_inputs": True}) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, self.nodes[2].fundrawtransaction, rawtx, add_inputs=False) + rawtxfund = self.nodes[2].fundrawtransaction(rawtx, add_inputs=True) dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) matchingOuts = 0 @@ -534,7 +534,7 @@ def test_spend_2of2(self): oldBalance = self.nodes[1].getbalance() inputs = [] outputs = {self.nodes[1].getnewaddress():11} - funded_psbt = wmulti.walletcreatefundedpsbt(inputs=inputs, outputs=outputs, options={'changeAddress': w2.getrawchangeaddress()})['psbt'] + funded_psbt = wmulti.walletcreatefundedpsbt(inputs=inputs, outputs=outputs, changeAddress=w2.getrawchangeaddress())['psbt'] signed_psbt = w2.walletprocesspsbt(funded_psbt) self.nodes[2].sendrawtransaction(signed_psbt['hex']) @@ -736,7 +736,7 @@ def test_all_watched_funds(self): self.nodes[3].loadwallet('wwatch') wwatch = self.nodes[3].get_wallet_rpc('wwatch') w3 = self.nodes[3].get_wallet_rpc(self.default_wallet_name) - result = wwatch.fundrawtransaction(rawtx, {'includeWatching': True, 'changeAddress': w3.getrawchangeaddress(), 'subtractFeeFromOutputs': [0]}) + result = wwatch.fundrawtransaction(rawtx, includeWatching=True, changeAddress=w3.getrawchangeaddress(), subtractFeeFromOutputs=[0]) res_dec = self.nodes[0].decoderawtransaction(result["hex"]) assert_equal(len(res_dec["vin"]), 1) assert res_dec["vin"][0]["txid"] == self.watchonly_txid @@ -765,10 +765,10 @@ def test_option_feerate(self): result = node.fundrawtransaction(rawtx) # uses self.min_relay_tx_fee (set by settxfee) btc_kvb_to_sat_vb = 100000 # (1e5) - result1 = node.fundrawtransaction(rawtx, {"fee_rate": str(2 * btc_kvb_to_sat_vb * self.min_relay_tx_fee)}) - result2 = node.fundrawtransaction(rawtx, {"feeRate": 2 * self.min_relay_tx_fee}) - result3 = node.fundrawtransaction(rawtx, {"fee_rate": 10 * btc_kvb_to_sat_vb * self.min_relay_tx_fee}) - result4 = node.fundrawtransaction(rawtx, {"feeRate": str(10 * self.min_relay_tx_fee)}) + result1 = node.fundrawtransaction(rawtx, fee_rate=str(2 * btc_kvb_to_sat_vb * self.min_relay_tx_fee)) + result2 = node.fundrawtransaction(rawtx, feeRate=2 * self.min_relay_tx_fee) + result3 = node.fundrawtransaction(rawtx, fee_rate=10 * btc_kvb_to_sat_vb * self.min_relay_tx_fee) + result4 = node.fundrawtransaction(rawtx, feeRate=str(10 * self.min_relay_tx_fee)) result_fee_rate = result['fee'] * 1000 / count_bytes(result['hex']) assert_fee_amount(result1['fee'], count_bytes(result1['hex']), 2 * result_fee_rate) @@ -783,54 +783,54 @@ def test_option_feerate(self): # With no arguments passed, expect fee of 225 satoshis. assert_approx(node.fundrawtransaction(rawtx)["fee"], vexp=0.00000225, vspan=0.00000001) # Expect fee to be 10,000x higher when an explicit fee rate 10,000x greater is specified. - result = node.fundrawtransaction(rawtx, {"fee_rate": 10000}) + result = node.fundrawtransaction(rawtx, fee_rate=10000) assert_approx(result["fee"], vexp=0.0225, vspan=0.0001) self.log.info("Test fundrawtxn with invalid estimate_mode settings") for k, v in {"number": 42, "object": {"foo": "bar"}}.items(): assert_raises_rpc_error(-3, f"JSON value of type {k} for field estimate_mode is not of expected type string", - node.fundrawtransaction, rawtx, {"estimate_mode": v, "conf_target": 0.1, "add_inputs": True}) + node.fundrawtransaction, rawtx, estimate_mode=v, conf_target=0.1, add_inputs=True) for mode in ["", "foo", Decimal("3.141592")]: assert_raises_rpc_error(-8, 'Invalid estimate_mode parameter, must be one of: "unset", "economical", "conservative"', - node.fundrawtransaction, rawtx, {"estimate_mode": mode, "conf_target": 0.1, "add_inputs": True}) + node.fundrawtransaction, rawtx, estimate_mode=mode, conf_target=0.1, add_inputs=True) self.log.info("Test fundrawtxn with invalid conf_target settings") for mode in ["unset", "economical", "conservative"]: self.log.debug("{}".format(mode)) for k, v in {"string": "", "object": {"foo": "bar"}}.items(): assert_raises_rpc_error(-3, f"JSON value of type {k} for field conf_target is not of expected type number", - node.fundrawtransaction, rawtx, {"estimate_mode": mode, "conf_target": v, "add_inputs": True}) + node.fundrawtransaction, rawtx, estimate_mode=mode, conf_target=v, add_inputs=True) for n in [-1, 0, 1009]: assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", # max value of 1008 per src/policy/fees.h - node.fundrawtransaction, rawtx, {"estimate_mode": mode, "conf_target": n, "add_inputs": True}) + node.fundrawtransaction, rawtx, estimate_mode=mode, conf_target=n, add_inputs=True) self.log.info("Test invalid fee rate settings") for param, value in {("fee_rate", 100000), ("feeRate", 1.000)}: assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", - node.fundrawtransaction, rawtx, {param: value, "add_inputs": True}) + node.fundrawtransaction, rawtx, add_inputs=True, **{param: value}) assert_raises_rpc_error(-3, "Amount out of range", - node.fundrawtransaction, rawtx, {param: -1, "add_inputs": True}) + node.fundrawtransaction, rawtx, add_inputs=True, **{param: -1}) assert_raises_rpc_error(-3, "Amount is not a number or string", - node.fundrawtransaction, rawtx, {param: {"foo": "bar"}, "add_inputs": True}) + node.fundrawtransaction, rawtx, add_inputs=True, **{param: {"foo": "bar"}}) # Test fee rate values that don't pass fixed-point parsing checks. for invalid_value in ["", 0.000000001, 1e-09, 1.111111111, 1111111111111111, "31.999999999999999999999"]: - assert_raises_rpc_error(-3, "Invalid amount", node.fundrawtransaction, rawtx, {param: invalid_value, "add_inputs": True}) + assert_raises_rpc_error(-3, "Invalid amount", node.fundrawtransaction, rawtx, add_inputs=True, **{param: invalid_value}) # Test fee_rate values that cannot be represented in sat/vB. for invalid_value in [0.0001, 0.00000001, 0.00099999, 31.99999999]: assert_raises_rpc_error(-3, "Invalid amount", - node.fundrawtransaction, rawtx, {"fee_rate": invalid_value, "add_inputs": True}) + node.fundrawtransaction, rawtx, fee_rate=invalid_value, add_inputs=True) self.log.info("Test min fee rate checks are bypassed with fundrawtxn, e.g. a fee_rate under 1 sat/vB is allowed") - node.fundrawtransaction(rawtx, {"fee_rate": 0.999, "add_inputs": True}) - node.fundrawtransaction(rawtx, {"feeRate": 0.00000999, "add_inputs": True}) + node.fundrawtransaction(rawtx, fee_rate=0.999, add_inputs=True) + node.fundrawtransaction(rawtx, feeRate=0.00000999, add_inputs=True) self.log.info("- raises RPC error if both feeRate and fee_rate are passed") assert_raises_rpc_error(-8, "Cannot specify both fee_rate (duff/B) and feeRate (DASH/kB)", - node.fundrawtransaction, rawtx, {"fee_rate": 0.1, "feeRate": 0.1, "add_inputs": True}) + node.fundrawtransaction, rawtx, fee_rate=0.1, feeRate=0.1, add_inputs=True) self.log.info("- raises RPC error if both feeRate and estimate_mode passed") assert_raises_rpc_error(-8, "Cannot specify both estimate_mode and feeRate", - node.fundrawtransaction, rawtx, {"estimate_mode": "economical", "feeRate": 0.1, "add_inputs": True}) + node.fundrawtransaction, rawtx, estimate_mode="economical", feeRate=0.1, add_inputs=True) for param in ["feeRate", "fee_rate"]: self.log.info("- raises RPC error if both {} and conf_target are passed".format(param)) @@ -840,7 +840,7 @@ def test_option_feerate(self): self.log.info("- raises RPC error if both fee_rate and estimate_mode are passed") assert_raises_rpc_error(-8, "Cannot specify both estimate_mode and fee_rate", - node.fundrawtransaction, rawtx, {"fee_rate": 1, "estimate_mode": "economical", "add_inputs": True}) + node.fundrawtransaction, rawtx, fee_rate=1, estimate_mode="economical", add_inputs=True) def test_address_reuse(self): """Test no address reuse occurs.""" @@ -870,10 +870,10 @@ def test_option_subtract_fee_from_outputs(self): # Test subtract fee from outputs with feeRate (BTC/kvB) result = [self.nodes[3].fundrawtransaction(rawtx), # uses self.min_relay_tx_fee (set by settxfee) - self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": []}), # empty subtraction list - self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": [0]}), # uses self.min_relay_tx_fee (set by settxfee) - self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 2 * self.min_relay_tx_fee}), - self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 2 * self.min_relay_tx_fee, "subtractFeeFromOutputs": [0]}),] + self.nodes[3].fundrawtransaction(rawtx, subtractFeeFromOutputs=[]), # empty subtraction list + self.nodes[3].fundrawtransaction(rawtx, subtractFeeFromOutputs=[0]), # uses self.min_relay_tx_fee (set by settxfee) + self.nodes[3].fundrawtransaction(rawtx, feeRate=2 * self.min_relay_tx_fee), + self.nodes[3].fundrawtransaction(rawtx, feeRate=2 * self.min_relay_tx_fee, subtractFeeFromOutputs=[0]),] dec_tx = [self.nodes[3].decoderawtransaction(tx_['hex']) for tx_ in result] output = [d['vout'][1 - r['changepos']]['value'] for d, r in zip(dec_tx, result)] change = [d['vout'][r['changepos']]['value'] for d, r in zip(dec_tx, result)] @@ -890,10 +890,10 @@ def test_option_subtract_fee_from_outputs(self): # Test subtract fee from outputs with fee_rate (sat/vB) btc_kvb_to_sat_vb = 100000 # (1e5) result = [self.nodes[3].fundrawtransaction(rawtx), # uses self.min_relay_tx_fee (set by settxfee) - self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": []}), # empty subtraction list - self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": [0]}), # uses self.min_relay_tx_fee (set by settxfee) - self.nodes[3].fundrawtransaction(rawtx, {"fee_rate": 2 * btc_kvb_to_sat_vb * self.min_relay_tx_fee}), - self.nodes[3].fundrawtransaction(rawtx, {"fee_rate": 2 * btc_kvb_to_sat_vb * self.min_relay_tx_fee, "subtractFeeFromOutputs": [0]}),] + self.nodes[3].fundrawtransaction(rawtx, subtractFeeFromOutputs=[]), # empty subtraction list + self.nodes[3].fundrawtransaction(rawtx, subtractFeeFromOutputs=[0]), # uses self.min_relay_tx_fee (set by settxfee) + self.nodes[3].fundrawtransaction(rawtx, fee_rate=2 * btc_kvb_to_sat_vb * self.min_relay_tx_fee), + self.nodes[3].fundrawtransaction(rawtx, fee_rate=2 * btc_kvb_to_sat_vb * self.min_relay_tx_fee, subtractFeeFromOutputs=[0]),] dec_tx = [self.nodes[3].decoderawtransaction(tx_['hex']) for tx_ in result] output = [d['vout'][1 - r['changepos']]['value'] for d, r in zip(dec_tx, result)] change = [d['vout'][r['changepos']]['value'] for d, r in zip(dec_tx, result)] @@ -914,7 +914,7 @@ def test_option_subtract_fee_from_outputs(self): # Add changePosition=4 to circumvent BIP69 input/output sorting result = [self.nodes[3].fundrawtransaction(rawtx, {"changePosition": 4}), # Split the fee between outputs 0, 2, and 3, but not output 1. - self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": [0, 2, 3], "changePosition": 4})] + self.nodes[3].fundrawtransaction(rawtx, subtractFeeFromOutputs=[0, 2, 3], changePosition=4)] dec_tx = [self.nodes[3].decoderawtransaction(result[0]['hex']), self.nodes[3].decoderawtransaction(result[1]['hex'])] @@ -956,7 +956,7 @@ def test_subtract_fee_with_presets(self): vout = find_vout_for_address(self.nodes[0], txid, addr) rawtx = self.nodes[0].createrawtransaction([{'txid': txid, 'vout': vout}], [{self.nodes[0].getnewaddress(): 5}]) - fundedtx = self.nodes[0].fundrawtransaction(rawtx, {'subtractFeeFromOutputs': [0]}) + fundedtx = self.nodes[0].fundrawtransaction(rawtx, subtractFeeFromOutputs=[0]) signedtx = self.nodes[0].signrawtransactionwithwallet(fundedtx['hex']) self.nodes[0].sendrawtransaction(signedtx['hex']) @@ -990,25 +990,25 @@ def test_external_inputs(self): assert_raises_rpc_error(-4, "Not solvable pre-selected input COutPoint(%s, %s)" % (ext_utxo["txid"], ext_utxo["vout"]), wallet.fundrawtransaction, raw_tx) # Error conditions - assert_raises_rpc_error(-5, 'Pubkey "not a pubkey" must be a hex string', wallet.fundrawtransaction, raw_tx, {"solving_data": {"pubkeys":["not a pubkey"]}}) - assert_raises_rpc_error(-5, 'Pubkey "01234567890a0b0c0d0e0f" must have a length of either 33 or 65 bytes', wallet.fundrawtransaction, raw_tx, {"solving_data": {"pubkeys":["01234567890a0b0c0d0e0f"]}}) - assert_raises_rpc_error(-5, "'not a script' is not hex", wallet.fundrawtransaction, raw_tx, {"solving_data": {"scripts":["not a script"]}}) - assert_raises_rpc_error(-8, "Unable to parse descriptor 'not a descriptor'", wallet.fundrawtransaction, raw_tx, {"solving_data": {"descriptors":["not a descriptor"]}}) - assert_raises_rpc_error(-8, "Invalid parameter, missing vout key", wallet.fundrawtransaction, raw_tx, {"input_sizes": [{"txid": ext_utxo["txid"]}]}) - assert_raises_rpc_error(-8, "Invalid parameter, vout cannot be negative", wallet.fundrawtransaction, raw_tx, {"input_sizes": [{"txid": ext_utxo["txid"], "vout": -1}]}) - assert_raises_rpc_error(-8, "Invalid parameter, missing size key", wallet.fundrawtransaction, raw_tx, {"input_sizes": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"]}]}) - assert_raises_rpc_error(-8, "Invalid parameter, size cannot be less than 41", wallet.fundrawtransaction, raw_tx, {"input_sizes": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": 40}]}) - assert_raises_rpc_error(-8, "Invalid parameter, size cannot be less than 41", wallet.fundrawtransaction, raw_tx, {"input_sizes": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": -1}]}) - assert_raises_rpc_error(-8, "Invalid parameter, size cannot be greater than", wallet.fundrawtransaction, raw_tx, {"input_sizes": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": 400001}]}) + assert_raises_rpc_error(-5, 'Pubkey "not a pubkey" must be a hex string', wallet.fundrawtransaction, raw_tx, solving_data={"pubkeys":["not a pubkey"]}) + assert_raises_rpc_error(-5, 'Pubkey "01234567890a0b0c0d0e0f" must have a length of either 33 or 65 bytes', wallet.fundrawtransaction, raw_tx, solving_data={"pubkeys":["01234567890a0b0c0d0e0f"]}) + assert_raises_rpc_error(-5, "'not a script' is not hex", wallet.fundrawtransaction, raw_tx, solving_data={"scripts":["not a script"]}) + assert_raises_rpc_error(-8, "Unable to parse descriptor 'not a descriptor'", wallet.fundrawtransaction, raw_tx, solving_data={"descriptors":["not a descriptor"]}) + assert_raises_rpc_error(-8, "Invalid parameter, missing vout key", wallet.fundrawtransaction, raw_tx, input_sizes=[{"txid": ext_utxo["txid"]}]) + assert_raises_rpc_error(-8, "Invalid parameter, vout cannot be negative", wallet.fundrawtransaction, raw_tx, input_sizes=[{"txid": ext_utxo["txid"], "vout": -1}]) + assert_raises_rpc_error(-8, "Invalid parameter, missing size key", wallet.fundrawtransaction, raw_tx, input_sizes=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"]}]) + assert_raises_rpc_error(-8, "Invalid parameter, size cannot be less than 41", wallet.fundrawtransaction, raw_tx, input_sizes=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": 40}]) + assert_raises_rpc_error(-8, "Invalid parameter, size cannot be less than 41", wallet.fundrawtransaction, raw_tx, input_sizes=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": -1}]) + assert_raises_rpc_error(-8, "Invalid parameter, size cannot be greater than", wallet.fundrawtransaction, raw_tx, input_sizes=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": 400001}]) # But funding should work when the solving data is provided - funded_tx = wallet.fundrawtransaction(raw_tx, {"solving_data": {"pubkeys": [addr_info['pubkey']], "scripts": [addr_info["embedded"]["scriptPubKey"]]}}) + funded_tx = wallet.fundrawtransaction(raw_tx, solving_data={"pubkeys": [addr_info['pubkey']], "scripts": [addr_info["embedded"]["scriptPubKey"]]}) signed_tx = wallet.signrawtransactionwithwallet(funded_tx['hex']) assert not signed_tx['complete'] signed_tx = self.nodes[0].signrawtransactionwithwallet(signed_tx['hex']) assert signed_tx['complete'] - funded_tx = wallet.fundrawtransaction(raw_tx, {"solving_data": {"descriptors": [desc]}}) + funded_tx = wallet.fundrawtransaction(raw_tx, solving_data={"descriptors": [desc]}) signed_tx1 = wallet.signrawtransactionwithwallet(funded_tx['hex']) assert not signed_tx1['complete'] signed_tx2 = self.nodes[0].signrawtransactionwithwallet(signed_tx1['hex']) @@ -1023,29 +1023,29 @@ def test_external_inputs(self): high_input_weight = input_weight * 2 # Funding should also work if the input weight is provided - funded_tx = wallet.fundrawtransaction(raw_tx, {"input_sizes": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": input_weight}]}) + funded_tx = wallet.fundrawtransaction(raw_tx, input_sizes=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": input_weight}]) signed_tx = wallet.signrawtransactionwithwallet(funded_tx["hex"]) signed_tx = self.nodes[0].signrawtransactionwithwallet(signed_tx["hex"]) assert_equal(self.nodes[0].testmempoolaccept([signed_tx["hex"]])[0]["allowed"], True) assert_equal(signed_tx["complete"], True) # Reducing the weight should have a lower fee - funded_tx2 = wallet.fundrawtransaction(raw_tx, {"input_sizes": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": low_input_weight}]}) + funded_tx2 = wallet.fundrawtransaction(raw_tx, input_sizes=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": low_input_weight}]) assert_greater_than(funded_tx["fee"], funded_tx2["fee"]) # Increasing the weight should have a higher fee - funded_tx2 = wallet.fundrawtransaction(raw_tx, {"input_sizes": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": high_input_weight}]}) + funded_tx2 = wallet.fundrawtransaction(raw_tx, input_sizes=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": high_input_weight}]) assert_greater_than(funded_tx2["fee"], funded_tx["fee"]) # The provided weight should override the calculated weight when solving data is provided - funded_tx3 = wallet.fundrawtransaction(raw_tx, {"solving_data": {"descriptors": [desc]}, "input_sizes": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": high_input_weight}]}) + funded_tx3 = wallet.fundrawtransaction(raw_tx, solving_data={"descriptors": [desc]}, input_sizes=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": high_input_weight}]) assert_equal(funded_tx2["fee"], funded_tx3["fee"]) # The feerate should be met - funded_tx4 = wallet.fundrawtransaction(raw_tx, {"input_sizes": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": high_input_weight}], "fee_rate": 10}) + funded_tx4 = wallet.fundrawtransaction(raw_tx, input_sizes=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": high_input_weight}], fee_rate=10) input_add_weight = high_input_weight - 41 tx4_size = wallet.decoderawtransaction(funded_tx4["hex"])["size"] + input_add_weight assert_fee_amount(funded_tx4["fee"], tx4_size, Decimal(0.0001)) # Funding with weight at csuint boundaries should not cause problems - funded_tx = wallet.fundrawtransaction(raw_tx, {"input_sizes": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": 255}]}) - funded_tx = wallet.fundrawtransaction(raw_tx, {"input_sizes": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": 65539}]}) + funded_tx = wallet.fundrawtransaction(raw_tx, input_sizes=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": 255}]) + funded_tx = wallet.fundrawtransaction(raw_tx, input_sizes=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": 65539}]) self.nodes[2].unloadwallet("extfund") @@ -1085,7 +1085,7 @@ def test_add_inputs_default_value(self): # 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}) + source_tx = self.nodes[0].send(outputs=[{addr1: 5}, {addr2: 5}], change_position=0) self.generate(self.nodes[0], 1) # Select only one input. @@ -1097,12 +1097,12 @@ def test_add_inputs_default_value(self): } ] } - assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, wallet.send, outputs=[{addr1: 8}], options=options) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, wallet.send, outputs=[{addr1: 8}], **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) + tx = wallet.send(outputs=[{addr1: 8}], **options) assert tx["complete"] # Case (4), Explicit add_inputs=true and preset inputs (with preset inputs covering the target amount) @@ -1110,7 +1110,7 @@ def test_add_inputs_default_value(self): "txid": source_tx["txid"], "vout": 2 # change position was hardcoded to index 0 }) - tx = wallet.send(outputs=[{addr1: 8}], options=options) + tx = wallet.send(outputs=[{addr1: 8}], **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'] @@ -1120,12 +1120,12 @@ def test_add_inputs_default_value(self): # 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) + tx = wallet.send(outputs=[{addr1: 8}], **options) assert tx["complete"] # 6. Explicit add_inputs=false, no preset inputs: options = {"add_inputs": False} - assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, wallet.send, outputs=[{addr1: 3}], options=options) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, wallet.send, outputs=[{addr1: 3}], **options) ################################################ @@ -1146,14 +1146,14 @@ def test_add_inputs_default_value(self): # Case (3), Explicit add_inputs=true and preset inputs (with preset inputs not-covering the target amount) options["add_inputs"] = True - assert "psbt" in wallet.walletcreatefundedpsbt(outputs=[{addr1: 8}], inputs=inputs, options=options) + assert "psbt" in wallet.walletcreatefundedpsbt(outputs=[{addr1: 8}], inputs=inputs, **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) + psbt_tx = wallet.walletcreatefundedpsbt(outputs=[{addr1: 8}], inputs=inputs, **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) @@ -1165,11 +1165,11 @@ def test_add_inputs_default_value(self): options = { "add_inputs": True } - assert "psbt" in wallet.walletcreatefundedpsbt(inputs=[], outputs=outputs, options=options) + assert "psbt" in wallet.walletcreatefundedpsbt(inputs=[], outputs=outputs, **options) # Case (6). Explicit add_inputs=false, no preset inputs: options = {"add_inputs": False} - assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, wallet.walletcreatefundedpsbt, inputs=[], outputs=outputs, options=options) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, wallet.walletcreatefundedpsbt, inputs=[], outputs=outputs, **options) self.nodes[2].unloadwallet("test_preset_inputs") @@ -1237,7 +1237,7 @@ def test_include_unsafe(self): assert_raises_rpc_error(-4, "Insufficient funds", wallet.fundrawtransaction, rawtx) # But we can opt-in to use them for funding. - fundedtx = wallet.fundrawtransaction(rawtx, {"include_unsafe": True}) + fundedtx = wallet.fundrawtransaction(rawtx, include_unsafe=True) tx_dec = wallet.decoderawtransaction(fundedtx['hex']) assert all((txin["txid"], txin["vout"]) in inputs for txin in tx_dec["vin"]) signedtx = wallet.signrawtransactionwithwallet(fundedtx['hex']) @@ -1245,7 +1245,7 @@ def test_include_unsafe(self): # And we can also use them once they're confirmed. self.generate(self.nodes[0], 1) - fundedtx = wallet.fundrawtransaction(rawtx, {"include_unsafe": False}) + fundedtx = wallet.fundrawtransaction(rawtx, include_unsafe=False) tx_dec = wallet.decoderawtransaction(fundedtx['hex']) assert all((txin["txid"], txin["vout"]) in inputs for txin in tx_dec["vin"]) signedtx = wallet.signrawtransactionwithwallet(fundedtx['hex']) @@ -1280,7 +1280,7 @@ def test_22670(self): # Create transactions in order to calculate fees for the target bounds that can trigger this bug change_tx = tester.fundrawtransaction(tester.createrawtransaction([], [{funds.getnewaddress(): 1.5}])) tx = tester.createrawtransaction([], [{funds.getnewaddress(): 2}]) - no_change_tx = tester.fundrawtransaction(tx, {"subtractFeeFromOutputs": [0]}) + no_change_tx = tester.fundrawtransaction(tx, subtractFeeFromOutputs=[0]) overhead_fees = feerate * len(tx) / 2 / 1000 cost_of_change = change_tx["fee"] - no_change_tx["fee"] @@ -1333,7 +1333,7 @@ def test_feerate_rounding(self): # To test this does not happen, we subtract 202 sats from the input value. If working correctly, this should # fail with insufficient funds rather than bitcoind asserting. rawtx = w.createrawtransaction(inputs=[], outputs=[{self.nodes[0].getnewaddress(): 1 - 0.00000202}]) - assert_raises_rpc_error(-4, "Insufficient funds", w.fundrawtransaction, rawtx, {"fee_rate": 1.85}) + assert_raises_rpc_error(-4, "Insufficient funds", w.fundrawtransaction, rawtx, fee_rate=1.85) def test_input_confs_control(self): self.nodes[0].createwallet("minconf") diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py index e32b817efe8a..fcd23630fc12 100755 --- a/test/functional/wallet_import_rescan.py +++ b/test/functional/wallet_import_rescan.py @@ -64,7 +64,7 @@ def do_import(self, timestamp): } response = self.node.importmulti( requests=[request], - options={"rescan": self.rescan in (Rescan.yes, Rescan.late_timestamp)}, + rescan=self.rescan in (Rescan.yes, Rescan.late_timestamp), ) assert_equal(response, [{"success": True}]) diff --git a/test/functional/wallet_keypool_hd.py b/test/functional/wallet_keypool_hd.py index cb1b5751ab80..926d3fcc25cf 100755 --- a/test/functional/wallet_keypool_hd.py +++ b/test/functional/wallet_keypool_hd.py @@ -199,30 +199,30 @@ def run_test(self): # Using a fee rate (10 sat / byte) well above the minimum relay rate # creating a 5,000 sat transaction with change should not be possible - assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it.", w2.walletcreatefundedpsbt, inputs=[], outputs=[{addr.pop(): 0.00005000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.000010}) + assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it.", w2.walletcreatefundedpsbt, inputs=[], outputs=[{addr.pop(): 0.00005000}], subtractFeeFromOutputs=[0], feeRate=0.000010) # creating a 10,000 sat transaction without change, with a manual input, should still be possible - res = w2.walletcreatefundedpsbt(inputs=w2.listunspent(), outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.000010}) + res = w2.walletcreatefundedpsbt(inputs=w2.listunspent(), outputs=[{destination: 0.00010000}], subtractFeeFromOutputs=[0], feeRate=0.000010) assert_equal("psbt" in res, True) # creating a 10,000 sat transaction without change should still be possible - res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.000010}) + res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], subtractFeeFromOutputs=[0], feeRate=0.000010) assert_equal("psbt" in res, True) # should work without subtractFeeFromOutputs if the exact fee is subtracted from the amount - res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00008900}], options={"feeRate": 0.000010}) + res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00008900}], feeRate=0.000010) assert_equal("psbt" in res, True) # dust change should be removed - res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00008800}], options={"feeRate": 0.000010}) + res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00008800}], feeRate=0.000010) assert_equal("psbt" in res, True) # create a transaction without change at the maximum fee rate, such that the output is still spendable: - res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.00008822}) + res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], subtractFeeFromOutputs=[0], feeRate=0.00008822) assert_equal("psbt" in res, True) assert_equal(res["fee"], Decimal("0.00001694")) # creating a 10,000 sat transaction with a manual change address should be possible - res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.000010, "changeAddress": addr.pop()}) + res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], subtractFeeFromOutputs=[0], feeRate=0.000010, changeAddress=addr.pop()) assert_equal("psbt" in res, True) if not self.options.descriptors: diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 3e11e8d27836..bdfcec0b0f07 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -352,7 +352,7 @@ def test_other_watchonly(self): imports0.importaddress(import_sent_addr) received_sent_watchonly_txid = default.sendtoaddress(import_sent_addr, 10) received_sent_watchonly_vout = find_vout_for_address(self.nodes[0], received_sent_watchonly_txid, import_sent_addr) - send = default.sendall(recipients=[default.getnewaddress()], options={"inputs": [{"txid": received_sent_watchonly_txid, "vout": received_sent_watchonly_vout}]}) + send = default.sendall(recipients=[default.getnewaddress()], inputs=[{"txid": received_sent_watchonly_txid, "vout": received_sent_watchonly_vout}]) sent_watchonly_txid = send["txid"] # Tx that has both a watchonly and spendable output diff --git a/test/functional/wallet_multisig_descriptor_psbt.py b/test/functional/wallet_multisig_descriptor_psbt.py index faff0cea1fba..2959574d5b0b 100755 --- a/test/functional/wallet_multisig_descriptor_psbt.py +++ b/test/functional/wallet_multisig_descriptor_psbt.py @@ -121,7 +121,7 @@ def run_test(self): to = participants["signers"][self.N - 1].getnewaddress() value = 1 self.log.info("First, make a sending transaction, created using `walletcreatefundedpsbt` (anyone can initiate this)...") - psbt = participants["multisigs"][0].walletcreatefundedpsbt(inputs=[], outputs={to: value}, options={"feeRate": 0.00010}) + psbt = participants["multisigs"][0].walletcreatefundedpsbt(inputs=[], outputs={to: value}, feeRate=0.00010) psbts = [] self.log.info("Now at least M users check the psbt with decodepsbt and (if OK) signs it with walletprocesspsbt...") @@ -143,7 +143,7 @@ def run_test(self): assert_equal(participants["signers"][self.N - 1].getbalance(), value) self.log.info("Send another transaction from the multisig, this time with a daisy chained signing flow (one after another in series)!") - psbt = participants["multisigs"][0].walletcreatefundedpsbt(inputs=[], outputs={to: value}, options={"feeRate": 0.00010}) + psbt = participants["multisigs"][0].walletcreatefundedpsbt(inputs=[], outputs={to: value}, feeRate=0.00010) for m in range(self.M): signers_multisig = participants["multisigs"][m] self._check_psbt(psbt["psbt"], to, value, signers_multisig) diff --git a/test/functional/wallet_resendwallettransactions.py b/test/functional/wallet_resendwallettransactions.py index b5d187076518..c12955647257 100755 --- a/test/functional/wallet_resendwallettransactions.py +++ b/test/functional/wallet_resendwallettransactions.py @@ -34,7 +34,7 @@ def run_test(self): self.log.info("Create a new transaction and wait until it's broadcast") parent_utxo, indep_utxo = node.listunspent()[:2] addr = node.getnewaddress() - txid = node.send(outputs=[{addr: 1}], options={"inputs": [parent_utxo]})["txid"] + txid = node.send(outputs=[{addr: 1}], inputs=[parent_utxo])["txid"] # Can take a few seconds due to transaction trickling def wait_p2p(): @@ -87,7 +87,7 @@ def wait_p2p(): # ordering of mapWallet is. mapWallet hashes txids with a salt drawn at random every time # the wallet is loaded, so reloading the wallet reshuffles its iteration order while # leaving the transactions themselves untouched. Reload until the child comes first. - child_txid = node.send(outputs=[{addr: 0.5}], options={"inputs": [{"txid":txid, "vout":0}]})["txid"] + child_txid = node.send(outputs=[{addr: 0.5}], inputs=[{"txid":txid, "vout":0}])["txid"] while True: txids = node.listreceivedbyaddress(minconf=0, address_filter=addr)[0]["txids"] if txids == [child_txid, txid]: @@ -111,7 +111,7 @@ def wait_p2p(): node.mockscheduler(60) # Evict these txs from the mempool - indep_send = node.send(outputs=[{node.getnewaddress(): 1}], options={"inputs": [indep_utxo]}) + indep_send = node.send(outputs=[{node.getnewaddress(): 1}], inputs=[indep_utxo]) node.getmempoolentry(indep_send["txid"]) assert_raises_rpc_error(-5, "Transaction not in mempool", node.getmempoolentry, txid) assert_raises_rpc_error(-5, "Transaction not in mempool", node.getmempoolentry, child_txid) diff --git a/test/functional/wallet_sendall.py b/test/functional/wallet_sendall.py index a57a69ba039d..bf11e6620d11 100755 --- a/test/functional/wallet_sendall.py +++ b/test/functional/wallet_sendall.py @@ -151,7 +151,7 @@ def sendall_invalid_amounts(self): self.log.info("Test sending more than balance") pre_sendall_balance = self.add_utxos([7, 14]) - expected_tx = self.wallet.sendall(recipients=[{self.recipient: 5}, self.remainder_target], options={"add_to_wallet": False}) + expected_tx = self.wallet.sendall(recipients=[{self.recipient: 5}, self.remainder_target], add_to_wallet=False) tx = self.wallet.decoderawtransaction(expected_tx['hex']) fee = 21 - sum([o["value"] for o in tx["vout"]]) @@ -192,7 +192,7 @@ def sendall_with_send_max(self): self.add_utxos([0.00000700, 0.00000600, 1]) # sendall with send_max - sendall_tx_receipt = self.wallet.sendall(recipients=[self.remainder_target], fee_rate=300, options={"send_max": True}) + sendall_tx_receipt = self.wallet.sendall(recipients=[self.remainder_target], fee_rate=300, send_max=True) tx_from_wallet = self.wallet.gettransaction(txid = sendall_tx_receipt["txid"], verbose = True) assert_equal(len(tx_from_wallet["decoded"]["vin"]), 1) @@ -208,7 +208,7 @@ def sendall_specific_inputs(self): self.add_utxos([17, 4]) utxo = self.wallet.listunspent()[0] - sendall_tx_receipt = self.wallet.sendall(recipients=[self.remainder_target], options={"inputs": [utxo]}) + sendall_tx_receipt = self.wallet.sendall(recipients=[self.remainder_target], inputs=[utxo]) tx_from_wallet = self.wallet.gettransaction(txid = sendall_tx_receipt["txid"], verbose = True) assert_equal(len(tx_from_wallet["decoded"]["vin"]), 1) assert_equal(len(tx_from_wallet["decoded"]["vout"]), 1) @@ -229,26 +229,26 @@ def sendall_fails_on_missing_input(self): # fails on out of bounds vout assert_raises_rpc_error(-8, "Input not found. UTXO ({}:{}) is not part of wallet.".format(spent_utxo["txid"], 1000), - self.wallet.sendall, recipients=[self.remainder_target], options={"inputs": [{"txid": spent_utxo["txid"], "vout": 1000}]}) + self.wallet.sendall, recipients=[self.remainder_target], inputs=[{"txid": spent_utxo["txid"], "vout": 1000}]) # fails on unconfirmed spent UTXO self.wallet.sendall(recipients=[self.remainder_target]) assert_raises_rpc_error(-8, "Input not available. UTXO ({}:{}) was already spent.".format(spent_utxo["txid"], spent_utxo["vout"]), - self.wallet.sendall, recipients=[self.remainder_target], options={"inputs": [spent_utxo]}) + self.wallet.sendall, recipients=[self.remainder_target], inputs=[spent_utxo]) # fails on specific previously spent UTXO, while other UTXOs exist self.generate(self.nodes[0], 1) self.add_utxos([19, 2]) assert_raises_rpc_error(-8, "Input not available. UTXO ({}:{}) was already spent.".format(spent_utxo["txid"], spent_utxo["vout"]), - self.wallet.sendall, recipients=[self.remainder_target], options={"inputs": [spent_utxo]}) + self.wallet.sendall, recipients=[self.remainder_target], inputs=[spent_utxo]) # fails because UTXO is unknown, while other UTXOs exist foreign_utxo = self.def_wallet.listunspent()[0] assert_raises_rpc_error(-8, "Input not found. UTXO ({}:{}) is not part of wallet.".format(foreign_utxo["txid"], foreign_utxo["vout"]), self.wallet.sendall, recipients=[self.remainder_target], - options={"inputs": [foreign_utxo]}) + inputs=[foreign_utxo]) @cleanup def sendall_fails_on_no_address(self): @@ -272,7 +272,7 @@ def sendall_fails_on_specific_inputs_with_send_max(self): "Cannot combine send_max with specific inputs.", self.wallet.sendall, recipients=[self.remainder_target], - options={"inputs": [utxo], "send_max": True}) + inputs=[utxo], send_max=True) @cleanup def sendall_fails_on_high_fee(self): @@ -310,7 +310,7 @@ def sendall_watchonly_specific_inputs(self): else: watchonly.importmulti(import_req) - sendall_tx_receipt = watchonly.sendall(recipients=[self.remainder_target], options={"inputs": [utxo]}) + sendall_tx_receipt = watchonly.sendall(recipients=[self.remainder_target], inputs=[utxo]) psbt = sendall_tx_receipt["psbt"] decoded = self.nodes[0].decodepsbt(psbt) assert_equal(len(decoded["inputs"]), 1) diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index 907127eb650c..411f65e0c945 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -197,13 +197,13 @@ def test_valid_signer(self): self.log.info('Test send using hww1') # Don't broadcast transaction yet so the RPC returns the raw hex - res = hww.send(outputs={dest:0.5},options={"add_to_wallet": False}) + res = hww.send(outputs={dest:0.5},add_to_wallet=False) assert res["complete"] assert_equal(res["hex"], mock_tx) self.log.info('Test sendall using hww1') - res = hww.sendall(recipients=[{dest:0.5}, hww.getrawchangeaddress()],options={"add_to_wallet": False}) + res = hww.sendall(recipients=[{dest:0.5}, hww.getrawchangeaddress()], add_to_wallet=False) assert res["complete"] assert_equal(res["hex"], mock_tx) diff --git a/test/functional/wallet_timelock.py b/test/functional/wallet_timelock.py index 57a7c3907ade..0a622979a49c 100755 --- a/test/functional/wallet_timelock.py +++ b/test/functional/wallet_timelock.py @@ -29,7 +29,7 @@ def run_test(self): self.log.info("Send to new address with locktime") node.send( outputs={address: 5}, - options={"locktime": mtp_tip - 1}, + locktime=mtp_tip - 1, ) self.generate(node, 1) diff --git a/test/functional/wallet_watchonly.py b/test/functional/wallet_watchonly.py index 0d01a5714d26..0ac4b234d551 100755 --- a/test/functional/wallet_watchonly.py +++ b/test/functional/wallet_watchonly.py @@ -99,13 +99,13 @@ def run_test(self): options = {'changeAddress': wo_change} no_wo_options = {'changeAddress': wo_change, 'includeWatching': False} - result = wo_wallet.walletcreatefundedpsbt(inputs=inputs, outputs=outputs, options=options) + result = wo_wallet.walletcreatefundedpsbt(inputs=inputs, outputs=outputs, **options) assert_equal("psbt" in result, True) assert_raises_rpc_error(-4, "Insufficient funds", wo_wallet.walletcreatefundedpsbt, inputs, outputs, 0, no_wo_options) self.log.info('Testing fundrawtransaction watch-only defaults') rawtx = wo_wallet.createrawtransaction(inputs=inputs, outputs=outputs) - result = wo_wallet.fundrawtransaction(hexstring=rawtx, options=options) + result = wo_wallet.fundrawtransaction(hexstring=rawtx, **options) assert_equal("hex" in result, True) assert_raises_rpc_error(-4, "Insufficient funds", wo_wallet.fundrawtransaction, rawtx, no_wo_options) From 7f10ca1d2de2ca2234e7b0b1ee3b732debaadb90 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Fri, 2 Jun 2023 16:44:51 +0100 Subject: [PATCH 05/11] Merge bitcoin/bitcoin#27790: walletdb: Add PrefixCursor ba616b932cb9e9adb7eb9f1826caa62ce422a22d wallet: Add GetPrefixCursor to DatabaseBatch (Andrew Chow) 1d858b055daeea363e0450f327672658548be4c6 walletdb: Handle when database keys are empty (Ryan Ofsky) 84b2f353bbefb9264284e7430863b2fa1d796d38 walletdb: Consistently clear key and value streams before writing (Andrew Chow) Pull request description: Split from #24914 as suggested in https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1442091917 This PR adds a wallet database cursor that gives a view over all of the records beginning with the same prefix. ACKs for top commit: ryanofsky: Code review ACK ba616b932cb9e9adb7eb9f1826caa62ce422a22d. Just suggested changes since last review furszy: ACK ba616b93 Tree-SHA512: 38a61849f108d8003d28c599b1ad0421ac9beb3afe14c02f1253e7b4efc3d4eef483e32647a820fc6636bca3f9efeff9fe062b6b602e0cded69f21f8b26af544 Co-authored-by: fanquake --- src/wallet/bdb.cpp | 28 ++++-- src/wallet/bdb.h | 7 +- src/wallet/db.h | 1 + src/wallet/salvage.cpp | 1 + src/wallet/sqlite.cpp | 50 ++++++++++- src/wallet/sqlite.h | 10 +++ src/wallet/test/db_tests.cpp | 128 +++++++++++++++++++++++++++ src/wallet/test/util.cpp | 16 +++- src/wallet/test/util.h | 22 +++-- src/wallet/test/walletload_tests.cpp | 2 +- 10 files changed, 248 insertions(+), 17 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 20bcc4f13088..ef4651152931 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -757,7 +757,8 @@ void BerkeleyDatabase::ReloadDbEnv() env->ReloadDbEnv(); } -BerkeleyCursor::BerkeleyCursor(BerkeleyDatabase& database, const BerkeleyBatch& batch) +BerkeleyCursor::BerkeleyCursor(BerkeleyDatabase& database, const BerkeleyBatch& batch, Span prefix) + : m_key_prefix(prefix.begin(), prefix.end()) { if (!database.m_db.get()) { throw std::runtime_error(STR_INTERNAL_BUG("BerkeleyDatabase does not exist")); @@ -774,19 +775,30 @@ DatabaseCursor::Status BerkeleyCursor::Next(DataStream& ssKey, DataStream& ssVal { if (m_cursor == nullptr) return Status::FAIL; // Read at cursor - SafeDbt datKey; + SafeDbt datKey(m_key_prefix.data(), m_key_prefix.size()); SafeDbt datValue; - int ret = m_cursor->get(datKey, datValue, DB_NEXT); + int ret = -1; + if (m_first && !m_key_prefix.empty()) { + ret = m_cursor->get(datKey, datValue, DB_SET_RANGE); + } else { + ret = m_cursor->get(datKey, datValue, DB_NEXT); + } + m_first = false; if (ret == DB_NOTFOUND) { return Status::DONE; } - if (ret != 0 || datKey.get_data() == nullptr || datValue.get_data() == nullptr) { + if (ret != 0) { return Status::FAIL; } + Span raw_key = SpanFromDbt(datKey); + if (!m_key_prefix.empty() && std::mismatch(raw_key.begin(), raw_key.end(), m_key_prefix.begin(), m_key_prefix.end()).second != m_key_prefix.end()) { + return Status::DONE; + } + // Convert to streams ssKey.clear(); - ssKey.write(SpanFromDbt(datKey)); + ssKey.write(raw_key); ssValue.clear(); ssValue.write(SpanFromDbt(datValue)); return Status::MORE; @@ -805,6 +817,12 @@ std::unique_ptr BerkeleyBatch::GetNewCursor() return std::make_unique(m_database, *this); } +std::unique_ptr BerkeleyBatch::GetNewPrefixCursor(Span prefix) +{ + if (!pdb) return nullptr; + return std::make_unique(m_database, *this, prefix); +} + bool BerkeleyBatch::TxnBegin() { if (!pdb || activeTxn) diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index 2d9bf4ef0e33..7d287f6f25d5 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -159,9 +159,13 @@ class BerkeleyCursor : public DatabaseCursor { private: Dbc* m_cursor; + std::vector m_key_prefix; + bool m_first{true}; public: - explicit BerkeleyCursor(BerkeleyDatabase& database, const BerkeleyBatch& batch); + // Constructor for cursor for records matching the prefix + // To match all records, an empty prefix may be provided. + explicit BerkeleyCursor(BerkeleyDatabase& database, const BerkeleyBatch& batch, Span prefix = {}); ~BerkeleyCursor() override; Status Next(DataStream& key, DataStream& value) override; @@ -197,6 +201,7 @@ class BerkeleyBatch : public DatabaseBatch void Close() override; std::unique_ptr GetNewCursor() override; + std::unique_ptr GetNewPrefixCursor(Span prefix) override; bool TxnBegin() override; bool TxnCommit() override; bool TxnAbort() override; diff --git a/src/wallet/db.h b/src/wallet/db.h index 55558ab87efe..1921b104db12 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -112,6 +112,7 @@ class DatabaseBatch virtual bool ErasePrefix(Span prefix) = 0; virtual std::unique_ptr GetNewCursor() = 0; + virtual std::unique_ptr GetNewPrefixCursor(Span prefix) = 0; virtual bool TxnBegin() = 0; virtual bool TxnCommit() = 0; virtual bool TxnAbort() = 0; diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp index 804813097a9d..3c704c27c512 100644 --- a/src/wallet/salvage.cpp +++ b/src/wallet/salvage.cpp @@ -45,6 +45,7 @@ class DummyBatch : public DatabaseBatch void Close() override {} std::unique_ptr GetNewCursor() override { return std::make_unique(); } + std::unique_ptr GetNewPrefixCursor(Span prefix) override { return GetNewCursor(); } bool TxnBegin() override { return true; } bool TxnCommit() override { return true; } bool TxnAbort() override { return true; } diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index 84ab061e4138..8d8510511f45 100644 --- a/src/wallet/sqlite.cpp +++ b/src/wallet/sqlite.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -64,7 +65,11 @@ static bool BindBlobToStatement(sqlite3_stmt* stmt, Span blob, const std::string& description) { - int res = sqlite3_bind_blob(stmt, index, blob.data(), blob.size(), SQLITE_STATIC); + // Pass a pointer to the empty string "" below instead of passing the + // blob.data() pointer if the blob.data() pointer is null. Passing a null + // data pointer to bind_blob would cause sqlite to bind the SQL NULL value + // instead of the empty blob value X'', which would mess up SQL comparisons. + int res = sqlite3_bind_blob(stmt, index, blob.data() ? static_cast(blob.data()) : "", blob.size(), SQLITE_STATIC); if (res != SQLITE_OK) { LogPrintf("Unable to bind %s to statement: %s\n", description, sqlite3_errstr(res)); sqlite3_clear_bindings(stmt); @@ -540,6 +545,7 @@ DatabaseCursor::Status SQLiteCursor::Next(DataStream& key, DataStream& value) SQLiteCursor::~SQLiteCursor() { + sqlite3_clear_bindings(m_cursor_stmt); sqlite3_reset(m_cursor_stmt); int res = sqlite3_finalize(m_cursor_stmt); if (res != SQLITE_OK) { @@ -563,6 +569,48 @@ std::unique_ptr SQLiteBatch::GetNewCursor() return cursor; } +std::unique_ptr SQLiteBatch::GetNewPrefixCursor(Span prefix) +{ + if (!m_database.m_db) return nullptr; + + // To get just the records we want, the SQL statement does a comparison of the binary data + // where the data must be greater than or equal to the prefix, and less than + // the prefix incremented by one (when interpreted as an integer) + std::vector start_range(prefix.begin(), prefix.end()); + std::vector end_range(prefix.begin(), prefix.end()); + auto it = end_range.rbegin(); + for (; it != end_range.rend(); ++it) { + if (*it == std::byte(std::numeric_limits::max())) { + *it = std::byte(0); + continue; + } + *it = std::byte(std::to_integer(*it) + 1); + break; + } + if (it == end_range.rend()) { + // If the prefix is all 0xff bytes, clear end_range as we won't need it + end_range.clear(); + } + + auto cursor = std::make_unique(start_range, end_range); + if (!cursor) return nullptr; + + const char* stmt_text = end_range.empty() ? "SELECT key, value FROM main WHERE key >= ?" : + "SELECT key, value FROM main WHERE key >= ? AND key < ?"; + int res = sqlite3_prepare_v2(m_database.m_db, stmt_text, -1, &cursor->m_cursor_stmt, nullptr); + if (res != SQLITE_OK) { + throw std::runtime_error(strprintf( + "SQLiteDatabase: Failed to setup cursor SQL statement: %s\n", sqlite3_errstr(res))); + } + + if (!BindBlobToStatement(cursor->m_cursor_stmt, 1, cursor->m_prefix_range_start, "prefix_start")) return nullptr; + if (!end_range.empty()) { + if (!BindBlobToStatement(cursor->m_cursor_stmt, 2, cursor->m_prefix_range_end, "prefix_end")) return nullptr; + } + + return cursor; +} + bool SQLiteBatch::TxnBegin() { if (!m_database.m_db || sqlite3_get_autocommit(m_database.m_db) == 0) return false; diff --git a/src/wallet/sqlite.h b/src/wallet/sqlite.h index b5d142434b59..5c379870bbc9 100644 --- a/src/wallet/sqlite.h +++ b/src/wallet/sqlite.h @@ -16,12 +16,21 @@ struct sqlite3; namespace wallet { class SQLiteDatabase; +/** RAII class that provides a database cursor */ class SQLiteCursor : public DatabaseCursor { public: sqlite3_stmt* m_cursor_stmt{nullptr}; + // Copies of the prefix things for the prefix cursor. + // Prevents SQLite from accessing temp variables for the prefix things. + std::vector m_prefix_range_start; + std::vector m_prefix_range_end; explicit SQLiteCursor() {} + explicit SQLiteCursor(std::vector start_range, std::vector end_range) + : m_prefix_range_start(std::move(start_range)), + m_prefix_range_end(std::move(end_range)) + {} ~SQLiteCursor() override; Status Next(DataStream& key, DataStream& value) override; @@ -64,6 +73,7 @@ class SQLiteBatch : public DatabaseBatch void Close() override; std::unique_ptr GetNewCursor() override; + std::unique_ptr GetNewPrefixCursor(Span prefix) override; bool TxnBegin() override; bool TxnCommit() override; bool TxnAbort() override; diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp index 8aa8bc2d43cd..7213327200b0 100644 --- a/src/wallet/test/db_tests.cpp +++ b/src/wallet/test/db_tests.cpp @@ -10,13 +10,56 @@ #include #include +#include +#ifdef USE_BDB #include +#endif +#ifdef USE_SQLITE +#include +#endif +#include +#include // for WALLET_FLAG_DESCRIPTORS #include #include #include +inline std::ostream& operator<<(std::ostream& os, const std::pair& kv) +{ + Span key{kv.first}, value{kv.second}; + os << "(\"" << std::string_view{reinterpret_cast(key.data()), key.size()} << "\", \"" + << std::string_view{reinterpret_cast(key.data()), key.size()} << "\")"; + return os; +} + namespace wallet { + +static Span StringBytes(std::string_view str) +{ + return AsBytes({str.data(), str.size()}); +} + +static SerializeData StringData(std::string_view str) +{ + auto bytes = StringBytes(str); + return SerializeData{bytes.begin(), bytes.end()}; +} + +static void CheckPrefix(DatabaseBatch& batch, Span prefix, MockableData expected) +{ + std::unique_ptr cursor = batch.GetNewPrefixCursor(prefix); + MockableData actual; + while (true) { + DataStream key, value; + DatabaseCursor::Status status = cursor->Next(key, value); + if (status == DatabaseCursor::Status::DONE) break; + BOOST_CHECK(status == DatabaseCursor::Status::MORE); + BOOST_CHECK( + actual.emplace(SerializeData(key.begin(), key.end()), SerializeData(value.begin(), value.end())).second); + } + BOOST_CHECK_EQUAL_COLLECTIONS(actual.begin(), actual.end(), expected.begin(), expected.end()); +} + BOOST_FIXTURE_TEST_SUITE(db_tests, BasicTestingSetup) static std::shared_ptr GetWalletEnv(const fs::path& path, fs::path& database_filename) @@ -82,5 +125,90 @@ BOOST_AUTO_TEST_CASE(getwalletenv_g_dbenvs_free_instance) BOOST_CHECK(env_2_a == env_2_b); } +static std::vector> TestDatabases(const fs::path& path_root) +{ + std::vector> dbs; + DatabaseOptions options; + DatabaseStatus status; + bilingual_str error; +#ifdef USE_BDB + dbs.emplace_back(MakeBerkeleyDatabase(path_root / "bdb", options, status, error)); +#endif +#ifdef USE_SQLITE + dbs.emplace_back(MakeSQLiteDatabase(path_root / "sqlite", options, status, error)); +#endif + dbs.emplace_back(CreateMockableWalletDatabase()); + return dbs; +} + +BOOST_AUTO_TEST_CASE(db_cursor_prefix_range_test) +{ + // Test each supported db + for (const auto& database : TestDatabases(m_path_root)) { + BOOST_ASSERT(database); + + std::vector prefixes = {"", "FIRST", "SECOND", "P\xfe\xff", "P\xff\x01", "\xff\xff"}; + + // Write elements to it + std::unique_ptr handler = database->MakeBatch(); + for (unsigned int i = 0; i < 10; i++) { + for (const auto& prefix : prefixes) { + BOOST_CHECK(handler->Write(std::make_pair(prefix, i), i)); + } + } + + // Now read all the items by prefix and verify that each element gets parsed correctly + for (const auto& prefix : prefixes) { + DataStream s_prefix; + s_prefix << prefix; + std::unique_ptr cursor = handler->GetNewPrefixCursor(s_prefix); + DataStream key; + DataStream value; + for (int i = 0; i < 10; i++) { + DatabaseCursor::Status status = cursor->Next(key, value); + BOOST_ASSERT(status == DatabaseCursor::Status::MORE); + + std::string key_back; + unsigned int i_back; + key >> key_back >> i_back; + BOOST_CHECK_EQUAL(key_back, prefix); + + unsigned int value_back; + value >> value_back; + BOOST_CHECK_EQUAL(value_back, i_back); + } + + // Let's now read it once more, it should return DONE + BOOST_CHECK(cursor->Next(key, value) == DatabaseCursor::Status::DONE); + } + } +} + +// Lower level DatabaseBase::GetNewPrefixCursor test, to cover cases that aren't +// covered in the higher level test above. The higher level test uses +// serialized strings which are prefixed with string length, so it doesn't test +// truly empty prefixes or prefixes that begin with \xff +BOOST_AUTO_TEST_CASE(db_cursor_prefix_byte_test) +{ + const MockableData::value_type + e{StringData(""), StringData("e")}, + p{StringData("prefix"), StringData("p")}, + ps{StringData("prefixsuffix"), StringData("ps")}, + f{StringData("\xff"), StringData("f")}, + fs{StringData("\xffsuffix"), StringData("fs")}, + ff{StringData("\xff\xff"), StringData("ff")}, + ffs{StringData("\xff\xffsuffix"), StringData("ffs")}; + for (const auto& database : TestDatabases(m_path_root)) { + std::unique_ptr batch = database->MakeBatch(); + for (const auto& [k, v] : {e, p, ps, f, fs, ff, ffs}) { + batch->Write(MakeUCharSpan(k), MakeUCharSpan(v)); + } + CheckPrefix(*batch, StringBytes(""), {e, p, ps, f, fs, ff, ffs}); + CheckPrefix(*batch, StringBytes("prefix"), {p, ps}); + CheckPrefix(*batch, StringBytes("\xff"), {f, fs, ff, ffs}); + CheckPrefix(*batch, StringBytes("\xff\xff"), {ff, ffs}); + } +} + BOOST_AUTO_TEST_SUITE_END() } // namespace wallet diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index 869b9a2f763a..e2ccc68b0c0c 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -106,6 +106,17 @@ CTxDestination getNewDestination(CWallet& w, OutputType output_type) return *Assert(w.GetNewDestination("")); } +// BytePrefix compares equality with other byte spans that begin with the same prefix. +struct BytePrefix { Span prefix; }; +bool operator<(BytePrefix a, Span b) { return a.prefix < b.subspan(0, std::min(a.prefix.size(), b.size())); } +bool operator<(Span a, BytePrefix b) { return a.subspan(0, std::min(a.size(), b.prefix.size())) < b.prefix; } + +MockableCursor::MockableCursor(const MockableData& records, bool pass, Span prefix) +{ + m_pass = pass; + std::tie(m_cursor, m_cursor_end) = records.equal_range(BytePrefix{prefix}); +} + DatabaseCursor::Status MockableCursor::Next(DataStream& key, DataStream& value) { if (!m_pass) { @@ -114,6 +125,8 @@ DatabaseCursor::Status MockableCursor::Next(DataStream& key, DataStream& value) if (m_cursor == m_cursor_end) { return Status::DONE; } + key.clear(); + value.clear(); const auto& [key_data, value_data] = *m_cursor; key.write(key_data); value.write(value_data); @@ -131,6 +144,7 @@ bool MockableBatch::ReadKey(DataStream&& key, DataStream& value) if (it == m_records.end()) { return false; } + value.clear(); value.write(it->second); return true; } @@ -186,7 +200,7 @@ bool MockableBatch::ErasePrefix(Span prefix) return true; } -std::unique_ptr CreateMockableWalletDatabase(std::map records) +std::unique_ptr CreateMockableWalletDatabase(MockableData records) { return std::make_unique(records); } diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h index fa58f061f785..ffa4e561e697 100644 --- a/src/wallet/test/util.h +++ b/src/wallet/test/util.h @@ -52,14 +52,17 @@ std::string getnewaddress(wallet::CWallet& w); /** Returns a new destination, of an specific type, from the wallet */ CTxDestination getNewDestination(wallet::CWallet& w, OutputType output_type); +using MockableData = std::map>; + class MockableCursor: public DatabaseCursor { public: - std::map::const_iterator m_cursor; - std::map::const_iterator m_cursor_end; + MockableData::const_iterator m_cursor; + MockableData::const_iterator m_cursor_end; bool m_pass; - explicit MockableCursor(const std::map& records, bool pass) : m_cursor(records.begin()), m_cursor_end(records.end()), m_pass(pass) {} + explicit MockableCursor(const MockableData& records, bool pass) : m_cursor(records.begin()), m_cursor_end(records.end()), m_pass(pass) {} + MockableCursor(const MockableData& records, bool pass, Span prefix); ~MockableCursor() {} Status Next(DataStream& key, DataStream& value) override; @@ -68,7 +71,7 @@ class MockableCursor: public DatabaseCursor class MockableBatch : public DatabaseBatch { private: - std::map& m_records; + MockableData& m_records; bool m_pass; bool ReadKey(DataStream&& key, DataStream& value) override; @@ -78,7 +81,7 @@ class MockableBatch : public DatabaseBatch bool ErasePrefix(Span prefix) override; public: - explicit MockableBatch(std::map& records, bool pass) : m_records(records), m_pass(pass) {} + explicit MockableBatch(MockableData& records, bool pass) : m_records(records), m_pass(pass) {} ~MockableBatch() {} void Flush() override {} @@ -88,6 +91,9 @@ class MockableBatch : public DatabaseBatch { return std::make_unique(m_records, m_pass); } + std::unique_ptr GetNewPrefixCursor(Span prefix) override { + return std::make_unique(m_records, m_pass, prefix); + } bool TxnBegin() override { return m_pass; } bool TxnCommit() override { return m_pass; } bool TxnAbort() override { return m_pass; } @@ -98,10 +104,10 @@ class MockableBatch : public DatabaseBatch class MockableDatabase : public WalletDatabase { public: - std::map m_records; + MockableData m_records; bool m_pass{true}; - MockableDatabase(std::map records = {}) : WalletDatabase(), m_records(records) {} + MockableDatabase(MockableData records = {}) : WalletDatabase(), m_records(records) {} ~MockableDatabase() {}; void Open() override {} @@ -122,7 +128,7 @@ class MockableDatabase : public WalletDatabase std::unique_ptr MakeBatch(bool flush_on_close = true) override { return std::make_unique(m_records, m_pass); } }; -std::unique_ptr CreateMockableWalletDatabase(std::map records = {}); +std::unique_ptr CreateMockableWalletDatabase(MockableData records = {}); MockableDatabase& GetMockableDatabase(CWallet& wallet); } // namespace wallet diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp index aa991f43521c..7d59e8e3b402 100644 --- a/src/wallet/test/walletload_tests.cpp +++ b/src/wallet/test/walletload_tests.cpp @@ -83,7 +83,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_ckey, TestingSetup) { SerializeData ckey_record_key; SerializeData ckey_record_value; - std::map records; + MockableData records; { // Context setup. From 7ad03f880723e936eeaf60121a4fde60caf37e76 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 12 Jun 2023 16:38:48 +0100 Subject: [PATCH 06/11] Merge bitcoin/bitcoin#27783: Add public Boost headers explicitly 2484cacb7a6367b24e924dba0825c843b1dfc1c3 Add public Boost headers explicitly (Hennadii Stepanov) fade2adb5bb4ce9753e7f25da5fb1521f2f503ec test: Avoid `BOOST_ASSERT` macro (Hennadii Stepanov) Pull request description: To check symbols in the code base, run: ``` git grep boost::multi_index::identity git grep boost::multi_index::indexed_by git grep boost::multi_index::tag git grep boost::make_tuple ``` Hoping on the absence of conflicts with top-prio PRs :) ACKs for top commit: MarcoFalke: lgtm ACK 2484cacb7a6367b24e924dba0825c843b1dfc1c3 TheCharlatan: ACK 2484cacb7a6367b24e924dba0825c843b1dfc1c3 Tree-SHA512: d122ab028eee76ee1c4609ed51ec8db0c8c768edcc2ff2c0e420a48e051aa71e99748cdb5d22985ae6d97c808c77c1a27561f0715f77b256f74c1c310b37694c Co-authored-by: fanquake --- src/txrequest.cpp | 6 +++++- src/wallet/test/wallet_tests.cpp | 5 ++--- src/wallet/test/walletload_tests.cpp | 2 +- test/lint/lint-includes.py | 4 +++- 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/txrequest.cpp b/src/txrequest.cpp index 06f3c7aa3d0a..5bb4c07f3ec5 100644 --- a/src/txrequest.cpp +++ b/src/txrequest.cpp @@ -9,8 +9,12 @@ #include #include -#include +#include #include +#include +#include +#include +#include #include #include diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index dfec1db4d420..9d46dccc772d 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -2012,11 +2012,10 @@ BOOST_FIXTURE_TEST_CASE(wallet_sync_tx_invalid_state_test, TestChain100Setup) } // Add tx to wallet - const auto& op_dest = wallet.GetNewDestination(""); - BOOST_ASSERT(op_dest); + const auto op_dest{*Assert(wallet.GetNewDestination(""))}; CMutableTransaction mtx; - mtx.vout.emplace_back(COIN, GetScriptForDestination(*op_dest)); + mtx.vout.emplace_back(COIN, GetScriptForDestination(op_dest)); mtx.vin.emplace_back(g_insecure_rand_ctx.rand256(), 0); const auto& tx_id_to_spend = wallet.AddToWallet(MakeTransactionRef(mtx), TxStateInMempool{})->GetHash(); diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp index 7d59e8e3b402..aa8893be5f84 100644 --- a/src/wallet/test/walletload_tests.cpp +++ b/src/wallet/test/walletload_tests.cpp @@ -169,7 +169,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_ckey, TestingSetup) // Fourth test case: // Verify that loading up a 'ckey' with an invalid pubkey throws an error CPubKey invalid_key; - BOOST_ASSERT(!invalid_key.IsValid()); + BOOST_CHECK(!invalid_key.IsValid()); SerializeData key = MakeSerializeData(DBKeys::CRYPTED_KEY, invalid_key); records[key] = ckey_record_value; diff --git a/test/lint/lint-includes.py b/test/lint/lint-includes.py index 9cf5e2d99c8a..b09d8993e99f 100755 --- a/test/lint/lint-includes.py +++ b/test/lint/lint-includes.py @@ -31,7 +31,9 @@ "boost/signals2/optional_last_value.hpp", "boost/signals2/signal.hpp", "boost/test/included/unit_test.hpp", - "boost/test/unit_test.hpp"] + "boost/test/unit_test.hpp", + "boost/tuple/tuple.hpp", + ] EXCLUDED_BOOST_DIRS = ["src/immer/"] From b9c72b5945ba74284cee512329618ab80fa8777b Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 20 Jun 2023 13:40:49 -0400 Subject: [PATCH 07/11] Merge bitcoin/bitcoin#27907: bench: bugfix, disable birth time block skip for wallet_create_tx.cpp a72af2e833bcf7baa79a246609da90ae0ade2a82 bench: disable birth time block skip for wallet_create_tx.cpp (furszy) Pull request description: As the benchmarks inside `wallet_create_tx.cpp` assert the wallet balance at the end, they require all blocks to be scanned by the wallet. So, we need to ensure that no blocks are skipped by the recently added wallet birth time functionality. This just means setting the wallet birth time to the genesis block time. So the wallet is always older than any new block. ACKs for top commit: achow101: ACK a72af2e833bcf7baa79a246609da90ae0ade2a82 hernanmarino: ACK a72af2e833bcf7baa79a246609da90ae0ade2a82 TheCharlatan: ACK a72af2e833bcf7baa79a246609da90ae0ade2a82 Tree-SHA512: d3148659bd633d20978736e1292e3456a2c6dd2b6c8f60625a4160e16818d923487c889237eb3f34693f7dd78b7d124b89afdc56e4c9fad370026d0733ef1e08 Co-authored-by: Andrew Chow --- src/bench/wallet_create_tx.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/bench/wallet_create_tx.cpp b/src/bench/wallet_create_tx.cpp index 17c249641518..67b1dd1d904d 100644 --- a/src/bench/wallet_create_tx.cpp +++ b/src/bench/wallet_create_tx.cpp @@ -84,6 +84,8 @@ static void WalletCreateTx(benchmark::Bench& bench, const OutputType output_type { const auto test_setup = MakeNoLogFileContext(); + // Set clock to genesis block, so the descriptors/keys creation time don't interfere with the blocks scanning process. + SetMockTime(test_setup->m_node.chainman->GetParams().GenesisBlock().nTime); CWallet wallet{test_setup->m_node.chain.get(), "", CreateMockableWalletDatabase()}; { LOCK(wallet.cs_wallet); @@ -137,6 +139,8 @@ static void WalletCreateTx(benchmark::Bench& bench, const OutputType output_type static void AvailableCoins(benchmark::Bench& bench, const std::vector& output_type) { const auto test_setup = MakeNoLogFileContext(); + // Set clock to genesis block, so the descriptors/keys creation time don't interfere with the blocks scanning process. + SetMockTime(test_setup->m_node.chainman->GetParams().GenesisBlock().nTime); CWallet wallet{test_setup->m_node.chain.get(), "", CreateMockableWalletDatabase()}; { LOCK(wallet.cs_wallet); From c1dec438cfbc9695ab121abbc74943bc9232ee95 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 22 Jun 2023 12:20:25 +0100 Subject: [PATCH 08/11] Merge bitcoin/bitcoin#27889: test: Kill `BOOST_ASSERT` and update the linter 28fff06afe98177c14a932abf95b380bb51c6653 test: Make linter to look for `BOOST_ASSERT` macros (Hennadii Stepanov) 47fe551e52d8b3f607d55ad20073c0436590e081 test: Kill `BOOST_ASSERT` (Hennadii Stepanov) Pull request description: One of the goals of https://github.com/bitcoin/bitcoin/pull/27783 was to get rid of the `BOOST_ASSERT` macros instead of including the `boost/assert.hpp` headers. See https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1210612717. It turns out that a couple of those macros sneaked into the codebase in https://github.com/bitcoin/bitcoin/pull/27790. This PR makes the linter guard against new instances of the `BOOST_ASSERT` macros and replaces the current ones. ACKs for top commit: kevkevinpal: ACK [28fff06](https://github.com/bitcoin/bitcoin/pull/27889/commits/28fff06afe98177c14a932abf95b380bb51c6653) stickies-v: ACK 28fff06af TheCharlatan: ACK 28fff06afe98177c14a932abf95b380bb51c6653 Tree-SHA512: 371f613592cf677afe0196d18c83943c6c8f1e998f57b4ff3ee58bfeff8636e4dac1357840d8611b4f7b197def94df10fe1a8ca3282b00b7b4eff4624552dda8 Co-authored-by: fanquake --- src/test/evo_deterministicmns_tests.cpp | 4 ++-- src/wallet/test/db_tests.cpp | 7 +++---- src/wallet/test/wallet_tests.cpp | 6 +++--- src/wallet/test/walletdb_tests.cpp | 2 +- test/lint/lint-assertions.py | 10 ++++++++++ 5 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/test/evo_deterministicmns_tests.cpp b/src/test/evo_deterministicmns_tests.cpp index df0146e63a87..1728ef9b55dd 100644 --- a/src/test/evo_deterministicmns_tests.cpp +++ b/src/test/evo_deterministicmns_tests.cpp @@ -997,7 +997,7 @@ void FuncDIP3Protx(TestChainSetup& setup) // check MN reward payments for (size_t i = 0; i < 20; i++) { auto dmnExpectedPayee = dmnman.GetListAtChainTip().GetMNPayee(tip_index()); - BOOST_ASSERT(dmnExpectedPayee); + BOOST_REQUIRE(dmnExpectedPayee); CBlock block = setup.CreateAndProcessBlock({}, coinbase_pk); sync_dmn_tip(); @@ -1108,7 +1108,7 @@ void FuncDIP3Protx(TestChainSetup& setup) bool foundRevived = false; for (size_t i = 0; i < 20; i++) { auto dmnExpectedPayee = dmnman.GetListAtChainTip().GetMNPayee(tip_index()); - BOOST_ASSERT(dmnExpectedPayee); + BOOST_REQUIRE(dmnExpectedPayee); if (dmnExpectedPayee->proTxHash == dmnHashes[0]) { foundRevived = true; } diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp index 7213327200b0..47c160fd64d5 100644 --- a/src/wallet/test/db_tests.cpp +++ b/src/wallet/test/db_tests.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include #ifdef USE_BDB @@ -145,12 +146,10 @@ BOOST_AUTO_TEST_CASE(db_cursor_prefix_range_test) { // Test each supported db for (const auto& database : TestDatabases(m_path_root)) { - BOOST_ASSERT(database); - std::vector prefixes = {"", "FIRST", "SECOND", "P\xfe\xff", "P\xff\x01", "\xff\xff"}; // Write elements to it - std::unique_ptr handler = database->MakeBatch(); + std::unique_ptr handler = Assert(database)->MakeBatch(); for (unsigned int i = 0; i < 10; i++) { for (const auto& prefix : prefixes) { BOOST_CHECK(handler->Write(std::make_pair(prefix, i), i)); @@ -166,7 +165,7 @@ BOOST_AUTO_TEST_CASE(db_cursor_prefix_range_test) DataStream value; for (int i = 0; i < 10; i++) { DatabaseCursor::Status status = cursor->Next(key, value); - BOOST_ASSERT(status == DatabaseCursor::Status::MORE); + BOOST_CHECK_EQUAL(status, DatabaseCursor::Status::MORE); std::string key_back; unsigned int i_back; diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 9d46dccc772d..fc82abcaf9da 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -1730,7 +1730,7 @@ BOOST_FIXTURE_TEST_CASE(UnconfirmableOutputsAreNotWalletFunds, ListCoinsTestingS LOCK(wallet->cs_wallet); const auto dest{wallet->GetNewDestination("")}; - BOOST_ASSERT(dest); + BOOST_REQUIRE(dest); // Use a real CoinJoin denomination so the denominated-credit paths apply. const CAmount denom{CoinJoin::GetSmallestDenomination()}; @@ -1760,7 +1760,7 @@ BOOST_FIXTURE_TEST_CASE(MempoolRemovalInvalidatesAnonymizableTally, ListCoinsTes LOCK(wallet->cs_wallet); const auto dest{wallet->GetNewDestination("")}; - BOOST_ASSERT(dest); + BOOST_REQUIRE(dest); // A wallet transaction in the mempool is trusted at depth zero, so its outputs count // towards the anonymizable tally and that tally is cacheable. @@ -1901,7 +1901,7 @@ BOOST_FIXTURE_TEST_CASE(AbandonedSpendRestoresDustLock, ListCoinsTestingSetup) wallet->m_dust_protection_threshold = 1 * COIN; const auto dest{wallet->GetNewDestination("")}; - BOOST_ASSERT(dest); + BOOST_REQUIRE(dest); // An external transaction (no input is ours) pays us a dust-protection // target; AddToWallet() locks the output on insertion. diff --git a/src/wallet/test/walletdb_tests.cpp b/src/wallet/test/walletdb_tests.cpp index 94fb2a5ca1b2..75b49da43ed2 100644 --- a/src/wallet/test/walletdb_tests.cpp +++ b/src/wallet/test/walletdb_tests.cpp @@ -160,7 +160,7 @@ BOOST_AUTO_TEST_CASE(walletdb_read_write_deadlock) DatabaseStatus status; bilingual_str error_string; std::unique_ptr db = MakeDatabase(m_path_root / strprintf("wallet_%d_.dat", db_format).c_str(), options, status, error_string); - BOOST_ASSERT(status == DatabaseStatus::SUCCESS); + BOOST_REQUIRE(status == DatabaseStatus::SUCCESS); std::shared_ptr wallet(new CWallet(m_node.chain.get(), "", std::move(db))); wallet->m_keypool_size = 4; diff --git a/test/lint/lint-assertions.py b/test/lint/lint-assertions.py index 76e16147064b..d9f86b22b8f7 100755 --- a/test/lint/lint-assertions.py +++ b/test/lint/lint-assertions.py @@ -35,6 +35,16 @@ def main(): ":(exclude)src/rpc/server.cpp", ], "CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code.") + # The `BOOST_ASSERT` macro requires to `#include boost/assert.hpp`, + # which is an unnecessary Boost dependency. + exit_code |= git_grep([ + "-E", + r"BOOST_ASSERT *\(.*\);", + "--", + "*.cpp", + "*.h", + ], "BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK.") + sys.exit(exit_code) From 2b726d31af1a256ce56bf89a85c5a3fafe8b2a14 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Fri, 23 Jun 2023 16:42:04 -0400 Subject: [PATCH 09/11] Merge bitcoin/bitcoin#27846: [coinselection] Increase SRD target by change_fee MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1771daa815ec014276cfcb30c934b0eaff4d72bf [fuzz] Show that SRD budgets for non-dust change (Murch) 941b8c6539d72890fd4e36fc900be9c300e1d737 [bug] Increase SRD target by change_fee (Murch) Pull request description: I discovered via fuzzing of another coin selection approach that at extremely high feerates SRD may find input sets that lead to transactions without change outputs. This is an unintended outcome since SRD is meant to always produce a transaction with a change output—we use other algorithms to specifically search for changeless solutions. The issue occurs when the flat allowance of 50,000 ṩ for change is insufficient to pay for the creation of a change output with a non-dust amount, at and above 1,613 ṩ/vB. Increasing the change budget by `change_fee` makes SRD behave as expected at any feerates. Note: The intermittent failures of `test/functional/interface_usdt_mempool.py` are a known issue: https://github.com/bitcoin/bitcoin/issues/27380 ACKs for top commit: achow101: ACK 1771daa815ec014276cfcb30c934b0eaff4d72bf S3RK: ACK 1771daa815ec014276cfcb30c934b0eaff4d72bf Tree-SHA512: 3f36a3e317ef0a711d0e409069c05032bff1d45403023f3728bf73dfd55ddd9e0dc2a9969d4d69fe0a426807ebb0bed1f54abfc05581409bfe42c327acf766d4 Co-authored-by: Andrew Chow --- src/wallet/coinselection.cpp | 4 ++-- src/wallet/coinselection.h | 2 +- src/wallet/spend.cpp | 2 +- src/wallet/test/coinselector_tests.cpp | 2 +- src/wallet/test/fuzz/coinselection.cpp | 7 +++++-- 5 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 64c90612167b..a11a3217091d 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -207,7 +207,7 @@ class MinOutputGroupComparator } }; -util::Result SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, FastRandomContext& rng, +util::Result SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, CAmount change_fee, FastRandomContext& rng, int max_weight, bool only_fully_mixed) { if (utxo_pool.empty()) return util::Error(); @@ -220,7 +220,7 @@ util::Result SelectCoinsSRD(const std::vector& utx // generated one, since SRD will result in a random change amount anyway; avoid making the // target needlessly large. if (!only_fully_mixed) { - target_value += CHANGE_LOWER; + target_value += CHANGE_LOWER + change_fee; } std::vector indexes; diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index de7c15496000..b874af9fa8fe 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -432,7 +432,7 @@ util::Result SelectCoinsBnB(std::vector& utxo_pool * @param[in] max_weight The maximum allowed weight for a selection result to be valid * @returns If successful, a valid SelectionResult, otherwise, util::Error */ -util::Result SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, FastRandomContext& rng, +util::Result SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, CAmount change_fee, FastRandomContext& rng, int max_weight, bool only_fully_mixed); // Original coin selection algorithm as a fallback diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index bb529e32fe3f..38fad606f74c 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -600,7 +600,7 @@ util::Result ChooseSelectionResult(const CWallet& wallet, const results.push_back(*knapsack_result); } else append_error(knapsack_result); - if (auto srd_result{SelectCoinsSRD(groups.positive_group, nTargetValue, coin_selection_params.rng_fast, max_inputs_weight, + if (auto srd_result{SelectCoinsSRD(groups.positive_group, nTargetValue, coin_selection_params.m_change_fee, coin_selection_params.rng_fast, max_inputs_weight, coin_selection_params.m_coin_type == CoinType::ONLY_FULLY_MIXED)}) { srd_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); results.push_back(*srd_result); diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 5d521c364eff..da7bc9bcecca 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -1048,7 +1048,7 @@ static util::Result SelectCoinsSRDResult(const CAmount& target, CoinEligibilityFilter filter(0, 0, 0); Groups group = GroupOutputs(*wallet, coin_setup(*wallet), cs_params, {{filter}})[filter].all_groups; - return wallet::SelectCoinsSRD(group.positive_group, target, cs_params.rng_fast, max_weight, false); + return wallet::SelectCoinsSRD(group.positive_group, target, cs_params.m_change_fee, cs_params.rng_fast, max_weight, false); } BOOST_AUTO_TEST_CASE(srd_tests) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 4903b5477574..a3ba1d996d50 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -89,8 +89,11 @@ FUZZ_TARGET(coinselection) // Run coinselection algorithms const auto result_bnb = SelectCoinsBnB(group_pos, target, cost_of_change, MAX_STANDARD_TX_SIZE); - auto result_srd = SelectCoinsSRD(group_pos, target, fast_random_context, MAX_STANDARD_TX_SIZE, false); - if (result_srd) result_srd->ComputeAndSetWaste(cost_of_change, cost_of_change, 0); + auto result_srd = SelectCoinsSRD(group_pos, target, coin_params.m_change_fee, fast_random_context, MAX_STANDARD_TX_SIZE, false); + if (result_srd) { + assert(result_srd->GetChange(CHANGE_LOWER, coin_params.m_change_fee) > 0); // Demonstrate that SRD creates change of at least CHANGE_LOWER + result_srd->ComputeAndSetWaste(cost_of_change, cost_of_change, 0); + } CAmount change_target{GenerateChangeTarget(target, coin_params.m_change_fee, fast_random_context)}; auto result_knapsack = KnapsackSolver(group_all, target, change_target, fast_random_context, From ac349d1802ff48fae7241b130dc1d0e90386ea2b Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 29 Jun 2023 13:05:42 +0100 Subject: [PATCH 10/11] Merge bitcoin/bitcoin#27932: test: Fuzz on macOS fae7c50d201726f605938c3511dd9119efeea5ec test: Run fuzz tests on macOS (MarcoFalke) Pull request description: Any reason not to? ACKs for top commit: jamesob: Github ACK https://github.com/bitcoin/bitcoin/pull/27932/commits/fae7c50d201726f605938c3511dd9119efeea5ec dergoegge: utACK fae7c50d201726f605938c3511dd9119efeea5ec Tree-SHA512: e45122d73fafb17cea312258314b826cb0745e08daadd28465f687ec02d4c127d2f8cbe20179a9fff5712038850c02c968abb4838fa088b7555e28709317d3a3 Co-authored-by: fanquake --- ci/test/00_setup_env_mac_native.sh | 2 ++ src/test/fuzz/rpc.cpp | 1 + test/config.ini.in | 2 +- test/fuzz/test_runner.py | 36 ++++++++++++++---------------- 4 files changed, 21 insertions(+), 20 deletions(-) diff --git a/ci/test/00_setup_env_mac_native.sh b/ci/test/00_setup_env_mac_native.sh index 9241fe348f55..c3edd52d499b 100755 --- a/ci/test/00_setup_env_mac_native.sh +++ b/ci/test/00_setup_env_mac_native.sh @@ -15,3 +15,5 @@ export CI_OS_NAME="macos" export NO_DEPENDS=1 export OSX_SDK="" export CCACHE_MAXSIZE=400M +export RUN_FUZZ_TESTS=true +export FUZZ_TESTS_CONFIG="--exclude banman" # https://github.com/bitcoin/bitcoin/issues/27924 diff --git a/src/test/fuzz/rpc.cpp b/src/test/fuzz/rpc.cpp index 874eb8a11bcf..e943c14c42ed 100644 --- a/src/test/fuzz/rpc.cpp +++ b/src/test/fuzz/rpc.cpp @@ -73,6 +73,7 @@ const std::vector RPC_COMMANDS_NOT_SAFE_FOR_FUZZING{ "addpeeraddress", // avoid DNS lookups "dumptxoutset", // avoid writing to disk "dumpwallet", // avoid writing to disk + "enumeratesigners", "echoipc", // avoid assertion failure (Assertion `"EnsureAnyNodeContext(request.context).init" && check' failed.) "generatetoaddress", // avoid prohibitively slow execution (when `num_blocks` is large) "generatetodescriptor", // avoid prohibitively slow execution (when `nblocks` is large) diff --git a/test/config.ini.in b/test/config.ini.in index 89d3e6995d41..28ab84ea6c6e 100644 --- a/test/config.ini.in +++ b/test/config.ini.in @@ -22,7 +22,7 @@ RPCAUTH=@abs_top_srcdir@/share/rpcauth/rpcauth.py @BUILD_BITCOIN_UTIL_TRUE@ENABLE_UTIL_TOOL=true @BUILD_BITCOIN_WALLET_TRUE@ENABLE_WALLET_TOOL=true @BUILD_BITCOIND_TRUE@ENABLE_BITCOIND=true -@ENABLE_FUZZ_TRUE@ENABLE_FUZZ=true +@ENABLE_FUZZ_BINARY_TRUE@ENABLE_FUZZ_BINARY=true @ENABLE_ZMQ_TRUE@ENABLE_ZMQ=true @ENABLE_EXTERNAL_SIGNER_TRUE@ENABLE_EXTERNAL_SIGNER=true @ENABLE_USDT_TRACEPOINTS_TRUE@ENABLE_USDT_TRACEPOINTS=true diff --git a/test/fuzz/test_runner.py b/test/fuzz/test_runner.py index 4bda3dd7cbc0..7ea4998b7813 100755 --- a/test/fuzz/test_runner.py +++ b/test/fuzz/test_runner.py @@ -96,8 +96,8 @@ def main(): configfile = os.path.abspath(os.path.dirname(__file__)) + "/../config.ini" config.read_file(open(configfile, encoding="utf8")) - if not config["components"].getboolean("ENABLE_FUZZ"): - logging.error("Must have fuzz targets built") + if not config["components"].getboolean("ENABLE_FUZZ_BINARY"): + logging.error("Must have fuzz executable built") sys.exit(1) fuzz_bin=os.getenv("DASHFUZZ", default=os.path.join(config["environment"]["BUILDDIR"], 'src', 'test', 'fuzz', 'fuzz')) @@ -154,11 +154,12 @@ def main(): ], env=get_fuzz_env(target=test_list_selection[0], source_dir=config['environment']['SRCDIR']), timeout=20, - check=True, + check=False, stderr=subprocess.PIPE, text=True, ).stderr - if "libFuzzer" not in help_output: + using_libfuzzer = "libFuzzer" in help_output + if (args.generate or args.m_dir) and not using_libfuzzer: logging.error("Must be built with libFuzzer") sys.exit(1) except subprocess.TimeoutExpired: @@ -192,6 +193,7 @@ def main(): test_list=test_list_selection, src_dir=config['environment']['SRCDIR'], fuzz_bin=fuzz_bin, + using_libfuzzer=using_libfuzzer, use_valgrind=args.valgrind, empty_min_time=args.empty_min_time, ) @@ -324,7 +326,7 @@ def job(t, args): future.result() -def run_once(*, fuzz_pool, corpus, test_list, src_dir, fuzz_bin, use_valgrind, empty_min_time): +def run_once(*, fuzz_pool, corpus, test_list, src_dir, fuzz_bin, using_libfuzzer, use_valgrind, empty_min_time): jobs = [] for t in test_list: corpus_path = corpus / t @@ -333,13 +335,16 @@ def run_once(*, fuzz_pool, corpus, test_list, src_dir, fuzz_bin, use_valgrind, e fuzz_bin, ] empty_dir = not any(corpus_path.iterdir()) - if empty_min_time and empty_dir: - args += [f"-max_total_time={empty_min_time}"] + if using_libfuzzer: + if empty_min_time and empty_dir: + args += [f"-max_total_time={empty_min_time}"] + else: + args += [ + "-runs=1", + corpus_path, + ] else: - args += [ - "-runs=1", - corpus_path, - ] + args += [corpus_path] if use_valgrind: args = ['valgrind', '--quiet', '--error-exitcode=1'] + args @@ -366,14 +371,7 @@ def job(t, args): logging.info(e.stdout) if e.stderr: logging.info(e.stderr) - logging.info("Target \"{}\" failed with exit code {}: {}".format(t, e.returncode, " ".join(args))) - sys.exit(1) - except subprocess.CalledProcessError as e: - if e.stdout: - logging.info(e.stdout) - if e.stderr: - logging.info(e.stderr) - logging.info("Target \"{}\" failed with exit code {}".format(" ".join(result.args), e.returncode)) + logging.info(f"Target {result.args} failed with exit code {e.returncode}") sys.exit(1) From 2552f141136b382687c451f7c896f81542150d0f Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 1 Aug 2023 09:41:36 +0100 Subject: [PATCH 11/11] Merge bitcoin/bitcoin#28194: test: python E721 and flake8 updates bee2d57a655645dbfaf0242e85c5af034023a2fb script: update flake8 to 6.1.0 (Jon Atack) 38c3fd846bff163eb7c50bd77efcdcf8fcbc7f43 test: python E721 updates (Jon Atack) Pull request description: Update our functional tests per [E721](https://www.flake8rules.com/rules/E721.html) enforced by [flake8 6.1.0](https://flake8.pycqa.org/en/latest/release-notes/6.1.0.html), and update our CI lint task to use that release. This makes the following linter output on current master with flake8 6.1.0 green. ``` $ ./test/lint/lint-python.py ; ./test/lint/lint-spelling.py test/functional/p2p_invalid_locator.py:35:16: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()` test/functional/test_framework/siphash.py:34:12: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()` test/functional/test_framework/siphash.py:64:12: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()` src/test/fuzz/descriptor_parse.cpp:88: occurences ==> occurrences ^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt ``` ACKs for top commit: MarcoFalke: lgtm ACK bee2d57a655645dbfaf0242e85c5af034023a2fb Tree-SHA512: f3788a543ca98e44eeeba1d06c32f1b11eec95d4aef068aa1b6b5c401261adfa3fb6c6d6c769f3fe6839d78e74a310d5c926867e7c367d6513a53d580fd376f3 Co-authored-by: fanquake --- ci/lint/04_install.sh | 2 +- contrib/containers/ci/ci-slim.Dockerfile | 2 +- test/functional/feature_governance.py | 3 +-- test/functional/feature_llmq_connections.py | 11 +++++------ test/functional/feature_llmq_singlenode.py | 3 +-- test/functional/feature_mnehf.py | 3 +-- test/functional/interface_zmq_dash.py | 3 +-- test/functional/p2p_invalid_locator.py | 2 +- test/functional/test_framework/crypto/siphash.py | 4 ++-- 9 files changed, 14 insertions(+), 19 deletions(-) diff --git a/ci/lint/04_install.sh b/ci/lint/04_install.sh index 02005530af72..cc1b740c6098 100755 --- a/ci/lint/04_install.sh +++ b/ci/lint/04_install.sh @@ -36,7 +36,7 @@ python3 --version # NOTE: BUMP ALSO contrib/containers/ci/ci-slim.Dockerfile ${CI_RETRY_EXE} pip3 install \ codespell==2.2.1 \ - flake8==5.0.4 \ + flake8==6.1.0 \ lief==0.13.2 \ mypy==0.981 \ pyzmq==24.0.1 \ diff --git a/contrib/containers/ci/ci-slim.Dockerfile b/contrib/containers/ci/ci-slim.Dockerfile index 9aca0169d8d4..2cce5c2c705c 100644 --- a/contrib/containers/ci/ci-slim.Dockerfile +++ b/contrib/containers/ci/ci-slim.Dockerfile @@ -80,7 +80,7 @@ ENV UV_SYSTEM_PYTHON=1 # NOTE: if versions are changed, update ci/lint/04_install.sh RUN uv pip install --system --break-system-packages \ codespell==2.2.1 \ - flake8==5.0.4 \ + flake8==6.1.0 \ jinja2 \ lief==0.13.2 \ mypy==0.981 \ diff --git a/test/functional/feature_governance.py b/test/functional/feature_governance.py index 191700baf99c..fad3c5833e36 100755 --- a/test/functional/feature_governance.py +++ b/test/functional/feature_governance.py @@ -14,7 +14,6 @@ from test_framework.messages import CBlock, from_hex, uint256_to_string from test_framework.test_framework import ( DashTestFramework, - MasternodeInfo, ) from test_framework.governance import EXPECTED_STDERR_GOV_PRUNED_TOO_FAR, have_trigger_for_height, prepare_object from test_framework.util import assert_equal, assert_raises_rpc_error, force_finish_mnsync, satoshi_round @@ -252,7 +251,7 @@ def run_test(self): _, mn_payee_protx = height_protx_list[1] payee_idx = None - for mn in self.mninfo: # type: MasternodeInfo + for mn in self.mninfo: if mn.proTxHash == mn_payee_protx: payee_idx = mn.nodeIdx break diff --git a/test/functional/feature_llmq_connections.py b/test/functional/feature_llmq_connections.py index b40d326fa285..663586460bd7 100755 --- a/test/functional/feature_llmq_connections.py +++ b/test/functional/feature_llmq_connections.py @@ -14,7 +14,6 @@ from test_framework.test_framework import ( DashTestFramework, - MasternodeInfo, ) from test_framework.util import assert_greater_than_or_equal, force_finish_mnsync @@ -86,7 +85,7 @@ def run_test(self): self.log.info("check that old masternode connections are dropped") removed = False - for mn in self.mninfo: # type: MasternodeInfo + for mn in self.mninfo: if len(mn.get_node(self).quorum("memberof", mn.proTxHash)) > 0: try: with mn.get_node(self).assert_debug_log(['removing masternodes quorum connections']): @@ -102,7 +101,7 @@ def run_test(self): self.log.info("check that inter-quorum masternode connections are added") added = False - for mn in self.mninfo: # type: MasternodeInfo + for mn in self.mninfo: if len(mn.get_node(self).quorum("memberof", mn.proTxHash)) > 0: try: with mn.get_node(self).assert_debug_log(['adding mn inter-quorum connections']): @@ -199,11 +198,11 @@ def check_mn_peers(node=mn_info.get_node(self), others=others): def check_reconnects(self, expected_connection_count): self.log.info("disable and re-enable networking on all masternodes") - for mn in self.mninfo: # type: MasternodeInfo + for mn in self.mninfo: mn.get_node(self).setnetworkactive(False) - for mn in self.mninfo: # type: MasternodeInfo + for mn in self.mninfo: self.wait_until(lambda: len(mn.get_node(self).getpeerinfo()) == 0) - for mn in self.mninfo: # type: MasternodeInfo + for mn in self.mninfo: mn.get_node(self).setnetworkactive(True) self.bump_mocktime(60) diff --git a/test/functional/feature_llmq_singlenode.py b/test/functional/feature_llmq_singlenode.py index 4e7040b85994..0238ddced9b2 100755 --- a/test/functional/feature_llmq_singlenode.py +++ b/test/functional/feature_llmq_singlenode.py @@ -14,7 +14,6 @@ from test_framework.authproxy import JSONRPCException from test_framework.test_framework import ( DashTestFramework, - MasternodeInfo, ) from test_framework.util import ( assert_raises_rpc_error, @@ -43,7 +42,7 @@ def check_sigs(self, hasrecsigs, isconflicting1, isconflicting2): conflicting_1 = False conflicting_2 = False - for mn in self.mninfo: # type: MasternodeInfo + for mn in self.mninfo: if mn.get_node(self).quorum("hasrecsig", q_type, id, msgHash): has_sig = True if mn.get_node(self).quorum("isconflicting", q_type, id, msgHash): diff --git a/test/functional/feature_mnehf.py b/test/functional/feature_mnehf.py index 7ae66bc23f97..10995fc43c2a 100755 --- a/test/functional/feature_mnehf.py +++ b/test/functional/feature_mnehf.py @@ -17,7 +17,6 @@ from test_framework.test_framework import ( DashTestFramework, - MasternodeInfo ) from test_framework.util import ( assert_equal, @@ -46,7 +45,7 @@ def restart_all_nodes(self, params=None): self.log.info(f"Actual restart options: {self.extra_args[inode]}") self.restart_node(0) - for mn in self.mninfo: # type: MasternodeInfo + for mn in self.mninfo: index = mn.nodeIdx self.stop_node(index) self.start_masternode(mn) diff --git a/test/functional/interface_zmq_dash.py b/test/functional/interface_zmq_dash.py index a97b49e1cb30..fa9782ea49e8 100755 --- a/test/functional/interface_zmq_dash.py +++ b/test/functional/interface_zmq_dash.py @@ -14,7 +14,6 @@ from test_framework.test_framework import ( DashTestFramework, - MasternodeInfo, ) from test_framework.p2p import P2PInterface from test_framework.util import ( @@ -219,7 +218,7 @@ def validate_recovered_sig(request_id, msg_hash): sign_id = uint256_to_string(random.getrandbits(256)) sign_msg_hash = uint256_to_string(random.getrandbits(256)) quorumHash = self.nodes[0].quorum("selectquorum", 100, sign_id)["quorumHash"] - for mn in self.get_quorum_masternodes(quorumHash): # type: MasternodeInfo + for mn in self.get_quorum_masternodes(quorumHash): mn.get_node(self).quorum("sign", 100, sign_id, sign_msg_hash) validate_recovered_sig(sign_id, sign_msg_hash) # Unsubscribe from recovered signature messages diff --git a/test/functional/p2p_invalid_locator.py b/test/functional/p2p_invalid_locator.py index a586b48d4c00..2a521ec0d7a2 100755 --- a/test/functional/p2p_invalid_locator.py +++ b/test/functional/p2p_invalid_locator.py @@ -32,7 +32,7 @@ def run_test(self): within_max_peer = node.add_p2p_connection(P2PInterface()) msg.locator.vHave = [int(node.getblockhash(i - 1), 16) for i in range(block_count, block_count - (MAX_LOCATOR_SZ), -1)] within_max_peer.send_message(msg) - if type(msg) == msg_getheaders: + if type(msg) is msg_getheaders: within_max_peer.wait_for_header(node.getbestblockhash()) else: within_max_peer.wait_for_block(int(node.getbestblockhash(), 16)) diff --git a/test/functional/test_framework/crypto/siphash.py b/test/functional/test_framework/crypto/siphash.py index 884dbcab4694..bd13b2c948d8 100644 --- a/test/functional/test_framework/crypto/siphash.py +++ b/test/functional/test_framework/crypto/siphash.py @@ -31,7 +31,7 @@ def siphash_round(v0, v1, v2, v3): def siphash(k0, k1, data): - assert type(data) == bytes + assert type(data) is bytes v0 = 0x736f6d6570736575 ^ k0 v1 = 0x646f72616e646f6d ^ k1 v2 = 0x6c7967656e657261 ^ k0 @@ -61,5 +61,5 @@ def siphash(k0, k1, data): def siphash256(k0, k1, num): - assert type(num) == int + assert type(num) is int return siphash(k0, k1, num.to_bytes(32, 'little'))