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: implement ultralight client #16904

Merged
merged 5 commits into from
Jan 24, 2019
Merged

les: implement ultralight client #16904

merged 5 commits into from
Jan 24, 2019

Conversation

b00ris
Copy link
Contributor

@b00ris b00ris commented Jun 5, 2018

les/fetcher.go Outdated Show resolved Hide resolved
les/fetcher.go Show resolved Hide resolved
eth/downloader/downloader.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
les/handler.go Outdated Show resolved Hide resolved
les/peer.go Outdated Show resolved Hide resolved
@GitCop
Copy link

GitCop commented Jun 14, 2018

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

  • Commit: 1fb5520

  • Commits must be prefixed with the package(s) they modify

  • Commit: a26c3a0

  • Commits must be prefixed with the package(s) they modify

  • Commit: c1f6710

  • Commits must be prefixed with the package(s) they modify

  • Commit: bf691f9

  • Commits must be prefixed with the package(s) they modify

  • Commit: d1a79df

  • Commits must be prefixed with the package(s) they modify

  • Commit: 3db20a8

  • Commits must be prefixed with the package(s) they modify

  • Commit: c535734

  • Commits must be prefixed with the package(s) they modify

  • Commit: c8e44cd

  • Commits must be prefixed with the package(s) they modify

  • Commit: e67d70a

  • Commits must be prefixed with the package(s) they modify

  • Commit: e3bb872

  • Commits must be prefixed with the package(s) they modify

  • Commit: 6c4b901

  • Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.


This message was auto-generated by https://gitcop.com

2 similar comments
@GitCop
Copy link

GitCop commented Jun 18, 2018

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

  • Commit: 1fb5520

  • Commits must be prefixed with the package(s) they modify

  • Commit: a26c3a0

  • Commits must be prefixed with the package(s) they modify

  • Commit: c1f6710

  • Commits must be prefixed with the package(s) they modify

  • Commit: bf691f9

  • Commits must be prefixed with the package(s) they modify

  • Commit: d1a79df

  • Commits must be prefixed with the package(s) they modify

  • Commit: 3db20a8

  • Commits must be prefixed with the package(s) they modify

  • Commit: c535734

  • Commits must be prefixed with the package(s) they modify

  • Commit: c8e44cd

  • Commits must be prefixed with the package(s) they modify

  • Commit: e67d70a

  • Commits must be prefixed with the package(s) they modify

  • Commit: e3bb872

  • Commits must be prefixed with the package(s) they modify

  • Commit: 6c4b901

  • Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.


This message was auto-generated by https://gitcop.com

@GitCop
Copy link

GitCop commented Jun 25, 2018

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

  • Commit: 1fb5520

  • Commits must be prefixed with the package(s) they modify

  • Commit: a26c3a0

  • Commits must be prefixed with the package(s) they modify

  • Commit: c1f6710

  • Commits must be prefixed with the package(s) they modify

  • Commit: bf691f9

  • Commits must be prefixed with the package(s) they modify

  • Commit: d1a79df

  • Commits must be prefixed with the package(s) they modify

  • Commit: 3db20a8

  • Commits must be prefixed with the package(s) they modify

  • Commit: c535734

  • Commits must be prefixed with the package(s) they modify

  • Commit: c8e44cd

  • Commits must be prefixed with the package(s) they modify

  • Commit: e67d70a

  • Commits must be prefixed with the package(s) they modify

  • Commit: e3bb872

  • Commits must be prefixed with the package(s) they modify

  • Commit: 6c4b901

  • Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.


This message was auto-generated by https://gitcop.com

les/fetcher.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
@GitCop
Copy link

GitCop commented Jul 25, 2018

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

  • Commit: 1fb5520

  • Commits must be prefixed with the package(s) they modify

  • Commit: a26c3a0

  • Commits must be prefixed with the package(s) they modify

  • Commit: c1f6710

  • Commits must be prefixed with the package(s) they modify

  • Commit: bf691f9

  • Commits must be prefixed with the package(s) they modify

  • Commit: d1a79df

  • Commits must be prefixed with the package(s) they modify

  • Commit: 3db20a8

  • Commits must be prefixed with the package(s) they modify

  • Commit: c535734

  • Commits must be prefixed with the package(s) they modify

  • Commit: c8e44cd

  • Commits must be prefixed with the package(s) they modify

  • Commit: e67d70a

  • Commits must be prefixed with the package(s) they modify

  • Commit: e3bb872

  • Commits must be prefixed with the package(s) they modify

  • Commit: 6c4b901

  • Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.


This message was auto-generated by https://gitcop.com

@GitCop
Copy link

GitCop commented Aug 2, 2018

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

  • Commit: 1fb5520

  • Commits must be prefixed with the package(s) they modify

  • Commit: a26c3a0

  • Commits must be prefixed with the package(s) they modify

  • Commit: c1f6710

  • Commits must be prefixed with the package(s) they modify

  • Commit: bf691f9

  • Commits must be prefixed with the package(s) they modify

  • Commit: d1a79df

  • Commits must be prefixed with the package(s) they modify

  • Commit: 3db20a8

  • Commits must be prefixed with the package(s) they modify

  • Commit: c535734

  • Commits must be prefixed with the package(s) they modify

  • Commit: c8e44cd

  • Commits must be prefixed with the package(s) they modify

  • Commit: e67d70a

  • Commits must be prefixed with the package(s) they modify

  • Commit: e3bb872

  • Commits must be prefixed with the package(s) they modify

  • Commit: 6c4b901

  • Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.


