Skip to content

Conversation

ethDreamer
Copy link
Member

@ethDreamer ethDreamer commented May 26, 2025

@ethDreamer ethDreamer requested a review from jxs as a code owner May 26, 2025 21:04
@dapplion dapplion changed the title Implement EIP-7892 Implement EIP-7892 BPO hardforks May 26, 2025
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.

LGTM, just minor nits.
I think we should merge this asap for the devnet, given the spec is a bit in flux, I think its okay to merge this with the TODOs and iterate as the spec gets clearer.

match self.fulu_fork_epoch {
Some(fulu_epoch) if epoch >= fulu_epoch => {
let mut max_blobs_per_block = self.max_blobs_per_block_electra;
let mut blob_schedule = self.blob_schedule.clone();
Copy link
Member

Choose a reason for hiding this comment

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

why not sort the chainspec blob_schedule at construction from the config instead of sorting it everytime on function call?

Copy link
Member Author

Choose a reason for hiding this comment

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

My concern was that the spec was constructed in multiple places and I want to ensure the blob schedule was always sorted. I've done that now by adding a wrapper BlobSchedule type. Let me know what you think :)

let mut max_blobs_per_block = self.max_blobs_per_block_electra;
let mut blob_schedule = self.blob_schedule.clone();
blob_schedule.sort_by_key(|entry| entry.epoch);
for entry in blob_schedule {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can do something like

    let index = self.blob_schedule.partition_point(|entry| entry.epoch <= epoch);
    if index > 0 {
        max_blobs = self.blob_schedule[index - 1].max_blobs_per_block;
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the function to be implemented on the BlobSchedule. It's simpler now. Let me know what you think :)

@pawanjay176
Copy link
Member

Tested this with the interop config in https://notes.ethereum.org/@ethpandaops/fusaka-devnet-0#Kurtosis-Interop-Conifg-Pre-devnet-testing

Let it run for 3 epochs which covered an increase and decrease in the blob count and it runs perfectly with the other clients. 🎉

@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. fulu Required for the upcoming Fulu hard fork labels May 27, 2025
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.

LGTM, the CI failures seem to be unrelated to this PR
cc @jimmygchen

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels May 29, 2025
@jimmygchen
Copy link
Member

Nice one! thanks 🎉

mergify bot added a commit that referenced this pull request May 29, 2025
Copy link

mergify bot commented May 29, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You can check the last failing draft PR here: #7531.

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@jimmygchen
Copy link
Member

LGTM, the CI failures seem to be unrelated to this PR cc @jimmygchen

Indeed, the RPC one has been fixed in #7522.
I created a PR for the AvailabilityCheckError::Unexpected failure here #7533.

@jimmygchen
Copy link
Member

@mergify requeue

Copy link

mergify bot commented May 29, 2025

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

mergify bot added a commit that referenced this pull request May 29, 2025
Copy link

mergify bot commented May 29, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You can check the last failing draft PR here: #7535.

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@jimmygchen
Copy link
Member

Indeed, the RPC one has been fixed in #7522. I created a PR for the AvailabilityCheckError::Unexpected failure here #7533.

Ah it was actually not that (even though it revealed a real bug that triggers the same failure) - the test is actually failing 100% now in its current state, because the failing test expects a blob, but the random block generator didn't create a block with blobs.

This comment was marked as duplicate.

@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-merge This PR is ready to merge. labels May 29, 2025
@jimmygchen
Copy link
Member

Indeed, the RPC one has been fixed in #7522. I created a PR for the AvailabilityCheckError::Unexpected failure here #7533.

Ah it was actually not that (even though it revealed a real bug that triggers the same failure) - the test is actually failing 100% now in its current state, because the failing test expects a blob, but the random block generator didn't create a block with blobs.

@ethDreamer I've added a setter method to the execution block generator to allow modifying blob generation behaviour on the fly.

Basically the test is failing because it expects blobs, but there's no blobs generated for the next block (passing before due to rng), and passing empty Vec<DataColumnSidecar> to DA checker would return an error. The fix ensures rig.next_blobs or rig.next_data_column is always non empty.

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jun 2, 2025
Copy link

mergify bot commented Jun 2, 2025

This pull request has been removed from the queue for the following reason: pull request manually updated.

The pull request #7521 has been manually updated.

If you want to requeue this pull request, you can post a @mergifyio requeue comment.

@jimmygchen
Copy link
Member

@Mergifyio requeue

Copy link

mergify bot commented Jun 2, 2025

requeue

☑️ This pull request is already queued

@mergify mergify bot merged commit ae30480 into sigp:unstable Jun 2, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fulu Required for the upcoming Fulu hard fork ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants