test: add `rescan utxos` inside MiniWallet's initialization #26886

kouloumos · 2023-01-13T11:21:24Z

When a pre-mined blockchain is used (default behavior), it contains coinbase outputs in blocks 76-10 to the MiniWallet's default address. That's why we always* rescan_utxos() after initializing the MiniWallet, in order for the MiniWallet to account for those mature UTXOs.

The tests following this usage pattern can be seen with:
git grep -n "MiniWallet(" $(git grep -le "rescan_utxos()" $(git grep -Le "self.setup_clean_chain = True"))

This PR adds rescan_utxos() inside MiniWallet's initialization to simplify usage when the MiniWallet is used with a pre-mined chain.

secondary changes

*There are a few tests that use the pre-mined blockchain but do not rescan_utxos(), they instead generate new blocks to create mature UTXOs.

Those were written before the rescan_utxos() method was introduced with p2p: Rename fBlocksOnly, Add test #22955 (fac66d0) and can be seen with:
git grep -n "MiniWallet(" $(git grep -Le "rescan_utxos()" $(git grep -Le "self.setup_clean_chain = True"))

After including rescan_utxos() inside MiniWallets initilization, this blocks generation logic is not needed as the MiniWallet already accounts for enough mature UTXOs to perform the tests. Therefore the now redundant blocks generation logic is removed from those tests with the second commit.

The rest of the MiniWallet tests use a clean chain (self.setup_clean_chain = True) and can be seen with
git grep -n "MiniWallet(" $(git grep -le "self.setup_clean_chain = True")

From those, there are a few that start from a clean chain and then create enough mature UTXOs for the MiniWallet with this kind of logic:

bitcoin/test/functional/mempool_expiry.py

Lines 36 to 40 in 07c54de

    
           self.wallet = MiniWallet(node) 
        
           # Add enough mature utxos to the wallet so that all txs spend confirmed coins. 
        
           self.generate(self.wallet, 4) 
        
           self.generate(node, COINBASE_MATURITY)

Those tests are simplified in the third commit to instead utilize the mature UTXOs of the pre-mined chain.

DrahtBot · 2023-01-13T11:21:27Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theStack, MarcoFalke

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

Conflicts

No conflicts as of last run.

theStack

Concept ACK

Nice idea, seems reasonable to automatically scan for relevant UTXOs on MiniWallet creation if that's what we need to do most of the times anyways. I was thinking if it's worth it to add an option to the constructor to prevent that (so that users could e.g. pass rescan=False), but on the other hand a single additional scantxoutset RPC should never be problematic at all, especially since the UTXO set would be likely empty in those cases anyways. Will do a detailled review tomorrow.

kouloumos · 2023-01-15T17:51:57Z

I was thinking if it's worth it to add an option to the constructor to prevent that (so that users could e.g. pass rescan=False), but on the other hand a single additional scantxoutset RPC should never be problematic at all, especially since the UTXO set would be likely empty in those cases anyways.

Yes, the UTXO set will be empty in those cases (when a clean chain is used) as at all times (except this one) the MiniWallet is initialized at the start of the test.

Also, just a note that since #25445(fa13375) there is an additional scantxoutset RPC every time the MiniWallet is involved in block generation.

maflcko · 2023-01-16T09:41:51Z

Needs rebase after ac4c79a ?

kouloumos · 2023-01-16T09:54:30Z

Rebased after ac4c79a and force-pushed to also remove the now redundant rescan_utxos() call from p2p_permissions.py

kouloumos · 2023-01-16T10:53:29Z

Rebased after d6fc1d6 and forced-pushed to remove the now redundant rescan_utxos() call from mempool_dust.py

theStack

ACK cc50ef8 🧹

Nice cleanup!

maflcko

Needs rebase cc50ef8 🚦

Show signature

Signature:

this simplifies usage when MiniWallet is used with a pre-mined chain.

those tests already have enough mature utxos from the pre-mined chain.

kouloumos · 2023-01-16T17:20:09Z

Rebased to resolve merge conflicts after d0a909a and forced-pushed to remove the now redundant blocks generation logic from feature_bip68_sequence.py

theStack

re-ACK 6bd098a

maflcko · 2023-01-17T08:37:57Z

ACK 6bd098a 🕷

Show signature

Signature:

…aliza…

DrahtBot added the Tests label Jan 13, 2023

kouloumos force-pushed the miniwallet-rescan_utxos branch 2 times, most recently from a4244eb to 9a39a9b Compare January 13, 2023 17:53

DrahtBot mentioned this pull request Jan 13, 2023

test: Run feature_bip68_sequence.py with MiniWallet #26657

Merged

theStack reviewed Jan 15, 2023

View reviewed changes

kouloumos force-pushed the miniwallet-rescan_utxos branch from 9a39a9b to 43ba876 Compare January 16, 2023 08:54

kouloumos force-pushed the miniwallet-rescan_utxos branch 2 times, most recently from 9f1cc01 to e565d46 Compare January 16, 2023 09:51

kouloumos force-pushed the miniwallet-rescan_utxos branch 2 times, most recently from dfccf1d to cc50ef8 Compare January 16, 2023 10:50

theStack approved these changes Jan 16, 2023

View reviewed changes

maflcko approved these changes Jan 16, 2023

View reviewed changes

test: add rescan_utxos in MiniWallet's initialization

0377d6b

this simplifies usage when MiniWallet is used with a pre-mined chain.

kouloumos force-pushed the miniwallet-rescan_utxos branch from cc50ef8 to 68eefb4 Compare January 16, 2023 17:10

kouloumos added 2 commits January 16, 2023 19:14

test: remove redundant blocks generation logic

42029a7

those tests already have enough mature utxos from the pre-mined chain.

test: simplify tests by using the pre-mined chain

6bd098a

kouloumos force-pushed the miniwallet-rescan_utxos branch from 68eefb4 to 6bd098a Compare January 16, 2023 17:14

theStack approved these changes Jan 16, 2023

View reviewed changes

maflcko merged commit 8339f3c into bitcoin:master Jan 17, 2023

kouloumos deleted the miniwallet-rescan_utxos branch January 17, 2023 08:40

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 17, 2023

Merge bitcoin#26886: test: add rescan utxos inside MiniWallet's initi…

5a207d2

…aliza…

bitcoin locked and limited conversation to collaborators Jan 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: add `rescan utxos` inside MiniWallet's initialization #26886

test: add `rescan utxos` inside MiniWallet's initialization #26886

Uh oh!

kouloumos commented Jan 13, 2023

Uh oh!

DrahtBot commented Jan 13, 2023 •

edited

Loading

Uh oh!

theStack left a comment

Uh oh!

kouloumos commented Jan 15, 2023

Uh oh!

maflcko commented Jan 16, 2023

Uh oh!

kouloumos commented Jan 16, 2023

Uh oh!

kouloumos commented Jan 16, 2023

Uh oh!

theStack left a comment

Uh oh!

maflcko left a comment

Uh oh!

kouloumos commented Jan 16, 2023

Uh oh!

theStack left a comment

Uh oh!

maflcko commented Jan 17, 2023

Uh oh!

Uh oh!

	self.wallet = MiniWallet(node)

	# Add enough mature utxos to the wallet so that all txs spend confirmed coins.
	self.generate(self.wallet, 4)
	self.generate(node, COINBASE_MATURITY)

test: add rescan utxos inside MiniWallet's initialization #26886

test: add rescan utxos inside MiniWallet's initialization #26886

Uh oh!

Conversation

kouloumos commented Jan 13, 2023

secondary changes

Uh oh!

DrahtBot commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

kouloumos commented Jan 15, 2023

Uh oh!

maflcko commented Jan 16, 2023

Uh oh!

kouloumos commented Jan 16, 2023

Uh oh!

kouloumos commented Jan 16, 2023

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

kouloumos commented Jan 16, 2023

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 17, 2023

Uh oh!

Uh oh!

test: add `rescan utxos` inside MiniWallet's initialization #26886

test: add `rescan utxos` inside MiniWallet's initialization #26886

DrahtBot commented Jan 13, 2023 •

edited

Loading