Skip to content

Rework blockchain import#17973

Merged
karalabe merged 2 commits intoethereum:masterfrom
holiman:splitter2
Nov 22, 2018
Merged

Rework blockchain import#17973
karalabe merged 2 commits intoethereum:masterfrom
holiman:splitter2

Conversation

@holiman
Copy link
Copy Markdown
Contributor

@holiman holiman commented Oct 24, 2018

This PR addresses some shortcomings in the blockchain downloader and block processing, specifically the behaviour around (invalid) long sidechains, as happened during the Ropsten constantinople HF.

@holiman
Copy link
Copy Markdown
Contributor Author

holiman commented Oct 29, 2018

This has now been running both full and fast sync on the benchmarking machines, and appear to be able to complete a full sync.
The graph below shows the fast-sync network IO after fast sync completion. Whereas ingress is almost the same, the ingress traffic is reduced heavily, almost to 50%. I believe that is because when we sync with a new peer, we now correclty determine the common ancestor at block N instead of block N-15, and do not have to fetch 15 full blocks unnecessarily.
network_io

The graphs also shows that block processing is faster in this PR -- but that's not really fair, because this PR handles some errors before 'starting the clock' -- and we only really measure the time execution of a block takes.

@karalabe karalabe self-assigned this Nov 7, 2018
Comment thread eth/downloader/downloader.go Outdated
Comment thread eth/downloader/downloader.go Outdated
Comment thread eth/downloader/downloader_test.go Outdated
Comment thread eth/downloader/downloader_test.go Outdated
Comment thread eth/downloader/downloader.go Outdated
Comment thread eth/downloader/downloader.go Outdated
Comment thread eth/downloader/downloader_test.go Outdated
Comment thread eth/downloader/downloader_test.go Outdated
@karalabe
Copy link
Copy Markdown
Member

@holiman Could you please rebase this on master? Thx

@holiman
Copy link
Copy Markdown
Contributor Author

holiman commented Nov 19, 2018

done

Comment thread eth/downloader/downloader_test.go Outdated
Copy link
Copy Markdown
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

An initial request set of comments. I still need to look through part of the core in detail, but there's quite a bit of refactoring I requested which modified the code heavily.

Comment thread eth/api.go Outdated
Comment thread internal/web3ext/web3ext.go Outdated
Comment thread core/chain_makers_test.go Outdated
Comment thread core/blockchain.go Outdated
Comment thread core/blockchain.go Outdated
Comment thread core/blockchain.go Outdated
Comment thread core/blockchain.go Outdated
Comment thread core/blockchain.go Outdated
Comment thread core/blockchain.go Outdated
Comment thread core/blockchain.go Outdated
@holiman
Copy link
Copy Markdown
Contributor Author

holiman commented Nov 19, 2018

I think I have addressed all concerns now. However, after the latest changes, the code is relatively untested. Once you are satisfied with the general 'look' of the code, we should either add testcases that cover long (bad-state) sidechains, or run some external tests on that scenario.

Comment thread core/blockchain.go Outdated
Comment thread eth/downloader/api.go Outdated
@karalabe karalabe force-pushed the splitter2 branch 3 times, most recently from eff5b07 to 0cd152e Compare November 20, 2018 14:56
@karalabe karalabe added this to the 1.8.19 milestone Nov 20, 2018
@karalabe karalabe merged commit 3ba0418 into ethereum:master Nov 22, 2018
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Apr 24, 2025
gzliudan added a commit to XinFinOrg/XDPoSChain that referenced this pull request Apr 25, 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.

2 participants