Merge bitcoin/bitcoin#23804: validation: followups for de-duplication of packages

3cd7f69 [unit test] package parents are a mix (glozow)
de075a9 [validation] better handle errors in SubmitPackage (glozow)
9d88853 AcceptPackage fixups (glozow)
2db77cd [unit test] different witness in package submission (glozow)
9ad211c [doc] more detailed explanation for deduplication (glozow)
83d4fb7 [packages] return DIFFERENT_WITNESS for same-txid-different-witness tx (glozow)

Pull request description:

  This addresses some comments from review on e12fafd from #22674.

  - Improve documentation about de-duplication: [comment](https://github.com/bitcoin/bitcoin/pull/22674/files#r770156708)
  - Fix code looking up same-txid-different-witness transaction in mempool: [comment](https://github.com/bitcoin/bitcoin/pull/22674/files#r770804029)
  - Improve the interface for when a same-txid-different-witness transaction is swapped: [comment](https://github.com/bitcoin/bitcoin/pull/22674/files#r770782822)
  - Add a test for witness swapping: [comment](https://github.com/bitcoin/bitcoin/pull/22674/files#r770804029)
  - Add a test for packages with a mix of duplicate/different witness/new parents: [comment](bitcoin/bitcoin#22674 (comment))
  - Fix issue with not notifying `CValidationInterface` when there's a partial submission due to fail-fast: [comment](bitcoin/bitcoin#22674 (comment))

ACKs for top commit:
  achow101:
    ACK 3cd7f69
  t-bast:
    LGTM, ACK bitcoin/bitcoin@3cd7f69
  instagibbs:
    ACK 3cd7f69
  ariard:
    ACK 3cd7f69

Tree-SHA512: a5d86ca86edab80a5a05fcbb828901c058b3f2fa2552912ea52f2871e29c3cf4cc34020e7aac2217959c9c3a01856f4bd3d631d844635b98144f212f76c2f3ef

Author: fanquake

src/policy/packages.h

@@ -25,6 +25,7 @@ enum class PackageValidationResult {
     PCKG_RESULT_UNSET = 0,        //!< Initial value. The package has not yet been rejected.
     PCKG_POLICY,                  //!< The package itself is invalid (e.g. too many transactions).
     PCKG_TX,                      //!< At least one tx is invalid.
+    PCKG_MEMPOOL_ERROR,           //!< Mempool logic error.
 };
 
 /** A package is an ordered list of transactions. The transactions cannot conflict with (spend the

src/test/txpackage_tests.cpp

@@ -327,4 +327,236 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup)
         BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child->GetHash())));
     }
 }
+
+// Tests for packages containing transactions that have same-txid-different-witness equivalents in
+// the mempool.
+BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup)
+{
+    // Mine blocks to mature coinbases.
+    mineBlocks(5);
+    LOCK(cs_main);
+
+    // Transactions with a same-txid-different-witness transaction in the mempool should be ignored,
+    // and the mempool entry's wtxid returned.
+    CScript witnessScript = CScript() << OP_DROP << OP_TRUE;
+    CScript scriptPubKey = GetScriptForDestination(WitnessV0ScriptHash(witnessScript));
+    auto mtx_parent = CreateValidMempoolTransaction(/*input_transaction=*/ m_coinbase_txns[0], /*vout=*/ 0,
+                                                    /*input_height=*/ 0, /*input_signing_key=*/ coinbaseKey,
+                                                    /*output_destination=*/ scriptPubKey,
+                                                    /*output_amount=*/ CAmount(49 * COIN), /*submit=*/ false);
+    CTransactionRef ptx_parent = MakeTransactionRef(mtx_parent);
+
+    // Make two children with the same txid but different witnesses.
+    CScriptWitness witness1;
+    witness1.stack.push_back(std::vector<unsigned char>(1));
+    witness1.stack.push_back(std::vector<unsigned char>(witnessScript.begin(), witnessScript.end()));
+
+    CScriptWitness witness2(witness1);
+    witness2.stack.push_back(std::vector<unsigned char>(2));
+    witness2.stack.push_back(std::vector<unsigned char>(witnessScript.begin(), witnessScript.end()));
+
+    CKey child_key;
+    child_key.MakeNewKey(true);
+    CScript child_locking_script = GetScriptForDestination(WitnessV0KeyHash(child_key.GetPubKey()));
+    CMutableTransaction mtx_child1;
+    mtx_child1.nVersion = 1;
+    mtx_child1.vin.resize(1);
+    mtx_child1.vin[0].prevout.hash = ptx_parent->GetHash();
+    mtx_child1.vin[0].prevout.n = 0;
+    mtx_child1.vin[0].scriptSig = CScript();
+    mtx_child1.vin[0].scriptWitness = witness1;
+    mtx_child1.vout.resize(1);
+    mtx_child1.vout[0].nValue = CAmount(48 * COIN);
+    mtx_child1.vout[0].scriptPubKey = child_locking_script;
+
+    CMutableTransaction mtx_child2{mtx_child1};
+    mtx_child2.vin[0].scriptWitness = witness2;
+
+    CTransactionRef ptx_child1 = MakeTransactionRef(mtx_child1);
+    CTransactionRef ptx_child2 = MakeTransactionRef(mtx_child2);
+
+    // child1 and child2 have the same txid
+    BOOST_CHECK_EQUAL(ptx_child1->GetHash(), ptx_child2->GetHash());
+    // child1 and child2 have different wtxids
+    BOOST_CHECK(ptx_child1->GetWitnessHash() != ptx_child2->GetWitnessHash());
+
+    // Try submitting Package1{parent, child1} and Package2{parent, child2} where the children are
+    // same-txid-different-witness.
+    {
+        const auto submit_witness1 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
+                                                       {ptx_parent, ptx_child1}, /*test_accept=*/ false);
+        BOOST_CHECK_MESSAGE(submit_witness1.m_state.IsValid(),
+                            "Package validation unexpectedly failed: " << submit_witness1.m_state.GetRejectReason());
+        auto it_parent1 = submit_witness1.m_tx_results.find(ptx_parent->GetWitnessHash());
+        auto it_child1 = submit_witness1.m_tx_results.find(ptx_child1->GetWitnessHash());
+        BOOST_CHECK(it_parent1 != submit_witness1.m_tx_results.end());
+        BOOST_CHECK_MESSAGE(it_parent1->second.m_state.IsValid(),
+                            "Transaction unexpectedly failed: " << it_parent1->second.m_state.GetRejectReason());
+        BOOST_CHECK(it_child1 != submit_witness1.m_tx_results.end());
+        BOOST_CHECK_MESSAGE(it_child1->second.m_state.IsValid(),
+                            "Transaction unexpectedly failed: " << it_child1->second.m_state.GetRejectReason());
+
+        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_parent->GetHash())));
+        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_child1->GetHash())));
+
+        const auto submit_witness2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
+                                                       {ptx_parent, ptx_child2}, /*test_accept=*/ false);
+        BOOST_CHECK_MESSAGE(submit_witness2.m_state.IsValid(),
+                            "Package validation unexpectedly failed: " << submit_witness2.m_state.GetRejectReason());
+        auto it_parent2_deduped = submit_witness2.m_tx_results.find(ptx_parent->GetWitnessHash());
+        auto it_child2 = submit_witness2.m_tx_results.find(ptx_child2->GetWitnessHash());
+        BOOST_CHECK(it_parent2_deduped != submit_witness2.m_tx_results.end());
+        BOOST_CHECK(it_parent2_deduped->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY);
+        BOOST_CHECK(it_child2 != submit_witness2.m_tx_results.end());
+        BOOST_CHECK(it_child2->second.m_result_type == MempoolAcceptResult::ResultType::DIFFERENT_WITNESS);
+        BOOST_CHECK_EQUAL(ptx_child1->GetWitnessHash(), it_child2->second.m_other_wtxid.value());
+
+        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_child2->GetHash())));
+        BOOST_CHECK(!m_node.mempool->exists(GenTxid::Wtxid(ptx_child2->GetWitnessHash())));
+    }
+
+    // Try submitting Package1{child2, grandchild} where child2 is same-txid-different-witness as
+    // the in-mempool transaction, child1. Since child1 exists in the mempool and its outputs are
+    // available, child2 should be ignored and grandchild should be accepted.
+    //
+    // This tests a potential censorship vector in which an attacker broadcasts a competing package
+    // where a parent's witness is mutated. The honest package should be accepted despite the fact
+    // that we don't allow witness replacement.
+    CKey grandchild_key;
+    grandchild_key.MakeNewKey(true);
+    CScript grandchild_locking_script = GetScriptForDestination(WitnessV0KeyHash(grandchild_key.GetPubKey()));
+    auto mtx_grandchild = CreateValidMempoolTransaction(/*input_transaction=*/ ptx_child2, /* vout=*/ 0,
+                                                        /*input_height=*/ 0, /*input_signing_key=*/ child_key,
+                                                        /*output_destination=*/ grandchild_locking_script,
+                                                        /*output_amount=*/ CAmount(47 * COIN), /*submit=*/ false);
+    CTransactionRef ptx_grandchild = MakeTransactionRef(mtx_grandchild);
+
+    // We already submitted child1 above.
+    {
+        const auto submit_spend_ignored = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
+                                                            {ptx_child2, ptx_grandchild}, /*test_accept=*/ false);
+        BOOST_CHECK_MESSAGE(submit_spend_ignored.m_state.IsValid(),
+                            "Package validation unexpectedly failed: " << submit_spend_ignored.m_state.GetRejectReason());
+        auto it_child2_ignored = submit_spend_ignored.m_tx_results.find(ptx_child2->GetWitnessHash());
+        auto it_grandchild = submit_spend_ignored.m_tx_results.find(ptx_grandchild->GetWitnessHash());
+        BOOST_CHECK(it_child2_ignored != submit_spend_ignored.m_tx_results.end());
+        BOOST_CHECK(it_child2_ignored->second.m_result_type == MempoolAcceptResult::ResultType::DIFFERENT_WITNESS);
+        BOOST_CHECK(it_grandchild != submit_spend_ignored.m_tx_results.end());
+        BOOST_CHECK(it_grandchild->second.m_result_type == MempoolAcceptResult::ResultType::VALID);
+
+        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_child2->GetHash())));
+        BOOST_CHECK(!m_node.mempool->exists(GenTxid::Wtxid(ptx_child2->GetWitnessHash())));
+        BOOST_CHECK(m_node.mempool->exists(GenTxid::Wtxid(ptx_grandchild->GetWitnessHash())));
+    }
+
+    // A package Package{parent1, parent2, parent3, child} where the parents are a mixture of
+    // identical-tx-in-mempool, same-txid-different-witness-in-mempool, and new transactions.
+    Package package_mixed;
+
+    // Give all the parents anyone-can-spend scripts so we don't have to deal with signing the child.
+    CScript acs_script = CScript() << OP_TRUE;
+    CScript acs_spk = GetScriptForDestination(WitnessV0ScriptHash(acs_script));
+    CScriptWitness acs_witness;
+    acs_witness.stack.push_back(std::vector<unsigned char>(acs_script.begin(), acs_script.end()));
+
+    // parent1 will already be in the mempool
+    auto mtx_parent1 = CreateValidMempoolTransaction(/*input_transaction=*/ m_coinbase_txns[1], /*vout=*/ 0,
+                                                     /*input_height=*/ 0, /*input_signing_key=*/ coinbaseKey,
+                                                     /*output_destination=*/ acs_spk,
+                                                     /*output_amount=*/ CAmount(49 * COIN), /*submit=*/ true);
+    CTransactionRef ptx_parent1 = MakeTransactionRef(mtx_parent1);
+    package_mixed.push_back(ptx_parent1);
+
+    // parent2 will have a same-txid-different-witness tx already in the mempool
+    CScript grandparent2_script = CScript() << OP_DROP << OP_TRUE;
+    CScript grandparent2_spk = GetScriptForDestination(WitnessV0ScriptHash(grandparent2_script));
+    CScriptWitness parent2_witness1;
+    parent2_witness1.stack.push_back(std::vector<unsigned char>(1));
+    parent2_witness1.stack.push_back(std::vector<unsigned char>(grandparent2_script.begin(), grandparent2_script.end()));
+    CScriptWitness parent2_witness2;
+    parent2_witness2.stack.push_back(std::vector<unsigned char>(2));
+    parent2_witness2.stack.push_back(std::vector<unsigned char>(grandparent2_script.begin(), grandparent2_script.end()));
+
+    // Create grandparent2 creating an output with multiple spending paths. Submit to mempool.
+    auto mtx_grandparent2 = CreateValidMempoolTransaction(/*input_transaction=*/ m_coinbase_txns[2], /* vout=*/ 0,
+                                                          /*input_height=*/ 0, /*input_signing_key=*/ coinbaseKey,
+                                                          /*output_destination=*/ grandparent2_spk,
+                                                          /*output_amount=*/ CAmount(49 * COIN), /*submit=*/ true);
+    CTransactionRef ptx_grandparent2 = MakeTransactionRef(mtx_grandparent2);
+
+    CMutableTransaction mtx_parent2_v1;
+    mtx_parent2_v1.nVersion = 1;
+    mtx_parent2_v1.vin.resize(1);
+    mtx_parent2_v1.vin[0].prevout.hash = ptx_grandparent2->GetHash();
+    mtx_parent2_v1.vin[0].prevout.n = 0;
+    mtx_parent2_v1.vin[0].scriptSig = CScript();
+    mtx_parent2_v1.vin[0].scriptWitness = parent2_witness1;
+    mtx_parent2_v1.vout.resize(1);
+    mtx_parent2_v1.vout[0].nValue = CAmount(48 * COIN);
+    mtx_parent2_v1.vout[0].scriptPubKey = acs_spk;
+
+    CMutableTransaction mtx_parent2_v2{mtx_parent2_v1};
+    mtx_parent2_v2.vin[0].scriptWitness = parent2_witness2;
+
+    CTransactionRef ptx_parent2_v1 = MakeTransactionRef(mtx_parent2_v1);
+    CTransactionRef ptx_parent2_v2 = MakeTransactionRef(mtx_parent2_v2);
+    // Put parent2_v1 in the package, submit parent2_v2 to the mempool.
+    const MempoolAcceptResult parent2_v2_result = m_node.chainman->ProcessTransaction(ptx_parent2_v2);
+    BOOST_CHECK(parent2_v2_result.m_result_type == MempoolAcceptResult::ResultType::VALID);
+    package_mixed.push_back(ptx_parent2_v1);
+
+    // parent3 will be a new transaction
+    auto mtx_parent3 = CreateValidMempoolTransaction(/*input_transaction=*/ m_coinbase_txns[3], /*vout=*/ 0,
+                                                     /*input_height=*/ 0, /*input_signing_key=*/ coinbaseKey,
+                                                     /*output_destination=*/ acs_spk,
+                                                     /*output_amount=*/ CAmount(49 * COIN), /*submit=*/ false);
+    CTransactionRef ptx_parent3 = MakeTransactionRef(mtx_parent3);
+    package_mixed.push_back(ptx_parent3);
+
+    // child spends parent1, parent2, and parent3
+    CKey mixed_grandchild_key;
+    mixed_grandchild_key.MakeNewKey(true);
+    CScript mixed_child_spk = GetScriptForDestination(WitnessV0KeyHash(mixed_grandchild_key.GetPubKey()));
+
+    CMutableTransaction mtx_mixed_child;
+    mtx_mixed_child.vin.push_back(CTxIn(COutPoint(ptx_parent1->GetHash(), 0)));
+    mtx_mixed_child.vin.push_back(CTxIn(COutPoint(ptx_parent2_v1->GetHash(), 0)));
+    mtx_mixed_child.vin.push_back(CTxIn(COutPoint(ptx_parent3->GetHash(), 0)));
+    mtx_mixed_child.vin[0].scriptWitness = acs_witness;
+    mtx_mixed_child.vin[1].scriptWitness = acs_witness;
+    mtx_mixed_child.vin[2].scriptWitness = acs_witness;
+    mtx_mixed_child.vout.push_back(CTxOut(145 * COIN, mixed_child_spk));
+    CTransactionRef ptx_mixed_child = MakeTransactionRef(mtx_mixed_child);
+    package_mixed.push_back(ptx_mixed_child);
+
+    // Submit package:
+    // parent1 should be ignored
+    // parent2_v1 should be ignored (and v2 wtxid returned)
+    // parent3 should be accepted
+    // child should be accepted
+    {
+        const auto mixed_result = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_mixed, false);
+        BOOST_CHECK_MESSAGE(mixed_result.m_state.IsValid(), mixed_result.m_state.GetRejectReason());
+        auto it_parent1 = mixed_result.m_tx_results.find(ptx_parent1->GetWitnessHash());
+        auto it_parent2 = mixed_result.m_tx_results.find(ptx_parent2_v1->GetWitnessHash());
+        auto it_parent3 = mixed_result.m_tx_results.find(ptx_parent3->GetWitnessHash());
+        auto it_child = mixed_result.m_tx_results.find(ptx_mixed_child->GetWitnessHash());
+        BOOST_CHECK(it_parent1 != mixed_result.m_tx_results.end());
+        BOOST_CHECK(it_parent2 != mixed_result.m_tx_results.end());
+        BOOST_CHECK(it_parent3 != mixed_result.m_tx_results.end());
+        BOOST_CHECK(it_child != mixed_result.m_tx_results.end());
+
+        BOOST_CHECK(it_parent1->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY);
+        BOOST_CHECK(it_parent2->second.m_result_type == MempoolAcceptResult::ResultType::DIFFERENT_WITNESS);
+        BOOST_CHECK(it_parent3->second.m_result_type == MempoolAcceptResult::ResultType::VALID);
+        BOOST_CHECK(it_child->second.m_result_type == MempoolAcceptResult::ResultType::VALID);
+        BOOST_CHECK_EQUAL(ptx_parent2_v2->GetWitnessHash(), it_parent2->second.m_other_wtxid.value());
+
+        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_parent1->GetHash())));
+        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_parent2_v1->GetHash())));
+        BOOST_CHECK(!m_node.mempool->exists(GenTxid::Wtxid(ptx_parent2_v1->GetWitnessHash())));
+        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_parent3->GetHash())));
+        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_mixed_child->GetHash())));
+    }
+}
 BOOST_AUTO_TEST_SUITE_END()