Merge bitcoin/bitcoin#22674: validation: mempool validation and submission for packages of 1 child + parents

laanwj · laanwj · commit 216f4ca9e7cc · 2021-12-15T20:42:33.000+01:00
046e8ff [unit test] package submission (glozow) e12fafd [validation] de-duplicate package transactions already in mempool (glozow) 8310d94 [packages] add sanity checks for package vs mempool limits (glozow) be3ff15 [validation] full package accept + mempool submission (glozow) 144a290 [policy] require submitted packages to be child-with-unconfirmed-parents (glozow) d59ddc5 [packages/doc] define and document package rules (glozow) ba26169 [unit test] context-free package checks (glozow) 9b2fdca [packages] add static IsChildWithParents function (glozow) Pull request description: This is 1 chunk of [Package Mempool Accept](https://gist.github.com/glozow/dc4e9d5c5b14ade7cdfac40f43adb18a); it restricts packages to 1 child with its parents, doesn't allow conflicts, and doesn't have CPFP (yet). Future PRs (see #22290) will add RBF and CPFP within packages. ACKs for top commit: laanwj: Code review ACK 046e8ff Tree-SHA512: 37dbba37d527712f8efef71ee05c90a8308992615af35f5e0cfeafc60d859cc792737d125aac526e37742fe7683ac8c155ac24af562426213904333c01260c95
diff --git a/doc/README.md b/doc/README.md
@@ -83,6 +83,7 @@ The Bitcoin repo's [root README](/README.md) contains relevant information on th
 - [Reduce Memory](reduce-memory.md)
 - [Reduce Traffic](reduce-traffic.md)
 - [Tor Support](tor.md)
+- [Transaction Relay Policy](policy/README.md)
 - [ZMQ](zmq.md)
 
 License
diff --git a/doc/policy/README.md b/doc/policy/README.md
@@ -0,0 +1,10 @@
+# Transaction Relay Policy
+
+Policy is a set of validation rules, in addition to consensus, enforced for unconfirmed
+transactions.
+
+This documentation is not an exhaustive list of all policy rules.
+
+- [Packages](packages.md)
+
+
diff --git a/doc/policy/packages.md b/doc/policy/packages.md
@@ -0,0 +1,59 @@
+# Package Mempool Accept
+
+## Definitions
+
+A **package** is an ordered list of transactions, representable by a connected Directed Acyclic
+Graph (a directed edge exists between a transaction that spends the output of another transaction).
+
+For every transaction `t` in a **topologically sorted** package, if any of its parents are present
+in the package, they appear somewhere in the list before `t`.
+
+A **child-with-unconfirmed-parents** package is a topologically sorted package that consists of
+exactly one child and all of its unconfirmed parents (no other transactions may be present).
+The last transaction in the package is the child, and its package can be canonically defined based
+on the current state: each of its inputs must be available in the UTXO set as of the current chain
+tip or some preceding transaction in the package.
+
+## Package Mempool Acceptance Rules
+
+The following rules are enforced for all packages:
+
+* Packages cannot exceed `MAX_PACKAGE_COUNT=25` count and `MAX_PACKAGE_SIZE=101KvB` total size
+   (#20833)
+
+   - *Rationale*: This is already enforced as mempool ancestor/descendant limits. If
+     transactions in a package are all related, exceeding this limit would mean that the package
+     can either be split up or it wouldn't pass individual mempool policy.
+
+   - Note that, if these mempool limits change, package limits should be reconsidered. Users may
+     also configure their mempool limits differently.
+
+* Packages must be topologically sorted. (#20833)
+
+* Packages cannot have conflicting transactions, i.e. no two transactions in a package can spend
+   the same inputs. Packages cannot have duplicate transactions. (#20833)
+
+* No transaction in a package can conflict with a mempool transaction. BIP125 Replace By Fee is
+  currently disabled for packages. (#20833)
+
+   - Package RBF may be enabled in the future.
+
+* When packages are evaluated against ancestor/descendant limits, the union of all transactions'
+  descendants and ancestors is considered. (#21800)
+
+   - *Rationale*: This is essentially a "worst case" heuristic intended for packages that are
+     heavily connected, i.e. some transaction in the package is the ancestor or descendant of all
+     the other transactions.
+
+The following rules are only enforced for packages to be submitted to the mempool (not enforced for
+test accepts):
+
+* Packages must be child-with-unconfirmed-parents packages. This also means packages must contain at
+  least 2 transactions. (#22674)
+
+   - *Rationale*: This allows for fee-bumping by CPFP. Allowing multiple parents makes it possible
+     to fee-bump a batch of transactions. Restricting packages to a defined topology is easier to
+     reason about and simplifies the validation logic greatly.
+
+   - Warning: Batched fee-bumping may be unsafe for some use cases. Users and application developers
+     should take caution if utilizing multi-parent packages.
diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp
@@ -60,3 +60,20 @@ bool CheckPackage(const Package& txns, PackageValidationState& state)
     }
     return true;
 }
+
+bool IsChildWithParents(const Package& package)
+{
+    assert(std::all_of(package.cbegin(), package.cend(), [](const auto& tx){return tx != nullptr;}));
+    if (package.size() < 2) return false;
+
+    // The package is expected to be sorted, so the last transaction is the child.
+    const auto& child = package.back();
+    std::unordered_set<uint256, SaltedTxidHasher> input_txids;
+    std::transform(child->vin.cbegin(), child->vin.cend(),
+                   std::inserter(input_txids, input_txids.end()),
+                   [](const auto& input) { return input.prevout.hash; });
+
+    // Every transaction must be a parent of the last transaction in the package.
+    return std::all_of(package.cbegin(), package.cend() - 1,
+                       [&input_txids](const auto& ptx) { return input_txids.count(ptx->GetHash()) > 0; });
+}
diff --git a/src/policy/packages.h b/src/policy/packages.h
@@ -41,4 +41,10 @@ class PackageValidationState : public ValidationState<PackageValidationResult> {
  */
 bool CheckPackage(const Package& txns, PackageValidationState& state);
 
+/** Context-free check that a package is exactly one child and its parents; not all parents need to
+ * be present, but the package must not contain any transactions that are not the child's parents.
+ * It is expected to be sorted, which means the last transaction must be the child.
+ */
+bool IsChildWithParents(const Package& package);
+
 #endif // BITCOIN_POLICY_PACKAGES_H
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
@@ -1028,6 +1028,8 @@ static RPCHelpMan testmempoolaccept()
             continue;
         }
         const auto& tx_result = it->second;
+        // Package testmempoolaccept doesn't allow transactions to already be in the mempool.
+        CHECK_NONFATAL(tx_result.m_result_type != MempoolAcceptResult::ResultType::MEMPOOL_ENTRY);
         if (tx_result.m_result_type == MempoolAcceptResult::ResultType::VALID) {
             const CAmount fee = tx_result.m_base_fees.value();
             // Check that fee does not exceed maximum fee
diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp
@@ -114,4 +114,217 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup)
     // Check that mempool size hasn't changed.
     BOOST_CHECK_EQUAL(m_node.mempool->size(), initialPoolSize);
 }
+
+BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup)
+{
+    // The signatures won't be verified so we can just use a placeholder
+    CKey placeholder_key;
+    placeholder_key.MakeNewKey(true);
+    CScript spk = GetScriptForDestination(PKHash(placeholder_key.GetPubKey()));
+    CKey placeholder_key_2;
+    placeholder_key_2.MakeNewKey(true);
+    CScript spk2 = GetScriptForDestination(PKHash(placeholder_key_2.GetPubKey()));
+
+    // Parent and Child Package
+    {
+        auto mtx_parent = CreateValidMempoolTransaction(m_coinbase_txns[0], 0, 0, coinbaseKey, spk,
+                                                        CAmount(49 * COIN), /* submit */ false);
+        CTransactionRef tx_parent = MakeTransactionRef(mtx_parent);
+
+        auto mtx_child = CreateValidMempoolTransaction(tx_parent, 0, 101, placeholder_key, spk2,
+                                                       CAmount(48 * COIN), /* submit */ false);
+        CTransactionRef tx_child = MakeTransactionRef(mtx_child);
+
+        PackageValidationState state;
+        BOOST_CHECK(CheckPackage({tx_parent, tx_child}, state));
+        BOOST_CHECK(!CheckPackage({tx_child, tx_parent}, state));
+        BOOST_CHECK_EQUAL(state.GetResult(), PackageValidationResult::PCKG_POLICY);
+        BOOST_CHECK_EQUAL(state.GetRejectReason(), "package-not-sorted");
+        BOOST_CHECK(IsChildWithParents({tx_parent, tx_child}));
+    }
+
+    // 24 Parents and 1 Child
+    {
+        Package package;
+        CMutableTransaction child;
+        for (int i{0}; i < 24; ++i) {
+            auto parent = MakeTransactionRef(CreateValidMempoolTransaction(m_coinbase_txns[i + 1],
+                                             0, 0, coinbaseKey, spk, CAmount(48 * COIN), false));
+            package.emplace_back(parent);
+            child.vin.push_back(CTxIn(COutPoint(parent->GetHash(), 0)));
+        }
+        child.vout.push_back(CTxOut(47 * COIN, spk2));
+
+        // The child must be in the package.
+        BOOST_CHECK(!IsChildWithParents(package));
+
+        // The parents can be in any order.
+        FastRandomContext rng;
+        Shuffle(package.begin(), package.end(), rng);
+        package.push_back(MakeTransactionRef(child));
+
+        PackageValidationState state;
+        BOOST_CHECK(CheckPackage(package, state));
+        BOOST_CHECK(IsChildWithParents(package));
+
+        package.erase(package.begin());
+        BOOST_CHECK(IsChildWithParents(package));
+
+        // The package cannot have unrelated transactions.
+        package.insert(package.begin(), m_coinbase_txns[0]);
+        BOOST_CHECK(!IsChildWithParents(package));
+    }
+
+    // 2 Parents and 1 Child where one parent depends on the other.
+    {
+        CMutableTransaction mtx_parent;
+        mtx_parent.vin.push_back(CTxIn(COutPoint(m_coinbase_txns[0]->GetHash(), 0)));
+        mtx_parent.vout.push_back(CTxOut(20 * COIN, spk));
+        mtx_parent.vout.push_back(CTxOut(20 * COIN, spk2));
+        CTransactionRef tx_parent = MakeTransactionRef(mtx_parent);
+
+        CMutableTransaction mtx_parent_also_child;
+        mtx_parent_also_child.vin.push_back(CTxIn(COutPoint(tx_parent->GetHash(), 0)));
+        mtx_parent_also_child.vout.push_back(CTxOut(20 * COIN, spk));
+        CTransactionRef tx_parent_also_child = MakeTransactionRef(mtx_parent_also_child);
+
+        CMutableTransaction mtx_child;
+        mtx_child.vin.push_back(CTxIn(COutPoint(tx_parent->GetHash(), 1)));
+        mtx_child.vin.push_back(CTxIn(COutPoint(tx_parent_also_child->GetHash(), 0)));
+        mtx_child.vout.push_back(CTxOut(39 * COIN, spk));
+        CTransactionRef tx_child = MakeTransactionRef(mtx_child);
+
+        PackageValidationState state;
+        BOOST_CHECK(IsChildWithParents({tx_parent, tx_parent_also_child}));
+        BOOST_CHECK(IsChildWithParents({tx_parent, tx_child}));
+        BOOST_CHECK(IsChildWithParents({tx_parent, tx_parent_also_child, tx_child}));
+        // IsChildWithParents does not detect unsorted parents.
+        BOOST_CHECK(IsChildWithParents({tx_parent_also_child, tx_parent, tx_child}));
+        BOOST_CHECK(CheckPackage({tx_parent, tx_parent_also_child, tx_child}, state));
+        BOOST_CHECK(!CheckPackage({tx_parent_also_child, tx_parent, tx_child}, state));
+        BOOST_CHECK_EQUAL(state.GetResult(), PackageValidationResult::PCKG_POLICY);
+        BOOST_CHECK_EQUAL(state.GetRejectReason(), "package-not-sorted");
+    }
+}
+
+BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup)
+{
+    LOCK(cs_main);
+    unsigned int expected_pool_size = m_node.mempool->size();
+    CKey parent_key;
+    parent_key.MakeNewKey(true);
+    CScript parent_locking_script = GetScriptForDestination(PKHash(parent_key.GetPubKey()));
+
+    // Unrelated transactions are not allowed in package submission.
+    Package package_unrelated;
+    for (size_t i{0}; i < 10; ++i) {
+        auto mtx = CreateValidMempoolTransaction(/* input_transaction */ m_coinbase_txns[i + 25], /* vout */ 0,
+                                                 /* input_height */ 0, /* input_signing_key */ coinbaseKey,
+                                                 /* output_destination */ parent_locking_script,
+                                                 /* output_amount */ CAmount(49 * COIN), /* submit */ false);
+        package_unrelated.emplace_back(MakeTransactionRef(mtx));
+    }
+    auto result_unrelated_submit = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
+                                                     package_unrelated, /* test_accept */ false);
+    BOOST_CHECK(result_unrelated_submit.m_state.IsInvalid());
+    BOOST_CHECK_EQUAL(result_unrelated_submit.m_state.GetResult(), PackageValidationResult::PCKG_POLICY);
+    BOOST_CHECK_EQUAL(result_unrelated_submit.m_state.GetRejectReason(), "package-not-child-with-parents");
+    BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
+
+    // Parent and Child (and Grandchild) Package
+    Package package_parent_child;
+    Package package_3gen;
+    auto mtx_parent = CreateValidMempoolTransaction(/* input_transaction */ m_coinbase_txns[0], /* vout */ 0,
+                                                    /* input_height */ 0, /* input_signing_key */ coinbaseKey,
+                                                    /* output_destination */ parent_locking_script,
+                                                    /* output_amount */ CAmount(49 * COIN), /* submit */ false);
+    CTransactionRef tx_parent = MakeTransactionRef(mtx_parent);
+    package_parent_child.push_back(tx_parent);
+    package_3gen.push_back(tx_parent);
+
+    CKey child_key;
+    child_key.MakeNewKey(true);
+    CScript child_locking_script = GetScriptForDestination(PKHash(child_key.GetPubKey()));
+    auto mtx_child = CreateValidMempoolTransaction(/* input_transaction */ tx_parent, /* vout */ 0,
+                                                   /* input_height */ 101, /* input_signing_key */ parent_key,
+                                                   /* output_destination */ child_locking_script,
+                                                   /* output_amount */ CAmount(48 * COIN), /* submit */ false);
+    CTransactionRef tx_child = MakeTransactionRef(mtx_child);
+    package_parent_child.push_back(tx_child);
+    package_3gen.push_back(tx_child);
+
+    CKey grandchild_key;
+    grandchild_key.MakeNewKey(true);
+    CScript grandchild_locking_script = GetScriptForDestination(PKHash(grandchild_key.GetPubKey()));
+    auto mtx_grandchild = CreateValidMempoolTransaction(/* input_transaction */ tx_child, /* vout */ 0,
+                                                       /* input_height */ 101, /* input_signing_key */ child_key,
+                                                       /* output_destination */ grandchild_locking_script,
+                                                       /* output_amount */ CAmount(47 * COIN), /* submit */ false);
+    CTransactionRef tx_grandchild = MakeTransactionRef(mtx_grandchild);
+    package_3gen.push_back(tx_grandchild);
+
+    // 3 Generations is not allowed.
+    {
+        auto result_3gen_submit = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
+                                                    package_3gen, /* test_accept */ false);
+        BOOST_CHECK(result_3gen_submit.m_state.IsInvalid());
+        BOOST_CHECK_EQUAL(result_3gen_submit.m_state.GetResult(), PackageValidationResult::PCKG_POLICY);
+        BOOST_CHECK_EQUAL(result_3gen_submit.m_state.GetRejectReason(), "package-not-child-with-parents");
+        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
+    }
+
+    // Child with missing parent.
+    mtx_child.vin.push_back(CTxIn(COutPoint(package_unrelated[0]->GetHash(), 0)));
+    Package package_missing_parent;
+    package_missing_parent.push_back(tx_parent);
+    package_missing_parent.push_back(MakeTransactionRef(mtx_child));
+    {
+        const auto result_missing_parent = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
+                                                             package_missing_parent, /* test_accept */ false);
+        BOOST_CHECK(result_missing_parent.m_state.IsInvalid());
+        BOOST_CHECK_EQUAL(result_missing_parent.m_state.GetResult(), PackageValidationResult::PCKG_POLICY);
+        BOOST_CHECK_EQUAL(result_missing_parent.m_state.GetRejectReason(), "package-not-child-with-unconfirmed-parents");
+        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
+
+    }
+
+    // Submit package with parent + child.
+    {
+        const auto submit_parent_child = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
+                                                           package_parent_child, /* test_accept */ false);
+        expected_pool_size += 2;
+        BOOST_CHECK_MESSAGE(submit_parent_child.m_state.IsValid(),
+                            "Package validation unexpectedly failed: " << submit_parent_child.m_state.GetRejectReason());
+        auto it_parent = submit_parent_child.m_tx_results.find(tx_parent->GetWitnessHash());
+        auto it_child = submit_parent_child.m_tx_results.find(tx_child->GetWitnessHash());
+        BOOST_CHECK(it_parent != submit_parent_child.m_tx_results.end());
+        BOOST_CHECK(it_parent->second.m_state.IsValid());
+        BOOST_CHECK(it_child != submit_parent_child.m_tx_results.end());
+        BOOST_CHECK(it_child->second.m_state.IsValid());
+
+        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
+        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent->GetHash())));
+        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child->GetHash())));
+    }
+
+    // Already-in-mempool transactions should be detected and de-duplicated.
+    {
+        const auto submit_deduped = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
+                                                      package_parent_child, /* test_accept */ false);
+        BOOST_CHECK_MESSAGE(submit_deduped.m_state.IsValid(),
+                            "Package validation unexpectedly failed: " << submit_deduped.m_state.GetRejectReason());
+        auto it_parent_deduped = submit_deduped.m_tx_results.find(tx_parent->GetWitnessHash());
+        auto it_child_deduped = submit_deduped.m_tx_results.find(tx_child->GetWitnessHash());
+        BOOST_CHECK(it_parent_deduped != submit_deduped.m_tx_results.end());
+        BOOST_CHECK(it_parent_deduped->second.m_state.IsValid());
+        BOOST_CHECK(it_parent_deduped->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY);
+        BOOST_CHECK(it_child_deduped != submit_deduped.m_tx_results.end());
+        BOOST_CHECK(it_child_deduped->second.m_state.IsValid());
+        BOOST_CHECK(it_child_deduped->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY);
+
+        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
+        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent->GetHash())));
+        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child->GetHash())));
+    }
+}
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/validation.cpp b/src/validation.cpp
diff --git a/src/validation.h b/src/validation.h

Original file line number	Diff line number	Diff line change
`@@ -1028,6 +1028,8 @@ static RPCHelpMan testmempoolaccept()`
`1028`	`1028`	`continue;`
`1029`	`1029`	`}`
`1030`	`1030`	`const auto& tx_result = it->second;`
	`1031`	`+ // Package testmempoolaccept doesn't allow transactions to already be in the mempool.`
	`1032`	`+ CHECK_NONFATAL(tx_result.m_result_type != MempoolAcceptResult::ResultType::MEMPOOL_ENTRY);`
`1031`	`1033`	`if (tx_result.m_result_type == MempoolAcceptResult::ResultType::VALID) {`
`1032`	`1034`	`const CAmount fee = tx_result.m_base_fees.value();`
`1033`	`1035`	`// Check that fee does not exceed maximum fee`