test: Use MiniWallet in mempool

ShubhamPalriwala commented at 7:35 am on July 25, 2021: contributor

This is a PR proposed in #20078

This PR enables running another non-wallet functional test even when the wallet is disabled thanks to the MiniWallet, i.e. it can be run even when bitcoin-core is compiled with –disable-wallet.

It also includes changes in wallet.py in the form of a new method, create_large_transactions() for the MiniWallet to create large transactions.

Efforts for this feature started in #20874 but were not continued and that PR was closed hence I picked this up.

To test this PR locally, compile and build bitcoin-core without the wallet and run:

0$ test/functional/mempool_limit.py

DrahtBot added the label Tests on Jul 25, 2021

ShubhamPalriwala force-pushed on Jul 25, 2021

amitiuttarwar commented at 7:07 pm on July 25, 2021: contributor

approach ACK :)

practicalswift commented at 8:05 pm on July 25, 2021: contributor

Concept ACK

DrahtBot commented at 10:28 am on July 28, 2021: member

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

Conflicts

No conflicts as of last run.

in test/functional/mempool_limit.py:39 in 8ac10d8e15 outdated

76+        node = self.nodes[0]
77+        miniwallet = MiniWallet(node)
78+        relayfee = node.getnetworkinfo()["relayfee"]
79+
80+        self.log.info("Check that mempoolminfee is minrelaytxfee")
81+        assert_equal(node.getmempoolinfo()["minrelaytxfee"], Decimal("0.00001000"))

MarcoFalke commented at 12:22 pm on July 28, 2021:

please don’t commit large style changes in the same commit as refactors/features. This makes review harder because it is not clear what is a refactor/style-change/feature

https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches

ShubhamPalriwala commented at 2:45 pm on August 3, 2021:

Thanks for the feedback, I’ve appended the commit with the code changes only (removed any “only style changes”) as of now, hope this makes reviewing the PR easier now.

Looking forward to your review

theStack commented at 8:18 pm on July 31, 2021: member

Concept ACK

ShubhamPalriwala force-pushed on Aug 3, 2021

Zero-1729 commented at 3:22 pm on August 3, 2021: contributor

Concept ACK

Shubhankar-Gambhir commented at 6:45 pm on August 5, 2021: contributor

Concept ACK

DrahtBot added the label Needs rebase on Aug 9, 2021

ShubhamPalriwala force-pushed on Aug 9, 2021

DrahtBot removed the label Needs rebase on Aug 9, 2021

in test/functional/mempool_limit.py:57 in dcd75d87ee outdated

68-            txids[i] = create_lots_of_big_transactions(self.nodes[0], txouts, utxos[30*i:30*i+30], 30, (i+1)*base_fee)
69-
70-        self.log.info('The tx should be evicted by now')
71-        assert txid not in self.nodes[0].getrawmempool()
72-        txdata = self.nodes[0].gettransaction(txid)
73-        assert txdata['confirmations'] ==  0  #confirmation should still be 0

NikhilBartwal commented at 8:50 pm on August 10, 2021:

Hey, any specific reason that we are not checking whether the confirmations of the original tx are still 0 or not. I couldn’t understand looking at the PR updates, so if you could explain :)

ShubhamPalriwala commented at 4:59 am on August 11, 2021:

Thread has been continued here

in test/functional/mempool_limit.py:64 in dcd75d87ee outdated

114+        base_fee = relayfee * 1000
115+
116+        self.log.info("Fill up the mempool with txs with higher fee rate")
117+        no_of_large_tx_created = 0
118+        for batch_of_txid in range(3):
119+            # Increment the tx fee rate gradually by a factor of (basee_fee) for each batch of 30 transactions

NikhilBartwal commented at 8:51 pm on August 10, 2021:

nit: Type-o in basee_fee -> base_fee :)

ShubhamPalriwala commented at 5:11 am on August 11, 2021:

Done!

in test/functional/mempool_limit.py:66 in dcd75d87ee outdated

116+        self.log.info("Fill up the mempool with txs with higher fee rate")
117+        no_of_large_tx_created = 0
118+        for batch_of_txid in range(3):
119+            # Increment the tx fee rate gradually by a factor of (basee_fee) for each batch of 30 transactions
120+            no_of_large_tx_created += miniwallet.create_large_transactions(
121+                node, txouts, 30, (batch_of_txid + 1) * base_fee

NikhilBartwal commented at 8:55 pm on August 10, 2021:

nit: Since, we are bumping up the fee_rate as a factor of base_fee with the batch_of_txid, IMO it would be more intuitive to use for batch_of_txid in range(1,4): so that we can simply use:

0miniwallet.create_large_transactions(
1                node, txouts, 30, batch_of_txid * base_fee
2)

ShubhamPalriwala commented at 5:11 am on August 11, 2021:

Done! Thanks

in test/functional/mempool_limit.py:70 in dcd75d87ee outdated

124+        self.log.info("The tx should be evicted by now")
125+        # The number of transactions created should be greater than the ones present in the mempool
126+        assert_greater_than(no_of_large_tx_created, len(node.getrawmempool()))
127+        # Initial tx created should not be present in the mempool anymore as it had a lower fee rate
128+        assert tx_to_be_evicted_id not in node.getrawmempool()
129+

NikhilBartwal commented at 8:57 pm on August 10, 2021:

In extension to the corresponding comment above, maybe we should add the confirmation test, something on the lines of:

0txdata = node.gettransaction(tx_to_be_evicted)
1assert txdata['confirmations'] == 0

ShubhamPalriwala commented at 4:58 am on August 11, 2021:

With respect to this and the above comment, gettransaction(tx_to_be_evicted) is a method that requires a wallet to be loaded, contradictory to the purpose of this PR. Hence, we’re just verifying if it’s evicted from the mempool!

NikhilBartwal approved

NikhilBartwal commented at 9:05 pm on August 10, 2021: contributor

Concept and Approach ACK and Tested dcd75d8

Checked that:

Running test/functional/mempool_limit.py after compiling bitcoin core from source with --disable-wallet leads to Test Skips: Test Skipped: wallet has not been compiled.
Running the same test with the same flag with the PR paches now successfully runs the tests: Tests successful

Just had a few comments to add :)

