test: verify node state after restart in assumeutxo

yashbhutwala commented at 3:55 pm on January 14, 2026: none

Summary

This PR replaces the TODO comment in wallet_assumeutxo.py (line 242) with actual test assertions that verify node and wallet behavior after a restart during assumeutxo background sync.

Changes

The new tests verify:

Two chainstates exist (background validation not complete)
Background chainstate is still at START_HEIGHT
Snapshot chainstate has synced to at least PAUSE_HEIGHT
Wallets cannot be loaded after restart (expected behavior)
Wallet backup from before snapshot height cannot be restored

Motivation

During implementation, I discovered that wallets cannot be loaded after a node restart during assumeutxo background sync. This is expected behavior because:

The wallet loading code checks if required blocks are available for rescanning
During assumeutxo background sync, blocks before the snapshot are not available
This applies to all wallets, including watch-only wallets created at the snapshot height

This is a valuable test addition because it documents this expected behavior and ensures it doesn’t regress. Users should be aware that if they restart their node during assumeutxo background sync, they won’t be able to load their wallets until the background sync completes.

refs #28648

Addresses the TODO comment that was originally added as part of the assumeutxo wallet test implementation.

DrahtBot added the label Tests on Jan 14, 2026

DrahtBot commented at 3:56 pm on January 14, 2026: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34286.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	fjahr
Concept ACK	Bicaru20

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34395 (test: rename assert_greater_than(_or_equal) to assert_gt/assert_ge by l0rinc)

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.

in test/functional/wallet_assumeutxo.py:252 in fc1b15c8fc

248+        # The background chainstate should still be at START_HEIGHT
249+        assert_equal(chainstates[0]['blocks'], START_HEIGHT)
250+        # The snapshot chainstate should be at PAUSE_HEIGHT or higher
251+        # (it may have synced more blocks before the stopatheight triggered)
252+        snapshot_height = chainstates[-1]['blocks']
253+        assert_greater_than_or_equal(snapshot_height, PAUSE_HEIGHT)

Bicaru20 commented at 7:04 pm on January 14, 2026:

Since you are checking the length of chainstates, maybe it is clear to put the index 1 instead of -1. That way, I believe it is easier to see that you are accessing the second chainstate.

Are you sure the assert_greater_than_or_equal is needed? I don’t see why snapshot_height would be higher. I have tried it several times to be sure, and it is always equal to PAUSE_HEIGHT. Maybe an assert_equal would be enough?

Since snapshot_height is just used here maybe we can put chainstates[-1]['blocks'] directly into the assert.

0        assert_equal(chainstates[1]['blocks'], PAUSE_HEIGHT)

yashbhutwala commented at 7:35 pm on January 14, 2026:

Thanks for the review! You’re right on all points. I’ve updated the code to:

Use index [1] instead of [-1] for clarity since we just verified len(chainstates) == 2
Use assert_equal since stopatheight is deterministic in practice (your testing confirms this)
Simplified by inlining the assertion

I also removed the now-unused assert_greater_than_or_equal import.

Bicaru20 commented at 7:05 pm on January 14, 2026: none

I tested the changes, and the test wallet_assumeutxo.py passes. The newly added assertions look good — they correctly verify the node state (number of chainstates and their respective heights). The test also checks that wallets cannot be loaded after a node restart, and finally confirms that restoring a wallet with a backup that predates the snapshot height is not allowed. Overall, the code looks solid. I’ve left a few minor comments and questions for clarification.

yashbhutwala commented at 7:22 pm on January 14, 2026: none

I’ve left a few minor comments and questions for clarification. @Bicaru20 I don’t see any comments or questions.

Bicaru20 commented at 7:29 pm on January 14, 2026: none

I’ve left a few minor comments and questions for clarification.

@Bicaru20 I don’t see any comments or questions.

My bad, now you should be able to see them

yashbhutwala force-pushed on Jan 14, 2026

yashbhutwala requested review from Bicaru20 on Jan 14, 2026

fjahr commented at 8:59 am on January 15, 2026: contributor

Concept ACK

DrahtBot added the label CI failed on Jan 15, 2026

Bicaru20 commented at 2:47 pm on January 15, 2026: none

Looks like some CI tests are failing. Probably because of the assert I suggested to change (my bad, sorry). You probably should test if the assert is the problem and change it back to what you had.

yashbhutwala force-pushed on Jan 15, 2026

yashbhutwala commented at 3:21 pm on January 15, 2026: none

@Bicaru20 Thanks for catching this! You were right that the CI failed.

The failure showed AssertionError: not(360 == 359) - the snapshot chainstate was at height 360, not 359.

This confirms what the stopatheight documentation says: “Blocks after target height may be processed during shutdown.”

I’ve reverted back to assert_greater_than_or_equal with a clearer comment explaining why:

0# The snapshot chainstate should be at least PAUSE_HEIGHT. It may be
1# higher because stopatheight may allow additional blocks to be
2# processed during shutdown (per stopatheight documentation).
3assert_greater_than_or_equal(chainstates[1]['blocks'], PAUSE_HEIGHT)

I kept the [1] index suggestion since that’s still clearer when we’ve verified len(chainstates) == 2.

yashbhutwala closed this on Jan 15, 2026

yashbhutwala reopened this on Jan 15, 2026

DrahtBot removed the label CI failed on Jan 15, 2026

Bicaru20 commented at 10:25 am on January 17, 2026: none

Test code ACK. Everything looks good!

yashbhutwala commented at 4:32 pm on January 21, 2026: none

Is this good to merge?

in test/functional/wallet_assumeutxo.py:260 in 3680ccb336 outdated

256+        # the wallet loading code checks if required blocks are available for
257+        # rescanning. During assumeutxo background sync, blocks before the
258+        # snapshot are not available, so wallet loading fails.
259+        # This applies to both wallet "w" (restored from backup) and wallet "w1"
260+        # (watch-only created at snapshot height).
261+        assert_raises_rpc_error(-4, "Wallet loading failed. Error loading wallet. Wallet requires blocks to be downloaded", n1.loadwallet, "w")

fjahr commented at 5:55 pm on January 21, 2026:

The error message repeats itself 3 times, so you should be able to put it into a variable and reuse it.

fjahr commented at 5:57 pm on January 21, 2026:

Also, this isn’t the full error message. There is more to it and it also includes the target height that the wallet needs which you should check as well. You can see the message some lines above, this can probably be deduplicated as well.

yashbhutwala commented at 2:23 pm on January 22, 2026:

Thanks for the review @fjahr! Good catch! Fixed by:

Created wallet_loading_error base string and reused it with the appropriate heights (SNAPSHOT_BASE_HEIGHT before restart, SNAPSHOT_BASE_HEIGHT + 1 after restart)
Renamed the descriptor error variable to descriptor_import_error so it doesn’t clobber the wallet one
Now verifying the full error message including the target height

Noticed something interesting while testing: after restart, all wallet loading errors show height 300 regardless of wallet creation time.

fjahr commented at 10:13 pm on January 21, 2026: contributor

Is this good to merge?

No, there are only concept ACKs here so far, so it needs more review.

yashbhutwala force-pushed on Jan 22, 2026

yashbhutwala requested review from fjahr on Jan 22, 2026

in test/functional/wallet_assumeutxo.py:219 in 2ecf4f0183 outdated

218         n1.createwallet(wallet_name, disable_private_keys=True)
219         key = get_generate_key()
220         time = n1.getblockchaininfo()['time']
221         timestamp = 0
222-        expected_error_message = f"Rescan failed for descriptor with timestamp {timestamp}. There was an error reading a block from time {time}, which is after or within 7200 seconds of key creation, and could contain transactions pertaining to the desc. As a result, transactions and coins using this desc may not appear in the wallet. This error is likely caused by an in-progress assumeutxo background sync. Check logs or getchainstates RPC for assumeutxo background sync progress and try again later."
223+        descriptor_import_error = f"Rescan failed for descriptor with timestamp {timestamp}. There was an error reading a block from time {time}, which is after or within 7200 seconds of key creation, and could contain transactions pertaining to the desc. As a result, transactions and coins using this desc may not appear in the wallet. This error is likely caused by an in-progress assumeutxo background sync. Check logs or getchainstates RPC for assumeutxo background sync progress and try again later."

fjahr commented at 12:32 pm on January 23, 2026:

nit: This renaming wasn’t necessary afaict and doesn’t seem to have anything to do with the actual change, better to revert this if you retouch.

yashbhutwala commented at 2:33 pm on January 23, 2026:

Fixed

in test/functional/wallet_assumeutxo.py:209 in 2ecf4f0183 outdated

206-        assert_raises_rpc_error(-4, expected_error_message, n1.restorewallet, "w2", "backup_w2.dat")
207+        # Error message for wallets that need blocks before the snapshot height.
208+        # The target height is SNAPSHOT_BASE_HEIGHT because that's when background sync completes.
209+        wallet_loading_error = "Wallet loading failed. Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order when using assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height"
210+        wallet_loading_error_snapshot_height = f"{wallet_loading_error} {SNAPSHOT_BASE_HEIGHT}"
211+        assert_raises_rpc_error(-4, wallet_loading_error_snapshot_height, n1.restorewallet, "w2", "backup_w2.dat")

fjahr commented at 12:36 pm on January 23, 2026:

I would have done it as a function and avoided these long additional variable names, this would save us a line below as well where the actual test additions are.

0        # Error message for wallets that need blocks before the snapshot height.
1        def loading_error(height):
2            return f"Wallet loading failed. Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order when using assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height {height}"
3        # The target height is SNAPSHOT_BASE_HEIGHT because that's when background sync completes.
4        assert_raises_rpc_error(-4, loading_error(SNAPSHOT_BASE_HEIGHT), n1.restorewallet, "w2", "backup_w2.dat")

yashbhutwala commented at 2:33 pm on January 23, 2026:

Fixed

fjahr commented at 12:40 pm on January 23, 2026: contributor

utACK 2ecf4f0183a87c32540d4c18c36346bd8794d522

Would be nice to address my comments but also fine if you wait for more review comments. It would be also fine for me if this is merged as is.

yashbhutwala force-pushed on Jan 23, 2026

test: verify node state after restart in assumeutxo

Replace the TODO comment in wallet_assumeutxo.py with actual test
assertions that verify node and wallet behavior after a restart
during assumeutxo background sync.

The new tests verify:
- Two chainstates exist (background validation not complete)
- Background chainstate is still at START_HEIGHT
- Snapshot chainstate has synced to at least PAUSE_HEIGHT
- Wallets cannot be loaded after restart (expected behavior during
  background sync because blocks before snapshot are unavailable
  for rescanning)
- Wallet backup from before snapshot height cannot be restored

This documents the expected behavior that wallets cannot be loaded
after a node restart during assumeutxo background sync, which is
an important edge case for users to be aware of.

refs #28648

5cd57943b8

yashbhutwala force-pushed on Jan 23, 2026

yashbhutwala requested review from fjahr on Jan 23, 2026

fjahr commented at 9:10 pm on January 23, 2026: contributor

ACK 5cd57943b8adc76ed0b8a75a83f27bc0f971cbef

Thanks for implementing my suggestions.

yashbhutwala requested review from fjahr on Jan 28, 2026