wallet: Ensure best block matches wallet scan state

achow101 commented at 9:58 pm on June 3, 2024: member

Implements the idea discussed in #29652 (comment)

Currently, m_last_block_processed and m_last_block_processed_height are not guaranteed to match the block locator stored in the wallet, nor do either of those fields actually represent the last block that the wallet is synced up to. This is confusing and unintuitive.

This PR changes the those last block fields to actually be in sync with the record stored on disk. This requires adding the block height to the BESTBLOCK_NOMERKLE record, which was done in a backwards compatible manner. Additionally, the issue with chainStateFlushed fixed in #29652 is also fixed here by removing chainStateFlushed entirely. The last block fields, now simply the best block, is updated for each blockConnected and blockDisconnected so that it does actually represent the block for which the wallet is synced up to.

To make it easier to deal with, the best block fields are consolidated into a BestBlock struct which also has the serialization code with backwards compatibility.

DrahtBot commented at 9:58 pm on June 3, 2024: 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/30221.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	fjahr, mzumsande

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:

#31296 (wallet: Translate [default wallet] string in progress messages by ryanofsky)
#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
#30214 (refactor: Improve assumeutxo state representation by ryanofsky)
#29680 (wallet: fix unrelated parent conflict doesn’t cause child tx to be marked as conflict by Eunovo)
#29652 (wallet: Avoid potentially writing incorrect best block locator by ryanofsky)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)

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 Wallet on Jun 3, 2024

Thompson1985 approved

ryanofsky commented at 3:34 pm on June 4, 2024: contributor

I’m not sure, but it seems like a potentially good thing to me that last_block_processed and BESTBLOCK are two distinct concepts.

The last block processed is the block that CWallet in-memory state has been synced to (particularly CWalletTx::m_state which includes mempool / abandoned information not serialized to disk).

The BESTBLOCK record is the last block whose data has been flushed to disk, that the wallet should begin syncing from next time it is reloaded.

It seems like the first commit ae3b8280a7dbb6cc87814ed97dbea5a122cf8215 could lead to a performance regression if it is writing the flushing the BESTBLOCK record on every single blockConnected event from the node. For example during reindexing it could write and sync the database each time a block is connected, even if the block is before the wallets birthday, or doesn’t have any relevant transactions?

I think it probably makes sense to write BESTBLOCK in a smarter way, and probably write it more frequently, but it seems unnecessary to have to write it every block, and it might be bad for performance now or make optimizations harder in the future.

I’m also curious about the idea of saving height with the best block record. I could imagine this being useful, since IIRC parts of wallet loading are complicated by not knowing heights of transactions before the chain is attached, but it doesn’t really seem like the height is used for much yet. Was this intended to be used by something else later?

achow101 commented at 3:48 pm on June 4, 2024: member

The last block processed is the block that CWallet in-memory state has been synced to (particularly CWalletTx::m_state which includes mempool / abandoned information not serialized to disk).

But none of that state is relevant to the last block processed. Any state related to blocks (confirmed or conflicted) is written to disk.

The BESTBLOCK record is the last block whose data has been flushed to disk, that the wallet should begin syncing from next time it is reloaded.

The point is that this discrepancy can result in skipping blocks. It doesn’t make sense to me that we wouldn’t store the block the wallet’s state has been synced to, and it doesn’t make sense that we should rely on a field that means something else to determine what point to start rescanning from on the next load.

If BESTBLOCK doesn’t match the chainstate, that’s fine because it’s a locator and we will just find the fork and sync from there. I don’t think it’s necessary to record which block we think the chainstate is synced to.

ryanofsky commented at 3:56 pm on June 4, 2024: contributor

It doesn’t make sense to me that we wouldn’t store the block the wallet’s state has been synced to

You may be right this is the better approach. But I think the previous approach does make some sense, too. You might not want to write to the wallet database each time a block is connected if the block doesn’t contain any relevant transactions, especially during reindexing.

achow101 commented at 9:03 pm on June 4, 2024: member

You might not want to write to the wallet database each time a block is connected if the block doesn’t contain any relevant transactions

I think you would since not writing it would result in possibly rescanning that block at the next loading which takes a little bit of time, regardless of whether any relevant transactions are in the block.

especially during reindexing.

Perhaps an easy solution to this is to just write the best block every 1000 blocks (or some other interval) when we are in IBD?

I’m also curious about the idea of saving height with the best block record. I could imagine this being useful, since IIRC parts of wallet loading are complicated by not knowing heights of transactions before the chain is attached, but it doesn’t really seem like the height is used for much yet. Was this intended to be used by something else later?

This was mainly done to avoid looking up the height every time we load since that was being problematic and causing a tests to fail. But it definitely could be more useful in the future.

ryanofsky commented at 3:16 pm on June 5, 2024: contributor

The idea of saving heights is interesting to me because wallet code assumes it know block heights many places, but it doesn’t actually store heights anywhere. So for example, if the wallet stored a mapping of block hashes to heights (or other block information) it might be useful for allowing the wallet to work in an offline mode or letting the wallet CLI tool have more capabilities. This is all pretty abstract though, so I’m not suggesting something specific.

Perhaps an easy solution to this is to just write the best block every 1000 blocks (or some other interval) when we are in IBD?

Yes but I think that just adds back the complexity you were trying to get rid of, in a form that seems worse than the status quo. If you implement that, the wallet will be back to tracking the last block processed separately from the best block written to the database. And now, instead of the node determining when data should be flushed to disk and having all wallets flush that data simultaneously, each wallet will have internal logic deciding when to flush. This would be more complicated, and could be worse for power consumption and performance if there are multiple wallets and flushes are happening more frequently at different times.

I think overall this PR is doing some good things, but the goals seem either not clear, or not clearly good, so I wonder if maybe you could take these changes and make more targeted PRs for the things you want to achieve?

Or, if this PR is definitely the direction you want to go, I’m happy to review it. I don’t like some things it is doing, but overall I think it is a reasonable change.

DrahtBot added the label Needs rebase on Jul 22, 2024

achow101 force-pushed on Jul 22, 2024

DrahtBot removed the label Needs rebase on Jul 22, 2024

DrahtBot added the label Needs rebase on Aug 12, 2024

in test/functional/wallet_assumeutxo.py:111 in ebf2e2863c outdated

84@@ -84,8 +85,6 @@ def run_test(self):
85             assert_equal(n.getblockchaininfo()[
86                          "headers"], SNAPSHOT_BASE_HEIGHT)
87 
88-        w.backupwallet("backup_w.dat")

fjahr commented at 11:18 am on August 20, 2024:

I’m thinking that it might make sense to keep this backup where it is and add second backup created at 199. Then this could test that both cases work as expected, i.e. 199 errors below and 299 doesn’t. 299 is an interesting edge case in general (snapshot height == backup height).

fjahr commented at 11:28 am on August 20, 2024: contributor

Concept ACK

I can confirm that best block locator and last_processed_block being different is confusing, see also #30455 (review)

Currently, this needs a rebase and I’m curious if @achow101 plans to make further changes based on @ryanofsky ’s comments.

ryanofsky commented at 2:28 pm on August 20, 2024: contributor

I can confirm that best block locator and last_processed_block being different is confusing

I wonder if something else could be done to resolve this confusion other than writing to every wallet file every time a new block is connected, even if the block doesn’t contain any relevant transactions. To me, “last block processed” and “last block flushed” seem like different concepts, and we could force them to be the same but only if we:

Give up flexibility of not needing to flush each wallet each time a block is connected (which seems inefficient during sync)
Give up ability to do coordinated flushes across the chainstate database, index databases and wallet databases time (which seems like it could waste resources and hurt system performance)

fjahr commented at 2:35 pm on August 20, 2024: contributor

I wonder if something else could be done to resolve this confusion

Ok, I would say it’s a problem when users see behavior that is inconsistent because of this difference. If the users don’t notice it I guess these don’t have to be the same at all times. There could be a more “pull-based” approach where they are aligned when the user interacts with the wallet, similar to what I tried to do for the backup case in #30678. But I am not clear yet on which approach is the better one all factors considered.

ryanofsky commented at 2:48 pm on August 20, 2024: contributor

It would be great if we could open an issue describing the user behavior that’s confusing. I read the comment you linked to but I didn’t really understand the context of what was going on with those backups. If there can be a github issue that simply describes steps to reproduce, expected behavior, and actual behavior, we can probably figure out if expected behavior is reasonable, and implement a fix if so.

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

It would be great if we could open an issue describing the user behavior that’s confusing.

I have created an issue here: #30686

achow101 force-pushed on Aug 29, 2024

Thompson1985 approved

Thompson1985 commented at 8:08 pm on August 29, 2024: none

Thank you 👍

DrahtBot removed the label Needs rebase on Aug 29, 2024

DrahtBot added the label Needs rebase on Sep 23, 2024

achow101 force-pushed on Oct 4, 2024

DrahtBot removed the label Needs rebase on Oct 4, 2024

DrahtBot commented at 9:52 pm on October 4, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/31102970538

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot added the label CI failed on Oct 4, 2024

maflcko commented at 3:35 pm on October 16, 2024: member

Maybe mark as draft while CI is red?

DrahtBot added the label Needs rebase on Oct 24, 2024

achow101 force-pushed on Oct 24, 2024

DrahtBot removed the label Needs rebase on Oct 24, 2024

DrahtBot added the label Needs rebase on Oct 25, 2024

achow101 force-pushed on Oct 25, 2024

DrahtBot removed the label Needs rebase on Oct 25, 2024

DrahtBot removed the label CI failed on Oct 25, 2024

DrahtBot added the label Needs rebase on Nov 15, 2024

achow101 force-pushed on Nov 19, 2024

DrahtBot removed the label Needs rebase on Nov 19, 2024

DrahtBot added the label Needs rebase on Jan 15, 2025

achow101 force-pushed on Jan 20, 2025

DrahtBot removed the label Needs rebase on Jan 20, 2025

DrahtBot added the label Needs rebase on Jan 31, 2025

achow101 force-pushed on Feb 1, 2025

DrahtBot removed the label Needs rebase on Feb 1, 2025

DrahtBot added the label CI failed on Feb 1, 2025

in src/wallet/wallet.cpp:1537 in ddc8095208 outdated

1530@@ -1517,8 +1531,11 @@ void CWallet::blockConnected(ChainstateRole role, const interfaces::BlockInfo& b
1531     assert(block.data);
1532     LOCK(cs_wallet);
1533 
1534-    m_last_block_processed_height = block.height;
1535-    m_last_block_processed = block.hash;
1536+    // Update the best block first. This will set the best block's height, which is
1537+    // needed by MarkConflicted.
1538+    // Although this also writes the best block to disk, this is okay even if there is an unclean
1539+    // shutdown since reloading the wallet will still rescan this block.

mzumsande commented at 10:25 pm on February 6, 2025:

this is okay even if there is an unclean shutdown since reloading the wallet will still rescan this block

I don’t think that this is the case. We will be temporarily in a state in which the wallet thinks it has scanned the block (locator is written to disk) but haven’t actually done the SyncTransaction work yet. This is fragile, because in case of an unclean shutdown a future rescan may not include this block if it thinks, based on the locator, that the block has already been scanned.

Therefore, it seems better to me to first do the SyncTransaction work and only then update the best block locator on disk - if there is an unclean shutdown with this order, we’ll be sure to sync the transactions again.

Although the best solution would be to it all in one batch.

in src/wallet/wallet.cpp:1591 in ddc8095208 outdated

1585@@ -1573,6 +1586,9 @@ void CWallet::blockDisconnected(const interfaces::BlockInfo& block)
1586             }
1587         }
1588     }
1589+
1590+    // Update the best block
1591+    SetLastBlockProcessed(block.height - 1, *Assert(block.prev_hash));

mzumsande commented at 8:58 pm on February 7, 2025:

To deal with unclean shutdowns, during disconnect the reverse order compared to blockConnected would be better: If the locator is updated first, an unclean shutdown will lead to a rescan of the disconnected block (which might do nothing but correct the locator). But if txns are set to Inactive first and have an unclean shutdown after that, the transactions will be missing from the wallet / balance. As mentioned above, doing it all in one batch would resolve this in a more elegant way.

mzumsande commented at 11:29 pm on February 7, 2025: contributor

Concept ACK

I verified that this would solve the issues from #31824, only looked at the first couple commits in detail so far.

With respect to the above discussion, I think that the concept of delayed flushing only makes sense if it applies to all relevant data for a given block connection /disconnection (which is the case for the indexes, for example). But when other changes to wallet txns are synced to disk immediately, it seems wrong to delay flushing the locator - this will lead to a mixed state where some relevant data is flushed and other data isn’t, which will lead to issues such as the one above.

As for the point of IBD performance due to updating the locator at every block: Maybe this could be prevented by keeping track during block (dis)connection, if we did any other changes to the wallet db for this block. If nothing else needed changing ( wallet not affected by the block, only the locator would be updated) it should be safe to delay / skip the update.

DrahtBot added the label Needs rebase on Feb 14, 2025

achow101 force-pushed on Mar 6, 2025

achow101 commented at 1:50 am on March 6, 2025: member

As discussed at CoreDev, I’ve updated this PR to sync the best block periodically or when a connected block contains a relevant transaction.

I’ve also had to make a couple of changes to AttachChain following #31629

achow101 force-pushed on Mar 6, 2025

DrahtBot removed the label Needs rebase on Mar 6, 2025

achow101 force-pushed on Mar 6, 2025

achow101 commented at 7:54 pm on March 6, 2025: member

I added a change to make sure that the most recent best block is written to disk on wallet unload. However, it seems like this uncovered some weirdness in the wallet unloading on shutdown. In particular, StopWallets() was being called before UnloadWallets() which I think is incorrect. I added a commit which flips this order, and it seems to work, although there may be side effects I have not discovered yet. This primarily affects BDB wallets as the shutdown for BDB wallets is much more involved and confusing due to the shared database environment. This is not an issue for SQLite.

achow101 force-pushed on Mar 7, 2025

DrahtBot removed the label CI failed on Mar 7, 2025

DrahtBot added the label Needs rebase on Mar 13, 2025

wallet: Update best block record after block dis/connect

When a block is connected, if the new block had anything relevant to the
wallet, update the best block record on disk. If not, also sync the best
block record to disk every 144 blocks.

8b8cb9f4e9

scripted-diff: Rename LastBlock to BestBlock

-BEGIN VERIFY SCRIPT-
sed -i "s/SetLastBlockProcessed/SetBestBlock/" $(git grep -l "SetLastBlockProcessed")
sed -i "s/GetLastBlock/GetBestBlock/g" $(git grep -l "GetLastBlock")
-END VERIFY SCRIPT-

0654b31390

wallet: Combine last block processed hash and height into a struct

m_last_block_processed and m_last_block_processed_height are combined
into a new struct BestBlock.

df8dca28c1

wallet: Replace chainStateFlushed in loading with WriteBestBlock

The only reason to call chainStateFlushed during wallet loading is to
ensure that the best block is written. Do these writes explicitly to
prepare for removing chainStateFlushed.

97f2d4ae27

wallet: Remove WriteBestBlock from chainStateFlushed 748063a982

wallet: Sync m_best_block with BESTBLOCK_NOMERKLE

The BestBlock struct now contains the block locator, and is also
serialized with the block height. For backwards compatibility, the
height is optional. The best block hash is retrieved from the block
locator when deserializing.

Since the best block record has to store the height, additional
changes to AttachChain are made to faciliate auto upgrade as well as
setting the in-memory best block.

f43e11adf3

wallet: Load best block record in LoadWallet

Instead of setting the best block info later during AttachChain, read it
during LoadWallet so it is set (if it exists) before we get there.

f24d9f620f

wallet: Use best block locator in migrate instead of re-reading it

Instead of calling ReadBestBlock to get the best block locator, use the
m_best_block.m_locator that we already have in CWallet.

574d7b930c

wallet: Swap order of Unload and Close on shutdown

StopWallets, now renamed to CloseWallets, was being called prior to
UnloadWallets, which does not entirely make sense as UnloadWallets does
additional database cleanups.

UnloadWallets is now called first, then CloseWallets.

6ffa4e92d2

wallet: Write best block record on unload b7c631e895

wallet: Remove chainStateFlushed

chainStateFlushed is no longer needed since the best block is updated
after a block is scanned. Since the chainstate being flushed does not
necessarily coincide with the wallet having processed said block, it
does not entirely make sense for the wallet to be recording that block
as its best block, and this can cause race conditions where some blocks
are not processed. Thus, remove this notification.

eecaca12f6

achow101 force-pushed on Mar 14, 2025

DrahtBot removed the label Needs rebase on Mar 14, 2025

DrahtBot added the label CI failed on Mar 14, 2025

in src/wallet/wallet.cpp:1562 in 8b8cb9f4e9 outdated

1560         transactionRemovedFromMempool(block.data->vtx[index], MemPoolRemovalReason::BLOCK);
1561     }
1562+
1563+    // Update on disk if this block resulted in us updating a tx, or periodically every 144 blocks (~1 day)
1564+    if (wallet_updated || block.height % 144 == 0) {
1565+        SetLastBlockProcessed(block.height, block.hash);

mzumsande commented at 6:39 pm on March 25, 2025:

8b8cb9f4e9055a3283805ed6ff9397236842bb34 : need to update wallet_reorgsrestore.py after #31757 introduced a test for the bug this PR fixes.

in src/wallet/wallet.cpp:652 in eecaca12f6

648@@ -649,15 +649,6 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase,
649     return false;
650 }
651 
652-void CWallet::chainStateFlushed(ChainstateRole role, const CBlockLocator& loc)

mzumsande commented at 9:28 pm on March 25, 2025:

eecaca12f6979aefed51c7b19f886c9d7d4e9be4: Two calls to chainStateFlushed are left, in MigrateLegacyToDescriptor and bench/wallet_migration.cpp

mzumsande commented at 9:46 pm on March 25, 2025: contributor

I don’t completely understand what the second part of the PR (BESTBLOCK_NOMERKLE serialization) achieves with the current approach. With 748063a9820cb740c9b68f80b9e8c97d2a21e83d, chainStateFlushed has been made a no-op and could be removed. On the other hand, the expressed goal of having m_last_block_processed and m_last_block_processed_height always in sync with the block locator is no longer current, given that we only update every 144 blocks if there is nothing relevant for the wallet in these blocks. In that case, why not remove chainstateFlushed after 748063a9820cb740c9b68f80b9e8c97d2a21e83d (plus update the locator on unload) and call it a day, avoiding any possible compatibility issues and the complications of a walletdb format change?

wallet: Ensure best block matches wallet scan state #30221

Code Coverage & Benchmarks

Reviews

Conflicts