Use MiniWallet in functional test rpc_signrawtransactionwithkey.

martinsaposnic commented at 9:55 pm on August 22, 2024: contributor

In response to issue #30600, optimizations have been implemented to enhance test efficiency and readability:

This PR refactors the rpc_signrawtransactionwithkey.py functional test to use MiniWallet for creating funding transactions. This simplifies the test code and improves performance by eliminating the need to mine new blocks for each funding transaction.

Key changes:

Replaced custom send_to_address method with MiniWallet’s send_to method
Removed unnecessary setup of a clean chain and second node
Simplified transaction creation and signing process

DrahtBot commented at 9:55 pm on August 22, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ismaelsadeeq, theStack, glozow

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

martinsaposnic force-pushed on Aug 22, 2024

DrahtBot added the label CI failed on Aug 22, 2024

DrahtBot commented at 10:01 pm on August 22, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29138070772

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot removed the label CI failed on Aug 22, 2024

theStack commented at 11:18 pm on August 22, 2024: contributor

Thanks for picking this up!

A few more suggestions to reduce the code even further:

the initialization of previously needed instance members (self.blk_idx, self.block_hash) can now be removed
instead of starting with a clean chain (see self.setup_clean_chain), you could use the pre-generated test chain to avoid the need of generating blocks; MiniWallet will rescan and pick up the relevant UTXOs up automically at instantiation. IIUC it shouldn’t be necessary to mine any blocks for this tests
regarding the P2A test change: MiniWallet’s send_to returns both the txid and the output index, so there is no need to find it manually after; you could e.g. return a tuple (txid,vout) from send_to_address

glozow added the label Refactoring on Aug 23, 2024

martinsaposnic force-pushed on Aug 23, 2024

martinsaposnic commented at 2:39 pm on August 23, 2024: contributor

Out of curiosity I ran a little benchmark

Master:

With MiniWallet:

Miniwallet is cool

martinsaposnic commented at 2:40 pm on August 23, 2024: contributor

@theStack I implemented the 3 suggestions. Let me know if you have further comments 🙂

in test/functional/rpc_signrawtransactionwithkey.py:50 in b9b6e341fc outdated

46@@ -46,16 +47,13 @@
47 
48 class SignRawTransactionWithKeyTest(BitcoinTestFramework):
49     def set_test_params(self):
50-        self.setup_clean_chain = True
51-        self.num_nodes = 2
52+        self.setup_clean_chain = False

theStack commented at 4:47 pm on August 23, 2024:

nit: using the pre-generated chain is the default, so could just remove this line

in test/functional/rpc_signrawtransactionwithkey.py:124 in b9b6e341fc outdated

119@@ -124,7 +120,7 @@ def verify_txn_with_witness_script(self, tx_type):
120         addr = script_to_p2sh(redeem_script)
121         script_pub_key = address_to_scriptpubkey(addr).hex()
122         # Fund that address
123-        txid = self.send_to_address(addr, 10)
124+        [txid, _] = self.send_to_address(addr, 10)
125         vout = find_vout_for_address(self.nodes[0], txid, addr)

theStack commented at 4:48 pm on August 23, 2024:

could use the vout result from send_to_address also here, so the find_vout_for_address helper isn’t needed anymore

in test/functional/rpc_signrawtransactionwithkey.py:125 in b9b6e341fc outdated

119@@ -124,7 +120,7 @@ def verify_txn_with_witness_script(self, tx_type):
120         addr = script_to_p2sh(redeem_script)
121         script_pub_key = address_to_scriptpubkey(addr).hex()
122         # Fund that address
123-        txid = self.send_to_address(addr, 10)
124+        [txid, _] = self.send_to_address(addr, 10)
125         vout = find_vout_for_address(self.nodes[0], txid, addr)
126         self.generate(self.nodes[0], 1)

theStack commented at 4:49 pm on August 23, 2024:

I assume this line can also be removed

theStack commented at 4:54 pm on August 23, 2024: contributor

lgtm, once again some smaller suggestions

martinsaposnic force-pushed on Aug 23, 2024

martinsaposnic commented at 5:29 pm on August 23, 2024: contributor

@theStack thank you for the suggestions! just forcePushed a new commit with the changes.

martinsaposnic force-pushed on Aug 23, 2024

DrahtBot commented at 5:31 pm on August 23, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29178413782

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot added the label CI failed on Aug 23, 2024

martinsaposnic force-pushed on Aug 23, 2024

DrahtBot removed the label CI failed on Aug 23, 2024

theStack approved

theStack commented at 4:57 pm on August 24, 2024: contributor

lgtm ACK 96d34fb2e493a2c37907e76808215c5ab0c3cf70

ismaelsadeeq commented at 5:01 pm on August 26, 2024: member

Concept ACK

I’ll prefer if you split this commit intro three as you highlight in commit message

1- Replace custom funding tx creation with MiniWallet 2- Remove unnecessary clean chain setup and second node 2- Simplify transaction handling

martinsaposnic force-pushed on Aug 26, 2024

martinsaposnic commented at 6:07 pm on August 26, 2024: contributor

@ismaelsadeeq rebased and pushed. thanks for the review!

in test/functional/rpc_signrawtransactionwithkey.py:56 in 780ecd0803 outdated

58-        self.blk_idx += 1
59-        rawtx = self.nodes[0].createrawtransaction([input], output)
60-        txid = self.nodes[0].sendrawtransaction(self.nodes[0].signrawtransactionwithkey(rawtx, [self.nodes[0].get_deterministic_priv_key().key])["hex"], 0)
61-        return txid
62+        script_pub_key = address_to_scriptpubkey(addr)
63+        tx = self.wallet.send_to(from_node=self.nodes[0],scriptPubKey=script_pub_key, amount=int(amount * COIN))

ismaelsadeeq commented at 8:29 am on August 27, 2024:

nit: add spaces between passed arguments

0        tx = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=script_pub_key, amount=int(amount * COIN))

ismaelsadeeq commented at 8:31 am on August 27, 2024:

Why are you converting the amount to an integer here? Won’t that change the assumptions in the test? I see 49.999 explicitly passed in the test—don’t we want to maintain precision?

martinsaposnic commented at 10:09 pm on August 27, 2024:

I noticed that MiniWallet’s send_to method doesn’t play nice with non-integer amounts. It’s failing because CTxOut’s nValue is an int, so when you pass a float, the to_bytes method throws an error.

I did a quick scan of the codebase:

We’ve got 21 uses of MiniWallet’s send_to method on the tests (not counting this PR)

17 of these are already sending an integer
4 are explicitly doing int(amount) before passing it to send_to

I think fixing this is probably outside what we’re trying to do in this PR. I’m happy to create a separate issue for it and tackle it. @ismaelsadeeq, what do you think?

martinsaposnic commented at 3:00 pm on August 28, 2024:

@theStack what do you think?

ismaelsadeeq commented at 3:13 pm on August 28, 2024:

It’s failing because CTxOut’s nValue is an int, so when you pass a float, the to_bytes method throws an error.

I can also verify that this fails locally.

I’m generally not a fan of magic numbers, but the obvious explanation is that some satoshis were subtracted from the UTXO value to account for transaction fees.

If that’s the case, then it’s fine to round it down.

I’m happy to create a separate issue for it and tackle it.

+1

theStack commented at 1:49 pm on August 29, 2024:

Why are you converting the amount to an integer here? Won’t that change the assumptions in the test? I see 49.999 explicitly passed in the test—don’t we want to maintain precision?

Note that no loss of precision is happening here, it’s just a conversion from one unit (BTC, represented with Decimal) to another (Satoshis, repr. with int), as that’s the unit MiniWallet.send_to works with. Before the int cast the BTC decimal value is multiplied with COIN (=10^8, i.e. the amount of sats per BTC), so in the mentioned example we’d convert 49.999 to 4999900000, without any rounding involved.

I think long-term it would be ideal to only ever use Satoshis as the unit to keep things simple, but many RPCs still work with BTC (e.g. the createrawtransaction RPC used in this test, or calls that fetch coin information from the UTXO set), so these conversions are unfortunately necessary sometimes :/

I’m generally not a fan of magic numbers…

Same here, happy to review a follow-up PR getting rid of them (for this as well as other tests).

in test/functional/rpc_signrawtransactionwithkey.py:57 in 780ecd0803 outdated

59-        rawtx = self.nodes[0].createrawtransaction([input], output)
60-        txid = self.nodes[0].sendrawtransaction(self.nodes[0].signrawtransactionwithkey(rawtx, [self.nodes[0].get_deterministic_priv_key().key])["hex"], 0)
61-        return txid
62+        script_pub_key = address_to_scriptpubkey(addr)
63+        tx = self.wallet.send_to(from_node=self.nodes[0],scriptPubKey=script_pub_key, amount=int(amount * COIN))
64+        return [tx["txid"], tx["sent_vout"]]

ismaelsadeeq commented at 8:32 am on August 27, 2024:

It might be better to return a tuple here

0        return tx["txid"], tx["sent_vout"]

in test/functional/rpc_signrawtransactionwithkey.py:4 in 8c64048c44 outdated

3@@ -4,9 +4,6 @@
4 # file COPYING or http://www.opensource.org/licenses/mit-license.php.

ismaelsadeeq commented at 8:36 am on August 27, 2024:

8c64048c443cddce201b6d54184b32f3894ab806 can be squashed together with the first commit

Replace custom funding tx creation with MiniWallet.

setup_clean_chain=True is deleted so it uses the default.
Also, vout is now returned from send_to_address,
so now there is no need to fetch it manually

Also remove not-needed code that was used with the old
transaction handling.

1f4cdb3d69

Remove second node since only 1 is needed for the test a563f41232

martinsaposnic force-pushed on Aug 27, 2024

ismaelsadeeq commented at 3:14 pm on August 28, 2024: member

code review ACK a563f41232e2dd360ee8e76f6348dd10c7f4f2a3

DrahtBot requested review from theStack on Aug 28, 2024

theStack approved

theStack commented at 1:50 pm on August 29, 2024: contributor

ACK a563f41232e2dd360ee8e76f6348dd10c7f4f2a3

glozow commented at 2:30 pm on August 29, 2024: member

ACK a563f41232e2dd360ee8e76f6348dd10c7f4f2a3

glozow merged this on Aug 29, 2024

glozow closed this on Aug 29, 2024

Use MiniWallet in functional test rpc_signrawtransactionwithkey. #30701

Code Coverage

Reviews