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

wallet+waddrmgr: sync and store up to MaxReorgDepth blocks #618

Merged
merged 12 commits into from Jun 14, 2019

Conversation

@wpaulino
Copy link
Contributor

@wpaulino wpaulino commented May 10, 2019

In this PR, we modify the wallet's block hash index to only store up to MaxReorgDepth blocks. This allows us to reduce consumed storage, as we'd be mostly storing duplicate data. We choose to store up to MaxReorgDepth to ensure we can recover from a potential long reorg.

We also add a migration that will be used by existing wallets to ensure they can adhere to the new requirement of storing up to MaxReorgDepth entries within the block hash index.

As a result of only storing up to MaxReorgDepth, we can also speed up the initial sync process for the wallet as we can just fetch the latest MaxReorgDepth blocks of the chain.

Depends on #617 to address some bitcoind issues.
Partly addresses #513, except we do not batch the requests to the chain backend.
Fixes #616.

NOTE: Since this is changing the initial sync logic, we should make sure to test manually and also through the set of tests within lnd to ensure there are no regressions.

@wpaulino wpaulino force-pushed the instant-wallet-startup branch 2 times, most recently from bb6526d to 72094a3 May 10, 2019
@Roasbeef
Copy link
Member

@Roasbeef Roasbeef commented May 11, 2019

@wpaulino wpaulino force-pushed the instant-wallet-startup branch 3 times, most recently from ad0969c to 7c0f376 May 12, 2019
@halseth
Copy link
Contributor

@halseth halseth commented May 12, 2019

for reference: #555

Copy link
Contributor

@halseth halseth left a comment

Super stoked about this PR! This will be a massive improvement in startup times, and I also find the code easier to reason about than before 😁

waddrmgr/migrations.go Show resolved Hide resolved
wallet/wallet.go Show resolved Hide resolved
wallet/wallet.go Outdated Show resolved Hide resolved
wallet/wallet.go Outdated Show resolved Hide resolved
wallet/chainntfns.go Outdated Show resolved Hide resolved
wallet/chainntfns.go Outdated Show resolved Hide resolved
Copy link
Contributor

@cfromknecht cfromknecht left a comment

overall approach looks pretty solid, some minor comments left inline

chain/bitcoind_client.go Outdated Show resolved Hide resolved
waddrmgr/db.go Outdated Show resolved Hide resolved
waddrmgr/migrations.go Show resolved Hide resolved
@wpaulino wpaulino force-pushed the instant-wallet-startup branch from 7c0f376 to b134eac May 25, 2019
@wpaulino
Copy link
Contributor Author

@wpaulino wpaulino commented May 25, 2019

Pushed a significant rewrite that addresses some comments @cfromknecht and I discussed offline where it was possible for us to miss scanning relevant blocks if the birthday was before the reorg safe height. I also included some nice refactors along the way.

PTAL @cfromknecht @halseth!

Copy link
Contributor

@cfromknecht cfromknecht left a comment

@wpaulino latest diff looking solid, dig the recovery refactor as well 👌

wallet/wallet.go Show resolved Hide resolved
wallet/rescan.go Show resolved Hide resolved
@wpaulino wpaulino force-pushed the instant-wallet-startup branch 2 times, most recently from e69047d to 17c350a May 30, 2019
@wpaulino wpaulino force-pushed the instant-wallet-startup branch 2 times, most recently from be199c9 to 3043579 Jun 1, 2019
Copy link
Contributor

@cfromknecht cfromknecht left a comment

LGTM 🌪 tested and indeed sped up the wallet initial sync!!

wallet/wallet.go Show resolved Hide resolved
wallet/wallet.go Show resolved Hide resolved
wallet/wallet.go Outdated Show resolved Hide resolved
@wpaulino wpaulino force-pushed the instant-wallet-startup branch from 3043579 to c3400a3 Jun 10, 2019
wallet/wallet.go Show resolved Hide resolved
wallet/wallet.go Show resolved Hide resolved
wallet/wallet.go Show resolved Hide resolved
wallet/wallet.go Show resolved Hide resolved
wallet/wallet.go Outdated Show resolved Hide resolved
wallet/wallet.go Outdated Show resolved Hide resolved
wpaulino added 2 commits Jun 11, 2019
In this commit, we modify the wallet's block hash index to only store up
to MaxReorgDepth blocks. This allows us to reduce consumed storage, as
we'd be mostly storing duplicate data. We choose to store up to
MaxReorgDepth to ensure we can recover from a potential long reorg.
In this commit, we add a migration that will be used by existing wallets
to ensure they can adhere to the new requirement of storing up to
MaxReorgDepth entries within the block hash index.
@wpaulino wpaulino force-pushed the instant-wallet-startup branch 2 times, most recently from 1c17ac5 to cd76787 Jun 11, 2019
Copy link
Contributor

@halseth halseth left a comment

