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.

Sign up for GitHub

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

Jump to bottom

Catch up filter headers and block headers in parallel #98

Merged
merged 7 commits into from
Oct 3, 2018
Merged

Catch up filter headers and block headers in parallel #98

merged 7 commits into from
Oct 3, 2018

Conversation

halseth
Copy link
Contributor

@halseth halseth commented Sep 17, 2018 •
edited
Loading

Catch up filter headers and block headers in parallel.

Also includes a fix for rollback of filter headers, and a proper fix to what was attempted in #95.

@coveralls
Copy link

coveralls commented Sep 17, 2018 •
edited
Loading

Coverage Status

Coverage decreased (-1.02%) to 69.478% when pulling ee90510 on halseth:parallel-header-catchup into 29105cb on lightninglabs:master.

Roasbeef
Roasbeef requested changes Sep 19, 2018
View reviewed changes
blockmanager.go Outdated Show resolved Hide resolved
blockmanager.go Outdated Show resolved Hide resolved
@halseth halseth mentioned this pull request Sep 20, 2018
Closed
5 tasks
@halseth halseth force-pushed the parallel-header-catchup branch 3 times, most recently from 90bea83 to fa55fcf Compare September 20, 2018 13:13
@halseth halseth changed the title Proof-of-concept: Catch up filter headers and block headers in parallel Catch up filter headers and block headers in parallel Sep 20, 2018
cfromknecht
cfromknecht reviewed Sep 21, 2018
View reviewed changes
blockmanager.go Outdated
b.newHeadersSignal.L.Lock()
for !b.IsCurrent() {
for !(b.IsCurrent() || b.filterHeaderTip+wire.CFCheckptInterval <= b.headerTip) {
Copy link
Contributor

Choose a reason for hiding this comment

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

already discussed irl, but it might be worth reversing the order (again) to avoid db hits amap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reversed.

@halseth
Copy link
Contributor Author

halseth commented Sep 21, 2018

Now waits 100 CF intervalse before starting catch up.

@halseth halseth force-pushed the parallel-header-catchup branch 3 times, most recently from 6aa7b26 to 973863b Compare September 24, 2018 12:13
@halseth
Copy link
Contributor Author

halseth commented Sep 24, 2018

Reverted back to waiting only one interval. Instead the latest commit will fetch the filter checkpoints up to the latest block checkpoint, avoiding doing this fetch every time the block headers advance. This means that we will reuse these checkpoints until blockeaders > chain_checkpoint, at which point we will fetch new checkpoints.

This was referenced Sep 25, 2018
Merged
Merged
Roasbeef
Roasbeef requested changes Sep 26, 2018
View reviewed changes
blockmanager.go
@@ -225,7 +226,14 @@ func newBlockManager(s *ChainService) (*blockManager, error) {
if err != nil {
return nil, err
}
bm.filterHeaderTipHash = header.BlockHash()

// We must also ensure the the filter header tip hash is set to the
Copy link
Member

Choose a reason for hiding this comment

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

👍

blockmanager.go Outdated Show resolved Hide resolved
blockmanager.go Show resolved Hide resolved
blockmanager.go Outdated Show resolved Hide resolved
blockmanager.go Show resolved Hide resolved
@Roasbeef
Copy link
Member

Tested this locally with multiple syncs from both a remote and local node, didn't run into any major issues. Ensured that it was able to proceed after forced restarts, and seemed to handle the started no problem. However, I think there're a few places where a quit signal isn't being properly threaded through, as I noticed after some attempts, I was unable to kill the process.

@halseth
Copy link
Contributor Author

halseth commented Sep 26, 2018

When do you experience you are unable to kill the process?

Haven't been able to reproduce it myself, but a hunch I had was that one of the loops where we are waiting for the newHeadersSignal would stall. However, in the block manager stop method we will repeatedly signal until all goroutines are shut down, so it should be handling that case.

This PR shouldn't change the quit behaviour in any way, but a goroutine dump in the unable-to-kill case should expose the culprit.

Roasbeef
Roasbeef approved these changes Sep 29, 2018
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🥑

Ready to go in after a rebase!

@halseth
neutrino: make sure new filter tip is updated after rollback iteration
6bb57d6
@halseth
blockmanager: set filterHeaderTipHash to correct block hash
10465fe
@halseth
blockmanager: remove filter goroutines.
52efa5c 
Since we are only handling one filter type, we can simplify the code by
making it clearly synchronous.
@halseth
blockmanager: start cfheader sync immediately when lagging a cp inter…
f0333c4 
…val behind

This commit makes the cfhandler start fetching checkpointed filter
headers imemdiately when they are lagging at least a CF checkpoint
interval behind the blockheaders.
@halseth
blockmanager: fetch filter checkpoints up to chain checkpoints
536c78f 
This commit makes the fetching of filter checkpoints go up to the hash
of the latest chain checkpoints in case the block headers haven't caught
up that far.

This helps in terms of avoiding doing a lot of filter checkpoint fetches
up to small heights while the block headers are catching up.
@halseth
blockmanager: sleep in case of error
3fb5213
@halseth
Copy link
Contributor Author

halseth commented Oct 1, 2018

Rebased.

@cfromknecht
Copy link
Contributor

@halseth perhaps the source of the inability to quit while syncing is a result of using Sleep? Might suggest selecting on time.After and quit to ensure we can exit

@halseth
Copy link
Contributor Author

halseth commented Oct 1, 2018

I think it should successfully quit after the sleep timeout is over, but I agree that's cleaner. Will add :)

Roasbeef
Roasbeef approved these changes Oct 3, 2018
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🕺

@Roasbeef Roasbeef merged commit 838f7ba into lightninglabs:master Oct 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfromknecht cfromknecht cfromknecht left review comments

@Roasbeef Roasbeef Roasbeef approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@halseth @coveralls @Roasbeef @cfromknecht