wallet: avoid rescans under assumed-valid blocks #23997

jamesob · 2022-01-07T04:30:10Z

This is part of the assumeutxo project (parent PR: #15606)

Refuse to load a wallet if it requires a rescan lower than the height of assumed-valid blocks.

Of course in live code right now, BLOCK_ASSUMED_VALID block index entries don't exist since they're a unique flag introduced by the use of UTXO snapshots, so this is prophylactic code exercised only by unittests.

luke-jr · 2022-01-07T07:00:18Z

Why would this make sense? If any blocks are assumed valid, not only them, but also every block afterward is also dubious.

The correct behaviour would seem to be to work normally, and refuse to show anything as confirmed until all blocks it depends on have been verified valid.

Sjors · 2022-01-07T10:22:30Z

@luke-jr IIUC this PR deals with a situation where a rescan is physically impossible, whereas you're referring to whether it's safe.

We could add a warning when a user tries to load any wallet before the background sync is finished. We also need some way to warn the user for this when they create a new wallet. Showing everything as unconfirmed would be one approach, but maybe it's better to show the transactions as confirmed, but still have a general warning about the background sync. Unconfirmed transactions might give users the wrong idea about e.g. the ability to fee bump or abandon them.

@jamesob CI is not happy

jamesob · 2022-01-07T15:33:50Z

Why would this make sense?

As @Sjors notes, the rescan is physically impossible if a block index entry is marked BLOCK_ASSUMED_VALID because it lacks the block data on disk, similar to pruning.

achow101 · 2022-01-10T21:59:42Z

Since this check is basically the same check that pruning does, why don't we just make the pruning check always happen and adjust the error message to be more generic? It would really just be removing the if (chain.havePruned()) condition, and there would be no need to add getLowestBlockDataHeight.

DrahtBot · 2022-01-14T19:07:02Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25494 (indexes: Stop using node internal types by ryanofsky)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

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.

jamesob · 2022-01-15T02:58:23Z

Since this check is basically the same check that pruning does, why don't we just make the pruning check always happen and adjust the error message to be more generic? It would really just be removing the if (chain.havePruned()) condition, and there would be no need to add getLowestBlockDataHeight.

Good suggestion. I've made this change (though still distinguish the two cases in the error message) and have improved the test.

Sjors · 2022-01-16T11:18:28Z

2ab917a deserves it's own PR. It's also what's causing the tests to fail, and nodes to crash when you shut them down after creating a new wallet(?).

jamesob · 2022-01-18T14:00:23Z

2ab917a deserves it's own PR. It's also what's causing the tests to fail, and nodes to crash when you shut them down after creating a new wallet(?).

Yep... I'm going to avoid attempting that fix here and instead scale the tests back.

jamesob · 2022-02-11T18:43:38Z

I've removed the test from this change since, as far as I can tell, it's impossible to unittest this behavior without leaking the wallet database. This happens because CWallet::Create eats the database unique_ptr into a shared_ptr and then doesn't return the shared_ptr upon failure.

src/wallet/wallet.cpp

Refuse to load a wallet if it requires a rescan lower than the height of an unvalidated snapshot we're running -- in more general terms, if we don't have data for the blocks.

achow101 · 2022-03-23T18:01:45Z

ACK 817326a

jamesob · 2022-06-06T13:59:26Z

Ready for merge?

maflcko

left a question

src/node/interfaces.cpp

maflcko · 2022-06-06T14:26:27Z

src/wallet/wallet.cpp

+                     _(
+                        "Assumed-valid: last wallet synchronisation goes beyond "
+                        "available block data. You need to wait for the background "
+                        "validation chain to download more blocks.") :


This doesn't seem actionable for the user? The user would have to disable the wallet, then restart without wallet, then wait for the background chainstate to catch up, then start again? And with pruning enabled they'd just trade this error with the other one, no?

I guess the only way this could happen is if the users loads a wallet from another datadir/backup? In that case it doesn't seem too bad to just do a full rescan along with the background download?

This error won't stop the node though, right? Won't the background validation process just continue after the loadwallet call has failed via this error? Maybe @achow101 @ryanofsky can clarify.

I haven't tested this, but this seems like a fatal error. Or rather, if it wasn't a fatal error, it should be one, based on the wording:

error = _("Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of pruned node)");

(I guess it would be nice if someone added a test for the prune case).

At least in these cases, the error is not fatal AFAICT:

when called from WalletLoader::createWallet() (in wallet/interfaces.cpp): https://github.com/jamesob/bitcoin/blob/801aaac2b39564aa14009785146ba26d2506fb53/src/wallet/wallet.cpp#L227-L233

when called from loadwallet RPC: https://github.com/jamesob/bitcoin/blob/801aaac2b39564aa14009785146ba26d2506fb53/src/wallet/rpc/wallet.cpp#L224-L226

Fair enough. (I'd still prefer if the wallet locator was automatically reset further back instead of asking the user to "wait manually".)

This would be fatal if the wallet was loaded on start. But via loadwallet or through the GUI, it will just return an error.

jonatack

Concept ACK

src/node/interfaces.cpp

src/wallet/wallet.cpp

maflcko · 2022-06-07T15:56:49Z

src/wallet/wallet.cpp

                // If a block is pruned after this check, we will load the wallet,
                // but fail the rescan with a generic error.
-                error = _("Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of pruned node)");
+
+                error = chain.hasAssumedValidChain() ?


Shouldn't the prune error overwrite the au error?

If prune is enabled, downloading au data won't help, as it will be pruned.

Suggested change

error = chain.hasAssumedValidChain() ?

error = chain.havePruned() ? "prune err" : "au err";

Ah, this is a good point.

Agree with all of Marco's points here and think this should be updated

If havePrune and hasAssumedValidChain are both true, better to show havePrune error message

Assumed-valid error message is vague and not very actionable. Would suggest "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 though assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height {block_height}"

ryanofsky

Code review ACK 817326a. This seems like the simplest change we can make to avoid wallet problems when an assumeutxo snapshot is loaded.

There are other improvements that could be made to the wallet after this to make it work better with assumeutxo snapshot loading. Wallet could rescan from the background chain and scan background blocks as they are downloaded. With the attachChain method in #24230, this would not require any wallet-specific assumeutxo code. Wallet could just passively receive block connected notifications it needs in order

ryanofsky · 2022-06-14T17:02:30Z

src/wallet/wallet.cpp

                // If a block is pruned after this check, we will load the wallet,
                // but fail the rescan with a generic error.
-                error = _("Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of pruned node)");
+
+                error = chain.hasAssumedValidChain() ?


Agree with all of Marco's points here and think this should be updated

If havePrune and hasAssumedValidChain are both true, better to show havePrune error message

Assumed-valid error message is vague and not very actionable. Would suggest "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 though assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height {block_height}"

achow101 · 2022-06-14T23:02:28Z

Thinking on this again, I'm actually not sure that there even needs to be an error. With assume utxo, we are background downloading and verifying the blocks. If the locator is ahead of those blocks, then we will get those blocks eventually and they will be scanned. Conversely, pruning means that those blocks are deleted and we won't be retrieving them automatically.

A big difference between assume utxo and pruning is that with assume utxo we will know the utxos for the wallet, whereas with just pruning we don't. So refusing to load wallets when only pruned makes sense, as funds may be missing, but with assume utxo, I don't think that holds. We can retrieve the utxos from the snapshot and blocks following. If it's good enough for validation, it should be good enough for the wallet. Of course we need to change the wallet to track utxos rather than full transactions, but that's a different problem.

ryanofsky · 2022-06-15T02:27:28Z

Thinking on this again, I'm actually not sure that there even needs to be an error. With assume utxo, we are background downloading and verifying the blocks.

I think because of a assumeutxo implementation detail the wallet won't actually be notified about these blocks and won't scan them.

But even if the wallet were notified about background blocks I assume it still need to be an error if any blocks are missing at startup because wallet expects to scan blocks in order. So even if background blocks were processed and added incoming transactions to the wallet, later blocks would still need to be rescanned so ~~wallet could see if those transactions were spent~~ (EDIT: this isn't really the right reason, but I think there are cases where wallet needs to scan blocks in order).

An alternative to this PR would make the wallet receive background block notifications and rescan more smartly, but just showing an error like this PR does is the simplest possible behavior that is not buggy

jamesob · 2022-07-18T17:51:02Z

I agree with @ryanofsky's assessment - this seems like the simplest change to avoid broken wallet behavior. I think anything beyond that is going to be a much more substantial change.

Is there anything else holding this PR up from further review/merge?

817326a wallet: avoid rescans if under the snapshot (James O'Beirne) Pull request description: This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: bitcoin#15606) --- Refuse to load a wallet if it requires a rescan lower than the height of assumed-valid blocks. Of course in live code right now, `BLOCK_ASSUMED_VALID` block index entries don't exist since they're a unique flag introduced by the use of UTXO snapshots, so this is prophylactic code exercised only by unittests. ACKs for top commit: achow101: ACK 817326a ryanofsky: Code review ACK 817326a. This seems like the simplest change we can make to avoid wallet problems when an assumeutxo snapshot is loaded. Tree-SHA512: cfa44b2eb33d1818d30df45210d0dde1e9b78cc9b7c88cb985054dc28427bba9e0905debe4196065d1d3a5ce7bca7e605e629d5ce5f0225b25395746e6d3d596

maflcko · 2022-07-19T07:07:47Z

I think you forgot to toggle the boolean check? Currently it seems the wrong way around, as commented in #23997 (comment)

jamesob · 2022-07-19T13:08:56Z

I think you forgot to toggle the boolean check? Currently it seems the wrong way around, as commented in #23997 (comment)

Argh, my mistake. I'll file a follow-up.

maflcko · 2022-08-02T17:08:24Z

Argh, my mistake. I'll file a follow-up.

Let us know if you don't, so that someone else can pick this up.

ryanofsky · 2022-10-06T01:14:52Z

re: #23997 (comment)

Argh, my mistake. I'll file a follow-up.

@jamesob, did you get around to this? It seems like this is still applicable to current code

jamesob · 2022-10-07T19:18:42Z

@ryanofsky @MarcoFalke I'm sorry about that guys, PR is finally up: #26282

… over assumedvalid 1c36baf wallet: have prune error take precedence over assumedvalid (James O'Beirne) Pull request description: Fixes bitcoin/bitcoin#23997 (comment). From Russ Yanofsky: > Agree with all of Marco's points here and think this should be updated > > If havePrune and hasAssumedValidChain are both true, better to show havePrune error message. Assumed-valid error message is vague and not very actionable. Would suggest "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 though assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height {block_height}" ACKs for top commit: MarcoFalke: ACK 1c36baf aureleoules: ACK 1c36baf Tree-SHA512: bfb0024bb962525cbbd392ade3c0331a8b0525e7f2f2ab52b2dbb9b6dd6311070d85ecb762a7689db84a30991971865698ab6fec187206e6a92133790c5a91dc

…sumedvalid 1c36baf wallet: have prune error take precedence over assumedvalid (James O'Beirne) Pull request description: Fixes bitcoin#23997 (comment). From Russ Yanofsky: > Agree with all of Marco's points here and think this should be updated > > If havePrune and hasAssumedValidChain are both true, better to show havePrune error message. Assumed-valid error message is vague and not very actionable. Would suggest "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 though assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height {block_height}" ACKs for top commit: MarcoFalke: ACK 1c36baf aureleoules: ACK 1c36baf Tree-SHA512: bfb0024bb962525cbbd392ade3c0331a8b0525e7f2f2ab52b2dbb9b6dd6311070d85ecb762a7689db84a30991971865698ab6fec187206e6a92133790c5a91dc

fanquake added the Wallet label Jan 7, 2022

jamesob force-pushed the 2022-01-snapshot-rescans branch from 621a2de to eef3e58 Compare January 7, 2022 15:29

DrahtBot mentioned this pull request Jan 14, 2022

wallet: Actually treat (un)confirmed txs as (un)confirmed #24067

Merged

jamesob force-pushed the 2022-01-snapshot-rescans branch from eef3e58 to 99881c2 Compare January 15, 2022 02:55

jamesob force-pushed the 2022-01-snapshot-rescans branch from 99881c2 to 4c25084 Compare January 15, 2022 03:11

DrahtBot mentioned this pull request Jan 15, 2022

assumeutxo #15606

Closed

18 tasks

jamesob force-pushed the 2022-01-snapshot-rescans branch from 4c25084 to 7d51ad9 Compare January 15, 2022 16:45

jamesob force-pushed the 2022-01-snapshot-rescans branch from 7d51ad9 to f4e565d Compare January 18, 2022 18:03

DrahtBot mentioned this pull request Feb 2, 2022

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

jamesob force-pushed the 2022-01-snapshot-rescans branch from f4e565d to 9bf1245 Compare February 11, 2022 18:30

achow101 reviewed Feb 15, 2022

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

wallet: avoid rescans if under the snapshot

817326a

Refuse to load a wallet if it requires a rescan lower than the height of an unvalidated snapshot we're running -- in more general terms, if we don't have data for the blocks.

jamesob force-pushed the 2022-01-snapshot-rescans branch from 9bf1245 to 817326a Compare February 16, 2022 01:50

DrahtBot mentioned this pull request Apr 26, 2022

wallet: ignore chainStateFlushed notifications while attaching chain #24984

Merged

DrahtBot added the Needs rebase label Apr 28, 2022

DrahtBot removed the Needs rebase label May 16, 2022

maflcko reviewed Jun 6, 2022

View reviewed changes

jonatack reviewed Jun 6, 2022

View reviewed changes

src/node/interfaces.cpp Show resolved Hide resolved

src/wallet/wallet.cpp Show resolved Hide resolved

maflcko reviewed Jun 7, 2022

View reviewed changes

ryanofsky approved these changes Jun 14, 2022

View reviewed changes

DrahtBot mentioned this pull request Jun 29, 2022

indexes: Stop using node internal types #25494

Merged

achow101 merged commit 8d4a058 into bitcoin:master Jul 18, 2022

ryanofsky mentioned this pull request Oct 6, 2022

Bugfix: Allow the user to start anyway when loading a wallet errors bitcoin-core/gui#236

Closed

jamesob mentioned this pull request Oct 7, 2022

wallet: have prune error take precedence over assumedvalid #26282

Merged

bitcoin locked and limited conversation to collaborators Aug 7, 2024

	error = chain.hasAssumedValidChain() ?
	error = chain.havePruned() ? "prune err" : "au err";

wallet: avoid rescans under assumed-valid blocks #23997

wallet: avoid rescans under assumed-valid blocks #23997

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!