in test/functional/mempool_limit.py:75 in dcd75d87ee outdated

125+        # The number of transactions created should be greater than the ones present in the mempool
126+        assert_greater_than(no_of_large_tx_created, len(node.getrawmempool()))
127+        # Initial tx created should not be present in the mempool anymore as it had a lower fee rate
128+        assert tx_to_be_evicted_id not in node.getrawmempool()
129+
130+        self.log.info("Check that mempoolminfee is larger than minrelytxfee")

NikhilBartwal commented at 9:07 pm on August 10, 2021:

nit: Type-o in minrelytxfee -> minrelaytxfee :)

ShubhamPalriwala commented at 5:11 am on August 11, 2021:

Done

ShubhamPalriwala force-pushed on Aug 11, 2021

in test/functional/mempool_limit.py:51 in bec7849f5d outdated

100+        # 1 to create a tx initially that will be evicted from the mempool later
101+        # 90 with a fee rate much higher than the previous UTXO (3 batches of 30 with increasing fee rate)
102+        # And 1 more to verify that this tx does not get added to the mempool with a fee rate less than the mempoolminfee
103+        miniwallet.generate(1 + (3 * 30) + 1)
104+
105+        # Mine 99 blocks so that the UTXOs are allowed to be spent

hg333 commented at 6:06 am on August 12, 2021:

Why is it sufficient to mine 99 blocks?

DariusParvin commented at 4:34 pm on August 16, 2021:

I think it might be because the block that the coinbase utxo is included in is counted as the first block, so you’d need 99 additional blocks for it to be 100 blocks deep?

But also, is it necessary to use generate? I think this test can be sped up using the miniwallet function scan_blocks instead. There are a few examples of other functional tests using it. Note that you’d need to remove self.setup_clean_chain = True to benefit from it.

hg333 approved

hg333 commented at 6:06 am on August 12, 2021: contributor

tACK https://github.com/bitcoin/bitcoin/pull/22543/commits/dcd75d87eef701b1214699fcad79328f06801176

tested on Ubuntu 20.04 On Master Branch:

0$ time ./test/functional/mempool_limit.py 
1...
2real	0m17.054s
3user	0m3.117s
4sys	0m0.515s

On PR Branch:

0$ time ./test/functional/mempool_limit.py 
1...
2
3real	0m7.905s
4user	0m2.777s
5sys	0m0.139s

Kirandevraj approved

Kirandevraj commented at 10:49 am on August 12, 2021: none

Concept and Approach ACK and Tested c6b17eb Compiled Bitcoin core with this PR patch and –disable-wallet flag and ran the test/functional/mempool_limit.py test: Test successful Compiled Bitcoin core without this PR patch with the same –disable-wallet flag and ran the test: Test Skipped

in test/functional/mempool_limit.py:48 in bec7849f5d outdated

 98+
 99+        # Generate 92 UTXOs to flood the mempool
100+        # 1 to create a tx initially that will be evicted from the mempool later
101+        # 90 with a fee rate much higher than the previous UTXO (3 batches of 30 with increasing fee rate)
102+        # And 1 more to verify that this tx does not get added to the mempool with a fee rate less than the mempoolminfee
103+        miniwallet.generate(1 + (3 * 30) + 1)

DariusParvin commented at 6:51 pm on August 14, 2021:

nit: Maybe just me but I think miniwallet.generate(92) is easier to read since the comment already describes where the 92 comes from.

in test/functional/test_framework/wallet.py:186 in bec7849f5d outdated

182@@ -182,6 +183,26 @@ def sendrawtransaction(self, *, from_node, tx_hex):
183         from_node.sendrawtransaction(tx_hex)
184         self.scan_tx(from_node.decoderawtransaction(tx_hex))
185 
186+    def create_large_transactions(

DariusParvin commented at 6:55 am on August 16, 2021:

I think it would be more consistent with other functions (send_self_transfer, create_self_transfer, sendrawtransaction) for this to be named send_large_transactions since it is also broadcasting them.

Note that it would also make sense to change no_of_large_tx_created to no_of_large_tx_sent or no_of_large_tx_broadcasted.

ShubhamPalriwala commented at 8:53 am on August 16, 2021:

Valid points! Will make suggested changes here to maintain the consistency Edit: Done

in test/functional/mempool_limit.py:66 in bec7849f5d outdated

116+        self.log.info("Fill up the mempool with txs with higher fee rate")
117+        no_of_large_tx_created = 0
118+        for batch_of_txid in range(1, 4):
119+            # Increment the tx fee rate gradually by a factor of (base_fee) for each batch of 30 transactions
120+            no_of_large_tx_created += miniwallet.create_large_transactions(
121+                node, txouts, 30, batch_of_txid * base_fee

DariusParvin commented at 6:59 am on August 16, 2021:

I tried smaller values instead of 30 transactions per batch, and the tests pass with 22 transactions. Is there a specific reason for it to be 30? If not then it seems like 22 is better since it is faster.

ShubhamPalriwala commented at 8:54 am on August 16, 2021:

Just wanted to populate the mempool that’s why went for 30, let’s do 22 then! Edit: Done

DariusParvin commented at 7:08 am on August 16, 2021: contributor

ACK bec7849 Tested it and looks good! A few suggestions

ShubhamPalriwala force-pushed on Aug 16, 2021

Zero-1729 commented at 2:10 pm on August 16, 2021: contributor

tACK 30a6853

Tested on macOS v11.4 and LGTM 🧉

Not sure why CI failed at L71 in mempool_limit.py, locally it checks out (see screenshot below). It looks like CI is reporting 67 for the no of txs in the mempool.

Note: I added extra logs to check

DariusParvin commented at 4:39 pm on August 16, 2021: contributor

I just realized I think you might be able to speed up the test using miniwallet.scan_blocks() instead of node.generate()? It was introduced #21200 and is used in a few of the updated functional tests.

in test/functional/mempool_limit.py:40 in 30a6853203 outdated

90+
91+        node = self.nodes[0]
92+        miniwallet = MiniWallet(node)
93+        relayfee = node.getnetworkinfo()["relayfee"]
94+
95+        self.log.info("Check that mempoolminfee is minrelytxfee")

amitiuttarwar commented at 4:40 pm on August 16, 2021:

0        self.log.info("Check that mempoolminfee is minrelaytxfee")

laanwj commented at 4:47 pm on August 16, 2021: member

Not sure why CI failed at L71 in mempool_limit.py, locally it checks out (see screenshot below). It looks like CI is reporting 67 for the no of txs in the mempool.

Can’t reproduce that locally here either. All the functional tests pass running master with this merged on top. It might be a rare race condition, or something specific to the 32-bit platform though.

in test/functional/test_framework/wallet.py:202 in 30a6853203 outdated

197+                tx_instance.vout.append(txout)
198+            tx_hex = tx_instance.serialize().hex()
199+            # Serializes and sends the tx to the nodes
200+            self.sendrawtransaction(from_node=node, tx_hex=tx_hex)
201+            no_of_tx_created += 1
202+        assert_equal(no_of_tx_created,no_of_tx_ids)

amitiuttarwar commented at 4:59 pm on August 16, 2021:

0        assert_equal(no_of_tx_created, no_of_tx_ids)

ShubhamPalriwala commented at 8:51 pm on August 16, 2021:

This assertion is itself removed now!

in test/functional/test_framework/wallet.py:187 in 30a6853203 outdated

182@@ -182,6 +183,26 @@ def sendrawtransaction(self, *, from_node, tx_hex):
183         from_node.sendrawtransaction(tx_hex)
184         self.scan_tx(from_node.decoderawtransaction(tx_hex))
185 
186+    def send_large_transactions(
187+        self, node, array_of_large_tx, no_of_tx_ids, fee_rate

amitiuttarwar commented at 5:06 pm on August 16, 2021:

variable naming suggestion: replacing no_of_tx_ids with num_txns

in test/functional/test_framework/wallet.py:186 in 30a6853203 outdated

182@@ -182,6 +183,26 @@ def sendrawtransaction(self, *, from_node, tx_hex):
183         from_node.sendrawtransaction(tx_hex)
184         self.scan_tx(from_node.decoderawtransaction(tx_hex))
185 
186+    def send_large_transactions(

amitiuttarwar commented at 5:09 pm on August 16, 2021:

since this is being added as a shared helper, would be nice to add a docstring to explain what this function is doing & possibly guidance on how the callers should provide the params.

in test/functional/test_framework/wallet.py:199 in 30a6853203 outdated

194+            # Converts it into a CTransaction() instance to append the vouts
195+            tx_instance = from_hex(CTransaction(), hex)
196+            for txout in array_of_large_tx:
197+                tx_instance.vout.append(txout)
198+            tx_hex = tx_instance.serialize().hex()
199+            # Serializes and sends the tx to the nodes

amitiuttarwar commented at 5:12 pm on August 16, 2021:

did you mean this comment to be a line prior? since the serialization is the previous line & the sending is the next line.

but you could just get rid of this comment. generally we want to add comments if we can explain why we’re doing something. and let the code explain what is happening. so in this case, this comment isn’t adding any new information.

in test/functional/test_framework/wallet.py:190 in 30a6853203 outdated

182@@ -182,6 +183,26 @@ def sendrawtransaction(self, *, from_node, tx_hex):
183         from_node.sendrawtransaction(tx_hex)
184         self.scan_tx(from_node.decoderawtransaction(tx_hex))
185 
186+    def send_large_transactions(
187+        self, node, array_of_large_tx, no_of_tx_ids, fee_rate
188+    ):
189+        # Create large transactions by appending txouts in vout
190+        no_of_tx_created = 0

amitiuttarwar commented at 5:24 pm on August 16, 2021:

I’m not really seeing the value of this local variable. if there was a way to return early in the for loop, incrementing the number could be a way to keep track of successes. but since there isn’t any logic of the sort, any unsuccessful attempts would simply cause an error (eg. by sendrawtransaction throwing an exception). so assert_equal(no_of_tx_created,no_of_tx_ids) isn’t actually increasing any guarantees from this code.

my suggestion would be to remove the no_of_tx_created logic from this function. you could return no_of_tx_ids if you want to maintain the current return value, or just update the call site since that is where the number of transactions to create gets passed in anyways.

ShubhamPalriwala commented at 8:52 pm on August 16, 2021:

Done! Thanks

in test/functional/mempool_limit.py:34 in 30a6853203 outdated

38 
39-    def skip_test_if_missing_module(self):
40-        self.skip_if_no_wallet()
41-
42     def run_test(self):
43         txouts = gen_return_txouts()

amitiuttarwar commented at 5:31 pm on August 16, 2021:

you could move this line down to right before the for loop where it gets used

to clarify, I’m talking about: txouts = gen_return_txouts()

amitiuttarwar commented at 5:54 pm on August 16, 2021: contributor

RE CI failure: I’m also surprised, I don’t see where there could have been a race since we submit the transactions directly to the node. Could there be variance in the way gen_return_txouts & send_large_transactions play together, which allows there to be an edge case where the size of created transactions doesn’t actually overflow the mempool size limit? Having 67 transactions means the mempool didn’t evict the first one. One way to check / improve is to track the number of bytes of created transactions rather than number of them. Then you can confirm the transaction bytes created is greater than the mempool limit passed in during the test setup. Just a thought.

Thanks for improving this test! Looks good overall, but I left lots of food for thought.

ShubhamPalriwala force-pushed on Aug 16, 2021

amitiuttarwar commented at 11:24 pm on August 16, 2021: contributor

hm, unfortunately looks like the 32-bit + dash [gui] run is failing on the same assertion 🔍

ShubhamPalriwala commented at 4:20 am on August 17, 2021: contributor

It’s failing when we take the minimum batch size of 22 transactions, increasing this will increase the test duration but the CI will pass as earlier

ShubhamPalriwala force-pushed on Aug 17, 2021

in test/functional/mempool_limit.py:75 in 5dbe0df05c outdated

101+        assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000'))
102+        assert_greater_than(
103+            node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')
104+        )
105 
106+        # Deliberately tries to create a tx with a fee less that the minimum mempool fee to assert that it does not get added to the mempool

Zero-1729 commented at 8:33 pm on August 17, 2021:

Minor nit:

0        # Deliberately tries to create a tx with a fee less than the minimum mempool fee to assert that it does not get added to the mempool

in test/functional/mempool_limit.py:50 in 5dbe0df05c outdated

70-            txids[i] = create_lots_of_big_transactions(self.nodes[0], txouts, utxos[30*i:30*i+30], 30, (i+1)*base_fee)
71+        tx_to_be_evicted_id = miniwallet.send_self_transfer(
72+            from_node=node, fee_rate=relayfee
73+        )["txid"]
74+
75+        # Increase the tx fee rate massively now to give the next transactions a higher priority in the mempool

Zero-1729 commented at 8:49 pm on August 17, 2021:

Could consider rewording to:

0        # Increase the tx fee rate massively to give subsequent transactions a higher priority in the mempool

Zero-1729 commented at 8:53 pm on August 17, 2021: contributor

re-tACK 5dbe0df

Everything still checks out locally. Glad to see the CI issue resolved.

Left a few nits.

ShubhamPalriwala force-pushed on Aug 18, 2021

Zero-1729 commented at 9:36 am on August 18, 2021: contributor

re-tACK 3773532d682a3bfb0cea8898da10caa5f444a23e

ShubhamPalriwala commented at 10:28 am on August 18, 2021: contributor

The CI passes now PS: Updated the batch size to 25 transactions below https://github.com/bitcoin/bitcoin/blob/3773532d682a3bfb0cea8898da10caa5f444a23e/test/functional/mempool_limit.py#L67

The PR is ready for a review

in test/functional/mempool_limit.py:32 in 3773532d68 outdated

38 
39-    def skip_test_if_missing_module(self):
40-        self.skip_if_no_wallet()
41-
42     def run_test(self):
43+

amitiuttarwar commented at 6:36 pm on August 18, 2021:

unnecessary new line

in test/functional/mempool_limit.py:43 in 3773532d68 outdated

47+
48+        self.log.info("Check that mempoolminfee is minrelaytxfee")
49+        assert_equal(node.getmempoolinfo()["minrelaytxfee"], Decimal("0.00001000"))
50+        assert_equal(node.getmempoolinfo()["mempoolminfee"], Decimal("0.00001000"))
51+
52+        # Generate 68 UTXOs to flood the mempool

amitiuttarwar commented at 6:40 pm on August 18, 2021:

68 is no longer accurate

0        # Generate 77 UTXOs to flood the mempool

this demonstrates one of the reasons it’s better for comments to supplement reasoning rather than what the code is doing- if the code changes, it’s easy for the comments to fall out of date.

So a better option would be to remove the hardcoding of the UTXO number from these comments.

in test/functional/mempool_limit.py:79 in 3773532d68 outdated

84         )
85 
86         # Deliberately tries to create a tx with a fee less than the minimum mempool fee to assert that it does not get added to the mempool
87-        self.log.info('Create a mempool tx that will not pass mempoolminfee')
88-        assert_raises_rpc_error(-26,"mempool min fee not met",miniwallet.send_self_transfer,from_node=node,fee_rate=relayfee,mempool_valid=False)
89+        self.log.info("Create a mempool tx that will not pass mempoolminfee")

amitiuttarwar commented at 6:48 pm on August 18, 2021:

since you introduced this code in the last commit, it would make more sense to just style it properly there & then not retouch in the subsequent commit

amitiuttarwar commented at 6:49 pm on August 18, 2021: contributor

utACK 3773532d68, but would be good to fix the incorrect comment

lsilva01 approved

lsilva01 commented at 8:58 pm on August 18, 2021: contributor

tACK on Ubuntu 20.04 https://github.com/bitcoin/bitcoin/pull/22543/commits/3773532d682a3bfb0cea8898da10caa5f444a23e

ShubhamPalriwala force-pushed on Aug 20, 2021

in test/functional/test_framework/wallet.py:188 in f0afb33dd8 outdated

182@@ -182,6 +183,23 @@ def sendrawtransaction(self, *, from_node, tx_hex):
183         from_node.sendrawtransaction(tx_hex)
184         self.scan_tx(from_node.decoderawtransaction(tx_hex))
185 
186+    def send_large_transactions(
187+        self, node, array_of_large_tx, num_txns, fee_rate
188+    ):

theStack commented at 3:37 pm on August 20, 2021:

Listing the parameters in an extra line is a style that we don’t follow for function definitions, as the following search in the codebase shows:

0$ cd ./test/functional
1$ git grep "def .*($"
2test_framework/wallet.py:    def send_large_transactions(

(the only result is the one introduced in this commit)

0    def send_large_transactions(self, node, array_of_large_tx, num_txns, fee_rate):

in test/functional/test_framework/wallet.py:195 in f0afb33dd8 outdated

190+        # Create large transactions by appending txouts in vout
191+        for _ in range(num_txns):
192+            # Create a self transfer here to get the tx details and then append the vout to increase the tx size
193+            hex = self.create_self_transfer(from_node=node, fee_rate=fee_rate)['hex']
194+            # Converts it into a CTransaction() instance to append the vouts
195+            tx_instance = from_hex(CTransaction(), hex)

theStack commented at 3:48 pm on August 20, 2021:

For the CTransaction case, we have an explicit helper tx_from_hex that is already imported (i.e. you can remove the from_hex import again):

0            tx_instance = tx_from_hex(hex)

in test/functional/test_framework/wallet.py:199 in f0afb33dd8 outdated

194+            # Converts it into a CTransaction() instance to append the vouts
195+            tx_instance = from_hex(CTransaction(), hex)
196+            for txout in array_of_large_tx:
197+                tx_instance.vout.append(txout)
198+            tx_hex = tx_instance.serialize().hex()
199+            self.sendrawtransaction(from_node=node, tx_hex=tx_hex)

theStack commented at 3:50 pm on August 20, 2021:

nit: probably a matter of taste, but there’s no need for introducing a temporary variable:

0            self.sendrawtransaction(from_node=node, tx_hex=tx_instance.serialize().hex())

in test/functional/mempool_limit.py:10 in f0afb33dd8 outdated

 4@@ -5,9 +5,12 @@
 5 """Test mempool limiting together/eviction with the wallet."""
 6 
 7 from decimal import Decimal
 8+from test_framework.blocktools import COINBASE_MATURITY
 9 
10 from test_framework.test_framework import BitcoinTestFramework

theStack commented at 3:55 pm on August 20, 2021:

I’d suggest grouping the COINBASE_MATURITY import to the (test-framework related) ones below, i.e.:

0from decimal import Decimal
1
2from test_framework.blocktools import COINBASE_MATURITY
3from test_framework.test_framework import BitcoinTestFramework

in test/functional/mempool_limit.py:47 in f0afb33dd8 outdated

42 
43-        txids = []
44-        utxos = create_confirmed_utxos(relayfee, self.nodes[0], 91)
45+        # Generate UTXOs to flood the mempool
46+        # 1 to create a tx initially that will be evicted from the mempool later
47+        # 3 batches of multiple transactions with a fee rate much higher than the previous UTXO

theStack commented at 4:09 pm on August 20, 2021:

I’d in some way mention here that each batch consists of 25 transactions – without that it is not immedately clear how you end up with the number 77.

ShubhamPalriwala commented at 4:48 pm on August 20, 2021:

Updated the code below the comments as miniwallet.generate(1 + (3 * 25) + 1) to make it clear

in test/functional/mempool_limit.py:74 in f0afb33dd8 outdated

100+        assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000'))
101+        assert_greater_than(
102+            node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')
103+        )
104 
105+        # Deliberately tries to create a tx with a fee less than the minimum mempool fee to assert that it does not get added to the mempool

theStack commented at 4:09 pm on August 20, 2021:

0        # Deliberately try to create a tx with a fee less than the minimum mempool fee to assert that it does not get added to the mempool

theStack commented at 4:10 pm on August 20, 2021: member

LGTM in general. Left some stylistic/grammar improvement suggestions below, will review the logic more in detail later.

ShubhamPalriwala force-pushed on Aug 20, 2021

theStack commented at 9:24 pm on August 21, 2021: member

@ShubhamPalriwala: Thanks for the changes. Currently the second commit (subject “style: mempool_limit.py”) is not showing up any more, you probably forgot it in your latest force-push?

DrahtBot added the label Needs rebase on Aug 24, 2021

ShubhamPalriwala force-pushed on Aug 24, 2021

DrahtBot removed the label Needs rebase on Aug 24, 2021

ShubhamPalriwala commented at 8:27 pm on August 24, 2021: contributor

The PR has been rebased and is ready to be reviewed

in test/functional/test_framework/wallet.py:190 in eb0ff37956 outdated

181@@ -182,6 +182,19 @@ def sendrawtransaction(self, *, from_node, tx_hex):
182         from_node.sendrawtransaction(tx_hex)
183         self.scan_tx(from_node.decoderawtransaction(tx_hex))
184 
185+    def send_large_transactions(self, node, array_of_large_tx, num_txns, fee_rate):
186+        """Creates and sends large transactions with the passed fee rate."""
187+        # Create large transactions by appending txouts in vout
188+        for _ in range(num_txns):
189+            # Create a self transfer here to get the tx details and then append the vout to increase the tx size
190+            hex = self.create_self_transfer(from_node=node, fee_rate=fee_rate)['hex']

theStack commented at 11:11 am on August 25, 2021:

Sorry for not noticing this earlier in my last review round, but it looks like the conversion from hex-to-tx is not necessary here, since create_self_transfer already provides you a CTransaction instance (can be accessed via ['tx']). I.e. the method can even be shorter:

 0--- a/test/functional/test_framework/wallet.py
 1+++ b/test/functional/test_framework/wallet.py
 2@@ -187,12 +187,10 @@ class MiniWallet:
 3         # Create large transactions by appending txouts in vout
 4         for _ in range(num_txns):
 5             # Create a self transfer here to get the tx details and then append the vout to increase the tx size
 6-            hex = self.create_self_transfer(from_node=node, fee_rate=fee_rate)['hex']
 7-            # Converts it into a CTransaction() instance to append the vouts
 8-            tx_instance = tx_from_hex(hex)
 9+            tx = self.create_self_transfer(from_node=node, fee_rate=fee_rate)['tx']
10             for txout in array_of_large_tx:
11-                tx_instance.vout.append(txout)
12-            self.sendrawtransaction(from_node=node, tx_hex=tx_instance.serialize().hex())
13+                tx.vout.append(txout)
14+            self.sendrawtransaction(from_node=node, tx_hex=tx.serialize().hex())
15         return num_txns

in test/functional/mempool_limit.py:46 in 53283a6b27 outdated

50+
51+        # Generate UTXOs to flood the mempool
52+        # 1 to create a tx initially that will be evicted from the mempool later
53+        # 3 batches of multiple transactions with a fee rate much higher than the previous UTXO
54+        # And 1 more to verify that this tx does not get added to the mempool with a fee rate less than the mempoolminfee
55+        miniwallet.generate(1 + (3 * 25) + 1)

MarcoFalke commented at 11:18 am on August 25, 2021:

Would be nice to use self.generate(miniwallet, ...) for new code

in test/functional/mempool_limit.py:49 in 53283a6b27 outdated

53+        # 3 batches of multiple transactions with a fee rate much higher than the previous UTXO
54+        # And 1 more to verify that this tx does not get added to the mempool with a fee rate less than the mempoolminfee
55+        miniwallet.generate(1 + (3 * 25) + 1)
56+
57+        # Mine 99 blocks so that the UTXOs are allowed to be spent
58+        node.generate(COINBASE_MATURITY - 1)

MarcoFalke commented at 11:18 am on August 25, 2021:

same

ShubhamPalriwala force-pushed on Aug 26, 2021

ShubhamPalriwala commented at 9:40 am on August 26, 2021: contributor

Thanks for the suggestions @theStack and @MarcoFalke!

I’ve made, tested, and pushed the subsequent commits! The PR is ready to be reviewed again :)

ShubhamPalriwala force-pushed on Aug 26, 2021

Zero-1729 commented at 11:45 am on August 28, 2021: contributor

re-ACK 4c5c9d6 🧪

LGTM, all comments & suggestions addressed, no behaviour changes (ran locally to verify).

in test/functional/mempool_limit.py:56 in 4902ce86af outdated

76+        no_of_large_tx_sent = 0
77+        txouts = gen_return_txouts()
78+
79+        for batch_of_txid in range(1, 4):
80+            # Increment the tx fee rate gradually by a factor of (base_fee) for each batch
81+            no_of_large_tx_sent += miniwallet.send_large_transactions(node, txouts, 25, batch_of_txid * base_fee)

josibake commented at 10:46 am on August 31, 2021:

in https://github.com/bitcoin/bitcoin/pull/22543/commits/4902ce86afc6b9c8a7a72b1c91466fe26a00f9b9:

id suggest using a named variable (instead of just 25) to make this more clear. also, no_of_large... should be num_of_large...:

0tx_batch_size = 25
1for batch_of_txid in range(1, 4):
2    # Increment the tx fee rate gradually by a factor of (base_fee) for each batch
3    miniwallet.send_large_transactions(node, txouts, tx_batch_size, batch_of_txid * base_fee) 
4    num_of_large_tx_sent += tx_batch_size

ShubhamPalriwala commented at 10:30 am on September 2, 2021:

Thank you for the naming suggestion. Has been fixed

in test/functional/test_framework/wallet.py:194 in 4902ce86af outdated

189+            # Create a self transfer here to get the tx details and then append the vout to increase the tx size
190+            tx = self.create_self_transfer(from_node=node, fee_rate=fee_rate)['tx']
191+            for txout in array_of_large_tx:
192+                tx.vout.append(txout)
193+            self.sendrawtransaction(from_node=node, tx_hex=tx.serialize().hex())
194+        return num_txns

josibake commented at 10:50 am on August 31, 2021:

is there a reason you have this helper function returning the same variable that was passed in? personally, i find this confusing and would not expect to have to handle a returned value if i were using this in another test

ShubhamPalriwala commented at 10:31 am on September 2, 2021:

Makes sense, as pointed out by other reviewers too, I have taken this into consideration, and converted this into a void function.

josibake commented at 10:55 am on August 31, 2021: member

Concept and Approach ACK

great to see more miniwallet usage if the actual wallet is not needed. this also makes the test logic easier to understand. i agree with adding the helper function to miniwallet directly for sending large transactions as this is a pattern that shows up a lot in tests. i did leave a few comments that i think would make the test (and function) more intuitive

ShubhamPalriwala force-pushed on Sep 2, 2021

ShubhamPalriwala commented at 10:50 am on September 2, 2021: contributor

PR ready for review

This PR has been made more understandable by the recent reviews! Changes have been made as suggested and it’s once again Up for Review.

in test/functional/mempool_limit.py:59 in 0b361fe5b8 outdated

79+        txouts = gen_return_txouts()
80+
81+        for batch_of_txid in range(num_of_batches):
82+            # Increment the tx fee rate gradually by a factor of (base_fee) for each batch
83+            miniwallet.send_large_transactions(node, txouts, tx_batch_size, (batch_of_txid+1) * base_fee)
84+            num_of_large_tx_sent +=tx_batch_size

josibake commented at 11:04 am on September 2, 2021:

this should be

0            num_of_large_tx_sent += tx_batch_size

per https://www.python.org/dev/peps/pep-0008/#other-recommendations

ShubhamPalriwala commented at 11:35 am on September 2, 2021:

All the styling changes have been handled in e8a3ff3b586beb4e2bb2d77a3bf4d7b98a1a2df2 :))

Hope this answers the query

josibake commented at 12:12 pm on September 2, 2021:

ah, gotcha, i missed that in the second commit.

in general, I would avoid re-editing the same line multiple times in different commits as it requires the reviewer to look at the same line multiple times. not a big deal tho

MarcoFalke commented at 3:49 pm on September 2, 2021:

It is a bit confusing to add new code in the wrong style and then fix it up in the second commit. Why not add it with the right style from the beginning?

josibake commented at 11:05 am on September 2, 2021: member

thanks for taking the suggestion for send_large_transactions! one small pep8 fix suggested

josibake commented at 12:14 pm on September 2, 2021: member

ACK https://github.com/bitcoin/bitcoin/commit/e8a3ff3b586beb4e2bb2d77a3bf4d7b98a1a2df2

nicely done, happy to see the send_large_transactions helper function being added to miniwallet!

Zero-1729 commented at 12:22 pm on September 2, 2021: contributor

re-ACK e8a3ff3

LGTM 💾

in test/functional/mempool_limit.py:77 in 0b361fe5b8 outdated

112+        assert_raises_rpc_error(-26,"mempool min fee not met",miniwallet.send_self_transfer,from_node=node,fee_rate=relayfee,mempool_valid=False)
113+
114 
115 if __name__ == '__main__':
116-    MempoolLimitTest().main()
117+    MempoolLimitTest().main()

MarcoFalke commented at 3:45 pm on September 2, 2021:

It is a bit confusing to mess with the style in unrelated lines in the first commit and then fix it up in the second commit. Why touch the lines in the first place?

in test/functional/mempool_limit.py:73 in 0b361fe5b8 outdated

107-        tx = self.nodes[0].createrawtransaction(inputs, outputs)
108-        # specifically fund this tx with a fee < mempoolminfee, >= than minrelaytxfee
109-        txF = self.nodes[0].fundrawtransaction(tx, {'feeRate': relayfee})
110-        txFS = self.nodes[0].signrawtransactionwithwallet(txF['hex'])
111-        assert_raises_rpc_error(-26, "mempool min fee not met", self.nodes[0].sendrawtransaction, txFS['hex'])
112+        assert_raises_rpc_error(-26,"mempool min fee not met",miniwallet.send_self_transfer,from_node=node,fee_rate=relayfee,mempool_valid=False)

MarcoFalke commented at 3:45 pm on September 2, 2021:

It is a bit confusing to add new code in the wrong style and then fix it up in the second commit. Why not add it with the right style from the beginning?

in test/functional/test_framework/wallet.py:185 in 0b361fe5b8 outdated

181@@ -182,6 +182,16 @@ def sendrawtransaction(self, *, from_node, tx_hex):
182         from_node.sendrawtransaction(tx_hex)
183         self.scan_tx(from_node.decoderawtransaction(tx_hex))
184 
185+    def send_large_transactions(self, node, array_of_large_tx, num_txns, fee_rate):

MarcoFalke commented at 3:47 pm on September 2, 2021:

Is this needed to be put into miniwallet?

If not, it would be better suited in the test that uses it. Either as function or inline, because it is only called once.

Regardless, the interface should be cleaned up. array_of_large_tx is not an array of txs, but of txouts.

amitiuttarwar commented at 6:54 pm on September 2, 2021:

I thought it could make sense as a shared helper since I expected the functionality to be useful in different tests as well. Although this only introduces one caller, would make it easier for others to find.

MarcoFalke commented at 7:59 am on September 3, 2021:

The issue I see is that the interface isn’t correct in that the passed fee_rate won’t be the fee rate of the tx returned. Inlining it would avoid the confusion in the interface.

amitiuttarwar commented at 4:03 pm on September 4, 2021:

gotcha, yeah agreed that the interface should be self-evident if the function is kept here, or moving it over would be another option.

ShubhamPalriwala commented at 11:50 am on September 10, 2021:

Makes sense, this is a very specific case hence I have moved the helper function to the mempool_limit.py Thanks for the suggestion

in test/functional/mempool_limit.py:27 in 0b361fe5b8 outdated

27-        self.skip_if_no_wallet()
28-
29     def run_test(self):
30-        txouts = gen_return_txouts()
31-        relayfee = self.nodes[0].getnetworkinfo()['relayfee']
32+        node = self.nodes[0]

MarcoFalke commented at 3:48 pm on September 2, 2021:

It is a bit confusing to review if the self.node[0] -> node replacement is done in the same commit as the miniwallet changes. Maybe split it up?

ShubhamPalriwala commented at 11:49 am on September 10, 2021:

Thank you for the suggestion. I have split up the commits

ShubhamPalriwala force-pushed on Sep 10, 2021

ShubhamPalriwala commented at 11:52 am on September 10, 2021: contributor

PR ready for review

Thank you to everybody who has reviewed it. However, the recent suggestions made the PR better and more helpful hence the changes as suggested have been implemented.

in test/functional/mempool_limit.py:63 in 22c73921c5 outdated

81+        base_fee = relayfee * 1000
82+
83+        self.log.info("Fill up the mempool with txs with higher fee rate")
84+        num_of_large_tx_sent = 0
85+        for batch_of_txid in range(num_of_batches):
86+            self.send_large_txs(node, miniwallet, txouts, batch_of_txid, base_fee, tx_batch_size)

MarcoFalke commented at 8:20 am on September 13, 2021:

nit in the first commit:

It could make sense to drop the batch_of_txid argument and instead pass the calculated fee rate. This reduces the number of arguments passed to the function and increases its flexibility.

Though, an alternative would be to simply inline the function.

0            self.send_large_txs(node, miniwallet, txouts, (batch_of_txid + 1) * base_fee, tx_batch_size)

ShubhamPalriwala commented at 11:20 am on September 13, 2021:

Makes sense! On it

in test/functional/mempool_limit.py:68 in 22c73921c5 outdated

89         self.log.info('The tx should be evicted by now')
90-        assert txid not in self.nodes[0].getrawmempool()
91-        txdata = self.nodes[0].gettransaction(txid)
92-        assert txdata['confirmations'] ==  0  #confirmation should still be 0
93+        # The number of transactions created should be greater than the ones present in the mempool
94+        assert_greater_than(num_of_large_tx_sent, len(node.getrawmempool()))

MarcoFalke commented at 8:28 am on September 13, 2021:

in the first commit: Is the num_of_large_tx_sent really needed? Wouldn’t this be equal to num_of_batches * tx_batch_size?

Also, the mempool should be deterministic, so it could make sense to check the exact number and mention in a comment that it should be less than 90 or num_of_batches * tx_batch_size?

ShubhamPalriwala commented at 11:54 am on September 13, 2021:

Done! Assuming one can understand that tx_batch_size * num_of_batches is equal to the number of transactions created, works!

MarcoFalke approved

MarcoFalke commented at 8:29 am on September 13, 2021: member

review ACK 22c73921c5c753c2e97fefbd207b163e610af15 🍚

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 22c73921c5c753c2e97fefbd207b163e610af15 🍚
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiNiQwAlZ9W+JInYlneTbs9NDBJWdFX+6yy4GyhGTls+oxQZa+f4AM7uNK28ak1
 8oja9zQn9c/xTfOO3k2C3W6ly0M7aonKgW09++7AWtSGT3vKQmQN0ElvZdPJVH+gb
 90aOxetJQrVHxBvwvFmiS2mq9WqmHLMPCOKZ9Rkj/36hMH5ItI/ylkBccqfq3RS/9
101XidxJWmtFVBai2nqmVrRfYIJenpplxDFMJXevMV9oq0iSsC1DL2boB3wBCtp0uY
11OGlI1ghPWFO3kACX+oI+ruHIqxGuyA7lChhIs/D+dDckXeyk5rKwtaBPXS08qbuu
12wYk71Co0wq2tlj3lI7LsRZwaQ1Zovhhdw2iwPe6Q1KPaRCO9wn7eCtHEenn7gS7v
138CEBAtqaOGL11JBq9IyIGnHfo5cMERUdyeuY0lb34RCsieMoDXlMT9OgBD9JXEhw
14hdoJnrBwWZ5T39/gdmmsSJUs5MpF2nP+9Mk2YN5zI+kTRzCIwVPFo15FfOK+HO1z
15aUF/Ov9m
16=3RiU
17-----END PGP SIGNATURE-----

Timestamp of file with hash 668a0177b4b7850b6d72a3a9f719e35848f7d38faaf8b37423b4d77b70903b93 -

MarcoFalke commented at 8:31 am on September 13, 2021: member

lmk if you want this merged or address the nits

ShubhamPalriwala commented at 11:58 am on September 13, 2021: contributor

@MarcoFalke the nits have been addressed! PTAL. The PR is ready to be merged from my end :)

