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

RFC35/Common Ancestor Approach #386

Merged
merged 46 commits into from
Jun 8, 2022

Conversation

manav2401
Copy link
Contributor

@manav2401 manav2401 commented May 8, 2022

This PR is the first step towards reducing reorgs and is a part of RFC35.

It adds a new background service called checkpoint whitelist which keeps running and asks it's heimdall for the latest checkpoint entries. Similar to the --whitelist flag, it stores the <block number> -> <block hash> mapping. The interesting part is that it leverages the heimdall checkpoints for making these entries. Basically it adds every end block number to end block hash entry to this in memory map (which is capped to 10).

This is to prevent the node to connect to wrong/invalid peers which are not on the correct fork. We decide that before even asking the peer for new blocks i.e. in the findAncestor function in downloader. Basically, we ask for the last checkpointed block of ours from the peer. If we're able to validate the details, then and then only we'll allow that peer to connect. In case of mismatch, we will eventually drop or stall the peer until it has relevant and correct blocks.

Sufficient unit tests are added and devnet testing has been carried on for the same. The next step is to make modifications in the forkchoice rule. Refer #425 for the next step.

@codecov-commenter
Copy link

codecov-commenter commented May 11, 2022

Codecov Report

Merging #386 (b4872ca) into master (783f93b) will decrease coverage by 0.09%.
The diff coverage is 25.80%.

@@            Coverage Diff             @@
##           master     #386      +/-   ##
==========================================
- Coverage   56.75%   56.65%   -0.10%     
==========================================
  Files         578      580       +2     
  Lines       68312    68428     +116     
==========================================
- Hits        38771    38770       -1     
- Misses      26182    26293     +111     
- Partials     3359     3365       +6     
Impacted Files Coverage Δ
consensus/bor/rest.go 0.00% <0.00%> (ø)
eth/api_backend.go 0.00% <0.00%> (ø)
eth/backend.go 0.00% <0.00%> (ø)
eth/handler_bor.go 0.00% <0.00%> (ø)
les/api_backend.go 0.00% <0.00%> (ø)
eth/downloader/downloader.go 70.78% <42.85%> (-0.42%) ⬇️
eth/downloader/whitelist/service.go 61.53% <61.53%> (ø)
eth/handler.go 54.12% <100.00%> (+0.16%) ⬆️
rlp/raw.go 79.37% <0.00%> (-5.63%) ⬇️
p2p/simulations/mocker.go 30.00% <0.00%> (-5.56%) ⬇️
... and 21 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 783f93b...b4872ca. Read the comment docs.

@manav2401 manav2401 changed the base branch from rfc35/common-ancestor-approach to master June 2, 2022 11:15
@manav2401 manav2401 dismissed JekaMas’s stale review June 2, 2022 11:15

The base branch was changed.

@manav2401 manav2401 changed the base branch from master to v0.2.16-candidate June 2, 2022 11:16
@manav2401 manav2401 changed the base branch from v0.2.16-candidate to rfc35/common-ancestor-approach June 2, 2022 11:17
@manav2401 manav2401 changed the title Rfc35/common ancestor approach units RFC35/Common Ancestor Approach Jun 2, 2022
@manav2401 manav2401 changed the base branch from rfc35/common-ancestor-approach to master June 2, 2022 15:33
@manav2401 manav2401 marked this pull request as ready for review June 2, 2022 15:34
eth/backend.go Show resolved Hide resolved
return 0, common.Hash{}, errEndBlock
}

hash := fmt.Sprintf("%v", block["hash"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use Hex() method?

JekaMas
JekaMas previously approved these changes Jun 2, 2022
Copy link
Contributor

@JekaMas JekaMas left a comment

Choose a reason for hiding this comment

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

LGTM overall.

@manav2401 manav2401 force-pushed the rfc35/common-ancestor-approach-units branch from 52562d3 to b19d5e1 Compare June 3, 2022 07:39
@temaniarpit27 temaniarpit27 changed the base branch from master to develop June 8, 2022 08:05
@temaniarpit27 temaniarpit27 merged commit ed2d9ae into develop Jun 8, 2022
@temaniarpit27 temaniarpit27 deleted the rfc35/common-ancestor-approach-units branch June 8, 2022 08:29
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