test: Extend test coverage of BIP125 and document confusing/inconsistent behavior by mjdietzx · Pull Request #22867 · bitcoin/bitcoin

mjdietzx · 2021-09-02T21:56:07Z

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

laanwj · 2021-09-09T12:22:56Z

Concept ACK, thanks for adding tests

fanquake · 2021-09-10T06:21:24Z

cc @ariard @darosior

DrahtBot · 2021-09-11T03:40:20Z

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.

mjdietzx · 2021-09-26T20:45:54Z

Rebased

pg156 · 2021-10-02T22:21:43Z

Concept ACK. Tested on macOS Big Sur.

test/functional/feature_rbf.py

mjdietzx · 2021-10-28T21:11:33Z

Addressed @pg156's comments/improvements and rebased.

mjdietzx · 2021-11-21T15:27:11Z

Rebased

mjdietzx · 2021-11-22T16:03:40Z

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

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\`.

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

mjdietzx · 2021-12-01T18:10:43Z

Rebased after #23437 and #22677 were merged

DariusParvin

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

DariusParvin · 2021-12-08T22:25:45Z

test/functional/feature_rbf.py

+                fee_rate=Decimal('0.0001'),
+            )
+            assert_equal(True, self.nodes[0].getmempoolentry(tx['txid'])['bip125-replaceable'])  # inherited
+        # Now we have a chain of: `optin_parent_tx`, `joined_tx`, and 99 txs. The last tx in the loop exceeded `MAX_REPLACEMENT_LIMIT`


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

Suggested change

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

# 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`

DariusParvin · 2021-12-08T22:33:13Z

test/functional/feature_rbf.py

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


nit: To check the specific error message

Suggested change

self.wallet.create_self_transfer(

replacement_parent_tx = self.wallet.create_self_transfer(

from_node=self.nodes[0],

utxo_to_spend=confirmed_utxos[0],

fee_rate=Decimal('0.01'),

mempool_valid=False

)['hex']

assert_raises_rpc_error(-26, 'too many potential replacements', self.nodes[0].sendrawtransaction, replacement_parent_tx, 0)

darosior · 2021-12-09T15:36:55Z

Concept ACK.

glozow

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.

glozow · 2021-12-03T11:15:48Z

test/functional/feature_rbf.py

        tx.vout[0].nValue -= 1
        assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx.serialize().hex())

+    def test_prechecks_overestimates_replacements(self):


commit 5e72716 "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.

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?

glozow · 2022-01-04T15:43:17Z

test/functional/feature_rbf.py

        self.wallet.get_utxo(txid=replacement_tx['txid'])

-    def test_reorged_inherited_signaling(self):
+    def test_reorged_inherited_signaling_and_descendant_limit(self):


commit 64c2040 "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.

aureleoules

tACK 64c2040 (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.

bitcoin/src/policy/rbf.cpp

Lines 64 to 69 in 64c2040

    
           if (nConflictingCount > MAX_BIP125_REPLACEMENT_CANDIDATES) { 
        
               return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", 
        
                                txid.ToString(), 
        
                                nConflictingCount, 
        
                                MAX_BIP125_REPLACEMENT_CANDIDATES); 
        
           }

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.

bitcoin/src/policy/rbf.h

Line 17 in 64c2040

static constexpr uint32_t MAX_BIP125_REPLACEMENT_CANDIDATES{100};

test_reorged_inherited_signaling_and_descendant_limit

Changing the following line

bitcoin/src/policy/rbf.cpp

Line 34 in 64c2040

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

to

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

kouloumos

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

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

kouloumos · 2022-06-13T19:32:44Z

test/functional/feature_rbf.py

+        joined_tx_txid = self.nodes[0].sendrawtransaction(joined_tx_hex)
+
+        # Get the `joined_tx` utxo into our wallet so we can spend a chain of descendants from it
+        joined_tx = self.nodes[0].decoderawtransaction(joined_tx_hex)
+        self.wallet.scan_tx(joined_tx)


Suggested change

joined_tx_txid = self.nodes[0].sendrawtransaction(joined_tx_hex)

# Get the `joined_tx` utxo into our wallet so we can spend a chain of descendants from it

joined_tx = self.nodes[0].decoderawtransaction(joined_tx_hex)

self.wallet.scan_tx(joined_tx)

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

kouloumos · 2022-06-13T19:46:58Z

test/functional/feature_rbf.py

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


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.

DrahtBot · 2022-07-08T10:58:25Z

🐙 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 · 2022-09-26T10:00:54Z

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.

DrahtBot added the Tests label Sep 2, 2021

mjdietzx mentioned this pull request Sep 3, 2021

RBF move 2/3: extract RBF logic into policy/rbf #22675

Merged

maflcko changed the title ~~Extend test coverage of BIP125 and document confusing/inconsistent behavior~~ test: Extend test coverage of BIP125 and document confusing/inconsistent behavior Sep 3, 2021

