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

build: add overflow-checks to governance code #4888

Closed
conorsch opened this issue Oct 7, 2024 · 3 comments
Closed

build: add overflow-checks to governance code #4888

conorsch opened this issue Oct 7, 2024 · 3 comments
Assignees
Labels
consensus-breaking breaking change to execution of on-chain data
Milestone

Comments

@conorsch
Copy link
Contributor

conorsch commented Oct 7, 2024

Based on one of the security audits in 2024Q3, we want to add overflow checks to the penumbra-governance crate. We can do this via a minimal diff to the workspace Cargo.toml file:

--- a/Cargo.toml
+++ b/Cargo.toml
@@ -85,6 +85,12 @@ x86_64-unknown-linux-gnu = "buildjet-32vcpu-ubuntu-2204"
 [profile.dist]
 inherits = "release"

+[profile.dev.package.penumbra-governance]
+overflow-checks = true
+
+[profile.release.package.penumbra-governance]
+overflow-checks = true
+
 # config for 'cargo release'
 [workspace.metadata.release]
 # Instruct cargo-release to increment versions for all packages in the workspace in lockstep.

This change was originally submitted as a PR in #4860, but due to its consensus-breaking nature, we opted not to merge it into the v0.80.x release series. When planning the next protocol upgrade, we should revisit this patch, and include it then, with appropriate documentation.

@conorsch conorsch added the consensus-breaking breaking change to execution of on-chain data label Oct 7, 2024
@github-actions github-actions bot added the needs-refinement unclear, incomplete, or stub issue that needs work label Oct 7, 2024
@conorsch conorsch removed the needs-refinement unclear, incomplete, or stub issue that needs work label Oct 7, 2024
@aubrika aubrika added this to the Sprint 15 milestone Oct 21, 2024
@aubrika aubrika moved this to Backlog in Penumbra Oct 21, 2024
@aubrika aubrika added this to Penumbra Oct 21, 2024
@aubrika
Copy link
Contributor

aubrika commented Oct 23, 2024

Kicking this out of this sprint to potentially include in a future upgrade

@aubrika aubrika moved this from In progress to Blocked in Penumbra Oct 23, 2024
@hdevalence
Copy link
Member

I don't think we should plan to do this, ever -- the consensus-critical logic shouldn't be determined by build flags. Instead, if there is critical logic, it should use checked operations.

@redshiftzero
Copy link
Contributor

OK we can use checked operations instead of doing the crate-wide overflow-checks. Closing this one

@github-project-automation github-project-automation bot moved this from Blocked to Done in Penumbra Nov 1, 2024
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
Archived in project
Development

No branches or pull requests

4 participants