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

glozow commented at 9:07 am on September 1, 2021: member

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

glozow added the label Docs on Sep 1, 2021

glozow added the label TX fees and policy on Sep 1, 2021

DrahtBot commented at 2:12 am on September 2, 2021: member

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.

glozow force-pushed on Sep 8, 2021

glozow renamed this:
~~RBF move 3/3: improve RBF documentation~~
RBF move 3/3: move followups + improve RBF documentation
on Sep 8, 2021

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

glozow force-pushed on Sep 10, 2021

glozow marked this as ready for review on Sep 10, 2021

[doc] improve RBF documentation

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.

3cf46f6055

in src/policy/rbf.cpp:57 in 7c48d198f9 outdated

56@@ -57,17 +57,18 @@ std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx,
57     uint64_t nConflictingCount = 0;

MarcoFalke commented at 8:48 am on September 10, 2021:

a few lines up: Could also fixup the whitespace after the refactors in 2/3? #22675#pullrequestreview-747002094

glozow commented at 9:33 am on September 10, 2021:

Oh right! Done, sorry I missed that

glozow force-pushed on Sep 10, 2021

fanquake requested review from ariard on Sep 10, 2021

fanquake commented at 1:55 pm on September 10, 2021: member

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.

in src/util/rbf.h:20 in 3cf46f6055 outdated

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

ariard commented at 10:48 pm on September 10, 2021:

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.

in src/validation.cpp:789 in 3cf46f6055 outdated

789+        // than all conflicts (i.e. the direct conflicts have high-fee descendants). However, if the
790+        // replacement doesn't pay more fees than its direct conflicts, then we can be sure it's not
791+        // more economically rational to mine. Before we go digging through the mempool for all
792+        // transactions that would need to be removed (direct conflicts and all descendants), check
793+        // that the replacement transaction pays more than its direct conflicts.
794         if (const auto err_string{PaysMoreThanConflicts(setIterConflicting, newFeeRate, hash)}) {

ariard commented at 11:10 pm on September 10, 2021:

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.

glozow commented at 10:31 am on September 15, 2021:

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 :sweat_smile: but if you have a more specific wording suggestion I’d be happy to add!

in src/policy/rbf.cpp:157 in 3cf46f6055 outdated

155                                       const uint256& txid)
156 {
157-    // The replacement must pay greater fees than the transactions it
158-    // replaces - if we did the bandwidth used by those conflicting
159-    // transactions would not be paid for.
160+    // BIP125 Rule #3: The replacement fees must be greater than or equal to fees of the

ariard commented at 11:10 pm on September 10, 2021:

See #22675 (review)

glozow commented at 10:42 am on September 15, 2021:

#22675 (review)

darosior commented at 6:47 pm on September 14, 2021: member

ACK 3cf46f6055f7cd2e5da81e0d29cafc51ad4aafba

theStack commented at 6:49 pm on September 16, 2021: member

Concept ACK

make MAX_BIP125_RBF_SEQUENCE constexpr c6abeb76fb

add missing includes in policy/rbf 0ef08f8bed

glozow commented at 9:54 am on September 21, 2021: member

Thanks for the reviews, addressed comments

jnewbery commented at 1:22 pm on September 22, 2021: member

utACK 0ef08f8bed537435f3f9db1e38b7d6f3551fe830

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 approved

fanquake commented at 8:40 am on September 23, 2021: member

ACK 0ef08f8bed537435f3f9db1e38b7d6f3551fe830

fanquake merged this on Sep 23, 2021

fanquake closed this on Sep 23, 2021

glozow deleted the branch on Sep 23, 2021

sidhujag referenced this in commit b3d6a8fe30 on Sep 24, 2021

in src/validation.cpp:788 in 0ef08f8bed

788+        // It's possible that the replacement pays more fees than its direct conflicts but not more
789+        // than all conflicts (i.e. the direct conflicts have high-fee descendants). However, if the
790+        // replacement doesn't pay more fees than its direct conflicts, then we can be sure it's not
791+        // more economically rational to mine. Before we go digging through the mempool for all
792+        // transactions that would need to be removed (direct conflicts and all descendants), check
793+        // that the replacement transaction pays more than its direct conflicts.

sdaftuar commented at 6:58 pm on January 1, 2022:

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

glozow commented at 4:25 pm on January 4, 2022:

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 (https://github.com/bitcoin/bitcoin/pull/23121#pullrequestreview-842354501), 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.

fanquake referenced this in commit bc49650b7c on Feb 22, 2022

sidhujag referenced this in commit b2aae5647b on Feb 22, 2022

DrahtBot locked this on Jan 4, 2023

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

Conflicts