Skip to content

RFC35/Common Ancestor Approach#386

Merged
temaniarpit27 merged 46 commits into
developfrom
rfc35/common-ancestor-approach-units
Jun 8, 2022
Merged

RFC35/Common Ancestor Approach#386
temaniarpit27 merged 46 commits into
developfrom
rfc35/common-ancestor-approach-units

Conversation

@manav2401

@manav2401 manav2401 commented May 8, 2022

Copy link
Copy Markdown
Member

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

codecov-commenter commented May 11, 2022

Copy link
Copy Markdown

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.

Comment thread eth/downloader/whitelist/service_test.go Outdated
@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
Comment thread eth/backend.go
Comment thread eth/handler_bor.go
Comment thread eth/handler_bor.go
return 0, common.Hash{}, errEndBlock
}

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

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.

Could we use Hex() method?

JekaMas
JekaMas previously approved these changes Jun 2, 2022

@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.

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