[Flow EVM] Add functionality to restrict EOAs from accessing EVM#8310
[Flow EVM] Add functionality to restrict EOAs from accessing EVM#8310m-Peter merged 2 commits intomasteronflow/flow-go:masterfrom mpeter/back-port-test-restricted-eoa-functionalityonflow/flow-go:mpeter/back-port-test-restricted-eoa-functionalityCopy head branch name to clipboard
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughWalkthroughAdds Restricted EOA handling to the EVM emulator (config, membership check, enforcement in single and batch runs), changes NewInvalidResult signature, introduces ErrRestrictedEOA, updates related tests and constants, and tweaks a minor formatting line. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Emulator
participant Config
participant ResultFactory
Client->>Emulator: RunTransaction(tx)
Emulator->>Emulator: parse tx -> sender, txType, txHash
Emulator->>Config: IsRestrictedEOA(sender)?
Config-->>Emulator: true / false
alt sender restricted
Emulator->>ResultFactory: NewInvalidResult(txType, txHash, ErrRestrictedEOA)
ResultFactory-->>Emulator: InvalidResult
Emulator-->>Client: return InvalidResult (ErrRestrictedEOA)
else sender allowed
Emulator->>Emulator: execute transaction
Emulator-->>Client: return Execution Result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @fvm/evm/emulator/config.go:
- Around line 31-35: The global mutable slice RestrictedEOAs is causing race
conditions because tests modify it (see evm_test.go); change it to immutable or
make access thread-safe: replace the exported variable with an unexported
constant slice or a getter function (e.g., restrictedEOAs() that returns a copy)
and remove any direct writes, or encapsulate it behind package functions plus a
sync.RWMutex or atomic.Value and add a TestOnly helper (e.g.,
SetRestrictedEOAsForTest) that safely swaps values for tests; update all call
sites to use the getter/API instead of directly mutating RestrictedEOAs.
In @fvm/evm/emulator/emulator.go:
- Line 57: The code currently injects the global RestrictedEOAs into every
emulator config via newConfig() using WithRestrictedEOAs, which couples the
emulator to a hardcoded list; change newConfig (or the emulator constructor) to
accept RestrictedEOAs as an argument (or accept a ConfigFactory/option that
supplies the list) and thread that parameter into the call site that applies
WithRestrictedEOAs so callers can provide a different slice at runtime; update
places that construct the emulator to pass the injected list (or factory) and
remove reliance on the global RestrictedEOAs variable.
In @fvm/evm/evm_test.go:
- Around line 329-406: The test uses t.Parallel() while mutating the global
emulator.RestrictedEOAs (appending in the test and removing via defer), causing
a race; fix by removing t.Parallel() from the t.Run block (or alternatively make
emulator per-test or protect mutations with a sync.Mutex) so
RunWithNewEnvironment / the test that calls GetTestEOAAccount and appends to
emulator.RestrictedEOAs no longer races with the other test that touches the
same global; if you choose the mutex approach, guard both the append and the
deferred slice restore around a package-level mutex to ensure atomic
modification and cleanup.
In @fvm/evm/types/errors.go:
- Around line 116-118: The ErrRestrictedEOA error message contains the wrong
security contact; update the string in ErrRestrictedEOA to replace
"security@flowfoundation.com" with the correct "security@onflow.org" while
keeping the rest of the message intact so users are directed to the proper
security contact.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/emulator/config.gofvm/evm/emulator/emulator.gofvm/evm/evm_test.gofvm/evm/stdlib/contract.cdcfvm/evm/testutils/accounts.gofvm/evm/types/errors.gofvm/evm/types/result.goutils/unittest/execution_state.go
🧰 Additional context used
📓 Path-based instructions (7)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in @docs/agents/CodingConventions.md
Follow Go coding standards and conventions as documented in @docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/types/errors.gofvm/evm/emulator/config.gofvm/evm/types/result.gofvm/evm/testutils/accounts.goutils/unittest/execution_state.gofvm/evm/evm_test.gofvm/evm/emulator/emulator.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Unit tests should be co-located with the code they test
Follow the existing pattern of*_test.gofiles for test naming
Use fixtures for realistic test data as defined in/utils/unittest/
Files:
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/evm_test.go
{module,engine,cmd}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
All major processing components must implement the
Componentinterface from/module/component/component.goto ensure consistent lifecycle management and graceful shutdown patterns
Files:
engine/execution/state/bootstrap/bootstrap_test.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/types/errors.gofvm/evm/emulator/config.gofvm/evm/types/result.gofvm/evm/testutils/accounts.gofvm/evm/evm_test.gofvm/evm/emulator/emulator.go
{network,engine,consensus}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Network messages must be authenticated and validated
Files:
engine/execution/state/bootstrap/bootstrap_test.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/evm/types/errors.gofvm/evm/emulator/config.gofvm/evm/types/result.gofvm/evm/testutils/accounts.gofvm/evm/evm_test.gofvm/evm/emulator/emulator.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/evm/types/errors.gofvm/evm/emulator/config.gofvm/evm/types/result.gofvm/evm/testutils/accounts.gofvm/evm/evm_test.gofvm/evm/emulator/emulator.go
🧠 Learnings (1)
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {storage,ledger,execution,fvm}/**/*.go : State consistency is paramount; use proper synchronization primitives
Applied to files:
utils/unittest/execution_state.go
🧬 Code graph analysis (2)
utils/unittest/execution_state.go (1)
model/flow/chain.go (1)
Sandboxnet(26-26)
fvm/evm/emulator/emulator.go (3)
fvm/evm/emulator/config.go (2)
WithRestrictedEOAs(312-317)RestrictedEOAs(32-35)fvm/evm/types/result.go (1)
NewInvalidResult(57-64)fvm/evm/types/errors.go (2)
ErrRestrictedEOA(116-118)ErrInvalidBalance(91-91)
🪛 Gitleaks (8.30.0)
fvm/evm/testutils/accounts.go
[high] 25-25: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 28-28: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (37)
- GitHub Check: Lint (./)
- GitHub Check: Lint (./integration/)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: analyze-code (go)
- GitHub Check: Docker Build
🔇 Additional comments (12)
fvm/evm/stdlib/contract.cdc (1)
693-694: LGTM! Formatting change only.The indentation adjustment has no functional impact on the code.
engine/execution/state/bootstrap/bootstrap_test.go (2)
61-61: Test expectation updated for new genesis state.The expected state commitment has been updated to reflect changes in genesis state initialization. This aligns with the updated
GenesisStateCommitmentHexinutils/unittest/execution_state.go.
110-110: Test expectation updated for empty transaction scenario.The expected state commitment for the empty transaction test has been updated consistently with other genesis state changes in this PR.
fvm/evm/testutils/accounts.go (1)
23-29: LGTM! Test account constant added for restricted EOA testing.The new
RestrictedEOATestAccount1KeyHexconstant appropriately supports testing of the restricted EOA functionality. The static analysis warning about a "Generic API Key" is a false positive—this is a test ECDSA private key for a deterministic test account, not a production credential.utils/unittest/execution_state.go (2)
26-26: Verify the genesis state commitment reflects new restricted EOA functionality.The
GenesisStateCommitmentHexconstant is properly used without duplication and is validated inTestBootstrapLedger. However, ensure the hex value was correctly derived by running the bootstrap process locally with the new restricted EOA implementation to confirm it matches the actual bootstrapped state.
90-90: These genesis commitment values are hardcoded test fixtures without active bootstrap test validation.The bootstrap tests in
engine/execution/state/bootstrap/bootstrap_test.goonly validate the Mainnet commitment (againstunittest.GenesisStateCommitment). The Testnet, Sandboxnet, and default genesis commitments at lines 90 and 95 are hardcoded fixture values that are not validated by any bootstrap test. The comment referencingTestBootstrapLedgeris misleading—that test only validates Mainnet. If these values were updated, ensure they were generated correctly by the state bootstrapping process, but be aware no automated test will catch regressions in non-Mainnet values.Likely an incorrect or invalid review comment.
fvm/evm/types/result.go (1)
57-64: LGTM: API change improves flexibility.The signature change from accepting a transaction object to explicit
txTypeandtxHashparameters is appropriate for the restricted EOA feature, where invalid results need to be created before full transaction processing.fvm/evm/emulator/config.go (2)
76-79: LGTM: Clean helper method.The
IsRestrictedEOAmethod provides a clean API for checking restricted addresses. Usingslices.Containsis appropriate for small lists.
310-317: LGTM: Consistent with option pattern.The
WithRestrictedEOAsoption function follows the existing configuration pattern and enables test-specific configurations.fvm/evm/emulator/emulator.go (3)
196-198: Well-placed security check.The restricted EOA check is correctly positioned after message construction but before execution, ensuring invalid transactions are rejected early without consuming resources beyond validation costs.
256-259: Good batch handling strategy.In batch processing, continuing to the next transaction when encountering a restricted EOA (rather than failing the entire batch) is the correct approach, maintaining consistency with other invalid transaction handling.
398-401: All NewInvalidResult call sites have been correctly updated.The signature change to
NewInvalidResult(txType uint8, txHash gethCommon.Hash, err error)has been applied across all call sites in the codebase. No remaining old-style calls were found.
| // List of EOAs with restricted access to EVM, due to malicious activity. | ||
| var RestrictedEOAs = []gethCommon.Address{ | ||
| gethCommon.HexToAddress("0x2e7C4b71397f10c93dC0C2ba6f8f179A47F994e1"), | ||
| gethCommon.HexToAddress("0x9D9247F5C3F3B78F7EE2C480B9CDaB91393Bf4D6"), | ||
| } |
There was a problem hiding this comment.
Thread-safety concern: Mutable global variable.
The global RestrictedEOAs variable is accessed by tests and production code. Tests modify it (as seen in evm_test.go), which creates race conditions in parallel test execution. Consider making this immutable or providing thread-safe test utilities.
As per coding guidelines, all inputs must be considered potentially byzantine; global mutable state should be avoided where possible.
🤖 Prompt for AI Agents
In @fvm/evm/emulator/config.go around lines 31 - 35, The global mutable slice
RestrictedEOAs is causing race conditions because tests modify it (see
evm_test.go); change it to immutable or make access thread-safe: replace the
exported variable with an unexported constant slice or a getter function (e.g.,
restrictedEOAs() that returns a copy) and remove any direct writes, or
encapsulate it behind package functions plus a sync.RWMutex or atomic.Value and
add a TestOnly helper (e.g., SetRestrictedEOAsForTest) that safely swaps values
for tests; update all call sites to use the getter/API instead of directly
mutating RestrictedEOAs.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the specific commit and show the changes
git fetch origin pull/8310/head:pr-8310 2>/dev/null || true
git checkout pr-8310 2>/dev/null || true
# Show the specific commit changes
echo "=== Commit changes ==="
git show 3cba2800255a216c5c37de16b900117f1fcbaa5c --stat
echo -e "\n=== Detailed diff for config.go ==="
git show 3cba2800255a216c5c37de16b900117f1fcbaa5c -- fvm/evm/emulator/config.go
echo -e "\n=== Detailed diff for evm_test.go ==="
git show 3cba2800255a216c5c37de16b900117f1fcbaa5c -- fvm/evm/evm_test.go
Length of output: 2050
@m-Peter, thank you for the improvement! The cleanup logic is now more robust by preserving and restoring the original slice state rather than just removing the last element.
However, the core thread-safety concern still remains. While the cleanup is better, the global emulator.RestrictedEOAs variable is still mutable and directly accessed by multiple tests. If tests run in parallel (go test -parallel), there's still a race condition where:
- Test A reads
RestrictedEOAs, makes a copy, and appends to it - Test B simultaneously reads
RestrictedEOAs, makes a copy, and appends to it - Both tests race when writing back to the global variable
To fully resolve this, consider one of these approaches:
Option 1 (Recommended): Pass restricted EOAs through the config instead of using a global variable:
- Remove the global default from
config.go - In tests, use
WithRestrictedEOAs(append(GetDefaultRestrictedEOAs(), testEOA))when creating the config - This eliminates shared mutable state entirely
Option 2: Add synchronization around the global variable access with a mutex, though this adds complexity.
Would you like to implement Option 1 for a cleaner, thread-safe solution?
🧬 Code Graph Analysis Results
File: model/flow/chain.go (lines 14-14)
ChainID string
File: fvm/evm/types/call.go (lines 317-317)
GasLimit uint64
File: utils/unittest/fixtures/time.go (lines 12-12)
Time timeFactory
File: fvm/evm/emulator/tracker.go (lines 12-14)
CallTracker struct {
callsByAddress map[types.Address]*types.PrecompiledCalls
}
File: fvm/evm/emulator/tracker.go (lines 17-19)
func NewCallTracker() *CallTracker {
return &CallTracker{}
}
🧠 Learnings used
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {storage,ledger,execution,fvm}/**/*.go : State consistency is paramount; use proper synchronization primitives
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,storage}/**/*.go : Security checks for cryptographic misuse must be enforced using gosec
| WithTransactionTracer(ctx.Tracer), | ||
| WithBlockTotalGasUsedSoFar(ctx.TotalGasUsedSoFar), | ||
| WithBlockTxCountSoFar(ctx.TxCountSoFar), | ||
| WithRestrictedEOAs(RestrictedEOAs), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find how RestrictedEOAs is initialized and used in production code
echo "=== RestrictedEOAs initialization patterns ==="
rg -n "RestrictedEOAs\s*=" --type=go -g '!*_test.go'Repository: onflow/flow-go
Length of output: 244
🏁 Script executed:
#!/bin/bash
# Examine the emulator.go file around line 57 and understand the context
echo "=== Emulator.go context around line 57 ==="
cat -n fvm/evm/emulator/emulator.go | sed -n '40,75p'Repository: onflow/flow-go
Length of output: 1513
🏁 Script executed:
#!/bin/bash
# Search for RestrictedEOAs declaration and all its usages
echo "=== RestrictedEOAs declaration and usages ==="
rg -n "RestrictedEOAs" --type=go | head -50Repository: onflow/flow-go
Length of output: 1225
🏁 Script executed:
#!/bin/bash
# Check if RestrictedEOAs is a package-level variable and how it's documented
echo "=== RestrictedEOAs package-level definition ==="
rg -B 3 -A 3 "var.*RestrictedEOAs|const.*RestrictedEOAs" --type=go -g '!*_test.go'Repository: onflow/flow-go
Length of output: 560
Consider dependency injection for RestrictedEOAs configuration.
The global RestrictedEOAs variable (hardcoded in config.go:32 with 2 malicious addresses) is wired into every emulator config via newConfig(). While this is intentional and the list is properly initialized, the design limits flexibility for different deployment scenarios and creates tight coupling. Consider accepting RestrictedEOAs as a parameter to the emulator or using a factory pattern to allow production deployments to override the list without code changes.
🤖 Prompt for AI Agents
In @fvm/evm/emulator/emulator.go at line 57, The code currently injects the
global RestrictedEOAs into every emulator config via newConfig() using
WithRestrictedEOAs, which couples the emulator to a hardcoded list; change
newConfig (or the emulator constructor) to accept RestrictedEOAs as an argument
(or accept a ConfigFactory/option that supplies the list) and thread that
parameter into the call site that applies WithRestrictedEOAs so callers can
provide a different slice at runtime; update places that construct the emulator
to pass the injected list (or factory) and remove reliance on the global
RestrictedEOAs variable.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Examine the commit to see how RestrictedEOAs handling was improved
git show 3cba2800255a216c5c37de16b900117f1fcbaa5c
Length of output: 1684
@m-Peter, thank you for the update! The commit improves test isolation by properly copying and restoring the RestrictedEOAs slice, which prevents test pollution—that's a good fix for the test code.
However, I wanted to clarify that my original suggestion was about the production code architecture. The commit addresses test cleanup, but the global RestrictedEOAs variable in config.go is still being directly referenced in newConfig() at line 57, which maintains the tight coupling I mentioned.
If you'd prefer to keep the current architecture with the global variable for simplicity, that's perfectly reasonable—the current implementation is functional and the restricted addresses are properly initialized. Just wanted to ensure we're on the same page about what was addressed. Let me know if you'd like to discuss the architecture further or if you're satisfied with the current approach!
🧬 Code Graph Analysis Results
[No user analysis performed. Providing relevant code snippets for context.]
- File: fvm/evm/emulator/state/stateDB.go — lines 45-57
func NewStateDB(ledger atree.Ledger, root flow.Address) (*StateDB, error) {
bv, err := NewBaseView(ledger, root)
if err != nil {
return nil, err
}
return &StateDB{
ledger: ledger,
root: root,
baseView: bv,
views: []*DeltaView{NewDeltaView(bv)},
cachedError: nil,
}, nil
}
- File: fvm/evm/emulator/state/stateDB.go — lines 34-40
StateDB struct {
ledger atree.Ledger
root flow.Address
baseView types.BaseView
views []*DeltaView
cachedError error
}
- File: fvm/evm/emulator/config.go — lines 39-65
Config struct {
// Chain Config
ChainConfig *gethParams.ChainConfig
// EVM config
EVMConfig gethVM.Config
// block context
BlockContext *gethVM.BlockContext
// transaction context
TxContext *gethVM.TxContext
// base unit of gas for direct calls
DirectCallBaseGasUsage uint64
// captures extra precompiled calls
PCTracker *CallTracker
// BlockTxCount captures the total number of
// transactions included in this block so far
BlockTxCountSoFar uint
// BlockTotalGasSoFar captures the total
// amount of gas used so far
BlockTotalGasUsedSoFar uint64
// PrecompiledContracts holds the applicable precompiled contracts
// for the current chain rules, as well as any extra precompiled
// contracts, such as Cadence Arch etc
PrecompiledContracts gethVM.PrecompiledContracts
// RestrictedEOAs holds a list of EOAs with restricted access to EVM,
// due to malicious activity
RestrictedEOAs []gethCommon.Address
}
- File: fvm/evm/types/balance.go — lines 105-107
func AttoFlowBalanceIsValidForFlowVault(bal *big.Int) bool {
return bal.Cmp(UFixToAttoConversionMultiplier) >= 0
}
- File: fvm/evm/types/balance.go — lines 110-115
func SubBalance(bal1 Balance, bal2 Balance) (Balance, error) {
if (*big.Int)(bal1).Cmp(bal2) == -1 {
return nil, ErrInvalidBalance
}
return new(big.Int).Sub(bal1, bal2), nil
}
- File: fvm/evm/types/balance.go — lines 118-120
func AddBalance(bal1 Balance, bal2 Balance) (Balance, error) {
return new(big.Int).Add(bal1, bal2), nil
}
- File: fvm/evm/types/result.go — lines 57-64
func NewInvalidResult(txType uint8, txHash gethCommon.Hash, err error) *Result {
return &Result{
TxType: txType,
TxHash: txHash,
ValidationError: err,
GasConsumed: InvalidTransactionGasCost,
}
}
- File: fvm/errors/errors.go — lines 37-39
func Is(err error, target error) bool {
return stdErrors.Is(err, target)
}
🧠 Learnings used
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
9c2ee73 to
90970dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @fvm/evm/evm_test.go:
- Around line 329-406: The test mutates the global emulator.RestrictedEOAs slice
while running with t.Parallel(), causing race conditions; instead, stop touching
the global slice and supply the restricted EOA via a per-test emulator config
(use the existing RunWithNewEnvironment setup to pass a WithRestrictedEOAs
option or equivalent) so the test creates its own emulator state containing
restrictedEOA.Address().ToCommon(); alternatively, if per-test emulator config
is not available, remove t.Parallel() from this test to avoid concurrent
mutation of emulator.RestrictedEOAs (references: emulator.RestrictedEOAs,
RunWithNewEnvironment, WithRestrictedEOAs, RestrictedEOATestAccount1KeyHex).
- Around line 1329-1420: The test "Batch run with with transactions from
restricted EOA" runs t.Parallel() but mutates the global emulator.RestrictedEOAs
slice (restrictedEOA appended then truncated), causing a race; modify the test
to avoid global mutation by using a test-local emulator/configuration or passing
a copy: instead of appending to emulator.RestrictedEOAs and deferring a
slice-popping cleanup, configure the VM/emulator used by this test to include
restrictedEOAs (or clone emulator.RestrictedEOAs into a local variable and set
it on the VM instance) so the test no longer mutates the shared emulator state
while running in parallel; update the code around GetTestEOAAccount,
emulator.RestrictedEOAs usage, and the deferred cleanup to operate on the
per-test emulator/config rather than the global slice.
🧹 Nitpick comments (1)
fvm/evm/emulator/emulator.go (1)
193-198: Consider improving test coverage for restricted EOA functionality.According to the Codecov report, this file has 37.50% patch coverage with 8 missing and 2 partial lines. The restricted EOA checks are security-critical features that should have comprehensive test coverage, including:
- Single transaction with restricted EOA
- Batch transaction with restricted EOA (at different positions in batch)
- Mixed batch with both restricted and non-restricted EOAs
- Case-insensitive address matching
- Error propagation and result handling
While tests exist in
restricted_eoa_test.goandevm_test.go, please verify that all code paths for restricted EOA handling are covered.Also applies to: 252-258
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/emulator/config.gofvm/evm/emulator/emulator.gofvm/evm/emulator/restricted_eoa_test.gofvm/evm/evm_test.gofvm/evm/stdlib/contract.cdcfvm/evm/testutils/accounts.gofvm/evm/types/errors.gofvm/evm/types/result.goutils/unittest/execution_state.go
🚧 Files skipped from review as they are similar to previous changes (3)
- engine/execution/state/bootstrap/bootstrap_test.go
- fvm/evm/stdlib/contract.cdc
- fvm/evm/emulator/config.go
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in @docs/agents/CodingConventions.md
Follow Go coding standards and conventions as documented in @docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
fvm/evm/emulator/restricted_eoa_test.gofvm/evm/types/errors.gofvm/evm/testutils/accounts.gofvm/evm/types/result.gofvm/evm/evm_test.gofvm/evm/emulator/emulator.goutils/unittest/execution_state.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Unit tests should be co-located with the code they test
Follow the existing pattern of*_test.gofiles for test naming
Use fixtures for realistic test data as defined in/utils/unittest/
Files:
fvm/evm/emulator/restricted_eoa_test.gofvm/evm/evm_test.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/evm/emulator/restricted_eoa_test.gofvm/evm/types/errors.gofvm/evm/testutils/accounts.gofvm/evm/types/result.gofvm/evm/evm_test.gofvm/evm/emulator/emulator.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
fvm/evm/emulator/restricted_eoa_test.gofvm/evm/types/errors.gofvm/evm/testutils/accounts.gofvm/evm/types/result.gofvm/evm/evm_test.gofvm/evm/emulator/emulator.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/evm/emulator/restricted_eoa_test.gofvm/evm/types/errors.gofvm/evm/testutils/accounts.gofvm/evm/types/result.gofvm/evm/evm_test.gofvm/evm/emulator/emulator.go
🧠 Learnings (3)
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,storage}/**/*.go : Security checks for cryptographic misuse must be enforced using gosec
Applied to files:
fvm/evm/emulator/restricted_eoa_test.go
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {storage,ledger,execution,fvm}/**/*.go : State consistency is paramount; use proper synchronization primitives
Applied to files:
fvm/evm/evm_test.goutils/unittest/execution_state.go
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Applied to files:
fvm/evm/emulator/emulator.go
🧬 Code graph analysis (4)
fvm/evm/emulator/restricted_eoa_test.go (2)
fvm/evm/emulator/config.go (3)
NewConfig(170-176)WithRestrictedEOAs(312-317)RestrictedEOAs(32-35)fvm/fvm.go (1)
Run(78-85)
fvm/evm/evm_test.go (3)
fvm/evm/testutils/accounts.go (2)
EOATestAccount(31-37)GetTestEOAAccount(116-126)fvm/evm/emulator/config.go (1)
RestrictedEOAs(32-35)fvm/evm/types/errors.go (1)
ErrRestrictedEOA(116-118)
fvm/evm/emulator/emulator.go (4)
fvm/evm/emulator/config.go (2)
WithRestrictedEOAs(312-317)RestrictedEOAs(32-35)fvm/evm/types/result.go (1)
NewInvalidResult(57-64)fvm/evm/types/errors.go (2)
ErrRestrictedEOA(116-118)ErrInvalidBalance(91-91)fvm/evm/types/balance.go (1)
AttoFlowBalanceIsValidForFlowVault(105-107)
utils/unittest/execution_state.go (1)
model/flow/chain.go (1)
Sandboxnet(26-26)
🪛 Gitleaks (8.30.0)
fvm/evm/testutils/accounts.go
[high] 25-25: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 28-28: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (37)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Lint (./)
- GitHub Check: Lint (./integration/)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (11)
utils/unittest/execution_state.go (1)
26-26: LGTM - Genesis state commitment constants updated.The updated genesis state commitment hex values align with the new state commitments referenced in the PR objectives and related bootstrap test changes.
Also applies to: 90-90, 95-95
fvm/evm/emulator/restricted_eoa_test.go (1)
11-86: LGTM - Comprehensive test coverage for restricted EOA detection.The test thoroughly validates the
IsRestrictedEOAfunctionality with multiple scenarios including known restricted addresses, non-restricted addresses, empty addresses, case sensitivity, and near-miss addresses. The test structure is clear and assertions are appropriate.fvm/evm/testutils/accounts.go (1)
23-29: LGTM - Test account key added for restricted EOA testing.The new constant follows the existing pattern for test account keys. The static analysis warning about a "Generic API Key" is a false positive—this is clearly a test-only private key for restricted EOA test scenarios, not a production secret.
fvm/evm/types/result.go (1)
57-63: All call sites have been properly updated to use the new signature. The signature change is complete across the codebase with 8 call sites infvm/evm/emulator/emulator.gocorrectly passingtxType(uint8) andtxHash(gethCommon.Hash) parameters along with the error. No outdated calls remain.fvm/evm/types/errors.go (1)
115-118: LGTM! Clear and actionable error message.The error message appropriately informs users about the restriction and provides a contact email for inquiries. The error is correctly defined as a plain validation error (not fatal or state error), consistent with other authorization/validation failures.
fvm/evm/emulator/emulator.go (6)
57-57: LGTM! Restricted EOA configuration properly integrated.The
RestrictedEOAsconstant from the config package is correctly applied to the emulator configuration during initialization.
193-198: LGTM! Restricted EOA check correctly implemented.The restricted EOA check is properly placed after message conversion to validate the actual sender address. Returning an invalid result with
ErrRestrictedEOAis the correct approach for this authorization failure.
252-258: LGTM! Batch transaction flow handles restricted EOAs correctly.The restricted EOA check is consistently implemented in the batch flow, correctly storing an invalid result and continuing to the next transaction without persisting any state changes.
397-401: LGTM! NewInvalidResult signature consistently updated.All DirectCall methods (
mintTo,withdrawFrom,deployAt) have been consistently updated to use the newNewInvalidResultsignature withcall.Typeandcall.Hash()parameters.Also applies to: 443-447, 452-456, 501-505
310-340: Verify whether DryRunTransaction should enforce restricted EOA checks.The
DryRunTransactionmethod doesn't check if thefromaddress is a restricted EOA. While dry runs don't persist state changes, allowing restricted EOAs to simulate transactions could:
- Enable them to test attack vectors without commitment
- Allow information gathering about contract states
- Verify transaction behavior before restrictions are lifted
Please confirm whether this omission is intentional. If restricted EOAs should not be able to dry run transactions, add the check after line 333:
🔒 Proposed fix to add restricted EOA check
// use the from as the signer msg.From = from + + if bl.config.IsRestrictedEOA(msg.From) { + return types.NewInvalidResult(tx.Type(), tx.Hash(), types.ErrRestrictedEOA), nil + } + // we need to skip nonce/transaction checks for dry run msg.SkipNonceChecks = true msg.SkipTransactionChecks = true
114-173: Remove this review comment – DirectCall does not need restricted EOA checks.DirectCall is not a user-facing API. It's created solely through internal system operations (COA deployment, deposits, withdrawals, transfers) in the handler's Account methods. Unlike
RunTransactionandBatchRunTransactions, which process user-signed transactions and extract the sender via signature verification, DirectCall uses system-controlled From addresses (bridge address, system addresses, internal accounts). The restricted EOA checks inRunTransactionandBatchRunTransactionsprotect against user-initiated calls; DirectCall does not process user input and therefore does not require these checks.Likely an incorrect or invalid review comment.
| t.Run("testing EVM.run (with restricted EOA)", func(t *testing.T) { | ||
| t.Parallel() | ||
| RunWithNewEnvironment(t, | ||
| chain, func( | ||
| ctx fvm.Context, | ||
| vm fvm.VM, | ||
| snapshot snapshot.SnapshotTree, | ||
| testContract *TestContract, | ||
| testAccount *EOATestAccount, | ||
| ) { | ||
| sc := systemcontracts.SystemContractsForChain(chain.ChainID()) | ||
| code := []byte(fmt.Sprintf( | ||
| ` | ||
| import EVM from %s | ||
| transaction(tx: [UInt8], coinbaseBytes: [UInt8; 20]){ | ||
| prepare(account: &Account) { | ||
| let coinbase = EVM.EVMAddress(bytes: coinbaseBytes) | ||
| let res = EVM.run(tx: tx, coinbase: coinbase) | ||
| assert(res.status == EVM.Status.successful, message: res.errorMessage) | ||
| } | ||
| } | ||
| `, | ||
| sc.EVMContract.Address.HexWithPrefix(), | ||
| )) | ||
|
|
||
| // This is only a test EOA, used during tests | ||
| // address: 0xad7cBF4b6edAd1A4Bc08Fa74741445918B3C54f4 | ||
| restrictedEOA := GetTestEOAAccount(t, RestrictedEOATestAccount1KeyHex) | ||
| emulator.RestrictedEOAs = append( | ||
| emulator.RestrictedEOAs, | ||
| restrictedEOA.Address().ToCommon(), | ||
| ) | ||
| defer func() { | ||
| emulator.RestrictedEOAs = emulator.RestrictedEOAs[:len(emulator.RestrictedEOAs)-1] | ||
| }() | ||
|
|
||
| num := int64(12) | ||
| innerTxBytes := restrictedEOA.PrepareSignAndEncodeTx(t, | ||
| testContract.DeployedAt.ToCommon(), | ||
| testContract.MakeCallData(t, "store", big.NewInt(num)), | ||
| big.NewInt(0), | ||
| uint64(100_000), | ||
| big.NewInt(0), | ||
| ) | ||
|
|
||
| innerTx := cadence.NewArray( | ||
| unittest.BytesToCdcUInt8(innerTxBytes), | ||
| ).WithType(stdlib.EVMTransactionBytesCadenceType) | ||
|
|
||
| coinbase := cadence.NewArray( | ||
| unittest.BytesToCdcUInt8(testAccount.Address().Bytes()), | ||
| ).WithType(stdlib.EVMAddressBytesCadenceType) | ||
|
|
||
| txBody, err := flow.NewTransactionBodyBuilder(). | ||
| SetScript(code). | ||
| SetPayer(sc.FlowServiceAccount.Address). | ||
| AddAuthorizer(sc.FlowServiceAccount.Address). | ||
| AddArgument(json.MustEncode(innerTx)). | ||
| AddArgument(json.MustEncode(coinbase)). | ||
| Build() | ||
| require.NoError(t, err) | ||
|
|
||
| tx := fvm.Transaction(txBody, 0) | ||
|
|
||
| _, output, err := vm.Run( | ||
| ctx, | ||
| tx, | ||
| snapshot, | ||
| ) | ||
| require.NoError(t, err) | ||
| require.Error(t, output.Err) | ||
| require.ErrorContains( | ||
| t, | ||
| output.Err, | ||
| types.ErrRestrictedEOA.Error(), | ||
| ) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch and display the commit changes to verify the fix
git show 3cba2800255a216c5c37de16b900117f1fcbaa5c --stat
echo "---"
git show 3cba2800255a216c5c37de16b900117f1fcbaa5c -- fvm/evm/evm_test.go | head -200
Length of output: 1955
@m-Peter Thank you for the improvement! The fix is better than before—copying the original slice and restoring it completely is more robust than just slicing off the last element.
However, there's still a race condition with t.Parallel() active:
The Issue:
- Both tests still modify the global
emulator.RestrictedEOAsvariable while running concurrently - When multiple parallel tests execute, they can simultaneously:
- Read
emulator.RestrictedEOAsto make a copy - Append to
emulator.RestrictedEOAs - Restore
emulator.RestrictedEOAsin the defer
- Read
- This creates a data race that Go's race detector would flag
Example scenario:
- Test A reads and copies
emulator.RestrictedEOAs(length 0) - Test B reads and copies
emulator.RestrictedEOAs(length 0) - Test A appends its EOA (length 1)
- Test B appends its EOA (length 2)
- Test A's defer restores to length 0
- Test B is still running with a corrupted slice
Recommended solutions:
-
Remove
t.Parallel()from these two tests (simplest fix):t.Run("testing EVM.run (with restricted EOA)", func(t *testing.T) { // Remove: t.Parallel() RunWithNewEnvironment(t, ...
-
Or add synchronization around all
emulator.RestrictedEOAsaccess (if you need parallelism)
The current fix improves cleanup reliability, but won't prevent race conditions when running with -race flag. Would you like me to prepare a suggestion for either approach?
🧬 Code Graph Analysis Results
[file: fvm/evm/impl/impl.go, lines 1093-1181]
func ResultSummaryFromEVMResultValue(val cadence.Value) (*types.ResultSummary, error) {
str, ok := val.(cadence.Struct)
if !ok {
return nil, fmt.Errorf("invalid input: unexpected value type")
}
fields := cadence.FieldsMappedByName(str)
const expectedFieldCount = 6
if len(fields) != expectedFieldCount {
return nil, fmt.Errorf(
"invalid input: field count mismatch: expected %d, got %d",
expectedFieldCount,
len(fields),
)
}
statusEnum, ok := fields[stdlib.EVMResultTypeStatusFieldName].(cadence.Enum)
if !ok {
return nil, fmt.Errorf("invalid input: unexpected type for status field")
}
status, ok := cadence.FieldsMappedByName(statusEnum)[sema.EnumRawValueFieldName].(cadence.UInt8)
if !ok {
return nil, fmt.Errorf("invalid input: unexpected type for status field")
}
errorCode, ok := fields[stdlib.EVMResultTypeErrorCodeFieldName].(cadence.UInt64)
if !ok {
return nil, fmt.Errorf("invalid input: unexpected type for error code field")
}
errorMsg, ok := fields[stdlib.EVMResultTypeErrorMessageFieldName].(cadence.String)
if !ok {
return nil, fmt.Errorf("invalid input: unexpected type for error msg field")
}
gasUsed, ok := fields[stdlib.EVMResultTypeGasUsedFieldName].(cadence.UInt64)
if !ok {
return nil, fmt.Errorf("invalid input: unexpected type for gas field")
}
data, ok := fields[stdlib.EVMResultTypeDataFieldName].(cadence.Array)
if !ok {
return nil, fmt.Errorf("invalid input: unexpected type for data field")
}
convertedData := make([]byte, len(data.Values))
for i, value := range data.Values {
convertedData[i] = byte(value.(cadence.UInt8))
}
var convertedDeployedAddress *types.Address
deployedAddressField, ok := fields[stdlib.EVMResultTypeDeployedContractFieldName].(cadence.Optional)
if !ok {
return nil, fmt.Errorf("invalid input: unexpected type for deployed contract field")
}
if deployedAddressField.Value != nil {
evmAddress, ok := deployedAddressField.Value.(cadence.Struct)
if !ok {
return nil, fmt.Errorf("invalid input: unexpected type for deployed contract field")
}
bytes, ok := cadence.SearchFieldByName(evmAddress, stdlib.EVMAddressTypeBytesFieldName).(cadence.Array)
if !ok {
return nil, fmt.Errorf("invalid input: unexpected type for deployed contract field")
}
convertedAddress := make([]byte, len(bytes.Values))
for i, value := range bytes.Values {
convertedAddress[i] = byte(value.(cadence.UInt8))
}
addr := types.Address(convertedAddress)
convertedDeployedAddress = &addr
}
return &types.ResultSummary{
Status: types.Status(status),
ErrorCode: types.ErrorCode(errorCode),
ErrorMessage: string(errorMsg),
GasConsumed: uint64(gasUsed),
MaxGasConsumed: uint64(gasUsed),
ReturnedData: convertedData,
DeployedContractAddress: convertedDeployedAddress,
}, nil
}[file: fvm/evm/types/result.go, lines 45-53]
type ResultSummary struct {
Status Status
ErrorCode ErrorCode
ErrorMessage string
GasConsumed uint64
MaxGasConsumed uint64
DeployedContractAddress *Address
ReturnedData Data
}[file: fvm/evm/types/errors.go, lines 12-12]
const (
ErrCodeNoError ErrorCode = 0
)🧠 Learnings used
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {storage,ledger,execution,fvm}/**/*.go : State consistency is paramount; use proper synchronization primitives
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
| t.Run("Batch run with with transactions from restricted EOA", func(t *testing.T) { | ||
| t.Parallel() | ||
| RunWithNewEnvironment(t, | ||
| chain, func( | ||
| ctx fvm.Context, | ||
| vm fvm.VM, | ||
| snapshot snapshot.SnapshotTree, | ||
| testContract *TestContract, | ||
| testAccount *EOATestAccount, | ||
| ) { | ||
| sc := systemcontracts.SystemContractsForChain(chain.ChainID()) | ||
| batchRunCode := []byte(fmt.Sprintf( | ||
| ` | ||
| import EVM from %s | ||
| transaction(txs: [[UInt8]], coinbaseBytes: [UInt8; 20]) { | ||
| execute { | ||
| let coinbase = EVM.EVMAddress(bytes: coinbaseBytes) | ||
| let batchResults = EVM.batchRun(txs: txs, coinbase: coinbase) | ||
| log("results") | ||
| log(batchResults) | ||
| assert(batchResults.length == txs.length, message: "invalid result length") | ||
| for i, res in batchResults { | ||
| assert(res.status == EVM.Status.successful, message: res.errorMessage) | ||
| } | ||
| } | ||
| } | ||
| `, | ||
| sc.EVMContract.Address.HexWithPrefix(), | ||
| )) | ||
|
|
||
| // This is only a test EOA, used during tests | ||
| // address: 0xad7cBF4b6edAd1A4Bc08Fa74741445918B3C54f4 | ||
| restrictedEOA := GetTestEOAAccount(t, RestrictedEOATestAccount1KeyHex) | ||
| emulator.RestrictedEOAs = append( | ||
| emulator.RestrictedEOAs, | ||
| restrictedEOA.Address().ToCommon(), | ||
| ) | ||
| defer func() { | ||
| emulator.RestrictedEOAs = emulator.RestrictedEOAs[:len(emulator.RestrictedEOAs)-1] | ||
| }() | ||
|
|
||
| batchCount := 6 | ||
| var num int64 | ||
| txBytes := make([]cadence.Value, batchCount) | ||
| for i := 0; i < batchCount; i++ { | ||
| num = int64(i) | ||
|
|
||
| // prepare batch of transaction payloads | ||
| tx := restrictedEOA.PrepareSignAndEncodeTx(t, | ||
| testContract.DeployedAt.ToCommon(), | ||
| testContract.MakeCallData(t, "store", big.NewInt(num)), | ||
| big.NewInt(0), | ||
| 100_000, | ||
| big.NewInt(0), | ||
| ) | ||
|
|
||
| // build txs argument | ||
| txBytes[i] = cadence.NewArray( | ||
| unittest.BytesToCdcUInt8(tx), | ||
| ).WithType(stdlib.EVMTransactionBytesCadenceType) | ||
| } | ||
|
|
||
| coinbase := cadence.NewArray( | ||
| unittest.BytesToCdcUInt8(testAccount.Address().Bytes()), | ||
| ).WithType(stdlib.EVMAddressBytesCadenceType) | ||
|
|
||
| txs := cadence.NewArray(txBytes). | ||
| WithType(cadence.NewVariableSizedArrayType( | ||
| stdlib.EVMTransactionBytesCadenceType, | ||
| )) | ||
|
|
||
| txBody, err := flow.NewTransactionBodyBuilder(). | ||
| SetScript(batchRunCode). | ||
| SetPayer(sc.FlowServiceAccount.Address). | ||
| AddArgument(json.MustEncode(txs)). | ||
| AddArgument(json.MustEncode(coinbase)). | ||
| Build() | ||
| require.NoError(t, err) | ||
|
|
||
| tx := fvm.Transaction(txBody, 0) | ||
|
|
||
| _, output, err := vm.Run(ctx, tx, snapshot) | ||
|
|
||
| require.NoError(t, err) | ||
| require.Error(t, output.Err) | ||
| require.ErrorContains( | ||
| t, | ||
| output.Err, | ||
| types.ErrRestrictedEOA.Error(), | ||
| ) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the specific commit to see what changes were made
git show 3cba2800255a216c5c37de16b900117f1fcbaa5c --stat
Length of output: 266
🏁 Script executed:
#!/bin/bash
# Get the detailed diff for the commit, focusing on the test file
git show 3cba2800255a216c5c37de16b900117f1fcbaa5c -- fvm/evm/evm_test.go | head -200
Length of output: 1684
@m-Peter, thanks for the update! I can see the improvement in commit 3cba280. The cleanup logic is now more robust—copying the original state and restoring it in the defer ensures the exact original state is preserved.
However, there's still a race condition present. While the cleanup is better, the tests still modify the global emulator.RestrictedEOAs variable while running with t.Parallel(). This means:
- Test A copies
emulator.RestrictedEOAs - Test B copies
emulator.RestrictedEOAs(potentially same time) - Test A appends to the global slice
- Test B appends to the global slice (overwrites Test A's changes)
- Test executions see inconsistent state
- Cleanup happens in unpredictable order
The core issue is that multiple goroutines are reading and writing a shared global variable without synchronization.
To fully resolve this, you could either:
- Remove
t.Parallel()from these two tests, OR - Refactor to use per-test emulator configuration that doesn't rely on global state (e.g., pass
RestrictedEOAsas a parameter to the emulator/VM)
Would you like to pursue one of these approaches to eliminate the race condition entirely?
🧬 Code Graph Analysis Results
<no_relevant_code_snippets>
🧠 Learnings used
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {storage,ledger,execution,fvm}/**/*.go : State consistency is paramount; use proper synchronization primitives
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
| t.Run("testing EVM.run (with restricted EOA)", func(t *testing.T) { | ||
| t.Parallel() | ||
| RunWithNewEnvironment(t, | ||
| chain, func( | ||
| ctx fvm.Context, | ||
| vm fvm.VM, | ||
| snapshot snapshot.SnapshotTree, | ||
| testContract *TestContract, | ||
| testAccount *EOATestAccount, | ||
| ) { | ||
| sc := systemcontracts.SystemContractsForChain(chain.ChainID()) | ||
| code := []byte(fmt.Sprintf( | ||
| ` | ||
| import EVM from %s | ||
| transaction(tx: [UInt8], coinbaseBytes: [UInt8; 20]){ | ||
| prepare(account: &Account) { | ||
| let coinbase = EVM.EVMAddress(bytes: coinbaseBytes) | ||
| let res = EVM.run(tx: tx, coinbase: coinbase) | ||
| assert(res.status == EVM.Status.successful, message: res.errorMessage) | ||
| } | ||
| } | ||
| `, | ||
| sc.EVMContract.Address.HexWithPrefix(), | ||
| )) | ||
|
|
||
| // This is only a test EOA, used during tests | ||
| // address: 0xad7cBF4b6edAd1A4Bc08Fa74741445918B3C54f4 | ||
| restrictedEOA := GetTestEOAAccount(t, RestrictedEOATestAccount1KeyHex) | ||
| emulator.RestrictedEOAs = append( | ||
| emulator.RestrictedEOAs, | ||
| restrictedEOA.Address().ToCommon(), | ||
| ) | ||
| defer func() { | ||
| emulator.RestrictedEOAs = emulator.RestrictedEOAs[:len(emulator.RestrictedEOAs)-1] | ||
| }() | ||
|
|
||
| num := int64(12) | ||
| innerTxBytes := restrictedEOA.PrepareSignAndEncodeTx(t, | ||
| testContract.DeployedAt.ToCommon(), | ||
| testContract.MakeCallData(t, "store", big.NewInt(num)), | ||
| big.NewInt(0), | ||
| uint64(100_000), | ||
| big.NewInt(0), | ||
| ) | ||
|
|
||
| innerTx := cadence.NewArray( | ||
| unittest.BytesToCdcUInt8(innerTxBytes), | ||
| ).WithType(stdlib.EVMTransactionBytesCadenceType) | ||
|
|
||
| coinbase := cadence.NewArray( | ||
| unittest.BytesToCdcUInt8(testAccount.Address().Bytes()), | ||
| ).WithType(stdlib.EVMAddressBytesCadenceType) | ||
|
|
||
| txBody, err := flow.NewTransactionBodyBuilder(). | ||
| SetScript(code). | ||
| SetPayer(sc.FlowServiceAccount.Address). | ||
| AddAuthorizer(sc.FlowServiceAccount.Address). | ||
| AddArgument(json.MustEncode(innerTx)). | ||
| AddArgument(json.MustEncode(coinbase)). | ||
| Build() | ||
| require.NoError(t, err) | ||
|
|
||
| tx := fvm.Transaction(txBody, 0) | ||
|
|
||
| _, output, err := vm.Run( | ||
| ctx, | ||
| tx, | ||
| snapshot, | ||
| ) | ||
| require.NoError(t, err) | ||
| require.Error(t, output.Err) | ||
| require.ErrorContains( | ||
| t, | ||
| output.Err, | ||
| types.ErrRestrictedEOA.Error(), | ||
| ) | ||
| }) | ||
| }) |
| t.Run("Batch run with with transactions from restricted EOA", func(t *testing.T) { | ||
| t.Parallel() | ||
| RunWithNewEnvironment(t, | ||
| chain, func( | ||
| ctx fvm.Context, | ||
| vm fvm.VM, | ||
| snapshot snapshot.SnapshotTree, | ||
| testContract *TestContract, | ||
| testAccount *EOATestAccount, | ||
| ) { | ||
| sc := systemcontracts.SystemContractsForChain(chain.ChainID()) | ||
| batchRunCode := []byte(fmt.Sprintf( | ||
| ` | ||
| import EVM from %s | ||
| transaction(txs: [[UInt8]], coinbaseBytes: [UInt8; 20]) { | ||
| execute { | ||
| let coinbase = EVM.EVMAddress(bytes: coinbaseBytes) | ||
| let batchResults = EVM.batchRun(txs: txs, coinbase: coinbase) | ||
| log("results") | ||
| log(batchResults) | ||
| assert(batchResults.length == txs.length, message: "invalid result length") | ||
| for i, res in batchResults { | ||
| assert(res.status == EVM.Status.successful, message: res.errorMessage) | ||
| } | ||
| } | ||
| } | ||
| `, | ||
| sc.EVMContract.Address.HexWithPrefix(), | ||
| )) | ||
|
|
||
| // This is only a test EOA, used during tests | ||
| // address: 0xad7cBF4b6edAd1A4Bc08Fa74741445918B3C54f4 | ||
| restrictedEOA := GetTestEOAAccount(t, RestrictedEOATestAccount1KeyHex) | ||
| emulator.RestrictedEOAs = append( | ||
| emulator.RestrictedEOAs, | ||
| restrictedEOA.Address().ToCommon(), | ||
| ) | ||
| defer func() { | ||
| emulator.RestrictedEOAs = emulator.RestrictedEOAs[:len(emulator.RestrictedEOAs)-1] | ||
| }() | ||
|
|
||
| batchCount := 6 | ||
| var num int64 | ||
| txBytes := make([]cadence.Value, batchCount) | ||
| for i := 0; i < batchCount; i++ { | ||
| num = int64(i) | ||
|
|
||
| // prepare batch of transaction payloads | ||
| tx := restrictedEOA.PrepareSignAndEncodeTx(t, | ||
| testContract.DeployedAt.ToCommon(), | ||
| testContract.MakeCallData(t, "store", big.NewInt(num)), | ||
| big.NewInt(0), | ||
| 100_000, | ||
| big.NewInt(0), | ||
| ) | ||
|
|
||
| // build txs argument | ||
| txBytes[i] = cadence.NewArray( | ||
| unittest.BytesToCdcUInt8(tx), | ||
| ).WithType(stdlib.EVMTransactionBytesCadenceType) | ||
| } | ||
|
|
||
| coinbase := cadence.NewArray( | ||
| unittest.BytesToCdcUInt8(testAccount.Address().Bytes()), | ||
| ).WithType(stdlib.EVMAddressBytesCadenceType) | ||
|
|
||
| txs := cadence.NewArray(txBytes). | ||
| WithType(cadence.NewVariableSizedArrayType( | ||
| stdlib.EVMTransactionBytesCadenceType, | ||
| )) | ||
|
|
||
| txBody, err := flow.NewTransactionBodyBuilder(). | ||
| SetScript(batchRunCode). | ||
| SetPayer(sc.FlowServiceAccount.Address). | ||
| AddArgument(json.MustEncode(txs)). | ||
| AddArgument(json.MustEncode(coinbase)). | ||
| Build() | ||
| require.NoError(t, err) | ||
|
|
||
| tx := fvm.Transaction(txBody, 0) | ||
|
|
||
| _, output, err := vm.Run(ctx, tx, snapshot) | ||
|
|
||
| require.NoError(t, err) | ||
| require.Error(t, output.Err) | ||
| require.ErrorContains( | ||
| t, | ||
| output.Err, | ||
| types.ErrRestrictedEOA.Error(), | ||
| ) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @fvm/evm/evm_test.go:
- Around line 329-408: The test starts parallel execution (t.Parallel()) but
mutates the shared global emulator.RestrictedEOAs slice, causing data races; fix
by removing the t.Parallel() call in the "testing EVM.run (with restricted EOA)"
test or, if parallelism is required, protect access to emulator.RestrictedEOAs
by introducing synchronization (e.g., a package-level mutex used around the
copy/append and defer-restore sequence) or refactor to inject a test-local
RestrictedEOAs configuration into the VM so no global mutation is needed; locate
the change around the test block that calls t.Parallel() and where
emulator.RestrictedEOAs is read/modified.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fvm/evm/evm_test.go
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in @docs/agents/CodingConventions.md
Follow Go coding standards and conventions as documented in @docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
fvm/evm/evm_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Unit tests should be co-located with the code they test
Follow the existing pattern of*_test.gofiles for test naming
Use fixtures for realistic test data as defined in/utils/unittest/
Files:
fvm/evm/evm_test.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/evm/evm_test.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
fvm/evm/evm_test.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/evm/evm_test.go
🧠 Learnings (2)
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {storage,ledger,execution,fvm}/**/*.go : State consistency is paramount; use proper synchronization primitives
Applied to files:
fvm/evm/evm_test.go
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Applied to files:
fvm/evm/evm_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (37)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Lint (./integration/)
- GitHub Check: Lint (./)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (state)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (1)
fvm/evm/evm_test.go (1)
28-28: LGTM: Import added for restricted EOA functionality.The emulator package import is necessary for accessing
RestrictedEOAsin the new test cases.
| t.Run("testing EVM.run (with restricted EOA)", func(t *testing.T) { | ||
| t.Parallel() | ||
| RunWithNewEnvironment(t, | ||
| chain, func( | ||
| ctx fvm.Context, | ||
| vm fvm.VM, | ||
| snapshot snapshot.SnapshotTree, | ||
| testContract *TestContract, | ||
| testAccount *EOATestAccount, | ||
| ) { | ||
| sc := systemcontracts.SystemContractsForChain(chain.ChainID()) | ||
| code := []byte(fmt.Sprintf( | ||
| ` | ||
| import EVM from %s | ||
| transaction(tx: [UInt8], coinbaseBytes: [UInt8; 20]){ | ||
| prepare(account: &Account) { | ||
| let coinbase = EVM.EVMAddress(bytes: coinbaseBytes) | ||
| let res = EVM.run(tx: tx, coinbase: coinbase) | ||
| assert(res.status == EVM.Status.successful, message: res.errorMessage) | ||
| } | ||
| } | ||
| `, | ||
| sc.EVMContract.Address.HexWithPrefix(), | ||
| )) | ||
|
|
||
| // This is only a test EOA, used during tests | ||
| // address: 0xad7cBF4b6edAd1A4Bc08Fa74741445918B3C54f4 | ||
| restrictedEOA := GetTestEOAAccount(t, RestrictedEOATestAccount1KeyHex) | ||
| restrictedEOAs := make([]common.Address, len(emulator.RestrictedEOAs)) | ||
| copy(restrictedEOAs, emulator.RestrictedEOAs) | ||
| emulator.RestrictedEOAs = append( | ||
| emulator.RestrictedEOAs, | ||
| restrictedEOA.Address().ToCommon(), | ||
| ) | ||
| defer func() { | ||
| emulator.RestrictedEOAs = restrictedEOAs | ||
| }() | ||
|
|
||
| num := int64(12) | ||
| innerTxBytes := restrictedEOA.PrepareSignAndEncodeTx(t, | ||
| testContract.DeployedAt.ToCommon(), | ||
| testContract.MakeCallData(t, "store", big.NewInt(num)), | ||
| big.NewInt(0), | ||
| uint64(100_000), | ||
| big.NewInt(0), | ||
| ) | ||
|
|
||
| innerTx := cadence.NewArray( | ||
| unittest.BytesToCdcUInt8(innerTxBytes), | ||
| ).WithType(stdlib.EVMTransactionBytesCadenceType) | ||
|
|
||
| coinbase := cadence.NewArray( | ||
| unittest.BytesToCdcUInt8(testAccount.Address().Bytes()), | ||
| ).WithType(stdlib.EVMAddressBytesCadenceType) | ||
|
|
||
| txBody, err := flow.NewTransactionBodyBuilder(). | ||
| SetScript(code). | ||
| SetPayer(sc.FlowServiceAccount.Address). | ||
| AddAuthorizer(sc.FlowServiceAccount.Address). | ||
| AddArgument(json.MustEncode(innerTx)). | ||
| AddArgument(json.MustEncode(coinbase)). | ||
| Build() | ||
| require.NoError(t, err) | ||
|
|
||
| tx := fvm.Transaction(txBody, 0) | ||
|
|
||
| _, output, err := vm.Run( | ||
| ctx, | ||
| tx, | ||
| snapshot, | ||
| ) | ||
| require.NoError(t, err) | ||
| require.Error(t, output.Err) | ||
| require.ErrorContains( | ||
| t, | ||
| output.Err, | ||
| types.ErrRestrictedEOA.Error(), | ||
| ) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Critical: Data race on shared global state in parallel test.
This test uses t.Parallel() while modifying the global emulator.RestrictedEOAs slice without synchronization. When multiple tests run concurrently, they will race on reading/modifying this shared state, leading to:
- Non-deterministic test failures
- Incorrect test isolation
- Lost updates during concurrent modifications
Even though defer restores the original slice, the backup-modify-restore pattern is not thread-safe when tests run in parallel.
Based on learnings, state consistency requires proper synchronization primitives for fvm/**/*.go files.
🔒 Recommended fixes
Option 1 (Preferred): Remove parallel execution for this test
- t.Run("testing EVM.run (with restricted EOA)", func(t *testing.T) {
- t.Parallel()
+ t.Run("testing EVM.run (with restricted EOA)", func(t *testing.T) {
+ // Cannot run in parallel due to modification of global emulator.RestrictedEOAs
RunWithNewEnvironment(t,Option 2: Add synchronization to emulator.RestrictedEOAs
If parallel execution is required, the emulator.RestrictedEOAs should be protected with a mutex or use a test-specific configuration mechanism that doesn't rely on global state.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.Run("testing EVM.run (with restricted EOA)", func(t *testing.T) { | |
| t.Parallel() | |
| RunWithNewEnvironment(t, | |
| chain, func( | |
| ctx fvm.Context, | |
| vm fvm.VM, | |
| snapshot snapshot.SnapshotTree, | |
| testContract *TestContract, | |
| testAccount *EOATestAccount, | |
| ) { | |
| sc := systemcontracts.SystemContractsForChain(chain.ChainID()) | |
| code := []byte(fmt.Sprintf( | |
| ` | |
| import EVM from %s | |
| transaction(tx: [UInt8], coinbaseBytes: [UInt8; 20]){ | |
| prepare(account: &Account) { | |
| let coinbase = EVM.EVMAddress(bytes: coinbaseBytes) | |
| let res = EVM.run(tx: tx, coinbase: coinbase) | |
| assert(res.status == EVM.Status.successful, message: res.errorMessage) | |
| } | |
| } | |
| `, | |
| sc.EVMContract.Address.HexWithPrefix(), | |
| )) | |
| // This is only a test EOA, used during tests | |
| // address: 0xad7cBF4b6edAd1A4Bc08Fa74741445918B3C54f4 | |
| restrictedEOA := GetTestEOAAccount(t, RestrictedEOATestAccount1KeyHex) | |
| restrictedEOAs := make([]common.Address, len(emulator.RestrictedEOAs)) | |
| copy(restrictedEOAs, emulator.RestrictedEOAs) | |
| emulator.RestrictedEOAs = append( | |
| emulator.RestrictedEOAs, | |
| restrictedEOA.Address().ToCommon(), | |
| ) | |
| defer func() { | |
| emulator.RestrictedEOAs = restrictedEOAs | |
| }() | |
| num := int64(12) | |
| innerTxBytes := restrictedEOA.PrepareSignAndEncodeTx(t, | |
| testContract.DeployedAt.ToCommon(), | |
| testContract.MakeCallData(t, "store", big.NewInt(num)), | |
| big.NewInt(0), | |
| uint64(100_000), | |
| big.NewInt(0), | |
| ) | |
| innerTx := cadence.NewArray( | |
| unittest.BytesToCdcUInt8(innerTxBytes), | |
| ).WithType(stdlib.EVMTransactionBytesCadenceType) | |
| coinbase := cadence.NewArray( | |
| unittest.BytesToCdcUInt8(testAccount.Address().Bytes()), | |
| ).WithType(stdlib.EVMAddressBytesCadenceType) | |
| txBody, err := flow.NewTransactionBodyBuilder(). | |
| SetScript(code). | |
| SetPayer(sc.FlowServiceAccount.Address). | |
| AddAuthorizer(sc.FlowServiceAccount.Address). | |
| AddArgument(json.MustEncode(innerTx)). | |
| AddArgument(json.MustEncode(coinbase)). | |
| Build() | |
| require.NoError(t, err) | |
| tx := fvm.Transaction(txBody, 0) | |
| _, output, err := vm.Run( | |
| ctx, | |
| tx, | |
| snapshot, | |
| ) | |
| require.NoError(t, err) | |
| require.Error(t, output.Err) | |
| require.ErrorContains( | |
| t, | |
| output.Err, | |
| types.ErrRestrictedEOA.Error(), | |
| ) | |
| }) | |
| }) | |
| t.Run("testing EVM.run (with restricted EOA)", func(t *testing.T) { | |
| // Cannot run in parallel due to modification of global emulator.RestrictedEOAs | |
| RunWithNewEnvironment(t, | |
| chain, func( | |
| ctx fvm.Context, | |
| vm fvm.VM, | |
| snapshot snapshot.SnapshotTree, | |
| testContract *TestContract, | |
| testAccount *EOATestAccount, | |
| ) { | |
| sc := systemcontracts.SystemContractsForChain(chain.ChainID()) | |
| code := []byte(fmt.Sprintf( | |
| ` | |
| import EVM from %s | |
| transaction(tx: [UInt8], coinbaseBytes: [UInt8; 20]){ | |
| prepare(account: &Account) { | |
| let coinbase = EVM.EVMAddress(bytes: coinbaseBytes) | |
| let res = EVM.run(tx: tx, coinbase: coinbase) | |
| assert(res.status == EVM.Status.successful, message: res.errorMessage) | |
| } | |
| } | |
| `, | |
| sc.EVMContract.Address.HexWithPrefix(), | |
| )) | |
| // This is only a test EOA, used during tests | |
| // address: 0xad7cBF4b6edAd1A4Bc08Fa74741445918B3C54f4 | |
| restrictedEOA := GetTestEOAAccount(t, RestrictedEOATestAccount1KeyHex) | |
| restrictedEOAs := make([]common.Address, len(emulator.RestrictedEOAs)) | |
| copy(restrictedEOAs, emulator.RestrictedEOAs) | |
| emulator.RestrictedEOAs = append( | |
| emulator.RestrictedEOAs, | |
| restrictedEOA.Address().ToCommon(), | |
| ) | |
| defer func() { | |
| emulator.RestrictedEOAs = restrictedEOAs | |
| }() | |
| num := int64(12) | |
| innerTxBytes := restrictedEOA.PrepareSignAndEncodeTx(t, | |
| testContract.DeployedAt.ToCommon(), | |
| testContract.MakeCallData(t, "store", big.NewInt(num)), | |
| big.NewInt(0), | |
| uint64(100_000), | |
| big.NewInt(0), | |
| ) | |
| innerTx := cadence.NewArray( | |
| unittest.BytesToCdcUInt8(innerTxBytes), | |
| ).WithType(stdlib.EVMTransactionBytesCadenceType) | |
| coinbase := cadence.NewArray( | |
| unittest.BytesToCdcUInt8(testAccount.Address().Bytes()), | |
| ).WithType(stdlib.EVMAddressBytesCadenceType) | |
| txBody, err := flow.NewTransactionBodyBuilder(). | |
| SetScript(code). | |
| SetPayer(sc.FlowServiceAccount.Address). | |
| AddAuthorizer(sc.FlowServiceAccount.Address). | |
| AddArgument(json.MustEncode(innerTx)). | |
| AddArgument(json.MustEncode(coinbase)). | |
| Build() | |
| require.NoError(t, err) | |
| tx := fvm.Transaction(txBody, 0) | |
| _, output, err := vm.Run( | |
| ctx, | |
| tx, | |
| snapshot, | |
| ) | |
| require.NoError(t, err) | |
| require.Error(t, output.Err) | |
| require.ErrorContains( | |
| t, | |
| output.Err, | |
| types.ErrRestrictedEOA.Error(), | |
| ) | |
| }) | |
| }) |
🤖 Prompt for AI Agents
In @fvm/evm/evm_test.go around lines 329 - 408, The test starts parallel
execution (t.Parallel()) but mutates the shared global emulator.RestrictedEOAs
slice, causing data races; fix by removing the t.Parallel() call in the "testing
EVM.run (with restricted EOA)" test or, if parallelism is required, protect
access to emulator.RestrictedEOAs by introducing synchronization (e.g., a
package-level mutex used around the copy/append and defer-restore sequence) or
refactor to inject a test-local RestrictedEOAs configuration into the VM so no
global mutation is needed; locate the change around the test block that calls
t.Parallel() and where emulator.RestrictedEOAs is read/modified.
| t.Run("Batch run with with transactions from restricted EOA", func(t *testing.T) { | ||
| t.Parallel() | ||
| RunWithNewEnvironment(t, | ||
| chain, func( | ||
| ctx fvm.Context, | ||
| vm fvm.VM, | ||
| snapshot snapshot.SnapshotTree, | ||
| testContract *TestContract, | ||
| testAccount *EOATestAccount, | ||
| ) { | ||
| sc := systemcontracts.SystemContractsForChain(chain.ChainID()) | ||
| batchRunCode := []byte(fmt.Sprintf( | ||
| ` | ||
| import EVM from %s | ||
| transaction(txs: [[UInt8]], coinbaseBytes: [UInt8; 20]) { | ||
| execute { | ||
| let coinbase = EVM.EVMAddress(bytes: coinbaseBytes) | ||
| let batchResults = EVM.batchRun(txs: txs, coinbase: coinbase) | ||
| log("results") | ||
| log(batchResults) | ||
| assert(batchResults.length == txs.length, message: "invalid result length") | ||
| for i, res in batchResults { | ||
| assert(res.status == EVM.Status.successful, message: res.errorMessage) | ||
| } | ||
| } | ||
| } | ||
| `, | ||
| sc.EVMContract.Address.HexWithPrefix(), | ||
| )) | ||
|
|
||
| // This is only a test EOA, used during tests | ||
| // address: 0xad7cBF4b6edAd1A4Bc08Fa74741445918B3C54f4 | ||
| restrictedEOA := GetTestEOAAccount(t, RestrictedEOATestAccount1KeyHex) | ||
| restrictedEOAs := make([]common.Address, len(emulator.RestrictedEOAs)) | ||
| copy(restrictedEOAs, emulator.RestrictedEOAs) | ||
| emulator.RestrictedEOAs = append( | ||
| emulator.RestrictedEOAs, | ||
| restrictedEOA.Address().ToCommon(), | ||
| ) | ||
| defer func() { | ||
| emulator.RestrictedEOAs = restrictedEOAs | ||
| }() | ||
|
|
||
| batchCount := 6 | ||
| var num int64 | ||
| txBytes := make([]cadence.Value, batchCount) | ||
| for i := 0; i < batchCount; i++ { | ||
| num = int64(i) | ||
|
|
||
| // prepare batch of transaction payloads | ||
| tx := restrictedEOA.PrepareSignAndEncodeTx(t, | ||
| testContract.DeployedAt.ToCommon(), | ||
| testContract.MakeCallData(t, "store", big.NewInt(num)), | ||
| big.NewInt(0), | ||
| 100_000, | ||
| big.NewInt(0), | ||
| ) | ||
|
|
||
| // build txs argument | ||
| txBytes[i] = cadence.NewArray( | ||
| unittest.BytesToCdcUInt8(tx), | ||
| ).WithType(stdlib.EVMTransactionBytesCadenceType) | ||
| } | ||
|
|
||
| coinbase := cadence.NewArray( | ||
| unittest.BytesToCdcUInt8(testAccount.Address().Bytes()), | ||
| ).WithType(stdlib.EVMAddressBytesCadenceType) | ||
|
|
||
| txs := cadence.NewArray(txBytes). | ||
| WithType(cadence.NewVariableSizedArrayType( | ||
| stdlib.EVMTransactionBytesCadenceType, | ||
| )) | ||
|
|
||
| txBody, err := flow.NewTransactionBodyBuilder(). | ||
| SetScript(batchRunCode). | ||
| SetPayer(sc.FlowServiceAccount.Address). | ||
| AddArgument(json.MustEncode(txs)). | ||
| AddArgument(json.MustEncode(coinbase)). | ||
| Build() | ||
| require.NoError(t, err) | ||
|
|
||
| tx := fvm.Transaction(txBody, 0) | ||
|
|
||
| _, output, err := vm.Run(ctx, tx, snapshot) | ||
|
|
||
| require.NoError(t, err) | ||
| require.Error(t, output.Err) | ||
| require.ErrorContains( | ||
| t, | ||
| output.Err, | ||
| types.ErrRestrictedEOA.Error(), | ||
| ) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Critical: Data race on shared global state in parallel test.
This test has the same concurrency issue as the first restricted EOA test: it uses t.Parallel() while modifying the global emulator.RestrictedEOAs slice. This will race with other parallel tests (including the test at line 329), causing non-deterministic failures and test isolation violations.
🔒 Recommended fixes
Option 1 (Preferred): Remove parallel execution for this test
- t.Run("Batch run with with transactions from restricted EOA", func(t *testing.T) {
- t.Parallel()
+ t.Run("Batch run with with transactions from restricted EOA", func(t *testing.T) {
+ // Cannot run in parallel due to modification of global emulator.RestrictedEOAs
RunWithNewEnvironment(t,Option 2: Refactor to avoid global state
Consider refactoring the emulator configuration to accept test-specific restricted EOA lists through a context or configuration parameter, rather than modifying a package-level global variable.
Back-port of #8297 from
v0.44tomasterbranch.Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.