This message was auto-generated by https://gitcop.com

les/fetcher.go Outdated Show resolved Hide resolved
les/handler_test.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
les/peer.go Outdated Show resolved Hide resolved
@b00ris
Copy link
Contributor Author

b00ris commented Oct 10, 2018

@zsfelfoldi Hi! Could you give me a review update? I can't reach you.

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.

A few cosmetic fixes suggested in the comments. If these are fixed, please squash commits and rebase, then I will approve.

les/txrelay.go Outdated Show resolved Hide resolved
les/handler.go Outdated Show resolved Hide resolved
les/handler.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
les/fetcher.go Outdated Show resolved Hide resolved
eth/ulc_config.go Outdated Show resolved Hide resolved
cmd/utils/flags.go Show resolved Hide resolved
zsfelfoldi
zsfelfoldi previously approved these changes Nov 12, 2018
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.

please squash into a single commit

@shazow
Copy link
Contributor

shazow commented Dec 17, 2018

FYI, I had a chance to chat with @zsfelfoldi, he tells me that @karalabe has indeed looked at this PR and he has a couple of comments that he still needs to write up.

Hope we can get an update soon! I will bug them more if we don't hear anything over the next few days.

@JekaMas
Copy link
Contributor

JekaMas commented Dec 21, 2018

@karalabe please, give us your comments and we'll fix them all quickly. We're really looking to finish the PR this year :)

@zsfelfoldi
Copy link
Contributor

I think the most imporant suggestion was to simplify the handling of trusted peers in the server pool. I have that thing in my head now and I will quickly do it in a separate branch so that we can finalize this PR after the holidays. Our next release will be a major one (1.9) which is a great opportunity to include new features like this one.

@zsfelfoldi zsfelfoldi added this to the 1.9.0 milestone Dec 28, 2018
@JekaMas
Copy link
Contributor

JekaMas commented Dec 28, 2018

Great news! If you need any help please ping @b00ris or me.

@b00ris
Copy link
Contributor Author

b00ris commented Dec 28, 2018

Wow! Great news!

@zsfelfoldi
Copy link
Contributor

@b00ris @JekaMas here is my modification: https://github.com/zsfelfoldi/go-ethereum/tree/ulc-serverpool-fix
Since p2p.Server automatically tries to keep static nodes connected it is cleaner if we just add them once at the beginning. We don't need to create a separate pool or even pool entries for them. Just remember the IDs so that we can ignore them and not add them to the known/new selection pools either. Btw your version worked fine too but during the review we concluded that it is best to not complicate this logic if not necessary. I tried it and it seems to work in both normal and ulc mode but please try it in your environment too, I don't want to screw it up.
@karalabe @fjl also PTAL

@zsfelfoldi
Copy link
Contributor

@b00ris please keep your version too until we all have a consensus about it

@zsfelfoldi
Copy link
Contributor

Also there was a suggestion to remove "ulc.config" option for a separate ULC config file because we already have a config file. Right now I am in favor of keeping it unless there is a strong argument against so I did not remove it. ULC config can be an application vendor-specific thing while other config parameters are more user environment-specific so maybe having an optional separate config file is not a bad idea.
If I remember correctly the rest of the PR was generally considered OK and mergeable.

@b00ris
Copy link
Contributor Author

b00ris commented Dec 28, 2018

@zsfelfoldi I've reviewed your suggestion and I like it. It's cleaner than mine. I've also tried it in my environment and it works well.
I thought to delete ulc.config, but it was so useful for testing and I decided to keep it. I think it would be better to change it to toml to keep consistency.

@shazow
Copy link
Contributor

shazow commented Jan 4, 2019

What's the next step here? Should @b00ris merge @zsfelfoldi's changes into this PR?

Are we still blocked on a review from @karalabe?

@b00ris
Copy link
Contributor Author

b00ris commented Jan 9, 2019

@zsfelfoldi, @karalabe Could you give a feedback?

@ethereum ethereum deleted a comment from GitCop Jan 9, 2019
@ethereum ethereum deleted a comment from GitCop Jan 9, 2019
@ethereum ethereum deleted a comment from GitCop Jan 9, 2019
@ethereum ethereum deleted a comment from GitCop Jan 9, 2019
@fjl fjl self-requested a review January 9, 2019 11:00
@zsfelfoldi
Copy link
Contributor

@b00ris sure, I pushed my changes on this branch, @fjl will take a last look and then we'll merge. It will definitely go into the next release.

@zsfelfoldi
Copy link
Contributor

@fjl @karalabe I am going to merge this PR on Friday if there is no objection

@zsfelfoldi
Copy link
Contributor

Probably not this week because we don't want to merge any big changes until the delayed fork issue is settled. It is still going into 1.9 of course.

@fjl fjl changed the title les: implement ulc les: implement ultralight client Jan 24, 2019
@fjl fjl merged commit 7696570 into ethereum:master Jan 24, 2019
@ssl-prime
Copy link

please correct variable name in 'light/lightchain.go' from 'mu' to 'chainmu'
line no 537, 538, 544, 545

@JekaMas
Copy link
Contributor

JekaMas commented Jan 24, 2019

@b00ris

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

Successfully merging this pull request may close these issues.

9 participants