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

Sidecar inclusion self review cleanup #4949

Conversation

realbigsean
Copy link
Member

Proposed Changes

  • Fixes a bug found by @marioevz - our json serde of blob lists was incorrect in the beacon apis
  • uses the ef_tests feature to mock out the proposing_at_re_org_slot logic
  • adds block root to GossipVerifiedBlob to make sure we calculate it at least once less, but we do calculate it other times, opened an issue related to this here: Block root caching in blob sidecar #4948
  • cleaned up some todo's

@realbigsean realbigsean added ready-for-review The code is ready for review deneb labels Nov 24, 2023
@@ -888,7 +887,6 @@ pub struct SseBlock {
}

#[derive(PartialEq, Debug, Serialize, Deserialize, Clone)]
// TODO(pawan): modify spec to maybe include proofs too?
Copy link
Member Author

Choose a reason for hiding this comment

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

Since we don't send the full block in the block event, I think it's ok to omit the inclusion proofs

Copy link
Member

Choose a reason for hiding this comment

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

Ohh did not realise this, seems good then

Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

Mergee it

@@ -888,7 +887,6 @@ pub struct SseBlock {
}

#[derive(PartialEq, Debug, Serialize, Deserialize, Clone)]
// TODO(pawan): modify spec to maybe include proofs too?
Copy link
Member

Choose a reason for hiding this comment

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

Ohh did not realise this, seems good then

@realbigsean realbigsean merged commit c6be31c into sigp:sidecar-inclusion-proof Nov 24, 2023
@realbigsean realbigsean deleted the sidecar-inclusion-selfreview branch November 24, 2023 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deneb ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants