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

Update CHT root automatically using GetHeaderProofs (Fixes #320) #476

Closed
wants to merge 4 commits into from

Conversation

pacamara
Copy link

@pacamara pacamara commented Nov 24, 2017

Fixes #320 : "Devise how to update CHT automatically"

Superseded by ethereum/go-ethereum#15673

Retrieve the most recent CHT root from a peer by sending a single GetHeaderProofs message at startup.
A change has been made to ChtRequest.Validate so that in this case where no ChtRoot is supplied in the ChtRequest, it's pulled out of the proof.
Tested and appears to work fine. As a sanity check, after the CHT root has been set, an old header is retrieved and verified.

Sample output:

INFO [11-24|23:58:47] Account reselected geth=StatusIM
INFO [11-24|23:58:47] Notification received geth=StatusIM event="{"type":"node.ready","event":{}}"
INFO [11-25|00:00:15] Downloading latest CHT root from peer geth=StatusIM peer.headBlockInfo="{Hash:[57 74 249 81 125 100 65 239 95 224 122 175 139 200 91 160 15 58 67 41 189 239 204 28 247 182 91 222 9 25 177 16] Number:2135303 Td:+6841184590931614}"
INFO [11-25|00:00:15] Retrieved latest CHT root from peer, got geth=StatusIM ChtRoot=0x8bdfeae99b0b4747ce04f2251aa37b957d41d237781f8889be7c521ab651602a
INFO [11-25|00:00:15] Added trusted CHT geth=StatusIM develop=true number=520 hash=0x8bdfeae99b0b4747ce04f2251aa37b957d41d237781f8889be7c521ab651602a
INFO [11-25|00:00:15] Sanity check: can download some very old header? geth=StatusIM
INFO [11-25|00:00:15] Sanity check result: geth=StatusIM sanityHeader.Number=21299 err=nil
INFO [11-25|00:00:15] Block synchronisation started
INFO [11-25|00:00:18] Imported new block headers count=576 elapsed=1.887s number=2130495 hash=180198…0ec15e ignored=0
INFO [11-25|00:00:18] Imported new block headers count=576 elapsed=90.973ms number=2131071 hash=c19d90…82d5ff ignored=0

@tiabc
Copy link
Contributor

tiabc commented Nov 25, 2017

Thank you very much for such a valuable to us contribution! I'll try to review it asap but it might take a while. Please, be patient.

@pacamara
Copy link
Author

You're welcome! Understood, no worries. Look forward to your review. :)

@pacamara
Copy link
Author

Added 2 minor commits

@pacamara pacamara changed the title Update CHT root automatically using GetHeaderProofs (#320) Update CHT root automatically using GetHeaderProofs (Fixes #320) Nov 25, 2017
@divan
Copy link
Contributor

divan commented Nov 27, 2017

@pacamara looks interesting! Two moments from brief diff overview:

  1. most of the changes are in go-ethereum's code and we have a process of introducing our custom changes into go-ethereum codebase. However, I want to emphasize that we try to minimize the number of our own patches as much as possible — they are a good case only if we need change that is very specific to Status app. In this case, it seems like other geth users might benefit from it as well, so perhaps the right way to go will be to implement this as a part of go-ethereum. We will get this change automatically on the next Geth update after rebasing and updating vendor/.
  2. code formatting: seems like you don't use go fmt(or goimports) in your setup. It is recommended to setup it as a save hook or format code before committing.

Regarding 1. and Open Bounty program — if the change will be implemented in Geth, it technically solves the issue, but the way how Bounty contracts work, probably won't catch this automatically, so we'll have to do this manually, I guess. We definitely would love to make it work automatically for similar cases in the future, so it's a nice case to verify ideas around. Pinging @annadanchenko to take a look at that case.

@pacamara
Copy link
Author

@divan Thanks for the review! I'll do (2) now, and reach out to you folks on Riot re. best way to proceed with (1).

@andytudhope
Copy link

andytudhope commented Nov 30, 2017

Hi @pacamara! First off, thanks - you rock. Second, I think that you should submit this as a PR to go-ethereum and let them review it as that is better for almost everyone. However, it will mean that the bounty does not automatically get paid and your GH name won't show up in the hunters list. This is unfortunate and something we are currently working on. As a solution, we can manually pay you half the bounty after any other team reviews, and then the other half once you convince the go-ethereum guys to merge the fix. Sound good?
P.S. Looking forward to seeing you on Riot, my handle there is @cryptowanderer.

@pacamara
Copy link
Author

@andytudhope Thanks man! Your suggested split on the bounty is fine by me. I've already updated my SNT receiving address in openbounty. I'll get working on the go-ethereum PR now, and will update it with any changes from the review here as needed. See you on Riot :)

@pacamara
Copy link
Author

pacamara commented Dec 6, 2017

Hi @andytudhope @divan @tiabc I've ported the changes to go-ethereum. The handling is quite different now, go-ethereum has "trusted checkpoints" which include both CHT and Bloom Trie roots. So it seemed better to use a cleaner approach and add a new request GetCheckpoint to LES.

This PR is against my own fork's master for now so you can take a look before I submit it to go-ethereum:
https://github.com/pacamara/go-ethereum/pull/1/files

@pacamara
Copy link
Author

The relevant PR against upstream is now ethereum/go-ethereum#15673

@mandrigin
Copy link
Contributor

Closing since the actual PR is opened against upstream.

@mandrigin mandrigin closed this Feb 5, 2018
This pull request was closed.
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.

5 participants