Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

matheusd
Copy link
Member

@matheusd matheusd commented Mar 4, 2024

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
Copy link
Member

jrick commented Mar 4, 2024

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
Copy link
Member Author

matheusd commented Mar 5, 2024

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
Copy link
Member Author

matheusd commented Mar 5, 2024

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
Copy link
Member

jrick commented Apr 1, 2024

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
Copy link
Member Author

matheusd commented Apr 3, 2024

Ok, will work on adding --offline.

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.
@matheusd
Copy link
Member Author

matheusd commented Apr 4, 2024

Updated with commit to add --offline

Copy link
Member

@jrick jrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good but you have a typo in your commit message (--ofline)

This adds the --offline sync mode to the wallet. This mode allows the
wallet to run without connecting to the network through either SPV or
RPC modes. Calls that do not require access to the network can still be
executed.

This is mostly useful for running wallets which are meant only to
generate addresses and sign transactions without actually tracking
balances, such as air-gapped wallets.

Previously, such wallets required running with an unconnected dcrd
instance or in SPV mode with --spvconnect specified to an address that
would not provide network services, but these methods are prone to be
misused and recent changes have broken their correct behavior.

The offline mode is introduced by writing a custom NetworkBackend
implementation that is guaranteed to never attempt any external
connections.
@matheusd
Copy link
Member Author

matheusd commented Apr 5, 2024

Fixed commit message.

@jrick jrick merged commit 3b70812 into decred:master Apr 5, 2024
2 checks passed
@matheusd matheusd deleted the wait-dcrd-sync branch April 5, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants