wallet: avoid rescans under assumed-valid blocks #23997
Conversation
|
Why would this make sense? If any blocks are assumed valid, not only them, but also every bloc 8000 k 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. |
|
@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 |
621a2de to
eef3e58
Compare
As @Sjors notes, the rescan is physically impossible if a block index entry is marked |
|
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 |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
eef3e58 to
99881c2
Compare
Good suggestion. I've made this change (though still distinguish the two cases in the error message) and have improved the test. |
99881c2 to
4c25084
Compare
4c25084 to
7d51ad9
Compare
|
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. |
7d51ad9 to
f4e565d
Compare
f4e565d to
9bf1245
Compare
|
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 |
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.
9bf1245 to
817326a
Compare
|
ACK 817326a |
|
Ready for merge? |
| _( | ||
| "Assumed-valid: last wallet synchronisation goes beyond " | ||
| "available block data. You need to wait for the background " | ||
| "validation chain to download more blocks.") : |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
loadwalletRPC: https://github.com/jamesob/bitcoin/blob/801aaac2b39564aa14009785146ba26d2506fb53/src/wallet/rpc/wallet.cpp#L224-L226
There was a problem hiding this comment.
Fair enough. (I'd still prefer if the wallet locator was automatically reset further back instead of asking the user to "wait manually".)
There was a problem hiding this comment.
This would be fatal if the wallet was loaded on start. But via loadwallet or through the GUI, it will just return an error.
| // 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() ? |
There was a problem hiding this comment.
Shouldn't the prune error overwrite the au error?
If prune is enabled, downloading au data won't help, as it will be pruned.
| error = chain.hasAssumedValidChain() ? | |
| error = chain.havePruned() ? "prune err" : "au err"; |
There was a problem hiding this comment.
Ah, this is a good point.
There was a problem hiding this comment.
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}"
There was a problem hiding this comment.
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
| // 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() ? |
There was a problem hiding this comment.
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}"
|
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. |
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 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 |
|
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
|
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. |
Let us know if you don't, so that someone else can pick this up. |
|
re: #23997 (comment)
@jamesob, did you get around to this? It seems like this is still applicable to current code |
|
@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
…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
…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
…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
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_VALIDblock 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.