MarcoFalke commented at 12:26 pm on September 13, 2021: member

ack

amitiuttarwar commented at 6:48 pm on September 13, 2021: contributor

I find the git commit breakup a bit confusing - I think the last commit 41fa226d27ab224d02bc21a47f560c32298612ab should be merged into the first commit cdf1bf67251b4089e96c2656cdbb913835c53f63, because it’s cleaning up the code that was introduced in this PR. It makes more sense to just introduce it correctly in the first place- it simplifies review & git blame history in the future.

Also, I’d prefer if the git commit messages were more descriptive & adhered to the conventions of this codebase. For example, I’ve never seen the prefix feat:. I used the command git log -S "miniwallet" to see that most of the miniwallet refactors use the prefix test: or maybe refactor:. A more common prefix instead of replace: or fix: would probably be style:. In terms of descriptiveness, commit messages are the most useful when you explain why the change is being made. While this is fine to skip on self-evident changes (like fixing a typo or renaming to node), the first commit could describe the motivation. If you’re interested in learning more, check out https://chris.beams.io/posts/git-commit/.

All that said, I think the code introduced looks reasonable, so review ACK 41fa226d27ab224d02bc21a47f560c32298612ab. If you decide to clean up the git history, I’m happy to reack- it should not produce a code diff.

test: use MiniWallet in mempool_limit.py

Co-authored-by: ShubhamPalriwala <spalriwalau@gmail.com>
Co-authored-by: stackman27 <sishirg27@gmail.com>
Signed-off-by: ShubhamPalriwala <spalriwalau@gmail.com>

dddca3899c

replace: self.nodes[0] with node d447ded6ba

fix typos in logging messages 08634e82c6

ShubhamPalriwala force-pushed on Sep 13, 2021

ShubhamPalriwala commented at 7:33 pm on September 13, 2021: contributor

Thank you for the git suggestions @amitiuttarwar Updated and force-pushed the commits for a more clear understanding of the changes.

The PR is once again up for review. Hopefully one last time

amitiuttarwar commented at 8:02 pm on September 13, 2021: contributor

ACK 08634e8, only git changes since last push (and one new line).

Zero-1729 commented at 9:58 pm on September 13, 2021: contributor

ACK 08634e82c68ea1be79e1395f4f551082f497023f 🧉

MarcoFalke merged this on Sep 14, 2021

MarcoFalke closed this on Sep 14, 2021

MarcoFalke commented at 9:12 am on September 14, 2021: member

Thanks for taking the feedback. Looks good now.

theStack commented at 9:32 am on September 14, 2021: member

Note that the helper send_large_txs with its current interface is misleading, as the passed fee_rate is not respected. It is merely passed to MiniWallet’s create_self_transfer, but this function can’t look in the future and thus is unable to know that several outputs are appended to the transaction after, modifying its size and hence also decreasing it’s feerate. I’d suggest to either pass an absolute fee that is simply deducted from tx.vout[0].nValue before the transaction is sent (simple solution), or keep the interface, but fix the fee calculation by deducting dependend on the tx’s final vsize (a little more complex solution). In both cases, create_self_transfer needs to be called with a zero fee_rate first. Will take a look into that later.

MarcoFalke commented at 9:38 am on September 14, 2021: member

@theStack See also #22543 (review) , where I mentioned in-lining helps. I think we should pick the simplest/shortest code to achieve the goal of creating txs with different fee levels. The feerate doesn’t need to be exact for the purposes of mempool eviction. The current code achieves its goal, but improvements are welcome if there are any.

theStack commented at 2:10 pm on September 14, 2021: member

@MarcoFalke: Sure, but in this case the fee-rate is not just “not exact”, but off by several orders of magnitute, considering that the tx’s vsize changes from 96 to 67552 vbytes (>700x), and MiniWallet only calculates the fee based on the 96 vbytes. So the value passed to this function is neither really a fee-rate nor an absolute fee, but something weird in-between that has just been increased (by 10x) until the test passes. I tried to increase the clarity of the test in #22972.

sidhujag referenced this in commit d3c814108b on Sep 15, 2021

MarcoFalke referenced this in commit c426e0dc6f on Oct 29, 2021

DrahtBot locked this on Oct 30, 2022

test: Use MiniWallet in mempool_limit.py #22543

Conflicts

PR ready for review

PR ready for review

The PR is once again up for review. Hopefully one last time