Alright, this more or less looks good to me now 👍 Only a few questions and nits, once those are addressed I think this one is good to go.

wallet/wallet.go Outdated

// Clear the batch of all processed blocks to reuse the same
// memory for future batches.
blocks = blocks[:0]
Copy link
Contributor

@halseth halseth Jun 13, 2019

are blocks actually used anywhere?

Copy link
Contributor Author

@wpaulino wpaulino Jun 13, 2019

They're used in the database transaction above.

wallet/wallet.go Outdated Show resolved Hide resolved
wallet/wallet.go Outdated Show resolved Hide resolved
"%v", err)
}

c.bestBlock.Hash = *bestHash
Copy link
Contributor

@halseth halseth Jun 13, 2019

no mutex needed here?

Copy link
Contributor Author

@wpaulino wpaulino Jun 13, 2019

Fixed.

Copy link
Contributor

@cfromknecht cfromknecht Jun 14, 2019

still haz mutex?

Copy link
Contributor Author

@wpaulino wpaulino Jun 14, 2019

It's meant to be there since BlockStamp can be called at any point, which acquires the lock.

chain/bitcoind_client.go Outdated Show resolved Hide resolved
@wpaulino wpaulino force-pushed the instant-wallet-startup branch from cd76787 to 89dd45a Jun 13, 2019
wallet/wallet.go Show resolved Hide resolved
wallet/wallet.go Outdated Show resolved Hide resolved
if err != nil {
return err
}
blocks = append(blocks, &waddrmgr.BlockStamp{
Copy link
Contributor

@halseth halseth Jun 13, 2019

should blocks before our birthday be added to the slice? If yes, add comment what this achieves.

Copy link
Contributor Author

@wpaulino wpaulino Jun 13, 2019

There's already a comment at the top of the loop explaining the rationale?

Since the recovery process itself acts as rescan, we'll also update our wallet's synced state along the way to reflect the blocks we process and prevent rescanning them later on.

@wpaulino wpaulino force-pushed the instant-wallet-startup branch from 89dd45a to e98f97c Jun 13, 2019
Copy link
Contributor

@cfromknecht cfromknecht left a comment

lgtm, just some minor comments

wallet/rescan.go Show resolved Hide resolved
"%v", err)
}

c.bestBlock.Hash = *bestHash
Copy link
Contributor

@cfromknecht cfromknecht Jun 14, 2019

still haz mutex?

wpaulino added 10 commits Jun 14, 2019
This ensures the wallet can properly do an initial sync, a recovery, or
detect if it's on a stale branch before attempting to process new blocks
at tip.

Since the rescan will be triggered synchronously as well, we'll need to
catch the wallet's quit chan when handling rescan batches in order to
allow for clean shutdowns.
IsCurrent allows us to determine if the chain backend considers itself
"current" with the chain.
This serves as groundwork for only storing up to MaxReorgDepth blocks
upon initial sync. To do so, we want to make sure the chain backend
considers itself current so that we can only fetch the latest
MaxReorgDepth blocks from it.
We do this as the wallet will no longer store blocks all the way from
genesis to the tip of the chain. Instead, in order to find a reasonable
birthday block, we resort to performing a binary search for a block
timestamp that's within +/-2 hours of the birthday timestamp.
This commit serves as another building point to allow the wallet to not
store blocks all the way from genesis to the tip of chain. We modify the
wallet's recovery logic to now start from either its birthday block, or
the current reorg safe height if it's before the birthday, to ensure the
wallet properly only stores MaxReorgDepth blocks.

We also refactor things a bit in hopes of making the logic a bit more
readable.
Currently, wallet rescans start from its known tip of the chain. Since
we no longer store blocks all the way from genesis to the tip of the
chain, performing a rescan would cause us to scan blocks all the way
from genesis, which we want to avoid. To prevent this, we set the
wallet's tip to be the current reorg safe height. This ensures that
we're unable to scan any blocks before it, and that we maintain
MaxReorgDepth blocks stored.
We use the recently introduced locateBirthdayBlock function within
birthdaySanityCheck as it serves as a more optimized alternative that
achieves the same purpose.
If a block arrives while the wallet is rescanning, users would be shown
a log message that is confusing and not very helpful.
One could argue that the behavior before this commit was incorrect, as
the ChainClient interface expects a call to NotifyBlocks before
notifying blocks at tip, so we decide to fix this.

Since we now wait for the chain backend to be considered "current"
before proceeding to sync the wallet with it, any blocks that were
processed while waiting would result in being notified and scanned
twice, once by processing it at tip, and another while rescanning the
wallet, which is not desirable.
@wpaulino wpaulino force-pushed the instant-wallet-startup branch from e98f97c to 4a913d0 Jun 14, 2019
@Roasbeef Roasbeef merged commit a335c56 into btcsuite:master Jun 14, 2019
1 check passed
@wpaulino wpaulino deleted the instant-wallet-startup branch Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants