wallet: lastprocessedblock can be inconsistent with internal best block

fjahr commented at 6:43 pm on August 20, 2024: contributor

Is there an existing issue for this?

I have searched the existing issues

Current behaviour

m_last_block_processed and m_last_block_processed_height, which the users can see in their getwalletinfo() result as lastprocessedblock > height, are not guaranteed to match the block locator stored in the wallet. In practice this means that if the user checks this height and then creates a backup of the wallet they might expect that the wallet will only need to continue syncing from the lastprocessedblock when the backup is loaded later. However, this may or may not be the case. Where the wallet starts syncing depends on when the block locator was last written to disk and that is not clear to the user.

An example of how this created confusion in the wallet_assumeutxo.py case can be seen here: #30455 (review)

Expected behaviour

The preferred way of dealing with this seems to be that the block locator is updated either every time the lastprocessedblock is updated or at least on “pull basis” every time the user interacts with these values. I.e. when the user might create a backup the block locator should be updated to be in sync with the current lastprocessedblock.

Another more minimalistic approach could be to show users the relevant block locator. That may be in getwalletinfo() or the return value of backupwallet(). I do think that this would still require extra documentation effort to explain to users how these values are different and how both of them relevant.

Steps to reproduce

This commit demonstrates the problem well though it does require understanding of assumeutxo: https://github.com/bitcoin/bitcoin/pull/30678/commits/4ffdc9b70711b2bec0e6e86a4f2cc0a87de406d2

The backup backup_w.dat is created at height 299. Loading it fails and the test says that it fails because it is part of the background sync (the IBD chain) and at this point the background sync has not finished meaning that the blocks that are needed are not available for scanning. However, height 299 is actually not part of the background sync. It is the snapshot height. The test only works on master because the block locator is not updated before the backup, so a block previously of 299 is where the wallet is trying to start syncing and that block is then indeed part of the background sync. The commit above changes this by writing the best block locator before the backup is created. This makes the test fail in the form it is on master. The commit then changes the test by creating a backup that is older (at height 199). This height is part of the background sync so the test now works again because it does not depend on the best block locator behavior anymore.

While the test does not make it explicit, when trying to change the test @alfonsoromanz observed this behavior and relied on lastprocessedblock to tell him what the start point for the sync would be when the backup was loaded (same link as above: #30455 (review))

Relevant log output

No response

How did you obtain Bitcoin Core

Compiled from source

What version of Bitcoin Core are you using?

master

Operating system and version

macos

Machine specifications

No response

fjahr commented at 7:42 pm on August 20, 2024: contributor

Another more minimalistic approach could be to show users the relevant block locator. That may be in getwalletinfo() or the return value of backupwallet().

Here is rough draft of what that could look like. I am not if that is a good solution as mentioned above but if people think it’s valuable I can clean it up and open a PR: https://github.com/fjahr/bitcoin/commit/fa4c9978a50802ad3ada5b6dce370f57b8ec16b8

ryanofsky commented at 8:07 pm on August 20, 2024: contributor

Thanks for explaining all this.

fa4c9978a50802ad3ada5b6dce370f57b8ec16b8 could solve the immediate problem if users notice the return value, but it doesn’t seem as robust a solution as updating the wallet before creating the backup like 4ffdc9b70711b2bec0e6e86a4f2cc0a87de406d2 is trying to do. Only problem with that commit is it might not be writing the right locator:

0    if (m_chain) {
1        WalletBatch batch{GetDatabase()};
2        CBlockLocator loc{m_chain->getTipLocator()};
3        if (!loc.IsNull()) {
4            batch.WriteBestBlock(loc);
5        }
6    }

Should probably be:

0    if (m_chain) {
1        CBlockLocator loc;
2        WITH_LOCK(cs_wallet, chain().findBlock(m_last_block_processed, FoundBlock().locator(loc)));
3        if (!loc.IsNull()) {
4            WalletBatch batch(GetDatabase());
5            batch.WriteBestBlock(loc);
6        }
7    }

ryanofsky commented at 8:14 pm on August 20, 2024: contributor

I just noticed #30678. It may be good to reopen that.

fjahr renamed this:
~~wallet: lastprocessedblock is be inconsistent with internal best block~~
wallet: lastprocessedblock can be inconsistent with internal best block
on Aug 23, 2024

wallet: lastprocessedblock can be inconsistent with internal best block #30686