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

les: add checkpoint challenge for LES #20125

Closed

Conversation

rjl493456442
Copy link
Member

It's a PR based on the #20120

This PR first fixes an issue in downloader which introduced in #19413.

Then this PR adds checkpoint challenge for LES too. In short, if the server is not synced or on the difference chain, then it will be rejected very soon.

@rjl493456442 rjl493456442 changed the title Checkpoint challenge 2 les: add checkpoint challenge for LES Sep 25, 2019
@holiman
Copy link
Contributor

holiman commented Sep 25, 2019

please rebase

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM, with some minor questions


if master {
d.cancel()
if d.isMaster(pid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

With this change, you are introducing a gap between checking isMaster and actually doing the cancel. I assume that the cancel now happens later, during UnregisterPeer -- but is it possible that when that happens, the pid is no longer the master?

Copy link
Member Author

Choose a reason for hiding this comment

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

d.cancelPeer is only assigned before syncing start at d.synchronise function.

Although we drop the peer but d.synchronising is still 1, so we don't need to worry about the mismatch

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume that the cancel now happens later

Actually cancel now happen earlier in dropPeer function


if master {
s.d.cancel()
if s.d.isMaster(req.peer.id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above

@adamschmideg
Copy link
Contributor

Can you split this PR into two, one for the light-client related stuff and the downloader stuff

@rjl493456442
Copy link
Member Author

Waiting #20692 to be merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants