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/wallet: redefine initial sync to birthday block not being set #577

Merged
merged 4 commits into from Jan 23, 2019

Conversation

Projects
None yet
5 participants
@wpaulino
Copy link
Contributor

wpaulino commented Nov 26, 2018

In this commit, we modify our initial sync assumption to depend on
whether the wallet's birthday block has been set. We can use this
assumption to prevent costly rescans from the birthday block upon
restarts if there are no UTXOs that exist within the wallet.

@roeierez

This comment has been minimized.

Copy link
Contributor

roeierez commented Nov 27, 2018

@wpaulino thanks! and following our discussion here lightningnetwork/lnd#2215, I tested and it solves the problem.
One bug, that is not introduced by this commit, is that if the first run during "initialSync" has passed the birthday and was stopped before starting the rescan, then on the next start, blocks in the range between the birthday to the best block known will be missed.
I think a good way to solve it is to better define the initialSync to be syncToBirthday and then trigger a rescan always from the known best block.
Here is a diff that is meant to show what I mean and hope can help in shaping the solution:
roeierez@bfa2e60

@wpaulino

This comment has been minimized.

Copy link
Contributor Author

wpaulino commented Dec 20, 2018

@roeierez agree that this is a good approach to go forward with! I think it merits its own PR though as this one aims to solve a different problem. Let me know if you'd like to propose the change yourself, otherwise I can take it over.

One thing that jumps out to me in your diff is that syncWithChain should be completely refactored to only sync until the birthday and not go past that since there's logic to catch up to the tip of the chain while the rescan is ongoing as well.

err = catchUpHashes(w, chainClient, n.Height)

@roeierez

This comment has been minimized.

Copy link
Contributor

roeierez commented Dec 21, 2018

Let me know if you'd like to propose the change yourself, otherwise I can take it over.

I will be happy to introduce a new PR for this and we can take it from there.

syncWithChain should be completely refactored to only sync until the birthday and not go past that

Agree! This is also the reason I was suggesting rename to "syncToBirthday".

@wpaulino If you are fine with this, I am going to start working on a proposed solution.

@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Dec 24, 2018

then on the next start, blocks in the range between the birthday to the best block known will be missed.

By missed, do you mean missed by the rescan, or that the "initial catch up loop" will miss those blocks? If it's the latter, then this would only be an issue in the case of deep re-orgs.

@roeierez

This comment has been minimized.

Copy link
Contributor

roeierez commented Dec 24, 2018

By missed, do you mean missed by the rescan, or that the "initial catch up loop" will miss those blocks?

I mean missing by the rescan, in that case it will only rescan from tip as I see here: https://github.com/btcsuite/btcwallet/blob/master/wallet/wallet.go#L696

@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Dec 24, 2018

"Tip" in that case is the wallet's synced to height, so if we restart right after the wallet's birthday has been passed, then we'll being from that point onwards.

@roeierez

This comment has been minimized.

Copy link
Contributor

roeierez commented Dec 24, 2018

I understand that and I agree that If the restart happened right after the wallet's birthday then we indeed don't have a problem. I was referring to the case where there is a gap between the birthday stamp and the point where the "initial sync" has reached.
if the birthday is block N then on regular start (without stopping) the rescan will indeed start from block N (as I see here: https://github.com/btcsuite/btcwallet/blob/master/wallet/wallet.go#L693).
But if the first start the "initial sync" has reached "N + 100", resulting in updating the Manager.SyncedTo() to N + 100, and then restarted the rescan will start from "N + 100" skipping 100 blocks (as I see here: https://github.com/btcsuite/btcwallet/blob/master/wallet/wallet.go#L696)
Do you think I am missing something?

@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Dec 24, 2018

Ahh ok, thanks for that example, I get where you're describing here now.

We might as well fix the bug since this commit makes it a bit more apparent. If you throw up a commit somewhere fixing it (possibly with that refactor mentioned above, as refactoring in this section of the code is looong over due), we can cherry pick it into this PR as we can test both of them as a hole. Thanks for pointing this out!

@roeierez

This comment has been minimized.

Copy link
Contributor

roeierez commented Jan 3, 2019

@wpaulino as @Roasbeef has asked I pushed my changes here: master...roeierez:sync
I made several commits so you can see the changes clearly enough as the last commit extract out only the initial sync logic to its own function.
The initial sync now stops at the birthday stamp and one thing I wasn't sure about is whether it holds fine also for the recovery mode. I think it is because the code triggers a rescan later from the updated tip but I figured it is good to point to mention here.
I still didn't tested it properly as first I would like to know if this is what you had in mind.
Any kind feedback is very welcome and appreciated.

@wpaulino wpaulino force-pushed the wpaulino:initial-sync-birthday-block branch 2 times, most recently from c161208 to 9a9f9aa Jan 8, 2019

@wpaulino

This comment has been minimized.

Copy link
Contributor Author

wpaulino commented Jan 8, 2019

@roeierez thanks for getting this started! We plan to get this in ASAP for our next release, so I'll be covering the rest from here. I made sure to provide you with credit on the relevant commits. The only major thing missing from your branch was modifying the recovery logic to maintain its current behavior (add blocks to the recovery manager from the birthday to the tip of the chain).

@Roasbeef this should be ready for review now.

Show resolved Hide resolved wallet/wallet.go
tx, err := w.db.BeginReadWriteTx()
if err != nil {
return nil, err
}

This comment has been minimized.

Copy link
@halseth

halseth Jan 8, 2019

Contributor

should just defer tx.Rollback() here.


// Finally, with the birthday stamp found, we can checkpoint our state
// and return.
if err := tx.Commit(); err != nil {

This comment has been minimized.

Copy link
@halseth

halseth Jan 8, 2019

Contributor

a rollback after commit is safe, so we can defer the rollback :)

Show resolved Hide resolved wallet/wallet.go
return bestHeight >= latestCheckptHeight
}

for height := birthdayStamp.Height; height <= bestHeight; height++ {

This comment has been minimized.

Copy link
@halseth

halseth Jan 8, 2019

Contributor

a lot of the logic is duplicated in these two methods. Maybe we could extract it into something like

// scan to birthday
err := scanChain(genesis, birthday, func(height, hash, header) {
		if header.Timestamp.After(birthday) {
			...
 			err := w.Manager.SetBirthdayBlock(
				ns, *birthdayStamp, true,
			)
		}

 		err = w.Manager.SetSyncedTo(ns, &waddrmgr.BlockStamp{
			Hash:      *hash,
			Height:    height,
			Timestamp: header.Timestamp,
		})

 		// Checkpoint our state every 10K blocks.
		if height%10000 == 0 {
			err := tx.Commit()

 			tx, err = w.db.BeginReadWriteTx()
			if err != nil {
				return nil, err
			}
			ns = tx.ReadWriteBucket(waddrmgrNamespaceKey)
		}
})

// recover funds from birthday
err := scanChain(birthday, nil, func(height, hash, header) {
		recoveryMgr.AddToBlockBatch(hash, height, header.Timestamp)

		if height%recoveryBatchSize == 0 {
			err := w.recoverDefaultScopes(
				chainClient, tx, ns, recoveryMgr.BlockBatch(),
				recoveryMgr.State(),
			)

 			err := tx.Commit()
 			tx, err = w.db.BeginReadWriteTx()
			if err != nil {
				return err
			}
			ns = tx.ReadWriteBucket(waddrmgrNamespaceKey)
		}
})

This comment has been minimized.

Copy link
@Roasbeef
Show resolved Hide resolved wallet/wallet.go
Show resolved Hide resolved wallet/wallet.go
// addresses. The birthdayStamp is used to indicate the starting point of our
// recovery, as it's not possible for us to create any addresses before our
// wallet's birthday.
func (w *Wallet) recovery(birthdayStamp *waddrmgr.BlockStamp) error {

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jan 9, 2019

Member

At a glance a large degree of code duplication exists in this method and the one introduced in the prior commit.

This comment has been minimized.

Copy link
@wpaulino

wpaulino Jan 11, 2019

Author Contributor

Fixed, thanks @halseth for the suggestion!

return bestHeight >= latestCheckptHeight
}

for height := birthdayStamp.Height; height <= bestHeight; height++ {

This comment has been minimized.

Copy link
@Roasbeef
Show resolved Hide resolved wallet/wallet.go
}

// If the rollback happened to go beyond our birthday stamp,
// we'll need to find a new one by syncing with the chain again

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jan 9, 2019

Member

Why do we need to rescan to find another birthday in this case? I can see how the prior logic of using that new roll back height as the birthday isn't very precise, but it avoids this duplicated scanning.

This comment has been minimized.

Copy link
@wpaulino

wpaulino Jan 11, 2019

Author Contributor

I was thinking of the case where a deep reorg puts us significantly behind our birthday, but as these are not common, perhaps it's not worth worrying about. Ended up removing it.

return nil, err
}

tx, err = w.db.BeginReadWriteTx()

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jan 10, 2019

Contributor

also need to defer Rollback on this tx

This comment has been minimized.

Copy link
@wpaulino

wpaulino Jan 11, 2019

Author Contributor

Can no longer defer them as we now use a function closure within scanChain. If we did, we'd end up calling it after every block we fetch.

This comment has been minimized.

Copy link
@halseth

halseth Jan 14, 2019

Contributor

Usually scope issues like this could be solved by not capturing the tx variable when it is first created:

tx, err := w.db.BeginReadWriteTx()
if err != nil {
	return nil, err
}
defer func() {
        tx.Rollback()
}()

but I think maybe that's even more confusing in this case, and we should keep it as is.

This comment has been minimized.

Copy link
@wpaulino

wpaulino Jan 15, 2019

Author Contributor

We still can't defer the Rollback since we're within the function closure, which is called on and terminates after every block.

Show resolved Hide resolved wallet/wallet.go
Show resolved Hide resolved wallet/wallet.go Outdated
ns = tx.ReadWriteBucket(waddrmgrNamespaceKey)
}
}

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jan 10, 2019

Contributor

Since bestHeight is only set at the beginning, we should reread it here in case more blocks have come in while performing this long running rescan. We can use a label to jump back up and execute the above logic if that happens, similar to https://github.com/lightninglabs/neutrino/blob/master/utxoscanner.go#L382

This comment has been minimized.

Copy link
@wpaulino

wpaulino Jan 11, 2019

Author Contributor

Don't think this is really needed as we'll perform a rescan at the end of syncWithChain which will end up catching up to any blocks we missed.

Show resolved Hide resolved wallet/wallet.go Outdated
// Finally, we'll rollback our transaction store to reflect the
// stale state. Rollback unconfirms transactions at and beyond
// the passed height, so add one to the new synced-to height to
// prevent unconfirming transactions from the synced-to block.

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jan 10, 2019

Contributor

perhaps "in" the synced-to block would be clearer?

This comment has been minimized.

Copy link
@wpaulino

wpaulino Jan 11, 2019

Author Contributor

Fixed.

@wpaulino wpaulino force-pushed the wpaulino:initial-sync-birthday-block branch 2 times, most recently from d622288 to 753926d Jan 11, 2019

@wpaulino

This comment has been minimized.

Copy link
Contributor Author

wpaulino commented Jan 11, 2019

Pushed out a new and much cleaner version -- PTAL @Roasbeef @halseth @cfromknecht.

@halseth
Copy link
Contributor

halseth left a comment

Great change! This makes the initial sync logic a lot easier to follow, and hopefully less error-prone.

My only suggestion would be to make it easier to see what actually changed here (other than just refactoring), by doing pure code refactoring in its own commit. Also possible to add a test for this code?

Show resolved Hide resolved wallet/wallet.go
@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Jan 15, 2019

Also possible to add a test for this code?

The tests for this area more or less reside within lnd and indirectly neutrino. The lnwallet integration tests touch much of this area, though it lives outsides of btcwallet (also lnd's integration tests). Generally it's something we need to improve and would be larger undertaking as it's also a major blocker in more aggressively refactoring btcwallet in general.

wpaulino and others added some commits Jan 8, 2019

wallet/wallet: add new syncToBirthday method
In this commit, we add a new syncToBirthday method to the wallet. This
method intends to sync the wallet's point of the view of the chain until
finding its birthday. Most of the logic found within it is heavily
borrowed from the existing syncWithChain method. This method is
currently unused, but it will end up replacing some of the existing sync
logic in a later commit.

Co-authored-by: Roei Erez <roeierez@gmail.com>
wallet/wallet: add new recovery method
In this commit, we add a new recovery method to the wallet. This method
attempts to recover any unspent outputs which pay to any of the wallet's
addresses. Most of the logic found within it is heavily borrowed from
the existing syncWithChain method. This method is currently unused, but
it will end up replacing some of the existing sync logic in a later
commit.
wallet/wallet: use new syncToBirthday and recovery methods
In this commit, we refactor the wallet's syncing logic with
syncWithChain to use the newer, simpler methods: syncToBirthday and
recovery. Along the way, we also fix a bug within the wallet where it
was possible to sync past the birthday, but not sync to tip completely
and restart, which would lead to us starting a rescan from the latest
synced height, rather than from the birthday stamp.

This commit slightly changes the wallet's syncing behavior to the
following:

  1. Ensure the wallet is synced to its birthday.
  2. Perform a recovery if requested.
  3. Check for chain reorgs.
  4. Dispatch a rescan from the current synced height.

Co-authored-by: Roei Erez <roeierez@gmail.com>
wallet/wallet: consolidate rollback logic
In this commit, we consolidate the existing rollback logic to carry out
its duties under one database transaction.

Co-authored-by: Roei Erez <roeierez@gmail.com>

@wpaulino wpaulino force-pushed the wpaulino:initial-sync-birthday-block branch from 753926d to c853cea Jan 15, 2019

@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 🦑

Tested locally on a few nodes, also tested that the existing seed recovery integration tests also pass. Needs a PR at lnd to update to modules to point to the latest version of btcwallet.

@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Jan 23, 2019

I think this is ready to, land, final Q seems to be whether to defer the roll back or not? @wpaulino @halseth ?

@wpaulino

This comment has been minimized.

Copy link
Contributor Author

wpaulino commented Jan 23, 2019

@Roasbeef don't think there's a way of having the defer there - see #577 (comment).

@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Jan 23, 2019

Alrighty, landing then! Thanks to @roeierez for getting this fix started!

@Roasbeef Roasbeef merged commit ba03278 into btcsuite:master Jan 23, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wpaulino wpaulino deleted the wpaulino:initial-sync-birthday-block branch Jan 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.