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.

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

Standardize DAS data store in the fork-choice store #3993

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

ppopth
Copy link
Member

@ppopth ppopth commented Oct 22, 2024

Previously we left the DAS data store (blob store in Deneb and data column store in PeerDAS) implementation-dependent. I noticed that that left many issues on the specification.

Currently, a sidecar is verified using only the GossipSub rules

This strictly disallows us to receive sidecars using alternative methods other than GossipSub. This has a bad effect on PeerDAS because it's explicitly stated that you can get a data column sidecar using Req/Resp as well. So. I decided to propose on_blob_sidecar and on_data_column_sidecar to be entry points for respective sidecars and all the verification will be done there before importing them to the store, in addition to the GossipSub rules.

You can think of the GossipSub rules as rules to decide whether to forward a message further or not, while the fork-choice handlers, like on_blob_sidecar and on_data_column_sidecar, are used to decide whether to import an object to the store or not.

You will probably notice that the verification done in the fork-choice handlers is the same as the one in the GossipSub rules. Yes, they are mostly the same and we did that since the beginning of the beacon chain (look at on_block for an example). It's not trivial to refactor this duplicate because the expected result of the fork-choice handlers is ACCEPT or FAIL, while the expected result of the Gossipsub is ACCEPT, IGNORE, or REJECT.

If the DAS data store is implementation-dependent, it's hard to make assumptions on what is inside the store and extend the spec further

Let's look at is_data_available from both Deneb and PeerDAS.

In PeerDAS is_data_available, we are doing some verification using verify_data_column_sidecar and verify_data_column_sidecar_kzg_proofs. I'm not sure why we do some verification here because we already did the whole verification using the GossipSub rules which are more comprehensive.

It's probably because we assume that retrieve_column_sidecars probably will return some invalid sidecars. If that's the case, why do we do only two verification rules instead of doing the whole bunch again.

If we assume that retrieve_column_sidecars will return only valid sidecars, why bother doing verification at all?

The same argument can be applied to verify_blob_kzg_proof_batch in Deneb is_data_available as well.

So I think the solution is to standardize the data store.

Some notes

  1. verify_blob_kzg_proof_batch can be completely removed from the Deneb crypto library which is interesting.
  2. I will do the test after the idea is accepted.

@jtraglia jtraglia added general:enhancement New feature or request EIP-7594 PeerDAS labels Oct 22, 2024
@jtraglia
Copy link
Member

jtraglia commented Nov 4, 2024

@ppopth I like this PR, could you fix the CI errors? Then I'll give it another review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EIP-7594 PeerDAS general:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants