test: Run mempool_accept.py even with wallet disabled

stackman27 commented at 6:49 pm on January 26, 2021: contributor

This is a PR proposed in #20078

This PR enables one more of the non-wallet functional tests (mempool_accept.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead.

This PR also includes changes on wallet.py to accommodate some of the features used in mempool_accept.py like explicitly stating nSequence for RBF and sending tx with max fee = 0

michaelfolkson commented at 7:02 pm on January 26, 2021: contributor

Concept ACK

Can you squash your commits @stackman27?

DrahtBot added the label Tests on Jan 26, 2021

stackman27 force-pushed on Jan 26, 2021

stackman27 commented at 9:00 pm on January 26, 2021: contributor

Concept ACK

Can you squash your commits @stackman27?

Yes! Should be fine now :)

DrahtBot commented at 1:25 am on January 27, 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:

#21762 (test: Speed up mempool_spend_coinbase.py by MarcoFalke)
#21754 (test: Run feature_cltv with MiniWallet by MarcoFalke)
#21178 (test: run mempool_reorg.py even with wallet disabled by DariusParvin)
#20874 (test: Run mempool_limit.py even with wallet disabled by stackman27)

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.

stackman27 force-pushed on Jan 27, 2021

in test/functional/mempool_accept.py:125 in 45010220ce outdated

121@@ -139,6 +122,7 @@ def run_test(self):
122         self.log.info('A transaction that conflicts with an unconfirmed tx')
123         # Send the transaction that replaces the mempool transaction and opts out of replaceability
124         node.sendrawtransaction(hexstring=tx.serialize().hex(), maxfeerate=0)
125+        self.mempool_size += 1

0xB10C commented at 9:38 am on January 28, 2021:

Why do we +1 here?

stackman27 commented at 5:14 pm on February 5, 2021:

oops that was my bad 😅. However, I just pushed a new commit and fixed the issue. Not the most eloquent solution, but I’d highly appreciate any suggestions to improve it

in test/functional/mempool_accept.py:306 in 45010220ce outdated

305@@ -327,13 +306,13 @@ def run_test(self):
306         self.log.info('A transaction that is locked by BIP68 sequence logic')
307         tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_reference)))
308         tx.vin[0].nSequence = 2  # We could include it in the second block mined from now, but not the very next one
309+        tx.nVersion = 2

0xB10C commented at 9:42 am on January 28, 2021:

Why not add version to send_self_transfer() as optional argument too?

stackman27 commented at 5:15 pm on February 5, 2021:

I did try to do that, but I wasn’t sure about the stylistic aspect of passing arguments in functions. Do we have a hard cap on the # of arguments we can pass on functions?

DariusParvin commented at 2:02 am on February 15, 2021:

Maybe it’s worth just assuming nVersion = 2 in the MiniWallet, and add it as an option argument in the future if/when there’s a test that requires nVersion = 1?

stackman27 force-pushed on Feb 5, 2021

in test/functional/mempool_accept.py:63 in 27f078c484 outdated

59 
60         self.log.info('Start with empty mempool, and 200 blocks')
61         self.mempool_size = 0
62-        assert_equal(node.getblockcount(), 200)
63+        miniwallet.generate(200)
64+        assert_equal(node.getblockcount(), 400)

amitiuttarwar commented at 2:04 am on February 7, 2021:

the log 3 lines up says 200 blocks, but here we check 400. can you make these consistent?

stackman27 commented at 6:36 pm on February 7, 2021:

Got it! Fixed

in test/functional/mempool_accept.py:125 in 27f078c484 outdated

121@@ -139,6 +122,7 @@ def run_test(self):
122         self.log.info('A transaction that conflicts with an unconfirmed tx')
123         # Send the transaction that replaces the mempool transaction and opts out of replaceability
124         node.sendrawtransaction(hexstring=tx.serialize().hex(), maxfeerate=0)
125+        #self.mempool_size += 1

amitiuttarwar commented at 2:05 am on February 7, 2021:

I think this line should be deleted. If you meant it communicate something about the test, please clarify

in test/functional/test_framework/wallet.py:70 in 27f078c484 outdated

67         self._utxos = sorted(self._utxos, key=lambda k: k['value'])
68         utxo_to_spend = utxo_to_spend or self._utxos.pop()  # Pick the largest utxo (if none provided) and hope it covers the fee
69+
70         vsize = Decimal(96)
71-        send_value = satoshi_round(utxo_to_spend['value'] - fee_rate * (vsize / 1000))
72+        send_value = satoshi_round(utxo_to_spend['value'] - fee_rate * (vsize / 1000)) if send_tx else satoshi_round(utxo_to_spend['value'] - fee_rate)

amitiuttarwar commented at 2:10 am on February 7, 2021:

I find this one-line if/else to be pretty confusing. could you just break it into a few lines to clarify the logic flow?

stackman27 commented at 6:37 pm on February 7, 2021:

I broke it down, I hope it makes more sense now :)

stackman27 force-pushed on Feb 7, 2021

in test/functional/mempool_accept.py:73 in 582d57cc94 outdated

78-            outputs=[{node.getnewaddress(): 0.3}, {node.getnewaddress(): 49}],
79-        ))['hex']
80-        txid_in_block = node.sendrawtransaction(hexstring=raw_tx_in_block, maxfeerate=0)
81-        node.generate(1)
82+        tx_in_block = miniwallet.send_self_transfer(from_node = node, default_fee = True)
83+        miniwallet.generate(1) # Generate a block, tx gets mined therefore mempool is empty

MarcoFalke commented at 4:12 pm on February 8, 2021:

Any reason to change this from node.generate(1)? miniwallet.generate will put an immature utxo in the wallet.

in test/functional/mempool_accept.py:86 in 582d57cc94 outdated

86-            result_expected=[{'txid': txid_in_block, 'allowed': False, 'reject-reason': 'txn-already-known'}],
87-            rawtxs=[raw_tx_in_block],
88+            result_expected=[{'txid': tx_in_block['txid'], 'allowed': False, 'reject-reason': 'txn-already-known'}],
89+            rawtxs=[tx_in_block['hex']],
90         )
91-

MarcoFalke commented at 4:13 pm on February 8, 2021:

any reason to change this? Previously there was a newline between each test

in test/functional/mempool_accept.py:81 in 582d57cc94 outdated

93         fee = Decimal('0.000007')
94-        raw_tx_0 = node.signrawtransactionwithwallet(node.createrawtransaction(
95-            inputs=[{"txid": txid_in_block, "vout": 0, "sequence": BIP125_SEQUENCE_NUMBER}],  # RBF is used later
96-            outputs=[{node.getnewaddress(): Decimal('0.3') - fee}],
97-        ))['hex']
98+        raw_tx_0 = miniwallet.send_self_transfer(from_node = node, seq_num = BIP125_SEQUENCE_NUMBER,  fee_rate = fee, send_tx = False)['hex'] #RBF enabled (0xffffffd)

MarcoFalke commented at 4:17 pm on February 8, 2021:

Any reason to mention 0xffffffd?

in test/functional/mempool_accept.py:91 in 582d57cc94 outdated

 95-            outputs=[{node.getnewaddress(): output_amount}],
 96-            locktime=node.getblockcount() + 2000,  # Can be anything
 97-        ))['hex']
 98-        tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_final)))
 99-        fee_expected = coin['amount'] - output_amount
100+        raw_tx_final = miniwallet.send_self_transfer(from_node = node, send_tx = False)

MarcoFalke commented at 4:18 pm on February 8, 2021:

why is this no longer passing SEQUENCE_FINAL, etc…?

stackman27 commented at 4:58 pm on February 8, 2021:

Thats because the default is set to 0xffffffff in send_send_transfer parameter

MarcoFalke commented at 5:23 pm on February 8, 2021:

So if the default changes, the test will fail? If so, I’d prefer to set it here to avoid a changed default breaking the test.

stackman27 commented at 5:30 pm on February 8, 2021:

Got it! Thank you

in test/functional/mempool_accept.py:114 in 582d57cc94 outdated

110@@ -128,7 +111,7 @@ def run_test(self):
111         tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_0)))
112         tx.vout[0].nValue -= int(fee * COIN)  # Double the fee
113         tx.vin[0].nSequence = BIP125_SEQUENCE_NUMBER + 1  # Now, opt out of RBF
114-        raw_tx_0 = node.signrawtransactionwithwallet(tx.serialize().hex())['hex']
115+        raw_tx_0 = miniwallet.send_self_transfer(from_node = node, fee_rate = fee * 2, send_tx = False, existing_tx = tx)['hex']

MarcoFalke commented at 4:24 pm on February 8, 2021:

Why is this call needed? The tx should already be signed

stackman27 commented at 4:59 pm on February 8, 2021:

I thought we changed the nValue and nSequence of raw_tx_0 therefore we have to resign it, is that not the case?

MarcoFalke commented at 5:24 pm on February 8, 2021:

miniwallet doesn’t do any sighashing, so no. What is the error message you see when not “signing”?

stackman27 commented at 5:29 pm on February 8, 2021:

I commented 114 out and it fails this assertion

0 self.check_mempool_result(
1  result_expected=[{'txid': txid_0, 'allowed': True, 'vsize': tx.get_vsize(), 'fees': {'base': (2 * fee)}}],
2  rawtxs=[raw_tx_0],
3 )
4****** OUTPUT ******
5AssertionError: not([{'txid': '9295ae1bef2de5cb6dcf3a63504d2a58435572a32cf95caa078cd1a988f19101', 'allowed': True, 'vsize': 96, 'fees': {'base': Decimal('0.000014')}}] == [{'txid': '9295ae1bef2de5cb6dcf3a63504d2a58435572a32cf95caa078cd1a988f19101', 'allowed': False, 'reject-reason': 'txn-already-in-mempool'}])

MarcoFalke commented at 5:42 pm on February 8, 2021:

You can’t comment out the line, but you can remove the “signing roundtrip” and simply take the tx as is.

stackman27 commented at 5:55 pm on February 8, 2021:

I’m sorry but do you mean just take the raw_tx_0 = tx.serialize().hex()?

stackman27 commented at 6:19 pm on February 8, 2021:

I tried the above approach but am unable to get/spend the utxo in line 149/151 associated with txid_0, because it doesnot have tides with send_self_transfer . Is there any way around it?

MarcoFalke commented at 6:21 pm on February 8, 2021:

Maybe add a send_tx to miniwallet which does the needed steps?

MarcoFalke commented at 4:24 pm on February 8, 2021: member

left some comments

stackman27 force-pushed on Feb 9, 2021

in test/functional/mempool_accept.py:41 in 91c2848db8 outdated

34@@ -35,7 +35,7 @@
35     assert_raises_rpc_error,
36     hex_str_to_bytes,
37 )
38-
39+from test_framework.wallet import MiniWallet
40 
41 class MempoolAcceptanceTest(BitcoinTestFramework):
42     def set_test_params(self):

DariusParvin commented at 4:11 pm on February 12, 2021:

You could use self.setup_clean_chain = True to start the blockchain with 0 blocks.

stackman27 commented at 4:22 pm on February 19, 2021:

Fixed!

in test/functional/test_framework/wallet.py:64 in 91c2848db8 outdated

61         self._utxos = sorted(self._utxos, key=lambda k: k['value'])
62         utxo_to_spend = utxo_to_spend or self._utxos.pop()  # Pick the largest utxo (if none provided) and hope it covers the fee
63         vsize = Decimal(96)
64-        send_value = satoshi_round(utxo_to_spend['value'] - fee_rate * (vsize / 1000))
65+
66+        # if send_tx is set to False then calculate send_value without vsize

