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

Holocene: initial op-node support for configurable eip-1559 params #12497

Merged
merged 3 commits into from
Oct 21, 2024

Conversation

roberto-bayardo
Copy link
Collaborator

Description

Implements configurable EIP-1559 parameters in op-node per the Holocene spec: https://github.com/ethereum-optimism/specs/tree/main/specs/protocol/holocene

Full SystemConfig contract updates coming next.

Tests

Unit tests added. E2e test in a followup PR.

@roberto-bayardo roberto-bayardo force-pushed the holocene-1559 branch 2 times, most recently from 402c377 to 6a7eec6 Compare October 16, 2024 22:36
@roberto-bayardo roberto-bayardo force-pushed the holocene-1559 branch 2 times, most recently from 9ba35e3 to 75dd66a Compare October 17, 2024 00:44
@roberto-bayardo
Copy link
Collaborator Author

I've got all the tests passing locally, not sure why those few not passing on the CI. Marking as ready for review, though we'll need to update go.mod to point to the actual op-geth repo once that corresponding PR is merged.

@roberto-bayardo roberto-bayardo marked this pull request as ready for review October 17, 2024 01:02
@roberto-bayardo roberto-bayardo force-pushed the holocene-1559 branch 2 times, most recently from 5988ce3 to cc6586d Compare October 17, 2024 15:05
@roberto-bayardo roberto-bayardo force-pushed the holocene-1559 branch 2 times, most recently from af5c222 to 2546ac5 Compare October 17, 2024 15:18
Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

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

Go changes LGTM, but not sure about contract changes.

@tynes
Copy link
Contributor

tynes commented Oct 18, 2024

Looks like some issues with kurtosis in CI

Caused by: Error response from daemon: Conflict. The container name "/kurtosis-fluentbit-configurator" is already in use by container "b25643cf545354883bfe8017c2d776d53c39765a2d6e0e4025a55155e872ac33". You have to remove (or rename) that container to be able to reuse that name.

@tynes
Copy link
Contributor

tynes commented Oct 18, 2024

CRIT [10-18|14:51:21.606] error checking deploy config             err="cannot unmarshal deploy config: json: unknown field \"l2GenesisBlockExtraData\"

The l2GenesisBlockExtraData needs to be removed from any JSON files now that it is removed from the DeployConfig, a git grep -rin l2GenesisBlockExtraData will find them

@roberto-bayardo roberto-bayardo force-pushed the holocene-1559 branch 2 times, most recently from 4af2171 to 6cd237f Compare October 18, 2024 15:42
@roberto-bayardo
Copy link
Collaborator Author

CRIT [10-18|14:51:21.606] error checking deploy config             err="cannot unmarshal deploy config: json: unknown field \"l2GenesisBlockExtraData\"

The l2GenesisBlockExtraData needs to be removed from any JSON files now that it is removed from the DeployConfig, a git grep -rin l2GenesisBlockExtraData will find them

done

@tynes
Copy link
Contributor

tynes commented Oct 19, 2024

Looks like something is wrong with the op-deployer-integration

Caused by: Error response from daemon: Conflict. The container name "/kurtosis-fluentbit-configurator" is already in use by container "b25643cf545354883bfe8017c2d776d53c39765a2d6e0e4025a55155e872ac33". You have to remove (or rename) that container to be able to reuse that name.

Perhaps there is a way to not spin up that image, a previous PR added a timestamp to the image names to prevent this problem, or if its required maybe we can also use a timestamp with its name?

cc @mslipper

@tynes
Copy link
Contributor

tynes commented Oct 19, 2024

Looks like a flake in the e2e tests

=== FAIL: op-e2e/system/altda TestBatcherConcurrentAltDARequests (18.09s)

   concurrent_test.go:76: Expected at least 10 transactions from the batcher
    setup.go:424: CLOSING
    service.go:289:             INFO [10-18|17:10:21.002] Stopping Proposer                        role=proposer
    driver.go:197:              INFO [10-18|17:10:21.002] Stopping Proposer                        role=proposer
    driver.go:473:              INFO [10-18|17:10:21.002] loop returning                           role=proposer
    driver.go:211:              INFO [10-18|17:10:21.002] Proposer stopped                         role=proposer
    service.go:335:             INFO [10-18|17:10:21.002] L2Output Submitter stopped               role=proposer
    service.go:399:             INFO [10-18|17:10:21.002] Stopping batcher                         role=batcher
    driver.go:169:              INFO [10-18|17:10:21.002] Stopping Batch Submitter                 role=batcher
    driver.go:617:              ERROR[10-18|17:10:21.003] Failed to post input to Alt DA           role=batcher error="Post \"http://127.0.0.1:35229/put\": context canceled"

Going to rerun and see if it fixes it

@sebastianst
Copy link
Member

Looks like something is wrong with the op-deployer-integration

Caused by: Error response from daemon: Conflict. The container name "/kurtosis-fluentbit-configurator" is already in use by container "b25643cf545354883bfe8017c2d776d53c39765a2d6e0e4025a55155e872ac33". You have to remove (or rename) that container to be able to reuse that name.

Perhaps there is a way to not spin up that image, a previous PR added a timestamp to the image names to prevent this problem, or if its required maybe we can also use a timestamp with its name?

cc @mslipper

Tracking here #12535

@tynes
Copy link
Contributor

tynes commented Oct 21, 2024

Looks like something is wrong with the op-deployer-integration

Caused by: Error response from daemon: Conflict. The container name "/kurtosis-fluentbit-configurator" is already in use by container "b25643cf545354883bfe8017c2d776d53c39765a2d6e0e4025a55155e872ac33". You have to remove (or rename) that container to be able to reuse that name.

Perhaps there is a way to not spin up that image, a previous PR added a timestamp to the image names to prevent this problem, or if its required maybe we can also use a timestamp with its name?
cc @mslipper

Tracking here #12535

This has been fixed on develop, so a rebase will fix the issue

@mslipper
Copy link
Collaborator

I merged in latest develop - this should be green now. Good to merge as soon as Adrian's latest comment is addressed.

Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.86%. Comparing base (76c1e1d) to head (4ff4d65).
Report is 8 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #12497      +/-   ##
===========================================
- Coverage    65.04%   64.86%   -0.18%     
===========================================
  Files           54       54              
  Lines         4460     4460              
===========================================
- Hits          2901     2893       -8     
- Misses        1382     1391       +9     
+ Partials       177      176       -1     
Flag Coverage Δ
cannon-go-tests 64.86% <ø> (-0.18%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

@sebastianst sebastianst added this pull request to the merge queue Oct 21, 2024
Merged via the queue into develop with commit e7fbaec Oct 21, 2024
51 checks passed
@sebastianst sebastianst deleted the holocene-1559 branch October 21, 2024 20:28
samlaf pushed a commit to samlaf/optimism that referenced this pull request Nov 10, 2024
…thereum-optimism#12497)

* extend op-node to support holocene 1559 params

* - add Holocene parameter validation to op-program engine

- updates for latest op-geth tag

---------

Co-authored-by: Matthew Slipper <[email protected]>
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.

7 participants