[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
|
Hey @janezpodhostnik , I want to give you a heads up about the result of the Glamsterdam hard-fork activation, with regards to gas cost increase, might be related to onflow/flips#369 . There's this EIP: https://eips.ethereum.org/EIPS/eip-8037 in Glamsterdam, with changes to Examples:
The reasoning behind this increase, is to mitigate the state growth, from new EOAs and smart contracts. Thought that it might be useful in any ongoing work you are doing. |
… transfers & burns)
m-Peter
force-pushed
the
mpeter/flow-evm-glamsterdam-upgrade
branch
from
65d6f28 to
10f911e
Compare
This comment has been minimized.
This comment has been minimized.
4 participants
Closes: #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