Skip to content

Commit fb462b6

Browse files
authored
Fix data race in downloader (#1961)
* chore: Fix some typos * downloader: Fix data race when fetching headers * downloaded: Cache extra data error as well
1 parent 2e37ebe commit fb462b6

File tree

14 files changed

+69
-35
lines changed

14 files changed

+69
-35
lines changed

consensus/consensustest/mockprotocol.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,7 @@ func (e *MockEngine) VerifyHeader(chain consensus.ChainHeaderReader, header *typ
228228
// verifyHeader checks whether a header conforms to the consensus rules
229229
func (e *MockEngine) verifyHeader(chain consensus.ChainHeaderReader, header, parent *types.Header, seal bool) error {
230230
// Ensure that the extra data format is satisfied
231-
if _, err := types.ExtractIstanbulExtra(header); err != nil {
231+
if _, err := header.IstanbulExtra(); err != nil {
232232
return errors.New("invalid extra data format")
233233
}
234234
// Verify the header's timestamp

consensus/istanbul/backend/backend.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -476,7 +476,7 @@ func (sb *Backend) ParentBlockValidators(proposal istanbul.Proposal) istanbul.Va
476476
}
477477

478478
func (sb *Backend) NextBlockValidators(proposal istanbul.Proposal) (istanbul.ValidatorSet, error) {
479-
istExtra, err := types.ExtractIstanbulExtra(proposal.Header())
479+
istExtra, err := proposal.Header().IstanbulExtra()
480480
if err != nil {
481481
return nil, err
482482
}
@@ -650,7 +650,7 @@ func (sb *Backend) verifyValSetDiff(proposal istanbul.Proposal, block *types.Blo
650650
header := block.Header()
651651

652652
// Ensure that the extra data format is satisfied
653-
istExtra, err := types.ExtractIstanbulExtra(header)
653+
istExtra, err := header.IstanbulExtra()
654654
if err != nil {
655655
return err
656656
}
@@ -806,7 +806,7 @@ func (sb *Backend) GetCurrentHeadBlockAndAuthor() (istanbul.Proposal, common.Add
806806

807807
func (sb *Backend) LastSubject() (istanbul.Subject, error) {
808808
lastProposal, _ := sb.GetCurrentHeadBlockAndAuthor()
809-
istExtra, err := types.ExtractIstanbulExtra(lastProposal.Header())
809+
istExtra, err := lastProposal.Header().IstanbulExtra()
810810
if err != nil {
811811
return istanbul.Subject{}, err
812812
}

consensus/istanbul/backend/engine.go

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ func (sb *Backend) verifyHeader(chain consensus.ChainHeaderReader, header *types
138138
}
139139

140140
// Ensure that the extra data format is satisfied
141-
if _, err := types.ExtractIstanbulExtra(header); err != nil {
141+
if _, err := header.IstanbulExtra(); err != nil {
142142
return errInvalidExtraDataFormat
143143
}
144144

@@ -273,7 +273,7 @@ func (sb *Backend) verifyAggregatedSeals(chain consensus.ChainHeaderReader, head
273273
return nil
274274
}
275275

276-
extra, err := types.ExtractIstanbulExtra(header)
276+
extra, err := header.IstanbulExtra()
277277
if err != nil {
278278
return err
279279
}
@@ -373,7 +373,7 @@ func (sb *Backend) VerifySeal(header *types.Header) error {
373373
return errUnknownBlock
374374
}
375375

376-
extra, err := types.ExtractIstanbulExtra(header)
376+
extra, err := header.IstanbulExtra()
377377
if err != nil {
378378
return errInvalidExtraDataFormat
379379
}
@@ -861,7 +861,7 @@ func (sb *Backend) snapshot(chain consensus.ChainHeaderReader, number uint64, ha
861861
return nil, errors.New("Cannot load genesis")
862862
}
863863

864-
istanbulExtra, err := types.ExtractIstanbulExtra(genesis)
864+
istanbulExtra, err := genesis.IstanbulExtra()
865865
if err != nil {
866866
log.Error("Unable to extract istanbul extra", "err", err)
867867
return nil, err
@@ -948,7 +948,7 @@ func (sb *Backend) addParentSeal(chain consensus.ChainHeaderReader, header *type
948948

949949
// Get parent's extra to fetch it's AggregatedSeal
950950
parent := chain.GetHeader(header.ParentHash, number-1)
951-
parentExtra, err := types.ExtractIstanbulExtra(parent)
951+
parentExtra, err := parent.IstanbulExtra()
952952
if err != nil {
953953
return err
954954
}
@@ -1046,7 +1046,7 @@ func ecrecover(header *types.Header) (common.Address, error) {
10461046
}
10471047

10481048
// Retrieve the signature from the header extra-data
1049-
istanbulExtra, err := types.ExtractIstanbulExtra(header)
1049+
istanbulExtra, err := header.IstanbulExtra()
10501050
if err != nil {
10511051
return common.Address{}, err
10521052
}
@@ -1099,7 +1099,7 @@ func writeValidatorSetDiff(header *types.Header, oldValSet []istanbul.ValidatorD
10991099
"addedValidators", common.ConvertToStringSlice(addedValidatorsAddresses), "removedValidators", removedValidators.Text(16))
11001100
}
11011101

1102-
extra, err := types.ExtractIstanbulExtra(header)
1102+
extra, err := header.IstanbulExtra()
11031103
if err != nil {
11041104
return nil
11051105
}
@@ -1124,7 +1124,7 @@ func writeSeal(h *types.Header, seal []byte) error {
11241124
return errInvalidSignature
11251125
}
11261126

1127-
istanbulExtra, err := types.ExtractIstanbulExtra(h)
1127+
istanbulExtra, err := h.IstanbulExtra()
11281128
if err != nil {
11291129
return err
11301130
}
@@ -1147,7 +1147,7 @@ func writeAggregatedSeal(h *types.Header, aggregatedSeal types.IstanbulAggregate
11471147
return errInvalidAggregatedSeal
11481148
}
11491149

1150-
istanbulExtra, err := types.ExtractIstanbulExtra(h)
1150+
istanbulExtra, err := h.IstanbulExtra()
11511151
if err != nil {
11521152
return err
11531153
}

consensus/istanbul/backend/engine_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ func TestVerifySeal(t *testing.T) {
177177

178178
// modify seal bitmap and expect to fail the quorum check
179179
header = block.Header()
180-
extra, err := types.ExtractIstanbulExtra(header)
180+
extra, err := header.IstanbulExtra()
181181
g.Expect(err).ToNot(HaveOccurred())
182182
extra.AggregatedSeal.Bitmap = big.NewInt(0)
183183
encoded, err := rlp.EncodeToBytes(extra)
@@ -357,7 +357,7 @@ func TestPrepareExtra(t *testing.T) {
357357
g.Expect(err).ToNot(HaveOccurred())
358358

359359
// the header must have the updated extra data
360-
updatedExtra, err := types.ExtractIstanbulExtra(h)
360+
updatedExtra, err := h.IstanbulExtra()
361361
g.Expect(err).ToNot(HaveOccurred())
362362

363363
var updatedExtraVals []istanbul.ValidatorData
@@ -407,7 +407,7 @@ func TestWriteSeal(t *testing.T) {
407407
g.Expect(err).NotTo(HaveOccurred())
408408

409409
// verify istanbul extra-data
410-
actualIstExtra, err := types.ExtractIstanbulExtra(h)
410+
actualIstExtra, err := h.IstanbulExtra()
411411
g.Expect(err).NotTo(HaveOccurred())
412412
g.Expect(actualIstExtra).To(Equal(expectedIstExtra))
413413

@@ -457,7 +457,7 @@ func TestWriteAggregatedSeal(t *testing.T) {
457457
g.Expect(err).NotTo(HaveOccurred())
458458

459459
// verify istanbul extra-data
460-
actualIstExtra, err := types.ExtractIstanbulExtra(h)
460+
actualIstExtra, err := h.IstanbulExtra()
461461
g.Expect(err).NotTo(HaveOccurred())
462462
g.Expect(actualIstExtra).To(Equal(expectedIstExtra))
463463

consensus/istanbul/backend/handler.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,7 @@ func (sb *Backend) UpdateMetricsForParentOfBlock(child *types.Block) {
239239

240240
// Now check if this validator signer is in the "parent seal" on the child block.
241241
// The parent seal is used for downtime calculations.
242-
childExtra, err := types.ExtractIstanbulExtra(child.Header())
242+
childExtra, err := child.Header().IstanbulExtra()
243243
if err != nil {
244244
return
245245
}

consensus/istanbul/backend/snapshot.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ func (s *Snapshot) apply(headers []*types.Header, db ethdb.Database) (*Snapshot,
130130
}
131131

132132
// Ensure that the extra data format is satisfied
133-
istExtra, err := types.ExtractIstanbulExtra(header)
133+
istExtra, err := header.IstanbulExtra()
134134
if err != nil {
135135
log.Error("Unable to extract the istanbul extra field from the header", "header", header)
136136
return nil, err

consensus/istanbul/uptime/monitor.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ func (um *Monitor) ProcessHeader(header *types.Header) error {
150150
}
151151

152152
// Get the bitmap from the previous block
153-
extra, err := types.ExtractIstanbulExtra(header)
153+
extra, err := header.IstanbulExtra()
154154
if err != nil {
155155
um.logger.Error("Unable to extract istanbul extra", "func", "ProcessBlock", "blocknum", headerNumber)
156156
return errors.New("could not extract block header extra")

core/types/block.go

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"io"
2424
"math/big"
2525
"reflect"
26+
"sync"
2627
"sync/atomic"
2728
"time"
2829

@@ -52,6 +53,11 @@ type Header struct {
5253
GasUsed uint64 `json:"gasUsed" gencodec:"required"`
5354
Time uint64 `json:"timestamp" gencodec:"required"`
5455
Extra []byte `json:"extraData" gencodec:"required"`
56+
57+
// Used to cache deserialized istanbul extra data
58+
extraLock sync.Mutex
59+
extraValue *IstanbulExtra
60+
extraError error
5561
}
5662

5763
// field type overrides for gencodec
@@ -100,6 +106,10 @@ func (h *Header) SanityCheck() error {
100106
// EmptyBody returns true if there is no additional 'body' to complete the header
101107
// that is: no transactions.
102108
func (h *Header) EmptyBody() bool {
109+
if _, err := h.IstanbulExtra(); err == nil {
110+
return false
111+
}
112+
103113
return h.TxHash == EmptyRootHash
104114
}
105115

@@ -271,8 +281,19 @@ func NewBlockWithHeader(header *Header) *Block {
271281
// CopyHeader creates a deep copy of a block header to prevent side effects from
272282
// modifying a header variable.
273283
func CopyHeader(h *Header) *Header {
274-
cpy := *h
275-
if cpy.Number = new(big.Int); h.Number != nil {
284+
cpy := Header{
285+
ParentHash: h.ParentHash,
286+
Coinbase: h.Coinbase,
287+
Root: h.Root,
288+
TxHash: h.TxHash,
289+
ReceiptHash: h.ReceiptHash,
290+
Bloom: h.Bloom,
291+
Number: new(big.Int),
292+
GasUsed: h.GasUsed,
293+
Time: h.Time,
294+
}
295+
296+
if h.Number != nil {
276297
cpy.Number.Set(h.Number)
277298
}
278299
if len(h.Extra) > 0 {
@@ -366,10 +387,10 @@ func (c *writeCounter) Write(b []byte) (int, error) {
366387
// WithHeader returns a new block with the data from b but the header replaced with
367388
// the sealed one.
368389
func (b *Block) WithHeader(header *Header) *Block {
369-
cpy := *header
390+
cpy := CopyHeader(header)
370391

371392
return &Block{
372-
header: &cpy,
393+
header: cpy,
373394
transactions: b.transactions,
374395
randomness: b.randomness,
375396
epochSnarkData: b.epochSnarkData,

core/types/celo_additions.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
package types
2+
3+
// IstanbulExtra returns the 'Extra' field of the header deserialized into an
4+
// IstanbulExtra struct, if there is an error deserializing the 'Extra' field
5+
// it will be returned.
6+
func (h *Header) IstanbulExtra() (*IstanbulExtra, error) {
7+
h.extraLock.Lock()
8+
defer h.extraLock.Unlock()
9+
10+
if h.extraValue == nil && h.extraError == nil {
11+
extra, err := extractIstanbulExtra(h)
12+
13+
h.extraValue = extra
14+
h.extraError = err
15+
}
16+
17+
return h.extraValue, h.extraError
18+
}

core/types/istanbul.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ func (ist *IstanbulExtra) DecodeRLP(s *rlp.Stream) error {
128128
// ExtractIstanbulExtra extracts all values of the IstanbulExtra from the header. It returns an
129129
// error if the length of the given extra-data is less than 32 bytes or the extra-data can not
130130
// be decoded.
131-
func ExtractIstanbulExtra(h *Header) (*IstanbulExtra, error) {
131+
func extractIstanbulExtra(h *Header) (*IstanbulExtra, error) {
132132
if len(h.Extra) < IstanbulExtraVanity {
133133
return nil, ErrInvalidIstanbulHeaderExtra
134134
}
@@ -146,7 +146,7 @@ func ExtractIstanbulExtra(h *Header) (*IstanbulExtra, error) {
146146
// decoded/encoded by rlp.
147147
func IstanbulFilteredHeader(h *Header, keepSeal bool) *Header {
148148
newHeader := CopyHeader(h)
149-
istanbulExtra, err := ExtractIstanbulExtra(newHeader)
149+
istanbulExtra, err := extractIstanbulExtra(newHeader)
150150
if err != nil {
151151
return nil
152152
}

0 commit comments

Comments
 (0)