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/29652.

Reviews

See the guideline for information on the review process.

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:

• #30221 (wallet: Ensure best block matches wallet scan state by achow101)
• #28710 (Remove the legacy wallet and BDB dependency by achow101)
• #19463 (Prune locks by luke-jr)

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.

I couldn't really come up with a test for this bug directly. I did find if I added an assert in chainStateFlushed that the locator block height matches the best block height, I could trigger it with test code below:

--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -634,6 +634,14 @@ void CWallet::chainStateFlushed(ChainstateRole role, const CBlockLocator& loc)
     if (m_attaching_chain || role == ChainstateRole::BACKGROUND) {
         return;
     }
+    if (!loc.IsNull()) {
+        LOCK(cs_wallet);
+        int loc_height{-1};
+        if (m_last_block_processed_height >= 0 && chain().findBlock(loc.vHave.front(), FoundBlock().height(loc_height))) {
+            WalletLogPrintf("chainStateFlushed processed height %i locator height %i\n", m_last_block_processed_height, loc_height);
+            assert(m_last_block_processed_height >= loc_height);
+        }
+    }
     WalletBatch batch(GetDatabase());
     batch.WriteBestBlock(loc);
 }
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -52,6 +52,8 @@ COMPLETE_IDX = {'synced': True, 'best_block_height': FINAL_HEIGHT}
 
 
 class AssumeutxoTest(BitcoinTestFramework):
+    def add_options(self, parser):
+        self.add_wallet_options(parser)
 
     def set_test_params(self):
         """Use the pregenerated, deterministic chain up to height 199."""
@@ -215,6 +217,7 @@ class AssumeutxoTest(BitcoinTestFramework):
         self.test_invalid_chainstate_scenarios()
 
         self.log.info(f"Loading snapshot into second node from {dump_output['path']}")
+        n1.createwallet('w', load_on_startup=True)
         loaded = n1.loadtxoutset(dump_output['path'])
         assert_equal(loaded['coins_loaded'], SNAPSHOT_BASE_HEIGHT)
         assert_equal(loaded['base_height'], SNAPSHOT_BASE_HEIGHT)

However, this doesn't trigger in the assumeutxo case where the snapshot block ChainStateFlushed notification is reliably sent before the BlockConnected notification, because wallet just ignores both notifications since they are associated with the background chain. The test above does trigger the assert with error chainStateFlushed processed height 199 locator height 299 but I think that is happening because one of walletInstance->chainStateFlushed(ChainstateRole::NORMAL, chain->getTipLocator()); calls on wallet startup.

This is all to say i couldn't really find a practical bug that could be triggered here or a good test to add to this PR.

Updated e295db8 -> c5583a3 (pr/noloc.1 -> pr/noloc.2, compare) dropping unused chainStateFlushed argument
Updated c5583a3 -> 5600631 (pr/noloc.2 -> pr/noloc.3, compare) fixing missed chainStateFlushed call in previous push.

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is 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.

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

_{Debug: https://github.com/bitcoin/bitcoin/runs/22673169748}

Concept ACK

I'm now wondering whether the wallet should even be doing anything on ChainStateFlushed since that doesn't seem like it should have any bearing on what the wallet knows about. It seems like the better thing to do would be to update the best block record every time we finish processing a block in blockConnected, and just ignore chainStateFlushed. The best block is really only used during loading anyways to figure out how much we need to rescan. One thing that could happen if we did this is that on an unclean shutdown, the wallet could store a record that is ahead of the chainstate. On the next load, we would end up resetting to maybe a few blocks behind where the chainstate is at and therefore rescan a bit more than necessary, but that doesn't seem like it would really be a problem.

I'm now wondering whether the wallet should even be doing anything on ChainStateFlushed since that doesn't seem like it should have any bearing on what the wallet knows about. [...]

I've implemented this idea in #30221

I'm now wondering whether the wallet should even be doing anything on ChainStateFlushed since that doesn't seem like it should have any bearing on what the wallet knows about.

Notifications are sent in order, so it does have some meaning. It means wallet is in-between blockConnected events and synchronized up to some block, and that the node has written some data to disk recently, so the wallet may want write its data to disk, too.

I think the only problem with the ChainStateFlushed notification is that because of the fact that FlushStateToDisk is called all over validation code, and notifications are asynchronous, the ChainStateFlushed locator argument is unreliable and buggy. The argument is also unnecessary for wallets and indexes because they already know the last block that they are synced to and don't need a redundant pointer to a potentially incorrect block (due to a bug on assumeutxo code or other other fragile parts of validation code).

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/22674943172}

Marking as draft, because this has a silent merge conflict and I don't think I will be able to fix it right away. I still do think this PR is a good idea, but #30221 can be reviewed too if the alternate approach it implements is preferred (updating the wallet every block even it doesn't change).

🐙 This pull request conflicts with the target branch and needs rebase.

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

• Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
• Is it no longer relevant? ➡️ Please close.
• Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

re: #29652 (review)

Since #30221 got merged, I think this one can be closed

Thanks! #30221 avoids the issue of wallets potentially writing the incorrect locators on flush notifications by just ignoring them and flushing in a less coordinated way. So the main concern of this PR is no longer an issue and it makes sense to close.

I still think the changes here dropping the Chain::getTipLocator and Chain::getActiveChainLocator methods would be good to apply. The first method is unused and since #25717, the second method has a misleading name and description is redundant with Chain::findBlock. So it would be nice to see those changes in another PR.

I still think the changes here dropping the Chain::getTipLocator and Chain::getActiveChainLocator methods would be good to apply. The first method is unused and since #25717, the second method has a misleading name and description is redundant with Chain::findBlock. So it would be nice to see those changes in another PR.

@ryanofsky, @furszy, done in #33169.