-
Notifications
You must be signed in to change notification settings - Fork 36.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: Fix rpc_scantxoutset intermittent issue #23858
Conversation
Can be reviewed with --word-diff-regex=. --ignore-all-space
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK fafe4de
I couldn't reproduce the bug but tested the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-review ACK fafe4de
Longer term it would be nice to remove the wallet and use MiniWallet here.
Opened a PR #23866, with the PEP8 changes of this PR cherry-picked. It can be seen as an alternative, happy to rebase though if this gets in first (which could make sense considering that it is an obvious immediate fix to an issue and reviewing the other likely takes longer than this one).
Going to merge this to fix the CI and to make a bugfix backport easier (if needed) |
…t.py e844115 test: use MiniWallet for rpc_scantxoutset.py (Sebastian Falbesoner) 983ca04 test: introduce `address_to_scriptpubkey` helper (Sebastian Falbesoner) e704d4d test: introduce `getnewdestination` helper for generating various address types (Sebastian Falbesoner) Pull request description: This PR enables one more of the non-wallet functional tests (rpc_scantxoutset.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078 and bitcoin/bitcoin#23858 (comment) more recently. Reviewer's guide: * [commit 1/3] For replacing the getnewaddress/getaddressinfo RPC calls a helper `getnewdestination` is introduced which allows to create addresses with the common address format types ('legacy', 'p2sh-segwit' and 'bech32'), but also additionally returns the corresponding pubkey and output script. * [commit 2/3] In order to send to addresses with MiniWallet, a helper `address_to_scriptpubkey` is introduced. It only supports legacy addresses (Base58Check) so far, which is sufficient for the scantxoutset test. * [commit 3/3] With those helpers, the use of MiniWallet in the test is quite straight-forward. To avoid repeatedly specifying parameters like `from_node` to MiniWallet's `send_to` method, another test-internal helper `sendtodestination` is introduced which supports specifying the destination both as outputscript or as address. ACKs for top commit: w0xlt: reACK [e844115](bitcoin/bitcoin@e844115) Tree-SHA512: e4823dc507019b2b8e479602963c5dddc4c78211e1d934218ee0f0e32c068ab7e44e9793307f549127946364f57d684c4ea1d70f25865ea70d30d4f3855836d0
I fail to see how this could have ever worked, since there is nothing that prevents the wallet from spending the coins in the utxo set.
Fixes #23847
Longer term it would be nice to remove the wallet and use MiniWallet here.