decred / dcrwallet

A secure Decred wallet daemon written in Go (golang).
https://decred.org
ISC License
212 stars 153 forks source link

chain: Wait for dcrd to sync before starting RPC sync #2340

Closed matheusd closed 3 months ago

matheusd commented 4 months ago

This prevents the wallet from syncing from blockConnected messages when it is connected to a dcrd instance that is not yet as synced as the one the wallet has used in the past. This improves performance by batching the sync work in the initial getHeaders stage and prevents wallet misuse by ensuring the underlying dcrd is also synced to recent blocks.

Note that the the case of a new wallet (with recorded tip height 0) connected to a network isolated dcrd instance (header and block height both 0) is explicitly allowed to proceed without waiting. This handles the use-cases of airgapped and empty simnet wallets.

jrick commented 4 months ago

I don't think this is the right way we want to fix this.

The big issue for us is with address discovery. We don't want to have dcrwallet to begin syncing to a dcrd that is not synced with the network, as it may not find all addresses for the rescan.

However, before that point, it is perfectly fine to sync headers as long as we are not increasing the block that we have marked synced transactions with.

Waiting for an out-of-date dcrd to catch up to the wallet, but perhaps not necessarily the network, will still trigger the address discovery issue.

dcrd has a pretty good idea about when it is "current" or not. We should probably look at a way to plumb that information to dcrwallet, while syncing only headers before that point.

matheusd commented 4 months ago

dcrd has a pretty good idea about when it is "current" or not. We should probably look at a way to plumb that information to dcrwallet, while syncing only headers before that point.

That's the initialblockdownload field in the getblockchaininfo call (here).

I intentionally did not take that into account here because it has a "blocks are less than 2 hours old" heuristic to consider itself synced when there are no peers, and I thought that would conflict with offline wallet scenarios, but I can add that as an additional check in waitRPCSync.

Ideally, we'd have a --nosync mode that correctly handles the offline wallet scenario, so maybe that should be an issue to be handled separately?

matheusd commented 4 months ago

Pushed the change which makes it track dcrd's IsCurrent().

But as I mentioned, this makes offline wallets keep looping in the "wainting dcrd sync" stage. I could add a special case to disregard the IBS flag when blocks == headers == wallet height == 0 or I could work on the --nosync mode (though that will take a bit longer).

What do you think?

jrick commented 3 months ago

A --nosync or --offline or similar would be best. Otherwise even if everything else works properly, we'd be endlessly spamming failed connection attempts in the log.

matheusd commented 3 months ago

Ok, will work on adding --offline.

matheusd commented 3 months ago

Updated with commit to add --offline

matheusd commented 3 months ago

Fixed commit message.