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, core: introduce les4 #20227

Closed
wants to merge 1 commit into from
Closed

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Nov 1, 2019

This PR introduces forkID into les protocol.

A few notable changes are listed:

  1. Now except for the "trusted" server, the client will randomly choose some servers for the signed announcement. The reason is: now only "trusted" server is required for the signed announce. So server
    can easily know I am chosen as trusted server.

Regarding the version bumping, we actually can add a new handshake field into the current version.
If forkID is available, then check it. Otherwise only genesis hash is checked. The reason I choose to bump the version is: in the les4 we can forcibly check the forkID.

Copy link
Contributor

@zsfelfoldi zsfelfoldi left a comment

Choose a reason for hiding this comment

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

Most of it looks fine, see comments.

//
// Enable signed announcement randomly even the server is not trusted.
p.announceType = announceTypeSimple
if p.trusted || r.Intn(10) > 3 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a bit weird. Maybe it is justified but I don't yet understand the reasoning behind this change. What difference does it make for the server if the client has selected it as trusted? If this is a problem for security reasons (I'm not sure if it is indeed) then we can also just always request signed headers, it is not a big overhead. I'm still sceptical though because trusted servers are operated by well-known entities and their operators know anyways that their server is usually trusted by clients. Especially if it is an "announce only" server which cannot be used for anything else.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason for the change is for security. Since now the server "can know" that he is selected as a trusted server since we won't request signed header for "untrusted" server right now. For the security perspective, it's may not a good thing.

So the change will select a few random "untrusted" server for signed header too. So it's hard to judge whether they are selected.

But you are right, maybe (1) we can choose to request signed header always (2) If the trusted server is basically based on the off-chain reputation so that it's totally another thing.

But I think we definitely can find some random server as the trusted instead of following "well-known" nodes. Is it safer?

les/peer.go Outdated Show resolved Hide resolved
les, core: introduce forkid for les4

les: update peer unit test

les: check peer version in handler

les: fix linter

les: address comments
@zsfelfoldi
Copy link
Contributor

This PR should be introduced in the same release as #20517

@rjl493456442
Copy link
Member Author

It's replaced by #21974. Close it now

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.

3 participants