Also, I had a look on the rest of RBF functional test coverage (i.e feature_rbf.py, mempool_accept.py, p2p_invalid_tx.py). AFAICT, the latest check on incremental relay fee increase doesn't seem to have coverage ?

utACK fb2047b

Concept ACK

Maybe we can mention this in doc/bips.md as well

Thanks @jonatack and @Sjors, I think I've addressed all your comments at 2eb0eed, but lmk if you've more I'll address them quickly.

@prayank23

Well in fact there is another well-known supplemental check implemented by our logic not mentioned in the BIP, which is the feerate comparison here :

@MarcoFalke I believe you're aware of few mempool-related RPCs which should be updated in consequence as follow-up of this PR?

ACK 2eb0eed

Well in fact there is another well-known supplemental check implemented by our logic not mentioned in the BIP, which is the feerate comparison here :

bitcoin/src/validation.cpp

Line 848 in ecf5f2c

if (newFeeRate <= oldFeeRate)

and absolute fee checks is done on original transactions and their descendants, should we document all of them ?

Not sure about this. According to my understanding fee rate and fees both should be higher in replacement transaction.

Example:
Tx1 has only 1 input and two outputs (RBF enabled)
Tx2 is replacement transaction for Tx1 which has 5 inputs and 2 outputs.

It is possible that Tx2 uses a lower fee rate compared to Tx1 but more fees. Miners normally prioritize based on fee rate so a higher fee rate would be better.

However, if there is a difference in BIP and the implementation it should be mentioned in doc/bip.md IMO

ACK 2eb0eed

Is a fix for the actual CVE being worked on as well?

@prayank23

Yes, unless you have a strong opinion here, I prefer differences between BIPs and the implementation in another PR, like one adding test coverage for incremental relay fee.

@benthecarman

Not really, as a fix, I'm currently studying moving towards full-rbf. If we do so, there wouldn't be any kind of signaling to proceed at all.

IMO, main motivation to not fix it is avoiding increasing the DoS surface of the mempool, where at replacement transaction submission you might have to traverse N * package_ancestor_limits. With N your number of allowed inputs for a MAX_STANDARD_TX_WEIGHT-sized transaction.

This has ACKs by @Sjors, @benthecarman and myself, if anyone would like to have a look.

IMO, main motivation to not fix it is avoiding increasing the DoS surface of the mempool, where at replacement transaction submission you might have to traverse N * package_ancestor_limits. With N your number of allowed inputs for a MAX_STANDARD_TX_WEIGHT-sized transaction.

Adding a bool rbf_enabled to CTxMemPoolEntry that's true if the tx signals for rbf or any of its inputs have rbf_enabled == true would remove that problem fairly easily, as far as I can see.

@MarcoFalke I believe you're aware of few mempool-related RPCs which should be updated in consequence as follow-up of this PR?

Sure, filed as #22209

@ajtowns

IIUC we would have to re-order RBF opt-in checks after fetching parents coins from the cache in PreChecks, but I don't believe such re-order would change anything in DoS prevention strategy around mempool acceptance. Any CTxMemPoolEntry would inherit rbf_enable == true if one of its parent entries have the flag sets so, that way you only check first-depth of ancestors of the replaced transactions.

At first sight, I don't see any blocker for this approach, though I was planning to propose soon moving towards full-rbf. Beyond simplifying a bit PreChecks code sanity, opt-in RBF raises issues for multi-party funded transactions. If full-rbf turns as a flames war like last time, we can consider again this approach to fix the discrepancy between the spec and our code.