test: Run mempool_limit.py even with wallet disabled

stackman27 commented at 9:40 am on January 7, 2021: contributor

This is a PR proposed in #20078.

This PR enables one more of the non-wallet functional tests by running mempool_limit.py even when the wallet is disabled. While restructuring the code, I added an extra method, send_multi_self_transfer in MiniWallet class. This method allows the creation of large transactions (by appending txouts to tx.vout) which will be useful in other files like mining_prioritisetransaction.py as well.

While my approach may not be the cleanest or the most efficient, I totally appreciate any suggestion to improve it. Thank you very much!

fanquake added the label Tests on Jan 7, 2021

DrahtBot commented at 7:55 pm on January 7, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21178 (test: run mempool_reorg.py even with wallet disabled by DariusParvin)

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.

in test/functional/mempool_limit.py:37 in 543919b4b1 outdated

39 
40-        txids = []
41-        utxos = create_confirmed_utxos(relayfee, self.nodes[0], 91)
42+        txids=[]
43+        miniwallet.generate(91)
44+        node.generate(100)

joelklabo commented at 4:55 pm on January 8, 2021:

Just curious, is there a reason to generate blocks through the MiniWallet and the TestNode separately?

mjdietzx commented at 5:03 pm on January 8, 2021:

We generate blocks in MiniWallet so we have utxos to spend (coinbases). It is helpful to generate 100 blocks on the node separately, so all the utxos in MiniWallet are mature.

If we just did, miniwallet.generate(191), only 91 of the utxos in mini wallets utxo set will be mature. And it gets tricky because some miniwallet transactions will now fail (And we don’t necessarily know which utxo we are gonna get as mini wallet sorts them by size)

stackman27 commented at 2:50 am on January 9, 2021:

@joelklabo I used the previous miniWallet tests as reference to generate utxos like the one done here https://github.com/bitcoin/bitcoin/pull/20688/files

joelklabo commented at 5:48 am on January 9, 2021:

Great, thanks been confused about that for a bit. I’ll check it out!

in test/functional/mempool_limit.py:40 in 543919b4b1 outdated

54-        txFS = self.nodes[0].signrawtransactionwithwallet(txF['hex'])
55-        txid = self.nodes[0].sendrawtransaction(txFS['hex'])
56-
57-        relayfee = self.nodes[0].getnetworkinfo()['relayfee']
58+        txid = miniwallet.send_self_transfer(fee_rate = relayfee, from_node = node)['txid']
59+        relayfee = node.getnetworkinfo()['relayfee']

mjdietzx commented at 5:14 pm on January 8, 2021:

Not sure if relayfee = node.getnetworkinfo()['relayfee'] is needed here anymore. seems like it shouldn’t have changed, right?

in test/functional/mempool_limit.py:51 in 543919b4b1 outdated

67-        assert txid not in self.nodes[0].getrawmempool()
68-        txdata = self.nodes[0].gettransaction(txid)
69-        assert txdata['confirmations'] ==  0  #confirmation should still be 0
70+        assert txid not in node.getrawmempool()
71 
72         self.log.info('Check that mempoolminfee is larger than minrelytxfee')

mjdietzx commented at 5:28 pm on January 8, 2021:

nit: might as well fix this type-o minrelytxfee => minrelaytxfee?

in test/functional/test_framework/wallet.py:96 in 543919b4b1 outdated

91+        tx.vout = [CTxOut(int(send_value * COIN), self._scriptPubKey)]
92+        tx.wit.vtxinwit = [CTxInWitness()]
93+        tx.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])]
94+        tx_hex = tx.serialize().hex()
95+
96+        if(big_txouts is not None):

mjdietzx commented at 5:34 pm on January 8, 2021:

nit: need a space between if and (. I think our linter won’t even want the parenthesis (superfluous parenthesis). Really, why not just do: if big_txouts: … else:

in test/functional/test_framework/wallet.py:98 in 543919b4b1 outdated

93+        tx.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])]
94+        tx_hex = tx.serialize().hex()
95+
96+        if(big_txouts is not None):
97+            for txout in big_txouts:
98+                tx.vout.append(txout)

mjdietzx commented at 5:37 pm on January 8, 2021:

nit: tx.vout.extend(big_txouts) would have the same behavior and get rid of a loop

in test/functional/test_framework/wallet.py:102 in 543919b4b1 outdated

 97+            for txout in big_txouts:
 98+                tx.vout.append(txout)
 99+            tx_hex = tx.serialize().hex()
100+            txid = from_node.sendrawtransaction(tx_hex, 0)
101+        else:
102+            txid = from_node.sendrawtransaction(tx_hex)

mjdietzx commented at 5:39 pm on January 8, 2021:

Do we even want this else condition? I would think big_txouts should be a required param. Otherwise this branch seems to just duplicate send_self_transfer

in test/functional/test_framework/wallet.py:85 in 543919b4b1 outdated

76@@ -77,3 +77,30 @@ def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_sp
77         assert_equal(tx_info['vsize'], vsize)
78         assert_equal(tx_info['fee'], fee)
79         return {'txid': txid, 'wtxid': tx_info['wtxid'], 'hex': tx_hex}
80+
81+    def send_multi_self_transfer(self, *, fee_rate=Decimal("0.03"), from_node, utxo_to_spend=None, big_txouts=None):
82+        """Create multiple tx with the specified fee_rate. Fee may be exact or at most one satoshi higher than needed."""
83+        self._utxos = sorted(self._utxos, key=lambda k: k['value'])
84+        utxo_to_spend = utxo_to_spend or self._utxos.pop() # Pick the largest utxo (if none provided) and hope it covers the fee
85+        send_value = satoshi_round(utxo_to_spend['value'] - fee_rate)

mjdietzx commented at 5:57 pm on January 8, 2021:

fee_rate seems to be a confusing param name. As this is now just the absolute fee, right?

in test/functional/test_framework/wallet.py:81 in 543919b4b1 outdated

76@@ -77,3 +77,30 @@ def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_sp
77         assert_equal(tx_info['vsize'], vsize)
78         assert_equal(tx_info['fee'], fee)
79         return {'txid': txid, 'wtxid': tx_info['wtxid'], 'hex': tx_hex}
80+
81+    def send_multi_self_transfer(self, *, fee_rate=Decimal("0.03"), from_node, utxo_to_spend=None, big_txouts=None):

mjdietzx commented at 6:10 pm on January 8, 2021:

I’m not sure this method name is ideal? I was expecting multiple txns, but I guess this sends one big transaction? I’m not sure send_big_self_transfer would be better, but just want to double check on naming here

in test/functional/mempool_limit.py:45 in 543919b4b1 outdated

59+        relayfee = node.getnetworkinfo()['relayfee']
60         base_fee = relayfee*100
61         for i in range (3):
62             txids.append([])
63-            txids[i] = create_lots_of_big_transactions(self.nodes[0], txouts, utxos[30*i:30*i+30], 30, (i+1)*base_fee)
64+            txids[i] = self.create_large_transactions(node, txouts, miniwallet, 30, (i+1)*base_fee)

mjdietzx commented at 6:13 pm on January 8, 2021:

I know this is existing behavior. But I’ma little confused because txids aren’t used. Should we be checking these are in the mempool?

stackman27 commented at 2:11 am on January 9, 2021:

I’m not very sure about that too. I just followed the existing behavior and kept it there. If it really serves no purpose, i can either get rid of it or add a test case to check those txs in the mempool

mjdietzx commented at 7:43 pm on January 10, 2021:

Since it’s not related to this PR I’d leave it. But I think adding an assertion to check all these txs are in the mempool would be good, and at least its a sanity check to make sure everything is as we expect

in test/functional/mempool_limit.py:54 in 543919b4b1 outdated

82-        tx = self.nodes[0].createrawtransaction(inputs, outputs)
83-        # specifically fund this tx with a fee < mempoolminfee, >= than minrelaytxfee
84-        txF = self.nodes[0].fundrawtransaction(tx, {'feeRate': relayfee})
85-        txFS = self.nodes[0].signrawtransactionwithwallet(txF['hex'])
86-        assert_raises_rpc_error(-26, "mempool min fee not met", self.nodes[0].sendrawtransaction, txFS['hex'])
87+        insufficient_fee_utxo = miniwallet.get_utxo()

mjdietzx commented at 6:17 pm on January 8, 2021:

Is there a need to grab a specific utxo here? I would think you can just let miniwallet pick it, and do:

assert_raises_rpc_error(-26, "mempool min fee not met", miniwallet.send_self_transfer, from_node=node, fee_rate=relayfee)

mjdietzx commented at 6:23 pm on January 8, 2021:

I think there is some confusing behavior here. bc you generate 91 utxos, then spend 1, spend 3*30, and are now spending again (so this won’t spend a Coinbase, rather it’ll spend one of our previous transactions).

And bc you explicitly get_utxo you are going to get the utxo from last transaction from create_large_transactions . Whereas if you let MiniWallet pick, it’d sort the utxo set by value, and you’d probably get the transaction that was evicted from the mempool since it had the lowest fee. idk what would happen in that case, very confusing to me.

Anyways, maybe you just want to generate 92 utxos at the get-go?

in test/functional/test_framework/wallet.py:99 in 543919b4b1 outdated

100+            txid = from_node.sendrawtransaction(tx_hex, 0)
101+        else:
102+            txid = from_node.sendrawtransaction(tx_hex)
103+        self._utxos.append({'txid': txid, 'vout': 0, 'value': send_value})
104+        tx_info = from_node.getmempoolentry(txid)
105+        assert_equal(tx_info['fee'], fee)

mjdietzx commented at 6:27 pm on January 8, 2021:

Just want to confirm that we no longer need a vsize assertion. I guess we don’t know the vsize or don’t care since we are passing an absolute fee and not calculating the fee based on vsize*fee_rate?

stackman27 commented at 2:44 am on January 9, 2021:

Yes thats what i realized. Also the vsize changed (from 96 to 67552) while making big txs and therefore checking it with a constant value of 96 didn’t really make sense. I can however, add an assertion to check big tx vsize since they’re all the same. Do you recommend?

mjdietzx commented at 7:42 pm on January 10, 2021:

I’m not really sure tbh. But we may be able to get rid of all of this, re one of my other comments in this review

mjdietzx commented at 6:28 pm on January 8, 2021: contributor

I took a look at doing this refactoring a week or so ago, and figured I’d do a few easier ones bs this one was so tricky. But looks like you figured it out! Anyways I left a bunch of comments on things I noticed

stackman27 force-pushed on Jan 9, 2021

stackman27 commented at 2:52 am on January 9, 2021: contributor

@mjdietzx Thank you very much for the review. I just pushed an update shortening the methods in miniwallet class by adding prepare_tx and also resolved most of your comments

in test/functional/mempool_limit.py:67 in 5546a4f7b6 outdated

87+
88+    def create_large_transactions(self, node, txouts, miniwallet, num, fee):
89+        large_txids = []
90+        for _ in range(num):
91+            large_txids.append(miniwallet.send_big_self_transfer(from_node=node, big_txouts=txouts, fee=fee)['txid'])
92+        return large_txids

mjdietzx commented at 7:29 pm on January 10, 2021:

if you wanted to be “cool” you could do:

0return map(lambda: miniwallet.send_big_self_transfer(from_node=node, big_txouts=txouts, fee=fee)['txid'], range(num))

stackman27 commented at 11:10 pm on January 10, 2021:

oh ya! I’ll try that 😎

in test/functional/mempool_limit.py:53 in 5546a4f7b6 outdated

70 
71         self.log.info('Check that mempoolminfee is larger than minrelytxfee')
72-        assert_equal(self.nodes[0].getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000'))
73-        assert_greater_than(self.nodes[0].getmempoolinfo()['mempoolminfee'], Decimal('0.00001000'))
74+        assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000'))
75+        assert_greater_than(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000'))

mjdietzx commented at 7:33 pm on January 10, 2021:

so just for my understanding, I guess mempoolminfee increases as the mempool fills up? Would you mind explaining this to me or linking to some info/code so I can learn? thanks

stackman27 commented at 11:10 pm on January 10, 2021:

The mempoolminfee is adjusted whenever the mempool reaches maxmempool size, and therefore it kicks out all the transaction below that fee, to make space for tx with accepted fees. I can’t find a specific method in the codebase as of now but i’ll lyk if I find it

mjdietzx commented at 0:36 am on January 11, 2021:

ah ok interesting, thanks for the explanation

in test/functional/test_framework/wallet.py:85 in 5546a4f7b6 outdated

86         tx_info = from_node.getmempoolentry(txid)
87         assert_equal(tx_info['vsize'], vsize)
88         assert_equal(tx_info['fee'], fee)
89         return {'txid': txid, 'wtxid': tx_info['wtxid'], 'hex': tx_hex}
90+
91+    def send_big_self_transfer(self, *, fee, from_node, utxo_to_spend=None, big_txouts):

mjdietzx commented at 7:35 pm on January 10, 2021:

I think big_txouts should come before the optional utxo_to_spend=None to spend now that it’s required, right?

in test/functional/test_framework/wallet.py:58 in 5546a4f7b6 outdated

54@@ -55,6 +55,15 @@ def get_utxo(self, *, txid=''):
55             index = self._utxos.index(utxo)
56         return self._utxos.pop(index)
57 
58+    def prepare_tx(self, utxo_to_spend, send_value):

mjdietzx commented at 7:41 pm on January 10, 2021:

See this: #20808 (review)

Basically I’m wondering if you’re going to even need to touch MiniWallet in this PR if we can align here. If we extend MiniWallet to have an option to “just-create-don’t-send” then you could

create the base tx with MiniWallet but don’t send
add all the big_txouts to that tx
send it yourself, outside of MiniWallet

To me it seems like that may be the cleanest solution

stackman27 commented at 11:14 pm on January 10, 2021:

🤔 hmm i’ll look into it and try to find a clean solution

stackman27 commented at 11:53 pm on January 10, 2021:

If I take this approach the code would be a lot cleaner but i’d have to get rid of some of the assertion checks for the tx because, i dont think i’ll have access to variables such as send_value or fee unless I really dig into the tx list:

The assertions & appends aren’t used in the current code. So i’m tempted to not include them in the new code unless they modify MiniWallet variable behaviors.

mjdietzx commented at 0:44 am on January 11, 2021:

I think it’s normal to do some assertions outside of MiniWallet if you think they’re needed. ie we have things in the current miniwallet tests like: assert_equal(entry_time, node.getmempoolentry(parent_txid)['time']) after sending a transaction.

Anyways do what you think is best here. If my proposed solution ends up having problems or doesn’t make sense I don’t want to mislead you

stackman27 commented at 0:58 am on January 11, 2021:

For sure! pushed a new commit with required changes :)

glozow commented at 8:26 pm on January 12, 2021:

Could we get a more informative function name and/or description? Also I think you should rename to utxos and make it a list. “utxo_to_spend” is kinda like “uneaten food to be eaten.”

stackman27 commented at 9:30 pm on January 12, 2021:

😅 my bad. Have never too good at naming functions. But will definitely keep in mind

stackman27 force-pushed on Jan 11, 2021

stackman27 commented at 0:57 am on January 11, 2021: contributor

@mjdietzx Thank you very much for the review. Pushed a new update highlighting all the comments and also updated MiniWallet by adding just-create-don’t-send method as proposed by @glozow in #20876. Also, I believe the new create_and_sign_rawtx method should resolve @nginocchio’s issues as proposed in #20808

stackman27 force-pushed on Jan 11, 2021

DrahtBot added the label Needs rebase on Jan 11, 2021

stackman27 force-pushed on Jan 11, 2021

DrahtBot removed the label Needs rebase on Jan 11, 2021

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

64 
65         self.log.info('The tx should be evicted by now')
66-        assert txid not in self.nodes[0].getrawmempool()
67-        txdata = self.nodes[0].gettransaction(txid)
68-        assert txdata['confirmations'] ==  0  #confirmation should still be 0
69+        assert txid not in txids # check txid inside the txids list

mjdietzx commented at 7:08 pm on January 12, 2021:

I was expecting something more along the lines of: assert txid in node.getrawmempool() for txid in txids

idk if that actually works, maybe assert_equal(len(list(lambda txid: txid in node.getrawmempool(), txids)), len(txids))

glozow commented at 7:57 pm on January 12, 2021:

I’m not really sure what this line does. You are asserting that none of the txns have the same txid?

stackman27 commented at 9:03 pm on January 12, 2021:

I was just trying to check whether txid is present in the txids list or not. We checked txid’s presence in the mempool but txids was just left hanging, so i was just working around an assertion for txids

glozow commented at 9:16 pm on January 12, 2021:

Sure, but why do you need to do that?

stackman27 commented at 10:00 pm on January 12, 2021:

@mjdietzx the assertion didn’t work and I believe its because len(txids) is 3 with this format [[*txs*],[*txs*],[*txs*]] and each of the *txs* has 30 transactions. And node.getrawmempool() is just a giant list with 65 txs ☹️

in test/functional/mempool_limit.py:60 in f5eb2bcca4 outdated

87+        assert_raises_rpc_error(-26, "mempool min fee not met", miniwallet.send_self_transfer, from_node=node, fee_rate=relayfee)
88+
89+    def create_large_transactions(self, node, txouts, miniwallet, num, fee):
90+        large_txids = []
91+        for _ in range(num):
92+            tx = miniwallet.create_and_sign_rawtx(from_node=node, flat_fee=fee)

mjdietzx commented at 7:13 pm on January 12, 2021:

If you pull in this change https://github.com/bitcoin/bitcoin/pull/20808/files#diff-7932a60a9127fd22d10d367526fd7b987f9647ce017595f8b1af5c32d5db0083R58-R59

I think you could do

0tx = miniwallet.create_and_sign_rawtx(from_node=node, fee_rate=fee_rate, submit_tx=False)  # calculate `fee_rate` so it yields the flat fee that you want
1tx.vout.extend(txouts)
2...

stackman27 commented at 8:58 pm on January 12, 2021:

I did try that solution but the issue was on flat_fee vs calculated fee, and i believe create_large_tx uses flat_fee. I didn’t wanted to use multiple if/else to make the code look unattractive. I did however find a hack by changing the base fee from base_fee = relayfee*100 => base_fee = relayfee*1000 . This didn’t change the overall behavior of the test code, but changed the mempoolminfee which is pretty expected.

I’m just wondering if we would need flat_fee based create-dont-send-tx for future reference? And if we do, would that require a different method?

mjdietzx commented at 7:15 pm on January 12, 2021: contributor

Nice, I think this is close! If you look at my comment, I think you can do this PR in a way that wallet.py is not changed by bringing in the submit_tx=False change for miniwallet.send_self_transfer

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

65         self.log.info('The tx should be evicted by now')
66-        assert txid not in self.nodes[0].getrawmempool()
67-        txdata = self.nodes[0].gettransaction(txid)
68-        assert txdata['confirmations'] ==  0  #confirmation should still be 0
69+        assert txid not in txids # check txid inside the txids list
70+        assert txid not in node.getrawmempool() # check txid in the rawmempool

glozow commented at 7:58 pm on January 12, 2021:

You can do a stronger assertion

0        assert_equal(sorted(txids), sorted(node.getrawmempool()))

stackman27 commented at 9:27 pm on January 12, 2021:

I believe there are 90 txs in txids, and 65 in getrawmempool. I tested this with the current code because I thought there was some issue with my code, but same result

glozow commented at 8:35 pm on January 12, 2021: member

Concept ACK. I actually think a cleaner approach would be to make create_lots_of_big_transactions and the other util.py transaction not need wallet. Could reduce the amount of refactoring needed in each of the functional tests?

stackman27 force-pushed on Jan 12, 2021

DrahtBot added the label Needs rebase on Jan 15, 2021

stackman27 force-pushed on Jan 15, 2021

DrahtBot removed the label Needs rebase on Jan 15, 2021

stackman27 force-pushed on Jan 22, 2021

stackman27 force-pushed on Jan 25, 2021

stackman27 force-pushed on Jan 27, 2021

stackman27 force-pushed on Feb 1, 2021

stackman27 commented at 7:59 pm on February 1, 2021: contributor

Thank you @glozow @mjdietzx for the review. Added stronger assertion as suggested :)

stackman27 force-pushed on Feb 2, 2021

in test/functional/mempool_limit.py:37 in 18951db04d outdated

40 
41-        txids = []
42-        utxos = create_confirmed_utxos(relayfee, self.nodes[0], 91)
43+        txids=[]
44+        miniwallet.generate(92)
45+        node.generate(100)

MarcoFalke commented at 4:57 pm on February 16, 2021:

Generating blocks takes a few seconds in valgrind, so I am thinking if this test may benefit from using miniwallet.scan_blocks (to be introduced in #21200). I haven’t looked at the details as why you picked 92 blocks to be mined, but if you can do with just 25 mature coinbases, this could make sense.

stackman27 commented at 3:15 pm on February 19, 2021:

Miniwallet.scan_blocks looks amazing! Can’t wait to use it. The reason behind 92 is that we are creating 90 transaction as a part of create_large_transactions method. Also, the same number is present in the current code, so I just thought about mimicking it.

stackman27 force-pushed on Feb 19, 2021

stackman27 force-pushed on Feb 22, 2021

stackman27 force-pushed on Feb 25, 2021

stackman27 commented at 3:46 pm on March 4, 2021: contributor

Concept ACK. I actually think a cleaner approach would be to make create_lots_of_big_transactions and the other util.py transaction not need wallet. Could reduce the amount of refactoring needed in each of the functional tests? @glozow Thank you very much for the review! Are you suggesting an alternative way to create_lots_of_big_transactions or just simply getting rid of it?

in test/functional/mempool_limit.py:45 in a478bf775f outdated

61+        mempool_min_fee = node.getmempoolinfo()['mempoolminfee']
62+        base_fee = relayfee*1000
63         for i in range (3):
64             txids.append([])
65-            txids[i] = create_lots_of_big_transactions(self.nodes[0], txouts, utxos[30*i:30*i+30], 30, (i+1)*base_fee)
66+            txids[i] = self.create_large_transactions(node, txouts, miniwallet, 30, (i+1)*base_fee, mempool_min_fee)

DariusParvin commented at 0:04 am on March 22, 2021:

What is the purpose of creating these transactions within this loop with the fee increasing each time? It seems unnecessarily complicated but I am probably missing something.

stackman27 commented at 3:05 am on April 2, 2021:

This is part of the feature as we’re flooding mempool with transaction with higher fees in every loop. This is also a part of the current codebase so I’m pretty much mimicking it

in test/functional/mempool_limit.py:65 in a478bf775f outdated

94+        for j in range(num):
95+            hex = miniwallet.send_self_transfer(from_node=node, fee_rate=fee, submit_tx=False)['hex']
96+            tx = FromHex(CTransaction(), hex)
97+            tx.vout.extend(txouts)
98+            tx_hex = tx.serialize().hex()
99+            txids = node.sendrawtransaction(tx_hex, 0)

DariusParvin commented at 1:04 am on March 22, 2021:

nit: since it’s just one txid, perhaps rename txids to txid?

in test/functional/mempool_limit.py:58 in a478bf775f outdated

87-        txF = self.nodes[0].fundrawtransaction(tx, {'feeRate': relayfee})
88-        txFS = self.nodes[0].signrawtransactionwithwallet(txF['hex'])
89-        assert_raises_rpc_error(-26, "mempool min fee not met", self.nodes[0].sendrawtransaction, txFS['hex'])
90+        assert_raises_rpc_error(-26, "mempool min fee not met", miniwallet.send_self_transfer, from_node=node, fee_rate=relayfee)
91+
92+    def create_large_transactions(self, node, txouts, miniwallet, num, fee, mempool_min_fee):

DariusParvin commented at 1:52 am on March 22, 2021:

I’m wondering if send_large_transactions would be a better name since it is also broadcasting them? It would be more consistent with send_self_transfer.

stackman27 commented at 3:11 am on April 2, 2021:

hmm just following the naming convention used here as it’s just a copy of that code. But happy to change :)

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

 95+            hex = miniwallet.send_self_transfer(from_node=node, fee_rate=fee, submit_tx=False)['hex']
 96+            tx = FromHex(CTransaction(), hex)
 97+            tx.vout.extend(txouts)
 98+            tx_hex = tx.serialize().hex()
 99+            txids = node.sendrawtransaction(tx_hex, 0)
100+            if node.getmempoolinfo()['mempoolminfee'] == mempool_min_fee:

DariusParvin commented at 2:00 am on March 22, 2021:

What is the purpose of having this condition in here? As a result, it means that there are transactions which get broadcast and enter the mempool, but are not added to large_txids. I’ll add a comment to where it’s used to suggest what I think would be a better alternative.

stackman27 commented at 3:14 am on April 2, 2021:

okay! will add a comment but the basic logic is that i’m making sure that the fees have changed and only include those txs whose fees have gone up. This also supports the assert logic at like 48

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

67 
68         self.log.info('The tx should be evicted by now')
69-        assert txid not in self.nodes[0].getrawmempool()
70-        txdata = self.nodes[0].gettransaction(txid)
71-        assert txdata['confirmations'] ==  0  #confirmation should still be 0
72+        assert_equal(len([tx for txid in txids for tx in txid]), len(node.getrawmempool()))

DariusParvin commented at 2:04 am on March 22, 2021:

Here i’m continuing from the comment below about line 66. I take it this is to double check that the mempool is full? I think a better way might be to remove the condition you have in create_large_transactions, so that you return all the broadcasted txids, and instead over here you use assert_greater_than, as in:

assert_greater_than(len([tx for txid in txids for tx in txid]), len(node.getrawmempool()))

DariusParvin commented at 2:12 am on March 22, 2021:

nit: it’s a bit confusing here since tx is actually a txid, and txid is another list of txids. I don’t think this is much better but maybe [txid for batch in txids for txid in batch]?

DariusParvin commented at 2:16 am on March 22, 2021: contributor

Concept ACK! Just some suggestions that might make it more readable. Also, now that Miniwallet.scan_blocks has been merged, I think you can use it at the start.

stackman27 force-pushed on Apr 16, 2021

stackman27 commented at 9:31 pm on April 16, 2021: contributor

Thank you @DariusParvin for the review. Added changes as suggested

in test/functional/mempool_limit.py:60 in ed1e869e14 outdated

89-        assert_raises_rpc_error(-26, "mempool min fee not met", self.nodes[0].sendrawtransaction, txFS['hex'])
90+        assert_raises_rpc_error(-26, "mempool min fee not met", miniwallet.send_self_transfer, from_node=node, fee_rate=relayfee)
91+
92+    def create_large_transactions(self, node, txouts, miniwallet, num, fee, mempool_min_fee):
93+        large_txids = []
94+        for j in range(num):

DariusParvin commented at 5:55 am on April 19, 2021:

nit: since j is unused, use _ instead?

in test/functional/mempool_limit.py:45 in ed1e869e14 outdated

61+        mempool_min_fee = node.getmempoolinfo()['mempoolminfee']
62+        base_fee = relayfee*1000
63         for i in range (3):
64             txids.append([])
65-            txids[i] = create_lots_of_big_transactions(self.nodes[0], txouts, utxos[30*i:30*i+30], 30, (i+1)*base_fee)
66+            txids[i] = self.create_large_transactions(node, txouts, miniwallet, 30, (i+1)*base_fee, mempool_min_fee)

DariusParvin commented at 6:01 am on April 19, 2021:

here num is set to 30 but the test also works with 22 (with only the original tx getting kicked from the mempool). My feeling is that it’s more beneficial to have the test run faster than to have some extra leeway with excess txs.

0            txids[i] = self.create_large_transactions(node, txouts, miniwallet, 22, (i+1)*base_fee, mempool_min_fee)

DrahtBot added the label Needs rebase on Apr 29, 2021

test:Miniwallet:added create_and_sign_rawtx option 3b14c57fe3

test:run mempool_limit with wallet disabled 9d2b30cc05

stackman27 force-pushed on May 27, 2021

DrahtBot removed the label Needs rebase on May 27, 2021

DrahtBot added the label Needs rebase on Jun 1, 2021

DrahtBot commented at 2:19 pm on June 1, 2021: member

🐙 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”.

MarcoFalke commented at 7:32 am on July 20, 2021: member

Are you still working on this?

stackman27 commented at 7:02 pm on July 20, 2021: contributor

Are you still working on this?

Not at the moment, have been busy with work and classes

fanquake added the label Up for grabs on Jul 21, 2021

fanquake closed this on Jul 21, 2021

MarcoFalke removed the label Up for grabs on Jul 25, 2021

MarcoFalke referenced this in commit 2b264971ad on Sep 14, 2021

DrahtBot locked this on Aug 18, 2022

test: Run mempool_limit.py even with wallet disabled #20874

Conflicts