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/state/snapshot: simplify snapshot rebuild #30772

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

Conversation

ARR4N
Copy link
Contributor

@ARR4N ARR4N commented Nov 20, 2024

This PR is purely for improved readability; I was doing work involving the file and think this may help others who are trying to understand what's going on.

  1. snapshot.Tree.Rebuild() now returns a function that blocks until regeneration is complete, allowing Tree.waitBuild() to be removed entirely as all it did was search for the done channel behind this new function.
  2. Its usage inside New() is also simplified by (a) only waiting if !AsyncBuild; and (b) avoiding the double negative of if !NoBuild.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Very elegant, looks good to me!

@holiman
Copy link
Contributor

holiman commented Nov 21, 2024

Seems to get stuck though :)

goroutine 22 [runnable]:
github.com/ethereum/go-ethereum/core/state/snapshot.New({0xc0003be008?, 0x2?, 0x0?, 0x0?}, {0x7f52d24be8f8, 0xc00003a680}, 0xc00045e030, {0xec, 0xec, 0xdf, ...})
	/home/appveyor/projects/go-ethereum/core/state/snapshot/snapshot.go:222 +0x2b2
github.com/ethereum/go-ethereum/core.NewBlockChain({0xac60f0, 0xc00003a680}, 0x0?, 0xc00003fe48, 0x0, {0xac3cb0, 0xc0001006c0}, {0x0, 0x0, 0x0, ...}, ...)
	/home/appveyor/projects/go-ethereum/core/blockchain.go:449 +0x182a
github.com/ethereum/go-ethereum/consensus/clique.TestReimportMirroredState(0xc00015b520)

core/state/snapshot/snapshot.go Outdated Show resolved Hide resolved
@holiman holiman changed the title refactor(snapshot): simplify rebuilding in New() core/state/snapshot: simplify snapshot rebuild Nov 21, 2024
@ARR4N
Copy link
Contributor Author

ARR4N commented Nov 21, 2024

Seems to get stuck though :)

Whoops, I read the for head != nil as if head != nil. Thanks for reverting that.

@ARR4N
Copy link
Contributor Author

ARR4N commented Nov 21, 2024

@holiman sorry for the post-approval push. I just made the for loop obvious as I and another developer I spoke to both assumed it was an if statement.

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.

3 participants