Fix data race in downloader

[palango]

Description

The race detector found a problem when running with go test -race -count=1 ./eth/downloader.

It turns out that

celo-blockchain/eth/downloader/queue.go

Line 505 in 67b6239

stale, throttle, item, err := q.resultCache.AddFetch(header, q.mode == FastSync)

emits a pointer to a value owned by the resultstore.

This value is later changed here (added in the latest upstream pull):

celo-blockchain/eth/downloader/queue.go

Lines 531 to 534 in 620b5fc

	if kind == bodyType {
	// All headers must be fetched so that the random beacon can be updated correctly.
	item.pending |= (1 << bodyType)
	}

but also accessed here in AllDone:

celo-blockchain/eth/downloader/resultstore.go

Line 154 in 67b6239

if result == nil || !result.AllDone() {

I guess this patch is the easy solution to avoid the race, but there might be a better way to avoid this in the first place. It seems ugly that pending is accessed in the first place there. I'm open to suggestions.

Other changes

Some typos.

Tested

This triggered the race detector when run with go test -race -count=1 ./eth/downloader. With these changes it didn't trigger locally with a count of 20.

Related issues

/

Backwards compatibility

/

[piersy]

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit 766f0f8

coverage: 45.8% of statements across all listed packages

coverage:  52.6% of statements in consensus/istanbul
coverage:  40.0% of statements in consensus/istanbul/announce
coverage:  54.7% of statements in consensus/istanbul/backend
coverage:   0.0% of statements in consensus/istanbul/backend/backendtest
coverage:  24.3% of statements in consensus/istanbul/backend/internal/replica
coverage:  52.0% of statements in consensus/istanbul/core
coverage:  45.0% of statements in consensus/istanbul/db
coverage:   0.0% of statements in consensus/istanbul/proxy
coverage:  64.4% of statements in consensus/istanbul/uptime
coverage:  51.8% of statements in consensus/istanbul/validator
coverage:  79.2% of statements in consensus/istanbul/validator/random

CommentID: a3fcedc7ea

[codecov]

Codecov Report

Base: 54.82% // Head: 54.79% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (766f0f8) compared to base (e831c00).
Patch coverage: 54.11% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1961      +/-   ##
==========================================
- Coverage   54.82%   54.79%   -0.03%     
==========================================
  Files         688      695       +7     
  Lines       92542    93251     +709     
==========================================
+ Hits        50737    51099     +362     
- Misses      37993    38309     +316     
- Partials     3812     3843      +31     

Impacted Files	Coverage Δ
cmd/geth/config.go	0.00% <0.00%> (ø)
cmd/geth/main.go	20.00% <ø> (ø)
cmd/utils/flags.go	2.76% <0.00%> (-0.03%)	⬇️
consensus/istanbul/config.go	0.00% <0.00%> (ø)
consensus/istanbul/core/preprepare.go	71.23% <0.00%> (+7.03%)	⬆️
consensus/istanbul/types.go	25.58% <0.00%> (-0.76%)	⬇️
contracts/gasprice_minimum/gasprice_minimum.go	45.61% <0.00%> (-14.86%)	⬇️
core/vm/operations_acl.go	4.44% <ø> (ø)
eth/api_backend.go	0.00% <0.00%> (ø)
eth/backend.go	0.00% <0.00%> (ø)
... and 68 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[piersy]

Great!