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: ensure state could be created in ToBlock #22780

Merged
merged 3 commits into from
May 11, 2021

Conversation

gballet
Copy link
Member

@gballet gballet commented Apr 29, 2021

While replacing the trie with verkle trees, I ran into this issue that state.New would return an error if it couldn't find the trie in the database, and that error wasn't being checked. This PR returns an error in state.New fails, so that a problem can be immediately detected by the calling code, and not cause a seemingly unrelated crash further down the road.

core/genesis.go Outdated
Comment on lines 270 to 273
statedb, err := state.New(common.Hash{}, state.NewDatabase(db), nil)
if err != nil {
return nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get this. How can this possibly error?

Copy link
Member Author

@gballet gballet Apr 30, 2021

Choose a reason for hiding this comment

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

if the root isn't in the database, the underlying trie.OpenTrie will return an error, and if this error isn't caught, you end up with statedb = nil, which will crash later. I spent too much time looking for where the problem was, it would have been immediately obvious had an error been returned here.

Copy link
Contributor

Choose a reason for hiding this comment

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

But the root is common.Hash{}, right? So I still don't get it

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, and in master, an empty hash will return an empty trie. I got a problem when I modified OpenTrie to support Verkle trees, which don't have the same behavior (it's expecting a translated layer, not an empty tree).

I can certainly do the same thing in Verkle trees: return an empty tree when receiving common.Hash. This PR still makes sense, though, because if any other new database system appears, the same problem will occur.

@fjl
Copy link
Contributor

fjl commented Apr 29, 2021

ToBlock is a convenience method for unit tests and should not return an error.

@fjl fjl changed the title Ensure state could be created in ToBlock core: ensure state could be created in ToBlock Apr 29, 2021
@gballet
Copy link
Member Author

gballet commented Apr 30, 2021

ToBlock is a convenience method for unit tests and should not return an error.

Aren't tests supposed to catch errors? I could agree if this was a local function. This one, however, is exported and so it could be used by any client code.

@holiman
Copy link
Contributor

holiman commented May 6, 2021

I still don't quite get this. So state.New already does return an error.
The (g *Genesis)ToBlock method does currently not return an error. For some reason, when you use Verkle tries, it does create an error due to the internal call to state.New from within ToBlock.

Now, zooming out a bit from the choice of state representation, whether it be mpt or verkle ... How can it possibly happen that a genesis state cannot be found?
Any genesis state is based off a blank slate which is then populated with some accounts and code: all of which is explicitly defined in the genesis. So what could possibly go wrong that cannot be solved internally in ToBlock ?

I don't mean "wcgw, yolo, let's ignore errors": I'm genuinely wondering why any type of error can be raised here.

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.

LGTM

@karalabe karalabe added this to the 1.10.4 milestone May 11, 2021
@karalabe karalabe merged commit a2c456a into ethereum:master May 11, 2021
@RayChang1996
Copy link

Hi

atif-konasl pushed a commit to frozeman/pandora-execution-engine that referenced this pull request Oct 15, 2021
…um#22780)

* Ensure state could be created in ToBlock

* Fix rebase errors

* use a panic instead
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants