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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #22798 (doc: Fix RPC result documentation by MarcoFalke)
• #22796 (RBF move (1/3): extract BIP125 Rule 5 into policy/rbf by glozow)
• #22788 (scripted-diff: Use generate* from TestFramework by MarcoFalke)
• #22698 (Implement RBF inherited signaling and fix getmempoolentry returned bip125-replaceable status by mjdietzx)
• #22689 (rpc: deprecate top-level fee fields in getmempool RPCs by josibake)
• #22677 ([RFC] cut the validation <-> txmempool circular dependency by glozow)
• #22675 (MOVEONLY policy: extract RBF logic into policy/rbf by glozow)
• #22650 (Remove -deprecatedrpc=addresses flag and corresponding code/logic by mjdietzx)
• #22290 (Package Mempool Submission with Package Fee-Bumping by glozow)
• #21260 (wallet: indicate whether a transaction is in the mempool by danben)
• #21245 (rpc: Add level 3 verbosity to getblock RPC call. by fyquah)
• #19792 (rpc: Add dumpcoinstats by fjahr)

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.

my main concern is that existing applications/users might currently rely on bip125-replaceable for this type of behavior

If they rely on this, then they are already broken (as demonstrated by CVE-2021-31876). This is the point of this PR actually, and i would argue it is actually a bugfix.

If they rely on this, then they are already broken (as demonstrated by CVE-2021-31876). This is the point of this PR actually, and i would argue it is actually a bugfix.

I'm specifically referring to this comment in MemPoolAccept::PreChecks:

// Applications relying on first-seen mempool behavior should
// check all unconfirmed ancestors; otherwise an opt-in ancestor
// might be replaced, causing removal of this descendant.

Before your PR, applications could do this check by just reading the result of {RPCResult::Type::STR, "bip125-replaceable", "(\"yes|no|unknown\") because isRBFOptIn went through all the transaction's ancestors and checked if they are opt-in.

But after your PR, this is no longer the case. I see that your PR fixes the bug from the perspective of "I know I can replace this transaction based on the value of bip125-replaceable, and it wont get unexpectedly rejected". But I do see it as a change in behavior from the perspective of "I'm an application that relies on first-seen mempool behavior, and I'm looking at the value of bip125-replaceable to know if this transaction might be removed due to an opt-in unconfirmed parent transaction being replaced somewhere up the ancestry tree"

Is this commit message: since we don't need the mempool anymore since we don't need the mempool anymore.. OK..?

I think it would be better to add a new RPC field "replaceable" (since it's not really BIP125) to return this as a bool result

Good call. Done in a50a332.

Also applied your and @Talkless 's comments.

This was already done as part of announcing the CVE

The approach that Core would be taking to address the discrepancy between the BIP and the Core implementation surely wasn't?

I don't think this is tricky at all.

Under the assumption that Core will maintain its current mempool behavior (versus changing it to meet the BIP wording because that is deemed superior logic) I agree it isn't tricky. But making that assumption in the first place seems tricky to me. A short term stopgap with minimal code changes in anticipation of full RBF or a more invasive code change for what some deem superior logic in the meantime as full RBF might never land. (I think that's where we are anyway). Let's at least be transparent about that rather than glossing over it.

Fixes #22209, kinda followup of #21946.

Does it really fix #22209? The new replacable field still doesn't accurately reflect whether a transaction can be replaced by a higher fee tx. See #22809. Both fields (the newly added one and the removed one in this pull) are needed to say that. And for users to interpret the fields for practical purposes, they will also need to know who controls the keys that were used to sign the inputs of the transactions.

Does it really fix #22209? The new replacable field still doesn't accurately reflect whether a transaction can be replaced by a higher fee tx.

The replaceable field does reflect whether a transaction signals for RBF. Maybe it'd be wiser that I update the documentation to explicitly state it, rather than maybe implying (i don't parse it like that but maybe it could be confused as such, idk) that !replaceable is an assurance that "your transaction won't be evicted not matter what" which we of course can't provide.
However i'm curious what should be done to fix #22209. But happy to remove the link from the OP (updated it).

Both fields (the newly added one and the removed one in this pull) are needed to say that

The newly added one is a subset of the removed one here, so i'm not sure to get how having both would help.

!replaceable is an assurance that "your transaction won't be evicted not matter what" which we of course can't provide.

I know that the new field doesn't do that. My point was that with the old field, you could see !bip125-replaceable and deduce that with the state of the blockchain at the time of the call and the policy rules of your node, the tx won't be replaced by another transaction with a higher fee in the mempool. (Obviously that doesn't mean you or your wallet should trust such a tx, but this discussion seems unrelated to returning the information for the raw mempool).

However i'm curious what should be done to fix #22209.

I have no idea what is needed to fix the issue. Maybe the reason should be reported as an enum? #16490?

The states could be:

• opt-in-signal
• rbf-inheritance
• (full-rbf)
• (timeout-rbf)
• ...

The ones in braces are for not-(yet) Bitcoin Core policies.

Fixed the bad english in the bip125-replaceable doc and removed needless includes in the fuzz target after @jnewbery 's review. Thanks!

I think the discussion on the Approach (N)ACK part of this PR and comparing the approaches of #22665 versus #22698 has been weak and almost dismissive (the BIP isn't a standard etc). In my view @mjdietzx @rajarshimaitra have a valid perspective including in @mjdietzx's case coding up and opening an alternative PR.

@laanwj said on IRC:

There are no other mempool policy implementations in wide use

There's a lot of inertia involved in trying to change the implementation, if something gets merged it will take a long time before that version is widely spread on the network to be behavior other software can rely on

From the above I'm an Approach ACK on this PR meeting shorter term user needs and think a rebased #22698 can/should be considered post the merging of this PR. But my gosh, unless we want to ditch the Concept ACK, Approach ACK part of the PR review process I'd hope we can do better in these kinds of scenarios where someone presents a competing PR....

Great comment too earlier from @glozow on this PR (which was deleted but I don't think it should have been)

I don't think this is tricky at all. Not thinking about the BIP while reviewing this PR:

In our mempool code, a transaction that doesn't signal RBF is not opting in. IsRBFOptIn returns a result that is inconsistent with what our mempool will do. Since the RPC code uses IsRBFOptIn(), it returns bip125-replaceable as "yes" or "unknown" for mempool transactions that are not actually RBFable. This can be very confusing and frustrating for users. We should not just change the result of bip125-replaceable, as this can also be confusing. So this PR adds a new field, replaceable, which tells users whether or not our mempool code thinks this transaction opts in to RBF.

Rebased after #22796 merge.

I think there is a difference between "This tx can be evicted due to mempool size if there are 300 MB worth of txs on top of it" and "This tx can be replaced by a single tx that pays a higher fee than the tx(s) it evicts".

I think it makes sense to distinguish the mempool removal reasons, as it is done internally in the code as well:

enum class MemPoolRemovalReason {
    EXPIRY,      //!< Expired from mempool
    SIZELIMIT,   //!< Removed in size limiting
    REORG,       //!< Removed for reorganization
    BLOCK,       //!< Removed for block
    CONFLICT,    //!< Removed for conflict with in-block transaction
    REPLACED,    //!< Removed for replacement
};

NACK from me on the changes here.

Sigh. Removal reason has nothing to do with this PR.

I have no intention to continue tweaking the RPC code nor keeping up with your nitpicks, i initially picked this up because the current interface is wrong and misleading hence potentially dangerous for downstream users. But as you wish, i guess.

Thanks to everyone who helped moving this forward. This is up for grabs, i guess.

Sigh. Removal reason has nothing to do with this PR.

This was a reply to the previous comment (#22665 (review)), which mentioned that txs can be evicted from the mempool for reaching the size limit.

But as you wish, i guess.

I hope you didn't close this pull request because I left a comment with a concern (which is not a nitpick). I want to apologise for leaving nitpicks before reviewing the concept of the pull.

the current interface is wrong and misleading

I agree. Though, I also think that the interface proposed in this pull request isn't crystal clear about the fee replacability status of the tx, which is why I raised the concern.

@MarcoFalke sorry for overreacting.

I also think that the interface proposed in this pull request isn't crystal clear about the fee replacability status of the tx

Ok, then we could go for the minimal fix (which i initially proposed, to stop returning "yes" or "unknown" if the transaction is not replaceable), and introduce the new interface in a following PR?

Regarding the interface, you proposed:

I have no idea what is needed to fix the issue. Maybe the reason should be reported as an enum? #16490?
The states could be:
- opt-in-signal
- rbf-inheritance
- (full-rbf)
- (timeout-rbf)
- ...
The ones in braces are for not-(yet) Bitcoin Core policies.

I think of those only opt-in-signal makes sense to have: reporting that it signals by inheritance while we don't implement this rule sounds confusing to me.
However i understand the future-proofing argument raised in #16490 and we now have the opportunity to introduce it. Maybe we can at least introduce the enum with a single variant for now?

So, how about going first going for the minimal fix here and then proceed with something like #16490 to deprecate bip125-replaceable in favour of a new field more complete about the replaceability status of the transaction?

reporting that it signals by inheritance while we don't implement this rule sounds confusing to me.

The rule isn't implemented fully, but surely is implemented partially in Bitcoin Core. Bitcoin Core simply has the additional requirement that an rbf-signalling parent must be replaced in the same instance.

Not implementing the rule (or rather implementing the inverse of the rule) would mean: A non-signalling child tx could remove the signalling status from a parent, and thus prevent replacement. This is what a plain no for the non-signalling child would imply (this pr), which is my concern.

For the benefit of reviewers this PR was NACKed by @MarcoFalke and it appears that @glozow is now comfortable with the approach taken in #22698 which was earlier seen as a competing PR.

This PR is currently closed but I think it is more important we get the approach right than just blindly picking whichever PR is kept open by its author. However, it does seem that @MarcoFalke, @glozow @mjdietzx @ariard etc are comfortable with the approach taken in #22698.

Thanks for the work on this @darosior, sorry for the frustration you've experienced. I've certainly wavered on this and seems like others have been unsure too.