aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorfanquake <fanquake@gmail.com>2022-01-25 10:13:41 +0800
committerfanquake <fanquake@gmail.com>2022-01-25 10:44:51 +0800
commit417e7503f80b8b9cfccd43131f313de8defc0ad5 (patch)
tree50d2db5d1a6ddf4768e4d8868b327afe01061f66
parent9ec3991ad3b2ae91997c696a4c2f187fe538eff0 (diff)
parent3cd7f693d3ed1bb7cf9ba3e0c482174df3684972 (diff)
Merge bitcoin/bitcoin#23804: validation: followups for de-duplication of packages
3cd7f693d3ed1bb7cf9ba3e0c482174df3684972 [unit test] package parents are a mix (glozow) de075a98eaf0b3f7676c5c78b50b66902202b34c [validation] better handle errors in SubmitPackage (glozow) 9d88853e0c85f765f7d982b15e8122ede50110ed AcceptPackage fixups (glozow) 2db77cd3b835d052de678755bcdde5a645ce2d65 [unit test] different witness in package submission (glozow) 9ad211c5753dbd148ba6f0ed56854f6364362ca8 [doc] more detailed explanation for deduplication (glozow) 83d4fb71260f268abd41d083fb3458476aed83ce [packages] return DIFFERENT_WITNESS for same-txid-different-witness tx (glozow) Pull request description: This addresses some comments from review on e12fafda2dfbbdf63f125e5af797ecfaa6488f66 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](https://github.com/bitcoin/bitcoin/pull/22674#discussion_r773037608) - Fix issue with not notifying `CValidationInterface` when there's a partial submission due to fail-fast: [comment](https://github.com/bitcoin/bitcoin/pull/22674#discussion_r773013162) ACKs for top commit: achow101: ACK 3cd7f693d3ed1bb7cf9ba3e0c482174df3684972 t-bast: LGTM, ACK https://github.com/bitcoin/bitcoin/pull/23804/commits/3cd7f693d3ed1bb7cf9ba3e0c482174df3684972 instagibbs: ACK 3cd7f693d3ed1bb7cf9ba3e0c482174df3684972 ariard: ACK 3cd7f69 Tree-SHA512: a5d86ca86edab80a5a05fcbb828901c058b3f2fa2552912ea52f2871e29c3cf4cc34020e7aac2217959c9c3a01856f4bd3d631d844635b98144f212f76c2f3ef
-rw-r--r--src/policy/packages.h1
-rw-r--r--src/test/txpackage_tests.cpp232
-rw-r--r--src/validation.cpp65
-rw-r--r--src/validation.h18
4 files changed, 292 insertions, 24 deletions
diff --git a/src/policy/packages.h b/src/policy/packages.h
index d2744f1265..9f274f6b7d 100644
--- a/src/policy/packages.h
+++ b/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
diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp
index 6f78b43826..560efb6b42 100644
--- a/src/test/txpackage_tests.cpp
+++ b/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()
diff --git a/src/validation.cpp b/src/validation.cpp
index b3d989e150..72e0b6218e 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -619,10 +619,10 @@ private:
// Submit all transactions to the mempool and call ConsensusScriptChecks to add to the script
// cache - should only be called after successful validation of all transactions in the package.
- // The package may end up partially-submitted after size limitting; returns true if all
+ // The package may end up partially-submitted after size limiting; returns true if all
// transactions are successfully added to the mempool, false otherwise.
- bool FinalizePackage(const ATMPArgs& args, std::vector<Workspace>& workspaces, PackageValidationState& package_state,
- std::map<const uint256, const MempoolAcceptResult>& results)
+ bool SubmitPackage(const ATMPArgs& args, std::vector<Workspace>& workspaces, PackageValidationState& package_state,
+ std::map<const uint256, const MempoolAcceptResult>& results)
EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs);
// Compare a package's feerate against minimum allowed.
@@ -1061,12 +1061,17 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws)
return true;
}
-bool MemPoolAccept::FinalizePackage(const ATMPArgs& args, std::vector<Workspace>& workspaces,
- PackageValidationState& package_state,
- std::map<const uint256, const MempoolAcceptResult>& results)
+bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>& workspaces,
+ PackageValidationState& package_state,
+ std::map<const uint256, const MempoolAcceptResult>& results)
{
AssertLockHeld(cs_main);
AssertLockHeld(m_pool.cs);
+ // Sanity check: none of the transactions should be in the mempool, and none of the transactions
+ // should have a same-txid-different-witness equivalent in the mempool.
+ assert(std::all_of(workspaces.cbegin(), workspaces.cend(), [this](const auto& ws){
+ return !m_pool.exists(GenTxid::Txid(ws.m_ptx->GetHash())); }));
+
bool all_submitted = true;
// ConsensusScriptChecks adds to the script cache and is therefore consensus-critical;
// CheckInputsFromMempoolAndCache asserts that transactions only spend coins available from the
@@ -1076,18 +1081,24 @@ bool MemPoolAccept::FinalizePackage(const ATMPArgs& args, std::vector<Workspace>
if (!ConsensusScriptChecks(args, ws)) {
results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state));
// Since PolicyScriptChecks() passed, this should never fail.
- all_submitted = Assume(false);
+ all_submitted = false;
+ package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR,
+ strprintf("BUG! PolicyScriptChecks succeeded but ConsensusScriptChecks failed: %s",
+ ws.m_ptx->GetHash().ToString()));
}
// Re-calculate mempool ancestors to call addUnchecked(). They may have changed since the
// last calculation done in PreChecks, since package ancestors have already been submitted.
- std::string err_string;
+ std::string unused_err_string;
if(!m_pool.CalculateMemPoolAncestors(*ws.m_entry, ws.m_ancestors, m_limit_ancestors,
m_limit_ancestor_size, m_limit_descendants,
- m_limit_descendant_size, err_string)) {
+ m_limit_descendant_size, unused_err_string)) {
results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state));
// Since PreChecks() and PackageMempoolChecks() both enforce limits, this should never fail.
- all_submitted = Assume(false);
+ all_submitted = false;
+ package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR,
+ strprintf("BUG! Mempool ancestors or descendants were underestimated: %s",
+ ws.m_ptx->GetHash().ToString()));
}
// If we call LimitMempoolSize() for each individual Finalize(), the mempool will not take
// the transaction's descendant feerate into account because it hasn't seen them yet. Also,
@@ -1097,7 +1108,9 @@ bool MemPoolAccept::FinalizePackage(const ATMPArgs& args, std::vector<Workspace>
if (!Finalize(args, ws)) {
results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state));
// Since LimitMempoolSize() won't be called, this should never fail.
- all_submitted = Assume(false);
+ all_submitted = false;
+ package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR,
+ strprintf("BUG! Adding to mempool failed: %s", ws.m_ptx->GetHash().ToString()));
}
}
@@ -1106,7 +1119,6 @@ bool MemPoolAccept::FinalizePackage(const ATMPArgs& args, std::vector<Workspace>
LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip(),
gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000,
std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)});
- if (!all_submitted) return false;
// Find the wtxids of the transactions that made it into the mempool. Allow partial submission,
// but don't report success unless they all made it into the mempool.
@@ -1211,8 +1223,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
if (args.m_test_accept) return PackageMempoolAcceptResult(package_state, std::move(results));
- if (!FinalizePackage(args, workspaces, package_state, results)) {
- package_state.Invalid(PackageValidationResult::PCKG_TX, "submission failed");
+ if (!SubmitPackage(args, workspaces, package_state, results)) {
+ // PackageValidationState filled in by SubmitPackage().
return PackageMempoolAcceptResult(package_state, std::move(results));
}
@@ -1237,11 +1249,13 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
return PackageMempoolAcceptResult(package_state, {});
}
- const auto& child = package[package.size() - 1];
+ // IsChildWithParents() guarantees the package is > 1 transactions.
+ assert(package.size() > 1);
// The package must be 1 child with all of its unconfirmed parents. The package is expected to
// be sorted, so the last transaction is the child.
+ const auto& child = package.back();
std::unordered_set<uint256, SaltedTxidHasher> unconfirmed_parent_txids;
- std::transform(package.cbegin(), package.end() - 1,
+ std::transform(package.cbegin(), package.cend() - 1,
std::inserter(unconfirmed_parent_txids, unconfirmed_parent_txids.end()),
[](const auto& tx) { return tx->GetHash(); });
@@ -1273,10 +1287,14 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
LOCK(m_pool.cs);
std::map<const uint256, const MempoolAcceptResult> results;
- // As node operators are free to set their mempool policies however they please, it's possible
- // for package transaction(s) to already be in the mempool, and we don't want to reject the
- // entire package in that case (as that could be a censorship vector). Filter the transactions
- // that are already in mempool and add their information to results, since we already have them.
+ // Node operators are free to set their mempool policies however they please, nodes may receive
+ // transactions in different orders, and malicious counterparties may try to take advantage of
+ // policy differences to pin or delay propagation of transactions. As such, it's possible for
+ // some package transaction(s) to already be in the mempool, and we don't want to reject the
+ // entire package in that case (as that could be a censorship vector). De-duplicate the
+ // transactions that are already in the mempool, and only call AcceptMultipleTransactions() with
+ // the new transactions. This ensures we don't double-count transaction counts and sizes when
+ // checking ancestor/descendant limits, or double-count transaction fees for fee-related policy.
std::vector<CTransactionRef> txns_new;
for (const auto& tx : package) {
const auto& wtxid = tx->GetWitnessHash();
@@ -1297,9 +1315,10 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
// transaction for the mempool one. Note that we are ignoring the validity of the
// package transaction passed in.
// TODO: allow witness replacement in packages.
- auto iter = m_pool.GetIter(wtxid);
+ auto iter = m_pool.GetIter(txid);
assert(iter != std::nullopt);
- results.emplace(txid, MempoolAcceptResult::MempoolTx(iter.value()->GetTxSize(), iter.value()->GetFee()));
+ // Provide the wtxid of the mempool tx so that the caller can look it up in the mempool.
+ results.emplace(wtxid, MempoolAcceptResult::MempoolTxDifferentWitness(iter.value()->GetTx().GetWitnessHash()));
} else {
// Transaction does not already exist in the mempool.
txns_new.push_back(tx);
@@ -1366,12 +1385,12 @@ PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTx
}();
// Uncache coins pertaining to transactions that were not submitted to the mempool.
- // Ensure the coins cache is still within limits.
if (test_accept || result.m_state.IsInvalid()) {
for (const COutPoint& hashTx : coins_to_uncache) {
active_chainstate.CoinsTip().Uncache(hashTx);
}
}
+ // Ensure the coins cache is still within limits.
BlockValidationState state_dummy;
active_chainstate.FlushStateToDisk(state_dummy, FlushStateMode::PERIODIC);
return result;
diff --git a/src/validation.h b/src/validation.h
index edbd68f783..6e9912cada 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -163,8 +163,12 @@ struct MempoolAcceptResult {
VALID, //!> Fully validated, valid.
INVALID, //!> Invalid.
MEMPOOL_ENTRY, //!> Valid, transaction was already in the mempool.
+ DIFFERENT_WITNESS, //!> Not validated. A same-txid-different-witness tx (see m_other_wtxid) already exists in the mempool and was not replaced.
};
+ /** Result type. Present in all MempoolAcceptResults. */
const ResultType m_result_type;
+
+ /** Contains information about why the transaction failed. */
const TxValidationState m_state;
// The following fields are only present when m_result_type = ResultType::VALID or MEMPOOL_ENTRY
@@ -175,6 +179,10 @@ struct MempoolAcceptResult {
/** Raw base fees in satoshis. */
const std::optional<CAmount> m_base_fees;
+ // The following field is only present when m_result_type = ResultType::DIFFERENT_WITNESS
+ /** The wtxid of the transaction in the mempool which has the same txid but different witness. */
+ const std::optional<uint256> m_other_wtxid;
+
static MempoolAcceptResult Failure(TxValidationState state) {
return MempoolAcceptResult(state);
}
@@ -187,6 +195,10 @@ struct MempoolAcceptResult {
return MempoolAcceptResult(vsize, fees);
}
+ static MempoolAcceptResult MempoolTxDifferentWitness(const uint256& other_wtxid) {
+ return MempoolAcceptResult(other_wtxid);
+ }
+
// Private constructors. Use static methods MempoolAcceptResult::Success, etc. to construct.
private:
/** Constructor for failure case */
@@ -203,6 +215,10 @@ private:
/** Constructor for already-in-mempool case. It wouldn't replace any transactions. */
explicit MempoolAcceptResult(int64_t vsize, CAmount fees)
: m_result_type(ResultType::MEMPOOL_ENTRY), m_vsize{vsize}, m_base_fees(fees) {}
+
+ /** Constructor for witness-swapped case. */
+ explicit MempoolAcceptResult(const uint256& other_wtxid)
+ : m_result_type(ResultType::DIFFERENT_WITNESS), m_other_wtxid(other_wtxid) {}
};
/**
@@ -212,7 +228,7 @@ struct PackageMempoolAcceptResult
{
const PackageValidationState m_state;
/**
- * Map from (w)txid to finished MempoolAcceptResults. The client is responsible
+ * Map from wtxid to finished MempoolAcceptResults. The client is responsible
* for keeping track of the transaction objects themselves. If a result is not
* present, it means validation was unfinished for that transaction. If there
* was a package-wide error (see result in m_state), m_tx_results will be empty.