Skip to content

op-program/client: add check for nil withdrawalsroot #14836

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
Merged
merged 1 commit into from
Mar 14, 2025
Merged

op-program/client: add check for nil withdrawalsroot #14836

merged 1 commit into from
Mar 14, 2025

Conversation

geoknee
Copy link
Contributor

@geoknee geoknee commented Mar 13, 2025 •
edited
Loading

@codecov Codecov
Copy link

codecov bot commented Mar 13, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.67%. Comparing base (d509f44) to head (caf58cd).
Report is 1 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #14836      +/-   ##
===========================================
- Coverage    45.94%   41.67%   -4.28%     
===========================================
  Files         1085      913     -172     
  Lines        94622    84475   -10147     
===========================================
- Hits         43478    35206    -8272     
+ Misses       47962    46258    -1704     
+ Partials      3182     3011     -171
Flag Coverage Δ
cannon-go-tests-32 ?
cannon-go-tests-64 ?
contracts-bedrock-tests ?

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

Files with missing lines Coverage Δ
op-program/client/l2/engineapi/l2_engine_api.go 32.76% <100.00%> (+0.49%) ⬆️

... and 181 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@geoknee geoknee marked this pull request as ready for review March 13, 2025 10:21
@geoknee geoknee requested review from a team as code owners March 13, 2025 10:21
@geoknee geoknee requested a review from ajsutton March 13, 2025 10:21
sebastianst
sebastianst reviewed Mar 13, 2025
View reviewed changes
@yoonchung8822 yoonchung8822 added the H-isthmus Hardfork: change is planned for isthmus upgrade label Mar 13, 2025
@geoknee geoknee requested a review from a team as a code owner March 13, 2025 18:28
@geoknee geoknee requested a review from protolambda March 13, 2025 18:28
@geoknee geoknee requested a review from a team as a code owner March 13, 2025 23:04
@geoknee geoknee marked this pull request as draft March 13, 2025 23:04
@geoknee
Copy link
Contributor Author

geoknee commented Mar 14, 2025

I did an experiment in kurtosis where I modified op-node to panic if it receives an execution payload over engine.getPayload. When I run this with op-reth, the panic is triggered. So my current hypothesis is that op-reth is not setting the field correctly on getPayload, but is then complaining when it gets it back over newPayload.

When I repeat the experiment and drop in op-geth instead of op-geth, no panic.

I'm going to use this PR to add more checks into op-program and op-node, but otherwise I don't think there is a bug in the Go codebase around this.

@geoknee geoknee force-pushed the gk/withdrawals-root-bug branch from 4aecce8 to caf58cd Compare March 14, 2025 13:03
@geoknee geoknee marked this pull request as ready for review March 14, 2025 13:05
@sebastianst sebastianst added this pull request to the merge queue Mar 14, 2025
Merged via the queue into develop with commit 9d86edb Mar 14, 2025
49 checks passed
@sebastianst sebastianst deleted the gk/withdrawals-root-bug branch March 14, 2025 14:14
Rjected pushed a commit to paradigmxyz/optimism that referenced this pull request Mar 19, 2025
@geoknee @Rjected
op-program/client: add check for nil withdrawalsroot (ethereum-optimi…
9aeb53c 
…sm#14836)
Rjected added a commit to paradigmxyz/optimism that referenced this pull request Mar 19, 2025
@Rjected @clabby
@sigma @scharissis @janjakubnanista @sebastianst @protolambda @ControlCplusControlV @smartcontracts @ajsutton @Inphi @pcw109550 @meyer9 @geoknee @mslipper @tcrypt25519 @agusduha @0xDiscotech @0xng @skeletor-spaceman @axelKingsley @bitwiseguy @teddyknox @maurelian @JosepBove @leruaa @yashvardhan-kukreja
Merge upstream 4 (#10)
936a178 
* fix(ctb): L2 genesis - History storage address (ethereum-optimism#14844)

* feat(devnet-sdk): Expose supervisor (ethereum-optimism#14772)

* fix(kurtosis-devnet): expose supervisor svc info

* feat(devnet-sdk): provide access to supervisor

* feat(op-acceptor): native binary execution (ethereum-optimism#14777)

* feat(op-acceptor): native binary execution

Adds native binary execution and makes this the default. It falls back to docker-based execution.
The reason for this change is that it runs much faster (10x) this way.

* feat(op-acceptor): mise-managed native binary

* chore: Use floats for fee multipliers, add base multiplier (ethereum-optimism#14818)

* chore: Use floats for fee multipliers

* fix: Comment

* fix: Typo

* chore: Use functional options pattern

* chore: Add unit tests for bigint multiplication

* fix(ci): github ratelimits. (ethereum-optimism#14856)

* op-node/rollup/sequencing: No user txs in Isthmus & Interop upgrade block (ethereum-optimism#14837)

* op-node/rollup/sequencing: No user txs in Isthmus & Interop upgrade blocks

* op-e2e: Fix Isthmus tests, remove case

* op-e2e: reintroduce AtIsthmusWithWithdrawalTx

* op-e2e/action: fix holocene invalid batches test

---------

Co-authored-by: protolambda <proto@protolambda.com>

* fix(ci): github ratelimits. (ethereum-optimism#14857)

* chore: Renamed OutputRoot -> Proposal, l2BlockNumber -> l2SequenceNumber (ethereum-optimism#14775)

* Renamed OutputRoot -> Proposal, l2BlockNumber -> l2SequenceNumber

* added interfaces

* Fixed spec tests

* ver bump

* fix tests

* fix snapshots

* fix snapshots

* semver

* semver comment

* fixed interfaces

* unused import

* correct branch

* fix tests

* final versioning fix

* again oops:

* revert semver

* fix: correct semver in upgrade test

* fix: correct semver lock

---------

Co-authored-by: Kelvin Fichter <kelvinfichter@gmail.com>

* fix(ci): github-token for ratelimiting. (ethereum-optimism#14860)

* feat(op-acceptor): enable ci go tests for op-acceptor. (ethereum-optimism#14861)

* op-challenger: Update contract bindings to handle renames. (ethereum-optimism#14779)

* op-challenger: Update contract bindings to handle renames.

Also renames a copule of fields to be more generic, but not a complete pass.

* op-challenger: Rename LocalGameInputs L2BlockNumber to L2SequenceNumber

* op-e2e: Interop deposit message action tests (ethereum-optimism#14864)

* op-program: Add aliases to flags for interop. (ethereum-optimism#14862)

* op-challenger: Update contract bindings to handle renames.

Also renames a copule of fields to be more generic, but not a complete pass.

* op-challenger: Rename LocalGameInputs L2BlockNumber to L2SequenceNumber

* op-program: Add aliases to flags for interop.

l2.blocknumber flag now has a l2.sequencenumber alias. Both options work allowing us to eventually migrate to the more generic name without breaking compatibility.

Updated help text for a couple of flags to make them clearer.

* Update help text.

Co-authored-by: Inphi <mlaw2501@gmail.com>

* Update help text.

Co-authored-by: Inphi <mlaw2501@gmail.com>

---------

Co-authored-by: Inphi <mlaw2501@gmail.com>

* ci: Remove fpp-verify job (ethereum-optimism#14866)

* Use SuperFaultDisputeGame ABI in more tests (ethereum-optimism#14867)

* Use SuperFaultDisputeGame ABI in more tests

* op-proposer: Test compatibility with SuperFaultDisputeGame abi

* feat(devnet-sdk): add op-program-svc client library (ethereum-optimism#14677)

This is a ligntweight client that op-deployer can make use of in order
to obtain correct prestates based on chain configs.

* fix(kt-devnet): use unique container tags (ethereum-optimism#14411)

Unfortunately kurtosis puts too much faith in the stability of docker
tags, and gets fooled into thinking a deployment step can be skipped
if it involves an image with the same tag.

This change makes the kurtosis config use a unique tag based on
container ID instead.

* fix(devnet-sdk): detect if op-geth types are used (ethereum-optimism#14533)

* kt-devnet: minimal interop test: initiate message (ethereum-optimism#14782)

* devnet-sdk: Plugin op-service ethclient

* Add low level client access

* Use e2e wait util

* kt-devnet: minimal interop test

* Rename wallet's sendMessage to initiateMessage

* cleanup

* Only Expose ABI for now

* simplify

* add value while tx building

* Refactor to use call() method

* op-node/rollup/derive: implement SetCodeTx span batches (ethereum-optimism#14197)

* feat: implement EIP-7702 span batch tests

* Don't overwrite alloc

* Check error

* Fix lint

* Add SetCode txs to tests and switch signer

* Use old signer

* Implement as FP action test

* Remove old test

* Clean up code

* Cover set code auth in unit tests

* Remove extra file

* Update channel out sizes since random tx changed

* Use test matrix

* Simplify RunFPP call

* add back bob address sanity check

---------

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* op-program/client: add check for nil withdrawalsroot (ethereum-optimism#14836)

* chore: freeze files for Upgrade 15 (ethereum-optimism#14865)

* fix l2 service representation (ethereum-optimism#14868)

* fix(kurtosis-devnet): common services detection (ethereum-optimism#14870)

This reverts commit c5f8808.

The issue was that the other L2s' services were mistakenly taken as common services.
In particular, that led to override of el/cl nodes, in addition to
spurious entries in the services map.

* kt-devnet: minimal interop test: execute message (ethereum-optimism#14838)

* devnet-sdk: Plugin op-service ethclient

* Add low level client access

* Use e2e wait util

* execute message

* typo

* newline

* use sha256 precompile

* comments

* Polish

* comment

* Log block time

* Reduce logs

* fix block number

* feat: dispute mon helper (ethereum-optimism#14743)

* feat(op-program-svc): add info.json to identify prestates (ethereum-optimism#14676)

* op-node,op-service: cleanup block signing abstractions (ethereum-optimism#14874)

* chore(op-e2e): Prefix proof action tests with `Test_ProgramAction` (ethereum-optimism#14881)

* op-deployer: Complete backport of v3.0.0-rc.2 (ethereum-optimism#14852) (ethereum-optimism#14875)

* op-deployer: Complete backport of v3.0.0-rc.2

* Update SR version

* op-deployer: De-schedule Isthmus from v3.0.0 (ethereum-optimism#14878)

* op-deployer: De-schedule Isthmus from v3.0.0

* activate by default in dev

* fix(kurtosis-devnet): fallback to docker user socket (ethereum-optimism#14884)

* feat(op-program-svc): simplify the API (ethereum-optimism#14678)

also ensure we have an info.json file that presents the various
prestates available.

* interop,tweak: Return ErrConflict for completed blocks with fewer logs than expected. (ethereum-optimism#14758)

* interop,tweak: Treat 'future' logs for complete blocks as conflicts.

* refactor: Move log-index out-of-range check to findLogInfo.

* fix(ci): github-token for ratelimiting. (ethereum-optimism#14890)

* chore(op-acceptor): bump to v0.1.2 (ethereum-optimism#14854)

* test(kurtosis-devnet): add to CI (ethereum-optimism#14871)

* op-node/rollup/derive: validate to non-nil for setcode txs (ethereum-optimism#14882)

* op-deployer: Default to MIPS v2 (ethereum-optimism#14888)

* feat(op-acceptor): auto-download target binary version (ethereum-optimism#14855)

* feat: interop access list (ethereum-optimism#14883)

* feat: access list (ethereum-optimism#326)

* feat: access list poc

* chore: remove is deposit tests

* fix: natspec and enhance comments

* chore: remove l1 block interop on solidity files

* fix: pr fixes

* chore: remove go logic over l1 block interop

* chore: remove some l1 block interop stuff on solidity side

* chore: remove from frozen files

* chore: remove missing l1 block interop stuff on go tests

* refactor: improve natspec and comments

* feat: add is warm tests

* feat: wip trying to integrate with common test

* fix: integrate with common test reading artifact json

* fix: pre-pr and tests

* fix: tests

* fix: tests

Co-authored-by: 0xng <ng@defi.sucks>
Co-authored-by: skeletor-spaceman <skeletor@defi.sucks>

* fix: pr review comments

* fix: op e2e inbox interface

---------

Co-authored-by: agusduha <agusnduha@gmail.com>
Co-authored-by: 0xng <ng@defi.sucks>
Co-authored-by: skeletor-spaceman <skeletor@defi.sucks>

* fix: undo identifier struct type changes (ethereum-optimism#328)

* op-e2e: skip interop tests for now until Go access-list changes land

* op-e2e: few more test-skips needed until access-list changes land

* op-e2e: missed one test-skip for access-list change

---------

Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com>
Co-authored-by: 0xng <ng@defi.sucks>
Co-authored-by: skeletor-spaceman <skeletor@defi.sucks>
Co-authored-by: protolambda <proto@protolambda.com>

* feat(op-acceptance-tests): bump op-acceptor to v0.1.3 (ethereum-optimism#14893)

* op-supervisor: access-list support, checksum handling (ethereum-optimism#14784)

* op-supervisor: access-list support, checksum handling

* op-supervisor: access-list update work

* op-supervisor: access-list types unit-tests

* op-supervisor: fix nit, update todo comments

* op-supervisor: fix tests, skip TestInteropBlockBuilding for now

* op-supervisor: ExecutingDescriptor with 0 instead of null timeout default

* op-deployer: Add v3.0.0 upgrader (ethereum-optimism#14887)

* op-node/rollup/derive: Drop SetCodeTxs in pre-Isthmus batches (ethereum-optimism#14877)

* op-node/rollup/derive: Drop SetCodeTxs in pre-Isthmus batches

Co-authored-by: Julian Meyer <julian.meyer@coinbase.com>

* rename tests with prefix Test_ProgramAction

* fix tests, use random txs depending on signer type

---------

Co-authored-by: Julian Meyer <julian.meyer@coinbase.com>

* op-e2e: Add Access Lists to interop txs (ethereum-optimism#14831)

* op-e2e: fix interop tests with access-lists, and rm test end-deposit-ctx assumptions

Co-authored-by: axelKingsley <axel.kingsley@gmail.com>

* go.mod: update op-geth to v1.101503.2-rc.1 with interop access-list handling

---------

Co-authored-by: protolambda <proto@protolambda.com>

* fix(ci): github-token for ratelimiting. (ethereum-optimism#14894)

* chore(kurtosis-devnet): bump kurtosis to 1.5.0 (ethereum-optimism#14788)

Co-authored-by: protolambda <proto@protolambda.com>

* op-deployer: add backport example to devdoc (ethereum-optimism#14845)

* op-deployer: add backport example to devdoc

* clear up wording in release docs

* feat(devnet-sdk): Make chain interface abstract and introduce "concrete" L1/L2 chain interfaces (ethereum-optimism#14834)

* fix(kurtosis-devnet): handle system/user sockets better (ethereum-optimism#14911)

- use url.Parse as the docker client library does unsanitary things with
the url Host field.

- fallback to the right socket on Docker Desktop for Linux (which *of
course* uses a different socket path)

- if we don't find a satisfactory user socket, do nothing and hope the
user environment is set properly to compensate.

* ctb: Document guidelines for writing upgradeable contracts (ethereum-optimism#14872)

* ctb: Document guidelines for writing upgradeable contracts

* Apply suggestions from code review

Co-authored-by: JosepBove <josep@oplabs.co>

---------

Co-authored-by: JosepBove <josep@oplabs.co>

* op-e2e/system: tune `TestBatcherAutoDA` to avoid flakes (ethereum-optimism#14906)

* op-e2e/system: tune TestBatcherAutoDA to avoid flakes

* delay batch submitting until after deposit txs are launched

* op-e2e/actions: test batcher should not warn when safe head moves (ethereum-optimism#14912)

This implementation of a batcher does not have block submission tracking fully implemented. Therefore it is expected that the "last submitted block" won't move before the safe head on the sequencer moves.

This change just marks that as expected behaviour to avoid sending devs on a wild goose chase when debugging.

* kt-devnet: message passing using access list (ethereum-optimism#14913)

* chore: add operator fee e2e tests (ethereum-optimism#14752)

* chore: add operator fee e2e tests

* chore: use ActBuildToL1HeadUnsafe()

* fix: handle feedbacks

* nit: add comment

* fix: handle feedbacks

* Fix: following pairing with George

* fix: nits

* fix & improve test

---------

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* feat(op-acceptance-test): op-acceptor v0.1.4 (ethereum-optimism#14916)

* feat(op-acceptance-tests): add some starter gates. (ethereum-optimism#14900)

* fix(op-deployer): fix inspect and state types to respect unmarshalling the state.json (ethereum-optimism#14889)

* fix(op-deployer): fix inspect and state types to respect unmarshalling the state.json

Signed-off-by: Yashvardhan Kukreja <yashvardhan@oplabs.co>

* fix: add custom marshaller to eth.BlockRef

Signed-off-by: Yashvardhan Kukreja <yashvardhan@oplabs.co>

* nit

Signed-off-by: Yashvardhan Kukreja <yashvardhan@oplabs.co>

* chore: remove the custom serializer from blockref

Signed-off-by: Yashvardhan Kukreja <yashvardhan@oplabs.co>

* chore: add unit tests for BlockRef's Serialize and Deserialise

Signed-off-by: Yashvardhan Kukreja <yashvardhan@oplabs.co>

* clean up test

* put marshaler inside start block

* Revert "put marshaler inside start block"

This reverts commit 46b5eaf.

* Revert "clean up test"

This reverts commit e196859.

* Revert "chore: add unit tests for BlockRef's Serialize and Deserialise"

This reverts commit d85ce6a.

* Revert "chore: remove the custom serializer from blockref"

This reverts commit 3dd0834.

* Revert "fix: add custom marshaller to eth.BlockRef"

This reverts commit fb4ff4a.

* unit tests

---------

Signed-off-by: Yashvardhan Kukreja <yashvardhan@oplabs.co>
Co-authored-by: Matthew Slipper <me@matthewslipper.com>

* op-e2e: Test that setting op fee pre-Isthmus is ignored (ethereum-optimism#14910)

* op-e2e: Test that setting op fee pre-Isthmus is ignored

* address feedback @mbaxter

* op-e2e/actions: introduce env.BatchMineAndSync (ethereum-optimism#14923)

* Run `TestPectraBlobSchedule` on latest fork (ethereum-optimism#14783)

* Run TestPectraBlobSchedule on latest fork

* Apply suggestions from code review

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

---------

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* op-node: Add pectra blob schedule fix kill-switch (ethereum-optimism#14922)

* op-node: Add pectra blob schedule fix kill-switch

* move ignore flag into node config

---------

Signed-off-by: Yashvardhan Kukreja <yashvardhan@oplabs.co>
Co-authored-by: clabby <ben@clab.by>
Co-authored-by: Yann Hodique <yann@oplabs.co>
Co-authored-by: Stefano Charissis <stefano@oplabs.co>
Co-authored-by: Ján Jakub Naništa <jan.jakub.nanista@gmail.com>
Co-authored-by: Sebastian Stammler <seb@oplabs.co>
Co-authored-by: protolambda <proto@protolambda.com>
Co-authored-by: ControlCplusControlV <44706811+ControlCplusControlV@users.noreply.github.com>
Co-authored-by: Kelvin Fichter <kelvinfichter@gmail.com>
Co-authored-by: Adrian Sutton <adrian@oplabs.co>
Co-authored-by: Inphi <mlaw2501@gmail.com>
Co-authored-by: Park Changwan <changwan@oplabs.co>
Co-authored-by: Julian Meyer <julian.meyer@coinbase.com>
Co-authored-by: George Knee <georgeknee@googlemail.com>
Co-authored-by: smartcontracts <kelvin@optimism.io>
Co-authored-by: Matthew Slipper <me@matthewslipper.com>
Co-authored-by: Tyler Smith <mail@tcry.pt>
Co-authored-by: AgusDuha <81362284+agusduha@users.noreply.github.com>
Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com>
Co-authored-by: 0xng <ng@defi.sucks>
Co-authored-by: skeletor-spaceman <skeletor@defi.sucks>
Co-authored-by: Axel Kingsley <axel.kingsley@gmail.com>
Co-authored-by: Sam Stokes <35908605+bitwiseguy@users.noreply.github.com>
Co-authored-by: Teddy Knox <teddy@oplabs.co>
Co-authored-by: Maurelian <john@oplabs.co>
Co-authored-by: JosepBove <josep@oplabs.co>
Co-authored-by: Aurélien <3535019+leruaa@users.noreply.github.com>
Co-authored-by: Yashvardhan Kukreja <yashvardhan@oplabs.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Inphi Inphi Inphi left review comments

@sebastianst sebastianst sebastianst approved these changes

@ajsutton ajsutton Awaiting requested review from ajsutton ajsutton is a code owner automatically assigned from ethereum-optimism/proofs

@protolambda protolambda Awaiting requested review from protolambda protolambda is a code owner automatically assigned from ethereum-optimism/op-stack

Assignees

@geoknee geoknee

Labels

H-isthmus Hardfork: change is planned for isthmus upgrade

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Check withdrawals root not nil newPayloadV4

4 participants

@geoknee @sebastianst @Inphi @yoonchung8822