docs / fixups from RBF and packages #24310

glozow · 2022-02-10T12:09:13Z

Use txid, not wtxid, for mempool.GetIter(): validation: mempool validation and submission for packages of 1 child + parents #22674 (comment)
Fix a historically inaccurate comment about RBF during the refactors: RBF move 3/3: move followups + improve RBF documentation #22855 (comment)
Add a section about package deduplication to policy/packages.md: policy / validation: CPFP fee bumping within packages #24152 (comment) and policy / validation: CPFP fee bumping within packages #24152 (comment)

(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)

glozow · 2022-02-10T12:16:19Z

cc @LarryRuane @mzumsande since you both mentioned that deduplication wasn't described in packages.md

ariard

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)

DrahtBot · 2022-02-11T11:09:50Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24152 (policy / validation: CPFP fee bumping within packages by glozow)

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.

fanquake · 2022-02-12T10:09:03Z

cc @darosior @t-bast @sdaftuar

darosior

ACK 84c8123

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.

sdaftuar · 2022-02-12T20:12:16Z

src/validation.cpp

-    // that the replacement transaction pays more than its direct conflicts.
+    // The replacement transaction must have a higher feerate than its direct conflicts. Before
+    // ancestor packages were used in mining, this was equivalent to checking that our mining code
+    // 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.

GetIter takes a txid, not wtxid.

…rate Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>

glozow · 2022-02-14T10:09:39Z

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.

t-bast

LGTM, ACK 77202f0

darosior

ACK 77202f0

LarryRuane

ACK 77202f0

LarryRuane · 2022-02-20T04:36:36Z

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


when wtxid != txid

(nonblocking:) This confused me; can this be stated more clearly? wtxid can never equal txid.

Suggested change

// Deduplication should work when wtxid != txid. Submit package with the already-in-mempool

// 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.

77202f0 [doc] package deduplication (glozow) d35a3cb [doc] clarify inaccurate comment about replacements paying higher feerate (glozow) 5ae187f [validation] look up transaction by txid (glozow) Pull request description: - Use txid, not wtxid, for `mempool.GetIter()`: bitcoin#22674 (comment) - Fix a historically inaccurate comment about RBF during the refactors: bitcoin#22855 (comment) - Add a section about package deduplication to policy/packages.md: bitcoin#24152 (comment) and bitcoin#24152 (comment) (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 bitcoin#24152) ACKs for top commit: t-bast: LGTM, ACK bitcoin@77202f0 darosior: ACK 77202f0 LarryRuane: ACK 77202f0 Tree-SHA512: a428e791dfa59c359d3ccc67e8d3a4c1239815d2f6b29898e129700079271c00b3a45f091f70b65a6e54aa00a3d5b678b6da29d2a76b6cd6f946eaa7082ea696

glozow added the Docs label Feb 10, 2022

ariard reviewed Feb 11, 2022

View reviewed changes

DrahtBot mentioned this pull request Feb 11, 2022

policy / validation: CPFP fee bumping within packages #24152

Merged

darosior reviewed Feb 12, 2022

View reviewed changes

sdaftuar reviewed Feb 12, 2022

View reviewed changes

glozow and others added 3 commits February 14, 2022 10:04

[validation] look up transaction by txid

5ae187f

GetIter takes a txid, not wtxid.

[doc] clarify inaccurate comment about replacements paying higher fee…

d35a3cb

…rate Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>

[doc] package deduplication

77202f0

glozow force-pushed the 2022-02-fixups branch from 84c8123 to 77202f0 Compare February 14, 2022 10:05

t-bast approved these changes Feb 14, 2022

View reviewed changes

darosior reviewed Feb 16, 2022

View reviewed changes

LarryRuane reviewed Feb 20, 2022

View reviewed changes

fanquake merged commit bc49650 into bitcoin:master Feb 22, 2022

glozow deleted the 2022-02-fixups branch February 22, 2022 09:40

bitcoin locked and limited conversation to collaborators Feb 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

docs / fixups from RBF and packages #24310

docs / fixups from RBF and packages #24310

Uh oh!

glozow commented Feb 10, 2022 •

edited

Loading

Uh oh!

glozow commented Feb 10, 2022

Uh oh!

ariard left a comment

Uh oh!

DrahtBot commented Feb 11, 2022

Uh oh!

fanquake commented Feb 12, 2022

Uh oh!

darosior left a comment

Uh oh!

sdaftuar Feb 12, 2022 •

edited

Loading

Uh oh!

glozow Feb 14, 2022

Uh oh!

glozow commented Feb 14, 2022

Uh oh!

t-bast left a comment

Uh oh!

darosior left a comment

Uh oh!

LarryRuane left a comment

Uh oh!

LarryRuane Feb 20, 2022

Uh oh!

darosior Feb 20, 2022

Uh oh!

Uh oh!

	// Deduplication should work when wtxid != txid. Submit package with the already-in-mempool
	// Deduplication should work when existing wtxid != package wtxid. Submit package with the already-in-mempool

docs / fixups from RBF and packages #24310

docs / fixups from RBF and packages #24310

Uh oh!

Conversation

glozow commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Feb 10, 2022

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Feb 11, 2022

Conflicts

Uh oh!

fanquake commented Feb 12, 2022

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

sdaftuar Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

glozow commented Feb 14, 2022

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

LarryRuane Feb 20, 2022

Choose a reason for hiding this comment

Uh oh!

darosior Feb 20, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glozow commented Feb 10, 2022 •

edited

Loading

sdaftuar Feb 12, 2022 •

edited

Loading