test: Use MiniWallet in mempool

sriramdvt commented at 6:23 pm on July 20, 2021: contributor

Replace all wallet-related functionality in test/functional/mempool_accept.py to use MiniWallet instead of the wallet built with Bitcoin Core. This allows the test to run even if Bitcoin Core was compiled with --disable-wallet.

Work on mempool_accept.py started in #21014, but it has been inactive for some time. This PR also makes use of additional features like scan_blocks() and create_self_transfer() that were added to MiniWallet.

To test this PR, build Bitcoin Core with(out) the wallet and run:

0$ test/functional/mempool_accept.py

sriramdvt force-pushed on Jul 20, 2021

DrahtBot added the label Tests on Jul 20, 2021

theStack commented at 9:29 am on July 21, 2021: member

Concept ACK

DrahtBot commented at 10:39 am on July 21, 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:

#22788 (scripted-diff: Use generate* from TestFramework by MarcoFalke)

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.

sriramdvt force-pushed on Jul 21, 2021

theStack commented at 12:23 pm on July 21, 2021: member

@sriramdvt: Your PR was fine as it is – #22378 is not merged into master yet, so there is no need to resolve a conflict by now; you can remove the second commit. The DrahtBot conflict message above just says that whenever either your PR or #22378 is merged, the other one very likely has to be rebased.

sriramdvt force-pushed on Jul 21, 2021

MarcoFalke commented at 1:27 pm on July 21, 2021: member

Concept ACK.

In the commit message, the Co-authored-by: needs to be a trailer (last line of the message). ( https://docs.github.com/en/github/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors )

in test/functional/mempool_accept.py:72 in 936f56bb2a outdated

73-        raw_tx_in_block = node.signrawtransactionwithwallet(node.createrawtransaction(
74-            inputs=[{'txid': coin['txid'], 'vout': coin['vout']}],
75-            outputs=[{node.getnewaddress(): 0.3}, {node.getnewaddress(): 49}],
76-        ))['hex']
77-        txid_in_block = node.sendrawtransaction(hexstring=raw_tx_in_block, maxfeerate=0)
78+        raw_tx_in_block = miniwallet.send_self_transfer(from_node=node)

theStack commented at 3:44 pm on July 21, 2021:

Naming: since the mini-wallet methods {create,send}_self_transfer() return more than just the raw transaction, I’d remove the raw_ prefix in those cases.

sriramdvt commented at 6:21 pm on July 21, 2021:

Removed all instances where having a raw_ prefix is misleading. There is one instance where raw_tx_coinbase_spent gets a raw transaction from node.getrawtransaction(). This is the only transaction variable that has a raw_ prefix now.

in test/functional/mempool_accept.py:81 in 936f56bb2a outdated

90-        raw_tx_0 = node.signrawtransactionwithwallet(node.createrawtransaction(
91-            inputs=[{"txid": txid_in_block, "vout": 0, "sequence": BIP125_SEQUENCE_NUMBER}],  # RBF is used later
92-            outputs=[{node.getnewaddress(): Decimal('0.3') - fee}],
93-        ))['hex']
94-        tx = tx_from_hex(raw_tx_0)
95+        fee = Decimal("0.003")  # The default fee_rate in MiniWallet

theStack commented at 3:45 pm on July 21, 2021:

Naming: as this is not an absolute fee, I’d suggest calling the variable fee_rate instead.

sriramdvt commented at 6:22 pm on July 21, 2021:

Changed fee to fee_rate, thank you!

in test/functional/mempool_accept.py:84 in 936f56bb2a outdated

93-        ))['hex']
94-        tx = tx_from_hex(raw_tx_0)
95+        fee = Decimal("0.003")  # The default fee_rate in MiniWallet
96+        coin = miniwallet.get_utxo()  # Get a utxo and store it for later
97+        raw_tx_0 = miniwallet.create_self_transfer(fee_rate=fee, from_node=node, utxo_to_spend=coin, sequence=BIP125_SEQUENCE_NUMBER) # RBF is used later
98+        tx = tx_from_hex(raw_tx_0['hex'])

