test: add BIP-125 rule 5 testcase with default mempool #25228

jamesob · 2022-05-27T17:51:20Z

Currently, we only test rule 5 of BIP-125 (replacement transactions cannot evict more than 100 transactions) by changing default mempool parameters to allow for more descendants. The current test works on a single transaction graph that has over 100 descendants.

This patch adds a test to exercise rule 5 using the default mempool parameters. The case is a little more sophisticated: instead of working on a single transaction graph, it uses a replacement transaction to "unite" several UTXOs which join independent transaction graphs. The total number of transactions in these graphs sum to more than the max allowable replacement.

I think the difference in transaction topology makes this a worthwhile testcase to have, setting aside the fact that this testcase works without having to use atypical mempool params.

See also: relevant discussion from IRC

And some little type annotation additions.

glozow

strong Concept ACK as we don't currently have test coverage for Rule 5 with default mempool limits

test/functional/feature_rbf.py

laanwj · 2022-05-31T10:36:18Z

Concept ACK

brunoerg

Concept ACK

dunxen · 2022-06-01T07:06:42Z

Concept ACK

jnewbery · 2022-06-01T17:20:11Z

Concept ACK. What do you think about also removing the test that uses the non-standard mempool config?

danielabrozzoni

Concept ACK

test/functional/feature_rbf.py

This testcase exercises rule 5 of BIP-125 (no more than 100 evictions due to replacement) without having to test under non-default mempool parametmers.

jamesob · 2022-06-02T14:30:35Z

Concept ACK. What do you think about also removing the test that uses the non-standard mempool config?

Personally I don't think it hurts to have the coverage - the test seems to run pretty fast - but I did think it'd be interesting to add another test case with default params that uses a different (simpler) topology, which could just be uniting >125 individual txns.

Edit: but IMO might as well just merge this and get the coverage, then think about adding more cases later.

LarryRuane

ACK 687adda
I really like this new test, comments are just suggestions

LarryRuane · 2022-06-02T22:27:53Z

test/functional/test_framework/wallet.py

        return self._address

-    def get_utxo(self, *, txid: str = '', vout: Optional[int] = None, mark_as_spent=True):
+    def get_utxo(self, *, txid: str = '', vout: Optional[int] = None, mark_as_spent=True) -> dict:


This PR doesn't seem to require this change, does it? If not, could you mention that in the commit message? That would help reviewers.

LarryRuane · 2022-06-02T22:29:01Z

test/functional/test_framework/wallet.py

-    def create_self_transfer_multi(self, *, from_node, utxos_to_spend=None, num_outputs=1, fee_per_output=1000):
+    def create_self_transfer_multi(
+            self, *, from_node,
+            utxos_to_spend: Optional[List[dict]] = None,


Same comment, this doesn't seem required; could you mention that in the commit comment?

LarryRuane · 2022-06-02T22:30:44Z

test/functional/feature_rbf.py

+            # For each root UTXO, create a package that contains the spend of that
+            # UTXO and `txs_per_graph` children tx.
+            for graph_num in range(num_tx_graphs):
+                root_utxos.append(wallet.get_utxo())


Suggested change

root_utxos.append(wallet.get_utxo())

utxo_parent_spend = wallet.get_utxo()

root_utxos.append(utxo_parent_spend)

so that code below is simpler

LarryRuane · 2022-06-02T22:35:02Z

test/functional/feature_rbf.py

+                optin_parent_tx = wallet.send_self_transfer_multi(
+                    from_node=normal_node,
+                    sequence=BIP125_SEQUENCE_NUMBER,
+                    utxos_to_spend=[root_utxos[graph_num]],


Suggested change

utxos_to_spend=[root_utxos[graph_num]],

utxos_to_spend=[utxo_parent_spend],

nit, slight readability improvement; I had to stare at the original for a while, the nested braces made me think at first it was some kind of double-nested index lookup! (Maybe it's just me.)

LarryRuane · 2022-06-02T22:36:36Z

test/functional/feature_rbf.py

+
+                    assert normal_node.getmempoolentry(child_tx['txid'])
+
+            num_txs_invalidated = len(root_utxos) + (num_tx_graphs * txs_per_graph)


Suggested change

num_txs_invalidated = len(root_utxos) + (num_tx_graphs * txs_per_graph)

num_txs_to_invalidate = len(root_utxos) + (num_tx_graphs * txs_per_graph)

nit variable rename, they're not invalided yet

LarryRuane · 2022-06-02T22:38:27Z

test/functional/feature_rbf.py

+                assert num_txs_invalidated > MAX_REPLACEMENT_LIMIT
+            else:
+                assert num_txs_invalidated <= MAX_REPLACEMENT_LIMIT
+


maybe add here (but requires a change below):

assert_equal(num_txs_to_invalidate, normal_node.getmempoolinfo()['size'])

LarryRuane · 2022-06-02T22:41:58Z

test/functional/feature_rbf.py

+                txid = normal_node.sendrawtransaction(tx_hex, 0)
+                assert normal_node.getmempoolentry(txid)


Suggested change

txid = normal_node.sendrawtransaction(tx_hex, 0)

assert normal_node.getmempoolentry(txid)

# The replacement tx should have been accepted into the mempool and it

# should be the only tx in the mempool (having replaced all others).

txid = normal_node.sendrawtransaction(tx_hex, 0)

assert normal_node.getmempoolentry(txid)

assert_equal(normal_node.getmempoolinfo()['size'], 1)

nit, just to clarify what is going on (would have helped me review), however the new assertion requires a change below.

LarryRuane · 2022-06-02T22:44:01Z

test/functional/feature_rbf.py

+        # Clear the mempool once finished, and rescan the other nodes' wallet
+        # to account for the spends we've made on `normal_node`.
+        self.generate(normal_node, 1)
+        self.wallet.rescan_utxos()


Indent these lines so the mempool gets cleared out at the end of each test case. Needed only if the assertions I recommended above are taken.

jamesob · 2022-06-03T17:14:19Z

I am done making relatively minor changes to this PR. If there are problems with the correctness of the test I will fix them, but otherwise I have other things to do.

laanwj · 2022-06-07T18:49:06Z

Code review ACK 687adda

I am done making relatively minor changes to this PR

Fair enough, opening a PR doesn't oblige you to incorporate every minor suggestion, i think it's fine like this

maflcko · 2022-07-01T06:32:17Z

test/functional/test_framework/wallet.py

+            self, *, from_node,
+            utxos_to_spend: Optional[List[dict]] = None,
+            num_outputs=1,
+            sequence=0,


Shouldn't this be an array of the same length as utxos_to_spend?

(Might fix in a follow-up)

test: allow passing sequence through create_self_transfer_multi

6120e8e

And some little type annotation additions.

fanquake added the Tests label May 27, 2022

jamesob force-pushed the 2022-05-bip125-rule5-test branch from 406e8de to 9b825e7 Compare May 27, 2022 18:00

fanquake requested a review from glozow May 28, 2022 12:16

glozow reviewed May 30, 2022

View reviewed changes

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

brunoerg reviewed May 31, 2022

View reviewed changes

jamesob force-pushed the 2022-05-bip125-rule5-test branch from 9b825e7 to fd564f3 Compare May 31, 2022 13:16

danielabrozzoni reviewed Jun 1, 2022

View reviewed changes

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

test: add BIP-125 rule 5 testcase with default mempool

687adda

This testcase exercises rule 5 of BIP-125 (no more than 100 evictions due to replacement) without having to test under non-default mempool parametmers.

jamesob force-pushed the 2022-05-bip125-rule5-test branch from fd564f3 to 687adda Compare June 2, 2022 14:19

LarryRuane reviewed Jun 2, 2022

View reviewed changes

laanwj merged commit e282764 into bitcoin:master Jun 7, 2022

maflcko reviewed Jul 1, 2022

View reviewed changes

bitcoin locked and limited conversation to collaborators Jul 1, 2023

	root_utxos.append(wallet.get_utxo())
	utxo_parent_spend = wallet.get_utxo()
	root_utxos.append(utxo_parent_spend)

	utxos_to_spend=[root_utxos[graph_num]],
	utxos_to_spend=[utxo_parent_spend],


		assert normal_node.getmempoolentry(child_tx['txid'])

		num_txs_invalidated = len(root_utxos) + (num_tx_graphs * txs_per_graph)

	num_txs_invalidated = len(root_utxos) + (num_tx_graphs * txs_per_graph)
	num_txs_to_invalidate = len(root_utxos) + (num_tx_graphs * txs_per_graph)

		txid = normal_node.sendrawtransaction(tx_hex, 0)
		assert normal_node.getmempoolentry(txid)

test: add BIP-125 rule 5 testcase with default mempool #25228

test: add BIP-125 rule 5 testcase with default mempool #25228

Uh oh!

Conversation

jamesob commented May 27, 2022

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laanwj commented May 31, 2022

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

dunxen commented Jun 1, 2022

Uh oh!

jnewbery commented Jun 1, 2022

Uh oh!

danielabrozzoni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesob commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob commented Jun 3, 2022

Uh oh!

laanwj commented Jun 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

jamesob commented Jun 2, 2022 •

edited

Loading