Added a new test in test/functional/rpc_blockchain.py to validate the gettxout RPC response. This new test ensures all response elements are verified, including bestblock, confirmations, value, coinbase, and scriptPubKey details.
Also renamed the method get_scriptPubKey from test/functional/test_framework/wallet.py to the modern name get_output_script as suggested by maflcko (https://github.com/bitcoin/bitcoin/pull/30226#discussion_r1925491846)
Why not just expanding the tests in wallet_basic.py?
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30226.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
Why not just expanding the tests in
wallet_basic.py?
I initially added the test to rpc_blockchain.py because that file focuses on testing RPC commands from src/rpc/blockchain.cpp, and "gettxout" is one of those commands. However, I'm open to expanding the tests in wallet_basic.py if that is preferred.
406 | + txout = node.gettxout(txid, 0) 407 | + 408 | + # Validate the gettxout response 409 | + assert_equal(txout['bestblock'], best_block_hash) 410 | + assert_equal(txout['confirmations'], 1) 411 | + assert_equal(txout['value'], create_coinbase(HEIGHT).vout[0].nValue / 100000000)
Should we use COIN here instead of 100000000
found in bitcoin/test/functional/test_framework/messages.py
Done. Thanks
389 | @@ -388,6 +390,33 @@ def _test_gettxoutsetinfo(self): 390 | # Unknown hash_type raises an error 391 | assert_raises_rpc_error(-8, "'foo hash' is not a valid hash_type", node.gettxoutsetinfo, "foo hash") 392 | 393 | + def _test_gettxout(self): 394 | + self.log.info("Test gettxout")
This logs seems a bit vague, a more descriptive log would be useful
Yeah, I agree. I was following the same log structure used by other tests in this file. I changed it to "Validating gettxout RPC response". Let me know if that's better. Thanks
Hey @maflcko, sorry for tagging you, but I would appreciate some additional feedback on this PR.
I believe rpc_blockchain.py is a suitable place to test the "gettxout" RPC call/response, as it already handles similar RPC commands like "gettxoutsetinfo." However, brunoerg suggested moving this test to wallet_basic.py.
I would value your input on this. I'm open to moving the test if that's the more appropriate approach.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/26357289945</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>
I believe
rpc_blockchain.pyis a suitable place to test the "gettxout" RPC call/response, as it already handles similar RPC commands like "gettxoutsetinfo." However, brunoerg suggested moving this test towallet_basic.py.
I don't think wallet_*.py makes sense, because gettxout is not a wallet RPC. This would mean the test doesn't run at all unless a wallet is compiled in. The current place seems fine.
However, the CI fails.
I don't think
wallet_*.pymakes sense, becausegettxoutis not a wallet RPC. This would mean the test doesn't run at all unless a wallet is compiled in. The current place seems fine.However, the CI fails.
Thanks. Yes I just noticed the failure on the lint job. I will check it and push the fix.
You'll probably have to rebase, as it may be a silent conflict.
You'll probably have to rebase, as it may be a silent conflict.
Yes, that was it. I've rebased and resolved the conflict. Thanks for your help!
lgtm ACK 11b90e4319319db702d602a68d22c87054781582
411 | + assert_equal(txout['bestblock'], best_block_hash) 412 | + assert_equal(txout['confirmations'], 1) 413 | + assert_equal(txout['value'], create_coinbase(HEIGHT).vout[0].nValue / COIN) 414 | + assert_equal(txout['scriptPubKey']['address'], self.wallet.get_address()) 415 | + assert_equal(txout['scriptPubKey']['hex'], self.wallet.get_scriptPubKey().hex()) 416 | + decoded_script = node.decodescript(self.wallet.get_scriptPubKey().hex())
decoded_script = node.decodescript(self.wallet.get_output_script().hex())
unrelated, but the method can be renamed to use the modern name
I tried the suggested change, but encountered
decoded_script = node.decodescript(self.wallet.get_output_script().hex())
AttributeError: 'MiniWallet' object has no attribute 'get_output_script'
I synced my fork and rebased master in my local branch, but the implementation for get_output_script doesn't seem to be present. Am I missing something?
I wanted to suggest to rename the method wholesale, as it isn't used widely and this pull is adding two new calls.
Ok, I understand now. I can make this change in the current PR while also addressing fjahr's feedback. Unless you think it's not worth invalidating the two ACKs, but it might be a good time to do it.
Done in 723440c5b8eb3a815c80bfb37ad195b5448b25ed
408 | + txout = node.gettxout(txid, 0) 409 | + 410 | + # Validate the gettxout response 411 | + assert_equal(txout['bestblock'], best_block_hash) 412 | + assert_equal(txout['confirmations'], 1) 413 | + assert_equal(txout['value'], create_coinbase(HEIGHT).vout[0].nValue / COIN)
nit: I find this a bit weird to use create_coinbase here for this. It's not a big deal but just using a fixed value would have been fine and a bit simpler IMO.
Done, thanks!
Code review ACK 11b90e4319319db702d602a68d22c87054781582
reACK 723440c5b8eb3a815c80bfb37ad195b5448b25ed
Just FYI, for changes of the test framework just using the prefix of test is fine.
lgtm ACK 723440c5b8eb3a815c80bfb37ad195b5448b25ed
code review ACK 723440c5b8eb3a815c80bfb37ad195b5448b25ed