Skip to content

eth: implement eth/62 synchronization logic#1701

Merged
fjl merged 5 commits intoethereum:developfrom
karalabe:eth62-sync-rebase
Aug 26, 2015
Merged

eth: implement eth/62 synchronization logic#1701
fjl merged 5 commits intoethereum:developfrom
karalabe:eth62-sync-rebase

Conversation

@karalabe
Copy link
Copy Markdown
Member

This PR is a replacement of the old eth/62 sync logic (#1661), rebased on top of develop. The old one had too many merge conflicts due to the double PR (this being opened on top of #1380). Disregard the old one and use this only.

@robotally
Copy link
Copy Markdown

Vote Count Reviewers
👍 2 @Gustav-Simonsson @zsfelfoldi
👎 0

Updated: Thu Aug 27 07:28:22 UTC 2015

@karalabe karalabe changed the title Eth62 sync rebase eth: implement eth/62 synchronization logic Aug 21, 2015
@obscuren obscuren modified the milestone: 1.2.0 Aug 21, 2015
Comment thread cmd/utils/flags.go Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

So 62 is not advertised by default? Is it planned to be advertised at some time after 62 is deployed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I didn't enable it because the plan was originally to have it in master too, with it being enabled in develop only. Since it will miss master, this needs to be bumped to 62 for develop :)

@Gustav-Simonsson
Copy link
Copy Markdown

👍

@karalabe
Copy link
Copy Markdown
Member Author

Rebased Squashed into 4 commits.

@zsfelfoldi
Copy link
Copy Markdown
Contributor

👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This loop is basically identical to the one above and the one below, except for the handling of hash packs. Maybe it should be defined only once in a function.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The reason I duplicated all code belonging to eth61 vs 62 is because at a certain point I'd like to see eth/61 go, and then we could simply delete the corresponding functions and not have to worry about interweaved logic. I agree with generalizing things, but only as long as it doesn't cross the eth/61 and eth/62 boundaries so the latter can evolve without having potentially bad side effects on the former.

Comment thread eth/downloader/queue.go
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note: types.DeriveSha([]type) == sha3("") for any type. Maybe use a global value or export core/types.emptyRootHash.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was also pondering about this, and also about extracting the empty sha out into a const/var. Will do in a follow up PR.

@fjl
Copy link
Copy Markdown
Contributor

fjl commented Aug 26, 2015

General impression: Code duplication is now a lot worse. The logic for headers vs. hashes is very similar but not quite. Nice to see core.GenerateChain in action.

I will merge this now and will roll it back if people report any problems.

fjl added a commit that referenced this pull request Aug 26, 2015
eth: implement eth/62 synchronization logic
@fjl fjl merged commit 6ec13e7 into ethereum:develop Aug 26, 2015
@fjl fjl removed the review label Aug 26, 2015
@karalabe
Copy link
Copy Markdown
Member Author

Just a side note to the duplication: the idea with this PR's eth/62 sync logic was that it should follow eth/61 as close as possible to make the transitioning smooth (i.e. worry about the header/body split and extra logic needed to handle it), without having to debug a brand new sync algo. If this proves to be stable, I'd like to introduce parallel header downloads, which will mostly obsolete the current FetchHeaders and possibly partially FetchBodies functions too, so duplication will be a lot less of an issue.

tony-ricciardi pushed a commit to tony-ricciardi/go-ethereum that referenced this pull request Jan 20, 2022
* Announce draft

* add peer handshake enodecertmsg

* Add changes

* Fix link

* Add NEV definition

* Style query spawning

* Add spec by node text

* Add stop line in query spawning

* Update

* Update consensus/istanbul/backend/announce.md

Co-authored-by: piersy <pierspowlesland@gmail.com>

* Update consensus/istanbul/backend/announce.md

Co-authored-by: piersy <pierspowlesland@gmail.com>

* Add terminology for nodes

* Add TODO 'add link for enode id'

* Reorganized first part of the announce protocol

* Re-redacted traffic section in announce protocol

* Add explanation for message format

* Message explanation

* Rewrote explanation of message handling

* Comment on the vc for nev spawning

* Revert error

* Add spec changes

* Inbound -> Initiator

Co-authored-by: piersy <pierspowlesland@gmail.com>
Rampex1 pushed a commit to streamingfast/go-ethereum that referenced this pull request Aug 20, 2025
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.

6 participants