test: add assumeutxo wallet test #28838

Sjors · 2023-11-10T01:15:37Z

Extracted from #28616, this adds a (very) basic wallet test for assume utxo. It checks some circumstances where a backup can and can't be loaded.

DrahtBot · 2023-11-10T01:15:40Z

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
ACK	theStack, maflcko, achow101
Concept ACK	BrandonOdiwuor
Stale ACK	pablomartin4btc

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28616 (Show transactions as not fully confirmed during background validation by Sjors)

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.

maflcko · 2023-11-10T08:50:23Z

test/functional/wallet_assumeutxo.py

+        for n in self.nodes:
+            assert_equal(n.getblockchaininfo()["headers"], SNAPSHOT_BASE_HEIGHT)
+
+        w.backupwallet("/tmp/abc.dat")


My diff was a hack, obviously. I don't think it makes sense to escape the datadir "sandbox" and potentially overwrite user data when running a test.

maflcko · 2023-11-10T08:51:47Z

test/functional/wallet_assumeutxo.py

+
+## Possible test improvements
+
+- TODO: the "assumed" field for a confirmed wallet transaction should be


Seems odd to add a TODO to add a test based on a feature that doesn't even exist.

maflcko · 2023-11-10T08:52:37Z

test/functional/wallet_assumeutxo.py

+
+        self.sync_blocks()
+
+        n0.createwallet('test',blank=True, disable_private_keys=True, descriptors=True)


nit: blank may not be needed? Also, could run a python formatter on this file to fix the whitespace?

Might indeed just give the wallet some keys, so later tests can have transactions and rescans.

Which formatter? Ah PEP-8 https://github.com/bitcoin/bitcoin/tree/master/test/functional#style-guidelines

test/functional/wallet_assumeutxo.py

Sjors · 2023-11-13T06:43:51Z

Pushed some cleanups.

maflcko · 2023-11-13T08:47:09Z

test/functional/wallet_assumeutxo.py

Could add a comment why this is needed?

It's not. Replaced with a TODO comment to add wallet test for a pruned node.

It doesn't seem like this was pushed?

Sjors · 2023-11-13T11:13:43Z

test/functional/wallet_assumeutxo.py

Also dropping this import since the wallet is no longer watch-only. I added a TODO instead to test scenarios like when you import a descriptor with existing transactions while background sync is in progress.

It doesn't seem like this was pushed?

Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^[email protected]>

pablomartin4btc

ACK f4f3a0c910a148a158e1879f8c9e6767575539db

@Sjors, I think you need to fix @maflcko's email on the co-authoring section in the commit body.

maflcko · 2023-11-27T09:32:21Z

lgtm ACK f4f3a0c910a148a158e1879f8c9e6767575539db

Sjors · 2023-11-28T15:24:10Z

@achow101 good catch, I indeed forgot to push.

@pablomartin4btc I think he likes it this way

BrandonOdiwuor

Concept ACK

theStack · 2024-01-10T12:43:40Z

Concept ACK

Will review in a bit. As a follow-up idea, maybe we could put the AssumeUTXO chain and snapshot creation in a shared module, in order to deduplicate code between feature_assumeutxo.py and wallet_assumeutxo.py? If we ever wanted to change the snapshot chainstate for e.g. more advanced tests, it'd be quite annoying having to touch several files.

…fter loading 9315754 test: assumeutxo: spend coin from snapshot chainstate after loading (Sebastian Falbesoner) Pull request description: This PR extends the AssumeUTXO functional test by submitting a spending transaction for an UTXO that is only available in a the snapshot chainstate (after loading via `loadtxoutset`), i.e. it hasn't been seen in a block before. With that we can verify that snapshot coins are visible to the mempool. Note that we unfortunately can't use MiniWallet here, as the only available UTXO to spend from the snapshot chainstate is at height 200, where a P2PKH created from the test framework's deterministic private key is used (see `TestNode.generate(...)` and the `PRIV_KEYS` array). Coinbase outputs with smaller heights (<= 199) would be part of the pre-generated chain and hence not qualify for the "UTXO is only in snapshot chainstate and has never been seen in a block" scenario, coinbase outputs with larger heights (>= 201) can't be spent due to immaturity, as the snapshot chainstate block height is 299. One could of course mine a different chain with outputs that MiniWallet supports (e.g. taproot anyone-can-spend), but this would change the hardcoded AssumeUTXO hash, colliding with other PRs like #28838, so I wanted to avoid that. ACKs for top commit: maflcko: lgtm ACK 9315754 jamesob: ACK 9315754 Tree-SHA512: 0665868e1e91fe74f408d0a239cc264bbbc11a6b55bcc0e86cc8b4b2ec1f44977884b817dbe9065a7c768332cab464636656858bc8b9c8e7d7810498e0a17d78

theStack

Code-review ACK 997b9a7

Also ran the test locally.

theStack · 2024-01-10T17:49:29Z

test/functional/wallet_assumeutxo.py

+            assert_equal(n.getblockchaininfo()[
+                         "headers"], SNAPSHOT_BASE_HEIGHT)


nit: looks like an unintended newline here?

Suggested change

assert_equal(n.getblockchaininfo()[

"headers"], SNAPSHOT_BASE_HEIGHT)

assert_equal(n.getblockchaininfo()["headers"], SNAPSHOT_BASE_HEIGHT)

maflcko · 2024-01-11T10:16:03Z

lgtm ACK 997b9a7

achow101 · 2024-01-11T16:23:48Z

ACK 997b9a7

jamesob · 2024-01-11T16:31:34Z

Posthumous ACK 997b9a7

Was just in the middle of reviewing this and figuring out how to add some balance checks. May file a followup.