This was referenced Sep 11, 2021

Package Mempool Submission with Package Fee-Bumping #22290

Closed

test: use MiniWallet for make_utxo helper in feature_rbf.py #22998

Merged

test: Replace MiniWallet scan_blocks with rescan_utxos #23017

Merged

DrahtBot added the Needs rebase label Sep 21, 2021

mjdietzx force-pushed the test_bip125_edge_cases branch from 84c88ec to 85a69f4 Compare September 26, 2021 20:45

DrahtBot removed the Needs rebase label Sep 26, 2021

DrahtBot mentioned this pull request Sep 28, 2021

[policy] check ancestor feerate in RBF, remove BIP125 Rule2 #23121

Closed

pg156 reviewed Oct 5, 2021

View reviewed changes

test/functional/feature_rbf.py Outdated Show resolved Hide resolved

test/functional/feature_rbf.py Outdated Show resolved Hide resolved

test/functional/feature_rbf.py Outdated Show resolved Hide resolved

mjdietzx force-pushed the test_bip125_edge_cases branch from 85a69f4 to e6c88a3 Compare October 28, 2021 21:08

mjdietzx force-pushed the test_bip125_edge_cases branch from e6c88a3 to a9a9d3f Compare October 29, 2021 01:40

DrahtBot mentioned this pull request Oct 29, 2021

tests: Add assert_less_than and assert_less_than_or_equal to test framework #23337

Closed

mjdietzx mentioned this pull request Nov 20, 2021

Implement RBF inherited signaling and fix getmempoolentry returned bip125-replaceable status #22698

Closed

mjdietzx force-pushed the test_bip125_edge_cases branch from a9a9d3f to 12509a0 Compare November 21, 2021 15:27

mjdietzx force-pushed the test_bip125_edge_cases branch from 12509a0 to 3fc1f8b Compare November 21, 2021 22:41

mjdietzx added 4 commits December 1, 2021 12:08

test: clarify no inherited replaceability assertions

f01954d

test: replacement tx rejected bc conflicts may be double counted

5e72716

test: incorrect rbf status when max replacement limit exceeded

64c2040

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

mjdietzx force-pushed the test_bip125_edge_cases branch from 3fc1f8b to 64c2040 Compare December 1, 2021 18:08

DariusParvin reviewed Dec 8, 2021

View reviewed changes

maflcko requested a review from glozow December 10, 2021 09:18

glozow reviewed Jan 4, 2022

View reviewed changes

aureleoules approved these changes Mar 12, 2022

View reviewed changes

kouloumos reviewed Jun 13, 2022

View reviewed changes

This was referenced Jun 15, 2022

Add a -mempoolfullrbf node setting #25353

Merged

doc requirement that replacement must have higher feerate than direct conflicts #25382

Merged

DrahtBot mentioned this pull request Jun 24, 2022

test: test RBF rule 3 #25461

Closed

DrahtBot added the Needs rebase label Jul 8, 2022

glozow closed this Sep 26, 2022

bitcoin locked and limited conversation to collaborators Sep 26, 2023

	# Now we have a chain of: `optin_parent_tx`, `joined_tx`, and 99 txs. The last tx in the loop exceeded `MAX_REPLACEMENT_LIMIT`
	# 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`

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

	if (nConflictingCount > MAX_BIP125_REPLACEMENT_CANDIDATES) {
	return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n",
	txid.ToString(),
	nConflictingCount,
	MAX_BIP125_REPLACEMENT_CANDIDATES);
	}

Conversation

mjdietzx commented Sep 2, 2021

Uh oh!

laanwj commented Sep 9, 2021

Uh oh!

fanquake commented Sep 10, 2021

Uh oh!

DrahtBot commented Sep 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

mjdietzx commented Sep 26, 2021

Uh oh!

pg156 commented Oct 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mjdietzx commented Oct 28, 2021

Uh oh!

mjdietzx commented Nov 21, 2021

Uh oh!

mjdietzx commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjdietzx commented Dec 1, 2021

Uh oh!

DariusParvin left a comment

Choose a reason for hiding this comment

Uh oh!

DariusParvin Dec 8, 2021

Choose a reason for hiding this comment

Uh oh!

DariusParvin Dec 8, 2021

Choose a reason for hiding this comment

Uh oh!

darosior commented Dec 9, 2021

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

glozow Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

kouloumos Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

glozow Jan 4, 2022

Choose a reason for hiding this comment

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

test_prechecks_overestimates_replacements

test_reorged_inherited_signaling_and_descendant_limit

Uh oh!

kouloumos left a comment

Choose a reason for hiding this comment

Uh oh!

kouloumos Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

kouloumos Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 8, 2022

Uh oh!

glozow commented Sep 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

DrahtBot commented Sep 11, 2021 •

edited

Loading

mjdietzx commented Nov 22, 2021 •

edited

Loading