test: add assumeutxo wallet test

Sjors commented at 1:15 am on November 10, 2023: member

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 commented at 1:15 am on November 10, 2023: 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
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.

DrahtBot added the label Tests on Nov 10, 2023

in test/functional/wallet_assumeutxo.py:87 in 0b81433961 outdated

82+
83+        # Ensure everyone is seeing the same headers.
84+        for n in self.nodes:
85+            assert_equal(n.getblockchaininfo()["headers"], SNAPSHOT_BASE_HEIGHT)
86+
87+        w.backupwallet("/tmp/abc.dat")

maflcko commented at 8:50 am on November 10, 2023:

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.

in test/functional/wallet_assumeutxo.py:10 in 0b81433961 outdated

 5+"""Test for assumeutxo wallet related behavior.
 6+See feature_assumeutxo.py for background.
 7+
 8+## Possible test improvements
 9+
10+- TODO: the "assumed" field for a confirmed wallet transaction should be

maflcko commented at 8:51 am on November 10, 2023:

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

Sjors commented at 6:29 am on November 13, 2023:

Dropped

in test/functional/wallet_assumeutxo.py:66 in 0b81433961 outdated

61+        for n in self.nodes:
62+            n.setmocktime(n.getblockheader(n.getbestblockhash())['time'])
63+
64+        self.sync_blocks()
65+
66+        n0.createwallet('test',blank=True, disable_private_keys=True, descriptors=True)

maflcko commented at 8:52 am on November 10, 2023:

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

Sjors commented at 6:25 am on November 13, 2023:

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

in test/functional/wallet_assumeutxo.py:76 in 0b81433961 outdated

71+        # Generate a series of blocks that `n0` will have in the snapshot,
72+        # but that n1 doesn't yet see. In order for the snapshot to activate,
73+        # though, we have to ferry over the new headers to n1 so that it
74+        # isn't waiting forever to see the header of the snapshot's base block
75+        # while disconnected from n0.
76+        for i in range(100):

brunoerg commented at 1:45 pm on November 10, 2023:

In 0b8143396183d23e215fc90a2d24813ad1b7ad23: nit

0        for _ in range(100):

Sjors force-pushed on Nov 13, 2023

Sjors commented at 6:43 am on November 13, 2023: member

Pushed some cleanups.

in test/functional/wallet_assumeutxo.py:33 in f4f3a0c910 outdated

28+        """Use the pregenerated, deterministic chain up to height 199."""
29+        self.num_nodes = 2
30+        self.rpc_timeout = 120
31+        self.extra_args = [
32+            [],
33+            ["-fastprune", "-prune=1"]

maflcko commented at 8:47 am on November 13, 2023:

Could add a comment why this is needed?

Sjors commented at 11:13 am on November 13, 2023:

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

achow101 commented at 2:14 pm on November 28, 2023:

It doesn’t seem like this was pushed?

in test/functional/wallet_assumeutxo.py:62 in f4f3a0c910 outdated

57+        self.sync_blocks()
58+
59+        n0.createwallet('w')
60+        w = n0.get_wallet_rpc("w")
61+
62+        w.importdescriptors([{"desc": "addr(mjTkW3DjgyZck4KbiRusZsqTgaYTxdSz6z)#jdtmxdcm",

Sjors commented at 11:13 am on November 13, 2023:

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.

achow101 commented at 2:14 pm on November 28, 2023:

It doesn’t seem like this was pushed?

test: add assumeutxo wallet test

Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

997b9a73e5

pablomartin4btc commented at 10:55 pm on November 25, 2023: member

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

maflcko commented at 9:32 am on November 27, 2023: member

lgtm ACK f4f3a0c910a148a158e1879f8c9e6767575539db

Sjors force-pushed on Nov 28, 2023

Sjors commented at 3:24 pm on November 28, 2023: member

@achow101 good catch, I indeed forgot to push. @pablomartin4btc I think he likes it this way

BrandonOdiwuor commented at 8:58 am on December 29, 2023: contributor

Concept ACK

theStack commented at 12:43 pm on January 10, 2024: contributor

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.

glozow referenced this in commit 632a2bb731 on Jan 10, 2024

in test/functional/wallet_assumeutxo.py:82 in 997b9a73e5

77+            n1.submitheader(newblock)
78+
79+        # Ensure everyone is seeing the same headers.
80+        for n in self.nodes:
81+            assert_equal(n.getblockchaininfo()[
82+                         "headers"], SNAPSHOT_BASE_HEIGHT)

theStack commented at 5:49 pm on January 10, 2024:

nit: looks like an unintended newline here?

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

theStack approved

theStack commented at 5:51 pm on January 10, 2024: contributor

Code-review ACK 997b9a73e5166b4244f7c5b4fe144d524f3005f4

Also ran the test locally.

DrahtBot requested review from pablomartin4btc on Jan 10, 2024

DrahtBot requested review from maflcko on Jan 10, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 10, 2024

DrahtBot removed review request from BrandonOdiwuor on Jan 10, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 10, 2024

maflcko commented at 10:16 am on January 11, 2024: member

lgtm ACK 997b9a73e5166b4244f7c5b4fe144d524f3005f4

DrahtBot removed review request from maflcko on Jan 11, 2024

DrahtBot removed review request from BrandonOdiwuor on Jan 11, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 11, 2024

achow101 commented at 4:23 pm on January 11, 2024: member

ACK 997b9a73e5166b4244f7c5b4fe144d524f3005f4

DrahtBot removed review request from BrandonOdiwuor on Jan 11, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 11, 2024

achow101 merged this on Jan 11, 2024

achow101 closed this on Jan 11, 2024

jamesob commented at 4:31 pm on January 11, 2024: member

Posthumous ACK https://github.com/bitcoin/bitcoin/commit/997b9a73e5166b4244f7c5b4fe144d524f3005f4

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

maflcko commented at 4:58 pm on January 11, 2024: member

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

0                                   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 deleted the branch on Jan 12, 2024

test: add assumeutxo wallet test #28838

Code Coverage

Reviews

Conflicts