Skip to content

Remove validation expect #237

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

Merged
merged 5 commits into from
Apr 11, 2025
Merged

Conversation

Zacholme7
Copy link
Member

Issue Addressed

N/

Proposed Changes

Qbft fuzzing hits the expect. Just propagate option via ? instead of calling expect

@Zacholme7 Zacholme7 added ready-for-review This PR is ready to be reviewed QBFT labels Apr 7, 2025
.operator_ids()
.first()
.expect("Confirmed to exist");
let signer = wrapped_msg.signed_message.operator_ids().first()?;

Choose a reason for hiding this comment

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

Does it happen when operator_ids is empty? If so, should execution get to this point?

Copy link
Member Author

Choose a reason for hiding this comment

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

This should never happen hence the expect. The fuzzing is very targeted.

My thought was that although this should never happen in practice, if the fuzzer can hit it then we should probably just handle it. Behavior is exactly the same.

Maybe there is also an argument that if this happens then we should panic and stop the node.

@jking-aus jking-aus merged commit ded3f0a into sigp:unstable Apr 11, 2025
11 checks passed
@Zacholme7 Zacholme7 deleted the qbft-error-handling branch April 11, 2025 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QBFT ready-for-review This PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants