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.
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.
@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
if (chain.havePruned()) condition, and there would be no need to add getLowestBlockDataHeight.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
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 addgetLowestBlockDataHeight.
Good suggestion. I’ve made this change (though still distinguish the two cases in the error message) and have improved the test.
CWallet::Create eats the database unique_ptr into a shared_ptr and then doesn’t return the shared_ptr upon failure.
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.
2902@@ -2903,22 +2903,28 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf
2903
2904 if (tip_height && *tip_height != rescan_height)
2905 {
2906- if (chain.havePruned()) {
in 9bf1245764579302fef8d86fbbe1003fc2bb7de4 “wallet: avoid rescans if under the snapshot”
I think we should retain this check and just expand it to also check hasAssumedValidChain. Otherwise we will run the while loop below every time a wallet is loaded, and that can have a significant effect on loading times.
With a testnet wallet where the rescan_height is set to 0, the wallet took 500 ms to load on master, while it took 5 secs with this PR.
722@@ -723,6 +723,11 @@ class ChainImpl : public Chain
723 notifications.transactionAddedToMempool(entry.GetSharedTx(), 0 /* mempool_sequence */);
724 }
725 }
726+ bool hasAssumedValidChain() override
727+ {
728+ return Assert(m_node.chainman)->IsSnapshotActive();
chainman().IsSnapshotActive()?
chainman references by way of this Chain interface here? Also I think we had a preference to avoid referring to “snapshots” outside of ChainstateManager to the extent possible. cc @ryanofsky
Assert(...) with chainman(), if you retouch.
2927+
2928+ error = chain.hasAssumedValidChain() ?
2929+ _(
2930+ "Assumed-valid: last wallet synchronisation goes beyond "
2931+ "available block data. You need to wait for the background "
2932+ "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?
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:
0error = _("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:
WalletLoader::createWallet() (in wallet/interfaces.cpp): https://github.com/jamesob/bitcoin/blob/801aaac2b39564aa14009785146ba26d2506fb53/src/wallet/wallet.cpp#L227-L233loadwallet RPC: https://github.com/jamesob/bitcoin/blob/801aaac2b39564aa14009785146ba26d2506fb53/src/wallet/rpc/wallet.cpp#L224-L226
loadwallet or through the GUI, it will just return an error.
722@@ -723,6 +723,11 @@ class ChainImpl : public Chain
723 notifications.transactionAddedToMempool(entry.GetSharedTx(), 0 /* mempool_sequence */);
724 }
725 }
726+ bool hasAssumedValidChain() override
727+ {
728+ return Assert(m_node.chainman)->IsSnapshotActive();
729+ }
private rather than public.
See its private neighbor, ChainstateManager& chainman() { return *Assert(m_node.chainman); }. It looks like hasAssumedValidChain() may not only be private but also simplified, if wanted, to use chainman(). Clang 15 doesn’t mind.
0@@ -502,6 +502,10 @@ class ChainImpl : public Chain
1 {
2 private:
3 ChainstateManager& chainman() { return *Assert(m_node.chainman); }
4+ bool hasAssumedValidChain() override
5+ {
6+ return chainman().IsSnapshotActive();
7+ }
8 public:
9 explicit ChainImpl(NodeContext& node) : m_node(node) {}
10@@ -767,10 +771,6 @@ public:
11 notifications.transactionAddedToMempool(entry.GetSharedTx(), 0 /* mempool_sequence */);
12 }
13 }
14- bool hasAssumedValidChain() override
15- {
16- return Assert(m_node.chainman)->IsSnapshotActive();
17- }
hasAssumedValidChain is called by CWallet::AttachChain(), which is itself private
hasAssumedValidChain is called by CWallet::AttachChain(), which is itself private
private is used to mark a symbol “private” to a class. For example, AttachChain can be private to CWallet, meaning only code in the scope of CWallet (aka member functions) can call AttachChain.
If a function is called from outside its class, it should not be marked private. Even if adding private happens to compile due to language design trade-offs.
Sure, but CWallet isn’t outside, as it inherits from public Chain::Notifications, an “is a” relationship?
private also forbids derived classes from access. This should be explained in any resource that explains C++ access modifiers.
You can also see with a simple diff that this can’t be private:
0wallet/wallet.cpp:2998:41: error: 'hasAssumedValidChain' is a private member of 'interfaces::Chain'
1 if (chain.havePruned() || chain.hasAssumedValidChain()) {
2 ^
3./interfaces/chain.h:97:18: note: implicitly declared private here
4 virtual bool hasAssumedValidChain() = 0;
0diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
1index c2babaaa46..11bca0706a 100644
2--- a/src/interfaces/chain.h
3+++ b/src/interfaces/chain.h
4@@ -95,2 +95,4 @@ class Chain
5 {
6+ //! Return true if an assumed-valid chain is in use.
7+ virtual bool hasAssumedValidChain() = 0;
8 public:
9@@ -285,5 +287,2 @@ public:
10 virtual void requestMempoolTransactions(Notifications& notifications) = 0;
11-
12- //! Return true if an assumed-valid chain is in use.
13- virtual bool hasAssumedValidChain() = 0;
14 };
In the virtual base class, no. In the derived child the implementation method can be private for both clang and gcc; why?
Anyway, as a reference to refer to if useful here, 693414d2718 added+used the chainman() accessor in the ChainImpl class.
0--- a/src/node/interfaces.cpp
1+++ b/src/node/interfaces.cpp
2@@ -769,7 +769,7 @@ public:
3 }
4 bool hasAssumedValidChain() override
5 {
6- return Assert(m_node.chainman)->IsSnapshotActive();
7+ return chainman().IsSnapshotActive();
8 }
In the virtual base class, no. In the derived child the implementation method can be private for both clang and gcc; why?
Found some answers to this. Separation of interface from implementation, and virtual functions exist to allow customization; unless they also need to be invoked directly from within derived classes’ code, there’s no need to ever make them anything but private.
Herb Sutter in http://www.gotw.ca/publications/mill18.htm: “Make virtual functions private, and only if derived classes need to invoke the base implementation of a virtual function, make the virtual function protected. Prefer to make interfaces non-virtual using the Template pattern.”
https://en.cppreference.com/w/cpp/language/virtual: “Base::vf does not need to be accessible or visible to be overridden. (Base::vf can be declared private, or Base can be inherited using private inheritance)”
“A (virtual) implementation member should be protected if it provides an operation or data that a derived class will need to use in its own implementation. Otherwise, implementation members should be private.” - C++ Primer (Lippman, Lajoie, Moo)
Interesting, it doesn’t load on my phone but from my computer with Firefox, it does. Maybe due to not being https.
Yes, the child builds when private but not the base vf, due to the design I guess. I’m going to re-review my changes in #24150 in light of what I’m learning.
2928+ error = chain.hasAssumedValidChain() ?
2929+ _(
2930+ "Assumed-valid: last wallet synchronisation goes beyond "
2931+ "available block data. You need to wait for the background "
2932+ "validation chain to download more blocks.") :
2933+ _("Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of pruned node)");
nit while touching this line
0 _("Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of pruned node.)");
2923+ // all block data is available.
2924 // If a block is pruned after this check, we will load the wallet,
2925 // but fail the rescan with a generic error.
2926- error = _("Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of pruned node)");
2927+
2928+ 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.
0 error = chain.havePruned() ? "prune err" : "au err";
Agree with all of Marco’s points here and think this should be updated
Code review ACK 817326a828d6148dc63d9ef08f641b9c0c522411. 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
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.
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
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?
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.
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)
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