Address comments remaining from #25353 #25575
Conversation
I think there's some confusion about why this RPC was implemented this way in the first place. The purpose was to allow a node operator to be able to determine whether a given transaction might be replaceable due to BIP 125 RBF semantics. Since replacing a parent causes all descendants to be removed from the mempool, this logic checks all the unconfirmed ancestors of a transaction to see if they are replaceable. I understand the confusion this has created for developers who may have understood the BIP to mean that it should be sufficient to conflict with such a non-signaling child transaction and be able to achieve RBF semantics. However, I think that is an oversight in documentation and maybe our release notes for what is intended with this logic, and not a mistake in the logic itself. I think as long as we support BIP 125 RBF, this RPC is doing what is intended (and hopefully we will eventually drop BIP 125 altogether and we can eliminate this sort of confusion!). |
|
NACK. A bool has two states, and the confusion around this is based on a misinterpretation of at least three states (tx can be replaced "by itself", tx can be replaced "via parent", tx can't be bip125 replaced). Toggling the bool doesn't help anything in fixing the underlying misunderstanding. If you really want to fix this you may offer the consumer a way to determine the state. I think it should be sufficient to just return the two boolean values |
getmempoolentry inaccurate inheritance signaling status|
Updated at 1056bbd with comments addressed and dropped the fix So from reviewing back #22698, it wasn't clear to me what was the state of the thinking on fixing the discrepancy between the BIP 125 documentation and our replacement logic. Effectively, if a non-signaling child transaction marked as "bip125-replaceable" should be interpreted as "tx can be replaced via parent', I agree it can be an acceptable-though-non-perfect result. It might still confuses developers attempting to replace the non-signaling child via an assumed "inherited signaling" and I believe it would be good to update or refresh BIP125 on that regard (though I don't plan to do so). It should be noted "inherited signaling" is implemented by other implementation so I think any update to BIP125 might require coordination beyond the scope of the project. As suggested, we could enrich |
6a547cb to
1056bbd
Compare
|
cr ACK 1056bbd |
|
ACK 1056bbd Thanks for splitting the PRs. Confirmed this resolves:
CI error in wallet_encryption.py is unrelated, since this only touches feature_rbf.py and docs. |
…ENCE_NUMBER to MAX_BIP125_RBF_SEQUENCE faace13 test: Remove duplicate MAX_BIP125_RBF_SEQUENCE constant (MacroFake) fa0404d scripted-diff: [test] Rename BIP125_SEQUENCE_NUMBER to MAX_BIP125_RBF_SEQUENCE (MacroFake) Pull request description: Not sure why the python constant is named differently than the constant in the C++ source code. Especially, if we use this in context of MAX+1 (bitcoin/bitcoin#25575 (comment)) the rename makes sense, in my eyes. ACKs for top commit: theStack: Code-review ACK faace13 glozow: concept ACK faace13 Tree-SHA512: 1dc8cd0f067717f6ace8121b660e99f2d0f8c485c0d61b80413e07e7830e7dfaf9ce2c922c63ba2c6b42e805d59d88cd0d9c80a4b4a2fca47e77a3aba6cd4ec6
…IP125 1dc03dd [doc] remove non-signaling mentions of BIP125 (glozow) 32024d4 scripted-diff: remove mention of BIP125 from non-signaling var names (glozow) Pull request description: 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 1fd49eb - See comments from people who are not me recently: - bitcoin/bitcoin#25038 (comment) - bitcoin/bitcoin#25575 (comment) 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. ACKs for top commit: darosior: ACK 1dc03dd ariard: ACK 1dc03dd t-bast: ACK bitcoin/bitcoin@1dc03dd Tree-SHA512: a3adc2039ec5785892d230ec442e50f47f7062717392728152bbbe27ce1c564141f85253143f53cb44e1331cf47476d74f5d2f4b3cd873fc3433d7a0aa783e02
This PR should address the remaining comments from #25353.