Forward sync columns by root

[pawanjay176]

Issue Addressed

N/A

The Problem

Our current strategy of syncing blocks + columns by range works roughly as follows for each batch:

1. Find a peer from the current SyncingChain to fetch blocks from and send a BlocksByRange request
2. Find peer(s) from the global peer pool that should have columns for the same batch based on their status message and the columns they are supposed to custody , and send them a by DataColumnsByRange request at the same time
3. Once we get responses for all blocks + columns components, try to couple them by checking that the block_root and the kzg_commitment matches. If the coupling failed, try to re-request the failed columns.
4. Send them for processing and try to progress the chain

This strategy works decently well when the chain is finalizing as most of our peers are on the same chain. However, in times of non-finality we need to potentially sync multiple head chains. This leads to issues with our current approach because the block peer and the data column peers might have a different view of the canonical chain due to multiple heads. So when we use the above approach, it is possible that the block peer returns us a batch of blocks for chain A while some or all data column peers send us the batch of data columns for a different chain B. Different data column peers might also be following different chains.

We initially tried to get around this problem by selecting column peers only from within the current SyncingChain. Each SyncingChain represents a head_root that we are trying to sync to and we group peers based on same head_root. That way, we know for sure that the block and column peers are on the same chain. This works in theory, but in practice, during long periods of non-finality, we tend to create multiple head chains based on the head_root and split the global peerset. Pre-fulu, this isn't a big deal since all peers are supposed to have all the blob data. But splitting peers with peerdas is a big challenge due to not all peers having the full data available. There are supernodes, but during bad network conditions, supernodes would be getting way too many requests and not even have any incoming peer slots. As we saw on fusaka devnets, this strategy leads to sync getting stalled and not progressing.

Proposed Changes

1. Use DataColumnsByRoot instead of DataColumnsByRange to fetch columns for forward sync

This is the main change. The new strategy would go as follows:

1. Find a peer from the current SyncingChain to fetch blocks from and send a BlocksByRange request
2. Hold off on requesting for columns until we receive the response for the above blocks request
3. Once we get the blocks response, extract all the block_roots and trigger a DataColumnsByRoot request for every block in the response that has any blobs based on the expected_kzg_commitments field.
4. Since we request by root, we know what we are expecting in the response. The column peer's view of the canonical chain might be chain A, but if we ask for chain B and they have chain B in their fork choice, they can still serve us what we need.
5. We couple the block + column responses and send them for processing as before.

(4) kinda assumes that most synced/advanced peers would have different chains in their fork choice to be able to serve specific by root requests. My hunch is that this is true, but we should validate this in a devnet 4 like chain split scenario.

Note: that we currently use this by root strategy only for forward sync, not for backfill. Backfill has to deal with only a single canonical chain so byrange requests should work well there.

2. ResponsiblePeers to attribute peer fault correctly

Adds the ResponsiblePeers struct which stores the block and the column peers that we made the download requests to. For most of our peer attributable errors, the processing error indicates whether the block peer was at fault or if the column peer was at fault.

We now communicate this information back to sync and downscore specific peers based on the fault type. This imo, is an improvement over current unstable where most of the time, we attribute fault to the peer that "completed" the request by being the last peer to respond. Due to this ambiguity in fault attribution, we weren't downscoring pretty serious processing errors like InvalidKzgProofs, InvalidExecutionPayload etc. I think this PR attributes the errors to the right peers. Reviewers please check that this claim is actually true.

3. Make AwaitingDownload an allowable in-between state

Note: This has been extracted to its own PR here and merged #7984 Prior to peerdas, a batch should never have been in AwaitingDownload state because we immediataly try to move from AwaitingDownload to Downloading state by sending batches. This was always possible as long as we had peers in the SyncingChain in the pre-peerdas world.

However, this is no longer the case as a batch can be stuck waiting in AwaitingDownload state if we have no peers to request the columns from. This PR makes AwaitingDownload to be an allowable in between state. If a batch is found to be in this state, then we attempt to send the batch instead of erroring like before. Note to reviewer: We need to make sure that this doesn't lead to a bunch of batches stuck in AwaitingDownload state if the chain can be progressed.

[mergify[bot]]

Some required checks have failed. Could you please take a look @pawanjay176? 🙏

[pawanjay176]

Why not use the same strategy for backfill sync too?

I don't want to nuke the existing coupled strategy we have got just yet. Also, for backfill, since its one chain, the coupled approach makes more logical sense imo. Want to test this out in the wild sufficiently and then we can use this uncoupled strategy once we are confident it works.

[pawanjay176]

This one should be ready for another round of review. Used claude to fix up the sync tests, lmk if you wanted a different approach. I don't have strong opinions on the test suite.

[mergify[bot]]

This pull request has merge conflicts. Could you please resolve them @pawanjay176? 🙏

[dapplion]

@pawanjay176 this commit merges data_columns_by_root_range_requests and data_columns_by_root_requests with a -216 lines diff. Feel free to cherry-pick into your branch but I feel the SyncNetworkContext is easier to understand this way https://github.com/sigp/lighthouse/commit/26ed0ebae

[pawanjay176]

@dapplion I'm not sure what's the best way to handle https://github.com/sigp/lighthouse/pull/7946#discussion_r2380520302

Other than that, i think this PR is in a pretty good shape and can be merged. If we see any critical issues in subsequent testnet forks, we can go back to the unstable model pretty easily by changing a single line here https://github.com/pawanjay176/lighthouse/blob/15df3d26e38d846848598f2d34e7d2b101c12f74/beacon_node/network/src/sync/network_context.rs#L1511

Sync speed that i'm observing is pretty similar to unstable. Let me know if you have any other reservations to merging this

[mergify[bot]]

Hi @pawanjay176, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.

[mergify[bot]]

Hi @pawanjay176, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.