erigontech / erigontech/erigon

execution/protocol: GetHashFn contains unreachable mutex protecting sequential-only code

Open
#20,219 18 comments 0 reactions 1 assignee Claimed by @awskii View on GitHub
tech debt reduction
Dominant language
Go
Stars
3.6k
Forks
1.5k
Avg merge
1d 16h
Merged PRs (30d)
465

Description

## Problem

`GetHashFn` in `execution/protocol/evm.go` creates a `sync.Mutex` and wraps every `getHeader` call in an unlock-during-IO pattern:

```go
hashLookupCache, _ := lru.New[uint64, common.Hash](8192)
hashLookupCacheLock := sync.Mutex{}

// ...inside the returned closure:
header, err := func() (*types.Header, error) {
hash, num := lastKnownHash, lastKnownNumber
hashLookupCacheLock.Unlock() // release during I/O
defer hashLookupCacheLock.Lock()
return getHeader(hash, num)
}()
```

The mutex is unreachable in practice. All callers execute transactions **sequentially**:
- `ExecuteBlockEphemerally` iterates `block.Transactions()` in a plain for-loop (`block_exec.go:128`)
- `exec3_serial.go`: single-threaded by design
- `exec3_parallel.go`: each worker goroutine owns its own closure instance — the closure is never shared across goroutines

The closure is created once per block and reused across all transactions in that block, so the walk-back state (`lastKnownNumber`, `lastKnownHash`) is shared intra-block but never concurrent.

Additionally, the LRU is sized at 8192 entries, but the EVM `BLOCKHASH` opcode can only look back 256 blocks (EIP-210). The cache will never hold more than 256 entries, making the extra capacity wasted overhead.

## Impact

- Dead synchronization code that makes the function harder to understand and audit
- Future contributors may assume concurrency is possible here and add more synchronization, compounding complexity
- `sync.Mutex` lock/unlock on every `getHeader` call adds noise to profiling and tracing
- LRU eviction bookkeeping on a cache that cannot fill beyond 256 entries

## Proposed Fix

1. Remove `hashLookupCacheLock` and all `Lock`/`Unlock` calls
2. Replace `lru.New[uint64, common.Hash](8192)` with `make(map[uint64]common.Hash, 256)`
3. Call `getHeader` directly without the IIFE unlock wrapper
4. Drop the `lru` import from this file if unused after the change

```go
// After: simple sequential walk-back with a plain map cache
hashLookupCache := make(map[uint64]common.Hash, 256)
hashLookupCache[refNumber] = refHash

return func(n uint64) (common.Hash, error) {
if n > refNumber {
lastKnownNumber = refNumber
lastKnownHash = refHash
}
if hash, ok := hashLookupCache[n]; ok {
return hash, nil
}
for lastKnownNumber != n {
if n > lastKnownNumber {
lastKnownNumber = refNumber
lastKnownHash = refHash
}
header, err := getHeader(lastKnownHash, lastKnownNumber)
if err != nil || header == nil {
return common.Hash{}, nil
}
lastKnownHash = header.ParentHash
lastKnownNumber = header.Number.Uint64() - 1
hashLookupCache[lastKnownNumber] = lastKnownHash
}
return lastKnownHash, nil
}
```

Result: identical behaviour, roughly half the current line count, no synchronization primitives.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.