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

  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

  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.

 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!

 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)

 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.