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.

Sign up for GitHub

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

Jump to bottom

feat: download block ranges #3416

Merged
merged 36 commits into from
Jul 6, 2023
Merged

feat: download block ranges #3416

merged 36 commits into from
Jul 6, 2023

Conversation

Rjected
Copy link
Member

@Rjected Rjected commented Jun 27, 2023 •
edited
Loading

This lets the engine download entire block ranges rather than one-by-one. This mainly implements the full block range client, which is currently modeled after the single full block client.

TODO:

  • Test range client
  • Integrate range client and inflight range requests into engine
  • Start range downloads instead of single block downloads after fetching the fcu head

This is the download half of #3117

@Rjected Rjected added C-enhancement New feature or request A-consensus Related to the consensus engine A-networking Related to networking in general labels Jun 27, 2023
mattsse
mattsse requested changes Jun 27, 2023
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

good start, left some suggestions

crates/consensus/beacon/src/engine/sync.rs Outdated Show resolved Hide resolved
crates/interfaces/src/p2p/full_block_range.rs Outdated Show resolved Hide resolved
crates/interfaces/src/p2p/full_block_range.rs Outdated Show resolved Hide resolved
crates/interfaces/src/p2p/full_block_range.rs Outdated Show resolved Hide resolved
@mattsse mattsse mentioned this pull request Jun 28, 2023
Closed
1 task
@onbjerg onbjerg added this to the 0.1.0-alpha.2 milestone Jun 29, 2023
@Rjected Rjected requested a review from mattsse June 30, 2023 02:18
@Rjected
Copy link
Member Author

Rjected commented Jun 30, 2023

Need to now integrate this

mattsse
mattsse requested changes Jun 30, 2023
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

nice, this is pretty close I think.

needs some fine-tuning but the integration into consensus engine shouldn't be that difficult if we return the blocks via the existing event variant

crates/interfaces/src/p2p/full_block.rs Outdated Show resolved Hide resolved
crates/interfaces/src/p2p/full_block.rs Outdated Show resolved Hide resolved
crates/interfaces/src/p2p/full_block.rs Show resolved Hide resolved
crates/interfaces/src/p2p/full_block.rs Show resolved Hide resolved
crates/interfaces/src/p2p/full_block.rs Outdated Show resolved Hide resolved
crates/consensus/beacon/src/engine/sync.rs Outdated Show resolved Hide resolved
@mattsse mattsse mentioned this pull request Jun 30, 2023
Closed
@codecov
Copy link

codecov bot commented Jul 1, 2023 •
edited
Loading

Codecov Report

Merging #3416 (2f52548) into main (428a6dc) will increase coverage by 0.13%.
The diff coverage is 81.32%.

Impacted file tree graph

Impacted Files Coverage Δ
crates/primitives/src/header.rs 94.30% <0.00%> (-1.67%) ⬇️
crates/primitives/src/lib.rs 100.00% <ø> (ø)
crates/consensus/beacon/src/engine/mod.rs 76.32% <12.50%> (-0.10%) ⬇️
crates/primitives/src/block.rs 77.93% <65.62%> (-0.76%) ⬇️
crates/consensus/beacon/src/engine/sync.rs 80.53% <84.46%> (+6.02%) ⬆️
crates/interfaces/src/p2p/full_block.rs 76.99% <85.17%> (+10.65%) ⬆️
crates/interfaces/src/test_utils/full_block.rs 69.66% <87.14%> (+64.39%) ⬆️

... and 8 files with indirect coverage changes

Flag Coverage Δ
integration-tests 15.95% <0.00%> (-0.13%) ⬇️
unit-tests 64.11% <81.32%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
reth binary 26.41% <ø> (ø)
blockchain tree 81.25% <ø> (ø)
pipeline 86.91% <ø> (ø)
storage (db) 73.48% <ø> (ø)
trie 94.66% <ø> (ø)
txpool 49.11% <ø> (ø)
networking 77.86% <ø> (-0.03%) ⬇️
rpc 58.05% <ø> (ø)
consensus 63.50% <79.27%> (+1.39%) ⬆️
revm 34.90% <ø> (ø)
payload builder 6.83% <ø> (ø)
primitives 88.27% <82.45%> (-0.01%) ⬇️

mattsse
mattsse reviewed Jul 1, 2023
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this looks very good!

the on_missing section is a bit large now, and I'd would like to extract this to a standalone function

crates/consensus/beacon/src/engine/mod.rs Outdated Show resolved Hide resolved
@mattsse mattsse mentioned this pull request Jul 2, 2023
Closed
1 task
@gakonst
Copy link
Member

gakonst commented Jul 2, 2023

@Rjected what's missing here? seeing the following in your list:

Start range downloads instead of single block downloads after fetching the fcu head

@onbjerg onbjerg added the M-changelog This change should be included in the changelog label Jul 3, 2023
@Rjected Rjected force-pushed the dan/download-range branch 2 times, most recently from ce9572d to 4539e76 Compare July 3, 2023 18:39
@Rjected Rjected marked this pull request as ready for review July 3, 2023 23:06
@mattsse mattsse mentioned this pull request Jul 4, 2023
Merged
Rjected added 19 commits July 5, 2023 16:47
@Rjected
insert single blocks into range heap and remove queued events
cf0058b
@Rjected
remove sync controller poll loop in favor of returning Pending
2e27e2e
@Rjected
move disconnected branch to method, add traces for download results
eee0e28
@Rjected
request until we have enough bodies, remove header len penalize
e79ae49
@Rjected
match bodies responses with root triples
4178531
@Rjected
assume response is in-order
f192952
@Rjected
add docs for future order and ordering assumption
1a6225f
@Rjected
use is_bodies_complete instead of len comparison
c04bc86
@Rjected
add more ordering docs
2d96256
@Rjected
update body result docs
2609639
@Rjected
correct remaining hashes comment
788ec71
@Rjected
rename hash to start_hash
5d00d7c
@Rjected
document get_full_block_range block ordering
a6973b3
@Rjected
change has_bodies_request_started to only check current bodies request
081f65c
@Rjected
validate entire bodies response
d988971
@Rjected
use num_hash for traces in sync controller
6a880d8
@Rjected
use download_full_block when count is one
9699b77
@Rjected
move full block client to test_utils, test ascending blocks on poll
0715374
@Rjected
reintegrate full blocks after rebase
9b9494d
@gakonst
Copy link
Member

gakonst commented Jul 5, 2023

Let's try to get this in by Thursday and test it w/ rest of merged code on main so we can release Friday AM!

mattsse
mattsse approved these changes Jul 6, 2023
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this looks very good,

love the tests.

the integration in the engine is very simple which is super nice.

@mattsse mattsse enabled auto-merge July 6, 2023 11:01
@mattsse mattsse added this pull request to the merge queue Jul 6, 2023
Merged via the queue into main with commit 596d326 Jul 6, 2023
@mattsse mattsse deleted the dan/download-range branch July 6, 2023 12:21
@onbjerg onbjerg mentioned this pull request Jul 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsse mattsse mattsse approved these changes

@rkrasiuk rkrasiuk Awaiting requested review from rkrasiuk rkrasiuk is a code owner

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

Assignees
No one assigned
Labels
A-consensus Related to the consensus engine A-networking Related to networking in general C-enhancement New feature or request M-changelog This change should be included in the changelog
Projects
None yet
Milestone
0.1.0-alpha.2
Development

Successfully merging this pull request may close these issues.
4 participants
@Rjected @gakonst @mattsse @onbjerg