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

"Rebase" to geth 1.7.3 and add status geth patches #492

Merged
merged 20 commits into from
Jan 15, 2018
Merged

Conversation

divan
Copy link
Contributor

@divan divan commented Dec 4, 2017

This PR codifies our changes to the go-ethereum upstream in the form of diffs (.patch file format) and serves as a prerequisite to change our approach to syncronization with geth codebase.

  • Patches need to be updated per each geth release
  • They are up to date, so anyone can inspect them and clearly see what exactly we add into geth codebase

It also updates geth dependency in vendor/ to match latest v.1.7.3 release.

  • extract previous geth commits from our forked version and codify them as patches
  • update patches to work with 1.7.3 geth release
  • apply patches and update vendor/ folder
  • write scripts (or just doc?) for automatically applying patches for future releases
  • cht.json now contains only bootnodes addresses. New CHT/BloomTrie checkpoints are still hardcoded into go-ethereum and updated via patches.

README for patches: https://github.com/status-im/status-go/blob/adf7895549037bea5cc287636acb2e1236c56b44/geth-patches/README.md

Closes #467

@divan divan self-assigned this Dec 4, 2017
@divan divan mentioned this pull request Dec 4, 2017
3 tasks
@divan divan changed the title Add geth-related changes as patches "Rebase" to geth 1.7.3 and add status geth patches Dec 4, 2017
@@ -0,0 +1,352 @@
diff --git a/accounts/keystore/key.go b/accounts/keystore/key.go
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we store geth patches here or in our fork of go-ethereum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adambabik where to store patches is an open question, but with this approach we don't need to keep "our fork" repo anymore. Patches will be applied from scratch on each release (I believe this process could be automated via simple scripting)

@@ -166,12 +142,7 @@ func activateEthService(stack *node.Node, config *params.NodeConfig) error {
ethConf.DatabaseCache = config.LightEthConfig.DatabaseCache

if err := stack.Register(func(ctx *node.ServiceContext) (node.Service, error) {
lightEth, err := les.New(ctx, &ethConf)
if err == nil {
updateCHT(lightEth, config)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is pretty important for us as otherwise LES must be synced from the beginning. How is it handled now? LES/2 feature?

Copy link
Contributor

@adambabik adambabik Dec 5, 2017

Choose a reason for hiding this comment

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

I have just read it in the PR description...

One way to test it is to run a full node with LES enabled and try to sync your local node using that full node as a peer. We have a full node in our cluster. Let me know if you don't have access.

@divan
Copy link
Contributor Author

divan commented Dec 5, 2017

@divan
Copy link
Contributor Author

divan commented Dec 8, 2017

After upgrading one of the Status bootnodes to Geth 1.7.3, and testing this PR, I can see that synchronization on Ropsten is starting from trusted checkpoint, hardcoded in Geth 1.7.3 code (and light mode sync takes around 5 mins on my laptop). This release seems to have implemented CHT functionality and my assumption is that trusted checkpoint should be updated regularly. Holding this PR until this moment is clarified.

Copy link
Contributor

@adambabik adambabik left a comment

Choose a reason for hiding this comment

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

Where are CHT checkpoints hardcoded now?

Copy link
Contributor

@themue themue left a comment

Choose a reason for hiding this comment

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

It's a limited LGTM as I'm not yet deep enough into the individual patches. But I like the approach and the realisation.

A final LGTM should come by @adambabik or @tiabc .

@divan
Copy link
Contributor Author

divan commented Dec 12, 2017

Where are CHT checkpoints hardcoded now?

@adambabik they're implemented as a patch, which will be updated and re-applied from time to time until we implement proper Trusted Checkpoints mechanism: https://github.com/status-im/status-go/pull/492/files#diff-baeffc2b5580436f546bac21a047f513

Also, make sure you've seen this README for geth-patches dir, it also a good subject for review: https://github.com/status-im/status-go/blob/adf7895549037bea5cc287636acb2e1236c56b44/geth-patches/README.md

@divan divan removed the in progress label Dec 13, 2017
bloomTrieRoot: common.HexToHash("aca7d7c504d22737242effc3fdc604a762a0af9ced898036b5986c3a15220208"),
}
+
+ statusRopstenCheckpoint = trustedCheckpoint{
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume it has been calculated recently, right? When testing locally how long does it take to sync to the most recent block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I did it a couple of days ago and sync took 9-10 seconds on my laptop.

git clone https://github.com/ethereum/go-ethereum

# update remote url to point to our fork repo
git remote set-url origin git@github.com:status-im/go-ethereum.git
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also recommend adding git remote set-url upstream git@github.com:ethereum/go-ethereum.git so any further pulls from upstream could be done like git pull upstream release/1.7:master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! Feel free to update README. I like the way you proposed — it's more explicit, but can't check it now from scratch, so let's update this after this PR is merged. Both options work, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds good. I will make a PR when this one is merged. Also, I will test the whole flow shortly after this PR is merged as I have a use-case for a new patch.

Copy link
Contributor

@tiabc tiabc left a comment

Choose a reason for hiding this comment

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

Please, let me review the changes before merging.

@divan
Copy link
Contributor Author

divan commented Jan 8, 2018

This PR is needed to be merged asap as it's blocking other PRs. I'm setting a deadline for tomorrow (Jan 9th). @tiabc if you have a chance to review it today, it would be great.

@divan divan merged commit f93cd81 into develop Jan 15, 2018
@adambabik adambabik deleted the rebase/patches branch December 27, 2019 10:03
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.

4 participants