mempool.GetIter(): #22674 (review)(I'm intending for this to be in v23 since it's fixups for things that are already merged, which is why I split it from #24152)
cc @LarryRuane @mzumsande since you both mentioned that deduplication wasn't described in packages.md
ACK 84c8123
competing package or transaction with a mutated witness, the honest package should be accepted
nit: I think it could say "package or transaction with a mutated witness, the better-feerate competing package, potentially with overlapping components, should be accepted". Just to get ride of the honesty concept in matters of mempool acceptance. I believe it's accurate to talk about dishonesty only in the context of specific second-layer attacks. Because for some (e.g LN), the "dishonest"/attacker package confirming might be a good outcome for funds safety, namely pushing forward the channel state.
(can't comment on the commit directly, GH buggy as usual)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
ACK 84c8123f6b2578e7b343c98b327574f0910b2078
Maybe worth a regression test? For instance a simple
diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp
index 560efb6b42..15bbae3455 100644
--- a/src/test/txpackage_tests.cpp
+++ b/src/test/txpackage_tests.cpp
@@ -413,6 +413,9 @@ 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())));
+
+ ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, {ptx_parent, ptx_child1},
+ /*test_accept=*/ false);
}
// Try submitting Package1{child2, grandchild} where child2 is same-txid-different-witness as
would fail on the assertion without the patch.
GetIter takes a txid, not wtxid.
Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>
915 | - // more economically rational to mine. Before we go digging through the mempool for all 916 | - // transactions that would need to be removed (direct conflicts and all descendants), check 917 | - // that the replacement transaction pays more than its direct conflicts. 918 | + // The replacement transaction must have a higher feerate than its direct conflicts. Before 919 | + // ancestor packages were used in mining, this was equivalent to checking that our mining code 920 | + // would prefer the replacement. Now, it serves as a fail-fast mechanism.
Now, it serves as a fail-fast mechanism.
This sentence strikes me as odd; I think it gives the idea that perhaps this check is redundant (but faster to calculate) with the check that involves all descendants, which isn't the case.
If it were me, I'd add a comment like this to the code:
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.
Thanks for writing an alternative! I've changed the comment to be this.
Thanks for the reviews! I've added the test suggested by @darosior and comment suggested by @sdaftuar.
nit: I think it could say "package or transaction with a mutated witness, the better-feerate competing package, potentially with overlapping components, should be accepted". Just to get ride of the honesty concept in matters of mempool acceptance. I believe it's accurate to talk about dishonesty only in the context of specific second-layer attacks. Because for some (e.g LN), the "dishonest"/attacker package confirming might be a good outcome for funds safety, namely pushing forward the channel state.
I've clarified the doc to indicate that we just don't want the same-txid-different-witness transaction to be rejected completely, because we normally would consider it a conflict that we won't replace. I only want to document what the current code does, and it doesn't care about the fees of witnesses right now. We can change it later if the code changes.
ACK 77202f0554dcbbbb167d0ed3927cca0bf4609ce8
412 | @@ -413,6 +413,17 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) 413 | 414 | BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_child2->GetHash()))); 415 | BOOST_CHECK(!m_node.mempool->exists(GenTxid::Wtxid(ptx_child2->GetWitnessHash()))); 416 | + 417 | + // Deduplication should work when wtxid != txid. Submit package with the already-in-mempool
when wtxid != txid
(nonblocking:) This confused me; can this be stated more clearly? wtxid can never equal txid.
// Deduplication should work when existing wtxid != package wtxid. Submit package with the already-in-mempool
The wtxid of a transaction is equal to its txid when there is no witness data. This is precisely what this commit fixes and why this test didn't catch the bug earlier: ptx_parent is submitted multiple times, but its txid is equal to its wtxid. Otherwise it would have failed the assertion.
ACK 77202f0554dcbbbb167d0ed3927cca0bf4609ce8