From 5ae187f8761f5f85a1ef41d24f75afb7eecf366f Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 21 Jan 2022 12:40:59 +0000 Subject: [PATCH 1/3] [validation] look up transaction by txid GetIter takes a txid, not wtxid. --- src/test/txpackage_tests.cpp | 11 +++++++++++ src/validation.cpp | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 560efb6b420..6013080dc60 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -413,6 +413,17 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_child2->GetHash()))); BOOST_CHECK(!m_node.mempool->exists(GenTxid::Wtxid(ptx_child2->GetWitnessHash()))); + + // Deduplication should work when wtxid != txid. Submit package with the already-in-mempool + // transactions again, which should not fail. + const auto submit_segwit_dedup = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, + {ptx_parent, ptx_child1}, /*test_accept=*/ false); + BOOST_CHECK_MESSAGE(submit_segwit_dedup.m_state.IsValid(), + "Package validation unexpectedly failed: " << submit_segwit_dedup.m_state.GetRejectReason()); + auto it_parent_dup = submit_segwit_dedup.m_tx_results.find(ptx_parent->GetWitnessHash()); + auto it_child_dup = submit_segwit_dedup.m_tx_results.find(ptx_child1->GetWitnessHash()); + BOOST_CHECK(it_parent_dup->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY); + BOOST_CHECK(it_child_dup->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY); } // Try submitting Package1{child2, grandchild} where child2 is same-txid-different-witness as diff --git a/src/validation.cpp b/src/validation.cpp index 1b1d01a4c23..5241b1a2c60 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1306,7 +1306,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // we know is that the inputs aren't available. if (m_pool.exists(GenTxid::Wtxid(wtxid))) { // Exact transaction already exists in the mempool. - auto iter = m_pool.GetIter(wtxid); + auto iter = m_pool.GetIter(txid); assert(iter != std::nullopt); results.emplace(wtxid, MempoolAcceptResult::MempoolTx(iter.value()->GetTxSize(), iter.value()->GetFee())); } else if (m_pool.exists(GenTxid::Txid(txid))) { From d35a3cb3968d7584c7d5c42b121a80f34ea656bf Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 10 Feb 2022 11:22:18 +0000 Subject: [PATCH 2/3] [doc] clarify inaccurate comment about replacements paying higher feerate Co-authored-by: Suhas Daftuar --- src/validation.cpp | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 5241b1a2c60..69d37c0323b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -909,12 +909,15 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) TxValidationState& state = ws.m_state; CFeeRate newFeeRate(ws.m_modified_fees, ws.m_vsize); - // It's possible that the replacement pays more fees than its direct conflicts but not more - // than all conflicts (i.e. the direct conflicts have high-fee descendants). However, if the - // replacement doesn't pay more fees than its direct conflicts, then we can be sure it's not - // more economically rational to mine. Before we go digging through the mempool for all - // transactions that would need to be removed (direct conflicts and all descendants), check - // that the replacement transaction pays more than its direct conflicts. + // The replacement transaction must have a higher feerate than its direct conflicts. + // - The motivation for this check is to ensure that the replacement transaction is preferable for + // block-inclusion, compared to what would be removed from the mempool. + // - This logic predates ancestor feerate-based transaction selection, which is why it doesn't + // consider feerates of descendants. + // - Note: Ancestor feerate-based transaction selection has made this comparison insufficient to + // guarantee that this is incentive-compatible for miners, because it is possible for a + // descendant transaction of a direct conflict to pay a higher feerate than the transaction that + // might replace them, under these rules. if (const auto err_string{PaysMoreThanConflicts(ws.m_iters_conflicting, newFeeRate, hash)}) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string); } From 77202f0554dcbbbb167d0ed3927cca0bf4609ce8 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 10 Feb 2022 11:59:26 +0000 Subject: [PATCH 3/3] [doc] package deduplication --- doc/policy/packages.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/doc/policy/packages.md b/doc/policy/packages.md index 07698f2af2b..7f7fbe18cdf 100644 --- a/doc/policy/packages.md +++ b/doc/policy/packages.md @@ -57,3 +57,18 @@ test accepts): - Warning: Batched fee-bumping may be unsafe for some use cases. Users and application developers should take caution if utilizing multi-parent packages. + +* Transactions in the package that have the same txid as another transaction already in the mempool + will be removed from the package prior to submission ("deduplication"). + + - *Rationale*: 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 there is no need to + repeat validation for those transactions or double-count them in fees. + + - *Rationale*: We want to prevent potential censorship vectors. We should not reject entire + packages because we already have one of the transactions. Also, if an attacker first broadcasts + a competing package or transaction with a mutated witness, even though the two + same-txid-different-witness transactions are conflicting and cannot replace each other, the + honest package should still be considered for acceptance.