Use MiniWallet in functional test rpc_signrawtransactionwithkey. #30701

pull martinsaposnic wants to merge 2 commits into bitcoin:master from martinsaposnic:test-miniwallet-on-rpc-signrawtransactionwithkey changing 1 files +13 −20
  1. 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
  2. DrahtBot commented at 9:55 PM on August 22, 2024: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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.

  3. martinsaposnic force-pushed on Aug 22, 2024
  4. martinsaposnic force-pushed on Aug 22, 2024
  5. DrahtBot added the label CI failed on Aug 22, 2024
  6. DrahtBot commented at 10:01 PM on August 22, 2024: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

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

    <details><summary>Hints</summary>

    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.

    </details>

  7. DrahtBot removed the label CI failed on Aug 22, 2024
  8. 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
  9. glozow added the label Refactoring on Aug 23, 2024
  10. martinsaposnic force-pushed on Aug 23, 2024
  11. martinsaposnic commented at 2:39 PM on August 23, 2024: contributor

    Out of curiosity I ran a little benchmark

    Master:

    <img width="607" alt="image" src="https://github.com/user-attachments/assets/875f96fd-5ef2-4730-a3df-f7b3ff40f039">

    With MiniWallet:

    <img width="903" alt="image" src="https://github.com/user-attachments/assets/40bdcb95-1b08-4054-9223-44566bdca548">

    Miniwallet is cool

  12. 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 🙂

  13. 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

  14. 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

  15. 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

  16. theStack commented at 4:54 PM on August 23, 2024: contributor

    lgtm, once again some smaller suggestions

  17. martinsaposnic force-pushed on Aug 23, 2024
  18. 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.

  19. martinsaposnic force-pushed on Aug 23, 2024
  20. DrahtBot commented at 5:31 PM on August 23, 2024: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

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

    <details><summary>Hints</summary>

    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.

    </details>

  21. DrahtBot added the label CI failed on Aug 23, 2024
  22. martinsaposnic force-pushed on Aug 23, 2024
  23. DrahtBot removed the label CI failed on Aug 23, 2024
  24. theStack approved
  25. theStack commented at 4:57 PM on August 24, 2024: contributor

    lgtm ACK 96d34fb2e493a2c37907e76808215c5ab0c3cf70

  26. 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

  27. martinsaposnic force-pushed on Aug 26, 2024
  28. martinsaposnic force-pushed on Aug 26, 2024
  29. martinsaposnic commented at 6:07 PM on August 26, 2024: contributor

    @ismaelsadeeq rebased and pushed. thanks for the review!

  30. 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

            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).

  31. 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

            return tx["txid"], tx["sent_vout"]
  32. 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

  33. 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
  34. Remove second node since only 1 is needed for the test a563f41232
  35. martinsaposnic force-pushed on Aug 27, 2024
  36. ismaelsadeeq commented at 3:14 PM on August 28, 2024: member

    code review ACK a563f41232e2dd360ee8e76f6348dd10c7f4f2a3

  37. DrahtBot requested review from theStack on Aug 28, 2024
  38. theStack approved
  39. theStack commented at 1:50 PM on August 29, 2024: contributor

    ACK a563f41232e2dd360ee8e76f6348dd10c7f4f2a3

  40. glozow commented at 2:30 PM on August 29, 2024: member

    ACK a563f41232e2dd360ee8e76f6348dd10c7f4f2a3

  41. glozow merged this on Aug 29, 2024
  42. glozow closed this on Aug 29, 2024
  43. bitcoin locked this on Aug 29, 2025
Contributors
martinsaposnicDrahtBottheStackismaelsadeeqglozow
Labels
Refactoring
Linked (view graph)
#30600 use MiniWallet in functional test `rpc_signrawtransactionwithkey.py` for funding txs
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-14 21:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me