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

Witness properly handles ratcheting forward from tree size 0 #266

Merged
merged 3 commits into from
Sep 19, 2024

Conversation

mhutchinson
Copy link
Contributor

@mhutchinson mhutchinson commented Sep 18, 2024

The tests show that this worked, providing that the proof was empty. When the merkle library is updated to pull in transparency-dev/merkle#140, this will fail too. In both of the new test cases, the witness should ratchet forward when a new checkpoint is provided.

The second commit in this PR updates the witness to handle transitions forward from tree size 0 properly. The tests have been updated accordingly. Updating the Merkle library should now be possible, because we can't reach the VerifyConsistency call if the starting tree size is 0.

As a separate note, I recommend rewriting these tests at some near point in the future. They are currently quite brittle as they rely on hard coded test data with no obvious script to update them. An in-memory test log would be a great way to generate the test data in a flexible way.

This fixes #265.

The tests show that this currently works, providing that the proof is empty. When the merkle library is updated to pull in transparency-dev/merkle#140, this will fail too. In both of the new test cases, the witness should ratchet forward when a new checkpoint is provided.

As a separate note, I recommend rewriting these tests at some near point in the future. They are currently quite brittle as they rely on hard coded test data with no obvious script to update them. An in-memory test log would be a great way to generate the test data in a flexible way.
@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 33.55%. Comparing base (3c58af4) to head (75fe8a6).
Report is 103 commits behind head on main.

Files with missing lines Patch % Lines
internal/witness/witness.go 50.00% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #266       +/-   ##
===========================================
- Coverage   51.05%   33.55%   -17.51%     
===========================================
  Files          11       23       +12     
  Lines         903     1383      +480     
===========================================
+ Hits          461      464        +3     
- Misses        374      822      +448     
- Partials       68       97       +29     

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

@mhutchinson
Copy link
Contributor Author

The diff looks bigger than it really is because the removal of the unused test struct field initSize has caused a lot of indenting to shift.

The only way to handle this is via a special case.
@mhutchinson mhutchinson changed the title Added tests for witnesses updating from tree size 0 Witness properly handles ratcheting forward from tree size 0 Sep 18, 2024
@mhutchinson mhutchinson merged commit 472156f into transparency-dev:main Sep 19, 2024
8 checks passed
@mhutchinson mhutchinson deleted the b265-testsForSize0 branch September 19, 2024 10:07
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.

Review witness handling of empty log states
3 participants