From e56fc7ce6a92eae7e80657d9f57a148cc002358d Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 4 Nov 2024 16:54:41 +0100 Subject: [PATCH 01/30] rpc: increase the defaults for -rpcthreads and -rpcworkqueue `rpcthreads` was introduced with a default of 4 in 2013 in 21eb5adadbe3110a8708f2570185566e1f137a49 `rpcworkqueue` was introduced with a default of 16 in 2015 in 40b556d3742a1f65d67e2d4c760d0b13fe8be5b7 Resolves: https://github.com/bitcoin/bitcoin/issues/29386 --- src/httpserver.h | 13 +++++++++++-- src/init.cpp | 2 +- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/httpserver.h b/src/httpserver.h index 33216a0119..6535dc6086 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -14,8 +14,17 @@ namespace util { class SignalInterrupt; } // namespace util -static const int DEFAULT_HTTP_THREADS=4; -static const int DEFAULT_HTTP_WORKQUEUE=16; +/** + * The default value for `-rpcthreads`. This number of threads will be created at startup. + */ +static const int DEFAULT_HTTP_THREADS=16; + +/** + * The default value for `-rpcworkqueue`. This is the maximum depth of the work queue, + * we don't allocate this number of work queue items upfront. + */ +static const int DEFAULT_HTTP_WORKQUEUE=64; + static const int DEFAULT_HTTP_SERVER_TIMEOUT=30; struct evhttp_request; diff --git a/src/init.cpp b/src/init.cpp index 72b4bba852..76bd3daba2 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -664,7 +664,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) argsman.AddArg("-rpcuser=", "Username for JSON-RPC connections", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::RPC); argsman.AddArg("-rpcwhitelist=", "Set a whitelist to filter incoming RPC calls for a specific user. The field comes in the format: :,,...,. If multiple whitelists are set for a given user, they are set-intersected. See -rpcwhitelistdefault documentation for information on default whitelist behavior.", ArgsManager::ALLOW_ANY, OptionsCategory::RPC); argsman.AddArg("-rpcwhitelistdefault", "Sets default behavior for rpc whitelisting. Unless rpcwhitelistdefault is set to 0, if any -rpcwhitelist is set, the rpc server acts as if all rpc users are subject to empty-unless-otherwise-specified whitelists. If rpcwhitelistdefault is set to 1 and no -rpcwhitelist is set, rpc server acts as if all rpc users are subject to empty whitelists.", ArgsManager::ALLOW_ANY, OptionsCategory::RPC); - argsman.AddArg("-rpcworkqueue=", strprintf("Set the depth of the work queue to service RPC calls (default: %d)", DEFAULT_HTTP_WORKQUEUE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::RPC); + argsman.AddArg("-rpcworkqueue=", strprintf("Set the maximum depth of the work queue to service RPC calls (default: %d)", DEFAULT_HTTP_WORKQUEUE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::RPC); argsman.AddArg("-server", "Accept command line and JSON-RPC commands", ArgsManager::ALLOW_ANY, OptionsCategory::RPC); if (can_listen_ipc) { argsman.AddArg("-ipcbind=
", "Bind to Unix socket address and listen for incoming connections. Valid address values are \"unix\" to listen on the default path, /node.sock, or \"unix:/custom/path\" to specify a custom path. Can be specified multiple times to listen on multiple paths. Default behavior is not to listen on any path. If relative paths are specified, they are interpreted relative to the network data directory. If paths include any parent directory components and the parent directories do not exist, they will be created.", ArgsManager::ALLOW_ANY, OptionsCategory::IPC); From a2c45ae5480a2ee643665d6ecaee9714a287a70e Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 7 Nov 2024 13:57:42 -0500 Subject: [PATCH 02/30] test: report failure during utf8 response decoding Useful for debugging issues. --- test/functional/test_framework/authproxy.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/functional/test_framework/authproxy.py b/test/functional/test_framework/authproxy.py index a357ae4d34..37fd5ae568 100644 --- a/test/functional/test_framework/authproxy.py +++ b/test/functional/test_framework/authproxy.py @@ -188,7 +188,12 @@ def _get_response(self): {'code': -342, 'message': 'non-JSON HTTP response with \'%i %s\' from server' % (http_response.status, http_response.reason)}, http_response.status) - responsedata = http_response.read().decode('utf8') + data = http_response.read() + try: + responsedata = data.decode('utf8') + except UnicodeDecodeError as e: + raise JSONRPCException({ + 'code': -342, 'message': f'Cannot decode response in utf8 format, content: {data}, exception: {e}'}) response = json.loads(responsedata, parse_float=decimal.Decimal) elapsed = time.time() - req_start_time if "error" in response and response["error"] is None: From 221c789e91696569fa34dbd162d26e98cf9cab64 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 21 Jul 2023 10:42:11 -0400 Subject: [PATCH 03/30] rpc: include verbose reject-details field in testmempoolaccept response --- src/rpc/mempool.cpp | 4 +++- test/functional/feature_cltv.py | 11 +++++++++-- test/functional/feature_dersig.py | 11 ++++++++--- test/functional/mempool_accept.py | 2 ++ test/functional/mempool_accept_wtxid.py | 6 ++++-- test/functional/rpc_packages.py | 15 ++++++++++----- 6 files changed, 36 insertions(+), 13 deletions(-) diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 6d462a7e85..52260d5794 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -146,7 +146,8 @@ static RPCHelpMan testmempoolaccept() {RPCResult{RPCResult::Type::STR_HEX, "", "transaction wtxid in hex"}, }}, }}, - {RPCResult::Type::STR, "reject-reason", /*optional=*/true, "Rejection string (only present when 'allowed' is false)"}, + {RPCResult::Type::STR, "reject-reason", /*optional=*/true, "Rejection reason (only present when 'allowed' is false)"}, + {RPCResult::Type::STR, "reject-details", /*optional=*/true, "Rejection details (only present when 'allowed' is false and rejection details exist)"}, }}, } }, @@ -245,6 +246,7 @@ static RPCHelpMan testmempoolaccept() result_inner.pushKV("reject-reason", "missing-inputs"); } else { result_inner.pushKV("reject-reason", state.GetRejectReason()); + result_inner.pushKV("reject-details", state.ToString()); } } rpc_result.push_back(std::move(result_inner)); diff --git a/test/functional/feature_cltv.py b/test/functional/feature_cltv.py index 557fcc7cea..60b3fb4e20 100755 --- a/test/functional/feature_cltv.py +++ b/test/functional/feature_cltv.py @@ -148,6 +148,10 @@ def run_test(self): # create and test one invalid tx per CLTV failure reason (5 in total) for i in range(5): spendtx = wallet.create_self_transfer()['tx'] + assert_equal(len(spendtx.vin), 1) + coin = spendtx.vin[0] + coin_txid = format(coin.prevout.hash, '064x') + coin_vout = coin.prevout.n cltv_invalidate(spendtx, i) expected_cltv_reject_reason = [ @@ -159,12 +163,15 @@ def run_test(self): ][i] # First we show that this tx is valid except for CLTV by getting it # rejected from the mempool for exactly that reason. + spendtx_txid = spendtx.hash + spendtx_wtxid = spendtx.getwtxid() assert_equal( [{ - 'txid': spendtx.hash, - 'wtxid': spendtx.getwtxid(), + 'txid': spendtx_txid, + 'wtxid': spendtx_wtxid, 'allowed': False, 'reject-reason': expected_cltv_reject_reason, + 'reject-details': expected_cltv_reject_reason + f", input 0 of {spendtx_txid} (wtxid {spendtx_wtxid}), spending {coin_txid}:{coin_vout}" }], self.nodes[0].testmempoolaccept(rawtxs=[spendtx.serialize().hex()], maxfeerate=0), ) diff --git a/test/functional/feature_dersig.py b/test/functional/feature_dersig.py index 1d18803f45..f4e3882918 100755 --- a/test/functional/feature_dersig.py +++ b/test/functional/feature_dersig.py @@ -109,18 +109,23 @@ def run_test(self): self.log.info("Test that transactions with non-DER signatures cannot appear in a block") block.nVersion = 4 - spendtx = self.create_tx(self.coinbase_txids[1]) + coin_txid = self.coinbase_txids[1] + spendtx = self.create_tx(coin_txid) unDERify(spendtx) spendtx.rehash() # First we show that this tx is valid except for DERSIG by getting it # rejected from the mempool for exactly that reason. + spendtx_txid = spendtx.hash + spendtx_wtxid = spendtx.getwtxid() assert_equal( [{ - 'txid': spendtx.hash, - 'wtxid': spendtx.getwtxid(), + 'txid': spendtx_txid, + 'wtxid': spendtx_wtxid, 'allowed': False, 'reject-reason': 'mandatory-script-verify-flag-failed (Non-canonical DER signature)', + 'reject-details': 'mandatory-script-verify-flag-failed (Non-canonical DER signature), ' + + f"input 0 of {spendtx_txid} (wtxid {spendtx_wtxid}), spending {coin_txid}:0" }], self.nodes[0].testmempoolaccept(rawtxs=[spendtx.serialize().hex()], maxfeerate=0), ) diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index ea609bbb34..27ecc3b4a8 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -67,6 +67,8 @@ def check_mempool_result(self, result_expected, *args, **kwargs): if "fees" in r: r["fees"].pop("effective-feerate") r["fees"].pop("effective-includes") + if "reject-details" in r: + r.pop("reject-details") assert_equal(result_expected, result_test) assert_equal(self.nodes[0].getmempoolinfo()['size'], self.mempool_size) # Must not change mempool state diff --git a/test/functional/mempool_accept_wtxid.py b/test/functional/mempool_accept_wtxid.py index e00e7b1ae4..f74d00e37c 100755 --- a/test/functional/mempool_accept_wtxid.py +++ b/test/functional/mempool_accept_wtxid.py @@ -100,13 +100,15 @@ def run_test(self): "txid": child_one_txid, "wtxid": child_one_wtxid, "allowed": False, - "reject-reason": "txn-already-in-mempool" + "reject-reason": "txn-already-in-mempool", + "reject-details": "txn-already-in-mempool" }]) assert_equal(node.testmempoolaccept([child_two.serialize().hex()])[0], { "txid": child_two_txid, "wtxid": child_two_wtxid, "allowed": False, - "reject-reason": "txn-same-nonwitness-data-in-mempool" + "reject-reason": "txn-same-nonwitness-data-in-mempool", + "reject-details": "txn-same-nonwitness-data-in-mempool" }) # sendrawtransaction will not throw but quits early when the exact same transaction is already in mempool diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index f5d42d0c34..a2f9210f94 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -110,17 +110,21 @@ def test_independent(self, coin): self.assert_testres_equal(package_bad, testres_bad) self.log.info("Check testmempoolaccept tells us when some transactions completed validation successfully") - tx_bad_sig_hex = node.createrawtransaction([{"txid": coin["txid"], "vout": 0}], + tx_bad_sig_hex = node.createrawtransaction([{"txid": coin["txid"], "vout": coin["vout"]}], {address : coin["amount"] - Decimal("0.0001")}) tx_bad_sig = tx_from_hex(tx_bad_sig_hex) testres_bad_sig = node.testmempoolaccept(self.independent_txns_hex + [tx_bad_sig_hex]) # By the time the signature for the last transaction is checked, all the other transactions # have been fully validated, which is why the node returns full validation results for all # transactions here but empty results in other cases. + tx_bad_sig_txid = tx_bad_sig.rehash() + tx_bad_sig_wtxid = tx_bad_sig.getwtxid() assert_equal(testres_bad_sig, self.independent_txns_testres + [{ - "txid": tx_bad_sig.rehash(), - "wtxid": tx_bad_sig.getwtxid(), "allowed": False, - "reject-reason": "mandatory-script-verify-flag-failed (Operation not valid with the current stack size)" + "txid": tx_bad_sig_txid, + "wtxid": tx_bad_sig_wtxid, "allowed": False, + "reject-reason": "mandatory-script-verify-flag-failed (Operation not valid with the current stack size)", + "reject-details": "mandatory-script-verify-flag-failed (Operation not valid with the current stack size), " + + f"input 0 of {tx_bad_sig_txid} (wtxid {tx_bad_sig_wtxid}), spending {coin['txid']}:{coin['vout']}" }]) self.log.info("Check testmempoolaccept reports txns in packages that exceed max feerate") @@ -304,7 +308,8 @@ def test_rbf(self): assert testres_rbf_single[0]["allowed"] testres_rbf_package = self.independent_txns_testres_blank + [{ "txid": replacement_tx["txid"], "wtxid": replacement_tx["wtxid"], "allowed": False, - "reject-reason": "bip125-replacement-disallowed" + "reject-reason": "bip125-replacement-disallowed", + "reject-details": "bip125-replacement-disallowed" }] self.assert_testres_equal(self.independent_txns_hex + [replacement_tx["hex"]], testres_rbf_package) From f9650e18ea6edb41c0136cc2ec3c7e0aba1bf83a Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Tue, 16 Apr 2024 15:34:24 -0400 Subject: [PATCH 04/30] rbf: remove unecessary newline at end of error string --- src/policy/rbf.cpp | 2 +- test/functional/mempool_package_rbf.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index b634dea561..bc76361fba 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -71,7 +71,7 @@ std::optional GetEntriesForConflicts(const CTransaction& tx, // descendants (i.e. if multiple conflicts share a descendant, it will be counted multiple // times), but we just want to be conservative to avoid doing too much work. if (nConflictingCount > MAX_REPLACEMENT_CANDIDATES) { - return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", + return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)", txid.ToString(), nConflictingCount, MAX_REPLACEMENT_CANDIDATES); diff --git a/test/functional/mempool_package_rbf.py b/test/functional/mempool_package_rbf.py index 7c0947a33d..4dc6f8fe36 100755 --- a/test/functional/mempool_package_rbf.py +++ b/test/functional/mempool_package_rbf.py @@ -219,7 +219,7 @@ def test_package_rbf_max_conflicts(self): package_child = self.wallet.create_self_transfer(fee_rate=child_feerate, utxo_to_spend=package_parent["new_utxos"][0]) pkg_results = node.submitpackage([package_parent["hex"], package_child["hex"]], maxfeerate=0) - assert_equal(f"package RBF failed: too many potential replacements, rejecting replacement {package_child['tx'].rehash()}; too many potential replacements (102 > 100)\n", pkg_results["package_msg"]) + assert_equal(f"package RBF failed: too many potential replacements, rejecting replacement {package_child['tx'].rehash()}; too many potential replacements (102 > 100)", pkg_results["package_msg"]) self.assert_mempool_contents(expected=expected_txns) # Make singleton tx to conflict with in next batch @@ -234,7 +234,7 @@ def test_package_rbf_max_conflicts(self): package_parent = self.wallet.create_self_transfer_multi(utxos_to_spend=double_spending_coins, fee_per_output=parent_fee_per_conflict) package_child = self.wallet.create_self_transfer(fee_rate=child_feerate, utxo_to_spend=package_parent["new_utxos"][0]) pkg_results = node.submitpackage([package_parent["hex"], package_child["hex"]], maxfeerate=0) - assert_equal(f"package RBF failed: too many potential replacements, rejecting replacement {package_child['tx'].rehash()}; too many potential replacements (101 > 100)\n", pkg_results["package_msg"]) + assert_equal(f"package RBF failed: too many potential replacements, rejecting replacement {package_child['tx'].rehash()}; too many potential replacements (101 > 100)", pkg_results["package_msg"]) self.assert_mempool_contents(expected=expected_txns) # Finally, evict MAX_REPLACEMENT_CANDIDATES From fa6e599cf9fbacb393ac17e0a1b3572f217817f0 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 2 Dec 2024 14:34:41 +0100 Subject: [PATCH 05/30] test: Call generate through test framework only --- test/functional/mempool_ephemeral_dust.py | 10 +++++----- test/functional/rpc_getdescriptoractivity.py | 4 ++-- test/functional/wallet_migration.py | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/test/functional/mempool_ephemeral_dust.py b/test/functional/mempool_ephemeral_dust.py index 56e4b404e4..10cacd9539 100755 --- a/test/functional/mempool_ephemeral_dust.py +++ b/test/functional/mempool_ephemeral_dust.py @@ -325,7 +325,7 @@ def test_reorgs(self): dusty_tx, _ = self.create_ephemeral_dust_package(tx_version=3) assert_raises_rpc_error(-26, "min relay fee not met", self.nodes[0].sendrawtransaction, dusty_tx["hex"]) - block_res = self.nodes[0].rpc.generateblock(self.wallet.get_address(), [dusty_tx["hex"]]) + block_res = self.generateblock(self.nodes[0], self.wallet.get_address(), [dusty_tx["hex"]], sync_fun=self.no_op) self.nodes[0].invalidateblock(block_res["hash"]) assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"]], sync=False) @@ -335,7 +335,7 @@ def test_reorgs(self): assert_raises_rpc_error(-26, "min relay fee not met", self.nodes[0].sendrawtransaction, sweep_tx["hex"]) # Mine the sweep then re-org, the sweep will not make it back in due to spend checks - block_res = self.nodes[0].rpc.generateblock(self.wallet.get_address(), [dusty_tx["hex"], sweep_tx["hex"]]) + block_res = self.generateblock(self.nodes[0], self.wallet.get_address(), [dusty_tx["hex"], sweep_tx["hex"]], sync_fun=self.no_op) self.nodes[0].invalidateblock(block_res["hash"]) assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"]], sync=False) @@ -344,7 +344,7 @@ def test_reorgs(self): self.add_output_to_create_multi_result(sweep_tx_2) assert_raises_rpc_error(-26, "min relay fee not met", self.nodes[0].sendrawtransaction, sweep_tx_2["hex"]) - reconsider_block_res = self.nodes[0].rpc.generateblock(self.wallet.get_address(), [dusty_tx["hex"], sweep_tx_2["hex"]]) + reconsider_block_res = self.generateblock(self.nodes[0], self.wallet.get_address(), [dusty_tx["hex"], sweep_tx_2["hex"]], sync_fun=self.no_op) self.nodes[0].invalidateblock(reconsider_block_res["hash"]) assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx_2["tx"]], sync=False) @@ -357,13 +357,13 @@ def test_reorgs(self): self.log.info("Test that ephemeral dust tx with fees or multi dust don't enter mempool via reorg") multi_dusty_tx, _ = self.create_ephemeral_dust_package(tx_version=3, num_dust_outputs=2) - block_res = self.nodes[0].rpc.generateblock(self.wallet.get_address(), [multi_dusty_tx["hex"]]) + block_res = self.generateblock(self.nodes[0], self.wallet.get_address(), [multi_dusty_tx["hex"]], sync_fun=self.no_op) self.nodes[0].invalidateblock(block_res["hash"]) assert_equal(self.nodes[0].getrawmempool(), []) # With fee and one dust dusty_fee_tx, _ = self.create_ephemeral_dust_package(tx_version=3, dust_tx_fee=1) - block_res = self.nodes[0].rpc.generateblock(self.wallet.get_address(), [dusty_fee_tx["hex"]]) + block_res = self.generateblock(self.nodes[0], self.wallet.get_address(), [dusty_fee_tx["hex"]], sync_fun=self.no_op) self.nodes[0].invalidateblock(block_res["hash"]) assert_equal(self.nodes[0].getrawmempool(), []) diff --git a/test/functional/rpc_getdescriptoractivity.py b/test/functional/rpc_getdescriptoractivity.py index 3efa1948b7..a0dc43718b 100755 --- a/test/functional/rpc_getdescriptoractivity.py +++ b/test/functional/rpc_getdescriptoractivity.py @@ -20,7 +20,7 @@ def run_test(self): node = self.nodes[0] wallet = MiniWallet(node) node.setmocktime(node.getblockheader(node.getbestblockhash())['time']) - wallet.generate(200, invalid_call=False) + self.generate(wallet, 200) self.test_no_activity(node) self.test_activity_in_block(node, wallet) @@ -195,7 +195,7 @@ def test_receive_then_spend(self, node, wallet): def test_no_address(self, node, wallet): raw_wallet = MiniWallet(self.nodes[0], mode=MiniWalletMode.RAW_P2PK) - raw_wallet.generate(100, invalid_call=False) + self.generate(raw_wallet, 100) no_addr_tx = raw_wallet.send_self_transfer(from_node=node) raw_desc = raw_wallet.get_descriptor() diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 5abb43e3b9..df9f3f0ba0 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -1,5 +1,5 @@ #!/usr/bin/env python3 -# Copyright (c) 2020-2022 The Bitcoin Core developers +# Copyright (c) 2020-present The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test Migrating a wallet from legacy to descriptor.""" @@ -461,7 +461,7 @@ def test_pk_coinbases(self): addr_info = wallet.getaddressinfo(addr) desc = descsum_create("pk(" + addr_info["pubkey"] + ")") - self.master_node.generatetodescriptor(1, desc, invalid_call=False) + self.generatetodescriptor(self.master_node, 1, desc) bals = wallet.getbalances() From f9cac635237142090271022164fa5d58e014493d Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 21 Jul 2023 10:42:41 -0400 Subject: [PATCH 06/30] test: cover testmempoolaccept debug-message in RBF test --- test/functional/feature_rbf.py | 60 +++++++++++++++++++++++++++------- 1 file changed, 48 insertions(+), 12 deletions(-) diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py index d9700e2ee2..ef2ecfab9e 100755 --- a/test/functional/feature_rbf.py +++ b/test/functional/feature_rbf.py @@ -103,14 +103,22 @@ def test_simple_doublespend(self): """Simple doublespend""" # we use MiniWallet to create a transaction template with inputs correctly set, # and modify the output (amount, scriptPubKey) according to our needs - tx = self.wallet.create_self_transfer()["tx"] + tx = self.wallet.create_self_transfer(fee_rate=Decimal("0.003"))["tx"] tx1a_txid = self.nodes[0].sendrawtransaction(tx.serialize().hex()) # Should fail because we haven't changed the fee tx.vout[0].scriptPubKey[-1] ^= 1 + tx.rehash() + tx_hex = tx.serialize().hex() # This will raise an exception due to insufficient fee - assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx.serialize().hex(), 0) + reject_reason = "insufficient fee" + reject_details = f"{reject_reason}, rejecting replacement {tx.hash}; new feerate 0.00300000 BTC/kvB <= old feerate 0.00300000 BTC/kvB" + res = self.nodes[0].testmempoolaccept(rawtxs=[tx_hex])[0] + assert_equal(res["reject-reason"], reject_reason) + assert_equal(res["reject-details"], reject_details) + assert_raises_rpc_error(-26, f"{reject_details}", self.nodes[0].sendrawtransaction, tx_hex, 0) + # Extra 0.1 BTC fee tx.vout[0].nValue -= int(0.1 * COIN) @@ -154,7 +162,14 @@ def test_doublespend_chain(self): dbl_tx_hex = dbl_tx.serialize().hex() # This will raise an exception due to insufficient fee - assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, dbl_tx_hex, 0) + reject_reason = "insufficient fee" + reject_details = f"{reject_reason}, rejecting replacement {dbl_tx.hash}, less fees than conflicting txs; 3.00 < 4.00" + res = self.nodes[0].testmempoolaccept(rawtxs=[dbl_tx_hex])[0] + assert_equal(res["reject-reason"], reject_reason) + assert_equal(res["reject-details"], reject_details) + assert_raises_rpc_error(-26, f"{reject_details}", self.nodes[0].sendrawtransaction, dbl_tx_hex, 0) + + # Accepted with sufficient fee dbl_tx.vout[0].nValue = int(0.1 * COIN) @@ -273,22 +288,30 @@ def test_spends_of_conflicting_outputs(self): utxo1 = self.make_utxo(self.nodes[0], int(1.2 * COIN)) utxo2 = self.make_utxo(self.nodes[0], 3 * COIN) - tx1a_utxo = self.wallet.send_self_transfer( + tx1a = self.wallet.send_self_transfer( from_node=self.nodes[0], utxo_to_spend=utxo1, sequence=0, fee=Decimal("0.1"), - )["new_utxo"] + ) + tx1a_utxo = tx1a["new_utxo"] # Direct spend an output of the transaction we're replacing. - tx2_hex = self.wallet.create_self_transfer_multi( + tx2 = self.wallet.create_self_transfer_multi( utxos_to_spend=[utxo1, utxo2, tx1a_utxo], sequence=0, amount_per_output=int(COIN * tx1a_utxo["value"]), - )["hex"] + )["tx"] + tx2_hex = tx2.serialize().hex() # This will raise an exception - assert_raises_rpc_error(-26, "bad-txns-spends-conflicting-tx", self.nodes[0].sendrawtransaction, tx2_hex, 0) + reject_reason = "bad-txns-spends-conflicting-tx" + reject_details = f"{reject_reason}, {tx2.hash} spends conflicting transaction {tx1a['tx'].hash}" + res = self.nodes[0].testmempoolaccept(rawtxs=[tx2_hex])[0] + assert_equal(res["reject-reason"], reject_reason) + assert_equal(res["reject-details"], reject_details) + assert_raises_rpc_error(-26, f"{reject_details}", self.nodes[0].sendrawtransaction, tx2_hex, 0) + # Spend tx1a's output to test the indirect case. tx1b_utxo = self.wallet.send_self_transfer( @@ -319,14 +342,21 @@ def test_new_unconfirmed_inputs(self): fee=Decimal("0.1"), ) - tx2_hex = self.wallet.create_self_transfer_multi( + tx2 = self.wallet.create_self_transfer_multi( utxos_to_spend=[confirmed_utxo, unconfirmed_utxo], sequence=0, amount_per_output=1 * COIN, - )["hex"] + )["tx"] + tx2_hex = tx2.serialize().hex() # This will raise an exception - assert_raises_rpc_error(-26, "replacement-adds-unconfirmed", self.nodes[0].sendrawtransaction, tx2_hex, 0) + reject_reason = "replacement-adds-unconfirmed" + reject_details = f"{reject_reason}, replacement {tx2.hash} adds unconfirmed input, idx 1" + res = self.nodes[0].testmempoolaccept(rawtxs=[tx2_hex])[0] + assert_equal(res["reject-reason"], reject_reason) + assert_equal(res["reject-details"], reject_details) + assert_raises_rpc_error(-26, f"{reject_details}", self.nodes[0].sendrawtransaction, tx2_hex, 0) + def test_too_many_replacements(self): """Replacements that evict too many transactions are rejected""" @@ -368,7 +398,13 @@ def test_too_many_replacements(self): double_tx_hex = double_tx.serialize().hex() # This will raise an exception - assert_raises_rpc_error(-26, "too many potential replacements", self.nodes[0].sendrawtransaction, double_tx_hex, 0) + reject_reason = "too many potential replacements" + reject_details = f"{reject_reason}, rejecting replacement {double_tx.hash}; too many potential replacements ({MAX_REPLACEMENT_LIMIT + 1} > {MAX_REPLACEMENT_LIMIT})" + res = self.nodes[0].testmempoolaccept(rawtxs=[double_tx_hex])[0] + assert_equal(res["reject-reason"], reject_reason) + assert_equal(res["reject-details"], reject_details) + assert_raises_rpc_error(-26, f"{reject_details}", self.nodes[0].sendrawtransaction, double_tx_hex, 0) + # If we remove an input, it should pass double_tx.vin.pop() From b6f0593f43304f4ff31e8b68558ceeb1b588403c Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 21 Jul 2023 10:45:14 -0400 Subject: [PATCH 07/30] doc: add release note about testmempoolaccept debug-message --- doc/release-notes-28121.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 doc/release-notes-28121.md diff --git a/doc/release-notes-28121.md b/doc/release-notes-28121.md new file mode 100644 index 0000000000..911b7c5620 --- /dev/null +++ b/doc/release-notes-28121.md @@ -0,0 +1,2 @@ +The RPC `testmempoolaccept` response now includes a "reject-details" field in some cases, +similar to the complete error messages returned by `sendrawtransaction` (#28121) \ No newline at end of file From fa0998f0a028161fe7264d0e81af36ffdcb43384 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 11 Dec 2024 16:31:55 +0100 Subject: [PATCH 08/30] test: Avoid intermittent error in assert_equal(pruneheight_new, 248) --- test/functional/feature_index_prune.py | 49 +++++++++++++------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/test/functional/feature_index_prune.py b/test/functional/feature_index_prune.py index 030bf51ed8..191c6466df 100755 --- a/test/functional/feature_index_prune.py +++ b/test/functional/feature_index_prune.py @@ -1,8 +1,9 @@ #!/usr/bin/env python3 -# Copyright (c) 2020-2022 The Bitcoin Core developers +# Copyright (c) 2020-present The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test indices in conjunction with prune.""" +import concurrent.futures import os from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( @@ -19,9 +20,25 @@ def set_test_params(self): ["-fastprune", "-prune=1", "-blockfilterindex=1"], ["-fastprune", "-prune=1", "-coinstatsindex=1"], ["-fastprune", "-prune=1", "-blockfilterindex=1", "-coinstatsindex=1"], - [] + [], ] + def setup_network(self): + self.setup_nodes() # No P2P connection, so that linear_sync works + + def linear_sync(self, node_from, *, height_from=None): + # Linear sync over RPC, because P2P sync may not be linear + to_height = node_from.getblockcount() + if height_from is None: + height_from = min([n.getblockcount() for n in self.nodes]) + 1 + with concurrent.futures.ThreadPoolExecutor(max_workers=self.num_nodes) as rpc_threads: + for i in range(height_from, to_height + 1): + b = node_from.getblock(blockhash=node_from.getblockhash(i), verbosity=0) + list(rpc_threads.map(lambda n: n.submitblock(b), self.nodes)) + + def generate(self, node, num_blocks, sync_fun=None): + return super().generate(node, num_blocks, sync_fun=sync_fun or (lambda: self.linear_sync(node))) + def sync_index(self, height): expected_filter = { 'basic block filter index': {'synced': True, 'best_block_height': height}, @@ -36,22 +53,9 @@ def sync_index(self, height): expected = {**expected_filter, **expected_stats} self.wait_until(lambda: self.nodes[2].getindexinfo() == expected) - def reconnect_nodes(self): - self.connect_nodes(0,1) - self.connect_nodes(0,2) - self.connect_nodes(0,3) - - def mine_batches(self, blocks): - n = blocks // 250 - for _ in range(n): - self.generate(self.nodes[0], 250) - self.generate(self.nodes[0], blocks % 250) - self.sync_blocks() - def restart_without_indices(self): for i in range(3): self.restart_node(i, extra_args=["-fastprune", "-prune=1"]) - self.reconnect_nodes() def run_test(self): filter_nodes = [self.nodes[0], self.nodes[2]] @@ -65,7 +69,7 @@ def run_test(self): for node in stats_nodes: assert node.gettxoutsetinfo(hash_type="muhash", hash_or_height=tip)['muhash'] - self.mine_batches(500) + self.generate(self.nodes[0], 500) self.sync_index(height=700) self.log.info("prune some blocks") @@ -104,7 +108,7 @@ def run_test(self): msg = "Querying specific block heights requires coinstatsindex" assert_raises_rpc_error(-8, msg, node.gettxoutsetinfo, "muhash", height_hash) - self.mine_batches(749) + self.generate(self.nodes[0], 749) self.log.info("prune exactly up to the indices best blocks while the indices are disabled") for i in range(3): @@ -118,7 +122,7 @@ def run_test(self): self.log.info("prune further than the indices best blocks while the indices are disabled") self.restart_without_indices() - self.mine_batches(1000) + self.generate(self.nodes[0], 1000) for i in range(3): pruneheight_3 = self.nodes[i].pruneblockchain(2000) @@ -134,12 +138,10 @@ def run_test(self): self.log.info("make sure the nodes start again with the indices and an additional -reindex arg") for i in range(3): - restart_args = self.extra_args[i]+["-reindex"] + restart_args = self.extra_args[i] + ["-reindex"] self.restart_node(i, extra_args=restart_args) - # The nodes need to be reconnected to the non-pruning node upon restart, otherwise they will be stuck - self.connect_nodes(i, 3) - self.sync_blocks(timeout=300) + self.linear_sync(self.nodes[3]) self.sync_index(height=2500) for node in self.nodes[:2]: @@ -150,8 +152,7 @@ def run_test(self): self.log.info("ensure that prune locks don't prevent indices from failing in a reorg scenario") with self.nodes[0].assert_debug_log(['basic block filter index prune lock moved back to 2480']): self.nodes[3].invalidateblock(self.nodes[0].getblockhash(2480)) - self.generate(self.nodes[3], 30) - self.sync_blocks() + self.generate(self.nodes[3], 30, sync_fun=lambda: self.linear_sync(self.nodes[3], height_from=2480)) if __name__ == '__main__': From b9766c9977e58a9ebc358d9879576376e76a72b1 Mon Sep 17 00:00:00 2001 From: yancy Date: Fri, 13 Dec 2024 19:55:45 -0600 Subject: [PATCH 09/30] Remove unused variable assignment The variable is conditionally assigned toward the end of the loop and not used after. It's then set back to its default value at the beginning of the loop. --- src/wallet/coinselection.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index cee558088f..6e6d7e053b 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -478,7 +478,6 @@ util::Result CoinGrinder(std::vector& utxo_pool, c // Neither adding to the current selection nor exploring the omission branch of the last selected UTXO can // find any solutions. Redirect to exploring the Omission branch of the penultimate selected UTXO (i.e. // set `next_utxo` to one after the penultimate selected, then deselect the last two selected UTXOs) - should_cut = false; deselect_last(); should_shift = true; } From fa83bec78ef3e86445e522afa396c97b58eb1902 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 18 Dec 2024 11:02:03 +0100 Subject: [PATCH 10/30] refactor: Allow std::byte in Read(LE/BE) --- src/crypto/chacha20.cpp | 18 ++++++++--------- src/crypto/common.h | 43 ++++++++++++++++++++++++++++------------- 2 files changed, 39 insertions(+), 22 deletions(-) diff --git a/src/crypto/chacha20.cpp b/src/crypto/chacha20.cpp index 4feed862cb..e756e0431e 100644 --- a/src/crypto/chacha20.cpp +++ b/src/crypto/chacha20.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2022 The Bitcoin Core developers +// Copyright (c) 2017-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -25,14 +25,14 @@ void ChaCha20Aligned::SetKey(Span key) noexcept { assert(key.size() == KEYLEN); - input[0] = ReadLE32(UCharCast(key.data() + 0)); - input[1] = ReadLE32(UCharCast(key.data() + 4)); - input[2] = ReadLE32(UCharCast(key.data() + 8)); - input[3] = ReadLE32(UCharCast(key.data() + 12)); - input[4] = ReadLE32(UCharCast(key.data() + 16)); - input[5] = ReadLE32(UCharCast(key.data() + 20)); - input[6] = ReadLE32(UCharCast(key.data() + 24)); - input[7] = ReadLE32(UCharCast(key.data() + 28)); + input[0] = ReadLE32(key.data() + 0); + input[1] = ReadLE32(key.data() + 4); + input[2] = ReadLE32(key.data() + 8); + input[3] = ReadLE32(key.data() + 12); + input[4] = ReadLE32(key.data() + 16); + input[5] = ReadLE32(key.data() + 20); + input[6] = ReadLE32(key.data() + 24); + input[7] = ReadLE32(key.data() + 28); input[8] = 0; input[9] = 0; input[10] = 0; diff --git a/src/crypto/common.h b/src/crypto/common.h index d45459b1f6..f151cbb625 100644 --- a/src/crypto/common.h +++ b/src/crypto/common.h @@ -1,4 +1,4 @@ -// Copyright (c) 2014-2020 The Bitcoin Core developers +// Copyright (c) 2014-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -7,82 +7,99 @@ #include +#include +#include #include #include -uint16_t static inline ReadLE16(const unsigned char* ptr) +template +concept ByteType = std::same_as || std::same_as; + +template +inline uint16_t ReadLE16(const B* ptr) { uint16_t x; memcpy(&x, ptr, 2); return le16toh_internal(x); } -uint32_t static inline ReadLE32(const unsigned char* ptr) +template +inline uint32_t ReadLE32(const B* ptr) { uint32_t x; memcpy(&x, ptr, 4); return le32toh_internal(x); } -uint64_t static inline ReadLE64(const unsigned char* ptr) +template +inline uint64_t ReadLE64(const B* ptr) { uint64_t x; memcpy(&x, ptr, 8); return le64toh_internal(x); } -void static inline WriteLE16(unsigned char* ptr, uint16_t x) +template +inline void WriteLE16(B* ptr, uint16_t x) { uint16_t v = htole16_internal(x); memcpy(ptr, &v, 2); } -void static inline WriteLE32(unsigned char* ptr, uint32_t x) +template +inline void WriteLE32(B* ptr, uint32_t x) { uint32_t v = htole32_internal(x); memcpy(ptr, &v, 4); } -void static inline WriteLE64(unsigned char* ptr, uint64_t x) +template +inline void WriteLE64(B* ptr, uint64_t x) { uint64_t v = htole64_internal(x); memcpy(ptr, &v, 8); } -uint16_t static inline ReadBE16(const unsigned char* ptr) +template +inline uint16_t ReadBE16(const B* ptr) { uint16_t x; memcpy(&x, ptr, 2); return be16toh_internal(x); } -uint32_t static inline ReadBE32(const unsigned char* ptr) +template +inline uint32_t ReadBE32(const B* ptr) { uint32_t x; memcpy(&x, ptr, 4); return be32toh_internal(x); } -uint64_t static inline ReadBE64(const unsigned char* ptr) +template +inline uint64_t ReadBE64(const B* ptr) { uint64_t x; memcpy(&x, ptr, 8); return be64toh_internal(x); } -void static inline WriteBE16(unsigned char* ptr, uint16_t x) +template +inline void WriteBE16(B* ptr, uint16_t x) { uint16_t v = htobe16_internal(x); memcpy(ptr, &v, 2); } -void static inline WriteBE32(unsigned char* ptr, uint32_t x) +template +inline void WriteBE32(B* ptr, uint32_t x) { uint32_t v = htobe32_internal(x); memcpy(ptr, &v, 4); } -void static inline WriteBE64(unsigned char* ptr, uint64_t x) +template +inline void WriteBE64(B* ptr, uint64_t x) { uint64_t v = htobe64_internal(x); memcpy(ptr, &v, 8); From fac3a782eaf3fa5f12cd908ef6dbc874d4b0e2ba Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 6 Feb 2024 19:47:16 +0100 Subject: [PATCH 11/30] refactor: Avoid needless, unsafe c-style cast --- src/base58.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/base58.cpp b/src/base58.cpp index f9165ed55f..cab99f0cb5 100644 --- a/src/base58.cpp +++ b/src/base58.cpp @@ -139,7 +139,7 @@ std::string EncodeBase58Check(Span input) // add 4-byte hash check to the end std::vector vch(input.begin(), input.end()); uint256 hash = Hash(vch); - vch.insert(vch.end(), (unsigned char*)&hash, (unsigned char*)&hash + 4); + vch.insert(vch.end(), hash.data(), hash.data() + 4); return EncodeBase58(vch); } From facc4f120b067af6f94f3125cecc9dafff3e5d57 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 17 Dec 2024 21:34:22 +0100 Subject: [PATCH 12/30] refactor: Replace fwd-decl with proper include This is fine, because the span.h include is lightweight and a proper include will be needed anyway when switching to std::span. --- src/script/solver.h | 2 +- src/test/util/net.h | 4 +--- src/util/hasher.h | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/script/solver.h b/src/script/solver.h index 5a7b685a6f..5b945477c9 100644 --- a/src/script/solver.h +++ b/src/script/solver.h @@ -10,6 +10,7 @@ #include #include