[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
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.
|
@m-Peter Thanks for the heads up. As discussed, lets just go forward with the costs defined in eip-8037 without any customization from our side. We should be careful about this in our future calibration... If we always create a new account in out synthetic traffic, its going to throw off the data. |
| // Arbitrary high gas limit that can be used for all the | ||
| // EVM transactions to ensure none of them run out of gas | ||
| gasLimit := 15000000 | ||
| gasLimit := 16_777_216 |
There was a problem hiding this comment.
Is this the exact limit needed now? If so maybe add some headroom and set it to 20m?
There was a problem hiding this comment.
This is the max gas limit (2^24) under the currently active hard-fork, which is Fusaka, see the relevant EIP: https://eips.ethereum.org/EIPS/eip-7825 . However, it will be no longer relevant in Glamsterdam, as the max gas limit of 2^24 is being revoked, in favor of EIP-8037: https://eips.ethereum.org/EIPS/eip-8037. Good point, I'll increase it to 20M, so that we don't have to come back to this anytime soon.
There was a problem hiding this comment.
Updated in 73f30b8 . The 15M gas limit was not sufficient for Glamsterdam.
| // to propose a new block, functioning as the network's "heartbeat" in PoS. | ||
| // Slots occur automatically every 12 seconds, and 32 slots are grouped together | ||
| // into a 6.4-minute period known as an epoch. | ||
| func (b *Block) SlotNumber(chainID flow.ChainID) uint64 { |
There was a problem hiding this comment.
Do we expect issues if 2 Flow blocks (and Flow EVM blocks) have the same slot number?
That could happen if the chain is running faster than usual.
There was a problem hiding this comment.
I really doubt if this will cause any issues, because on Flow EVM we don't really utilize the consensus mechanism of Ethereum mainnet, we simply piggy-back on Flow's consensus mechanism. This is just a rough calculation to provide some sensible values for any dev/team out there that may be using some existing Solidty dApp, which makes use of this opcode. So the main usage of this is to be compatible with the opcodes offered by Ethereum mainnet.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
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