Skip to content

Commit

Permalink
core: fix header copy race (#13485)
Browse files Browse the repository at this point in the history
relates to #13480
fully fixes #13474

Seems like `Header.hash` memoization changes from a few months ago have
made `Block.Header()` (which makes a copy by calling `CopyHeader`) show
race warnings. This is because we are copying over `atomic.Pointer` in
`CopyHeader`.

This PR fixes this by manually copy-ing all fields and skipping copy of
the `hash atomic.Pointer` attribute (instead it will get set using its
zero value - correct behaviour) and then the hash will be memoized using
it as part of the copy later on.

This is a bit clumsier as we can miss updating the `CopyHeader` function
when adding new attributes to the `Header` struct - to address that I've
added a reflection based test which will generate random values for all
struct fields and as a result capture this pitfall and remind developers
to update.

Example race detected:
```
==================
WARNING: DATA RACE
Read at 0x00c03acc0838 by goroutine 26498:
  github.com/erigontech/erigon/core/types.CopyHeader()
      /home/ubuntu/erigon/core/types/block.go:1098 +0x5c
  github.com/erigontech/erigon/core/types.(*Block).Header()
      /home/ubuntu/erigon/core/types/block.go:1278 +0x54
  github.com/erigontech/erigon/turbo/execution/eth1/eth1_utils.ConvertBlockToRPC()
      /home/ubuntu/erigon/turbo/execution/eth1/eth1_utils/grpc.go:108 +0x55
  github.com/erigontech/erigon/turbo/execution/eth1/eth1_utils.ConvertBlocksToRPC()
      /home/ubuntu/erigon/turbo/execution/eth1/eth1_utils/grpc.go:102 +0xc4
  github.com/erigontech/erigon/polygon/sync.(*executionClient).InsertBlocks()
      /home/ubuntu/erigon/polygon/sync/execution_client.go:71 +0x4f
  github.com/erigontech/erigon/polygon/sync.(*ExecutionClientStore).insertBlocks()
      /home/ubuntu/erigon/polygon/sync/store.go:162 +0x14f
  github.com/erigontech/erigon/polygon/sync.(*ExecutionClientStore).Run()
      /home/ubuntu/erigon/polygon/sync/store.go:143 +0x1ab
  github.com/erigontech/erigon/polygon/sync.(*Service).Run.func2()
      /home/ubuntu/erigon/polygon/sync/service.go:125 +0x69
  golang.org/x/sync/errgroup.(*Group).Go.func1()
==================
```
  • Loading branch information
taratorio authored Jan 18, 2025
1 parent 4497748 commit 8dec5ac
Show file tree
Hide file tree
Showing 3 changed files with 115 additions and 7 deletions.
36 changes: 29 additions & 7 deletions core/types/block.go
Original file line number Diff line number Diff line change
Expand Up @@ -1095,25 +1095,38 @@ func NewBlockFromNetwork(header *Header, body *Body) *Block {
// CopyHeader creates a deep copy of a block header to prevent side effects from
// modifying a header variable.
func CopyHeader(h *Header) *Header {
cpy := *h //nolint
cpy := Header{} // note: do not copy hash atomic.Pointer
cpy.ParentHash = h.ParentHash
cpy.UncleHash = h.UncleHash
cpy.Coinbase = h.Coinbase
cpy.Root = h.Root
cpy.TxHash = h.TxHash
cpy.ReceiptHash = h.ReceiptHash
cpy.Bloom = h.Bloom
if cpy.Difficulty = new(big.Int); h.Difficulty != nil {
cpy.Difficulty.Set(h.Difficulty)
}
if cpy.Number = new(big.Int); h.Number != nil {
cpy.Number.Set(h.Number)
}
if h.BaseFee != nil {
cpy.BaseFee = new(big.Int)
cpy.BaseFee.Set(h.BaseFee)
}
if len(h.Extra) > 0 {
cpy.GasLimit = h.GasLimit
cpy.GasUsed = h.GasUsed
cpy.Time = h.Time
if h.Extra != nil {
cpy.Extra = make([]byte, len(h.Extra))
copy(cpy.Extra, h.Extra)
}
if len(h.AuRaSeal) > 0 {
cpy.MixDigest = h.MixDigest
cpy.Nonce = h.Nonce
cpy.AuRaStep = h.AuRaStep
if h.AuRaSeal != nil {
cpy.AuRaSeal = make([]byte, len(h.AuRaSeal))
copy(cpy.AuRaSeal, h.AuRaSeal)
}
if h.BaseFee != nil {
cpy.BaseFee = new(big.Int)
cpy.BaseFee.Set(h.BaseFee)
}
if h.WithdrawalsHash != nil {
cpy.WithdrawalsHash = new(libcommon.Hash)
cpy.WithdrawalsHash.SetBytes(h.WithdrawalsHash.Bytes())
Expand All @@ -1134,6 +1147,15 @@ func CopyHeader(h *Header) *Header {
cpy.RequestsHash = new(libcommon.Hash)
cpy.RequestsHash.SetBytes(h.RequestsHash.Bytes())
}
cpy.Verkle = h.Verkle
if h.VerkleProof != nil {
cpy.VerkleProof = make([]byte, len(h.VerkleProof))
copy(cpy.VerkleProof, h.VerkleProof)
}
if h.VerkleKeyVals != nil {
cpy.VerkleKeyVals = make([]verkle.KeyValuePair, len(h.VerkleKeyVals))
copy(cpy.VerkleKeyVals, h.VerkleKeyVals)
}
cpy.mutable = h.mutable
if hash := h.hash.Load(); hash != nil {
hashCopy := *hash
Expand Down
12 changes: 12 additions & 0 deletions core/types/block_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -606,3 +606,15 @@ func TestCopyTxs(t *testing.T) {
copies := CopyTxs(txs)
assert.Equal(t, txs, copies)
}

func TestCopyHeader(t *testing.T) {
// if this test fails when adding a new attribute to the Header struct
// please update the copy function to include logic for the new attribute
const runCount = 1000
tr := NewTRand()
for range make([]byte, runCount) {
h1 := tr.RandHeaderReflectAllFields()
h2 := CopyHeader(h1)
require.Equal(t, h1, h2)
}
}
74 changes: 74 additions & 0 deletions core/types/encdec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"testing"
"time"

"github.com/gballet/go-verkle"
"github.com/holiman/uint256"

libcommon "github.com/erigontech/erigon-lib/common"
Expand Down Expand Up @@ -77,10 +78,29 @@ func (tr *TRand) RandHash() libcommon.Hash {
return libcommon.Hash(tr.RandBytes(32))
}

func (tr *TRand) RandBoolean() bool {
return tr.rnd.Intn(2) == 0
}

func (tr *TRand) RandBloom() Bloom {
return Bloom(tr.RandBytes(BloomByteLength))
}

func (tr *TRand) RandVerkleKeyValuePairs(count int) []verkle.KeyValuePair {
res := make([]verkle.KeyValuePair, count)
for i := 0; i < count; i++ {
res[i] = tr.RandVerkleKeyValuePair()
}
return res
}

func (tr *TRand) RandVerkleKeyValuePair() verkle.KeyValuePair {
return verkle.KeyValuePair{
Key: tr.RandBytes(tr.RandIntInRange(1, 32)),
Value: tr.RandBytes(tr.RandIntInRange(1, 32)),
}
}

func (tr *TRand) RandWithdrawal() *Withdrawal {
return &Withdrawal{
Index: tr.rnd.Uint64(),
Expand Down Expand Up @@ -117,6 +137,60 @@ func (tr *TRand) RandHeader() *Header {
}
}

func (tr *TRand) RandHeaderReflectAllFields(skipFields ...string) *Header {
skipSet := make(map[string]struct{}, len(skipFields))
for _, field := range skipFields {
skipSet[field] = struct{}{}
}

emptyUint64 := uint64(0)
h := &Header{}
// note unexported fields are skipped in reflection auto-assign as they are not assignable
h.mutable = tr.RandBoolean()
headerValue := reflect.ValueOf(h)
headerElem := headerValue.Elem()
numField := headerElem.Type().NumField()
for i := 0; i < numField; i++ {
field := headerElem.Field(i)
if !field.CanSet() {
continue
}

if _, skip := skipSet[headerElem.Type().Field(i).Name]; skip {
continue
}

switch field.Type() {
case reflect.TypeOf(libcommon.Hash{}):
field.Set(reflect.ValueOf(tr.RandHash()))
case reflect.TypeOf(&libcommon.Hash{}):
randHash := tr.RandHash()
field.Set(reflect.ValueOf(&randHash))
case reflect.TypeOf(libcommon.Address{}):
field.Set(reflect.ValueOf(tr.RandAddress()))
case reflect.TypeOf(Bloom{}):
field.Set(reflect.ValueOf(tr.RandBloom()))
case reflect.TypeOf(BlockNonce{}):
field.Set(reflect.ValueOf(BlockNonce(tr.RandBytes(8))))
case reflect.TypeOf(&big.Int{}):
field.Set(reflect.ValueOf(tr.RandBig()))
case reflect.TypeOf(uint64(0)):
field.Set(reflect.ValueOf(*tr.RandUint64()))
case reflect.TypeOf(&emptyUint64):
field.Set(reflect.ValueOf(tr.RandUint64()))
case reflect.TypeOf([]byte{}):
field.Set(reflect.ValueOf(tr.RandBytes(tr.RandIntInRange(128, 1024))))
case reflect.TypeOf(false):
field.Set(reflect.ValueOf(tr.RandBoolean()))
case reflect.TypeOf([]verkle.KeyValuePair{}):
field.Set(reflect.ValueOf(tr.RandVerkleKeyValuePairs(tr.RandIntInRange(1, 3))))
default:
panic(fmt.Sprintf("don't know how to generate rand value for Header field type %v - please add handler", field.Type()))
}
}
return h
}

func (tr *TRand) RandAccessTuple() AccessTuple {
n := tr.RandIntInRange(1, 5)
sk := make([]libcommon.Hash, n)
Expand Down

0 comments on commit 8dec5ac

Please sign in to comment.