Merge parent and current sync lookups #5655

dapplion · 2024-04-26T09:10:23Z

Issue Addressed

Part of

Block lookup sync technical debt #5549

Block lookups fulfill the need to:

Sync blocks referenced by root by other consensus objects (i.e. attestations)
Sync and handle a chain of blocks whose parent is unknown (not rooted in our view of the chain)

Both goals interleave with each other, but in current unstable code each one is treated differently. This has been a significant source of pain and bugs, and the logic to transition between "parent lookup" and "current lookup" is gnarly.

This PR attempts to reduce the surface area for issues by:

Reducing the state transition on these request lookup / states, and making them strict
Splitting the complexity of handling the lookup (retries, send to processor) and the complexity of unknown parent sync itself. In a similar spirit of Handle sync lookup request streams in network context #5583 which moved the complexity of streams to somewhere else

Proposed Changes

Merge parent and current sync lookups

Deep dive into the rationale and changes overview (26 minutes) =D

https://drive.google.com/file/d/1vTOrvZKuYdn6iAJ3fdiNZhvqJycwkoiy/view?usp=sharing

Next steps

…nc-merged-lookup

beacon_node/beacon_chain/src/beacon_chain.rs

beacon_node/network/src/sync/block_lookups/common.rs

beacon_node/network/src/sync/block_lookups/mod.rs

beacon_node/network/src/sync/block_lookups/single_block_lookup.rs

beacon_node/network/src/sync/block_lookups/common.rs

beacon_node/network/src/sync/block_lookups/single_block_lookup.rs

beacon_node/network/src/sync/block_lookups/tests.rs

realbigsean · 2024-04-30T03:16:27Z

beacon_node/network/src/sync/block_lookups/tests.rs

