Transition block lookup sync to range sync

[dapplion]

Issue Addressed

For context of the issue 👇

• Allow to sync a long unknown fork #6099

Closes #6099

Proposed Changes

This solution links range sync and lookup sync by:

• Trigger lookup sync with status messages that include unknown root. Currently range sync "swallows" peer statuses that are too close to head but on a potentially unknown fork. Range sync is not suitable to handle short forks, so instead deffer to lookup sync to retrieve that unknown root.
• If lookup sync reaches the max chain length, trigger range sync on that set of peers. The idea is that most peers in that group should have this chain as canonical so we can blocks_by_range from them. In the future we may use Add BeaconBlocksByRange v3 ethereum/consensus-specs#3845 to be sure what fork are we syncing from.

I believe this change is the minimal solution to cover the corner case of #6099. It's not maximally efficient as it will download a section of blocks twice. However this situation is rare, and we just need some way to recover even if it's at the expense of some extra bandwidth.

Testing

There are plans to with EF devops to test nasty long non-finality situations, but not now. For now I can try to write representative unit sync tests with some sample scenarios. This change should not impact good network codepaths, so should have no downsides. Range sync will only be triggered when we need to discover a long unknown fork, which has only happened in LH in cooked testnets of lookup sync bugs.

[dapplion]

Updated tests to assert a syncing chain is created. Plus removed down-scoring the peers for a long chain

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

Lines 294 to 299 in 4ec5594

	// Do not downscore peers here. Because we can't distinguish a valid chain from
	// a malicious one we may penalize honest peers for attempting to discover us a
	// valid chain. Until blocks_by_range allows to specify a tip, for example with
	// https://github.com/ethereum/consensus-specs/pull/3845 we will have poor
	// attributability. A peer can send us garbage blocks over blocks_by_root, and
	// then correct blocks via blocks_by_range.

As a final item we could check that for a syncing chain if it reaches the end, assert that the block root matches that syncing root

[AgeManning]

I ran out of time looking through to find out the scenario when this occurs.

Its when we don't have any single block lookups that match the current parent tip. In this case we start a range sync based on the parents ancestor, rather than the parent_chain_tip?

I didn't fully parse this.

Overall I like the idea!

[dapplion]

This should never happen. I can log an error and return, but used this fallback value for "simplicity". Happy to change if this confuses more than helps

[AgeManning]

Oh yeah. I think that might be helpful. This way we can catch any wild states rather than doing a range sync. Also (at least for me, will make it easier to follow).

[AgeManning]

Yeah I think we should test this out. Logic seems reasonable to me.

[realbigsean]

LGTM

[realbigsean]

@mergify queue

[mergify]

queue

🛑 The pull request has been removed from the queue default

The merge conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

[realbigsean]

@mergify refresh

[mergify]

refresh

✅ Pull request refreshed

[realbigsean]

@mergify queue

[mergify]

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 71c5388