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

Skip the panic reason from canonical serialization #826

Merged
merged 3 commits into from
Sep 16, 2024

Conversation

xgreenx
Copy link
Collaborator

@xgreenx xgreenx commented Sep 13, 2024

Related specification PR: FuelLabs/fuel-specs#609

The main reason for removing it from canonical serialization is to avoid affecting the block ID for different panic reasons. The primary goal is just to know that some operation was done incorrectly; what exactly failed is less important. It reduces the chance of getting different blocks within one panic.

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself

Dentosal
Dentosal previously approved these changes Sep 13, 2024
@xgreenx xgreenx requested a review from a team September 13, 2024 11:38
xgreenx added a commit to FuelLabs/fuel-specs that referenced this pull request Sep 13, 2024
The main reason for removing it from canonical serialization is to avoid
affecting the block ID for different panic reasons. The primary goal is
just to know that some operation was done incorrectly; what exactly
failed is less important. It reduces the chance of getting different
blocks within one panic.

Related VM change: FuelLabs/fuel-vm#826

### Before requesting review
- [x] I have reviewed the code myself
netrome
netrome previously approved these changes Sep 16, 2024
rafal-ch
rafal-ch previously approved these changes Sep 16, 2024
Copy link
Contributor

@rafal-ch rafal-ch left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@xgreenx xgreenx added this pull request to the merge queue Sep 16, 2024
Merged via the queue into master with commit e75f4e8 Sep 16, 2024
39 checks passed
@xgreenx xgreenx deleted the feature/skip-panic-reason-from-canonical-sede branch September 16, 2024 15:12
@xgreenx xgreenx mentioned this pull request Sep 17, 2024
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.

4 participants