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.

Sign up for GitHub

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

Jump to bottom

Dencun header changes #7993

Merged
merged 23 commits into from
Nov 8, 2023
Merged

Dencun header changes #7993

merged 23 commits into from
Nov 8, 2023

Conversation

EvanJRichard
Copy link
Contributor

@EvanJRichard EvanJRichard commented Nov 1, 2023 •
edited
Loading

Description

This PR seeks to cherry pick a subset of #7349. Specifically, we just want the rpc-header change in op-service/sources, the basic types in op-service/eth, and we also seek to add some action test like TestShapellaL1Fork in #5144.

Tests

Adds a new test TestDencunL1Fork in op-e2e/actions/dencun_fork_test.go, which just makes sure that the L2 can catch up to an L1 that supports Cancun.

Additional context

Thank you Roberto and Proto.

Metadata

Co-authored-by: protolambda [email protected]
Co-authored-by: Roberto Bayardo [email protected]

semgrep-app[bot]
semgrep-app bot reviewed Nov 1, 2023
View reviewed changes
op-service/eth/blob.go Outdated Show resolved Hide resolved
semgrep-app[bot]
semgrep-app bot reviewed Nov 1, 2023
View reviewed changes
op-service/eth/blob.go Outdated Show resolved Hide resolved
@trianglesphere
Copy link
Contributor

This needs a merge in / rebase from develop to fix the bindings build. Not sure about the other tests yet though

@EvanJRichard
Copy link
Contributor Author

EvanJRichard commented Nov 6, 2023 •
edited
Loading

@trianglesphere git is telling me the PR is up to date with develop, but the bindings build is still broken - am I maybe mistaken that it's up to date, or something? EDIT Yes I was mistaken

@EvanJRichard EvanJRichard marked this pull request as ready for review November 6, 2023 20:11
@EvanJRichard EvanJRichard requested a review from a team as a code owner November 6, 2023 20:11
EvanJRichard and others added 19 commits November 6, 2023 15:14
@protolambda @roberto-bayardo
Pull in basic types from op-service/eth package.
556e500 
Co-authored-by: protolambda <[email protected]>
Co-authored-by: Roberto Bayardo <[email protected]>
@roberto-bayardo @protolambda
Pull in basic types from op-service/sources.
ba2ec85 
Co-authored-by: Roberto Bayardo <[email protected]>
Co-authored-by: protolambda <[email protected]>
@protolambda @roberto-bayardo
Pull in blob and blob_test from PR.
6b4cb22 
Co-authored-by: protolambda <[email protected]>
Co-authored-by: Roberto Bayardo <[email protected]>
@protolambda @roberto-bayardo
Revert "Pull in basic types from op-service/eth package."
78716c9 
This reverts commit 7eccd9f.

Co-authored-by: protolambda <[email protected]>
Co-authored-by: Roberto Bayardo <[email protected]>
Add placeholder for dencun fork test.
c174cb5
Add filler scaffolding for dencun fork test: literally the old shapel…
149a6bc 
…la test.
Start to tweak things in dencun_fork_test: mainly replacing "shanghai…
4806862 
…" with "Cancun"
More minor test tweaks: change require statement's message, get head'…
adcdf91 
…s time differently, add a couple breadcrumb comments.
Fix usage of IsCancun.
118ce7f
Tweak test message: really we're just testing isCancun, so acknowledg…
d0a7922 
…e that.
Use types.Withdrawals in rpcBlock, not eth.Withdrawals.
5c584ca
Flag the failing line, it's non-obvious in CI output.
d57f75f
Add how test is failing just for onlookers.
bc2b4de
Add issue links per semgrep request.
79932ba
WIP stash for discussion.
1131944
Commit missed hunk editing a comment in dencun_fork_test.
8389e8c
Remove useless call to ParentBeaconRoot.
acf1c64
Remove some commented-out cruft.
65916c5
Set necessary header fields if config is cancun.
2c57e7d
@protolambda @roberto-bayardo
Preserve co-authorship.
78eeb2a 
Co-authored-by: protolambda <[email protected]>
Co-authored-by: Roberto Bayardo <[email protected]>
ajsutton
ajsutton requested changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

We seem to have lost a couple of parts of the commit 73c7512 but we'll want to keep them. I'm somewhat surprised that there aren't failures in CI though as that commit had plenty of tests...

Ooo, the test is failing on the bad block hash, not on the withdrawals being invalid. That's a bug in the test. I'll put up a separate PR to fix the tests in a bit.

Still looking through the actual blob verification side too but figured I'd post this now.

op-service/sources/types.go Outdated Show resolved Hide resolved
op-service/sources/types.go Outdated Show resolved Hide resolved
ajsutton
ajsutton reviewed Nov 6, 2023
View reviewed changes
op-service/eth/blob.go Outdated Show resolved Hide resolved
@ajsutton ajsutton mentioned this pull request Nov 6, 2023
Merged
@semgrep-app semgrep-app
Copy link
Contributor

semgrep-app bot commented Nov 7, 2023

Semgrep found 1 nil-check-after-call finding:

Potential p2pmetrics nil dereference when NewBandwidthCounter is called

Ignore this finding from nil-check-after-call.

@EvanJRichard
Copy link
Contributor Author

Ignoring semgrep nil-after-call finding.

ajsutton
ajsutton approved these changes Nov 7, 2023
View reviewed changes
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

LGTM.

op-service/eth/blob.go Outdated Show resolved Hide resolved
@ajsutton ajsutton added this pull request to the merge queue Nov 8, 2023
Merged via the queue into develop with commit 5b67802 Nov 8, 2023
59 checks passed
@ajsutton ajsutton deleted the dencun-header-change branch November 8, 2023 03:22
@EvanJRichard EvanJRichard mentioned this pull request Nov 8, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@semgrep-app semgrep-app[bot] semgrep-app[bot] left review comments

@protolambda protolambda protolambda left review comments

@tynes tynes tynes left review comments

@ajsutton ajsutton ajsutton approved these changes

@trianglesphere trianglesphere Awaiting requested review from trianglesphere

Assignees

@sebastianst sebastianst

@EvanJRichard EvanJRichard

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@EvanJRichard @trianglesphere @ajsutton @tynes @protolambda @sebastianst