DariusParvin commented at 2:00 am on February 15, 2021:

I’m curious why send_tx = False should affect the fee?

stackman27 commented at 4:21 pm on February 19, 2021:

It was just a little hack that I found to get the vsize and fee assertion to work. But, I believe I found a way out! Pushed a new commit :)

in test/functional/test_framework/wallet.py:58 in 91c2848db8 outdated

54@@ -55,25 +55,34 @@ def get_utxo(self, *, txid=''):
55             index = self._utxos.index(utxo)
56         return self._utxos.pop(index)
57 
58-    def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_spend=None):
59+    def send_self_transfer(self, *, fee_rate=Decimal("0.003"), seq_num = 0xffffffff, from_node, utxo_to_spend=None, send_tx = True, default_fee = False):

DariusParvin commented at 2:08 am on February 15, 2021:

It seems like it might be misleading to have a method whose name is send_self_transfer, but then have a flag to say send = False.

Instead, maybe it would be more straightforward to have a separate method create_self_transfer, that returns the tx hex. This function could be used within send_self_transfer to create the transaction before broadcasting it.

I’m about to submit a PR for another test where I had a go at doing it this way.

stackman27 commented at 4:21 pm on February 19, 2021:

That totally makes sense! or maybe we can simply rename the method to something that mimics it’s accurate behavior, something in the lines of sign_or_send_self_transfer. Would love to hear @MarcoFalke’s thoughts on this

in test/functional/test_framework/wallet.py:80 in 91c2848db8 outdated

78         tx.wit.vtxinwit = [CTxInWitness()]
79         tx.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])]
80         tx_hex = tx.serialize().hex()
81 
82         tx_info = from_node.testmempoolaccept([tx_hex])[0]
83         self._utxos.append({'txid': tx_info['txid'], 'vout': 0, 'value': send_value})

DariusParvin commented at 2:18 am on February 15, 2021:

If send_tx was set to False, this utxo could be unspendable, potentially causing problems downstream where the get_utxo does not yield a spendable utxo. So I it’s probably better to only append the new utxo if send_tx == True, after calling sendrawtransaction.

stackman27 commented at 4:22 pm on February 19, 2021:

Thank you for the suggestion! I moved the append utxo after sendrawtransaction

DariusParvin commented at 2:43 am on February 15, 2021: contributor

Looks good! The only thing is I think having a separate create_self_transfer method would be more straightforward than having a send_tx flag in send_self_transfer. With the send_tx flag, it also becomes a bit ambiguous whether the new utxo has been appended to the list of spendable utxos when send_tx = False.

test:minwallet:changes to accomodate mempool_accept 4cd36f695e

in test/functional/mempool_accept.py:62 in 91c2848db8 outdated

59 
60-        self.log.info('Start with empty mempool, and 200 blocks')
61+        self.log.info('Start with empty mempool, and 400 blocks')
62         self.mempool_size = 0
63-        assert_equal(node.getblockcount(), 200)
64+        miniwallet.generate(200)

MarcoFalke commented at 4:55 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)

stackman27 force-pushed on Feb 19, 2021

test:run mempool_accept even when wallet is disabled 5ed2bdef6d

stackman27 force-pushed on Mar 1, 2021

in test/functional/test_framework/wallet.py:58 in 5ed2bdef6d

54@@ -55,7 +55,7 @@ def get_utxo(self, *, txid=''):
55             index = self._utxos.index(utxo)
56         return self._utxos.pop(index)
57 
58-    def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_spend=None):
59+    def send_self_transfer(self, *, fee_rate=Decimal("0.003"), seq_num = 0xffffffff, from_node, utxo_to_spend=None, send_tx = True, nVersion = 1, default_fee = False):

glozow commented at 3:19 am on March 3, 2021:

Since it’s optional

0    def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_spend=None, send_tx=True, nVersion=1, seq_num=0xffffffff, default_fee=False):

stackman27 commented at 3:35 pm on March 4, 2021:

Gotchu!

in test/functional/test_framework/wallet.py:77 in 5ed2bdef6d

75 
76         tx_info = from_node.testmempoolaccept([tx_hex])[0]
77-        self._utxos.append({'txid': tx_info['txid'], 'vout': 0, 'value': send_value})
78-        from_node.sendrawtransaction(tx_hex)
79+        if send_tx:
80+            if default_fee:

glozow commented at 3:26 am on March 3, 2021:

This should be called bypass_maxfeerate. But really, since it’s a parameter for sendrawtransaction, why isn’t the caller doing a send_tx=False and then submitting it themselves?

stackman27 commented at 3:34 pm on March 4, 2021:

So this was following Darius’s comment. I could only append tx in self.utxos after sending a transaction. If I try to simply send transaction from somewhere else, I wouldn’t be able to append it in the internal wallet.py list

in test/functional/test_framework/wallet.py:70 in 5ed2bdef6d

64@@ -65,15 +65,20 @@ def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_sp
65         assert send_value > 0
66 
67         tx = CTransaction()
68-        tx.vin = [CTxIn(COutPoint(int(utxo_to_spend['txid'], 16), utxo_to_spend['vout']))]
69+        tx.vin = [CTxIn(COutPoint(int(utxo_to_spend['txid'], 16), utxo_to_spend['vout']), nSequence = seq_num)]
70         tx.vout = [CTxOut(int(send_value * COIN), self._scriptPubKey)]
71+        tx.nVersion = 2

glozow commented at 3:28 am on March 3, 2021:

You’re accepting a nVersion param but not using it?

stackman27 commented at 3:29 pm on March 4, 2021:

oops my bad 😅

in test/functional/mempool_accept.py:83 in 5ed2bdef6d

93-        fee = Decimal('0.000007')
94-        raw_tx_0 = node.signrawtransactionwithwallet(node.createrawtransaction(
95-            inputs=[{"txid": txid_in_block, "vout": 0, "sequence": BIP125_SEQUENCE_NUMBER}],  # RBF is used later
96-            outputs=[{node.getnewaddress(): Decimal('0.3') - fee}],
97-        ))['hex']
98+        fee = Decimal('0.00000672')

glozow commented at 3:36 am on March 3, 2021:

You’re changing the fee?

stackman27 commented at 3:27 pm on March 4, 2021:

Yea cause the previous fee of fee = Decimal('0.000007') was too low to pass the mempool test. I manually calculated the fee here fee_rate = Decimal('0.00007672') - fee,, to get the best option

in test/functional/mempool_accept.py:64 in 5ed2bdef6d

55@@ -58,38 +56,32 @@ def check_mempool_result(self, result_expected, *args, **kwargs):
56 
57     def run_test(self):
58         node = self.nodes[0]
59+        miniwallet = MiniWallet(node)
60 
61         self.log.info('Start with empty mempool, and 200 blocks')
62         self.mempool_size = 0
63+        miniwallet.generate(10)
64+        node.generate(190)

glozow commented at 3:39 am on March 3, 2021:

Assuming this is done to mature the coinbases… is there any reason to do more than 100?

stackman27 commented at 3:28 pm on March 4, 2021:

Not really, just wanted to keep 200 blocks to mimic the current code

glozow commented at 3:41 am on March 3, 2021: member

A few comments Also on style - from PEP8: “Don’t use spaces around the = sign when used to indicate a keyword argument, or when used to indicate a default value for an unannotated function parameter”

DrahtBot added the label Needs rebase on Apr 29, 2021

DrahtBot commented at 7:10 am on April 29, 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 1:53 pm on July 18, 2021: member

Are you still working on this? Otherwise someone else could pick this up.

MarcoFalke commented at 6:36 pm on July 20, 2021: member

Picked up in #22509

fanquake closed this on Jul 21, 2021

DrahtBot locked this on Aug 16, 2022

test: Run mempool_accept.py even with wallet disabled #21014

Conflicts