Conversation
yperbasis
left a comment
yperbasis
left a comment
There was a problem hiding this comment.
Why are the unit tests failing?
# Conflicts: # execution/types/block_access_list.go
Becuase this branch is running parallel execution by default and needs some fixes which are in main to make it work. |
Let's merge this to |
There was a problem hiding this comment.
Pull request overview
This PR fixes failing non-list validation Hive tests for Amsterdam fork testing by addressing gas calculation ordering and state access handling issues in the EVM execution layer.
Changes:
- Reordered gas calculations to ensure stateless checks occur before storage access to fail early and prevent invalid operations
- Modified state access tracking to be more sticky and less susceptible to journal reversion, ensuring conformity with the BALs spec
- Added parallel execution requirement for Amsterdam fork testing
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rpc/jsonrpc/eth_receipts.go | Added early return for uninitialized node state |
| node/endpoints.go | Changed timeout sanitization logging from Warn to Debug for zero values; added copy-paste error in idle timeout message |
| execution/vm/operations_acl.go | Reordered gas checks and access list updates to enforce stateless validation before storage access |
| execution/vm/jump_table.go | Added new function signatures for stateless and stateful gas calculation separation |
| execution/vm/interpreter.go | Improved error wrapping to preserve ErrOutOfGas when it's already the error type |
| execution/vm/gas_table.go | Refactored gas calculation functions to separate stateless and stateful components |
| execution/vm/evm.go | Adjusted timing of MarkAddressAccess calls for CREATE operations |
| execution/tests/mock/mock_sentry.go | Added automatic disabling of Amsterdam for non-parallel execution in tests |
| execution/tests/engine_api_bal_test.go | Added test skip for non-parallel execution environments |
| execution/state/versionmap.go | Fixed type conversion for storage values in WriteChanges |
| execution/state/versionedio.go | Modified getStateObject calls to control read recording behavior |
| execution/state/state_test.go | Updated test to use new getStateObject signature and added code resolution test |
| execution/state/state_object.go | Enhanced SetState and SetCode to return change status and improved early returns |
| execution/state/rw_v3.go | Enhanced debug output with stack traces |
| execution/state/journal.go | Renamed touchChange to touchAccount and improved reversion logic for address access |
| execution/state/intra_block_state_test.go | Updated getStateObject calls to include recordRead parameter |
| execution/state/intra_block_state.go | Extensive refactoring of state access tracking with revertable options |
| execution/stagedsync/stageloop/stageloop.go | Fixed condition check for notification handling |
| execution/stagedsync/stage_execute.go | Added experimental BAL flag to parallel execution condition |
| execution/stagedsync/exec3_serial.go | Added Amsterdam fork validation to prevent serial execution |
| execution/stagedsync/bal_create_test.go | Fixed test output formatting for storage changes |
| execution/protocol/state_transition.go | Changed authority marking to non-revertable |
| execution/execmodule/forkchoice.go | Improved background task error handling and pruning conditions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if timeouts.IdleTimeout > 0 { | ||
| log.Warn("Sanitizing invalid HTTP idle timeout", "provided", timeouts.IdleTimeout, "updated", rpccfg.DefaultHTTPTimeouts.IdleTimeout) | ||
| } else { | ||
| log.Debug("Sanitizing invalid HTTP write timeout", "provided", timeouts.WriteTimeout, "updated", rpccfg.DefaultHTTPTimeouts.WriteTimeout) |
There was a problem hiding this comment.
The error message incorrectly refers to 'write timeout' but should refer to 'idle timeout' to match the field being sanitized.
| log.Debug("Sanitizing invalid HTTP write timeout", "provided", timeouts.WriteTimeout, "updated", rpccfg.DefaultHTTPTimeouts.WriteTimeout) | |
| log.Debug("Sanitizing invalid HTTP idle timeout", "provided", timeouts.IdleTimeout, "updated", rpccfg.DefaultHTTPTimeouts.IdleTimeout) |
| if dbg.TraceDyanmicGas && evm.intraBlockState.Trace() { | ||
| fmt.Printf("%d (%d.%d) CallCode Gas: base: %d memory(%d): %d call: %d\n", | ||
| evm.intraBlockState.BlockNumber(), evm.intraBlockState.TxIndex(), evm.intraBlockState.Incarnation(), gas-memoryGas, memorySize, memoryGas, callGas) | ||
| } |
There was a problem hiding this comment.
The debug trace condition on line 513 is duplicated - it's already checked on line 512. This nested condition should be removed to eliminate redundant code.
| var value uint256.Int | ||
| value.SetBytes32(change.Value[:]) | ||
| vm.Write(accountChanges.Address, StoragePath, storageChanges.Slot, Version{TxIndex: int(change.Index) - 1}, value, true) |
There was a problem hiding this comment.
The value variable is declared and set inline when it could be passed directly. Consider using uint256.Int{}.SetBytes32(change.Value[:]) directly in the Write call to reduce variable scope.
There was a problem hiding this comment.
This is wrong: uint256.Int{}.SetBytes32(change.Value[:]) will cause the following compiler error:
cannot call pointer method SetBytes32 on uint256.Int
| currentHeader := rawdb.ReadCurrentHeader(tx) | ||
| if (h.notifications.Accumulator != nil) && (currentHeader != nil) { | ||
| if currentHeader.Number.Sign() == 0 { | ||
| if currentHeader.Number.Sign() >= 0 { |
There was a problem hiding this comment.
This condition will always be true since Sign() returns -1, 0, or 1, and a block number cannot be negative. The original check for Sign() == 0 was checking for the genesis block specifically. This change may introduce unintended behavior.
| if currentHeader.Number.Sign() >= 0 { | |
| if currentHeader.Number.Sign() == 0 { |
There was a problem hiding this comment.
making this == 0 introduces a bug which I fixed, because it was only updating block zero. The assertion is correct but the suggested fix is wrong - it should be to remove the if entierly.
Note that this is a bug which was introduced by a previous AI driven refactor.
Fixed by 2f4c99e
|
|
||
| if !dbg.BatchCommitments || shouldGenerateChangesets || se.cfg.syncCfg.KeepExecutionProofs { | ||
| if dbg.TraceBlock(blockNum) { | ||
| fmt.Println(blockNum, "Commitment") |
There was a problem hiding this comment.
This is only printed if block tracing is turned on. What this print does is make the trace oututput for the trie calculation look like the example below.
This is useful when you have a large file with a lot of trie entries and you want to find the commitment calculation for a particular block. Also if you only have trie tracing tiurned on and don't have this seperator - even in tests its difficult to see where one block starts and the other stops.
At the moment the commitment trace does not include block numbers becuase it may be a bulk calculation so the internal printer does not know the block associated with the particular touch.
The main use of this is to find the cuase of a state root mismatch by performing a comparison between a good and a bad block.
It will only get printed if block tracing is on.
24271547 Commitment
(1/6) plainKey [71562b71999873db5b286df957af199ec94617f7] Flags: [+Balance+Nonce+Code], Balance: [999657745], Nonce: [8], CodeHash: [c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470] hashedKey [00000b0f04090f0404000a010c0d000502070e040d00060e020706050605040c000f05060405020205070501060d0709030a090b080d0600040d0c0f0d0f020a] currentKey []
(2/6) plainKey [3a220f351252089d385b29beca14e27f204c296a] Flags: [+Balance+Nonce+Code], Balance: [0], Nonce: [5], CodeHash: [85a288cf6632a91047b1a2ebbaa77a1ab8e82961b95a7eb13123cd089c38e18b] hashedKey [010c0b020508030704080c02060e08090e0f01090c020a080502090b00050a0207000f0703050505030b040d04040b060f020a010809040908070a07010c080b] currentKey [00000b0f04090f0404000a010c0d000502070e040d00060e020706050605040c000f05060405020205070501060d0709030a090b080d0600040d0c0f0d0f02]
...
This conatains fixes for all outstanding non list validation fails when running:
Which should result in:
The know failing tests are:
I have also seen a one off fail of when testing but never been able to reproduce it, so am assuming it was a transient error:
The majority of the fixes are adjustments to the order of gas calculations to make sure that statless gas check haooen and fail early so that no storage acces occurs.
There are also some adjustments to when reads and writes occur and how they are reverted to ensure that these conform to the model required by the bals spec. These generally make state accesses more 'sticky' than the journalling process.