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

fix: guarantee that max prefix length is < min prefix length + child size #369

Merged

Conversation

crodriguezvega
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Sep 10, 2024

Codecov Report

Attention: Patch coverage is 96.15385% with 1 line in your changes missing coverage. Please review.

Project coverage is 67.76%. Comparing base (22a0173) to head (2b6f34f).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
rust/src/verify.rs 96.15% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #369      +/-   ##
==========================================
+ Coverage   67.51%   67.76%   +0.24%     
==========================================
  Files           7        7              
  Lines        3014     3037      +23     
==========================================
+ Hits         2035     2058      +23     
  Misses        979      979              
Flag Coverage Δ
rust 67.76% <96.15%> (+0.24%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

Nice work! Ideally we would also have a test that malleates the prefix in order to fool the proof system that now fails. You should be able to pull out the test case from the audit report so don't have to create yourself

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Agree that we should have the malleate test case (also a unit test for go code?). The audit recommended updating the proto documentation to indicate this restriction as well

(also looks like rust formatting needs to be fixed)

rust/src/verify.rs Outdated Show resolved Hide resolved
@colin-axner
Copy link
Contributor

requesting review from @romac to verify the rust changes

@colin-axner colin-axner self-assigned this Sep 25, 2024
@colin-axner colin-axner marked this pull request as draft September 25, 2024 10:11
@colin-axner
Copy link
Contributor

colin-axner commented Sep 25, 2024

gonna mark as draft until:

@romac
Copy link
Member

romac commented Sep 25, 2024

requesting review from @romac to verify the rust changes

LGTM, just needs a cargo fmt --all

@@ -166,6 +166,10 @@ func (op *InnerOp) CheckAgainstSpec(spec *ProofSpec, b int) error {
return errors.New("spec.InnerSpec.ChildSize must be >= 1")
}

if spec.InnerSpec.MaxPrefixLength >= spec.InnerSpec.MinPrefixLength+spec.InnerSpec.ChildSize {
Copy link
Contributor

Choose a reason for hiding this comment

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

for what it is worth. Spec fields being validated against themselves are better off in a spec.Validate() function, but for the sake of minimizing changes, this approach makes sense to me

@colin-axner colin-axner marked this pull request as ready for review September 26, 2024 08:32
@colin-axner colin-axner removed their assignment Sep 26, 2024
@colin-axner colin-axner merged commit 7000936 into master Sep 26, 2024
12 checks passed
@colin-axner colin-axner deleted the carlos/maxprefixlength-less-minprefixlength-plus+childsize branch September 26, 2024 13:52
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.

5 participants