Refactor: MiniWallet functionality for more readable code in functional test `rpc_signtransactionwithkey.py` #30667

pull allanperlee wants to merge 4 commits into bitcoin:master from allanperlee:master changing 1 files +7 −6

allanperlee commented at 9:28 PM on August 16, 2024: none

Addressing issue #30600, this pull request simplifies the send_to_address functional test in rpc_signrawtransactionwithkey.py. The code is more terse with a MiniWallet object, its ADDRESS_OP_TRUE mode and send_to method on line 54, thus removing the "low-level" details of the transaction. Suggestions will be addressed. Thank you for reviewing!
Simplified the functional for send_to_address by importing MiniWallet and using in this function; will consider further modifications upon review 861683f8cb
Merge branch 'master' of https://github.com/allanperlee/bitcoin 79a138856c
DrahtBot commented at 9:28 PM on August 16, 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

Concept NACK ismaelsadeeq

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label CI failed on Aug 16, 2024
DrahtBot commented at 10:40 PM on August 16, 2024: contributor


🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/28879496602</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>
ismaelsadeeq commented at 9:13 AM on August 19, 2024: member

Thank you for attempting to address this issue.

The changes in this PR are incorrect and do not address the issue. Therefore, I am inclined to approach NACK. If you have any questions about this PR, please ask them in the issue and review the guidelines at CONTRIBUTING.md before opening a new PR.

Hint to anyone wanting to work on this. check MiniWallet::send_to method. https://github.com/bitcoin/bitcoin/blob/ee367170cb2acf82b6ff8e0ccdbc1cce09730662/test/functional/test_framework/wallet.py#L273-L294

And convert the address in SignRawTransactionWithKeyTest::send_to_address to a scriptPubKey before passing it to MiniWallet::send_to method.
Merge branch 'bitcoin:master' into master d133ccb3e3
Addressing the comments on the first attempted pull request, this change converts the address to a script public key per the send_to MiniWallet method's requirements. 3f4b1d7e4f
allanperlee commented at 8:28 PM on August 19, 2024: none

@ismaelsadeeq Thanks for reviewing the PR. I corrected the mistake. I really want to get this right, I really appreciate the suggestions.
maflcko commented at 6:50 AM on August 20, 2024: member

You'll have to run the test locally, after you modify it and before you push the code to a pull request. There is a contributing guide for developers and how to run the tests.
in test/functional/rpc_signrawtransactionwithkey.py:56 in 3f4b1d7e4f
```
  57 | -        output = {addr: amount}
  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 | +        script_pub_key = address_to_scriptpubkey(addr).hex()
  62 | +        wallet = MiniWallet(self.node[0], mode=MiniWalletMode.ADDRESS_OP_TRUE)
```
glozow commented at 2:00 PM on August 20, 2024:

The point of this test is to check that signrawtransactionwithkey works. Replacing the call to signrawtransactionwithkey with a different signing function seems wrong.

allanperlee commented at 8:05 PM on August 21, 2024:

I see your point, the issue was readability in this particular test, according to the issuer. Should I leave this alone and write a separate test? signrawtransactionwithkey is tested in other parts of this functional test.

glozow commented at 1:09 PM on August 22, 2024:

Ah apologies, I was too hasty and didn't notice that this is for the funding transactions. You can ignore that.
glozow commented at 2:37 PM on August 20, 2024: member

Thanks for your interest in contributing. I recommend reading the contributing guidelines and developer notes, which contain helpful tips for getting started, squashing commits and rebasing, motivation for refactoring, etc.
allanperlee renamed this:
~~MiniWallet funcitonality for more readable code in functional test `rpc_signtransactionwithkey.py`~~
Refactor: MiniWallet functionality for more readable code in functional test `rpc_signtransactionwithkey.py`
on Aug 21, 2024
glozow commented at 1:23 PM on August 22, 2024: member
I'm going to close this pull request for now. If you'd like to work on the issue, here are some hints before you open a new pull request.
1. Please ensure your code works before submitting a pull request. There is a lot of documentation for the functional test suite if you need help https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md
2. Write code a bit more carefully. node isn't an attribute; you forgot an s. MiniWallet does not have a sendrawtransaction function; you perhaps confused that with send_to_address. The new wallet also doesn't have any utxos to spend; it should probably be instantiated at the start of the test and be reused.
3. Maybe create a branch instead of PRing from your master https://git-scm.com/book/en/v2/Git-Branching-Branches-in-a-Nutshell
4. Squash your commits https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
glozow closed this on Aug 22, 2024
bitcoin locked this on Aug 22, 2025

Type	Reviewers
Concept NACK	ismaelsadeeq

Contributors

Labels

Linked (view graph)

#30600 use MiniWallet in functional test `rpc_signrawtransactionwithkey.py` for funding txs

Refactor: MiniWallet functionality for more readable code in functional test `rpc_signtransactionwithkey.py` #30667

Code Coverage

Reviews