Skip to content

Commit

Permalink
Bad blocks fix (fix datarace in ActivePrecompiles) (#312)
Browse files Browse the repository at this point in the history
ActivePrecompiles referenced a global variable named PrecompiledAddressesCel2
from multiple goroutines without any synchronisation. This PR fixes that by
making PrecompiledAddressesCel2 a local variable within ActivePrecompiles.

This also looks like it is a fix for the bad blocks we had been seeing by
causing the ActivePrecompiles return value to lack some of our active
precompiles, which in turn would lead to them not being pre-warmed in the
access list, which would result in them costing an extra 2500 gas when executed
for the first time in a transaction. (Thank you to @Kourin1996 & @karlb for
figuring this out!)


See the data race trace below.

op-geth-1  | WARNING: DATA RACE
op-geth-1  | Read at 0x0000040a7920 by goroutine 651651:
op-geth-1  |   github.com/ethereum/go-ethereum/core/vm.ActivePrecompiles()
op-geth-1  |       github.com/ethereum/go-ethereum/core/vm/contracts.go:257 +0x35c
op-geth-1  |   github.com/ethereum/go-ethereum/core.(*StateTransition).innerTransitionDb()
op-geth-1  |       github.com/ethereum/go-ethereum/core/state_transition.go:600 +0x190
op-geth-1  |   github.com/ethereum/go-ethereum/core.(*StateTransition).TransitionDb()
op-geth-1  |       github.com/ethereum/go-ethereum/core/state_transition.go:550 +0x148
op-geth-1  |   github.com/ethereum/go-ethereum/core.ApplyMessage()
op-geth-1  |       github.com/ethereum/go-ethereum/core/state_transition.go:250 +0x608
op-geth-1  |   github.com/ethereum/go-ethereum/core.precacheTransaction()
op-geth-1  |       github.com/ethereum/go-ethereum/core/state_prefetcher.go:90 +0xac
op-geth-1  |   github.com/ethereum/go-ethereum/core.(*statePrefetcher).Prefetch()
op-geth-1  |       github.com/ethereum/go-ethereum/core/state_prefetcher.go:69 +0x640
op-geth-1  |   github.com/ethereum/go-ethereum/core.(*BlockChain).insertChain.func3()
op-geth-1  |       github.com/ethereum/go-ethereum/core/blockchain.go:1842 +0x11c
op-geth-1  |   github.com/ethereum/go-ethereum/core.(*BlockChain).insertChain.gowrap2()
op-geth-1  |       github.com/ethereum/go-ethereum/core/blockchain.go:1848 +0x78
op-geth-1  | 
op-geth-1  | Previous write at 0x0000040a7920 by goroutine 651491:
op-geth-1  |   github.com/ethereum/go-ethereum/core/vm.ActivePrecompiles()
op-geth-1  |       github.com/ethereum/go-ethereum/core/vm/contracts.go:257 +0x368
op-geth-1  |   github.com/ethereum/go-ethereum/core.(*StateTransition).innerTransitionDb()
op-geth-1  |       github.com/ethereum/go-ethereum/core/state_transition.go:600 +0x190
op-geth-1  |   github.com/ethereum/go-ethereum/core.(*StateTransition).TransitionDb()
op-geth-1  |       github.com/ethereum/go-ethereum/core/state_transition.go:550 +0x148
op-geth-1  |   github.com/ethereum/go-ethereum/core.ApplyMessage()
op-geth-1  |       github.com/ethereum/go-ethereum/core/state_transition.go:250 +0x608
op-geth-1  |   github.com/ethereum/go-ethereum/core.ApplyTransactionWithEVM()
op-geth-1  |       github.com/ethereum/go-ethereum/core/state_processor.go:132 +0x4e8
op-geth-1  |   github.com/ethereum/go-ethereum/core.(*StateProcessor).Process()
op-geth-1  |       github.com/ethereum/go-ethereum/core/state_processor.go:92 +0x9cc
op-geth-1  |   github.com/ethereum/go-ethereum/core.(*BlockChain).processBlock()
op-geth-1  |       github.com/ethereum/go-ethereum/core/blockchain.go:1934 +0x3b4
op-geth-1  |   github.com/ethereum/go-ethereum/core.(*BlockChain).insertChain()
op-geth-1  |       github.com/ethereum/go-ethereum/core/blockchain.go:1853 +0x2cd8
op-geth-1  |   github.com/ethereum/go-ethereum/core.(*BlockChain).InsertChain()
op-geth-1  |       github.com/ethereum/go-ethereum/core/blockchain.go:1625 +0xde0
op-geth-1  |   github.com/ethereum/go-ethereum/eth/downloader.(*Downloader).importBlockResults()
op-geth-1  |       github.com/ethereum/go-ethereum/eth/downloader/downloader.go:771 +0x6f4
op-geth-1  |   github.com/ethereum/go-ethereum/eth/downloader.(*Downloader).processFullSyncContent()
op-geth-1  |       github.com/ethereum/go-ethereum/eth/downloader/downloader.go:742 +0xa8
op-geth-1  |   github.com/ethereum/go-ethereum/eth/downloader.(*Downloader).syncToHead.func8()
op-geth-1  |       github.com/ethereum/go-ethereum/eth/downloader/downloader.go:537 +0x2c
op-geth-1  |   github.com/ethereum/go-ethereum/eth/downloader.(*Downloader).spawnSync.func1()
op-geth-1  |       github.com/ethereum/go-ethereum/eth/downloader/downloader.go:549 +0x90
  • Loading branch information
piersy authored Jan 21, 2025
1 parent 40f1e56 commit 1d58f64
Showing 1 changed file with 6 additions and 5 deletions.
11 changes: 6 additions & 5 deletions core/vm/contracts.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,6 @@ var PrecompiledCeloContractsCel2 = map[common.Address]CeloPrecompiledContract{

var (
PrecompiledAddressesGranite []common.Address
PrecompiledAddressesCel2 []common.Address
PrecompiledAddressesFjord []common.Address
PrecompiledAddressesPrague []common.Address
PrecompiledAddressesCancun []common.Address
Expand Down Expand Up @@ -286,14 +285,16 @@ func ActivePrecompiles(rules params.Rules) []common.Address {
return addresses
}

PrecompiledAddressesCel2 = PrecompiledAddressesCel2[:0]
PrecompiledAddressesCel2 = append(PrecompiledAddressesCel2, addresses...)
// We can't hardcode the cel2 precompiles because they depend on the underlying
// active optimism fork, so instead we dynamically calculate them here.
precompiledAddressesCel2 := make([]common.Address, 0, len(addresses)+len(PrecompiledCeloContractsCel2))
precompiledAddressesCel2 = append(precompiledAddressesCel2, addresses...)

for k := range PrecompiledCeloContractsCel2 {
PrecompiledAddressesCel2 = append(PrecompiledAddressesCel2, k)
precompiledAddressesCel2 = append(precompiledAddressesCel2, k)
}

return PrecompiledAddressesCel2
return precompiledAddressesCel2
}

// RunPrecompiledContract runs and evaluates the output of a precompiled contract.
Expand Down

0 comments on commit 1d58f64

Please sign in to comment.