RBF move 3/3: move followups + improve RBF documentation #22855

glozow · 2021-09-01T09:07:26Z

Followups to #22675 and documentation-only changes intended to clarify the code/logic concerning mempool Replace-by-Fee.

DrahtBot · 2021-09-02T02:12:17Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22539 (Re-include RBF replacement txs in fee estimation by darosior)

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.

src/policy/rbf.cpp

Document a few non-obvious things and delete no-longer-relevant comments (e.g. about taking a lock that we're already holding). No change in behavior.

fanquake · 2021-09-10T13:55:24Z

Concept ACK. While you're tidying up might as well add all the missing <std> headers to policy/rbf.*, and MAX_BIP125_RBF_SEQUENCE can be constexpr.

ariard · 2021-09-10T22:48:15Z

src/util/rbf.h

@@ -17,7 +17,7 @@ static const uint32_t MAX_BIP125_RBF_SEQUENCE = 0xfffffffd;
 *
 * SEQUENCE_FINAL-1 is picked to still allow use of nLockTime by non-replaceable transactions. All
 * inputs rather than just one is for the sake of multi-party protocols, where we don't want a single
-* party to be able to disable replacement. */
-bool SignalsOptInRBF(const CTransaction &tx);
+* party to be able to disable replacement by opting out in their own input. */


micro-nit "by opting out of their single-owned input". If the input spent is 2-of-2 like LN's commitment you should check that counterparty's signature is covering a RBF-signaling nSequence field.

ariard · 2021-09-10T23:10:04Z

src/validation.cpp

+        // 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.
        if (const auto err_string{PaysMoreThanConflicts(setIterConflicting, newFeeRate, hash)}) {


I think it would interesting to add a comment noting that this higher-feerate than any directly replaced transactions it's not part of the BIP.

Also I think this check might bounce-off higher-fee transactions than the replaced one if the conflict transaction has a higher feerate. Let's say a 2_000 vb/10_000 sats replacement vs a 100 vb/8_000 sats conflicts. Without entering in a discussion on the miner block strategy, this is at lest consistent with BlockAssembler::addPackageTxs which is selecting packages on feerate.

I think it would interesting to add a comment noting that this higher-feerate than any directly replaced transactions it's not part of the BIP.

I thought that's what I was doing by adding this comment 😅 but if you have a more specific wording suggestion I'd be happy to add!

src/policy/rbf.cpp

darosior

ACK 3cf46f6

theStack · 2021-09-16T18:49:48Z

Concept ACK

glozow · 2021-09-21T09:54:23Z

Thanks for the reviews, addressed comments

jnewbery

utACK 0ef08f8

If you retouch this branch, perhaps you could add the justification/motivation to the commit log for commit [policy/refactor] pass in relay fee instead of using global. If the change was suggested in a previous review comment, you could also link to it from the commit log.

fanquake

ACK 0ef08f8

…entation

sdaftuar · 2022-01-01T18:58:16Z

src/validation.cpp

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


Sorry for the late review note, but I think this comment is confusing: in PaysMoreThanConflicts() we're checking that the feerate of the new transaction is higher than its direct conflicts, not that the total fees paid are higher (eg if the new transaction is smaller, the feerate could be higher but the total fees paid can be lower).

Ah thank you, I should have said feerate.

I also misinterpreted the purpose of the code - I was thinking of mining scores=ancestor feerate so I assumed it was an incomplete-but-early-exit check. Based on the context you provided (#23121 (review)), this was checking that the replacement is a better mining candidate based on the block template code at the time. I'll open a PR to update this comment to reflect that.

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/bitcoin#22674 (comment) - Fix a historically inaccurate comment about RBF during the refactors: bitcoin/bitcoin#22855 (comment) - Add a section about package deduplication to policy/packages.md: bitcoin/bitcoin#24152 (comment) and bitcoin/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 #24152) ACKs for top commit: t-bast: LGTM, ACK bitcoin/bitcoin@77202f0 darosior: ACK 77202f0 LarryRuane: ACK 77202f0 Tree-SHA512: a428e791dfa59c359d3ccc67e8d3a4c1239815d2f6b29898e129700079271c00b3a45f091f70b65a6e54aa00a3d5b678b6da29d2a76b6cd6f946eaa7082ea696

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 Docs TX fees and policy labels Sep 1, 2021

This was referenced Sep 1, 2021

RBF move 2/3: extract RBF logic into policy/rbf #22675

Merged

RBF move (1/3): extract BIP125 Rule 5 into policy/rbf #22796

Merged

DrahtBot mentioned this pull request Sep 2, 2021

Re-include RBF replacement txs in fee estimation #22539

Merged

This was referenced Sep 2, 2021

Package Mempool Submission with Package Fee-Bumping #22290

Closed

validation: mempool validation and submission for packages of 1 child + parents #22674

Merged

glozow force-pushed the 2021-09-rbf-docs branch from df285a0 to cecb025 Compare September 8, 2021 08:56

glozow changed the title ~~RBF move 3/3: improve RBF documentation~~ RBF move 3/3: move followups + improve RBF documentation Sep 8, 2021

[policy/refactor] pass in relay fee instead of using global

c78eb86

glozow force-pushed the 2021-09-rbf-docs branch from cecb025 to 7c48d19 Compare September 10, 2021 08:42

glozow marked this pull request as ready for review September 10, 2021 08:43

maflcko reviewed Sep 10, 2021

View reviewed changes

src/policy/rbf.cpp Show resolved Hide resolved

[doc] improve RBF documentation

3cf46f6

Document a few non-obvious things and delete no-longer-relevant comments (e.g. about taking a lock that we're already holding). No change in behavior.

glozow force-pushed the 2021-09-rbf-docs branch from 7c48d19 to 3cf46f6 Compare September 10, 2021 09:32

fanquake requested a review from ariard September 10, 2021 13:55

ariard reviewed Sep 10, 2021

View reviewed changes

darosior reviewed Sep 14, 2021

View reviewed changes

glozow added 2 commits September 20, 2021 13:32

make MAX_BIP125_RBF_SEQUENCE constexpr

c6abeb7

add missing includes in policy/rbf

0ef08f8

jnewbery reviewed Sep 22, 2021

View reviewed changes

fanquake approved these changes Sep 23, 2021

View reviewed changes

fanquake merged commit 8bda5e0 into bitcoin:master Sep 23, 2021

glozow deleted the 2021-09-rbf-docs branch September 23, 2021 09:15

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 24, 2021

Merge bitcoin#22855: RBF move 3/3: move followups + improve RBF docum…

b3d6a8f

…entation

sdaftuar reviewed Jan 1, 2022

View reviewed changes

glozow mentioned this pull request Feb 10, 2022

docs / fixups from RBF and packages #24310

Merged

bitcoin locked and limited conversation to collaborators Jan 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

RBF move 3/3: move followups + improve RBF documentation #22855

RBF move 3/3: move followups + improve RBF documentation #22855

Uh oh!

glozow commented Sep 1, 2021 •

edited

Loading

Uh oh!

DrahtBot commented Sep 2, 2021 •

edited

Loading

Uh oh!

Uh oh!

fanquake commented Sep 10, 2021

Uh oh!

ariard Sep 10, 2021

Uh oh!

ariard Sep 10, 2021

Uh oh!

glozow Sep 15, 2021

Uh oh!

Uh oh!

darosior left a comment •

edited

Loading

Uh oh!

theStack commented Sep 16, 2021

Uh oh!

glozow commented Sep 21, 2021

Uh oh!

jnewbery left a comment

Uh oh!

fanquake left a comment

Uh oh!

sdaftuar Jan 1, 2022

Uh oh!

glozow Jan 4, 2022 •

edited

Loading

Uh oh!

Uh oh!

RBF move 3/3: move followups + improve RBF documentation #22855

RBF move 3/3: move followups + improve RBF documentation #22855

Uh oh!

Conversation

glozow commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

fanquake commented Sep 10, 2021

Uh oh!

ariard Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

ariard Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

glozow Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

darosior left a comment • 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!

theStack commented Sep 16, 2021

Uh oh!

glozow commented Sep 21, 2021

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

sdaftuar Jan 1, 2022

Choose a reason for hiding this comment

Uh oh!

glozow Jan 4, 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!

Uh oh!

glozow commented Sep 1, 2021 •

edited

Loading

DrahtBot commented Sep 2, 2021 •

edited

Loading

darosior left a comment •

edited

Loading

glozow Jan 4, 2022 •

edited

Loading