Skip to content

feat: Add error handling to Commit() function#467

Open
Apostlex0 wants to merge 1 commit intoethereum:masterfrom
Apostlex0:fix/test-error-handling
Open

feat: Add error handling to Commit() function#467
Apostlex0 wants to merge 1 commit intoethereum:masterfrom
Apostlex0:fix/test-error-handling

Conversation

@Apostlex0
Copy link
Copy Markdown

@Apostlex0 Apostlex0 commented Jun 16, 2025

Description

This PR adds error handling to the Commit() function in the Verkle tree implementation. Previously, the Commit() function did not return errors, which could lead to silent failures.

Changes

  • Modified the VerkleNode interface to make Commit() return (*Point, error)
  • Updated all implementations of Commit() to properly handle and return errors
  • Updated all callers of Commit() to handle the returned error
  • Fixed test cases to properly handle errors from Commit()

Why

The original implementation did not handle potential errors during commitment computation, which could lead to:

  1. Silent failures in critical operations
  2. Inconsistent state when errors occur
  3. Difficulty in debugging issues
  4. Potential security implications

Testing

  • All tests have been updated to handle errors from Commit() and they pass

Related Issues

  • Improves error handling consistency across the codebase

@Apostlex0
Copy link
Copy Markdown
Author

Hey, @gballet could you please review the pr and see if the changes are of any help.

Copy link
Copy Markdown
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. This repo is currently no longer used in geth and will probably be archived soon (although I will keep it alive if it's useful to some people).

In order to merge this, the requested changes have to be applied, but on top of that, extra care needs to be taken that if something goes wrong, there is enough information given to debug this. Which means that info about the curve point needs to be added to the only error that can arise in commitNodesAtLevel.

Comment thread hashednode.go
// reconsider splitting the interface or find some safer workaround.
panic("can not commit a hash node")
func (n HashedNode) Commit() (*Point, error) {
return n.Commitment(), nil
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is a no-op change, and it removes some comments that are useful in case we want to resurrect this repo later.

Comment thread tree.go
}
} else {
var wg sync.WaitGroup
errChan := make(chan error, 1)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is incorrect, as more than one goroutine can write an error to the channel. If so, multiple goroutines can leak.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

so this needs to be something like:

Suggested change
errChan := make(chan error, 1)
errChan := make(chan error, (len(nodes) + batchSize - 1) / batchSize)

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.

2 participants