[Flow EVM] Upgrade to Ethereum Glamsterdam hard-fork#8554
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Amsterdam fork activation and slot-number support, migrates EVM numeric fields to holiman/uint256, implements per-transaction state-access tracking and burn logs, refactors deployment/run paths, updates error mappings and tests, and bumps go-ethereum and related dependencies. ChangesGlamsterdam Hard-fork Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This comment was marked as outdated.
This comment was marked as outdated.
| @@ -188,7 +196,7 @@ func WithOrigin(origin gethCommon.Address) Option { | |||
| // WithGasPrice sets the gas price for the transaction (usually the one sets by the sender) | |||
| func WithGasPrice(gasPrice *big.Int) Option { | |||
There was a problem hiding this comment.
WithGasPrice seems to be not used in flow-go or flow-evm-gateway. Seems like a relic that we could remove entirely.
| // if the block gas limit is set to anything than max | ||
| // we need to update this code. | ||
| gasPool := (*gethCore.GasPool)(&proc.config.BlockContext.GasLimit) | ||
| gasPool := gethCore.NewGasPool(proc.config.BlockContext.GasLimit) |
There was a problem hiding this comment.
I'm not sure why the gasPool used to receive the maximum value as the available gas:
DefaultBlockLevelGasLimit = uint64(math.MaxUint64)but given that we apply only a single EVM message, per gasPool instance, it might be safer to do what Geth does:
// Do not panic if the gas pool is nil. This is allowed when executing
// a single message via RPC invocation.
if gp == nil {
gp = NewGasPool(msg.GasLimit)
}and use as a sane default the GasLimit from the given EVM message.
There was a problem hiding this comment.
Good catch. I don't know. I think its because we already checked that there is sufficient gas. Lets leave this as is for this PR but maybe lets revisit it in a separate PR.
This comment was marked as outdated.
This comment was marked as outdated.
3 similar comments
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
m-Peter
force-pushed
the
mpeter/flow-evm-glamsterdam-upgrade
branch
from
a2d3b10 to
9c45c56
Compare
This comment was marked as outdated.
This comment was marked as outdated.
2 similar comments
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
m-Peter
force-pushed
the
mpeter/flow-evm-glamsterdam-upgrade
branch
from
2f6db64 to
99db4ec
Compare
This comment was marked as outdated.
This comment was marked as outdated.
| // as slot duration we can use the block production rate, which is about 0.8 | ||
| // seconds on mainnet. | ||
| genesisTimestamp := GenesisBlock(chainID).Timestamp | ||
| return ((b.Timestamp - genesisTimestamp) * 5) / 4 |
There was a problem hiding this comment.
We can differentiate the slot duration for testnet, as I recall that it has a different block production rate, compared to mainnet. Does anyone know what that value is?
There was a problem hiding this comment.
testnet is running at 2 block / sec.
mainnet is running at 1.25 block / sec.
There was a problem hiding this comment.
maybe mention these number in the comments ^
There was a problem hiding this comment.
Do we want to store this in a slot and read from it to be configable? but that would involve a storage read.
Currently the block rate is configured by smart contract:
import FlowEpoch from 0x8624b52f9ddcd04a
access(all) fun main(): {String: UInt64} {
let config = FlowEpoch.getConfigMetadata()
let timing = FlowEpoch.getEpochTimingConfig()
return {
"numViewsInEpoch": config.numViewsInEpoch,
"epochDurationSeconds": timing.duration
}
}
For mainnet this returns
Result: {"numViewsInEpoch": 756000, "epochDurationSeconds": 604800}
- 756,000 views per epoch
- 604,800 seconds (1 week) per epoch
- so targetViewTime = 604800 / 756000 = 0.8 seconds/block = 800ms/block or 1.25 blocks / sec
There was a problem hiding this comment.
Nice, much thanks 🙌
maybe mention these number in the comments ^
I will update the comments and the calculation accordingly.
Do we want to store this in a slot and read from it to be configable? but that would involve a storage read.
In Ethereum, the slot duration is hard-coded to 12 seconds, on the consensus layer, regardless of whether the block proposer managed to build a block during a given slot. For Flow EVM, I think we can base the slot number calculation on the block production rate for each network. As long as we feed the SLOTNUM opcode a deterministic value, I think we are compatible and good-to-go. I wouldn't go so far to store this in a slot and add 1 more storage read. Note that adding fields in models such as BlockProposal adds a bit more complexity, due to using RLP encoding/decoding for storage.
There was a problem hiding this comment.
Added comments and updated the calculation per network in 8a3ce81 .
| } | ||
|
|
||
| // convert tx into message | ||
| msg, err := gethCore.TransactionToMessage( |
There was a problem hiding this comment.
We can no longer use gethCore.TransactionToMessage, as it returns a nil Message when no valid signature is provided (see https://github.com/ethereum/go-ethereum/blob/v1.17.3/core/state_transition.go#L240-L244). For EVM.dryRun we do not have such signature values, so we have to reconstruct the Message ourselves.
| MainnetOsakaActivation = uint64(1764784800) // Wednesday, December 03, 2025 18:00:00 GMT+0000 | ||
|
|
||
| PreviewnetAmsterdamActivation = uint64(0) // already on Amsterdam for PreviewNet | ||
| TestnetAmsterdamActivation = uint64(1798740000) // Thursday, December 31, 2026 at 18:00:00 GMT+0000 |
There was a problem hiding this comment.
TestnetAmsterdamActivation & MainnetAmsterdamActivation will be updated accordingly when there's official activation times.
| if i%2 == 0 { | ||
| // fail with too low gas limit | ||
| gas = 22_000 | ||
| gas = 23_500 |
There was a problem hiding this comment.
Change due to EIP-7976: Increase Calldata Floor Cost
| ), | ||
| big.NewInt(0), | ||
| uint64(2_132_171), | ||
| uint64(3_375_890), |
There was a problem hiding this comment.
Change due to EIP-7976: Increase Calldata Floor Cost
| // | ||
| require.NotEmpty(t, blockEventPayload.Hash) | ||
| require.Equal(t, uint64(2_132_170), blockEventPayload.TotalGasUsed) | ||
| require.Equal(t, uint64(3_396_880), blockEventPayload.TotalGasUsed) |
There was a problem hiding this comment.
Change due to EIP-7976: Increase Calldata Floor Cost
| require.Equal(t, types.ValidationErrCodeMisc, res.ErrorCode) | ||
| require.Equal( | ||
| t, | ||
| "transaction gas limit too high (cap: 16777216, tx: 16777226)", |
There was a problem hiding this comment.
Change due to EIP-8037: State Creation Gas Cost Increase
| let res = EVM.run(tx: tx, coinbase: coinbase) | ||
| assert(res.status == EVM.Status.invalid, message: "unexpected status") | ||
| assert(res.errorCode == 100, message: "unexpected error code: \(res.errorCode)") | ||
| assert(res.errorMessage == "transaction gas limit too high (cap: 16777216, tx: 16777220)") |
There was a problem hiding this comment.
Change due to EIP-8037: State Creation Gas Cost Increase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fvm/evm/emulator/emulator.go`:
- Around line 336-350: When constructing the gethCore.Message for dry-run in
emulator.go (the msg literal), populate the blob-specific fields so validation
matches normal execution: set BlobGasFeeCap to tx.BlobGasFeeCap() and BlobHashes
to tx.BlobHashes() (alongside the existing GasTipCap/GasFeeCap fields) so blob
gas fee and blob hash validation are enforced during dry-run.
In `@fvm/evm/emulator/state/stateDB.go`:
- Around line 663-665: Finalise currently returns the live pointer
db.stateReadList which allows subsequent Commit and its access-recording getters
(HasSelfDestructed, GetBalance, GetNonce, GetCode, GetCodeHash, GetState) to
mutate the returned list; change Finalise to return a snapshot copy of
stateReadList (e.g. allocate a new slice/map and copy entries) so callers
receive an immutable snapshot of the EIP-2929 access footprint, or alternatively
disable access recording during the Commit walk invoked after Finalise; update
the Finalise function to perform the copy and return that copy instead of
db.stateReadList.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4eb8059c-ecbd-49d6-9b49-55535271d3f9
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.suminsecure/go.sumis excluded by!**/*.sumintegration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
fvm/evm/emulator/config.gofvm/evm/emulator/emulator.gofvm/evm/emulator/emulator_test.gofvm/evm/emulator/state/stateDB.gofvm/evm/emulator/state/stateDB_test.gofvm/evm/evm_test.gofvm/evm/handler/handler.gofvm/evm/testutils/contract.gofvm/evm/testutils/contracts/factory.solfvm/evm/testutils/contracts/factory_abi.jsonfvm/evm/testutils/contracts/factory_bytes.hexfvm/evm/types/block.gofvm/evm/types/block_test.gofvm/evm/types/call.gofvm/evm/types/codeFinder.gofvm/evm/types/emulator.gogo.modinsecure/go.modintegration/go.mod
✅ Files skipped from review due to trivial changes (1)
- fvm/evm/types/emulator.go
| return 0 | ||
| } | ||
|
|
||
| switch chainID { |
There was a problem hiding this comment.
@m-Peter what is the reasoning behind having slot numbers to be dependent on the block time?
Is there anything stopping us from just saying that the slot size is 1s or 2s or 10s?
What does the slot length affect?
There was a problem hiding this comment.
Some context from the Ethereum docs:
In Ethereum, a slot number is a specific unit of time in the proof-of-stake consensus clock. Each slot is exactly 12 seconds long and acts as an opportunity for a randomly selected validator to propose a new block. If no block is proposed, the slot is recorded as missed.
The "Ticker" of the Blockchain: The Beacon Chain uses slot numbers to keep time. Because slots happen precisely every 12 seconds, you can calculate the exact slot number from the current block timestamp, and vice versa.
Regarding your questions:
Is there anything stopping us from just saying that the slot size is 1s or 2s or 10s?
I guess no, since we don't use Ethereum's PoS consensus mechanism. I used the block production rate of TN/MN, to base the calculation on a unit of time that resembles the notion of slot duration in Ethereum.
What does the slot length affect?
Not entirely sure about the planned use-cases, but there will be a SLOTNUM opcode, that will basically allow for retrieving the current slot number on-chain (in Solidity smart contracts).
There was a problem hiding this comment.
Thanks for the explanation. Yeah, I think lets leave it as you have it then.
| // if the block gas limit is set to anything than max | ||
| // we need to update this code. | ||
| gasPool := (*gethCore.GasPool)(&proc.config.BlockContext.GasLimit) | ||
| gasPool := gethCore.NewGasPool(proc.config.BlockContext.GasLimit) |
There was a problem hiding this comment.
Good catch. I don't know. I think its because we already checked that there is sufficient gas. Lets leave this as is for this PR but maybe lets revisit it in a separate PR.
… transfers & burns)
m-Peter
force-pushed
the
mpeter/flow-evm-glamsterdam-upgrade
branch
from
8a3ce81 to
93ab927
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
fvm/evm/types/call.go (1)
171-176: ⚡ Quick winAdd table-driven tests for the new Osaka/Amsterdam gas-cap branch.
This fork-transition branch is protocol-critical, and current patch coverage for
fvm/evm/types/call.gois reported as 0%, so regressions can slip in unnoticed. Please add explicit cases for: skip-check enabled,Osaka && !Amsterdamover-cap (invalid),Osaka && Amsterdamover-cap (valid), and pre-Osaka (valid).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fvm/evm/types/call.go` around lines 171 - 176, Add table-driven tests that exercise the Osaka/Amsterdam gas-cap branch in the call validation logic (the code that checks dc.GasLimit against gethParams.MaxTxGas and uses rules.IsOsaka and rules.IsAmsterdam). Create cases for: skip-check enabled (should bypass the cap), Osaka && !Amsterdam with GasLimit > MaxTxGas (should be invalid), Osaka && Amsterdam with GasLimit > MaxTxGas (should be valid), and pre-Osaka with GasLimit > MaxTxGas (should be valid). In each case construct a minimal Call (dc) and Rules state (setting rules.IsOsaka / rules.IsAmsterdam) and assert the boolean result from the validation function that contains the dc.GasLimit > gethParams.MaxTxGas check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fvm/evm/emulator/emulator.go`:
- Around line 735-738: Batch-level gas is not enforced because run always
creates a fresh gas pool from proc.config.BlockContext.GasLimit; change run to
account for gas already consumed by the batch (BatchRunTransactions'
BlockTotalGasUsedSoFar) by computing remainingGas :=
proc.config.BlockContext.GasLimit - proc.BlockTotalGasUsedSoFar (clamp to zero)
and passing remainingGas into gethCore.NewGasPool instead of the full
BlockContext.GasLimit, so later transactions cannot use gas already spent in the
batch.
- Around line 314-333: The overflow branches in emulator.go (calls to
uint256.FromBig for tx.Value, tx.GasPrice, tx.GasFeeCap, tx.GasTipCap) currently
return hard Go errors which abort EVM.dryRun; instead construct an invalid
dry-run result and return it: call types.NewInvalidResult(...) (or the existing
result builder used by RunTransaction), set the validation error via
result.SetValidationError(...) with a descriptive error (include the offending
field and bit length or original error context such as
gethCore.ErrFeeCapVeryHigh / gethCore.ErrTipVeryHigh), and return result, nil;
replace each fmt.Errorf return in those overflow branches with this pattern so
dryRun returns an invalid EVM result rather than raising an error.
---
Nitpick comments:
In `@fvm/evm/types/call.go`:
- Around line 171-176: Add table-driven tests that exercise the Osaka/Amsterdam
gas-cap branch in the call validation logic (the code that checks dc.GasLimit
against gethParams.MaxTxGas and uses rules.IsOsaka and rules.IsAmsterdam).
Create cases for: skip-check enabled (should bypass the cap), Osaka &&
!Amsterdam with GasLimit > MaxTxGas (should be invalid), Osaka && Amsterdam with
GasLimit > MaxTxGas (should be valid), and pre-Osaka with GasLimit > MaxTxGas
(should be valid). In each case construct a minimal Call (dc) and Rules state
(setting rules.IsOsaka / rules.IsAmsterdam) and assert the boolean result from
the validation function that contains the dc.GasLimit > gethParams.MaxTxGas
check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 552b5c1a-ee2b-4ea1-9c19-5c4c7c203727
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.suminsecure/go.sumis excluded by!**/*.sumintegration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
fvm/evm/emulator/config.gofvm/evm/emulator/emulator.gofvm/evm/emulator/emulator_test.gofvm/evm/emulator/state/stateDB.gofvm/evm/emulator/state/stateDB_test.gofvm/evm/evm_test.gofvm/evm/handler/handler.gofvm/evm/testutils/contract.gofvm/evm/testutils/contracts/factory.solfvm/evm/testutils/contracts/factory_abi.jsonfvm/evm/testutils/contracts/factory_bytes.hexfvm/evm/types/block.gofvm/evm/types/block_test.gofvm/evm/types/call.gofvm/evm/types/codeFinder.gofvm/evm/types/emulator.gogo.modinsecure/go.modintegration/go.mod
This comment has been minimized.
This comment has been minimized.
4 participants
Work Towards: #8553
Description
Upgrade Flow EVM with the upcoming changes included in the Ethereum Glamsterdam hard-fork.
Updated return value of
GenesisTimestamp()forEmulator&Previenetnetworks, to2024-01-01so that we have a deterministic value for theSLOTNUMopcode.See description in #8553, for more details regarding the upgrade changes involved.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores