-
Notifications
You must be signed in to change notification settings - Fork 1k
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
EIP4844: remove optimistic sync mentions #3125
Conversation
09f4bb7
to
3624742
Compare
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 like making clear the abstraction of DA in the spec, especially because the sidecar is not a 1st class consensus object.
That said, not too strongly held position
I kind of like the idea of keeping it as it would make it very easy to transition from 4844 to full sharding just be changing the implementation of that function. Also, are there no other ways of getting blocks? How about when syncing? |
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.
While not impactful in the implementation, the existence of is_data_available
helped me when reading the specs for the first time. I think we should leave it, since it consistently ties to the future roadmap, and hints on its implications.
@dapplion perhaps we can just remove the |
I'm ok with leaving it. My current implementation is taking in account that data might be retrieved async in the future. |
👍 |
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.
LGTM, but you should rename PR to something like "remove optimistic sync mentions", until we have better consensus on it
is_data_available
I'm good with this now. @terencechain can you patch the description of the issue given the scope of the issue now? |
Sorry I missed that. I just updated the description |
This PR removes optimistic sync mentions around
is_data_available
. As was discussed, we don't want to enable clients to sync optimistically if the blob is not available