+        }
+
+        // TODO: Eventually deprecate this function
+        fn set_block_id_for_import(mut self) -> Self {


when should this be deprecated? I'm going to remove the TODO because it should be identified as dead code when we will want to remove it

removed in dapplion#26

The deneb test rig made some assumptions about which is the target block to download that somehow broke when modifying the test cases, I think when adding block_missing_components function. I could not reason why, so I added this method to get the tests to pass. Seems like a bit of a hack, could you look into it?

it makes sense that we wouldn't have a block req id because the block was included in unknown parent error, so no request was ever triggered. i added a fallback to the blob req id here:

4726cbd

Also seems like there was a duplicate blobs response in the test, which didn't look like what it was trying to test so i removed the second

beacon_node/network/src/sync/block_lookups/common.rs

* fix compile after merge * remove todos, fix typos etc * fix compile * stable rng * delete TODO and unfilled out test * make download result a struct * enums instead of bools as params * fix comment

Squashed commit of the following: commit 0203843 Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue Apr 30 15:14:30 2024 +0900 Various fixes commit 9d185d1 Author: realbigsean <seananderson33@GMAIL.com> Date: Tue Apr 30 01:59:05 2024 -0400 #5655 pr review (#26) * fix compile after merge * remove todos, fix typos etc * fix compile * stable rng * delete TODO and unfilled out test * make download result a struct * enums instead of bools as params * fix comment commit 4bde25b Author: realbigsean <seananderson33@GMAIL.com> Date: Mon Apr 29 21:29:55 2024 -0400 fix compile after merge commit c309c07 Merge: 13bc011 c8ffafb Author: realbigsean <seananderson33@GMAIL.com> Date: Mon Apr 29 21:29:29 2024 -0400 Merge branch 'unstable' of https://github.com/sigp/lighthouse into sync-merged-lookup commit 13bc011 Merge: bad87b1 8b24880 Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Sat Apr 27 12:16:29 2024 +0900 Merge remote-tracking branch 'origin/unstable' into sync-merged-lookup commit bad87b1 Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Fri Apr 26 10:32:23 2024 +0900 Merge current and parent lookups tests commit 5d29618 Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Thu Apr 25 21:59:22 2024 +0900 Merge current and parent lookups clean up todos commit 611c37a Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Wed Apr 24 08:52:06 2024 +0900 Merge current and parent lookups commit 3b662de Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue Apr 23 15:14:17 2024 +0900 Send parent blocks one by one commit 4c8143f Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue Apr 23 01:28:53 2024 +0900 Drop reconstructed for processing commit e0b862b Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue Apr 23 01:14:26 2024 +0900 Drop lookup type trait for a simple arg

dapplion · 2024-04-30T11:32:30Z

Bump: Added a screen recording of mine going over the rationale and top level changes (26 min) link above in the main PR body

realbigsean

This looks good to go from my POV. Awesome job lion!

realbigsean · 2024-04-30T19:33:10Z

@mergify queue

mergify · 2024-04-30T19:33:56Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at ce66582

* Fix execution integration tests (#5647) * update waiting status * revert to old nethermind version * Add electra presets to beacon API (#5630) * add presets to API * add extra fields to config spec in beacon API * remove unused * add mainnet presets for gnosis and fix minimal preset default values * Rename `Merge` to `Bellatrix` (#5601) * Rename Merge to Bellatrix * Remove tree-hash-cache which got readded from the rebase * Deterministic block generation for tests (#5654) * Deterministic block generation for tests * Electra other containers (#5652) * add new fields to execution payload and header * beacon state changes * partial beacon state * safe arith in upgrade to electra * initialize balances cache in interop genesis state * Revert "initialize balances cache in interop genesis state" This reverts commit c60b522. * always initialize balances cache if necessary in electra upgrade * build cache earlier * fix block test * per fork NUM_FIELDS_POW2 * Merge branch 'unstable' of https://github.com/sigp/lighthouse into electra-other-containers * fix lints * get fields based on post state, as is spec'd * fix type and move cache build * Add more electra helpers (#5653) * Add new helpers * Fix some stuff * Fix compilation errors * lint * Address review * Ignore gossip blob already imported (#5656) * Ignore gossip blob already imported * Beta compiler fix (#5659) * fix beta compiler compilation * remove unused import * Revert "remove unused import" This reverts commit 0bef36b. * Revert "fix beta compiler compilation" This reverts commit 23152cf. * rename ununsed fields * allow dead code on some error variants * remove unused blob download queue * add back debug to backfill error * more allow dead code on errors * fix(validator_client): raise soft fd limit (#4796) * fix(validator_client): raise soft fd limit * Merge branch 'unstable' of https://github.com/sigp/lighthouse into rkrasiuk/raise-vc-fdlimit * cargo lock * Merge branch 'unstable' of https://github.com/sigp/lighthouse into rkrasiuk/raise-vc-fdlimit * Proposer and attester slashing sse events (#5327) * default vc to block v3 endpoint and deprecate block-v3 flag * Merge branch 'unstable' of https://github.com/sigp/lighthouse into unstable * add proposer and attester event variants * add TOOOs * add tests, event triggers * Merge branch 'unstable' of https://github.com/sigp/lighthouse into proposer-and-attester-slashing-sse-events * revert * revert * remove double event tracking * Merge branch 'unstable' into proposer-and-attester-slashing-sse-events * remove todo, fix test * resolve merge conflicts * Merge branch 'proposer-and-attester-slashing-sse-events' of https://github.com/eserilev/lighthouse into proposer-and-attester-slashing-sse-events * leftover debugging * Merge branch 'unstable' of https://github.com/sigp/lighthouse into proposer-and-attester-slashing-sse-events * Merge branch 'unstable' of https://github.com/sigp/lighthouse into proposer-and-attester-slashing-sse-events * pin macos release runner to `macos-13` (#5665) * pin macos release runner to `macos-13` * Update .github/workflows/release.yml * Remove snapshot cache related code (#5661) * Remove snapshot cache and other references. * Fix default state cache size in docs * Remove cache miss comment entirely * Add state cache CLI tests * Uncomment self_hosted_runner after PR Merge #5137 (#5291) * Uncomment self_hosted_runner after PR Merge #5137 * Merge branch 'unstable' into fix_todo * Merge branch 'unstable' of https://github.com/sigp/lighthouse into fix_todo * Only `portable` builds (binaries) (#5615) * release workflow: portable builds by default * Delete outdated comment * Merge branch 'unstable' into portable-builds-binaries # Conflicts: # .github/workflows/release.yml * Merge parent and current sync lookups (#5655) * Drop lookup type trait for a simple arg * Drop reconstructed for processing * Send parent blocks one by one * Merge current and parent lookups * Merge current and parent lookups clean up todos * Merge current and parent lookups tests * Merge remote-tracking branch 'origin/unstable' into sync-merged-lookup * Merge branch 'unstable' of https://github.com/sigp/lighthouse into sync-merged-lookup * fix compile after merge * #5655 pr review (#26) * fix compile after merge * remove todos, fix typos etc * fix compile * stable rng * delete TODO and unfilled out test * make download result a struct * enums instead of bools as params * fix comment * Various fixes * Track ignored child components * Track dropped lookup reason as metric * fix test * add comment describing behavior of avail check error * update ordering * delete spammy log (#5672) * delete spammy log * Ensure block only range requests don't fail on download (#5675) * ensure pruned blobs don't fail on download * Typo * Improve ENR updates (#5483) * Improve ENR updates * forever fmt * Appease my old friend clippy * Merge network unstable * Check da_checker before doing a block lookup request (#5681) * Check da_checker before doing a block lookup request * Ensure consistent handling of lookup result * use req resp pre import cache rather than da checker * Update Cargo.lock (#5670) * update rust-yamux * update Cargo.lock * Merge branch 'unstable' of github.com:jxs/lighthouse into update-cargo * Merge branch 'unstable' of github.com:sigp/lighthouse into update-cargo * update to new libp2p versions * Add metric for current epoch total balance (#5688) * Add metric for current epoch total balance --------- Co-authored-by: realbigsean <sean@sigmaprime.io> Co-authored-by: Mac L <mjladson@pm.me> Co-authored-by: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Co-authored-by: Pawan Dhananjay <pawandhananjay@gmail.com> Co-authored-by: Roman Krasiuk <rokrassyuk@gmail.com> Co-authored-by: Eitan Seri-Levi <eserilev@ucsc.edu> Co-authored-by: antondlr <anton@delaruelle.net> Co-authored-by: Jimmy Chen <jchen.tc@gmail.com> Co-authored-by: Ærvin <arvin.morawej@gmail.com> Co-authored-by: Age Manning <Age@AgeManning.com> Co-authored-by: João Oliveira <hello@jxs.pt> Co-authored-by: Michael Sproul <michael@sigmaprime.io>

* Drop lookup type trait for a simple arg * Drop reconstructed for processing * Send parent blocks one by one * Merge current and parent lookups * Merge current and parent lookups clean up todos * Merge current and parent lookups tests * Merge remote-tracking branch 'origin/unstable' into sync-merged-lookup * Merge branch 'unstable' of https://github.com/sigp/lighthouse into sync-merged-lookup * fix compile after merge * sigp#5655 pr review (sigp#26) * fix compile after merge * remove todos, fix typos etc * fix compile * stable rng * delete TODO and unfilled out test * make download result a struct * enums instead of bools as params * fix comment * Various fixes * Track ignored child components * Track dropped lookup reason as metric * fix test * add comment describing behavior of avail check error * update ordering

dapplion added 6 commits April 23, 2024 01:14

Drop lookup type trait for a simple arg

e0b862b

Drop reconstructed for processing

4c8143f

Send parent blocks one by one

3b662de

Merge current and parent lookups

611c37a

Merge current and parent lookups clean up todos

5d29618

Merge current and parent lookups tests

bad87b1

realbigsean added the ready-for-review The code is ready for review label Apr 26, 2024

dapplion and others added 3 commits April 27, 2024 12:16

Merge remote-tracking branch 'origin/unstable' into sync-merged-lookup

13bc011

Merge branch 'unstable' of https://github.com/sigp/lighthouse into sy…

c309c07

…nc-merged-lookup

fix compile after merge

4bde25b

This was referenced Apr 30, 2024

Trigger unknown parent lookups from RPC blob processing #5660

Open

Key single lookups by block root instead of id #5662

Open

#5655 pr review dapplion/lighthouse#26

Merged

realbigsean requested changes Apr 30, 2024

View reviewed changes

realbigsean and others added 2 commits April 30, 2024 14:59

sigp#5655 pr review (#26)

9d185d1

* fix compile after merge * remove todos, fix typos etc * fix compile * stable rng * delete TODO and unfilled out test * make download result a struct * enums instead of bools as params * fix comment

Various fixes

0203843

jimmygchen added the v5.2.0 Q2 2024 label Apr 30, 2024

michaelsproul mentioned this pull request Apr 30, 2024

Release v5.2.0 #5664

Merged

dapplion and others added 3 commits April 30, 2024 20:43

Track ignored child components

e8fc482

Track dropped lookup reason as metric

38eaa8b

fix test

4726cbd

realbigsean mentioned this pull request Apr 30, 2024

Separate block and blob processing errors #5667

Open

realbigsean added 2 commits April 30, 2024 11:08

add comment describing behavior of avail check error

3544fbd

update ordering

49a9e39

realbigsean approved these changes Apr 30, 2024

View reviewed changes

mergify bot added a commit that referenced this pull request Apr 30, 2024

Merge of #5655

e601de7

This was referenced Apr 30, 2024

merge queue: embarking unstable (aea02c6) and [#5615 + #5655] together #5673

Closed

Only portable builds (binaries) #5615

Merged

mergify bot merged commit ce66582 into sigp:unstable Apr 30, 2024
27 checks passed

dapplion deleted the sync-merged-lookup branch May 1, 2024 04:18

dapplion mentioned this pull request May 1, 2024

Check da_checker before doing a block lookup request #5681

Merged

dapplion mentioned this pull request May 28, 2024

Add peers to parent lookups #5858

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge parent and current sync lookups #5655

Merge parent and current sync lookups #5655

dapplion commented Apr 26, 2024 •

edited

Loading

realbigsean Apr 30, 2024

realbigsean Apr 30, 2024

dapplion Apr 30, 2024

realbigsean Apr 30, 2024

dapplion commented Apr 30, 2024

realbigsean left a comment

realbigsean commented Apr 30, 2024

mergify bot commented Apr 30, 2024 •

edited

Loading

Merge parent and current sync lookups #5655

Merge parent and current sync lookups #5655

Conversation

dapplion commented Apr 26, 2024 • edited Loading

Issue Addressed

Proposed Changes

Next steps

realbigsean Apr 30, 2024

Choose a reason for hiding this comment

realbigsean Apr 30, 2024

Choose a reason for hiding this comment

dapplion Apr 30, 2024

Choose a reason for hiding this comment

realbigsean Apr 30, 2024

Choose a reason for hiding this comment

dapplion commented Apr 30, 2024

realbigsean left a comment

Choose a reason for hiding this comment

realbigsean commented Apr 30, 2024

mergify bot commented Apr 30, 2024 • edited Loading

✅ The pull request has been merged automatically

dapplion commented Apr 26, 2024 •

edited

Loading

mergify bot commented Apr 30, 2024 •

edited

Loading