Skip to content

Conversation

@adiasg
Copy link
Contributor

@adiasg adiasg commented Jan 13, 2024

What type of PR is this?
Feature

What does this PR do? Why is it needed?
Changes the fork choice filteration logic as needed by ethereum/consensus-specs#3431

@adiasg adiasg requested a review from a team as a code owner January 13, 2024 00:03
@CLAassistant
Copy link

CLAassistant commented Jan 13, 2024

CLA assistant check
All committers have signed the CLA.

@potuz
Copy link
Contributor

potuz commented Jan 13, 2024

Discussing offline the changes

"max_blobs_per_block.size": "6",
"max_blob_commitments.size": "16",
"kzg_commitment_inclusion_proof_depth.size": "9",
"kzg_commitment_inclusion_proof_depth.size": "17",
Copy link
Member

Choose a reason for hiding this comment

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

Please explain this change. Was there an upstream spec change for this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe it's this one: ethereum/consensus-specs#3255

Copy link
Member

Choose a reason for hiding this comment

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

This change fails test //testing/spectest/minimal/deneb/ssz_static:go_default_test

Copy link
Contributor

Choose a reason for hiding this comment

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

the blob size will not change the proof size, the proof is for the KZG commitment, the block is smaller so it needs a smaller Merkle Tree, I'm pushing a revert of this and approving this PR

@adiasg adiasg force-pushed the filter-changes branch 2 times, most recently from 18f3c60 to 600316b Compare February 7, 2024 18:32
@potuz potuz added the blocked label Feb 8, 2024
@potuz
Copy link
Contributor

potuz commented Feb 8, 2024

Blocking just in case cause I still don't understand a failing testcase

@potuz potuz enabled auto-merge February 9, 2024 13:51
potuz
potuz previously approved these changes Feb 9, 2024
@potuz potuz removed the blocked label Feb 9, 2024
@potuz potuz disabled auto-merge February 9, 2024 14:38
@potuz potuz enabled auto-merge February 9, 2024 14:53
@potuz potuz added this pull request to the merge queue Feb 9, 2024
Merged via the queue into OffchainLabs:develop with commit 6a605e6 Feb 9, 2024
@potuz potuz deleted the filter-changes branch February 9, 2024 17:08
potuz added a commit that referenced this pull request Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants