docs: remove non-signaling mentions of BIP125 #25775

pull glozow wants to merge 2 commits into bitcoin:master from glozow:2022-08-bip125-signal-only changing 15 files +52 −52
  1. glozow commented at 2:35 PM on August 3, 2022: member

    We have pretty thorough documentation of our RBF policy in doc/policy/mempool-replacements.md. It enumerates each rule with several sentences of rationale. Also, each rule pretty much has its own function (3 and 4 share one), with extensive comments. The doc states explicitly that our rules are similar but differ from BIP125, and contains a record of historical changes to RBF policy.

    We should not use "BIP125" as synonymous with our RBF policy because:

    • Our RBF policy is different from what is specified in BIP125, for example:
      • the BIP does not mention our rule about the replacement feerate being higher (our Rule 6)
      • the BIP uses minimum relay feerate for Rule 4, while we have used incremental relay feerate since #9380
      • the "inherited signaling" question (CVE-2021-31876). Call it discrepancy, ambiguous wording, doc misinterpretation, or implementation details, I would recommend users refer to doc/policy/mempool-replacements.md
      • the signaling policy is configurable, see #25353
    • Our RBF policy may change further
    • We have already marked BIP125 as only "partially implemented" in docs/bips.md since 1fd49eb498c75a1d14193bb736d195a3dc75ae12
    • See comments from people who are not me recently:

    This PR removes all non-signaling mentions of BIP125 (if people feel strongly, we can remove all mentions of BIP125 period). It may be useful to refer to the concept of "tx opts in to RBF if it has at least one nSequence less than (0xffffffff - 1)" as "BIP125 signaling" because:

    • It is succint.
    • It has already been widely marketed as BIP125 opt-in signaling.
    • Our API uses it when referring to signaling (e.g. getmempoolentry["bip125-replaceable"] and wallet error message "not BIP 125 replaceable"). Changing those is more invasive.
    • If/when we have other ways to signal in the future, we can disambiguate them this way. See #25038 which proposes another way of signaling, and where I pulled these commits from.

    Alternatives:

    • Changing our policy to match BIP125. This doesn't make sense as, for example, we would have to remove the requirement that a replacement tx has a higher feerate (Rule 6).
    • Changing BIP125 to match what we have. This doesn't make sense as it would be a significant change to a BIP years after it was finalized and already used as a spec to implement RBF in other places.
    • Document our policy as a new BIP and give it a number. This might make sense if we don't expect things to change a lot, and can be done as a next step.
  2. scripted-diff: remove mention of BIP125 from non-signaling var names
    Our RBF policy is different from the rules specified in BIP125 (refer to
doc/policy/mempool-replacements.md instead), and will continue to
diverge with package RBF.  Keep references to BIP125 sequence number,
since that is still useful and correct.

-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }

ren m_allow_bip125_replacement m_allow_replacement
ren allow_bip125_replacement allow_replacement
ren MAX_BIP125_REPLACEMENT_CANDIDATES MAX_REPLACEMENT_CANDIDATES
-END VERIFY SCRIPT-
    32024d40f0
  3. fanquake added the label Docs on Aug 3, 2022
  4. glozow force-pushed on Aug 3, 2022
  5. petertodd commented at 2:50 PM on August 3, 2022: contributor
    • Changing BIP125 to match what we have.

    Another alternative: delete the details from BIP125. The only interesting thing in it is the opt-in part. The rest was just suggestions for how to implement RBF that have turned out to not be ideal. It's not consensus after all.

  6. glozow commented at 2:55 PM on August 3, 2022: member

    Another alternative: delete the details from BIP125. The only interesting thing in it is the opt-in part. The rest was just suggestions for how to implement RBF that have turned out to not be ideal. It's not consensus after all.

    Sure, that makes sense. Though we would still want these changes, because then saying "BIP125 Rule 5" would be quite confusing.

  7. michaelfolkson commented at 3:15 PM on August 3, 2022: contributor

    I think it makes sense to remove references to BIP125 in the code (e.g. variable names). But I don't know why references to BIP125 have to be removed from the comments. The comments can (and should) refer to BIP125 e.g. BIP125 rule X and explain what differences there are between a BIP125 rule and how it is implemented in Core.

    BIP wise, ideally we'd have a replacement BIP or a BIP125 version 2 at some point in future and then the code comments could refer to that. I think the maintaining a historical record argument will prevent widespread changes to BIP125 itself.

  8. ariard commented at 3:28 PM on August 3, 2022: member

    Concept ACK

    Changing BIP125 to match what we have. This doesn't make sense as it would be a significant change to a BIP years after it was finalized and already used as a spec to implement RBF in other places.

    I think I would still recommend to update BIP125 with a historical note linking to this PR if it's accepted to provide awareness to future readers that the replacement policy described by BIP125 is not the one effectively implemented by Core.

    Document our policy as a new BIP and give it a number. This might make sense if we don't expect things to change a lot, and can be done as a next step.

    I don't know about documenting the Core mempool policy as a BIP. I'm quite sure I've argued for that practice in the past though it was at a time we didn't have doc/policy consistently and rigorously describing policy, in an optic to be consumed by wallet or second-layers developers. Now, we would have two sets of document to keep in-sync. There is also the fact that raising our policy documentation as a BIP "best" standard could give an inaccurate perception across the ecosystem that Core policy is "authoritative" and discourage experimentation or divergent policy deployment by other Bitcoin softwares. Of course, I think P2P extensions still deserve a BIP to achieve an unified P2P network. No strong opinions here.

  9. michaelfolkson commented at 4:00 PM on August 3, 2022: contributor

    I'm quite sure I've argued for that practice in the past though it was at a time we didn't have doc/policy consistently and rigorously describing policy, in an optic to be consumed by wallet or second-layers developers.

    Documentation in this repo is often an afterthought despite best intentions. My concern would be that without a (relatively) static formal document outlining what rules are being followed and why the rationale for policy changes won't be clear. Regular unscrutinized changes and tweaks will become the norm and make them much harder to monitor and analyze.

    There is also the fact that raising our policy documentation as a BIP "best" standard could give an inaccurate perception across the ecosystem that Core policy is "authoritative" and discourage experimentation or divergent policy deployment by other Bitcoin softwares.

    Other than on consensus no BIP is attempting to be authoritative. As the dominant implementation on the network Core is setting the default policy rules for the vast majority of nodes and attempting to maintain compatibility with a BIP means more scrutiny and an alternative point of reference to compare to. (The recent exercise of comparing current Core code to BIP125 for example was much more rigorous than it would have been if comparing Core code to an internal Core doc that may or may not be up to date.)

    edit: Maybe it is acceptable (or even desirable) for the RBF rules to be in a constant state of flux. There have been multiple ideas/proposals on changing the RBF rules in recent months. In that case a new BIP clearly does not make sense. But if the aspiration is to have a set of RBF rules with strong rationales that don't regularly change, moving from having a BIP to internal Core documentation seems like an erosion of scrutiny to me.

  10. murchandamus commented at 4:20 PM on August 3, 2022: contributor

    Concept ACK

    It would feel a bit strange to refer to the rules by № if there was no documentation that comprehensively enumerates them as such anymore. If e.g. the details were deleted from BIP125, should the rules perhaps be listed in code documentation? Otherwise, a new BIP to supersede BIP125 and document the actual behavior would sound useful.

  11. glozow commented at 4:34 PM on August 3, 2022: member

    I don't know why references to BIP125 have to be removed from the comments. The comments can (and should) refer to BIP125 e.g. BIP125 rule X and explain what differences there are between a BIP125 rule and how it is implemented in Core.

    It would feel a bit strange to refer to the rules by № if there was no documentation that comprehensively enumerates them as such anymore. If e.g. the details were deleted from BIP125, should the rules perhaps be listed in code documentation? Otherwise, a new BIP to supersede BIP125 and document the actual behavior would sound useful.

    Not sure if this is widely known, but doc/policy/mempool-replacements.md lists each rule with several sentences of rationale. Each rule pretty much has its own function (3 and 4 share one), with extensive comments. The doc states explicitly that our rules are similar but differ from BIP125, and contains a record of historical changes to RBF policy. Comments about what code doesn't do and referencing BIPs it doesn't implement is just clutter and confusing.

  12. michaelfolkson commented at 4:45 PM on August 3, 2022: contributor

    Maybe it is acceptable (or even desirable) for the RBF rules to be in a constant state of flux. There have been multiple ideas/proposals on changing the RBF rules in recent months. In that case a new BIP clearly does not make sense. But if the aspiration is to have a set of RBF rules with strong rationales that don't regularly change, moving from having a BIP to internal Core documentation seems like an erosion of scrutiny to me. @glozow: Perhaps you can comment on the above then. There is a difference between cross checking code with a formal static doc like a BIP and updating regularly changing documentation (assuming one remembers to). Otherwise there is no argument to BIP or spec anything in Core whatsoever (except maybe consensus changes).

  13. murchandamus commented at 4:50 PM on August 3, 2022: contributor

    Not sure if this is widely known, but doc/policy/mempool-replacements.md lists each rule with several sentences of rationale.

    Oh great! It wasn't clear to me from the first post's description that this already existed—you could perhaps highlight that more in the first post. Given that most of the nodes on the network are Bitcoin Core, I'm not sure whether a BIP is strictly needed in that case then, but a BIP might still be a bit easier to discover. Since it's bad to have two diverging authoritative sources, I would agree that a BIP should wait until the rules aren't expected to change much anymore.

  14. darosior commented at 8:45 AM on August 4, 2022: member

    Concept ACK

  15. glozow commented at 10:35 AM on August 4, 2022: member

    Oh great! It wasn't clear to me from the first post's description that this already existed—you could perhaps highlight that more in the first post.

    Thanks, done!

    if the aspiration is to have a set of RBF rules with strong rationales that don't regularly change, moving from having a BIP to internal Core documentation seems like an erosion of scrutiny to me. There is a difference between cross checking code with a formal static doc like a BIP and updating regularly changing documentation (assuming one remembers to). Otherwise there is no argument to BIP or spec anything in Core whatsoever (except maybe consensus changes).

    Putting code documentation in a BIP does not inherently make it more correct or up-to-date. There is a pretty large logical leap here from "docs should change when code changes" to "there's no point in writing a spec." Feel free to open a PR if you see a divergence between code and documentation, which is what this PR is doing.

  16. michaelfolkson commented at 11:21 AM on August 4, 2022: contributor

    @glozow: You're not answering the question on whether the aim is RBF rules that are constantly in flux which lends itself to internal Core documentation or whether the aim is to have a set of static RBF rules with clear rationales that are universally well understood by the ecosystem which lends itself to a BIP.

    I'll write something up for the mailing list as this desire to move from BIPs to internal Core documentation has concerned me for a while and it won't be resolved on this PR.

    If there is a purge of all references to RBF rules being BIPed in this repo then there are additional references to BIP125 in the wallet and test code.

    I will probably draft a BIP assuming I can work out if and when we have a set of replacement RBF rules for BIP 125 that are intended to be static. This is not a criticism of that work, it is important, challenging and difficult to follow. But when security of L2 protocols are depending on it (whether we like it or not) and it is possible to achieve I do think a set of static RBF rules with clear rationales is what we should be aiming for.

  17. [doc] remove non-signaling mentions of BIP125
    Our RBF policy is different from the rules specified in BIP125. For
example, the BIP does not mention Rule 6, and our Rule 4 uses the
(configurable) incremental relay feerate (distinct from the
minimum relay feerate). Those interested in our policy should refer to
doc/policy/mempool-replacements.md instead. These rules may also
continue to diverge with package RBF and other RBF improvements. Keep
references to the BIP125 signaling wrt sequence numbers, since that is
still correct and widely used. It is helpful to refer to this as "BIP125
signaling" since it is unambiguous and succint, especially if we have
multiple ways to signal replaceability in the future.

The rule numbers in doc/policy/mempool-replacements.md correspond
largely to those of BIP 125, so we can still refer to them like "Rule 5."
    1dc03dda05
  18. glozow force-pushed on Aug 4, 2022
  19. DrahtBot commented at 4:24 AM on August 5, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #25718 (net: Add -allowinbound config option by fjahr)
    • #25038 (policy: nVersion=3 and Package RBF 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.

  20. fanquake requested review from instagibbs on Aug 5, 2022
  21. instagibbs commented at 1:59 PM on August 5, 2022: member

    Given the document doc/policy/mempool-replacements.md already exists(and explains the relationship to bip125), concept ACK

  22. michaelfolkson commented at 8:23 AM on August 8, 2022: contributor

    Concept ACK. I have reservations but seems there are good reasons not to BIP RBF rules anymore and seems I was in the minority anyway.

  23. t-bast commented at 2:55 PM on August 8, 2022: contributor

    Concept ACK

  24. darosior commented at 8:05 AM on August 11, 2022: member

    ACK 1dc03dda05e9dce128e57f05bb7b1bb02b3cfb9e

    This PR removes all non-signaling mentions of BIP125 (if people feel strongly, we can remove all mentions of BIP125 period).

    I'd be in favour of this. BIP125 caused too much confusion. But i don't feel strongly, the signaling part will hopefully become less important as we go and could be slowly phased out.

  25. in src/wallet/rpc/spend.cpp:336 in 1dc03dda05 
     332 | @@ -333,7 +333,7 @@ RPCHelpMan sendmany()
 333 |                              {"address", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Subtract fee from this address"},
 334 |                          },
 335 |                      },
 336 | -                    {"replaceable", RPCArg::Type::BOOL, RPCArg::DefaultHint{"wallet default"}, "Allow this transaction to be replaced by a transaction with higher fees via BIP 125"},
 337 | +                    {"replaceable", RPCArg::Type::BOOL, RPCArg::DefaultHint{"wallet default"}, "Signal that this transaction can replaced by a transaction (BIP 125)"},
    ariard commented at 4:09 PM on August 21, 2022:

    I wonder if you should update the argument descriptions similarly in TransactionDescriptionString() (L420 in src/wallet/rpc/transactions.cpp) and in MempoolEntryDescription() (L258 in src/rpc/mempool.cpp)

    If yes, can be done as a follow-up.

    MarcoFalke commented at 10:03 AM on August 22, 2022:

    Same?

    glozow commented at 2:08 PM on August 22, 2022:

    Thanks, done in #25902

    glozow commented at 2:08 PM on August 22, 2022:

    Thanks, done in #25902

  26. ariard approved
  27. ariard commented at 4:15 PM on August 21, 2022: member

    ACK 1dc03dda

    This PR removes all non-signaling mentions of BIP125 (if people feel strongly, we can remove all mentions of BIP125 period).

    I would favor too slow phase out of even the signaling part of BIP125.

  28. t-bast approved
  30. fanquake merged this on Aug 22, 2022
  31. fanquake closed this on Aug 22, 2022
  32. in src/policy/rbf.h:22 in 1dc03dda05 
      18 | @@ -19,9 +19,9 @@
  19 |  class CFeeRate;
  20 |  class uint256;
  21 |  
  22 | -/** Maximum number of transactions that can be replaced by BIP125 RBF (Rule #5). This includes all
  23 | +/** Maximum number of transactions that can be replaced by RBF Rule #5. This includes all
    MarcoFalke commented at 10:03 AM on August 22, 2022:

    I think it was easier to parse with the () kept.

    glozow commented at 2:08 PM on August 22, 2022:

    Thanks, done in #25902

  33. in src/wallet/rpc/spend.cpp:227 in 1dc03dda05 
     223 | @@ -224,7 +224,7 @@ RPCHelpMan sendtoaddress()
 224 |                                           "transaction, just kept in your wallet."},
 225 |                      {"subtractfeefromamount", RPCArg::Type::BOOL, RPCArg::Default{false}, "The fee will be deducted from the amount being sent.\n"
 226 |                                           "The recipient will receive less bitcoins than you enter in the amount field."},
 227 | -                    {"replaceable", RPCArg::Type::BOOL, RPCArg::DefaultHint{"wallet default"}, "Allow this transaction to be replaced by a transaction with higher fees via BIP 125"},
 228 | +                    {"replaceable", RPCArg::Type::BOOL, RPCArg::DefaultHint{"wallet default"}, "Signal that this transaction can replaced by a transaction (BIP 125)"},
    MarcoFalke commented at 10:03 AM on August 22, 2022:

    can replaced?

    glozow commented at 2:08 PM on August 22, 2022:

    Thanks, done in #25902

  34. MarcoFalke commented at 10:04 AM on August 22, 2022: member

    Is the grammar right, as I can't parse some sentences?

  35. glozow deleted the branch on Aug 22, 2022
  36. MarcoFalke referenced this in commit 92bb7001d8 on Aug 22, 2022
  37. sidhujag referenced this in commit 2ed5925e2a on Aug 22, 2022
  38. fanquake added this to the milestone 24.0 on Sep 15, 2022
  39. bitcoin locked this on Sep 15, 2023
Contributors
glozowpetertoddmichaelfolksonariardmurchandamusdarosiorDrahtBotinstagibbst-bastMarcoFalke


instagibbs

Labels
Docs

Milestone
24.0

Linked (view graph)
#9380 Separate different uses of minimum fees#25038 policy: nVersion=3 and Package RBF#25353 Add a `-mempoolfullrbf` node setting#25575 Address comments remaining from #25353#25902 fixups for BIP125 doc cleanup
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 21:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me