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.

Sign up for GitHub

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

Jump to bottom

core: copy genesis before modifying #31097

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

core: copy genesis before modifying #31097

wants to merge 6 commits into from
+25 −20

Conversation

MariusVanDerWijden
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden commented Jan 30, 2025 •
edited
Loading

This PR fixes the following data race:

The issue was that SetupGenesisWithOverride would overwrite the passed chain config

WARNING: DATA RACE
Write at 0x0000011fcea0 by goroutine 107204:
  github.com/ethereum/go-ethereum/core.SetupGenesisBlockWithOverride()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/core/genesis.go:302 +0x244
  github.com/ethereum/go-ethereum/core.NewBlockChain()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/core/blockchain.go:285 +0x668
  github.com/ethereum/go-ethereum/eth/downloader.newTestBlockchain.func1()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/eth/downloader/testchain_test.go:220 +0x1cb
  sync.(*Once).doSlow()
      /home/matematik/sdk/go1.22.1/src/sync/once.go:74 +0xf0
  sync.(*Once).Do()
      /home/matematik/sdk/go1.22.1/src/sync/once.go:65 +0x44
  github.com/ethereum/go-ethereum/eth/downloader.newTestBlockchain()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/eth/downloader/testchain_test.go:216 +0x2a8
  github.com/ethereum/go-ethereum/eth/downloader.init.2.func4()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/eth/downloader/testchain_test.go:109 +0x44
  github.com/ethereum/go-ethereum/eth/downloader.init.2.gowrap1()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/eth/downloader/testchain_test.go:111 +0x61

Previous read at 0x0000011fcea0 by goroutine 107207:
  github.com/ethereum/go-ethereum/core.EnableVerkleAtGenesis()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/core/genesis.go:565 +0x8b
  github.com/ethereum/go-ethereum/core.NewBlockChain()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/core/blockchain.go:275 +0xb0
  github.com/ethereum/go-ethereum/eth/downloader.newTestBlockchain.func1()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/eth/downloader/testchain_test.go:220 +0x1cb
  sync.(*Once).doSlow()
      /home/matematik/sdk/go1.22.1/src/sync/once.go:74 +0xf0
  sync.(*Once).Do()
      /home/matematik/sdk/go1.22.1/src/sync/once.go:65 +0x44
  github.com/ethereum/go-ethereum/eth/downloader.newTestBlockchain()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/eth/downloader/testchain_test.go:216 +0x2a8
  github.com/ethereum/go-ethereum/eth/downloader.init.2.func4()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/eth/downloader/testchain_test.go:109 +0x44
  github.com/ethereum/go-ethereum/eth/downloader.init.2.gowrap1()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/eth/downloader/testchain_test.go:111 +0x61

Goroutine 107204 (running) created at:
  github.com/ethereum/go-ethereum/eth/downloader.init.2()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/eth/downloader/testchain_test.go:108 +0xcd3

Goroutine 107207 (running) created at:
  github.com/ethereum/go-ethereum/eth/downloader.init.2()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/eth/downloader/testchain_test.go:108 +0xcd3
==================
==================
WARNING: DATA RACE
Write at 0x0000011fcea0 by goroutine 107210:
  github.com/ethereum/go-ethereum/core.SetupGenesisBlockWithOverride()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/core/genesis.go:302 +0x244
  github.com/ethereum/go-ethereum/core.NewBlockChain()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/core/blockchain.go:285 +0x668
  github.com/ethereum/go-ethereum/eth/downloader.newTestBlockchain.func1()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/eth/downloader/testchain_test.go:220 +0x1cb
  sync.(*Once).doSlow()
      /home/matematik/sdk/go1.22.1/src/sync/once.go:74 +0xf0
  sync.(*Once).Do()
      /home/matematik/sdk/go1.22.1/src/sync/once.go:65 +0x44
  github.com/ethereum/go-ethereum/eth/downloader.newTestBlockchain()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/eth/downloader/testchain_test.go:216 +0x2a8
  github.com/ethereum/go-ethereum/eth/downloader.init.2.func4()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/eth/downloader/testchain_test.go:109 +0x44
  github.com/ethereum/go-ethereum/eth/downloader.init.2.gowrap1()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/eth/downloader/testchain_test.go:111 +0x61

Previous read at 0x0000011fcea0 by goroutine 107204:
  github.com/ethereum/go-ethereum/core.(*Genesis).Commit()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/core/genesis.go:509 +0xe4
  github.com/ethereum/go-ethereum/core.SetupGenesisBlockWithOverride()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/core/genesis.go:304 +0x2a4
  github.com/ethereum/go-ethereum/core.NewBlockChain()
      /home/matematik/go/sr

@fjl
Copy link
Contributor

fjl commented Jan 30, 2025

Hmm, I checked this in genesis, and it looks like the recent PR makes it modify the passed-in Genesis struct by setting the Genesis.Config to a new config with overrides applied. This needs to be fixed.

@MariusVanDerWijden MariusVanDerWijden changed the title eth/downloader: fix racy test core: copy genesis before modifying Jan 30, 2025
@MariusVanDerWijden
Copy link
Member Author

@fjl Yes you're right. I think the correct thing would be to copy the genesis struct. WDYT?

fjl
fjl reviewed Jan 30, 2025
View reviewed changes
core/genesis.go Outdated
@@ -550,6 +551,19 @@ func (g *Genesis) MustCommit(db ethdb.Database, triedb *triedb.Database) *types.
return block
}

// Copy copies the genesis.
func (g *Genesis) Copy() *Genesis {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be unexported.

fjl
fjl previously approved these changes Jan 30, 2025
View reviewed changes
rjl493456442
rjl493456442 reviewed Jan 31, 2025
View reviewed changes
core/genesis.go Outdated
func (g *Genesis) copy() *Genesis {
if g != nil {
cpy := *g
if g.Config != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In overrides.apply, it also deep-copies the chain config before modification, maybe you can remove the logic there?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its true, but I wanted to make copy a correct deep copy. I can remove it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry what I meant is to remove the copy logic in the overrides.apply. I think it's good to have a correct deep-copy function

lightclient
lightclient reviewed Jan 31, 2025
View reviewed changes
core/genesis.go Outdated Show resolved Hide resolved
@fjl fjl added this to the 1.15.0 milestone Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lightclient lightclient lightclient left review comments

@rjl493456442 rjl493456442 rjl493456442 left review comments

@fjl fjl fjl left review comments

@holiman holiman Awaiting requested review from holiman holiman is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.15.0
Development

Successfully merging this pull request may close these issues.
4 participants
@MariusVanDerWijden @fjl @rjl493456442 @lightclient