From 63b62e123e38cb92c2135e63eb1a5b760c11dd4e Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 9 Feb 2024 22:06:26 +0000 Subject: [PATCH 1/4] [doc] fix docs and comments from v3 --- doc/policy/mempool-replacements.md | 2 +- src/policy/v3_policy.cpp | 5 ++--- src/test/fuzz/tx_pool.cpp | 2 -- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/doc/policy/mempool-replacements.md b/doc/policy/mempool-replacements.md index 96ab4112e20..3fd7ff2ad2d 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 158881aeb9a..f838dc6c0fd 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 b44b528b6f6..fcf230642a6 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); } } From a9346421db813ebb1233f6477fe924e2f7c562ad Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 12 Feb 2024 14:08:57 +0000 Subject: [PATCH 2/4] [test] PackageV3Checks with inheritance violation in mempool ancestor --- src/test/txvalidation_tests.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index 98d5e892f96..e045949b437 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 // ^ ^ From 5c998a696c7a4ff2a91fe3d8c7177d2b806eb7ac Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 12 Feb 2024 14:26:03 +0000 Subject: [PATCH 3/4] [refactor] use MAX_PUBKEYS_PER_MULTISIG instead of magic numbers in test --- test/functional/mempool_sigoplimit.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/functional/mempool_sigoplimit.py b/test/functional/mempool_sigoplimit.py index 384423e5f57..d3fb5f91191 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 From 6b161cb82a9766ef814f05e5b8f019f15d5ee14d Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 12 Feb 2024 14:55:27 +0000 Subject: [PATCH 4/4] [test] second child of a v3 tx can be replaced individually Co-authored-by: ismaelsadeeq --- test/functional/mempool_accept_v3.py | 33 ++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/test/functional/mempool_accept_v3.py b/test/functional/mempool_accept_v3.py index ca599a9993f..7ac3c97c4b4 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__":