test: Extend test coverage of BIP125 and document confusing/inconsistent behavior

mjdietzx commented at 9:56 pm on September 2, 2021: contributor

Intention of this PR is to extend test coverage of BIP125. Mostly around Rule 5. Focus is on highlighting confusing/inconsistent behavior that I think we need to consider as we move forward with multiple PRs (and different approaches to BIP125 in general) that are under review.

This PR also splits tests out of #22698 (which I’ve made a draft for now until some related PRs are merged) as they are generally useful and highlight some behavior I think is worth considering.

All that said, I do not see any reason we can’t eliminate all these inconsistencies while keeping inherited signaling, which is what #22698 does. But either way, making it clear in tests at least motivates a longer-term fix

DrahtBot added the label Tests on Sep 2, 2021

MarcoFalke renamed this:
~~Extend test coverage of BIP125 and document confusing/inconsistent behavior~~
test: Extend test coverage of BIP125 and document confusing/inconsistent behavior
on Sep 3, 2021

laanwj commented at 12:22 pm on September 9, 2021: member

Concept ACK, thanks for adding tests

fanquake commented at 6:21 am on September 10, 2021: member

cc @ariard @darosior

DrahtBot commented at 3:40 am on September 11, 2021: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25353 (Add a -mempoolfullrbf node setting by ariard)

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.

DrahtBot added the label Needs rebase on Sep 21, 2021

mjdietzx force-pushed on Sep 26, 2021

mjdietzx commented at 8:45 pm on September 26, 2021: contributor

Rebased

DrahtBot removed the label Needs rebase on Sep 26, 2021

pg156 commented at 10:21 pm on October 2, 2021: none

Concept ACK. Tested on macOS Big Sur.

in test/functional/feature_rbf.py:646 in 85a69f40dd outdated

655@@ -638,5 +656,129 @@ def test_replacement_relay_fee(self):
656         tx.vout[0].nValue -= 1
657         assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx.serialize().hex())
658 
659+    def test_prechecks_overestimates_replacements(self):
660+        confirmed_utxos = [self.wallet.get_utxo(), self.wallet.get_utxo()]

pg156 commented at 3:53 pm on October 5, 2021:

This assumes the wallet has at least two utxos, and therefore requires another test creating the utxos to run beforehand.

mjdietzx commented at 9:10 pm on October 28, 2021:

This seems to be a somewhat common pattern, I see other tests that rely on there being some utxos in the wallet. So I’m going to leave it for now. I do realize that these sub-tests are not totally stateless bc of this. If anyone thinks it is a big deal and should be fixed lmk and I’ll do it.

in test/functional/feature_rbf.py:764 in 85a69f40dd outdated

759+        # conflicting txns being evicted from the mempool. However, it (and all of its descendants) are still signaling replaceability.
760+        # We would've expected that once `MAX_REPLACEMENT_LIMIT` is exceeded, the opt-in parent txn stops signaling
761+        # replaceability, along with _all_ of its descendants.
762+        entry = self.nodes[0].getmempoolentry(parent_txs[0]["txid"])
763+        assert_greater_than(entry['descendantcount'], MAX_REPLACEMENT_LIMIT)
764+        assert_equal(True, self.nodes[0].getmempoolentry(parent_txs[0]["txid"])['bip125-replaceable'])

pg156 commented at 4:01 pm on October 5, 2021:

Minor suggestion, otherwise it is possibly unclear if 0 is opt-in and 1 is opt-out, or vice versa.

0        assert_equal(True, self.nodes[0].getmempoolentry(optin_parent_tx["txid"])['bip125-replaceable'])

mjdietzx commented at 8:49 pm on October 5, 2021:

Good suggestion, I’ll also followup w/ this

mjdietzx commented at 9:10 pm on October 28, 2021:

Done in e6c88a3

in test/functional/feature_rbf.py:122 in 85a69f40dd outdated

