Skip to content

RFC35/Common Ancestor: Modifying the forkchoice rule#425

Merged
manav2401 merged 41 commits into
developfrom
rfc35/common-ancestor-approach-forkchoice
Jul 15, 2022
Merged

RFC35/Common Ancestor: Modifying the forkchoice rule#425
manav2401 merged 41 commits into
developfrom
rfc35/common-ancestor-approach-forkchoice

Conversation

@manav2401

@manav2401 manav2401 commented Jun 8, 2022

Copy link
Copy Markdown
Member

This PR is the second step towards reducing reorgs and is a part of RFC35. Refer #386 for the first step.

It adds more functionality to the previously added checkpoint whitelist service. It performs some additional checks during the chain insertion process. The forkchoice module is responsible for checking against any incoming chain if we need to reorg or not using the ReorgNeeded function. It basically compared the total difficulty of the local chain and remote incoming chain. This PR adds some additional prevention by introducing a ValidateReorg which calls the checkpoint whitelist service to validate the chain. Some important validations are given below.

  • Past chain (where all blocks are <= last current block) case: check the chain against our local whitelist entry.
  • Future chain (where all blocks are > last current block) case: only accept a chain of some specific length (This number is fixed to 256 for now but will be made configurable later on).

A minor modification over last iteration is that we now fetch N (latest) checkpoint entries instead of 1 in the first run to handle some sync cases for the client. Later on, the subsequent iterations only fetch 1 (latest) entry from heimdall.

This will ideally prevent the node to get reorged to some wrong/invalid fork and will maintain consistency. Sufficient unit tests are added and devnet testing has been carried on for the same.

Comment thread eth/downloader/whitelist/service.go Outdated
Comment thread eth/downloader/whitelist/service.go Outdated
Comment thread eth/downloader/whitelist/service.go Outdated
Comment thread eth/downloader/whitelist/service.go
Comment thread eth/downloader/whitelist/service.go Outdated
Comment thread interfaces.go
Comment thread eth/downloader/whitelist/service_test.go Outdated
@manav2401 manav2401 requested a review from JekaMas June 24, 2022 16:17

@JekaMas JekaMas left a comment

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.

looks good!

@manav2401 manav2401 changed the title Rfc35/common ancestor approach forkchoice RFC35/Common Ancestor: Modifying the forkchoice rule Jul 5, 2022
@manav2401 manav2401 marked this pull request as ready for review July 5, 2022 11:36
@manav2401 manav2401 requested review from a team and JekaMas July 5, 2022 19:11
Comment thread eth/downloader/downloader.go
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Merging #425 (b38a872) into develop (f28f384) will increase coverage by 0.07%.
The diff coverage is 44.23%.

@@             Coverage Diff             @@
##           develop     #425      +/-   ##
===========================================
+ Coverage    56.30%   56.37%   +0.07%     
===========================================
  Files          597      600       +3     
  Lines        69014    69149     +135     
===========================================
+ Hits         38855    38983     +128     
- Misses       26812    26830      +18     
+ Partials      3347     3336      -11     
Impacted Files Coverage Δ
consensus/bor/heimdall/client.go 6.25% <0.00%> (-1.45%) ⬇️
eth/backend.go 0.00% <0.00%> (ø)
eth/handler_bor.go 0.00% <0.00%> (ø)
internal/cli/server/server.go 25.14% <ø> (ø)
les/client.go 0.00% <0.00%> (ø)
core/blockchain.go 59.55% <50.00%> (-0.27%) ⬇️
core/headerchain.go 68.36% <55.55%> (-0.17%) ⬇️
eth/downloader/whitelist/service.go 81.57% <92.10%> (+20.04%) ⬆️
accounts/abi/bind/backends/simulated.go 63.86% <100.00%> (ø)
core/forkchoice.go 70.96% <100.00%> (+5.58%) ⬆️
... and 47 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f28f384...b38a872. Read the comment docs.

@manav2401 manav2401 requested review from a team and JekaMas July 13, 2022 07:07
@manav2401 manav2401 merged commit c446937 into develop Jul 15, 2022
@manav2401 manav2401 deleted the rfc35/common-ancestor-approach-forkchoice branch July 15, 2022 05:07
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