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

jamesob commented at 5:51 PM on May 27, 2022: member

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

test: allow passing sequence through create_self_transfer_multi

And some little type annotation additions.

6120e8e287

fanquake added the label Tests on May 27, 2022

jamesob force-pushed on May 27, 2022

fanquake requested review from glozow on May 28, 2022

in test/functional/feature_rbf.py:47 in 9b825e7eab outdated

  41 | @@ -42,6 +42,10 @@ def set_test_params(self):
  42 |                  "-limitdescendantcount=200",
  43 |                  "-limitdescendantsize=101",
  44 |              ],
  45 | +            # second node has default mempool parameters
  46 | +            [
  47 | +                "-acceptnonstdtxn=1",

glozow commented at 8:20 PM on May 30, 2022:

Not sure if this is me being pedantic, but I don't think we're really testing "default mempool parameters" if we're configuring the node to acceptnonstdtxn. Is there a good reason we need to set this?

ajtowns commented at 5:29 AM on May 31, 2022:

I think the only reason this needs to accept nonstd txs is because there's some left over txs in node 0's mempool from earlier tests, which results in syncing mempools failing when you try to mine a block with node 1. Doing:

+        #clear the mempools
+        self.generate(self.nodes[0], 1)
+
         normal_node = self.nodes[1]
         wallet = MiniWallet(normal_node)
         wallet.rescan_utxos()

Looks to me like it lets you drop the -acceptnonstdtxn=1.

jamesob commented at 1:10 PM on May 31, 2022:

Good call @glozow @ajtowns, willfix.

glozow commented at 8:22 PM on May 30, 2022: member

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

laanwj commented at 10:36 AM on May 31, 2022: member

Concept ACK

brunoerg commented at 12:53 PM on May 31, 2022: member

Concept ACK

jamesob force-pushed on May 31, 2022

dunxen commented at 7:06 AM on June 1, 2022: contributor

Concept ACK

jnewbery commented at 5:20 PM on June 1, 2022: member

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

in test/functional/feature_rbf.py:477 in fd564f3460 outdated

 472 | +
 473 | +            # Now attempt to submit a tx that double-spends all the root tx inputs, which
 474 | +            # would invalidate `num_txs_invalidated` transactions.
 475 | +            double_tx = wallet.create_self_transfer_multi(
 476 | +                from_node=normal_node,
 477 | +                sequence=BIP125_SEQUENCE_NUMBER,

danielabrozzoni commented at 6:02 PM on June 1, 2022:

small nit: you can avoid setting the sequence number here (the default works as well, double_tx doesn't need to be replaced), but if you want to leave it like that it's fine :)

jamesob commented at 2:28 PM on June 2, 2022:

Fixed, thanks.

danielabrozzoni commented at 6:07 PM on June 1, 2022: contributor

Concept ACK

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

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.

687addaf13

jamesob force-pushed on Jun 2, 2022

jamesob commented at 2:30 PM on June 2, 2022: member

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.

in test/functional/test_framework/wallet.py:151 in 687addaf13

 147 | @@ -147,7 +148,7 @@ def get_descriptor(self):
 148 |      def get_address(self):
 149 |          return self._address
 150 |  
 151 | -    def get_utxo(self, *, txid: str = '', vout: Optional[int] = None, mark_as_spent=True):
 152 | +    def get_utxo(self, *, txid: str = '', vout: Optional[int] = None, mark_as_spent=True) -> dict:

LarryRuane commented at 10:27 PM on June 2, 2022:

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.

in test/functional/test_framework/wallet.py:214 in 687addaf13

 208 | @@ -208,14 +209,21 @@ def send_self_transfer_multi(self, **kwargs):
 209 |          return {'new_utxos': [self.get_utxo(txid=txid, vout=vout) for vout in range(len(tx.vout))],
 210 |                  'txid': txid, 'hex': tx.serialize().hex(), 'tx': tx}
 211 |  
 212 | -    def create_self_transfer_multi(self, *, from_node, utxos_to_spend=None, num_outputs=1, fee_per_output=1000):
 213 | +    def create_self_transfer_multi(
 214 | +            self, *, from_node,
 215 | +            utxos_to_spend: Optional[List[dict]] = None,

LarryRuane commented at 10:29 PM on June 2, 2022:

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

in test/functional/feature_rbf.py:446 in 687addaf13

 441 | +            root_utxos = []
 442 | +
 443 | +            # For each root UTXO, create a package that contains the spend of that
 444 | +            # UTXO and `txs_per_graph` children tx.
 445 | +            for graph_num in range(num_tx_graphs):
 446 | +                root_utxos.append(wallet.get_utxo())

LarryRuane commented at 10:30 PM on June 2, 2022:

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

so that code below is simpler

in test/functional/feature_rbf.py:451 in 687addaf13

 446 | +                root_utxos.append(wallet.get_utxo())
 447 | +
 448 | +                optin_parent_tx = wallet.send_self_transfer_multi(
 449 | +                    from_node=normal_node,
 450 | +                    sequence=BIP125_SEQUENCE_NUMBER,
 451 | +                    utxos_to_spend=[root_utxos[graph_num]],

LarryRuane commented at 10:35 PM on June 2, 2022:

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

in test/functional/feature_rbf.py:466 in 687addaf13

 461 | +                        utxo_to_spend=utxo,
 462 | +                    )
 463 | +
 464 | +                    assert normal_node.getmempoolentry(child_tx['txid'])
 465 | +
 466 | +            num_txs_invalidated = len(root_utxos) + (num_tx_graphs * txs_per_graph)

LarryRuane commented at 10:36 PM on June 2, 2022:

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

nit variable rename, they're not invalided yet

in test/functional/feature_rbf.py:472 in 687addaf13

 467 | +
 468 | +            if failure_expected:
 469 | +                assert num_txs_invalidated > MAX_REPLACEMENT_LIMIT
 470 | +            else:
 471 | +                assert num_txs_invalidated <= MAX_REPLACEMENT_LIMIT
 472 | +

LarryRuane commented at 10:38 PM on June 2, 2022:

maybe add here (but requires a change below):

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

in test/functional/feature_rbf.py:487 in 687addaf13

 482 | +            if failure_expected:
 483 | +                assert_raises_rpc_error(
 484 | +                    -26, "too many potential replacements", normal_node.sendrawtransaction, tx_hex, 0)
 485 | +            else:
 486 | +                txid = normal_node.sendrawtransaction(tx_hex, 0)
 487 | +                assert normal_node.getmempoolentry(txid)

LarryRuane commented at 10:41 PM on June 2, 2022:

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

in test/functional/feature_rbf.py:492 in 687addaf13

 487 | +                assert normal_node.getmempoolentry(txid)
 488 | +
 489 | +        # Clear the mempool once finished, and rescan the other nodes' wallet
 490 | +        # to account for the spends we've made on `normal_node`.
 491 | +        self.generate(normal_node, 1)
 492 | +        self.wallet.rescan_utxos()

LarryRuane commented at 10:44 PM on June 2, 2022:

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.

LarryRuane commented at 10:48 PM on June 2, 2022: contributor

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

jamesob commented at 5:14 PM on June 3, 2022: member

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 commented at 6:49 PM on June 7, 2022: member

Code review ACK 687addaf136356e0f3698d6345c92d875e0a3362

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

laanwj merged this on Jun 7, 2022

laanwj closed this on Jun 7, 2022

sidhujag referenced this in commit 5b0c5a0120 on Jun 8, 2022

in test/functional/test_framework/wallet.py:216 in 687addaf13

 208 | @@ -208,14 +209,21 @@ def send_self_transfer_multi(self, **kwargs):
 209 |          return {'new_utxos': [self.get_utxo(txid=txid, vout=vout) for vout in range(len(tx.vout))],
 210 |                  'txid': txid, 'hex': tx.serialize().hex(), 'tx': tx}
 211 |  
 212 | -    def create_self_transfer_multi(self, *, from_node, utxos_to_spend=None, num_outputs=1, fee_per_output=1000):
 213 | +    def create_self_transfer_multi(
 214 | +            self, *, from_node,
 215 | +            utxos_to_spend: Optional[List[dict]] = None,
 216 | +            num_outputs=1,
 217 | +            sequence=0,

MarcoFalke commented at 6:32 AM on July 1, 2022:

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

(Might fix in a follow-up)

DrahtBot locked this on Jul 1, 2023