[Flow EVM] Upgrade to Ethereum Glamsterdam hard-fork#8554
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 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
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 docstrings
🧪 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.
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.
… transfers & burns)
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?
| } | ||
|
|
||
| // 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. 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
| ethTransferLog := gethLogs[0] | ||
| require.Equal(t, gethParams.SystemAddress, ethTransferLog.Address) | ||
| require.Len(t, ethTransferLog.Topics, 3) | ||
| require.Equal(t, gethParams.EthTransferLogEvent, ethTransferLog.Topics[0]) |
There was a problem hiding this comment.
Change due to EIP-7708: ETH transfers emit a log
| ethTransferLog := gethLogs[0] | ||
| require.Equal(t, gethParams.SystemAddress, ethTransferLog.Address) | ||
| require.Len(t, ethTransferLog.Topics, 3) | ||
| require.Equal(t, gethParams.EthTransferLogEvent, ethTransferLog.Topics[0]) |
There was a problem hiding this comment.
Change due to EIP-7708: ETH transfers emit a log
| ethTransferLog := gethLogs[0] | ||
| require.Equal(t, gethParams.SystemAddress, ethTransferLog.Address) | ||
| require.Len(t, ethTransferLog.Topics, 3) | ||
| require.Equal(t, gethParams.EthTransferLogEvent, ethTransferLog.Topics[0]) |
There was a problem hiding this comment.
Change due to EIP-7708: ETH transfers emit a log
| ethTransferLog := gethLogs[0] | ||
| require.Equal(t, gethParams.SystemAddress, ethTransferLog.Address) | ||
| require.Len(t, ethTransferLog.Topics, 3) | ||
| require.Equal(t, gethParams.EthTransferLogEvent, ethTransferLog.Topics[0]) |
There was a problem hiding this comment.
Change due to EIP-7708: ETH transfers emit a log
| ) | ||
| } | ||
|
|
||
| func TestEthLogEmissionWithSelfDestruct(t *testing.T) { |
There was a problem hiding this comment.
Change due to EIP-7708: ETH transfers emit a log
m-Peter
requested review from
janezpodhostnik,
peterargue,
turbolent and
zhangchiqing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
fvm/evm/types/call.go (1)
94-104:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftUse
FromBig()with explicit overflow handling instead of panickingMustFromBig().Line 98 uses
uint256.MustFromBig(dc.Value), which panics if the big.Int value exceeds uint256 max. SinceDirectCall.Valueis deserialized via RLP inDirectCallFromEncoded()(line 69) without validation, untrusted input can trigger a panic. Per coding guidelines, treat all inputs as potentially byzantine and explicitly handle errors rather than relying on panics.Replace with
uint256.FromBig(dc.Value), which returns(u *Int, overflow bool), and handle the overflow condition with an error return. This pattern is already used elsewhere in the codebase (e.g., emulator.go, offchain/query/view.go).🤖 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 94 - 104, The Message() method on DirectCall currently uses uint256.MustFromBig(dc.Value) which can panic on overflow; change DirectCall.Message to use uint256.FromBig(dc.Value) and check the returned overflow bool, returning an error when overflow is true (i.e., change the signature to DirectCall.Message() (*gethCore.Message, error) and propagate errors to its callers), update all call sites accordingly (they may also need to handle the new error), and remove the MustFromBig usage; ensure DirectCallFromEncoded remains the source of untrusted input and does not assume Values are safe.fvm/evm/testutils/contract.go (1)
109-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the
DeployContracthelper to support contracts with non-payable constructors.The hardcoded
big.NewInt(1_000_000_000)wei at line 114 will cause deployments of contracts with non-payable constructors to revert. The FileSystem contract (used inTestEVMFileSystemContractat evm_test.go:6190 and 6251) has a non-payable constructor and is being deployed through this helper. Either make the funding amount a parameter, default to zero, or move contract-specific funding to individual test cases.🤖 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/testutils/contract.go` around lines 109 - 115, The DeployContract helper is forcing 1_000_000_000 wei into every deployment which breaks contracts with non-payable constructors; update the DeployContract helper (the call site that constructs types.NewDeployCall and calls blk2.DirectCall) to accept an explicit value parameter (or default it to zero) instead of the hardcoded big.NewInt(1_000_000_000), and propagate that parameter through callers/tests so tests that need funding can pass a non-zero value while contracts with non-payable constructors pass zero.
🧹 Nitpick comments (3)
fvm/evm/emulator/state/stateDB_test.go (1)
258-283: ⚡ Quick winAssert the returned BAL contents, not just nil/non-nil.
db.Finalise(true)returning an allocated-but-empty list would still satisfy this test, so regressions in Amsterdam access tracking can slip through. Capture the returned list and verify it contains the addresses/slots seeded byPrepare.🤖 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/emulator/state/stateDB_test.go` around lines 258 - 283, Capture the result of db.Finalise(true) into a variable instead of only asserting non-nil; then assert the list contains the expected entries seeded by db.Prepare: verify each address from precompiles appears in the final BAL and for each element in txAccesses verify its Address and each StorageKeys entry are present in the BAL (use existing helpers like AddressInAccessList and SlotInAccessList or iterate the BAL to match expected address/slot pairs). Keep the existing require.NoError(t, err) and db.Prepare(rules, sender, coinbase, &dest, precompiles, txAccesses) calls, then replace the require.NotNil(t, db.Finalise(true)) check with these explicit content assertions against the captured BAL.fvm/evm/evm_test.go (2)
1259-1363: ⚡ Quick winMake the Amsterdam precondition explicit in these cap-lift tests.
Both cases currently prove the new behavior only as long as the emulator bootstrap stays Amsterdam-active by default. If the activation timestamp moves, these tests will fail for configuration reasons instead of validating the lifted-cap path. Pin the fork-active block/timestamp used by the test so it always exercises the Amsterdam branch.
Also applies to: 3383-3443
🤖 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/evm_test.go` around lines 1259 - 1363, The test relies on the emulator default being Amsterdam-active; make that explicit by setting the Amsterdam fork activation in the environment used by this test so the Amsterdam branch is always exercised: update the test setup (the call into RunWithNewEnvironment or the environment builder it uses) to inject a fork config that pins Amsterdam as active at the test block/timestamp (e.g., set the Amsterdam activation block/timestamp to a value <= the test block or to the current time), so the EVM.run path under Amsterdam is forced; apply the same change to the other cap-lift test referenced (lines ~3383-3443) and reference the test harness functions RunWithNewEnvironment and any environment/fork config struct used to set the activation.
2174-2217: ⚡ Quick winAvoid hard-coding event positions in the new ETH-log assertions.
These checks depend on offsets like
output.Events[2],[4], and[6]. This PR already changed emitted event shape/order; another unrelated event insertion will break these tests without regressing the behavior under test. Prefer selecting the target event by type/payload and then decoding its logs.Also applies to: 2814-2835, 2894-2931, 2992-3020
🤖 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/evm_test.go` around lines 2174 - 2217, Tests assume event positions like output.Events[2] etc.; instead iterate over output.Events and select the event(s) you need by inspecting event.Type or by attempting to convert with TxEventToPayload / events.FlowEventToCadenceEvent and checking the payload contents (e.g., non-empty Logs for the EVM tx event, matching gethParams.SystemAddress or bridgeAddress in the decoded geth log, or BalanceAfterInAttoFlow for the deposit event). Replace direct index access with a small helper that finds the first event that successfully decodes to the expected payload (refer to TxEventToPayload, gethLogs decoding, events.FlowEventToCadenceEvent, and DecodeFLOWTokensDepositedEventPayload) and use that found event in the existing assertions; apply same approach for the other test blocks (callEVMHeartBeat / blockEventPayload assertions).
🤖 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/config.go`:
- Around line 197-200: Guard the Option constructors against nil big.Int inputs:
in WithGasPrice, check if gasPrice is nil before calling
uint256.MustFromBig(gasPrice) and only set c.TxContext.GasPrice when gasPrice is
non-nil (otherwise leave it unchanged or set to a clear zero value); likewise,
in WithChainID check for a nil *big.Int before calling .Uint64() and handle the
nil case safely (e.g., leave ChainID unchanged or set to 0) so no panic can
occur when callers pass nil.
In `@fvm/evm/emulator/emulator_test.go`:
- Around line 194-195: The test uses gethABI.MaxUint256 (the largest valid
uint256) so it doesn't trigger the overflow branch in checkAndConvertValue;
change the test's amount to an out-of-range value (e.g., a bigint representing
1<<256 or any value > gethABI.MaxUint256) so the code path that detects and
rejects values exceeding uint256 is exercised; update the variable `amount` in
the test at the site where it is set (currently using gethABI.MaxUint256) to a
properly overflowed value and ensure the test asserts the expected error from
checkAndConvertValue.
In `@fvm/evm/emulator/emulator.go`:
- Around line 635-638: The success path is only charging createDataGas and omits
init-code execution gas from proc.evm.Run(contract, ...); after calling
proc.evm.Run you must obtain the execution gas used (from the executed contract
instance or the gasBudget used during Run) and set res.GasConsumed =
executionGasUsed + createDataGas (instead of just createDataGas), and update the
subsequent limit check (the comparison at lines 650–655) to compare
(executionGasUsed + createDataGas) against the call.GasLimit so the deployAt
success path is charged the full execution plus data deposit cost; adjust
references to proc.evm.Run, res.GasConsumed, createDataGas and gasBudget
accordingly.
In `@fvm/evm/emulator/state/stateDB.go`:
- Around line 683-685: The stateReadList allocated in Prepare when
rules.IsAmsterdam can leak into later non-Amsterdam transactions; update Prepare
(or Reset) in the StateDB implementation to explicitly clear/unset the
stateReadList when rules.IsAmsterdam is false so Finalise() cannot return stale
accesses from a prior Amsterdam tx—i.e., where the current code only does "if
rules.IsAmsterdam { db.stateReadList = gethBAL.NewStateAccessList() }", add the
corresponding else branch (or Reset path) to set db.stateReadList = nil (or an
empty access list) so stateReadList is always cleared when Amsterdam is
inactive.
In `@fvm/evm/types/block.go`:
- Around line 217-226: The SlotNumber method can underflow when b.Timestamp <
genesisTimestamp; add a pre-check in BlockProposal.SlotNumber to return 0 if
b.Timestamp <= genesisTimestamp to avoid the uint64 wrap, then perform the
existing calculation using (b.Timestamp - genesisTimestamp) to compute the slot;
reference the GenesisBlock(chainID) call and the b.Timestamp/genesisTimestamp
subtraction when locating the fix.
---
Outside diff comments:
In `@fvm/evm/testutils/contract.go`:
- Around line 109-115: The DeployContract helper is forcing 1_000_000_000 wei
into every deployment which breaks contracts with non-payable constructors;
update the DeployContract helper (the call site that constructs
types.NewDeployCall and calls blk2.DirectCall) to accept an explicit value
parameter (or default it to zero) instead of the hardcoded
big.NewInt(1_000_000_000), and propagate that parameter through callers/tests so
tests that need funding can pass a non-zero value while contracts with
non-payable constructors pass zero.
In `@fvm/evm/types/call.go`:
- Around line 94-104: The Message() method on DirectCall currently uses
uint256.MustFromBig(dc.Value) which can panic on overflow; change
DirectCall.Message to use uint256.FromBig(dc.Value) and check the returned
overflow bool, returning an error when overflow is true (i.e., change the
signature to DirectCall.Message() (*gethCore.Message, error) and propagate
errors to its callers), update all call sites accordingly (they may also need to
handle the new error), and remove the MustFromBig usage; ensure
DirectCallFromEncoded remains the source of untrusted input and does not assume
Values are safe.
---
Nitpick comments:
In `@fvm/evm/emulator/state/stateDB_test.go`:
- Around line 258-283: Capture the result of db.Finalise(true) into a variable
instead of only asserting non-nil; then assert the list contains the expected
entries seeded by db.Prepare: verify each address from precompiles appears in
the final BAL and for each element in txAccesses verify its Address and each
StorageKeys entry are present in the BAL (use existing helpers like
AddressInAccessList and SlotInAccessList or iterate the BAL to match expected
address/slot pairs). Keep the existing require.NoError(t, err) and
db.Prepare(rules, sender, coinbase, &dest, precompiles, txAccesses) calls, then
replace the require.NotNil(t, db.Finalise(true)) check with these explicit
content assertions against the captured BAL.
In `@fvm/evm/evm_test.go`:
- Around line 1259-1363: The test relies on the emulator default being
Amsterdam-active; make that explicit by setting the Amsterdam fork activation in
the environment used by this test so the Amsterdam branch is always exercised:
update the test setup (the call into RunWithNewEnvironment or the environment
builder it uses) to inject a fork config that pins Amsterdam as active at the
test block/timestamp (e.g., set the Amsterdam activation block/timestamp to a
value <= the test block or to the current time), so the EVM.run path under
Amsterdam is forced; apply the same change to the other cap-lift test referenced
(lines ~3383-3443) and reference the test harness functions
RunWithNewEnvironment and any environment/fork config struct used to set the
activation.
- Around line 2174-2217: Tests assume event positions like output.Events[2]
etc.; instead iterate over output.Events and select the event(s) you need by
inspecting event.Type or by attempting to convert with TxEventToPayload /
events.FlowEventToCadenceEvent and checking the payload contents (e.g.,
non-empty Logs for the EVM tx event, matching gethParams.SystemAddress or
bridgeAddress in the decoded geth log, or BalanceAfterInAttoFlow for the deposit
event). Replace direct index access with a small helper that finds the first
event that successfully decodes to the expected payload (refer to
TxEventToPayload, gethLogs decoding, events.FlowEventToCadenceEvent, and
DecodeFLOWTokensDepositedEventPayload) and use that found event in the existing
assertions; apply same approach for the other test blocks (callEVMHeartBeat /
blockEventPayload assertions).
🪄 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: 937e7c70-774c-44d2-8834-fed75045d13e
⛔ 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
| // sort addresses | ||
| sortedAddresses := make([]gethCommon.Address, 0, len(addresses)) | ||
| for addr := range addresses { | ||
| sortedAddresses = append(sortedAddresses, addr) |
There was a problem hiding this comment.
This is not sorted, but just deduplicated.
I can see it's sorted later at L634, but can you think of the random access to the map at L609 would be an issue?
There was a problem hiding this comment.
That's a good catch. The allocation of sortedAddresses array is redundant. We only want to sort the derived []removedAccountWithBalance list. More specifically, the returned []*gethTypes.Log should be deterministic, that's why we sort the []removedAccountWithBalance list, prior to generating the array of EthBurnLog for each address. For reference, here's the original Geth implementation. How we traverse the array/map of dirty addresses is not important, as long as the result array of logs is sorted (and therefore deterministic).
Updated in b79f49f .
3 participants
Work Towards: #8553
Description
Upgrade Flow EVM with the upcoming changes included in the Ethereum Glamsterdam hard-fork.
See description in #8553, for more details regarding the upgrade changes involved.
Summary by CodeRabbit
New Features
Bug Fixes
Chores