Skip to content

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

Closed
meyer9 wants to merge 3 commits intoethereum-optimism:meyer9/implement-isthmus-7702-span-batch-testsfrom
meyer9:meyer9/isthmus-7702-pre-isthmus-test
Closed

op-node/rollup/derive: Drop SetCodeTxs in pre-Isthmus batches#14288
meyer9 wants to merge 3 commits intoethereum-optimism:meyer9/implement-isthmus-7702-span-batch-testsfrom
meyer9:meyer9/isthmus-7702-pre-isthmus-test

Conversation

@meyer9
Copy link
Contributor

@meyer9 meyer9 commented Feb 10, 2025

Description

Adds a test to ensure SetCode transactions before Isthmus result in the channel being dropped.

This is tested by configuring a network to hardfork to Isthmus prior to the derivation pipeline of the verifier. When the verifier tries to check the singular batch containing a SetCode tx, it drops the batch and prints an error which is checked by the test.

@meyer9 meyer9 requested review from a team as code owners February 10, 2025 18:39
@meyer9 meyer9 requested review from ajsutton, axelKingsley and pauldowman and removed request for a team February 10, 2025 18:39
@meyer9 meyer9 marked this pull request as draft February 10, 2025 18:39
@meyer9 meyer9 mentioned this pull request Feb 10, 2025
Merged
@meyer9 meyer9 force-pushed the meyer9/implement-isthmus-7702-span-batch-tests branch from 8bd139e to 64a4eda Compare February 10, 2025 18:44
@meyer9 meyer9 force-pushed the meyer9/isthmus-7702-pre-isthmus-test branch from 75a39a2 to ef4c5d4 Compare February 10, 2025 18:47
@meyer9 meyer9 requested a review from sebastianst February 10, 2025 18:47
@meyer9
Copy link
Contributor Author

meyer9 commented Feb 20, 2025 •
edited
Loading

Blocked by ethereum-optimism/op-geth#507

@meyer9 meyer9 marked this pull request as ready for review February 27, 2025 20:42
@tynes
Copy link
Contributor

tynes commented Feb 28, 2025

Blocked by ethereum-optimism/op-geth#507

Looks like this is merged now

@meyer9
Copy link
Contributor Author

meyer9 commented Feb 28, 2025

Unfortunately it's in geth v1.15.1 (.1 later than is currently merged), so need this PR actually: ethereum-optimism/op-geth#526

@meyer9 meyer9 force-pushed the meyer9/implement-isthmus-7702-span-batch-tests branch from 64a4eda to 8e24670 Compare February 28, 2025 18:46
refcell
refcell approved these changes Feb 28, 2025
View reviewed changes
Copy link
Contributor

@refcell refcell left a comment

Choose a reason for hiding this comment

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

These look good to me!

@meyer9 meyer9 force-pushed the meyer9/implement-isthmus-7702-span-batch-tests branch from 8e24670 to ce5b0cd Compare March 6, 2025 19:30
@meyer9 meyer9 force-pushed the meyer9/isthmus-7702-pre-isthmus-test branch from e34e3ee to 0e0ed1a Compare March 6, 2025 19:33
@tynes
Copy link
Contributor

tynes commented Mar 12, 2025

/ci authorize 7dea6f8

@tynes
Copy link
Contributor

tynes commented Mar 12, 2025

We need to merge this still, cc @sebastianst

@tynes
Copy link
Contributor

tynes commented Mar 12, 2025

Looks like a build issue cc @meyer9

op-e2e/actions/upgrades/dencun_fork_test.go:1: : # github.com/ethereum-optimism/optimism/op-e2e/actions/upgrades [github.com/ethereum-optimism/optimism/op-e2e/actions/upgrades.test]
op-e2e/actions/upgrades/isthmus_fork_test.go:697:21: multiple-value seqEngine.EngineApi.IncludeTx(tx, dp.Addresses.Alice) (value of type (*"github.com/ethereum/go-ethereum/core/types".Receipt, error)) in single-value context (typecheck)
package upgrades
make: *** [Makefile:24: lint-go] Error 1

@sebastianst sebastianst added the H-isthmus Hardfork: change is planned for isthmus upgrade label Mar 12, 2025
@meyer9 meyer9 force-pushed the meyer9/implement-isthmus-7702-span-batch-tests branch from ce5b0cd to de2a0d0 Compare March 12, 2025 19:20
@meyer9 meyer9 force-pushed the meyer9/isthmus-7702-pre-isthmus-test branch from 7dea6f8 to 25f48fe Compare March 12, 2025 19:27
@meyer9 meyer9 requested a review from a team as a code owner March 12, 2025 19:27
@meyer9 meyer9 force-pushed the meyer9/isthmus-7702-pre-isthmus-test branch from 25f48fe to 05ed399 Compare March 12, 2025 19:30
@meyer9
Copy link
Contributor Author

meyer9 commented Mar 12, 2025

CI passing 🎉

@sebastianst sebastianst self-assigned this Mar 12, 2025
sebastianst
sebastianst requested changes Mar 12, 2025
View reviewed changes
@sebastianst sebastianst changed the title feat: ensure 7702 txs are dropped from batches pre-Isthmus op-node/rollup/derive: Drop SetCodeTxs in pre-Isthmus batches Mar 13, 2025
@sebastianst sebastianst added A-op-e2e Area: op-e2e A-derivation Area: derivation changes (op-node/rollup/derive) labels Mar 13, 2025
@meyer9 meyer9 force-pushed the meyer9/implement-isthmus-7702-span-batch-tests branch from cb57812 to 2a3e364 Compare March 13, 2025 20:13
tynes
tynes reviewed Mar 13, 2025
View reviewed changes
op-node/rollup/derive/span_batch_tx.go
GasTipCap: setCodeTxInner.GasTipCap,
GasFeeCap: setCodeTxInner.GasFeeCap,
Gas: gas,
To: *to,
Copy link
Contributor

@tynes tynes Mar 13, 2025

Choose a reason for hiding this comment

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

We are sure this can never be nil?

Copy link
Member

@sebastianst sebastianst Mar 14, 2025

Choose a reason for hiding this comment

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

This is related to the other PR this was stacked on #14197

Copy link
Member

@sebastianst sebastianst Mar 14, 2025

Choose a reason for hiding this comment

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

This is a very good observation @tynes I believe the current format could be hacked to make this nil. The contract creation bit could be set to 1 for SetCode transactions. We need to decide how to handle this case. We probably can just return an error.

@tynes
Copy link
Contributor

tynes commented Mar 13, 2025

This generally looks good to me besides the comments that @sebastianst left

@meyer9 meyer9 force-pushed the meyer9/isthmus-7702-pre-isthmus-test branch from 05ed399 to 6aa3d79 Compare March 14, 2025 00:10
@tynes
Copy link
Contributor

tynes commented Mar 14, 2025

/ci authorize 0a8831a

@tynes
Copy link
Contributor

tynes commented Mar 14, 2025

Looks like there are some failing tests here @meyer9

@sebastianst sebastianst mentioned this pull request Mar 14, 2025
Merged
@sebastianst
Copy link
Member

sebastianst commented Mar 14, 2025

Closing in favor of #14877 - @meyer9 has been given credit as co-author in commits.

@sebastianst sebastianst mentioned this pull request Mar 14, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sebastianst sebastianst sebastianst requested changes

@tynes tynes tynes left review comments

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

@axelKingsley axelKingsley Awaiting requested review from axelKingsley axelKingsley is a code owner automatically assigned from ethereum-optimism/go-reviewers

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

+1 more reviewer

@refcell refcell refcell approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sebastianst sebastianst

Labels

A-derivation Area: derivation changes (op-node/rollup/derive) A-op-e2e Area: op-e2e 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.

4 participants

@meyer9 @tynes @sebastianst @refcell