Document and test lack of inherited signaling in RBF policy

ariard commented at 6:15 pm on May 13, 2021: member

Contrary to BIP125 or other full-node implementation (e.g btcd), Bitcoin Core’s mempool policy doesn’t implement inherited signaling.

This PR documents our mempool behavior on this and add a test demonstrating the case.

ariard commented at 6:18 pm on May 13, 2021: member

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 ?

Sjors commented at 6:20 pm on May 13, 2021: member

utACK fb2047b

in test/functional/feature_rbf.py:626 in fb2047b241 outdated

621+        replacement_child_tx = self.nodes[0].signrawtransactionwithwallet(replacement_child_tx)
622+
623+        # Broadcast replacement child tx
624+        # BIP 125 :
625+        # 1. The original transactions signal replaceability explicitly or through inheritance as described in the above
626+        # Summary section.

Sjors commented at 6:23 pm on May 13, 2021:

This is a good place to refer to the CVE so it’s clear the behaviour here is not following BIP125.

ariard commented at 8:30 pm on May 13, 2021:

Yes already add a ref in new document, added another here if you think so.

Sjors commented at 9:22 am on May 14, 2021:

Probably a good idea; few people will read validation.cpp along side this test.

DrahtBot added the label Validation on May 13, 2021

ariard force-pushed on May 13, 2021

ghost commented at 1:07 am on May 14, 2021: none

Concept ACK

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

in src/validation.cpp:634 in 3d57b5eb05 outdated

633+                // *not* replaceable under this policy, even if one of their
634+                // unconfirmed ancestors signals replaceability. This is diverging
635+                // from BIP125's inherited signaling description (see CVE-2021-31876).
636+                // Applications relying on first-seen mempool behavior should be
637+                // checking all unconfirmed ancestors; doing otherwise an opt-in ancestor
638+                // might be replaced, causing removal of this descendant.

jonatack commented at 9:56 am on May 14, 2021:

suggestions, most important one being s/policy/logic|code/, the others are grammar

 0                 // Transactions that don't explicitly signal replaceability are
 1-                // *not* replaceable under this policy, even if one of their
 2-                // unconfirmed ancestors signals replaceability. This is diverging
 3+                // *not* replaceable with the current logic, even if one of their
 4+                // unconfirmed ancestors signals replaceability. This diverges
 5                 // from BIP125's inherited signaling description (see CVE-2021-31876).
 6-                // Applications relying on first-seen mempool behavior should be
 7-                // checking all unconfirmed ancestors; doing otherwise an opt-in ancestor
 8+                // Applications relying on first-seen mempool behavior should
 9+                // check all unconfirmed ancestors, otherwise an opt-in ancestor
10                 // might be replaced, causing removal of this descendant.

jonatack commented at 10:01 am on May 14, 2021: member

Concept ACK. Context/additional information:

in test/functional/feature_rbf.py:575 in 3d57b5eb05 outdated

570+        # Send tx from which to conflict outputs later
571+        base_txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), Decimal("10"))
572+        self.nodes[0].generate(1)
573+        self.sync_blocks()
574+
575+        # Create a explicitly opting parent transaction

jonatack commented at 10:09 am on May 14, 2021:

0        # Create an explicitly opt-in parent transaction

in test/functional/feature_rbf.py:602 in 3d57b5eb05 outdated

597+        res = self.nodes[0].testmempoolaccept(rawtxs=[replacement_parent_tx['hex']], maxfeerate=0)[0]
598+
599+        # Parent can be replaced.
600+        assert_equal(res['allowed'], True)
601+
602+        # Create a optout child tx spending opting parent

jonatack commented at 10:09 am on May 14, 2021:

0        # Create an opt-out child tx spending the opt-in parent

jonatack commented at 10:10 am on May 14, 2021: member

ACK 3d57b5eb053c7a34c3621875fa60401782a280b4

Happy to re-ACK if you update per the comments.

test: Extend feature_rbf.py with no inherited signaling 906b6d9da6

validation: document lack of inherited signaling in RBF policy 2eb0eeda39

ariard force-pushed on May 14, 2021

ariard commented at 6:36 pm on May 14, 2021: member

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 : https://github.com/bitcoin/bitcoin/blob/ecf5f2c1a06edd8372402872525f8de1d4277453/src/validation.cpp#L848 and absolute fee checks is done on original transactions and their descendants, should we document all of them ?

ariard commented at 6:38 pm on May 14, 2021: member

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

jonatack commented at 8:05 pm on May 14, 2021: member

ACK 2eb0eeda39cab997f9a5f817f7c12e7fffeaf35a

ghost commented at 11:10 am on May 17, 2021: none

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 :

https://github.com/bitcoin/bitcoin/blob/ecf5f2c1a06edd8372402872525f8de1d4277453/src/validation.cpp#L848

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

benthecarman commented at 3:44 am on May 19, 2021: contributor

ACK 2eb0eeda39cab997f9a5f817f7c12e7fffeaf35a

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

ariard commented at 3:09 am on May 21, 2021: member

@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.

jonatack commented at 1:01 pm on June 8, 2021: member

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

MarcoFalke merged this on Jun 8, 2021

MarcoFalke closed this on Jun 8, 2021

ajtowns commented at 3:52 am on June 9, 2021: member

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.

sidhujag referenced this in commit 40701a3327 on Jun 9, 2021

MarcoFalke commented at 11:03 am on June 10, 2021: member

@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

ariard commented at 4:14 pm on June 13, 2021: member

@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.

gwillen referenced this in commit b3fdb4bed1 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

Document and test lack of inherited signaling in RBF policy #21946