-
Notifications
You must be signed in to change notification settings - Fork 961
Add Gloas data column support #8682
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
base: unstable
Are you sure you want to change the base?
Conversation
d0b22fb to
abd2a8c
Compare
abd2a8c to
1ef6104
Compare
|
Thanks! 🙌 Looks like this would have been a lot of work 🙏 |
| /// Stores all received data indices for a given `(ValidatorIndex, Slot)` tuple. | ||
| items: HashMap<ProposalKey, HashSet<u64>>, | ||
| /// Stores all received data indices for a given `slot`. | ||
| items: HashMap<Slot, HashSet<u64>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem to be a reason why we use ProposalKey instead of Slot. post-gloas data columns no longer have a signed_beacon_block_header field which means we cant easily access the proposer_index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for DOS protection - if we use slot, an attacker could produce a column sidecar that passes gossip and prevent other column sidecars from getting processed.
Checking the spec, it looks like we need to use beacon_block_root and index
https://github.com/ethereum/consensus-specs/blob/master/specs/gloas/p2p-interface.md#data_column_sidecar_subnet_id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah okay they key is now block root and slot
42c71c5
|
Some required checks have failed. Could you please take a look @eserilev? 🙏 |
| }) | ||
| .collect::<Result<Vec<_>, _>>()?; | ||
|
|
||
| reconstruct_blobs(&chain.kzg, data_columns, blob_indices, block, &chain.spec).map_err( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reconstruct_blobs() will return error in gloas, because signed_block_header and kzg_commitments_inclusion_proof no longer exists in the Gloas DataColumnSidecar - may be worth adding a TODO here so we don't miss this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added d7b692a
|
Some required checks have failed. Could you please take a look @eserilev? 🙏 |
|
|
||
| #[derive(Debug, PartialEq, Clone)] | ||
| pub enum Error { | ||
| IncorrectStateVariant, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could just add this to DataColumnSidecarError?
| bytes: &[u8], | ||
| fork_name: ForkName, | ||
| ) -> Result<Self, ssz::DecodeError> { | ||
| if fork_name.gloas_enabled() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be safer to use match here, in the unlikely case if a new variant is added we don't convert into the wrong type here.
| SszStaticHandler::<DataColumnSidecarFulu<MinimalEthSpec>, MinimalEthSpec>::fulu_only() | ||
| .run(); | ||
| SszStaticHandler::<DataColumnSidecar<MainnetEthSpec>, MainnetEthSpec>::fulu_and_later() | ||
| SszStaticHandler::<DataColumnSidecarFulu<MainnetEthSpec>, MainnetEthSpec>::fulu_only() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add Gloas too?
| } | ||
|
|
||
| pub(crate) fn blobs_known_for_proposal( | ||
| pub(crate) fn blobs_known_for_observation_key( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we only want to use this new tuple from gloas?
| } | ||
|
|
||
| pub(crate) fn data_column_known_for_proposal( | ||
| pub(crate) fn data_column_known_for_observation_key( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above - i think fulu rules remains unchanged
| if let Some(observed_columns) = chain_adapter_cloned.data_column_known_for_proposal( | ||
| ProposalKey::new(block.message().proposer_index(), block.slot()), | ||
| ) { | ||
| if let Some(observed_columns) = chain_adapter_cloned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may need separate logic for fulu and gloas here
| BlockComponent::DataColumn(column) => match column.value.as_ref() { | ||
| DataColumnSidecar::Fulu(column) => column.block_parent_root(), | ||
| // TODO(gloas) we don't have a parent root post gloas, not sure what to do here | ||
| DataColumnSidecar::Gloas(column) => column.beacon_block_root, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might actually be correct for lookup but need to confirm - the parent is technically the beacon block root - if we don't know the beacon block root, we'd have to look it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, yeah it might be correct. Pawan said he's looking into this as he's working on lookup sync
| slasher.accept_block_header(data_colum.signed_block_header()); | ||
| for data_column in &data_columns { | ||
| // TODO(gloas) no block header post-gloas, what should we do here | ||
| if let DataColumnSidecar::Fulu(c) = data_column.as_data_column() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this can be skipped and superceded by a different gossip check in gloas:
| let data_indices = self | ||
| .items | ||
| .entry(ProposalKey { | ||
| .entry(ObservationKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ObservationKey may have to be an enum now to support Gloas, and we could pass ObservationKey and leave it to the caller to construct
jimmygchen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
I haven't finished but I've added comments I have so far - we mainly need to make sure it doesn't impact / change Fulu code path before we merge this, so I think we'll need to retain the ObservedDataSidecars behaviour for Fulu.
|
I know a new observation key technically affects fulu code paths but it seems like unnecessary overhead to maintain separate code paths for this section of the code. Is there a reason why block root + slot won't work the same as proposer index + slot? |
|
re: observation key |
333563c to
86b105a
Compare
data columns: https://github.com/ethereum/consensus-specs/blob/master/specs/gloas/p2p-interface.md#modified-datacolumnsidecar
There are some TODOs related to gloas data column gossip verification that should be handled in a separate PR