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

add note about not invalidating valid and available blocks #3526

Merged
merged 3 commits into from
Oct 23, 2023
Merged

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented Oct 20, 2023

This came up on the call today as a path that is not entirely clear in the spec.

I think that this note is either appropriate here (before is_data_available()) or embedded as a comment in on_block() around the call to is_data_available(). Open to thoughts on how to make sure we are abundantly clear here.

Copy link
Contributor

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

LGTM. Might consider adding an extra mention for ‘with KZG referenced blobs’ to clarify that it’s the block that should be valid, rather than suggesting a valid block could contain an invalid blob

@tbenr
Copy link
Contributor

tbenr commented Oct 20, 2023

I think that this note is either appropriate here (before is_data_available()) or embedded as a comment in on_block() around the call to is_data_available(). Open to thoughts on how to make sure we are abundantly clear here.

I think that is good to have a mention on on_block(). The concept that is important to highlight is that if is_data_available fails the block MUST not be considered invalid per se.

While over is_data_available definition is important to highlight that must not fail if sees extraneous blob_sidecars, and MUST return true as long as a valid blob_sidecar has been seen\received for each block's commitment.

@djrtwo
Copy link
Contributor Author

djrtwo commented Oct 20, 2023

okay, the warning is now in both places and makes a note about these being in addition to the expected/referenced blobs

Copy link
Contributor

@tbenr tbenr left a comment

Choose a reason for hiding this comment

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

LGTM

@hwwhww hwwhww added the Deneb was called: eip-4844 label Oct 21, 2023
specs/deneb/fork-choice.md Outdated Show resolved Hide resolved
@djrtwo djrtwo merged commit 612d148 into dev Oct 23, 2023
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deneb was called: eip-4844
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants