Skip to content

chore: add comment to PendingComponents #7979

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.

Sign up for GitHub

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

Merged
mergify merged 4 commits into from
Sep 10, 2025
Merged

chore: add comment to PendingComponents #7979

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d7687a9
add small comment
kevaundray Sep 2, 2025
ebaaa56
Merge branch 'unstable' into kw/add-comment-pending-comp
jimmygchen Sep 9, 2025
ab36402
Merge branch 'unstable' into kw/add-comment-pending-comp
jimmygchen Sep 9, 2025
92d3bdd
Merge branch 'unstable' into kw/add-comment-pending-comp
jimmygchen Sep 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,15 @@ use types::{
///
/// The blobs are all gossip and kzg verified.
/// The block has completed all verifications except the availability check.
///
/// There are currently three distinct hardfork eras that one should take note of:
/// - Pre-Deneb: No availability requirements (Block is immediately available)
/// - Post-Deneb, Pre-PeerDAS: Blobs are needed, but columns are not for the availability check
/// - Post-PeerDAS: Columns are needed, but blobs are not for the availability check
///
/// Note: from this, one can immediately see that `verified_blobs` and `verified_data_columns`
/// are mutually exclusive. i.e. If we are verifying columns to determine a block's availability
/// we are ignoring the `verified_blobs` field.
pub struct PendingComponents<E: EthSpec> {
Copy link
Contributor Author

@kevaundray kevaundray Sep 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC This structure will work across fork boundaries because a lot of the logic will explicitly handle the different forks at runtime. So merge_blobs if called during peerDAS epoch, will do nothing for example.

If that's correct then I think something like this:

pub enum PendingComponents<E: EthSpec> {
    PostDeneb {
        block_root: Hash256,
        executed_block: Option<DietAvailabilityPendingExecutedBlock<E>>,
        verified_blobs: RuntimeFixedVector<Option<KzgVerifiedBlob<E>>>,
        span: Span,
    },
    
    PostPeerDAS {
        block_root: Hash256,
        executed_block: Option<DietAvailabilityPendingExecutedBlock<E>>,
        verified_data_columns: Vec<KzgVerifiedCustodyDataColumn<E>>,
        reconstruction_started: bool,
        span: Span,
    },

Might express the intentions clearer and then it would be possible to panic if PendingComponents is PostPeerDAS but merge_blobs has been called for example.

Copy link
Member

@jimmygchen jimmygchen Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kevaundray - Agree the above is cleaner, although we may not refactor this further at this stage as we're in the process of redesigning DA checker but we'll keep this in mind, thanks!

Copy link
Contributor Author

@kevaundray kevaundray Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah this is great to know -- is there currently a draft branch or doc for this refactor?

pub block_root: Hash256,
pub verified_blobs: RuntimeFixedVector<Option<KzgVerifiedBlob<E>>>,
Expand Down
Loading