diff options
| -rw-r--r-- | doc/policy/packages.md | 15 | ||||
| -rw-r--r-- | src/test/txpackage_tests.cpp | 11 | ||||
| -rw-r--r-- | src/validation.cpp | 17 |
3 files changed, 36 insertions, 7 deletions
diff --git a/doc/policy/packages.md b/doc/policy/packages.md index 07698f2af2..7f7fbe18cd 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. diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 560efb6b42..6013080dc6 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 2813b62462..86d0022947 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -915,12 +915,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); } @@ -1318,7 +1321,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))) { |