Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: fix header copy race #13485

Merged
merged 5 commits into from
Jan 18, 2025
Merged

core: fix header copy race #13485

merged 5 commits into from
Jan 18, 2025

Conversation

taratorio
Copy link
Member

@taratorio taratorio commented Jan 17, 2025

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()
==================

@taratorio taratorio merged commit 8dec5ac into main Jan 18, 2025
13 checks passed
@taratorio taratorio deleted the fix-header-copy-race branch January 18, 2025 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

polygon sync: race in announceObserver
2 participants