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

Pectra on Isthmus FMA #201

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Pectra on Isthmus FMA #201

wants to merge 28 commits into from
+282 −0

Conversation

meyer9
Copy link

@meyer9 meyer9 commented Feb 10, 2025

Description

Adds an FMA for Pectra on Isthmus.

@meyer9 meyer9 marked this pull request as draft February 10, 2025 22:06
@meyer9 meyer9 changed the title WIP: Pectra on Isthmus FMA Pectra on Isthmus FMA Feb 12, 2025
@meyer9 meyer9 marked this pull request as ready for review February 12, 2025 18:20
Ethnical
Ethnical reviewed Mar 7, 2025
View reviewed changes
security/fma-isthmus-l2-pectra.md Outdated
| Created at | 2025-02-10 |
| Initial Reviewers | refcell |
| Need Approval From | |
| Status | In Review 🔎 |
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to add this add the end of the current review please not accept this current suggestion now.
I will accept this at the end myself.

Suggested change
| Status | In Review 🔎 |
| Status | Implementing Action 🛫 |

@lewejahlil
Copy link

In review by EVM Safety: https://github.com/ethereum-optimism/security-pod/issues/241

geoknee
geoknee reviewed Mar 25, 2025
View reviewed changes
security/fma-isthmus-l2-pectra.md Outdated
- [ ] (BLOCKING): Add a E2E test that Validate that minting to EOA that contains code (with Type4) is not an issue (@meyer9 assignee for now but could be someone else).
- [ ] (NON-BLOCKING): Identify path from the sequencer during the deposit transaction on the L2 that can cause unexpected behavior when an EOA has some code.
- [ ] (NON-BLOCKING): Indicate in this FMA reference the kurtosis devnet that allow the perform testing on Isthmus (L2 Pectra).
- [ ] (NON-BLOCKING): Tests to make sure that `ressources_limit` and `gas_limit` for each Superchain are not failling by overflowing from the config on L1 (assignee: @geoknee)
Copy link
Contributor

Choose a reason for hiding this comment

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

I have performed this check manually code here and confirmed that all chains currently in the registry currently have a gasLimit of at least 30M and a maxResourceLimit of at least 20M.

It wasn't worth committing this as a CI check, but I think we can add the link above and consider this item done.

Copy link
Contributor

@geoknee geoknee Mar 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Test output in this fold 
➜  rollup git:(gk/superchain-upgrade-gas-check) ✗ go test -timeout 30s -run ^TestSuperchainGasLimit$ ./... -v                        
=== RUN   TestSuperchainGasLimit
dialing client at https://ethereum-sepolia-rpc.publicnode.commaking batch call
finished batch call
dialing client at https://ethereum-rpc.publicnode.commaking batch call
finished batch call
    superchain_test.go:76: 60000000
    superchain_test.go:78: Chain: cyber-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: cyber-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: mint-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: mint-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: creator-chain-testnet-sepolia GasLimit 30.0M
    superchain_test.go:80: Chain: creator-chain-testnet-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: cyber-sepolia GasLimit 30.0M
    superchain_test.go:80: Chain: cyber-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: mode-sepolia GasLimit 30.0M
    superchain_test.go:80: Chain: mode-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: unichain-sepolia GasLimit 30.0M
    superchain_test.go:80: Chain: unichain-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: automata-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: automata-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: ethernity-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: ethernity-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: ink-mainnet GasLimit 60.0M
    superchain_test.go:80: Chain: ink-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: lisk-mainnet GasLimit 60.0M
    superchain_test.go:80: Chain: lisk-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: ethernity-sepolia GasLimit 30.0M
    superchain_test.go:80: Chain: ethernity-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: funki-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: funki-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: metal-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: metal-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: mode-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: mode-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: polynomial-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: polynomial-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: race-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: race-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: sseed-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: sseed-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: xterio-eth-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: xterio-eth-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: pivotal-sepolia GasLimit 30.0M
    superchain_test.go:80: Chain: pivotal-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: soneium-minato-sepolia GasLimit 30.0M
    superchain_test.go:80: Chain: soneium-minato-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: worldchain-sepolia GasLimit 30.0M
    superchain_test.go:80: Chain: worldchain-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: hashkeychain-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: hashkeychain-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: orderly-mainnet GasLimit 180.0M
    superchain_test.go:80: Chain: orderly-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: snax-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: snax-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: soneium-mainnet GasLimit 40.0M
    superchain_test.go:80: Chain: soneium-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: swan-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: swan-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: op-sepolia GasLimit 60.0M
    superchain_test.go:80: Chain: op-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: op-mainnet GasLimit 60.0M
    superchain_test.go:80: Chain: op-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: redstone-mainnet GasLimit 100.0M
    superchain_test.go:80: Chain: redstone-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: unichain-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: unichain-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: worldchain-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: worldchain-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: zora-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: zora-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: funki-sepolia GasLimit 30.0M
    superchain_test.go:80: Chain: funki-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: race-sepolia GasLimit 30.0M
    superchain_test.go:80: Chain: race-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: shape-sepolia GasLimit 30.0M
    superchain_test.go:80: Chain: shape-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: bob-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: bob-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: zora-sepolia GasLimit 30.0M
    superchain_test.go:80: Chain: zora-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: lyra-mainnet GasLimit 120.0M
    superchain_test.go:80: Chain: lyra-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: ink-sepolia GasLimit 30.0M
    superchain_test.go:80: Chain: ink-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: arena-z-mainnet GasLimit 60.0M
    superchain_test.go:80: Chain: arena-z-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: shape-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: shape-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: swell-mainnet GasLimit 60.0M
    superchain_test.go:80: Chain: swell-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: tbn-mainnet GasLimit 30.0M
    superchain_test.go:80: Chain: tbn-mainnet ResourceConfig 20.0M
    superchain_test.go:78: Chain: arena-z-testnet-sepolia GasLimit 30.0M
    superchain_test.go:80: Chain: arena-z-testnet-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: base-sepolia GasLimit 60.0M
    superchain_test.go:80: Chain: base-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: lisk-sepolia GasLimit 30.0M
    superchain_test.go:80: Chain: lisk-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: metal-sepolia GasLimit 30.0M
    superchain_test.go:80: Chain: metal-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: tbn-sepolia GasLimit 30.0M
    superchain_test.go:80: Chain: tbn-sepolia ResourceConfig 20.0M
    superchain_test.go:78: Chain: base-mainnet GasLimit 116.0M
    superchain_test.go:80: Chain: base-mainnet ResourceConfig 20.0M
--- PASS: TestSuperchainGasLimit (0.19s)

Copy link
Contributor

@Ethnical Ethnical Mar 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks!
How to make sure we are running this just before the isthmus upgrade?
To make sure no Systemconfig has been updated by mistake after we just ran the script?
I will complete this Action item and open one that would require to run this script before the execution in the warroom of the upgrade tell me if you are okay with this one @geoknee please?

@Ethnical @geoknee
Update security/fma-isthmus-l2-pectra.md
d11dcc2 
Co-authored-by: George Knee <georgeknee@googlemail.com>
tynes
tynes reviewed Mar 26, 2025
View reviewed changes
security/fma-isthmus-l2-pectra.md

### FM8: Smart contracts relying on sender check no longer ensures sender does not have code

- **Description:** As part of EIP-7702, `msg.sender == tx.origin` no longer implies that the caller has no code. If unaddressed, this could cause
Copy link
Contributor

Choose a reason for hiding this comment

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

This has been handled, there is a new library for handling this check that has been introduced into our L1 contracts

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct the changes has been made on L1.
Also this is fixed on the L2 the modifier onlyEOA is used now:
https://github.com/ethereum-optimism/optimism/blob/9249efc6343208f69283290fc9c5c8f6e7b243f8/packages/contracts-bedrock/src/L2/L2StandardBridge.sol#L104

I would like still to know if there is any operation on the op-node that can potentially revert or cause unexpected behavior if interacting with an EIP7702 address such as minting, refund etc.
But the smart contractsa are correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

we should be safe from eip-3607 from just rebasing on top of latest geth, but good call on checking that deposits work as expected, https://eips.ethereum.org/EIPS/eip-3607

From Mark, this is correct but just in case can someone point me the tests that can cover this if there is?

@Ethnical Ethnical added the FMA Dedicated to identify Failures Mode Analysis label Mar 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajsutton ajsutton ajsutton left review comments

@geoknee geoknee geoknee left review comments

@sebastianst sebastianst sebastianst left review comments

@tynes tynes tynes left review comments

@mds1 mds1 mds1 left review comments

@refcell refcell refcell left review comments

@Ethnical Ethnical Ethnical left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@meyer9 meyer9

@geoknee geoknee

@refcell refcell

Labels
FMA Dedicated to identify Failures Mode Analysis H-isthmus
Projects
Protocol Team
Status: In Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@meyer9 @lewejahlil @ajsutton @geoknee @sebastianst @tynes @mds1 @refcell @Ethnical