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

EIP4844: remove optimistic sync mentions #3125

Merged
merged 2 commits into from
Dec 6, 2022

Conversation

terencechain
Copy link
Contributor

@terencechain terencechain commented Nov 22, 2022

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

Copy link
Contributor

@djrtwo djrtwo left a 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

@dankrad
Copy link
Contributor

dankrad commented Nov 22, 2022

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?

Copy link
Collaborator

@dapplion dapplion left a 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.

@terencechain
Copy link
Contributor Author

@dapplion perhaps we can just remove the processed further optimistically... part, that's ultimately what we decide against

@tbenr
Copy link
Contributor

tbenr commented Nov 28, 2022

I'm ok with leaving it. My current implementation is taking in account that data might be retrieved async in the future.

@tbenr
Copy link
Contributor

tbenr commented Nov 28, 2022

remove the processed further optimistically... part

👍

@hwwhww hwwhww added the Deneb was called: eip-4844 label Nov 28, 2022
@terencechain
Copy link
Contributor Author

@tbenr @dapplion please take another look 🙏🏼

Copy link
Collaborator

@dapplion dapplion left a 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

@terencechain terencechain changed the title EIP4844: remove is_data_available EIP4844: remove optimistic sync mentions Dec 5, 2022
@djrtwo
Copy link
Contributor

djrtwo commented Dec 6, 2022

I'm good with this now. @terencechain can you patch the description of the issue given the scope of the issue now?

@terencechain
Copy link
Contributor Author

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

@djrtwo djrtwo merged commit 5498519 into ethereum:dev Dec 6, 2022
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.

6 participants