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

trie, core/state: revert error removal in (*state.Trie).Commit #27544

Merged

Conversation

gballet
Copy link
Member

@gballet gballet commented Jun 23, 2023

This has been discussed with @rjl493456442 : the error result was removed from (*state.Trie).Commit as no error could occur in the MPT's Commit function. In verkle, however, errors can happen and should be reported.

@fjl
Copy link
Contributor

fjl commented Jun 23, 2023

What kind of errors can happen in the verkle tree during commit?

s.data.Root = root
return nodes, nil
root, nodes, err := tr.Commit(false)
if err == nil {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to reform the code like

root, nodes, err := tr.Commit(false)
if err != nil {
    return nil, err
}
s.data.Root = root
return nodes, nil

Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

I have nothing to against, if verkle needs to return error.

Only a nitpick, lgtm.

@gballet
Copy link
Member Author

gballet commented Jun 26, 2023

What kind of errors can happen in the verkle tree during commit?

Looking at the code:

  • invalid root node type if the deserialization didn't work properly
  • an error occuring during the batch-serialization of a node
  • error writing the serialized nodes to disk (although this would no longer happen once PBSS becomes verkle-friendly)

None of that should happen in practice, but if it does it'd be vital to capture that error.

Co-Authored-By:  rjl493456442 <garyrong0905@gmail.com>
@rjl493456442
Copy link
Member

Yeah, in MPT, (2) node encoding will never fail since we always write to a local buffer and (3) we won't do any database write even in current master in commit function.

But I think it's ok to return an error. Error might occurs in verkle, e.g. some cryptography operations.

@karalabe karalabe added this to the 1.12.1 milestone Jun 27, 2023
@karalabe karalabe merged commit c7b099b into ethereum:master Jun 27, 2023
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
…eum#27544)

* trie, core/state: revert error removal in (*state.Trie).Commit

* Gary's nitpick :)

Co-Authored-By:  rjl493456442 <garyrong0905@gmail.com>

---------

Co-authored-by: rjl493456442 <garyrong0905@gmail.com>
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
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.

4 participants