After the implementation was merged, the mailing list description was
written up as a BIP, and merged
into the BIPs repository as
BIP125.
The RBF policy in Bitcoin Core uses the nSequence field to signal opt-in for replaceability. According to the BIP:
This policy specifies two ways a transaction can signal that it is replaceable.
Explicit signaling: A transaction is considered to have opted in to
allowing replacement of itself if any of its inputs have an nSequence
number less than (0xffffffff - 1).
Inherited signaling: Transactions that don’t explicitly signal
replaceability are replaceable under this policy for as long as any one of
their ancestors signals replaceability and remains unconfirmed.
It was recently
discovered
that the Bitcoin Core implementation does not treat the descendants of
unconfirmed, opt-in RBF-signaling transactions as implicitly signaling
for replaceability.
PR 21946 added code comments
to document the discrepancy between BIP125 and the implementation.
PR 22665 replaces the
bip125_replaceable field in the mempool and wallet RPC commands with a
replaceable field. The bip125_replaceable field returned whether a
transaction was replaceable according to the BIP125 rules (including through
inherited signaling), while the new replaceable field returns whether
the Bitcoin Core mempool considers the transaction replaceable.
There’s an alternative PR
22698 which implements the
inherited signaling as documented in BIP125. Which of the two approaches do you
prefer? Why?
Prior to this PR, there was a Chain interface member function
isRBFOptIn().
This PR changes the caller to use SignalsOptInRBF() instead. Why is
SignalsOptInRBF() a free function and not a member of the Chain
interface class?
Before this PR, the bip125-replaceable field could be yes, no, or
unknown. The new replaceable field can only be true or false. Why is
it never set to unknown?
What is the IsDeprecatedRPCEnabled() function used for? Why does this
PR move that function from rpc/server to rpc/util? Describe the process
for deprecating RPC methods and fields. Why do we deprecate in this way?
Do you agree that we should use a deprecation process to change the name
from bip125-replaceable to replaceable? Why don’t we just update the
value that is returned in bip125-replaceable?
<michaelfolkson> I guess doing something about the "CVE" is the Concept ACK. And then the Approach ACK is which of the two PRs. If so it is a Concept ACK
<michaelfolkson> There are a few discussions to be had on this one. If BIPs are being used by other implementations and Core code doesn't follow the BIP...
<jnewbery> this ties nicely with the next question: There’s an alternative PR 22698 which implements the inherited signaling as documented in BIP125. Which of the two approaches do you prefer? Why?
<raj> Also there can be downstream wallets that depends on the BIP described behavior for their operation. Either directly on core, or via their own implementation.
<raj> jnewbery, because if I have an ancestor replaceable by fee, I would expect replacing it would also replace the descendants. Thus descendants naturally inherits the replacement.
<darius27> if BIP 125 was merged, doesn't it mean it was decided at that point that that was the more desired behavior? And it seems like it was unintentional that bitcoin core did not implement this behavior. So yeah i would have thought it would make sense to implement BIP 125 instead
<raj> jnewbery, but doesn't the fact that replaceability inheritance documented in the BIP irrespective of code behavior, makes that it a more intuitive behavior? One could equally argue that the code missed a behavior which is logical?
<jnewbery> I think it's worth reviewing the discussion in https://github.com/bitcoin/bitcoin/pull/7222, which added the bip125-replaceable field to the wallet RPCs. It's a little subtle, but I think there are definitely arguments against saying that the descendant of a bip125-replaceable is itself replaceable
<theStack> side-question: are there any other cases of bitcoin improvements where the BIP was following/documenting the implementation, rather than the other way round?
<michaelfolkson> From scanning that IRC conversation log I think the BIP authors just agree to write a BIP after the code has been written rather than state they will write a BIP to document what the code does
<michaelfolkson> Well in a world with multiple implementations that is the point of the BIPs. It isn't documentation of Core entirely (although it is hopefully that as well)
<jnewbery> theStack: I think often the BIP will be written in parallel with the implementation. There's often an "implementation" section of the BIP that links to an (unmerged) branch
<raj> glozow, let me know if i have it wrong, if I have a descendant with RBF disables, but have one of its ancestor RBF enables, replacing that ancestor would remove the descendant from mempool. Yet when I query its RBF status, I would get false. Isn't that correct?
<jnewbery> raj: that's true, but the ancestor could also be conflicted and removed from the mempool, along with all of its descendants, regardless of whether they're signaling replaceability
<michaelfolkson> If another implementation comes along and implements it according to the BIP, it is a little awkward to say "Oh sorry you should have ignored the BIP and just copied what Core did"
<jnewbery> remember that replaceability is simply a local policy. Miners or other nodes can decide what policy they have for replaceability, which may be BIP125, or something completely different
<raj> On BIPS: I don't thinks BIPs are (or should) be written to document code behavior. BIPS are suppose to be standards that other parts of the industry can follow knowing that it will not make any breaking behavior change. Which is violated in this case.
<jnewbery> I think it's unusual that a BIP was written for a local mempool policy. I agree that it should be documented, but a BIP doesn't seem like the right place for it. Implementers should be free to update their policy at any time, without reference to BIPs (again, those policies should be well documented)
<theStack> jnewbery: yes, i can remember seeing an implementation section some BIPs. i just thought that there is a rather strict rule that the BIP has to be merged first
<jnewbery> theStack: For consensus changes or P2P protocol changes, I'd agree - the specification (BIP) should be finalized before the code is merged into Bitcoin Core
<darius27> jnewbery - RE "I think it's unusual that a BIP was written for a local mempool policy". Isn't this similar to having BIPs for standardness rules? Or am i missing something
<michaelfolkson> sipa: I think that kind of perspective cripples the value of the BIPs personally. Why bother with them? Alternative implementations should just look at Core code as the BIPs are unreliable
<jnewbery> sipa: I think "an unambigous way of referring to that idea" is more useful when the idea is probably going to be mostly fixed (ie consensus or p2p protocol). For a policy, it's much more likely that implementers will make future changes in the implementation, in which case the BIP can potentially become more harmful than useful.
<michaelfolkson> Lightning is a very different world (not one dominant implementation) but they seem to take greater care with their BOLTs and have a different perspective on what they are attempting to achieve
<raj> sipa, But there are BIPs at different status. Some are accepted some are drafts, some are rejected. Can a accepted BIP (an implemented) become a bad idea?
<jnewbery> Prior to this PR, there was a Chain interface member function isRBFOptIn(). This PR changes the caller to use SignalsOptInRBF() instead. Why is SignalsOptInRBF() a free function and not a member of the Chain interface class?
<sipa> i'm not all that happy personally about the status of BIPs; apart from consensus bips for which there is an objective determination of "in use", it seems the bip status mostly reflects whether the author took the time to update ot
<jnewbery> if we're considering a transaction's unconfirmed ancestors, then we need mempool state, so the wallet needs to go through the Chain interface
<jnewbery> if we're _just_ looking at the nSequence fields in the transaction itself, then we don't need any context, so we can just call a utility function
<jnewbery> ok, next question. Before this PR, the bip125-replaceable field could be yes, no, or unknown. The new replaceable field can only be true or false. Why is it never set to unknown?
<jnewbery> especially for something like bip125 replacebility, it's really not a property of the CTransaction, but rather how we decide to interpret the CTransaction
<jnewbery> glozow: I think rpc/server is code for running on the node. rpc/util may also be run in the bitcoin-cli client. I'm a bit rusty on that though
<jnewbery> Which leads us nicely to the next question. What is the IsDeprecatedRPCEnabled() function used for? Why does this PR move that function from rpc/server to rpc/util? Describe the process for deprecating RPC methods and fields. Why do we deprecate in this way?
<raj> jnewbery, it checks a list of strings to find deprecated methods. If a method is found in the list, then only corresponding functions are called and results are displayed (with an warning that it has been deprecated). Which gives a nice non breaking interface for downstream users to adopt the breaking changes.
<jnewbery> ok, final question. Do you agree that we should use a deprecation process to change the name from bip125-replaceable to replaceable? Why don’t we just update the value that is returned in bip125-replaceable?
<michaelfolkson> We didn't (really) discuss the alternative PR but I think long term we go with the superior solution (assuming we can get consensus on what the superior solution is). And if that means changing the code and/or the BIP we do that
<jnewbery> raj: I'm not sure. As sipa says, BIPs are just there to document ideas. BIP125 remains unchanged as an idea whatever we decide to implement in Bitcoin Core.
<sipa> BIP125 is a name given to an idea; bitcoin core does not implement that idea; the solution is either documenting that bitcoin core does not implement bip125, or perhaps writing another BIP that does (if people feel that idea is BIP-worthy). Under no circumstances can BIP125 be changed to suddenly mean something else
<sipa> if it were in draft status, and still subject to change, that'd be different, but changing BIP125 now is both impossible (per BIP2) and would be extremely confusing
<jnewbery> I don't think a new BIP is necessary - as long as Bitcoin Core clearly documents its policy, that's enough. There's no reason it should be in the BIPs repository.