diff --git a/doc/policy/mempool-replacements.md b/doc/policy/mempool-replacements.md index 96ab4112e2..3fd7ff2ad2 100644 --- a/doc/policy/mempool-replacements.md +++ b/doc/policy/mempool-replacements.md @@ -12,7 +12,7 @@ other consensus and policy rules, each of the following conditions are met: 1. The directly conflicting transactions all signal replaceability explicitly. A transaction is signaling BIP125 replaceability if any of its inputs have an nSequence number less than (0xffffffff - 1). - A transaction also signals replaceibility if its nVersion field is set to 3. + A transaction also signals replaceability if its nVersion field is set to 3. *Rationale*: See [BIP125 explanation](https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki#motivation). diff --git a/src/policy/v3_policy.cpp b/src/policy/v3_policy.cpp index 158881aeb9..f838dc6c0f 100644 --- a/src/policy/v3_policy.cpp +++ b/src/policy/v3_policy.cpp @@ -81,9 +81,9 @@ std::optional PackageV3Checks(const CTransactionRef& ptx, int64_t v vsize, V3_CHILD_MAX_VSIZE); } + // Exactly 1 parent exists, either in mempool or package. Find it. const auto parent_info = [&] { if (mempool_ancestors.size() > 0) { - // There's a parent in the mempool. auto& mempool_parent = *mempool_ancestors.begin(); Assume(mempool_parent->GetCountWithDescendants() == 1); return ParentInfo{mempool_parent->GetTx().GetHash(), @@ -91,7 +91,6 @@ std::optional PackageV3Checks(const CTransactionRef& ptx, int64_t v mempool_parent->GetTx().nVersion, /*has_mempool_descendant=*/mempool_parent->GetCountWithDescendants() > 1}; } else { - // Ancestor must be in the package. Find it. auto& parent_index = in_package_parents.front(); auto& package_parent = package.at(parent_index); return ParentInfo{package_parent->GetHash(), @@ -184,7 +183,7 @@ std::optional SingleV3Checks(const CTransactionRef& ptx, // The rest of the rules only apply to transactions with nVersion=3. if (ptx->nVersion != 3) return std::nullopt; - // Check that V3_ANCESTOR_LIMIT would not be violated, including both in-package and in-mempool. + // Check that V3_ANCESTOR_LIMIT would not be violated. if (mempool_ancestors.size() + 1 > V3_ANCESTOR_LIMIT) { return strprintf("tx %s (wtxid=%s) would have too many ancestors", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()); diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index b44b528b6f..fcf230642a 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -407,8 +407,6 @@ FUZZ_TARGET(tx_pool, .init = initialize_tx_pool) const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID; if (accepted) { txids.push_back(tx->GetHash()); - // Only check fees if accepted and not bypass_limits, otherwise it's not guaranteed that - // trimming has happened for this tx and previous iterations. CheckMempoolV3Invariants(tx_pool); } } diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index 98d5e892f9..e045949b43 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -119,6 +119,8 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) Package package_v3_v2{mempool_tx_v3, tx_v2_from_v3}; BOOST_CHECK_EQUAL(*PackageV3Checks(tx_v2_from_v3, GetVirtualTransactionSize(*tx_v2_from_v3), package_v3_v2, empty_ancestors), expected_error_str); + CTxMemPool::setEntries entries_mempool_v3{pool.GetIter(mempool_tx_v3->GetHash().ToUint256()).value()}; + BOOST_CHECK_EQUAL(*PackageV3Checks(tx_v2_from_v3, GetVirtualTransactionSize(*tx_v2_from_v3), {tx_v2_from_v3}, entries_mempool_v3), expected_error_str); // mempool_tx_v3 mempool_tx_v2 // ^ ^ @@ -149,6 +151,8 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) Package package_v2_v3{mempool_tx_v2, tx_v3_from_v2}; BOOST_CHECK_EQUAL(*PackageV3Checks(tx_v3_from_v2, GetVirtualTransactionSize(*tx_v3_from_v2), package_v2_v3, empty_ancestors), expected_error_str); + CTxMemPool::setEntries entries_mempool_v2{pool.GetIter(mempool_tx_v2->GetHash().ToUint256()).value()}; + BOOST_CHECK_EQUAL(*PackageV3Checks(tx_v3_from_v2, GetVirtualTransactionSize(*tx_v3_from_v2), {tx_v3_from_v2}, entries_mempool_v2), expected_error_str); // mempool_tx_v3 mempool_tx_v2 // ^ ^ diff --git a/test/functional/mempool_accept_v3.py b/test/functional/mempool_accept_v3.py index ca599a9993..7ac3c97c4b 100755 --- a/test/functional/mempool_accept_v3.py +++ b/test/functional/mempool_accept_v3.py @@ -2,6 +2,7 @@ # Copyright (c) 2024 The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. +from decimal import Decimal from test_framework.messages import ( MAX_BIP125_RBF_SEQUENCE, @@ -397,6 +398,37 @@ class MempoolAcceptV3(BitcoinTestFramework): test_accept_2children_with_in_mempool_parent = node.testmempoolaccept([tx_v3_child_1["hex"], tx_v3_child_2["hex"]]) assert all([result["package-error"] == expected_error_extra for result in test_accept_2children_with_in_mempool_parent]) + @cleanup(extra_args=["-acceptnonstdtxn=1"]) + def test_reorg_2child_rbf(self): + node = self.nodes[0] + self.log.info("Test that children of a v3 transaction can be replaced individually, even if there are multiple due to reorg") + + ancestor_tx = self.wallet.send_self_transfer_multi(from_node=node, num_outputs=2, version=3) + self.check_mempool([ancestor_tx["txid"]]) + + block = self.generate(node, 1)[0] + self.check_mempool([]) + + child_1 = self.wallet.send_self_transfer(from_node=node, version=3, utxo_to_spend=ancestor_tx["new_utxos"][0]) + child_2 = self.wallet.send_self_transfer(from_node=node, version=3, utxo_to_spend=ancestor_tx["new_utxos"][1]) + self.check_mempool([child_1["txid"], child_2["txid"]]) + + self.generate(node, 1) + self.check_mempool([]) + + # Create a reorg, causing ancestor_tx to exceed the 1-child limit + node.invalidateblock(block) + self.check_mempool([ancestor_tx["txid"], child_1["txid"], child_2["txid"]]) + assert_equal(node.getmempoolentry(ancestor_tx["txid"])["descendantcount"], 3) + + # Create a replacement of child_1. It does not conflict with child_2. + child_1_conflict = self.wallet.send_self_transfer(from_node=node, version=3, utxo_to_spend=ancestor_tx["new_utxos"][0], fee_rate=Decimal("0.01")) + + # Ensure child_1 and child_1_conflict are different transactions + assert (child_1_conflict["txid"] != child_1["txid"]) + self.check_mempool([ancestor_tx["txid"], child_1_conflict["txid"], child_2["txid"]]) + assert_equal(node.getmempoolentry(ancestor_tx["txid"])["descendantcount"], 3) + def run_test(self): self.log.info("Generate blocks to create UTXOs") node = self.nodes[0] @@ -412,6 +444,7 @@ class MempoolAcceptV3(BitcoinTestFramework): self.test_mempool_sibling() self.test_v3_package_inheritance() self.test_v3_in_testmempoolaccept() + self.test_reorg_2child_rbf() if __name__ == "__main__": diff --git a/test/functional/mempool_sigoplimit.py b/test/functional/mempool_sigoplimit.py index 384423e5f5..d3fb5f9119 100755 --- a/test/functional/mempool_sigoplimit.py +++ b/test/functional/mempool_sigoplimit.py @@ -39,7 +39,7 @@ from test_framework.wallet import MiniWallet from test_framework.wallet_util import generate_keypair DEFAULT_BYTES_PER_SIGOP = 20 # default setting - +MAX_PUBKEYS_PER_MULTISIG = 20 class BytesPerSigOpTest(BitcoinTestFramework): def set_test_params(self): @@ -159,13 +159,13 @@ class BytesPerSigOpTest(BitcoinTestFramework): # Separately, the parent tx is ok parent_individual_testres = self.nodes[0].testmempoolaccept([tx_parent.serialize().hex()])[0] assert parent_individual_testres["allowed"] - # Multisig is counted as MAX_PUBKEYS_PER_MULTISIG = 20 sigops - assert_equal(parent_individual_testres["vsize"], 5000 * 20) + max_multisig_vsize = MAX_PUBKEYS_PER_MULTISIG * 5000 + assert_equal(parent_individual_testres["vsize"], max_multisig_vsize) # But together, it's exceeding limits in the *package* context. If sigops adjusted vsize wasn't being checked # here, it would get further in validation and give too-long-mempool-chain error instead. packet_test = self.nodes[0].testmempoolaccept([tx_parent.serialize().hex(), tx_child.serialize().hex()]) - expected_package_error = f"package-mempool-limits, package size {2*20*5000} exceeds ancestor size limit [limit: 101000]" + expected_package_error = f"package-mempool-limits, package size {2*max_multisig_vsize} exceeds ancestor size limit [limit: 101000]" assert_equal([x["package-error"] for x in packet_test], [expected_package_error] * 2) # When we actually try to submit, the parent makes it into the mempool, but the child would exceed ancestor vsize limits