132@@ -127,6 +133,20 @@ def make_utxo(self, node, amount, confirmed=True, scriptPubKey=DUMMY_P2WPKH_SCRI
133 
134         return COutPoint(int(txid, 16), 1)
135 
136+    def create_multiple_input_self_transfer(self, input_utxos, fee_rate, sequence=BIP125_SEQUENCE_NUMBER):
137+        """Given two input utxos, create one transaction that spends both of them"""
138+        [tx, staging_tx] = list(map(lambda utxo: self.wallet.create_self_transfer(

pg156 commented at 4:09 pm on October 5, 2021:

As input_utxos is a pair instead of arbitrary length, does a function name such as create_double_input_self_transfer or create_pair_input_self_transfer indicate the intention better?

mjdietzx commented at 8:48 pm on October 5, 2021:

I think this is a good suggestion. I’ll make this change as suggested

mjdietzx commented at 9:09 pm on October 28, 2021:

Done in 6804fc5

mjdietzx force-pushed on Oct 28, 2021

mjdietzx commented at 9:11 pm on October 28, 2021: contributor

Addressed @pg156’s comments/improvements and rebased.

mjdietzx force-pushed on Oct 29, 2021

mjdietzx force-pushed on Nov 21, 2021

mjdietzx commented at 3:27 pm on November 21, 2021: contributor

Rebased

mjdietzx force-pushed on Nov 21, 2021

mjdietzx commented at 4:03 pm on November 22, 2021: contributor

I better organized the commits and improved the commit messages. Very minor / no behavior change

test: clarify no inherited replaceability assertions f01954d74a

test: replacement tx rejected bc conflicts may be double counted 5e72716a45

test: dynamic rbf inherited signaling in the case of a reorg

In the event of a reorg, the assumption that a newly added tx has no
in-mempool children is false. If the children opted-out of rbf they
would show \`RBFTransactionState::FINAL\` prior to the reorg.
Upon their rbf opt-in parent being accepted back into the mempool, they
would show \`RBFTransactionState::REPLACEABLE_BIP125\`.

25f1e8aed1

test: incorrect rbf status when max replacement limit exceeded

Highlight that transactions may signal they are replaceable even
though they are not due to BIP125 RBF (Rule #5).

64c20402b2

mjdietzx force-pushed on Dec 1, 2021

mjdietzx commented at 6:10 pm on December 1, 2021: contributor

Rebased after #23437 and #22677 were merged

in test/functional/feature_rbf.py:640 in 5e72716a45 outdated

636@@ -621,5 +637,64 @@ def test_replacement_relay_fee(self):
637         tx.vout[0].nValue -= 1
638         assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx.serialize().hex())
639 
640+    def test_prechecks_overestimates_replacements(self):

glozow commented at 11:15 am on December 3, 2021:

commit 5e72716a457e994c263dc1c226a4a1861cab6bbf “test: replacement tx rejected bc conflicts may be double counted.” can be dropped

I assume the purpose of this test is to illustrate the limitation/overestimation in GetEntriesForConflicts(). It is now documented here. If we want to test this, as it’s a low-level implementation detail, a unit test would be more appropriate than a functional test.

kouloumos commented at 8:05 pm on June 13, 2022:

What’s documented here is not such an important issue as the CVE in test_no_inherited_signaling but documenting an issue/limitation seems to have a precedent. Maybe this could make sense as an additional reference for the docs?

in test/functional/feature_rbf.py:751 in 64c20402b2

746+                utxo_to_spend=joined_utxo if tx is None else self.wallet.get_utxo(txid=tx['txid']),  # a straight line of descendants
747+                sequence=0xffffffff,
748+                fee_rate=Decimal('0.0001'),
749+            )
750+            assert_equal(True, self.nodes[0].getmempoolentry(tx['txid'])['bip125-replaceable'])  # inherited
751+        # Now we have a chain of: `optin_parent_tx`, `joined_tx`, and 99 txs. The last tx in the loop exceeded `MAX_REPLACEMENT_LIMIT`

DariusParvin commented at 10:25 pm on December 8, 2021:

nit: My understanding is that it’s better not to have actual values in comments

0        # Now we have a chain of: `optin_parent_tx`, `joined_tx`, and `MAX_REPLACEMENT_LIMIT - 1` txs. The last tx in the loop exceeded `MAX_REPLACEMENT_LIMIT`

in test/functional/feature_rbf.py:763 in 64c20402b2

758+        assert_greater_than(entry['descendantcount'], MAX_REPLACEMENT_LIMIT)
759+        assert_equal(True, entry['bip125-replaceable'])
760+        assert_equal(True, self.nodes[0].getmempoolentry(tx["txid"])['bip125-replaceable'])
761+
762+        # Case in point, we can't actually replace `optin_parent_tx` once it has `MAX_REPLACEMENT_LIMIT` descendants
763+        self.wallet.create_self_transfer(

DariusParvin commented at 10:33 pm on December 8, 2021:

nit: To check the specific error message

0        replacement_parent_tx = self.wallet.create_self_transfer(
1            from_node=self.nodes[0],
2            utxo_to_spend=confirmed_utxos[0],
3            fee_rate=Decimal('0.01'),
4            mempool_valid=False
5        )['hex']
6        assert_raises_rpc_error(-26, 'too many potential replacements', self.nodes[0].sendrawtransaction, replacement_parent_tx, 0)

DariusParvin commented at 10:39 pm on December 8, 2021: contributor

Tested ACK 64c2040 Great job, these tests really clarify the current behavior of rbf.

darosior commented at 3:36 pm on December 9, 2021: member

Concept ACK.

MarcoFalke requested review from glozow on Dec 10, 2021

in test/functional/feature_rbf.py:708 in 64c20402b2

704@@ -705,7 +705,7 @@ def test_prechecks_overestimates_replacements(self):
705         self.wallet.get_utxo(txid=tx['txid'])
706         self.wallet.get_utxo(txid=replacement_tx['txid'])
707 
708-    def test_reorged_inherited_signaling(self):
709+    def test_reorged_inherited_signaling_and_descendant_limit(self):

glozow commented at 3:43 pm on January 4, 2022:

commit 64c20402b258709af8c18eac1697939eca07082f “test: incorrect rbf status when max replacement limit exceeded”

I think this one is also a misunderstanding and can be dropped/replaced with a clarification in the RPC helpstring. The bip125-replaceable field is only concerned with signaling is not an authority on all RBF rules, so Rule 5 shouldn’t affect its result. If that isn’t clear to users, we should more clearly document what information it provides.

glozow commented at 4:05 pm on January 4, 2022: member

Thank you for making an effort to clarify the RBF interface, but I believe that clear documentation is more appropriate than a functional test for 2/4 of these.

aureleoules approved

aureleoules commented at 1:50 pm on March 12, 2022: member

tACK 64c20402b258709af8c18eac1697939eca07082f (make check and test/functional/test_runner.py). Tested on NixOS 22.05 64 bits.

test_prechecks_overestimates_replacements

I commented the following lines locally and the test failed accordingly.
https://github.com/bitcoin/bitcoin/blob/64c20402b258709af8c18eac1697939eca07082f/src/policy/rbf.cpp#L64-L69

Increasing the following constant to 104 or more makes the test fail as expected because there would be enough room in the mempool to fit the chain of 104 transactions : parent_txs (2) + joined_tx (2) + tx chain (100 / 2 but UTXOs counted twice so 100) = 104.
https://github.com/bitcoin/bitcoin/blob/64c20402b258709af8c18eac1697939eca07082f/src/policy/rbf.h#L17

test_reorged_inherited_signaling_and_descendant_limit

Changing the following line https://github.com/bitcoin/bitcoin/blob/64c20402b258709af8c18eac1697939eca07082f/src/policy/rbf.cpp#L34 to

0pool.CalculateMemPoolAncestors(entry, ancestors, MAX_BIP125_REPLACEMENT_CANDIDATES, noLimit, noLimit, noLimit, dummy, false);

results in the test failing as expected. The last transaction of the transaction chain of MAX_REPLACEMENT_LIMIT size would stop signaling RBF with inheritance because the child transaction signaling RBF would be buried too deep in the chain to be detected: which is to my understanding not the current behavior of Bitcoin.

in test/functional/feature_rbf.py:663 in 64c20402b2

658+        joined_tx_hex = self.create_double_input_self_transfer(parent_utxos, Decimal('0.0001'))
659+        joined_tx_txid = self.nodes[0].sendrawtransaction(joined_tx_hex)
660+
661+        # Get the `joined_tx` utxo into our wallet so we can spend a chain of descendants from it
662+        joined_tx = self.nodes[0].decoderawtransaction(joined_tx_hex)
663+        self.wallet.scan_tx(joined_tx)

kouloumos commented at 7:32 pm on June 13, 2022:

0      joined_tx_txid = self.wallet.sendrawtransaction(from_node=self.nodes[0], tx_hex=joined_tx_hex)

in test/functional/feature_rbf.py:679 in 64c20402b2

674+
675+        # Even though there are well under `MAX_REPLACEMENT_LIMIT` transactions that will be evicted due to this replacement,
676+        # in this case we still reject the replacement attempt because of the way `MemPoolAccept::PreChecks` double-counts descendants.
677+        # Each `confirmed_utxo` has the exact same descendants, but they are each counted twice!
678+        replacement_attempt_tx_hex = self.create_double_input_self_transfer(confirmed_utxos, Decimal('0.01'))
679+        assert_raises_rpc_error(-26, 'too many potential replacements', self.nodes[0].sendrawtransaction, replacement_attempt_tx_hex, 0)

kouloumos commented at 7:46 pm on June 13, 2022:

You are doing good job documenting this limitation, you could maybe add one or two getmempoolinfo()["size"]) assertions at appropriate places to help even more with the overall explanation.

kouloumos commented at 8:05 pm on June 13, 2022: contributor

ACK 5e72716a457e994c263dc1c226a4a1861cab6bbf, not supportive of the last 2 commits.

I believe that 25f1e8aed1dea72ef2316ea5e39ca12d09c64921 has a lot of similarities with test_opt_in, it could be used to enchance that one.

DrahtBot added the label Needs rebase on Jul 8, 2022

DrahtBot commented at 10:58 am on July 8, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

glozow commented at 10:00 am on September 26, 2022: member

Closing as I believe this is resolved with doc/policy/mempool_replacements.md (which documents the limitations) and #25674, and this has needed rebase for a while.

glozow closed this on Sep 26, 2022

bitcoin locked this on Sep 26, 2023

test: Extend test coverage of BIP125 and document confusing/inconsistent behavior #22867

Conflicts

test_prechecks_overestimates_replacements

test_reorged_inherited_signaling_and_descendant_limit