From fa327c77e34e0cfb7994842c23f539ab11bf5d3b Mon Sep 17 00:00:00 2001 From: marcofleon Date: Mon, 4 Nov 2024 11:32:23 +0000 Subject: [PATCH 01/40] util: Add ConsumeArithUInt256InRange fuzzing helper --- src/test/fuzz/util.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index de097457301f8..8e2b8639c2895 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -180,6 +180,22 @@ template return UintToArith256(ConsumeUInt256(fuzzed_data_provider)); } +[[nodiscard]] inline arith_uint256 ConsumeArithUInt256InRange(FuzzedDataProvider& fuzzed_data_provider, const arith_uint256& min, const arith_uint256& max) noexcept +{ + assert(min <= max); + const arith_uint256 range = max - min; + const arith_uint256 value = ConsumeArithUInt256(fuzzed_data_provider); + arith_uint256 result = value; + // Avoid division by 0, in case range + 1 results in overflow. + if (range != ~arith_uint256(0)) { + const arith_uint256 quotient = value / (range + 1); + result = value - (quotient * (range + 1)); + } + result += min; + assert(result >= min && result <= max); + return result; +} + [[nodiscard]] std::map ConsumeCoins(FuzzedDataProvider& fuzzed_data_provider) noexcept; [[nodiscard]] CTxDestination ConsumeTxDestination(FuzzedDataProvider& fuzzed_data_provider) noexcept; From a6ca8f324396522e9748c9a7bbefb3bf1c74a436 Mon Sep 17 00:00:00 2001 From: marcofleon Date: Mon, 4 Nov 2024 12:31:44 +0000 Subject: [PATCH 02/40] fuzz: Fix difficulty target generation in p2p_headers_presync The hardcoded nBits range would occasionally produce values for the difficulty target that were too low, causing the total work of the test chain to exceed MinimumChainWork. This fix uses ConsumeArithUInt256InRange to properly generate targets that will produce header chains with less work than MinimumChainWork. --- src/test/fuzz/p2p_headers_presync.cpp | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/test/fuzz/p2p_headers_presync.cpp b/src/test/fuzz/p2p_headers_presync.cpp index 2670aa8ee4e1a..5c9ce8723df49 100644 --- a/src/test/fuzz/p2p_headers_presync.cpp +++ b/src/test/fuzz/p2p_headers_presync.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -89,10 +90,23 @@ CBlockHeader ConsumeHeader(FuzzedDataProvider& fuzzed_data_provider, const uint2 { CBlockHeader header; header.nNonce = 0; - // Either use the previous difficulty or let the fuzzer choose - header.nBits = fuzzed_data_provider.ConsumeBool() ? - prev_nbits : - fuzzed_data_provider.ConsumeIntegralInRange(0x17058EBE, 0x1D00FFFF); + // Either use the previous difficulty or let the fuzzer choose. The upper target in the + // range comes from the bits value of the genesis block, which is 0x1d00ffff. The lower + // target comes from the bits value of mainnet block 840000, which is 0x17034219. + // Calling lower_target.SetCompact(0x17034219) and upper_target.SetCompact(0x1d00ffff) + // should return the values below. + // + // RPC commands to verify: + // getblockheader 000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f + // getblockheader 0000000000000000000320283a032748cef8227873ff4872689bf23f1cda83a5 + if (fuzzed_data_provider.ConsumeBool()) { + header.nBits = prev_nbits; + } else { + arith_uint256 lower_target = UintToArith256(uint256{"0000000000000000000342190000000000000000000000000000000000000000"}); + arith_uint256 upper_target = UintToArith256(uint256{"00000000ffff0000000000000000000000000000000000000000000000000000"}); + arith_uint256 target = ConsumeArithUInt256InRange(fuzzed_data_provider, lower_target, upper_target); + header.nBits = target.GetCompact(); + } header.nTime = ConsumeTime(fuzzed_data_provider); header.hashPrevBlock = prev_hash; header.nVersion = fuzzed_data_provider.ConsumeIntegral(); From 16c87d91fd4d7709fa9d8824d5b641ef71821931 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Thu, 12 Sep 2024 14:00:08 +0200 Subject: [PATCH 03/40] test: Introduce ensure_for helper --- test/functional/feature_assumeutxo.py | 5 ++--- test/functional/feature_minchainwork.py | 20 +++++++++----------- test/functional/interface_zmq.py | 5 ++--- test/functional/mempool_unbroadcast.py | 11 ++++++----- test/functional/p2p_segwit.py | 7 +++---- test/functional/test_framework/util.py | 21 +++++++++++++++++++++ test/functional/wallet_multiwallet.py | 5 ++--- 7 files changed, 45 insertions(+), 29 deletions(-) diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py index 57f34264ec2fb..08d47d1e87f45 100755 --- a/test/functional/feature_assumeutxo.py +++ b/test/functional/feature_assumeutxo.py @@ -9,7 +9,6 @@ The assumeutxo value generated and used here is committed to in `CRegTestParams::m_assumeutxo_data` in `src/kernel/chainparams.cpp`. """ -import time from shutil import rmtree from dataclasses import dataclass @@ -31,6 +30,7 @@ assert_approx, assert_equal, assert_raises_rpc_error, + ensure_for, sha256sum_file, try_rpc, ) @@ -305,8 +305,7 @@ def test_sync_from_assumeutxo_node(self, snapshot): # If it does request such blocks, the snapshot_node will ignore requests it cannot fulfill, causing the ibd_node # to stall. This stall could last for up to 10 min, ultimately resulting in an abrupt disconnection due to the # ibd_node's perceived unresponsiveness. - time.sleep(3) # Sleep here because we can't detect when a node avoids requesting blocks from other peer. - assert_equal(len(ibd_node.getpeerinfo()[0]['inflight']), 0) + ensure_for(duration=3, f=lambda: len(ibd_node.getpeerinfo()[0]['inflight']) == 0) # Now disconnect nodes and finish background chain sync self.disconnect_nodes(ibd_node.index, snapshot_node.index) diff --git a/test/functional/feature_minchainwork.py b/test/functional/feature_minchainwork.py index 34228f6f380d6..d6654ef8ed904 100755 --- a/test/functional/feature_minchainwork.py +++ b/test/functional/feature_minchainwork.py @@ -19,7 +19,10 @@ from test_framework.p2p import P2PInterface, msg_getheaders from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import assert_equal +from test_framework.util import ( + assert_equal, + ensure_for, +) # 2 hashes required per regtest block (with no difficulty adjustment) REGTEST_WORK_PER_BLOCK = 2 @@ -58,18 +61,14 @@ def run_test(self): hashes = self.generate(self.nodes[0], num_blocks_to_generate, sync_fun=self.no_op) self.log.info(f"Node0 current chain work: {self.nodes[0].getblockheader(hashes[-1])['chainwork']}") - - # Sleep a few seconds and verify that node2 didn't get any new blocks - # or headers. We sleep, rather than sync_blocks(node0, node1) because - # it's reasonable either way for node1 to get the blocks, or not get - # them (since they're below node1's minchainwork). - time.sleep(3) - self.log.info("Verifying node 2 has no more blocks than before") self.log.info(f"Blockcounts: {[n.getblockcount() for n in self.nodes]}") # Node2 shouldn't have any new headers yet, because node1 should not # have relayed anything. - assert_equal(len(self.nodes[2].getchaintips()), 1) + # We wait 3 seconds, rather than sync_blocks(node0, node1) because + # it's reasonable either way for node1 to get the blocks, or not get + # them (since they're below node1's minchainwork). + ensure_for(duration=3, f=lambda: len(self.nodes[2].getchaintips()) == 1) assert_equal(self.nodes[2].getchaintips()[0]['height'], 0) assert self.nodes[1].getbestblockhash() != self.nodes[0].getbestblockhash() @@ -81,8 +80,7 @@ def run_test(self): msg.locator.vHave = [int(self.nodes[2].getbestblockhash(), 16)] msg.hashstop = 0 peer.send_and_ping(msg) - time.sleep(5) - assert "headers" not in peer.last_message or len(peer.last_message["headers"].headers) == 0 + ensure_for(duration=5, f=lambda: "headers" not in peer.last_message or len(peer.last_message["headers"].headers) == 0) self.log.info("Generating one more block") self.generate(self.nodes[0], 1) diff --git a/test/functional/interface_zmq.py b/test/functional/interface_zmq.py index b960f40ccc702..74860cd0b8c23 100755 --- a/test/functional/interface_zmq.py +++ b/test/functional/interface_zmq.py @@ -6,7 +6,6 @@ import os import struct import tempfile -from time import sleep from io import BytesIO from test_framework.address import ( @@ -27,6 +26,7 @@ from test_framework.util import ( assert_equal, assert_raises_rpc_error, + ensure_for, p2p_port, ) from test_framework.wallet import ( @@ -394,11 +394,10 @@ def test_sequence(self): block_count = self.nodes[0].getblockcount() best_hash = self.nodes[0].getbestblockhash() self.nodes[0].invalidateblock(best_hash) - sleep(2) # Bit of room to make sure transaction things happened # Make sure getrawmempool mempool_sequence results aren't "queued" but immediately reflective # of the time they were gathered. - assert self.nodes[0].getrawmempool(mempool_sequence=True)["mempool_sequence"] > seq_num + ensure_for(duration=2, f=lambda: self.nodes[0].getrawmempool(mempool_sequence=True)["mempool_sequence"] > seq_num) assert_equal((best_hash, "D", None), seq.receive_sequence()) assert_equal((rbf_txid, "A", seq_num), seq.receive_sequence()) diff --git a/test/functional/mempool_unbroadcast.py b/test/functional/mempool_unbroadcast.py index 7c96b4b570d4b..b9692d2e21d17 100755 --- a/test/functional/mempool_unbroadcast.py +++ b/test/functional/mempool_unbroadcast.py @@ -5,11 +5,12 @@ """Test that the mempool ensures transaction delivery by periodically sending to peers until a GETDATA is received.""" -import time - from test_framework.p2p import P2PTxInvStore from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import assert_equal +from test_framework.util import ( + assert_equal, + ensure_for, +) from test_framework.wallet import MiniWallet MAX_INITIAL_BROADCAST_DELAY = 15 * 60 # 15 minutes in seconds @@ -83,8 +84,8 @@ def test_broadcast(self): conn = node.add_p2p_connection(P2PTxInvStore()) node.mockscheduler(MAX_INITIAL_BROADCAST_DELAY) - time.sleep(2) # allow sufficient time for possibility of broadcast - assert_equal(len(conn.get_invs()), 0) + # allow sufficient time for possibility of broadcast + ensure_for(duration=2, f=lambda: len(conn.get_invs()) == 0) self.disconnect_nodes(0, 1) node.disconnect_p2ps() diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index 9be53d2ab8b61..ac35762427e4b 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -5,7 +5,6 @@ """Test segwit transactions and blocks on P2P network.""" from decimal import Decimal import random -import time from test_framework.blocktools import ( WITNESS_COMMITMENT_HEADER, @@ -83,8 +82,9 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, - softfork_active, assert_raises_rpc_error, + ensure_for, + softfork_active, ) from test_framework.wallet import MiniWallet from test_framework.wallet_util import generate_keypair @@ -184,8 +184,7 @@ def announce_tx_and_wait_for_getdata(self, tx, success=True, use_wtxid=False): else: self.wait_for_getdata([tx.sha256]) else: - time.sleep(5) - assert not self.last_message.get("getdata") + ensure_for(duration=5, f=lambda: not self.last_message.get("getdata")) def announce_block_and_wait_for_getdata(self, block, use_header, timeout=60): with p2p_lock: diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index ce68de7eaafba..c2fc9f2b0afa9 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -268,6 +268,27 @@ def satoshi_round(amount: Union[int, float, str], *, rounding: str) -> Decimal: return Decimal(amount).quantize(SATOSHI_PRECISION, rounding=rounding) +def ensure_for(*, duration, f, check_interval=0.2): + """Check if the predicate keeps returning True for duration. + + check_interval can be used to configure the wait time between checks. + Setting check_interval to 0 will allow to have two checks: one in the + beginning and one after duration. + """ + # If check_interval is 0 or negative or larger than duration, we fall back + # to checking once in the beginning and once at the end of duration + if check_interval <= 0 or check_interval > duration: + check_interval = duration + time_end = time.time() + duration + predicate_source = "''''\n" + inspect.getsource(f) + "'''" + while True: + if not f(): + raise AssertionError(f"Predicate {predicate_source} became false within {duration} seconds") + if time.time() > time_end: + return + time.sleep(check_interval) + + def wait_until_helper_internal(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, timeout_factor=1.0): """Sleep until the predicate resolves to be True. diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index 156f4279b4381..a25c96f7a7842 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -12,7 +12,6 @@ import platform import shutil import stat -import time from test_framework.authproxy import JSONRPCException from test_framework.blocktools import COINBASE_MATURITY @@ -21,6 +20,7 @@ from test_framework.util import ( assert_equal, assert_raises_rpc_error, + ensure_for, get_rpc_proxy, ) @@ -373,8 +373,7 @@ def wallet_file(name): w2.encryptwallet('test') w2.walletpassphrase('test', 1) w2.unloadwallet() - time.sleep(1.1) - assert 'w2' not in self.nodes[0].listwallets() + ensure_for(duration=1.1, f=lambda: 'w2' not in self.nodes[0].listwallets()) # Successfully unload all wallets for wallet_name in self.nodes[0].listwallets(): From 5468a23eb9a3fd2b0c08dbca69fe3df58af42530 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Sat, 21 Sep 2024 21:23:14 +0200 Subject: [PATCH 04/40] test: Add check_interval parameter to wait_until This also replaces two sleep calls in functional tests with wait_until --- test/functional/test_framework/p2p.py | 4 ++-- test/functional/test_framework/test_framework.py | 4 ++-- test/functional/test_framework/test_node.py | 4 ++-- test/functional/test_framework/util.py | 4 ++-- test/functional/wallet_inactive_hdchains.py | 8 ++++---- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py index 4f1265eb54889..8ccd3e44a64d2 100755 --- a/test/functional/test_framework/p2p.py +++ b/test/functional/test_framework/p2p.py @@ -579,13 +579,13 @@ def on_version(self, message): # Connection helper methods - def wait_until(self, test_function_in, *, timeout=60, check_connected=True): + def wait_until(self, test_function_in, *, timeout=60, check_connected=True, check_interval=0.05): def test_function(): if check_connected: assert self.is_connected return test_function_in() - wait_until_helper_internal(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor) + wait_until_helper_internal(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor, check_interval=check_interval) def wait_for_connect(self, *, timeout=60): test_function = lambda: self.is_connected diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 49212eb0195d7..19003d44f230e 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -787,8 +787,8 @@ def sync_all(self, nodes=None): self.sync_blocks(nodes) self.sync_mempools(nodes) - def wait_until(self, test_function, timeout=60): - return wait_until_helper_internal(test_function, timeout=timeout, timeout_factor=self.options.timeout_factor) + def wait_until(self, test_function, timeout=60, check_interval=0.05): + return wait_until_helper_internal(test_function, timeout=timeout, timeout_factor=self.options.timeout_factor, check_interval=check_interval) # Private helper methods. These should not be accessed by the subclass test scripts. diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 60ca9269a5b9f..bf1f2112b2595 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -838,8 +838,8 @@ def bumpmocktime(self, seconds): self.mocktime += seconds self.setmocktime(self.mocktime) - def wait_until(self, test_function, timeout=60): - return wait_until_helper_internal(test_function, timeout=timeout, timeout_factor=self.timeout_factor) + def wait_until(self, test_function, timeout=60, check_interval=0.05): + return wait_until_helper_internal(test_function, timeout=timeout, timeout_factor=self.timeout_factor, check_interval=check_interval) class TestNodeCLIAttr: diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index c2fc9f2b0afa9..6e88a50cd7235 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -289,7 +289,7 @@ def ensure_for(*, duration, f, check_interval=0.2): time.sleep(check_interval) -def wait_until_helper_internal(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, timeout_factor=1.0): +def wait_until_helper_internal(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, timeout_factor=1.0, check_interval=0.05): """Sleep until the predicate resolves to be True. Warning: Note that this method is not recommended to be used in tests as it is @@ -313,7 +313,7 @@ def wait_until_helper_internal(predicate, *, attempts=float('inf'), timeout=floa if predicate(): return attempt += 1 - time.sleep(0.05) + time.sleep(check_interval) # Print the cause of the timeout predicate_source = "''''\n" + inspect.getsource(predicate) + "'''" diff --git a/test/functional/wallet_inactive_hdchains.py b/test/functional/wallet_inactive_hdchains.py index 3b0c09c02bed0..1a2ea5b9ff1e6 100755 --- a/test/functional/wallet_inactive_hdchains.py +++ b/test/functional/wallet_inactive_hdchains.py @@ -6,7 +6,6 @@ Test Inactive HD Chains. """ import shutil -import time from test_framework.authproxy import JSONRPCException from test_framework.test_framework import BitcoinTestFramework @@ -75,12 +74,13 @@ def do_inactive_test(self, base_wallet, test_wallet, encrypt=False): self.generate(self.nodes[0], 1) # Wait for the test wallet to see the transaction - while True: + def is_tx_available(txid): try: test_wallet.gettransaction(txid) - break + return True except JSONRPCException: - time.sleep(0.1) + return False + self.nodes[0].wait_until(lambda: is_tx_available(txid), timeout=10, check_interval=0.1) if encrypt: # The test wallet will not be able to generate the topped up keypool From 111465d72dd35e42361fc2a089036f652417ed37 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Sun, 29 Sep 2024 22:24:31 +0200 Subject: [PATCH 05/40] test: Remove unused attempts parameter from wait_until --- test/functional/test_framework/util.py | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index 6e88a50cd7235..6947ac5895b5e 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -289,7 +289,7 @@ def ensure_for(*, duration, f, check_interval=0.2): time.sleep(check_interval) -def wait_until_helper_internal(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, timeout_factor=1.0, check_interval=0.05): +def wait_until_helper_internal(predicate, *, timeout=60, lock=None, timeout_factor=1.0, check_interval=0.05): """Sleep until the predicate resolves to be True. Warning: Note that this method is not recommended to be used in tests as it is @@ -298,13 +298,10 @@ def wait_until_helper_internal(predicate, *, attempts=float('inf'), timeout=floa properly scaled. Furthermore, `wait_until()` from `P2PInterface` class in `p2p.py` has a preset lock. """ - if attempts == float('inf') and timeout == float('inf'): - timeout = 60 timeout = timeout * timeout_factor - attempt = 0 time_end = time.time() + timeout - while attempt < attempts and time.time() < time_end: + while time.time() < time_end: if lock: with lock: if predicate(): @@ -312,17 +309,12 @@ def wait_until_helper_internal(predicate, *, attempts=float('inf'), timeout=floa else: if predicate(): return - attempt += 1 time.sleep(check_interval) # Print the cause of the timeout predicate_source = "''''\n" + inspect.getsource(predicate) + "'''" logger.error("wait_until() failed. Predicate: {}".format(predicate_source)) - if attempt >= attempts: - raise AssertionError("Predicate {} not true after {} attempts".format(predicate_source, attempts)) - elif time.time() >= time_end: - raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout)) - raise RuntimeError('Unreachable') + raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout)) def sha256sum_file(filename): From 15d982f91e6b0f145c9dd4edf29827cfabb37a3f Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Wed, 23 Oct 2024 12:24:41 -0400 Subject: [PATCH 06/40] Add package hash to package-rbf log message --- src/validation.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 62cae2cfb5046..bc2245f053b7d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1205,9 +1205,10 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector& txn "package RBF failed: " + err_tup.value().second, ""); } - LogDebug(BCLog::TXPACKAGES, "package RBF checks passed: parent %s (wtxid=%s), child %s (wtxid=%s)\n", + LogDebug(BCLog::TXPACKAGES, "package RBF checks passed: parent %s (wtxid=%s), child %s (wtxid=%s), package hash (%s)\n", txns.front()->GetHash().ToString(), txns.front()->GetWitnessHash().ToString(), - txns.back()->GetHash().ToString(), txns.back()->GetWitnessHash().ToString()); + txns.back()->GetHash().ToString(), txns.back()->GetWitnessHash().ToString(), + GetPackageHash(txns).ToString()); return true; From 87d92fa340195d9c87be3d023ca133b90b3b7d4e Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Thu, 24 Oct 2024 08:24:00 -0400 Subject: [PATCH 07/40] test: Add unit test coverage of package rbf + prioritisetransaction --- src/test/txpackage_tests.cpp | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index ea211aedf3ac3..ecee82c48a48e 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -1078,7 +1078,25 @@ BOOST_AUTO_TEST_CASE(package_rbf_tests) BOOST_CHECK_EQUAL(it_child_3->second.m_effective_feerate.value().GetFee(package3_total_vsize), 199 + 1300); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); - } + // Finally, check that we can prioritise tx_child_1 to get package1 into the mempool. + // It should not be possible to resubmit package1 and get it in without prioritisation. + const auto submit4 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, false, std::nullopt); + if (auto err_4{CheckPackageMempoolAcceptResult(package1, submit4, /*expect_valid=*/false, m_node.mempool.get())}) { + BOOST_ERROR(err_4.value()); + } + m_node.mempool->PrioritiseTransaction(tx_child_1->GetHash(), 1363); + const auto submit5 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, false, std::nullopt); + if (auto err_5{CheckPackageMempoolAcceptResult(package1, submit5, /*expect_valid=*/true, m_node.mempool.get())}) { + BOOST_ERROR(err_5.value()); + } + it_parent_1 = submit5.m_tx_results.find(tx_parent_1->GetWitnessHash()); + it_child_1 = submit5.m_tx_results.find(tx_child_1->GetWitnessHash()); + BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + LOCK(m_node.mempool->cs); + BOOST_CHECK(m_node.mempool->GetIter(tx_parent_1->GetHash()).has_value()); + BOOST_CHECK(m_node.mempool->GetIter(tx_child_1->GetHash()).has_value()); + } } BOOST_AUTO_TEST_SUITE_END() From 802214c0832de00f24268183f7763fa984ba7903 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Fri, 23 Aug 2024 18:45:49 -0400 Subject: [PATCH 08/40] Introduce mempool changesets Introduce the CTxMemPool::ChangeSet, a wrapper for creating (potential) new mempool entries and removing conflicts. --- src/txmempool.cpp | 20 +++++++++++++++++++ src/txmempool.h | 31 ++++++++++++++++++++++++++++ src/validation.cpp | 50 +++++++++++++++++++++++++++------------------- 3 files changed, 80 insertions(+), 21 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 8b6f993843124..ed0b3913d7bc7 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1369,3 +1369,23 @@ util::Result, std::vector>> CTxMemPool:: std::sort(new_chunks.begin(), new_chunks.end(), std::greater()); return std::make_pair(old_chunks, new_chunks); } + +CTxMemPool::ChangeSet::TxHandle CTxMemPool::ChangeSet::StageAddition(const CTransactionRef& tx, const CAmount fee, int64_t time, unsigned int entry_height, uint64_t entry_sequence, bool spends_coinbase, int64_t sigops_cost, LockPoints lp) +{ + auto newit = m_to_add.emplace(tx, fee, time, entry_height, entry_sequence, spends_coinbase, sigops_cost, lp).first; + m_entry_vec.push_back(newit); + return newit; +} + +void CTxMemPool::ChangeSet::Apply() +{ + LOCK(m_pool->cs); + m_pool->RemoveStaged(m_to_remove, false, MemPoolRemovalReason::REPLACED); + for (size_t i=0; iaddUnchecked(*tx_entry); + } + m_to_add.clear(); + m_to_remove.clear(); + m_entry_vec.clear(); +} diff --git a/src/txmempool.h b/src/txmempool.h index f914cbd729e8f..1025082dc883b 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -816,6 +816,37 @@ class CTxMemPool assert(m_epoch.guarded()); // verify guard even when it==nullopt return !it || visited(*it); } + + class ChangeSet { + public: + explicit ChangeSet(CTxMemPool* pool) : m_pool(pool) {} + ~ChangeSet() = default; + + ChangeSet(const ChangeSet&) = delete; + ChangeSet& operator=(const ChangeSet&) = delete; + + using TxHandle = CTxMemPool::txiter; + + TxHandle StageAddition(const CTransactionRef& tx, const CAmount fee, int64_t time, unsigned int entry_height, uint64_t entry_sequence, bool spends_coinbase, int64_t sigops_cost, LockPoints lp); + void StageRemoval(CTxMemPool::txiter it) { m_to_remove.insert(it); } + + util::Result CalculateMemPoolAncestors(TxHandle tx, const Limits& limits) + { + LOCK(m_pool->cs); + auto ret{m_pool->CalculateMemPoolAncestors(*tx, limits)}; + return ret; + } + + void Apply() EXCLUSIVE_LOCKS_REQUIRED(cs_main); + + private: + CTxMemPool* m_pool; + CTxMemPool::indexed_transaction_set m_to_add; + std::vector m_entry_vec; // track the added transactions' insertion order + CTxMemPool::setEntries m_to_remove; + }; + + std::unique_ptr GetChangeSet() EXCLUSIVE_LOCKS_REQUIRED(cs) { return std::make_unique(this); } }; /** diff --git a/src/validation.cpp b/src/validation.cpp index bc2245f053b7d..7445eb6b8a659 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -633,9 +633,9 @@ class MemPoolAccept CTxMemPool::setEntries m_iters_conflicting; /** All mempool ancestors of this transaction. */ CTxMemPool::setEntries m_ancestors; - /** Mempool entry constructed for this transaction. Constructed in PreChecks() but not - * inserted into the mempool until Finalize(). */ - std::unique_ptr m_entry; + /* Changeset representing adding a transaction and removing its conflicts. */ + std::unique_ptr m_changeset; + CTxMemPool::ChangeSet::TxHandle m_tx_handle; /** Whether RBF-related data structures (m_conflicts, m_iters_conflicting, m_all_conflicting, * m_replaced_transactions) include a sibling in addition to txns with conflicting inputs. */ bool m_sibling_eviction{false}; @@ -780,7 +780,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Alias what we need out of ws TxValidationState& state = ws.m_state; - std::unique_ptr& entry = ws.m_entry; if (!CheckTransaction(tx, state)) { return false; // state filled in by CheckTransaction @@ -909,9 +908,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Set entry_sequence to 0 when bypass_limits is used; this allows txs from a block // reorg to be marked earlier than any child txs that were already in the mempool. const uint64_t entry_sequence = bypass_limits ? 0 : m_pool.GetSequence(); - entry.reset(new CTxMemPoolEntry(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), entry_sequence, - fSpendsCoinbase, nSigOpsCost, lock_points.value())); - ws.m_vsize = entry->GetTxSize(); + ws.m_changeset = m_pool.GetChangeSet(); + ws.m_tx_handle = ws.m_changeset->StageAddition(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), entry_sequence, fSpendsCoinbase, nSigOpsCost, lock_points.value()); + + ws.m_vsize = ws.m_tx_handle->GetTxSize(); // Enforces 0-fee for dust transactions, no incentive to be mined alone if (m_pool.m_opts.require_standard) { @@ -983,7 +983,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) maybe_rbf_limits.descendant_size_vbytes += conflict->GetSizeWithDescendants(); } - if (auto ancestors{m_pool.CalculateMemPoolAncestors(*entry, maybe_rbf_limits)}) { + if (auto ancestors{ws.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, maybe_rbf_limits)}) { ws.m_ancestors = std::move(*ancestors); } else { // If CalculateMemPoolAncestors fails second time, we want the original error string. @@ -1015,7 +1015,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || ws.m_ptx->version == TRUC_VERSION) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); } - if (auto ancestors_retry{m_pool.CalculateMemPoolAncestors(*entry, cpfp_carve_out_limits)}) { + if (auto ancestors_retry{ws.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, cpfp_carve_out_limits)}) { ws.m_ancestors = std::move(*ancestors_retry); } else { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); @@ -1114,6 +1114,11 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) return state.Invalid(TxValidationResult::TX_RECONSIDERABLE, strprintf("insufficient fee%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); } + + // Add all the to-be-removed transactions to the changeset. + for (auto it : m_subpackage.m_all_conflicts) { + ws.m_changeset->StageRemoval(it); + } return true; } @@ -1173,7 +1178,9 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector& txn "package RBF failed: too many potential replacements", *err_string); } + for (CTxMemPool::txiter it : m_subpackage.m_all_conflicts) { + parent_ws.m_changeset->StageRemoval(it); m_subpackage.m_conflicting_fees += it->GetModifiedFee(); m_subpackage.m_conflicting_size += it->GetTxSize(); } @@ -1283,7 +1290,6 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) const uint256& hash = ws.m_hash; TxValidationState& state = ws.m_state; const bool bypass_limits = args.m_bypass_limits; - std::unique_ptr& entry = ws.m_entry; if (!m_subpackage.m_all_conflicts.empty()) Assume(args.m_allow_replacement); // Remove conflicting transactions from the mempool @@ -1296,25 +1302,23 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) it->GetTxSize(), hash.ToString(), tx.GetWitnessHash().ToString(), - entry->GetFee(), - entry->GetTxSize()); + ws.m_tx_handle->GetFee(), + ws.m_tx_handle->GetTxSize()); TRACEPOINT(mempool, replaced, it->GetTx().GetHash().data(), it->GetTxSize(), it->GetFee(), std::chrono::duration_cast>(it->GetTime()).count(), hash.data(), - entry->GetTxSize(), - entry->GetFee() + ws.m_tx_handle->GetTxSize(), + ws.m_tx_handle->GetFee() ); m_subpackage.m_replaced_transactions.push_back(it->GetSharedTx()); } - m_pool.RemoveStaged(m_subpackage.m_all_conflicts, false, MemPoolRemovalReason::REPLACED); + ws.m_changeset->Apply(); // Don't attempt to process the same conflicts repeatedly during subpackage evaluation: - // they no longer exist on subsequent calls to Finalize() post-RemoveStaged + // they no longer exist on subsequent calls to Finalize() post-Apply() m_subpackage.m_all_conflicts.clear(); - // Store transaction in memory - m_pool.addUnchecked(*entry, ws.m_ancestors); // trim mempool and check if tx was trimmed // If we are validating a package, don't trim here because we could evict a previous transaction @@ -1359,7 +1363,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& // Re-calculate mempool ancestors to call addUnchecked(). They may have changed since the // last calculation done in PreChecks, since package ancestors have already been submitted. { - auto ancestors{m_pool.CalculateMemPoolAncestors(*ws.m_entry, m_pool.m_opts.limits)}; + auto ancestors{ws.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, m_pool.m_opts.limits)}; if(!ancestors) { results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); // Since PreChecks() and PackageMempoolChecks() both enforce limits, this should never fail. @@ -1400,6 +1404,8 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& // Add successful results. The returned results may change later if LimitMempoolSize() evicts them. for (Workspace& ws : workspaces) { + auto iter = m_pool.GetIter(ws.m_ptx->GetHash()); + Assume(iter.has_value()); const auto effective_feerate = args.m_package_feerates ? ws.m_package_feerate : CFeeRate{ws.m_modified_fees, static_cast(ws.m_vsize)}; const auto effective_feerate_wtxids = args.m_package_feerates ? all_package_wtxids : @@ -1410,7 +1416,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& if (!m_pool.m_opts.signals) continue; const CTransaction& tx = *ws.m_ptx; const auto tx_info = NewMempoolTransactionInfo(ws.m_ptx, ws.m_base_fees, - ws.m_vsize, ws.m_entry->GetHeight(), + ws.m_vsize, (*iter)->GetHeight(), args.m_bypass_limits, args.m_package_submission, IsCurrentForFeeEstimation(m_active_chainstate), m_pool.HasNoInputsOf(tx)); @@ -1481,8 +1487,10 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef if (m_pool.m_opts.signals) { const CTransaction& tx = *ws.m_ptx; + auto iter = m_pool.GetIter(tx.GetHash()); + Assume(iter.has_value()); const auto tx_info = NewMempoolTransactionInfo(ws.m_ptx, ws.m_base_fees, - ws.m_vsize, ws.m_entry->GetHeight(), + ws.m_vsize, (*iter)->GetHeight(), args.m_bypass_limits, args.m_package_submission, IsCurrentForFeeEstimation(m_active_chainstate), m_pool.HasNoInputsOf(tx)); From 01e145b9758f1df14a7ea18058ba9577bf88e459 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Sat, 24 Aug 2024 06:53:43 -0400 Subject: [PATCH 09/40] Move changeset from workspace to subpackage Removes a redundant check that mempool limits will not be violated during package acceptance. --- src/txmempool.cpp | 9 ++++- src/txmempool.h | 9 +++++ src/validation.cpp | 89 ++++++++++++++++++++-------------------------- 3 files changed, 56 insertions(+), 51 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index ed0b3913d7bc7..8aa33a46af39d 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1383,9 +1383,16 @@ void CTxMemPool::ChangeSet::Apply() m_pool->RemoveStaged(m_to_remove, false, MemPoolRemovalReason::REPLACED); for (size_t i=0; iaddUnchecked(*tx_entry); + if (i == 0 && m_ancestors.count(tx_entry)) { + m_pool->addUnchecked(*tx_entry, m_ancestors[tx_entry]); + } else { + // We always recalculate ancestors from scratch if we're dealing + // with transactions which may have parents in the same package. + m_pool->addUnchecked(*tx_entry); + } } m_to_add.clear(); m_to_remove.clear(); m_entry_vec.clear(); + m_ancestors.clear(); } diff --git a/src/txmempool.h b/src/txmempool.h index 1025082dc883b..40ea838720256 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -832,8 +832,15 @@ class CTxMemPool util::Result CalculateMemPoolAncestors(TxHandle tx, const Limits& limits) { + // Look up transaction in our cache first + auto it = m_ancestors.find(tx); + if (it != m_ancestors.end()) return it->second; + + // If not found, try to have the mempool calculate it, and cache + // for later. LOCK(m_pool->cs); auto ret{m_pool->CalculateMemPoolAncestors(*tx, limits)}; + if (ret) m_ancestors.try_emplace(tx, *ret); return ret; } @@ -843,6 +850,8 @@ class CTxMemPool CTxMemPool* m_pool; CTxMemPool::indexed_transaction_set m_to_add; std::vector m_entry_vec; // track the added transactions' insertion order + // map from the m_to_add index to the ancestors for the transaction + std::map m_ancestors; CTxMemPool::setEntries m_to_remove; }; diff --git a/src/validation.cpp b/src/validation.cpp index 7445eb6b8a659..7ba870a10a008 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -633,8 +633,7 @@ class MemPoolAccept CTxMemPool::setEntries m_iters_conflicting; /** All mempool ancestors of this transaction. */ CTxMemPool::setEntries m_ancestors; - /* Changeset representing adding a transaction and removing its conflicts. */ - std::unique_ptr m_changeset; + /* Handle to the tx in the changeset */ CTxMemPool::ChangeSet::TxHandle m_tx_handle; /** Whether RBF-related data structures (m_conflicts, m_iters_conflicting, m_all_conflicting, * m_replaced_transactions) include a sibling in addition to txns with conflicting inputs. */ @@ -691,7 +690,7 @@ class MemPoolAccept // Try to add the transaction to the mempool, removing any conflicts first. // Returns true if the transaction is in the mempool after any size // limiting is performed, false otherwise. - bool Finalize(const ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); + bool FinalizeSubpackage(const ATMPArgs& args, std::vector& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Submit all transactions to the mempool and call ConsensusScriptChecks to add to the script // cache - should only be called after successful validation of all transactions in the package. @@ -746,6 +745,8 @@ class MemPoolAccept CTxMemPool::setEntries m_all_conflicts; /** Mempool transactions that were replaced. */ std::list m_replaced_transactions; + /* Changeset representing adding transactions and removing their conflicts. */ + std::unique_ptr m_changeset; /** Total modified fees of mempool transactions being replaced. */ CAmount m_conflicting_fees{0}; @@ -908,8 +909,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Set entry_sequence to 0 when bypass_limits is used; this allows txs from a block // reorg to be marked earlier than any child txs that were already in the mempool. const uint64_t entry_sequence = bypass_limits ? 0 : m_pool.GetSequence(); - ws.m_changeset = m_pool.GetChangeSet(); - ws.m_tx_handle = ws.m_changeset->StageAddition(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), entry_sequence, fSpendsCoinbase, nSigOpsCost, lock_points.value()); + if (!m_subpackage.m_changeset) { + m_subpackage.m_changeset = m_pool.GetChangeSet(); + } + ws.m_tx_handle = m_subpackage.m_changeset->StageAddition(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), entry_sequence, fSpendsCoinbase, nSigOpsCost, lock_points.value()); ws.m_vsize = ws.m_tx_handle->GetTxSize(); @@ -983,7 +986,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) maybe_rbf_limits.descendant_size_vbytes += conflict->GetSizeWithDescendants(); } - if (auto ancestors{ws.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, maybe_rbf_limits)}) { + if (auto ancestors{m_subpackage.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, maybe_rbf_limits)}) { ws.m_ancestors = std::move(*ancestors); } else { // If CalculateMemPoolAncestors fails second time, we want the original error string. @@ -1015,7 +1018,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || ws.m_ptx->version == TRUC_VERSION) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); } - if (auto ancestors_retry{ws.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, cpfp_carve_out_limits)}) { + if (auto ancestors_retry{m_subpackage.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, cpfp_carve_out_limits)}) { ws.m_ancestors = std::move(*ancestors_retry); } else { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); @@ -1117,7 +1120,7 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) // Add all the to-be-removed transactions to the changeset. for (auto it : m_subpackage.m_all_conflicts) { - ws.m_changeset->StageRemoval(it); + m_subpackage.m_changeset->StageRemoval(it); } return true; } @@ -1180,7 +1183,7 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector& txn for (CTxMemPool::txiter it : m_subpackage.m_all_conflicts) { - parent_ws.m_changeset->StageRemoval(it); + m_subpackage.m_changeset->StageRemoval(it); m_subpackage.m_conflicting_fees += it->GetModifiedFee(); m_subpackage.m_conflicting_size += it->GetTxSize(); } @@ -1282,13 +1285,13 @@ bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws) return true; } -bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) +bool MemPoolAccept::FinalizeSubpackage(const ATMPArgs& args, std::vector& workspaces) { AssertLockHeld(cs_main); AssertLockHeld(m_pool.cs); - const CTransaction& tx = *ws.m_ptx; - const uint256& hash = ws.m_hash; - TxValidationState& state = ws.m_state; + const CTransaction& tx = *workspaces.front().m_ptx; + const uint256& hash = workspaces.front().m_hash; + TxValidationState& state = workspaces.front().m_state; const bool bypass_limits = args.m_bypass_limits; if (!m_subpackage.m_all_conflicts.empty()) Assume(args.m_allow_replacement); @@ -1302,20 +1305,21 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) it->GetTxSize(), hash.ToString(), tx.GetWitnessHash().ToString(), - ws.m_tx_handle->GetFee(), - ws.m_tx_handle->GetTxSize()); + workspaces[0].m_tx_handle->GetFee(), + workspaces[0].m_tx_handle->GetTxSize()); TRACEPOINT(mempool, replaced, it->GetTx().GetHash().data(), it->GetTxSize(), it->GetFee(), std::chrono::duration_cast>(it->GetTime()).count(), hash.data(), - ws.m_tx_handle->GetTxSize(), - ws.m_tx_handle->GetFee() + workspaces[0].m_tx_handle->GetTxSize(), + workspaces[0].m_tx_handle->GetFee() ); m_subpackage.m_replaced_transactions.push_back(it->GetSharedTx()); } - ws.m_changeset->Apply(); + m_subpackage.m_changeset->Apply(); + m_subpackage.m_changeset.reset(); // Don't attempt to process the same conflicts repeatedly during subpackage evaluation: // they no longer exist on subsequent calls to Finalize() post-Apply() m_subpackage.m_all_conflicts.clear(); @@ -1345,6 +1349,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& return !m_pool.exists(GenTxid::Txid(ws.m_ptx->GetHash())); })); bool all_submitted = true; + FinalizeSubpackage(args, workspaces); // ConsensusScriptChecks adds to the script cache and is therefore consensus-critical; // CheckInputsFromMempoolAndCache asserts that transactions only spend coins available from the // mempool or UTXO set. Submit each transaction to the mempool immediately after calling @@ -1358,36 +1363,19 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR, strprintf("BUG! PolicyScriptChecks succeeded but ConsensusScriptChecks failed: %s", ws.m_ptx->GetHash().ToString())); - } - - // Re-calculate mempool ancestors to call addUnchecked(). They may have changed since the - // last calculation done in PreChecks, since package ancestors have already been submitted. - { - auto ancestors{ws.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, m_pool.m_opts.limits)}; - if(!ancestors) { - results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); - // Since PreChecks() and PackageMempoolChecks() both enforce limits, this should never fail. - Assume(false); - all_submitted = false; - package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR, - strprintf("BUG! Mempool ancestors or descendants were underestimated: %s", - ws.m_ptx->GetHash().ToString())); - } - ws.m_ancestors = std::move(ancestors).value_or(ws.m_ancestors); - } - // If we call LimitMempoolSize() for each individual Finalize(), the mempool will not take - // the transaction's descendant feerate into account because it hasn't seen them yet. Also, - // we risk evicting a transaction that a subsequent package transaction depends on. Instead, - // allow the mempool to temporarily bypass limits, the maximum package size) while - // submitting transactions individually and then trim at the very end. - if (!Finalize(args, ws)) { - results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); - // Since LimitMempoolSize() won't be called, this should never fail. - Assume(false); - all_submitted = false; - package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR, - strprintf("BUG! Adding to mempool failed: %s", ws.m_ptx->GetHash().ToString())); - } + // Remove the transaction from the mempool. + if (!m_subpackage.m_changeset) m_subpackage.m_changeset = m_pool.GetChangeSet(); + m_subpackage.m_changeset->StageRemoval(m_pool.GetIter(ws.m_ptx->GetHash()).value()); + } + } + if (!all_submitted) { + Assume(m_subpackage.m_changeset); + // This code should be unreachable; it's here as belt-and-suspenders + // to try to ensure we have no consensus-invalid transactions in the + // mempool. + m_subpackage.m_changeset->Apply(); + m_subpackage.m_changeset.reset(); + return false; } std::vector all_package_wtxids; @@ -1430,7 +1418,8 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef AssertLockHeld(cs_main); LOCK(m_pool.cs); // mempool "read lock" (held through m_pool.m_opts.signals->TransactionAddedToMempool()) - Workspace ws(ptx); + std::vector workspaces{Workspace(ptx)}; + Workspace &ws = workspaces.front(); const std::vector single_wtxid{ws.m_ptx->GetWitnessHash()}; if (!PreChecks(args, ws)) { @@ -1478,7 +1467,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef ws.m_base_fees, effective_feerate, single_wtxid); } - if (!Finalize(args, ws)) { + if (!FinalizeSubpackage(args, workspaces)) { // The only possible failure reason is fee-related (mempool full). // Failed for fee reasons. Provide the effective feerate and which txns were included. Assume(ws.m_state.GetResult() == TxValidationResult::TX_RECONSIDERABLE); From 57983b8add72a04721d3f2050c063a3c4d8683ed Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Sat, 24 Aug 2024 07:26:09 -0400 Subject: [PATCH 10/40] Move LimitMempoolSize to take place outside FinalizeSubpackage --- src/validation.cpp | 38 +++++++++++++++----------------------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 7ba870a10a008..c78a66dab5282 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -688,9 +688,7 @@ class MemPoolAccept bool ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Try to add the transaction to the mempool, removing any conflicts first. - // Returns true if the transaction is in the mempool after any size - // limiting is performed, false otherwise. - bool FinalizeSubpackage(const ATMPArgs& args, std::vector& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); + void FinalizeSubpackage(const ATMPArgs& args, std::vector& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Submit all transactions to the mempool and call ConsensusScriptChecks to add to the script // cache - should only be called after successful validation of all transactions in the package. @@ -1285,14 +1283,12 @@ bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws) return true; } -bool MemPoolAccept::FinalizeSubpackage(const ATMPArgs& args, std::vector& workspaces) +void MemPoolAccept::FinalizeSubpackage(const ATMPArgs& args, std::vector& workspaces) { AssertLockHeld(cs_main); AssertLockHeld(m_pool.cs); const CTransaction& tx = *workspaces.front().m_ptx; const uint256& hash = workspaces.front().m_hash; - TxValidationState& state = workspaces.front().m_state; - const bool bypass_limits = args.m_bypass_limits; if (!m_subpackage.m_all_conflicts.empty()) Assume(args.m_allow_replacement); // Remove conflicting transactions from the mempool @@ -1323,18 +1319,6 @@ bool MemPoolAccept::FinalizeSubpackage(const ATMPArgs& args, std::vector& workspaces, @@ -1467,11 +1451,19 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef ws.m_base_fees, effective_feerate, single_wtxid); } - if (!FinalizeSubpackage(args, workspaces)) { - // The only possible failure reason is fee-related (mempool full). - // Failed for fee reasons. Provide the effective feerate and which txns were included. - Assume(ws.m_state.GetResult() == TxValidationResult::TX_RECONSIDERABLE); - return MempoolAcceptResult::FeeFailure(ws.m_state, CFeeRate(ws.m_modified_fees, ws.m_vsize), {ws.m_ptx->GetWitnessHash()}); + FinalizeSubpackage(args, workspaces); + + // trim mempool and check if tx was trimmed + // If we are validating a package, don't trim here because we could evict a previous transaction + // in the package. LimitMempoolSize() should be called at the very end to make sure the mempool + // is still within limits and package submission happens atomically. + if (!args.m_package_submission && !args.m_bypass_limits) { + LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip()); + if (!m_pool.exists(GenTxid::Txid(ws.m_hash))) { + // The tx no longer meets our (new) mempool minimum feerate but could be reconsidered in a package. + ws.m_state.Invalid(TxValidationResult::TX_RECONSIDERABLE, "mempool full"); + return MempoolAcceptResult::FeeFailure(ws.m_state, CFeeRate(ws.m_modified_fees, ws.m_vsize), {ws.m_ptx->GetWitnessHash()}); + } } if (m_pool.m_opts.signals) { From 34b6c5833d11ea84fbd4b891e06408f6f4ca6fac Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Sat, 24 Aug 2024 07:26:09 -0400 Subject: [PATCH 11/40] Clean up FinalizeSubpackage to avoid workspace-specific information Also, use the "package hash" for logging replacements in the package rbf setting. --- src/txmempool.h | 12 +++++++++ src/validation.cpp | 61 ++++++++++++++++++++++++++++------------------ 2 files changed, 49 insertions(+), 24 deletions(-) diff --git a/src/txmempool.h b/src/txmempool.h index 40ea838720256..fd6bfb4a872f3 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -844,6 +844,18 @@ class CTxMemPool return ret; } + std::vector GetAddedTxns() const { + std::vector ret; + ret.reserve(m_entry_vec.size()); + for (const auto& entry : m_entry_vec) { + ret.emplace_back(entry->GetSharedTx()); + } + return ret; + } + + size_t GetTxCount() const { return m_entry_vec.size(); } + const CTransaction& GetAddedTxn(size_t index) const { return m_entry_vec.at(index)->GetTx(); } + void Apply() EXCLUSIVE_LOCKS_REQUIRED(cs_main); private: diff --git a/src/validation.cpp b/src/validation.cpp index c78a66dab5282..aba170769efd1 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -688,7 +688,7 @@ class MemPoolAccept bool ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Try to add the transaction to the mempool, removing any conflicts first. - void FinalizeSubpackage(const ATMPArgs& args, std::vector& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); + void FinalizeSubpackage(const ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Submit all transactions to the mempool and call ConsensusScriptChecks to add to the script // cache - should only be called after successful validation of all transactions in the package. @@ -1283,34 +1283,48 @@ bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws) return true; } -void MemPoolAccept::FinalizeSubpackage(const ATMPArgs& args, std::vector& workspaces) +void MemPoolAccept::FinalizeSubpackage(const ATMPArgs& args) { AssertLockHeld(cs_main); AssertLockHeld(m_pool.cs); - const CTransaction& tx = *workspaces.front().m_ptx; - const uint256& hash = workspaces.front().m_hash; if (!m_subpackage.m_all_conflicts.empty()) Assume(args.m_allow_replacement); // Remove conflicting transactions from the mempool for (CTxMemPool::txiter it : m_subpackage.m_all_conflicts) { - LogDebug(BCLog::MEMPOOL, "replacing mempool tx %s (wtxid=%s, fees=%s, vsize=%s). New tx %s (wtxid=%s, fees=%s, vsize=%s)\n", - it->GetTx().GetHash().ToString(), - it->GetTx().GetWitnessHash().ToString(), - it->GetFee(), - it->GetTxSize(), - hash.ToString(), - tx.GetWitnessHash().ToString(), - workspaces[0].m_tx_handle->GetFee(), - workspaces[0].m_tx_handle->GetTxSize()); + std::string log_string = strprintf("replacing mempool tx %s (wtxid=%s, fees=%s, vsize=%s). ", + it->GetTx().GetHash().ToString(), + it->GetTx().GetWitnessHash().ToString(), + it->GetFee(), + it->GetTxSize()); + FeeFrac feerate{m_subpackage.m_total_modified_fees, int32_t(m_subpackage.m_total_vsize)}; + uint256 tx_or_package_hash{}; + if (m_subpackage.m_changeset->GetTxCount() == 1) { + const CTransaction& tx = m_subpackage.m_changeset->GetAddedTxn(0); + tx_or_package_hash = tx.GetHash(); + log_string += strprintf("New tx %s (wtxid=%s, fees=%s, vsize=%s)", + tx.GetHash().ToString(), + tx.GetWitnessHash().ToString(), + feerate.fee, + feerate.size); + } else { + tx_or_package_hash = GetPackageHash(m_subpackage.m_changeset->GetAddedTxns()); + log_string += strprintf("New package %s with %lu txs, fees=%s, vsize=%s", + tx_or_package_hash.ToString(), + m_subpackage.m_changeset->GetTxCount(), + feerate.fee, + feerate.size); + + } + LogDebug(BCLog::MEMPOOL, "%s\n", log_string); TRACEPOINT(mempool, replaced, it->GetTx().GetHash().data(), it->GetTxSize(), it->GetFee(), std::chrono::duration_cast>(it->GetTime()).count(), - hash.data(), - workspaces[0].m_tx_handle->GetTxSize(), - workspaces[0].m_tx_handle->GetFee() + tx_or_package_hash.data(), + feerate.size, + feerate.fee ); m_subpackage.m_replaced_transactions.push_back(it->GetSharedTx()); } @@ -1333,7 +1347,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& return !m_pool.exists(GenTxid::Txid(ws.m_ptx->GetHash())); })); bool all_submitted = true; - FinalizeSubpackage(args, workspaces); + FinalizeSubpackage(args); // ConsensusScriptChecks adds to the script cache and is therefore consensus-critical; // CheckInputsFromMempoolAndCache asserts that transactions only spend coins available from the // mempool or UTXO set. Submit each transaction to the mempool immediately after calling @@ -1402,8 +1416,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef AssertLockHeld(cs_main); LOCK(m_pool.cs); // mempool "read lock" (held through m_pool.m_opts.signals->TransactionAddedToMempool()) - std::vector workspaces{Workspace(ptx)}; - Workspace &ws = workspaces.front(); + Workspace ws(ptx); const std::vector single_wtxid{ws.m_ptx->GetWitnessHash()}; if (!PreChecks(args, ws)) { @@ -1414,6 +1427,9 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef return MempoolAcceptResult::Failure(ws.m_state); } + m_subpackage.m_total_vsize = ws.m_vsize; + m_subpackage.m_total_modified_fees = ws.m_modified_fees; + // Individual modified feerate exceeded caller-defined max; abort if (args.m_client_maxfeerate && CFeeRate(ws.m_modified_fees, ws.m_vsize) > args.m_client_maxfeerate.value()) { ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "max feerate exceeded", ""); @@ -1451,12 +1467,9 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef ws.m_base_fees, effective_feerate, single_wtxid); } - FinalizeSubpackage(args, workspaces); + FinalizeSubpackage(args); - // trim mempool and check if tx was trimmed - // If we are validating a package, don't trim here because we could evict a previous transaction - // in the package. LimitMempoolSize() should be called at the very end to make sure the mempool - // is still within limits and package submission happens atomically. + // Limit the mempool, if appropriate. if (!args.m_package_submission && !args.m_bypass_limits) { LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip()); if (!m_pool.exists(GenTxid::Txid(ws.m_hash))) { From 7fb62f7db60c7d793828ae45f87bc3f5c63cc989 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Thu, 10 Oct 2024 14:59:23 -0400 Subject: [PATCH 12/40] Apply mempool changeset transactions directly into the mempool Rather than individually calling addUnchecked for each transaction added in a changeset (after removing all the to-be-removed transactions), instead we can take advantage of boost::multi_index's splicing features to extract and insert entries directly from the staging multi_index into mapTx. This has the immediate advantage of saving allocation overhead for mempool entries which have already been allocated once. This also means that the memory locations of mempool entries will not change when transactions go from staging to the main mempool. Additionally, eliminate addUnchecked and require all new transactions to enter the mempool via a CTxMemPoolChangeSet. --- src/bench/mempool_ephemeral_spends.cpp | 3 +- src/bench/mempool_eviction.cpp | 3 +- src/bench/mempool_stress.cpp | 3 +- src/bench/rpc_mempool.cpp | 3 +- src/test/blockencodings_tests.cpp | 8 +- src/test/fuzz/mini_miner.cpp | 4 +- src/test/fuzz/partially_downloaded_block.cpp | 2 +- src/test/fuzz/rbf.cpp | 8 +- src/test/mempool_tests.cpp | 92 ++++++++++---------- src/test/miner_tests.cpp | 62 ++++++------- src/test/miniminer_tests.cpp | 42 ++++----- src/test/policyestimator_tests.cpp | 12 +-- src/test/rbf_tests.cpp | 60 ++++++------- src/test/txvalidation_tests.cpp | 24 ++--- src/test/util/setup_common.cpp | 18 ++-- src/test/util/txmempool.cpp | 10 +++ src/test/util/txmempool.h | 4 + src/txmempool.cpp | 76 ++++++++++------ src/txmempool.h | 41 +++++---- src/validation.cpp | 2 +- 20 files changed, 268 insertions(+), 209 deletions(-) diff --git a/src/bench/mempool_ephemeral_spends.cpp b/src/bench/mempool_ephemeral_spends.cpp index e867c61752c1e..f34664a736bee 100644 --- a/src/bench/mempool_ephemeral_spends.cpp +++ b/src/bench/mempool_ephemeral_spends.cpp @@ -11,6 +11,7 @@ #include