maflcko · 2024-01-11T16:58:49Z

https://github.com/bitcoin/bitcoin/actions/runs/7491713362/job/20393561752?pr=29218#step:27:7881

                                   test_framework.authproxy.JSONRPCException: remove_all: The process cannot access the file because it is being used by another process.: "D:\a\_temp\test_runner_?_??_20240111_163825\wallet_assumeutxo_47\node1\regtest\w" (-1)

Sjors · 2024-01-12T18:10:45Z

test/functional/wallet_assumeutxo.py

+            assert_equal(n.getblockchaininfo()[
+                         "headers"], SNAPSHOT_BASE_HEIGHT)
+
+        w.backupwallet("backup_w.dat")


@jamesob I think you can just close the wallet after this call?

From reading the test again (it's a been a while) I don't think we care about this loaded wallet w anymore.

I don't think the problem is that the original wallet is open. The error is the path for the restored wallet on node1, not the original on node0.

I see (just saw the log in #29234).

Sounds like the node that made the backup holds on to it after writing? Maybe restart the node (n0)? Or copy the backup file?

Sounds like the node that made the backup holds on to it after writing? Maybe restart the node? Or copy the backup file?

We don't remove the backup. The failure is in cleaning up the restored wallet after it fails to restore. The backup file is copied to a new path, and that is what remove_all is trying to remove.

I think it's more likely that the database isn't fully closed yet even though the CWallet should be destructed.

Ok, so it has nothing to do with what n0 did. I'm surprised none of the other wallet backup & restore tests have this issue. But I guess it's a different failure condition.

It looks like we don't have any other tests that have a failure in CWallet::Create, which I don't think is that surprising since generally the wallet needs to be corrupted for that to happen.

I think it's more likely that the database isn't fully closed yet even though the CWallet should be destructed.

Maybe it's because of -unsafesqlitesync, going to try turning that off for the test.

I wonder why it's called unsafe :-)

DrahtBot added the Tests label Nov 10, 2023

maflcko reviewed Nov 10, 2023

View reviewed changes

brunoerg reviewed Nov 10, 2023

View reviewed changes

test/functional/wallet_assumeutxo.py Outdated Show resolved Hide resolved

Sjors force-pushed the 2023/11/test-wallet-assume branch from 0b81433 to f4f3a0c Compare November 13, 2023 06:43

maflcko reviewed Nov 13, 2023

View reviewed changes

DrahtBot mentioned this pull request Nov 13, 2023

Show transactions as not fully confirmed during background validation #28616

Closed

Sjors commented Nov 13, 2023

View reviewed changes

test: add assumeutxo wallet test

997b9a7

Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^[email protected]>

pablomartin4btc reviewed Nov 25, 2023

View reviewed changes

Sjors force-pushed the 2023/11/test-wallet-assume branch from f4f3a0c to 997b9a7 Compare November 28, 2023 15:23

BrandonOdiwuor reviewed Dec 29, 2023

View reviewed changes

theStack mentioned this pull request Jan 10, 2024

test: assumeutxo: spend coin from snapshot chainstate after loading #29215

Merged

theStack approved these changes Jan 10, 2024

View reviewed changes

DrahtBot requested review from BrandonOdiwuor, maflcko and pablomartin4btc and removed request for BrandonOdiwuor January 10, 2024 17:51

DrahtBot requested review from BrandonOdiwuor and removed request for BrandonOdiwuor and maflcko January 11, 2024 10:16

DrahtBot removed the request for review from BrandonOdiwuor January 11, 2024 16:23

DrahtBot requested a review from BrandonOdiwuor January 11, 2024 16:23

achow101 merged commit 4e104e2 into bitcoin:master Jan 11, 2024

fanquake mentioned this pull request Jan 11, 2024

ci: failure in wallet_assumeutxo.py --descriptors #29234

Closed

jamesob mentioned this pull request Jan 11, 2024

test: unbreak: exclude windows from wallet_assumeutxo #29238

Closed

Sjors deleted the 2023/11/test-wallet-assume branch January 12, 2024 18:04

Sjors commented Jan 12, 2024

View reviewed changes

murchandamus mentioned this pull request Jan 13, 2024

wallet: Reset chain notifications handler if AttachChain fails #29243

Merged

bitcoin locked and limited conversation to collaborators Jan 11, 2025


		## Possible test improvements

		- TODO: the "assumed" field for a confirmed wallet transaction should be


		self.sync_blocks()

		n0.createwallet('test',blank=True, disable_private_keys=True, descriptors=True)

		assert_equal(n.getblockchaininfo()[
		"headers"], SNAPSHOT_BASE_HEIGHT)

	assert_equal(n.getblockchaininfo()[
	"headers"], SNAPSHOT_BASE_HEIGHT)
	assert_equal(n.getblockchaininfo()["headers"], SNAPSHOT_BASE_HEIGHT)

test: add assumeutxo wallet test #28838

test: add assumeutxo wallet test #28838

Uh oh!

Conversation

Sjors commented Nov 10, 2023

Uh oh!

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

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Nov 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 27, 2023

Uh oh!

Sjors commented Nov 28, 2023

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

theStack commented Jan 10, 2024

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 11, 2024

Uh oh!

achow101 commented Jan 11, 2024

Uh oh!

jamesob commented Jan 11, 2024

Uh oh!

maflcko commented Jan 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DrahtBot commented Nov 10, 2023 •

edited

Loading

Sjors Nov 13, 2023 •

edited

Loading

Sjors Nov 13, 2023 •

edited

Loading

Sjors Jan 12, 2024 •

edited

Loading

achow101 Jan 12, 2024 •

edited

Loading