Add DataColumnSidecarsByRoot req/resp protocol

[jimmygchen]

Issue Addressed

Part of #4983.

Proposed Changes

Add implementation for DataColumnSidecarsByRoot.

Currently blobs are served from three sources, in the following order

1. data availability checker (before we have all the blocks and blobs)
2. early attester cache (before we import the block)
3. blobs db (after import)

I've implemented the equivalent for data columns, and it currently reuses the blobs db but with a different DbColumn to separate data columns from blobs.

This PR does not cover the following:

1. Sending outbound requests
2. Handling of responses from peers

[eserilev]

What happens here if we have both blobs and data_columns available? I guess we'd return a BlockAndBlobs variant? Would that imply that if blobs are avail, we'll just skip the data sampling checks?

would a BlockAndBlobsAndDataColumns variant allow us to support both blobs and data_columns until were ready to migrate away from blob subnets completely?

[jimmygchen]

Yeah, so originally I was thinking to prioritise blobs, but then it's kind of pointless because we aren't executing the data column code paths.

I've been thinking about this and talking to Sean about this as well. A potentially way to introduce this and test it out may be add column subnets and sampling (without making it mandatory for is_data_avaialble, perhaps log warnings if it fails), and keep the existing blob subnets and DA filter logic - so perhaps an intermediate BlockAndBlobsAndDataColumns variant may make sense until we fully switch over to 1D PeerDAS.

[dapplion]

Can you

match self {
  RpcBlockInner::*=> RpcBlock
}

so the enum type guarantees that blobs and columns do not exist at the same time

[jimmygchen]

Not sure if I'm understanding correctly but self is actually a struct type which contains optional blobs and columns:

lighthouse/beacon_node/beacon_chain/src/data_availability_checker.rs

Lines 603 to 608 in e707536

	pub struct AvailableBlock<E: EthSpec> {
	block_root: Hash256,
	block: Arc<SignedBeaconBlock<E>>,
	blobs: Option<BlobSidecarList<E>>,
	data_columns: Option<DataColumnSidecarList<E>>,
	}

[jimmygchen]

For an RpcBlock, we will have either block + blobs or block + samples if we've transitioned to DAS.

[eserilev]

I think match statements are executed in order from left to right. There might be a weird edge case here where both data columns and blobs are avail, but we return 0. If we're trying to support both blobs and data_columns initially, this edge case has the potential to cause issues

[jimmygchen]

Yep, you're right - perhaps we can add the BlockAndBlobsAndDataColumns variant to the other arm?

[jimmygchen]

So this type is used for RPC responses, so i don't think blobs and data column will coexist - we either request for block + blobs or block + data columns during lookups and sync, depending on the fork we're at.

[eserilev]

is there a similar data_columns_required_for_blocks check we'll need to make here in the future?

[jimmygchen]

They are the same condition - blobs_required_for_block checks for number of commitments in the block - if there are commitments, there are blobs and we should verify data columns if they exist.

[eserilev]

I think we may want a Vec<KeyValueStoreOp> dedicated to data_columns instead of re-using blob_batch

[jimmygchen]

I think the benefit I see with using a dedicated one is if we want to write to a separate database? I'm re-using the blob db for simplicity here (since the maintainence/pruning logic are the same), but happy to create a separate db if there's good reasons to do that.

[jimmygchen]

So I'm reusing the same blobs db for data columns but using a different DbColumn to separate data columns from blobs.

It feels like we could benefit from better UX - no change for users that's already using the blobs-db flag and save on some code duplication.

[jimmygchen]

@dapplion @eserilev
I've addressed the review comments and I think this is ready for another review.

There are a few things that need more work but I think it's probably best to keep them separate from this one as this PR is already quite bloated (mostly boilerplates) and further work depends on some of the changes here. It would be good to merge this soon and iterate from there.

There are a few things to note that are not covered here:

1. There's no guarantee that the data column will be stored properly, because:
   • We don't wait for them to arrive before importing the block - I think we'll need another condition under chain.process_gossip_data_column to handle the new code path, i.e. if block has been imported, just verify and store them on disk, otherwise it goes through the existing DA checker code path.
   • Look up via RPC has not yet been implemented
2. There are some recent changes to the consensus-spec PeerDAS PR, I think it's best to update them in a new PR.

[AgeManning]

I skimmed this. Looks like a lot of boiler plate. Others may want to review, but happy to merge down to build on this.