theStack commented at 3:51 pm on July 21, 2021:

Calling the tx_from_hex helper is not needed here (and in many more similar instances below), since the {create,send}self_transfer() method already provides the CTransaction instance in the result via dict field tx:

0        tx = raw_tx_0['tx']

sriramdvt commented at 6:26 pm on July 21, 2021:

tx_from_hex is now used only when the previous transaction from {create,send}_self_transfer() has been modified, or when the test needs to construct a transaction from the raw transaction from node.getrawtransaction().

in test/functional/mempool_accept.py:85 in 936f56bb2a outdated

94-        tx = tx_from_hex(raw_tx_0)
95+        fee = Decimal("0.003")  # The default fee_rate in MiniWallet
96+        coin = miniwallet.get_utxo()  # Get a utxo and store it for later
97+        raw_tx_0 = miniwallet.create_self_transfer(fee_rate=fee, from_node=node, utxo_to_spend=coin, sequence=BIP125_SEQUENCE_NUMBER) # RBF is used later
98+        tx = tx_from_hex(raw_tx_0['hex'])
99         txid_0 = tx.rehash()

theStack commented at 4:00 pm on July 21, 2021:

nit: I think the txid can simpy be obtained by accessing the dict field txid of the self-transfer method’s result (rehashing shouldn’t be needed here).

sriramdvt commented at 6:29 pm on July 21, 2021:

Removed multiple instances where rehashing the transaction was not required. tx.rehash() is now used only when a CTransaction() object is changed.

in test/functional/mempool_accept.py:92 in 936f56bb2a outdated

112-            outputs=[{node.getnewaddress(): output_amount}],
113-            locktime=node.getblockcount() + 2000,  # Can be anything
114-        ))['hex']
115-        tx = tx_from_hex(raw_tx_final)
116-        fee_expected = coin['amount'] - output_amount
117+        raw_tx_final = miniwallet.create_self_transfer(from_node=node, sequence=0xffffffff, locktime=node.getblockcount()+2000) # SEQUENCE_FINAL, locktime can be anything

theStack commented at 4:04 pm on July 21, 2021:

stylistic nit, to better match comments to parameters:

0        raw_tx_final = miniwallet.create_self_transfer(
1            from_node=node,
2            sequence=0xffffffff,  # SEQUENCE_FINAL
3            locktime=node.getblockcount()+2000),  # locktime can be anything
4        )

sriramdvt commented at 6:32 pm on July 21, 2021:

This was indeed how the first commit (aa068ad397cfe5766dccfd45c0f4ba846e7abd22) had it, but I think I missed it in later force pushes. Thank you!

theStack commented at 4:19 pm on July 21, 2021: member

Left some review comments below, most concerning variable naming and stylistic improvements. Probably the changes in MiniWallet (comment fix in scan_blocks, returning fee in create_self_transfer) deserve an own commit ahead of the changes in mempool_accept.py.

sriramdvt force-pushed on Jul 21, 2021

practicalswift commented at 7:54 pm on July 24, 2021: contributor

Concept ACK

More MiniWallet is better.

in test/functional/mempool_accept.py:146 in 7cd06b3c9b outdated

154-        txid_spend_both = node.sendrawtransaction(hexstring=raw_tx_spend_both, maxfeerate=0)
155+        node.sendrawtransaction(hexstring=tx_0['hex'], maxfeerate=0) # spend the tx_0
156+        self.mempool_size += 1
157+        tx_1 = miniwallet.send_self_transfer(fee_rate=4*fee_rate,from_node=node)
158+        utxo_1 = miniwallet.get_utxo()
159+        miniwallet.send_self_transfer(from_node=node, utxo_to_spend=utxo_1)['txid']

MarcoFalke commented at 11:30 am on July 30, 2021:

0        miniwallet.send_self_transfer(from_node=node)['txid']

should be identical

MarcoFalke commented at 11:32 am on July 30, 2021:

Also, why the ['txid'] in the end?

sriramdvt commented at 8:30 am on September 1, 2021:

Removed the unnecessary ['txid']. The suggestion is not identical, since miniwallet.send_self_transfer sorts the utxo set by the value, before choosing a transaction.

in test/functional/mempool_accept.py:163 in 7cd06b3c9b outdated

179-            inputs=[{'txid': txid_spend_both, 'vout': 0}],
180-            outputs=[{node.getnewaddress(): 0.05}],
181-        ))['hex']
182-        tx = tx_from_hex(raw_tx_reference)
183+        utxo_2 = miniwallet.get_utxo()
184+        tx_reference = miniwallet.create_self_transfer(from_node=node, utxo_to_spend=utxo_2)

MarcoFalke commented at 11:31 am on July 30, 2021:

same

in test/functional/mempool_accept.py:175 in 7cd06b3c9b outdated

193 
194         self.log.info('A transaction with no outputs')
195-        tx = tx_from_hex(raw_tx_reference)
196+        tx = tx_from_hex(tx_reference['hex'])
197         tx.vout = []
198         # Skip re-signing the transaction for context independent checks from now on

MarcoFalke commented at 11:37 am on July 30, 2021:

Can also remove this line, because there are no signatures

sriramdvt commented at 8:36 am on September 1, 2021:

tx.vout = [] is necessary since the transaction is serialized in check_mempool_result. Without this, the transaction does not get rejected for the right reasons. Please let me know if I misunderstood the suggestion.

MarcoFalke commented at 2:36 pm on September 1, 2021:

I meant the comment line :sweat_smile:

in test/functional/mempool_accept.py:171 in 7cd06b3c9b outdated

191             maxfeerate=0,
192         )
193 
194         self.log.info('A transaction with no outputs')
195-        tx = tx_from_hex(raw_tx_reference)
196+        tx = tx_from_hex(tx_reference['hex'])

MarcoFalke commented at 11:39 am on July 30, 2021:

wouldn’t the diff be smaller if the raw_tx_reference symbol was kept?

sriramdvt commented at 8:32 am on September 1, 2021:

Agreed! Added a raw_tx_reference symbol and the diff is much smaller now.

MarcoFalke commented at 11:40 am on July 30, 2021: member

Left some comments

DrahtBot added the label Needs rebase on Aug 2, 2021

Minor changes to MiniWallet dbf283a584

Run mempool_accept.py with MiniWallet

Replaced all wallet functionality in `test/functional/mempool_accept.py` to use `test/functional/test_framework/wallet.py` instead of the wallet built with Bitcoin Core.

Co-authored-by: Sishir Giri <sishirg27@gmail.com>
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>

69dda69bf6

sriramdvt force-pushed on Sep 1, 2021

sriramdvt commented at 8:37 am on September 1, 2021: contributor

Rebased and updated with the suggestions

DrahtBot removed the label Needs rebase on Sep 1, 2021

DrahtBot added the label Needs rebase on Sep 9, 2021

DrahtBot commented at 1:05 pm on September 9, 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”.

laanwj commented at 7:04 pm on December 17, 2021: member

Unfortunately this needs significant changes due to changes to the affected tests since. Are you still planning to address this?

sriramdvt marked this as a draft on Dec 17, 2021

sriramdvt commented at 7:27 pm on December 17, 2021: contributor

@laanwj Thank you for the reminder! I’ve edited it to be a draft PR in the meanwhile, I will address the changes in a few days and change the status.

MarcoFalke added the label Up for grabs on Mar 15, 2022

theStack commented at 3:27 pm on March 23, 2022: member

mempool_accept.py is already using MiniWallet on master, i.e. this PR should be closed.

fanquake closed this on Mar 23, 2022

fanquake removed the label Up for grabs on Mar 23, 2022

fanquake removed the label Needs rebase on Mar 23, 2022

MarcoFalke commented at 3:34 pm on March 23, 2022: member

Right, see commit 807169e10b4a18324356ed6ee4d69587b96a7c70

DrahtBot locked this on Mar 23, 2023

test: Use MiniWallet in mempool_accept.py #22509

Conflicts