wallet: avoid rescans under assumed-valid blocks #23997

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.

Good catch, thanks.

nit: Any reason to not use the existing chainman().IsSnapshotActive()?

I'm not sure I follow here - isn't wallet code supposed to be insulated from 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

Oh, I just meant to replace the Assert(...) with chainman(), if you retouch.

Fixed in #25638 for the whole file

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.

This can be private rather than public.

Why would that be? This is part of the public Chain interface and is called by wallet code.

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.

@@ -502,6 +502,10 @@ class ChainImpl : public Chain
 {
 private:
     ChainstateManager& chainman() { return *Assert(m_node.chainman); }
+    bool hasAssumedValidChain() override
+    {
+        return chainman().IsSnapshotActive();
+    }
 public:
     explicit ChainImpl(NodeContext& node) : m_node(node) {}
@@ -767,10 +771,6 @@ public:
             notifications.transactionAddedToMempool(entry.GetSharedTx(), 0 /* mempool_sequence */);
         }
     }
-    bool hasAssumedValidChain() override
-    {
-        return Assert(m_node.chainman)->IsSnapshotActive();
-    }

I agree with jamesob. Just because a diff compiles, doesn't mean it makes sense.

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?

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:

wallet/wallet.cpp:2998:41: error: 'hasAssumedValidChain' is a private member of 'interfaces::Chain'
        if (chain.havePruned() || chain.hasAssumedValidChain()) {
                                        ^
./interfaces/chain.h:97:18: note: implicitly declared private here
    virtual bool hasAssumedValidChain() = 0;

diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
index c2babaaa46..11bca0706a 100644
--- a/src/interfaces/chain.h
+++ b/src/interfaces/chain.h
@@ -95,2 +95,4 @@ class Chain
 {
+    //! Return true if an assumed-valid chain is in use.
+    virtual bool hasAssumedValidChain() = 0;
 public:
@@ -285,5 +287,2 @@ public:
     virtual void requestMempoolTransactions(Notifications& notifications) = 0;
-
-    //! Return true if an assumed-valid chain is in use.
-    virtual bool hasAssumedValidChain() = 0;
 };

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.

--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -769,7 +769,7 @@ public:
     }
     bool hasAssumedValidChain() override
     {
-        return Assert(m_node.chainman)->IsSnapshotActive();
+        return chainman().IsSnapshotActive();
     }

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)

The page from Canada doesn't load for me and making the virtual function private doesn't compile for me (see above). If making the overridden function private is a goal, I'd say it should be done for all of them for consistency, and not only for the one added here.

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.

nit while touching this line

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

This will also invalidate the translation

Thanks, good point

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.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}"