Merge bitcoin/bitcoin#29424: v3 followups

6b161cb [test] second child of a v3 tx can be replaced individually (glozow)
5c998a6 [refactor] use MAX_PUBKEYS_PER_MULTISIG instead of magic numbers in test (glozow)
a934642 [test] PackageV3Checks with inheritance violation in mempool ancestor (glozow)
63b62e1 [doc] fix docs and comments from v3 (glozow)

Pull request description:

  Addresses final comments from #28948:
  - thread at bitcoin/bitcoin#28948 (comment), using ismaelsadeeq/bitcoin@87fc7f0 with some modifications
  - bitcoin/bitcoin#28948 (comment)
  - bitcoin/bitcoin#28948 (comment)
  - bitcoin/bitcoin#28948 (comment)
  - bitcoin/bitcoin#28948 (comment)
  - bitcoin/bitcoin#28948 (comment)
  - bitcoin/bitcoin#28948 (comment)
  - bitcoin/bitcoin#28948 (comment)
  - bitcoin/bitcoin#28948 (comment)

ACKs for top commit:
  instagibbs:
    ACK 6b161cb
  sdaftuar:
    utACK 6b161cb

Tree-SHA512: 584fce7810f4d704ee6ab51fdc7d42bab342140cae3d076f89b5e1966dd1dd8293cb25b3121e41a4dcd65f9d4a735102b9ab2e90f98aa770b84e21f4d35d63d3

Author: fanquake

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).

src/policy/v3_policy.cpp

@@ -81,17 +81,16 @@ std::optional<std::string> 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(),
                                       mempool_parent->GetTx().GetWitnessHash(),
                                       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<std::string> 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());

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);
         }
     }

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
         //            ^    ^

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 @@ def test_v3_in_testmempoolaccept(self):
         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 @@ def run_test(self):
         self.test_mempool_sibling()
         self.test_v3_package_inheritance()
         self.test_v3_in_testmempoolaccept()
+        self.test_reorg_2child_rbf()
 
 
 if __name__ == "__main__":

test/functional/mempool_sigoplimit.py

@@ -39,7 +39,7 @@
 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 @@ def create_bare_multisig_tx(utxo_to_spend=None):
         # 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