-
Notifications
You must be signed in to change notification settings - Fork 955
Convert RpcBlock to an enum that indicates availability #8424
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
|
Some required checks have failed. Could you please take a look @eserilev? 🙏 |
|
need to fix a few tests but the general idea is here |
…c-block-avail-varaint
…c-block-avail-varaint
|
Some required checks have failed. Could you please take a look @eserilev? 🙏 |
beginning changes
…c-block-avail-varaint
|
Some required checks have failed. Could you please take a look @eserilev? 🙏 |
| /// viewed the same as `None` passed in. | ||
| pub fn new( | ||
| block_root: Option<Hash256>, | ||
| pub fn new<T>( |
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.
Add a comment above specifying when to use Some(block_data) or None
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 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.
sorry where was this added? can't find it
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.
whoops comments are now added: ab54242
|
|
||
| #[derive(Debug)] | ||
| #[derive(Debug, Clone)] | ||
| pub enum AvailableBlockData<E: EthSpec> { |
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.
future refactor - move this to block_verification_types
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 TODO
| } | ||
| } | ||
| } | ||
|
|
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.
future refactor - move to block_verification_types
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 TODO
| // Test that `signature_verify_chain_segment` correctly handles a mix of `FullyAvailable` | ||
| // and `BlockOnly` RpcBlocks. This situation should not happen in production | ||
| // but this test may help catch potential future regressions. | ||
| #[tokio::test] |
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.
flip this test
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.
test is flipped
| assert_eq!(verified[i].block_root(), *expected_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.
test for successfully importing blocks without columns (that need them) when past the data availability boundary
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.
…/lighthouse into rpc-block-avail-varaint
|
Some required checks have failed. Could you please take a look @eserilev? 🙏 |
pawanjay176
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.
Really clean! Thanks for fixing this up.
Just minor nits and a question
| /// viewed the same as `None` passed in. | ||
| pub fn new( | ||
| block_root: Option<Hash256>, | ||
| pub fn new<T>( |
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.
sorry where was this added? can't find it
| blobs_available_timestamp: None, | ||
| spec: self.spec.clone(), | ||
| }) | ||
| for available_block in available_blocks { |
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.
maybe we can move this to before the kzg verification to avoid expensive verification if the batch doesn't have required data? not super necessary though
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.
RpcBlock
Convert
RpcBlockinto an enum with two variants:RpcBlock::FullyAvailableIndicates a fully available RPC block. An RpcBlock of this variant is guaranteed to have all blobs or the columns its required to custody. In the future we could move KZG verification as part of
FullyAvailableconstruction validation logic so that we also have type safety guarntees that KZG verification for the block data is also validRpcBlock::BlockOnlyIndicates a maybe available block. No blob/column data has been fetched and we don't check if the block is expected to have any blob data. This variant is mostly used in lookup sync.
When constructing an
RpcBlockthe arguments provided decide wether its aFullyAvailableorBlocksOnlyvariantif
Noneis passed in forblock_datathis indicates aBlocksOnlyvariant. If aSomevalue is passed in it will be treated as aFullyAvailablevariant. See below for the variousblock_datavariants:AvailableBlockDataWhen constructing the different variants of
AvailableBlockDatathe following checks are madeAvailableBlockData::NoData: no blobs are expected for the associated blockAvailableBlockData::Blobs: all expected blobs are present and the block is post-Deneb and pre-PeerDASAvailableBlockData::DataColumns: all expected data columns for the nodes custody requirements are present