-
Notifications
You must be signed in to change notification settings - Fork 6
Fix panic when updating proofs #254
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a panic when updating element proofs by modifying the update flow in Manager and improving the test utility for proof updates. Key changes include the introduction of a non-panicking error handling mechanism in updateV2TransactionProofs, additions to test utilities to support proof updates without panicking, and updating error messages with ErrInvalidElementProof.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
testutil/proofs.go | Adds an in‑memory state store for testing proof updates; contains a goroutine that still panics on error. |
chain/pool_test.go | Updates pool transaction tests to use the new element state store and verifies error handling for invalid proofs. |
chain/manager.go | Modifies updateV2TransactionProofs to catch panics and return a descriptive error instead. |
.changeset/fixed_a_panic_when_attempting_to_update_element_proofs.md | Documents the change in error handling for element proof updates. |
@lukechampine @ChrisSchinnerl @peterjan merging this to put out a hot fix, but please still review |
switch panicErr := panicErr.(type) { | ||
case string, fmt.Stringer, error: | ||
err = fmt.Errorf("proof update from %q to %q failed at %q with panic %q: %w", from, to, index, panicErr, ErrInvalidElementProof) | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not logging panicErr
at all in the default case, would it no be better to log failed at %q with panic %#v: %w
to ensure we're not needlessly dropping any debug context?
Don't panic when attempting to update proofs and return an error instead.