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

governance: add overflow-checks crate-wide #4860

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

redshiftzero
Copy link
Member

Describe your changes

Adds a guard against underflow/overflows for the penumbra-governance crate

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    Provided that there are not underflows/overflows, this should not be consensus-breaking

this is an easier option than modifying the entire crate to use
overflow-aware arithmetic
@conorsch
Copy link
Contributor

Provided that there are not underflows/overflows, this should not be consensus-breaking

What kind of additional assurance can we perform to ensure that there are not underflows or overflows in the wild? Would full confidence require syncing a node built with the Cargo.toml from this PR against e.g. mainnet data?

@conorsch conorsch added the consensus-breaking breaking change to execution of on-chain data label Sep 17, 2024
@conorsch conorsch self-requested a review September 17, 2024 18:02
Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Requesting changes to block merge, since technically this is a consensus-breaking diff. Let's return to this diff once there are plans for a chain upgrade, at which time it would be acceptable to ship breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus-breaking breaking change to execution of on-chain data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants