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 pull redux #1121

Merged
merged 9 commits into from Nov 13, 2019
Merged

Chain pull redux #1121

merged 9 commits into from Nov 13, 2019

Conversation

@mzabaluev
Copy link
Contributor

mzabaluev commented Nov 12, 2019

Re-submitting the chain pull implementation using PushHeaders/PullHeaders.

@mzabaluev mzabaluev requested a review from NicolasDP Nov 12, 2019
@mzabaluev

This comment has been minimized.

Copy link
Contributor Author

mzabaluev commented Nov 12, 2019

I observe trouble here which should also apply to master: sometimes the list of checkpoints is short and it does not go back far enough to get behind a fork to the just announced block.

@mzabaluev mzabaluev marked this pull request as ready for review Nov 12, 2019
mzabaluev added 9 commits Nov 8, 2019
This reverts commit c8be7a6.
This reverts commit a186c9c.
This reverts commit 62453f9.
This reverts commit 83fe55c.
The main purpose of this is to avoid propagating blocks that
are going to be succeeded by more blocks in the stream.

The streams from GetBlocks response and UploadBlocks request
are now forwarded to the blockchain task that processes them
as streams.
Failed Precondition makes strange diagnostics.
Display hashes without their Debug cruft, use KV for the node ID.
This can happen if the peer requests strange things.
Explain why polling the DelayQueue has to be backstopped
by returning Ready when the queue has no entries to yield,
and suggest a comprehensive rework in a comment.
@mzabaluev mzabaluev force-pushed the chain-pull-redux branch from 7e478df to ed5bb17 Nov 13, 2019
@mzabaluev mzabaluev requested a review from NicolasDP Nov 13, 2019
@NicolasDP NicolasDP merged commit 7041765 into master Nov 13, 2019
6 of 7 checks passed
6 of 7 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
ci/circleci: cargo_audit Your tests passed on CircleCI!
Details
ci/circleci: cargo_fetch Your tests passed on CircleCI!
Details
ci/circleci: rustfmt Your tests passed on CircleCI!
Details
ci/circleci: test_debug Your tests passed on CircleCI!
Details
ci/circleci: test_nightly Your tests passed on CircleCI!
Details
ci/circleci: test_release Your tests passed on CircleCI!
Details
@NicolasDP NicolasDP deleted the chain-pull-redux branch Nov 13, 2019
@rinor

This comment has been minimized.

Copy link
Contributor

rinor commented Nov 13, 2019

There is smth wrong with this. It started forking again. Can you check the attached logs from 2 nodes while I dig further (in case I missed smth, or the issue is in some other place). Thanks

UPDATE: Fixed on #1138

@mzabaluev

This comment has been minimized.

Copy link
Contributor Author

mzabaluev commented Nov 15, 2019

I've noticed that when running on a young chain which gets forked by some other nodes, the checkpoints for pull may be insufficiently far to go beyond the fork. Investigating why.

@mzabaluev

This comment has been minimized.

Copy link
Contributor Author

mzabaluev commented Nov 15, 2019

Oops, a process_new_ref call fell by the wayside. Stop the presses, I need to fix this.

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