[storage/mmr/verification.rs] remove Error being returned from proof verification functions #1164

roberto-bayardo · 2025-06-27T17:03:13Z

Having these functions potentially return Error is a holdover from when we had them "async" due to how grafting was previously implemented. This is no longer necessary, errors would have never been returned, since the only possible errors (MissingDigest/ExtraDigests) get captured and converted to verification failures (false).

Copilot

Pull Request Overview

This PR removes the now-unnecessary possibility of returning errors from proof verification functions by converting them to simple boolean results and refactoring related error types. Key changes include:

Changing function signatures in proof verification from Result<bool, Error> to bool.
Introducing a new ReconstructionError type and updating its usage across MMR proof reconstruction.
Updating panic messages and format strings to use Rust’s new inline formatting syntax.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
storage/src/mmr/verification.rs	Updates verification functions to return bool and use ReconstructionError
storage/src/mmr/mod.rs	Removes unused Error variants related to proof verification
storage/src/mmr/mem.rs, hasher.rs	Standardizes panic message formatting
storage/src/mmr/bitmap.rs	Adjusts verify_bit_inclusion and related tests to new bool return type
storage/src/adb/current.rs, any.rs	Refactors proof verification usage reflecting new return types
storage/src/mmr/benches/*.rs	Updates bench tests to work with simplified verification function outputs

Comments suppressed due to low confidence (3)

storage/src/mmr/verification.rs:89

Consider updating the documentation for verify_element_inclusion to explicitly note that error conditions are now handled internally and that the function returns false instead of a Result.

    ) -> bool

storage/src/mmr/verification.rs:142

Please update the doc comment for reconstruct_root to clearly state that it now returns a ReconstructionError on invalid input rather than a generic Error.

    ) -> Result<D, ReconstructionError>

storage/src/adb/current.rs:477

It would help to update the function's documentation here to clarify that verification now yields a boolean outcome instead of propagating detailed errors.

    ) -> bool {

danlaine · 2025-06-27T17:25:42Z

I agree that it doesn't make sense to have both a boolean (in the Ok) and an error in the signature, since both false and Err signify "this proof isn't valid."

Would it make more sense to change the signature to Result<(), Error> so the caller knows why the proof verification failed?

roberto-bayardo · 2025-06-27T17:31:44Z

I agree that it doesn't make sense to have both a boolean (in the Ok) and an error in the signature, since both false and Err signify "this proof isn't valid."

Would it make more sense to change the signature to Result<(), Error> so the caller knows why the proof verification failed?

My expectation is the caller doesn't typically care about the reason -- an adversary sending you bogus data is sort of expected behavior and there's generally no need to treat the types of bogus data differently. If you do want the reason, you can always use the lower level reconstruction API. (I thought it would be useful exposing at that level for debugging code issues.) So I prefer the simple bool.

danlaine

Personally, I think Result<(), Error> is a bit more rusty, but not a sticking point for me. I think this is still an improvement over the existing code. Left some nits, otherwise LGTM.

danlaine · 2025-06-27T17:34:43Z

storage/src/adb/current.rs

+            Err(ReconstructionError::MissingDigests) => {
                debug!("Not enough digests in proof to reconstruct root");
-                return Ok(false);
+                return false;
            }
-            Err(ExtraDigests) => {
+            Err(ReconstructionError::ExtraDigests) => {
                debug!("Not all digests in proof were used to reconstruct root");
-                return Ok(false);
+                return false;


nit: we could do Err(e) and just print the error text instead of enumerating the cases

danlaine · 2025-06-27T17:35:41Z

storage/src/adb/current.rs

+            Err(ReconstructionError::MissingDigests) => {
                debug!("Not enough digests in proof to reconstruct root");
-                return Ok(false);
+                return false;
            }
-            Err(ExtraDigests) => {
+            Err(ReconstructionError::ExtraDigests) => {
                debug!("Not all digests in proof were used to reconstruct root");
-                return Ok(false);
+                return false;


Same comment as above about Err(e) vs enumerating all error cases

danlaine · 2025-06-27T17:36:24Z

storage/src/mmr/bitmap.rs

+            Err(ReconstructionError::MissingDigests) => {
                debug!("Not enough digests in proof to reconstruct root");
-                return Ok(false);
+                return false;
            }
-            Err(ExtraDigests) => {
+            Err(ReconstructionError::ExtraDigests) => {
                debug!("Not all digests in proof were used to reconstruct root");
-                return Ok(false);
+                return false;


Same comment as above about Err(e) vs enumerating all error cases

danlaine · 2025-06-27T17:37:30Z

storage/src/mmr/verification.rs

-            Ok(reconstructed_root) => Ok(*root_digest == reconstructed_root),
-            Err(MissingDigests) => {
+            Ok(reconstructed_root) => *root_digest == reconstructed_root,
+            Err(ReconstructionError::MissingDigests) => {


Same comment as above about Err(e) vs enumerating all error cases

danlaine · 2025-06-27T17:38:50Z

storage/src/mmr/verification.rs


+/// Errors that can occur when reconstructing a digest from a proof due to invalid input.
+#[derive(Error, Debug)]
+pub enum ReconstructionError {


It doesn't seem like this will ever be exposed outside the crate. If that's the case:

Suggested change

pub enum ReconstructionError {

pub(crate) enum ReconstructionError {

it's used by Bitmap which uses the lower level API to implement its special verification rules.

oh I guess crate is storage:: .. yeah makes sense. changed.

danlaine · 2025-06-27T17:40:34Z

storage/src/mmr/verification.rs


    /// Reconstructs the root digest of the MMR from the digests in the proof and the provided range
-    /// of elements.
+    /// of elements, or returns a `ReconstructionError` if the input data is invalid.


Nit:

Suggested change

/// of elements, or returns a `ReconstructionError` if the input data is invalid.

/// of elements, or returns a [ReconstructionError] if the input data is invalid.

I realize there are many instances of using using \TypeName`instead of[TypeName]` across the codebase, but I think it makes sense to move in the direction of using links to type definitions.

yup, changed (and fixed a few others as well).

roberto-bayardo · 2025-06-27T18:50:02Z

Personally, I think Result<(), Error> is a bit more rusty, but not a sticking point for me. I think this is still an improvement over the existing code. Left some nits, otherwise LGTM.

Perhaps @patrick-ogrady has an opinion on this. I don't see this as much of a rusty/non-rusty thing but a "don't burden the user with an unnecessarily complex API" issue, but don't feel super strongly.

codecov · 2025-06-27T20:28:49Z

Codecov Report

Attention: Patch coverage is 87.82288% with 33 lines in your changes missing coverage. Please review.

Project coverage is 91.02%. Comparing base (3a2774e) to head (9c29aa0).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
storage/src/adb/current.rs	65.11%	15 Missing ⚠️
storage/src/mmr/bitmap.rs	75.00%	6 Missing ⚠️
storage/src/mmr/verification.rs	94.33%	6 Missing ⚠️
storage/src/adb/any.rs	75.00%	2 Missing ⚠️
storage/src/mmr/hasher.rs	97.26%	2 Missing ⚠️
storage/src/mmr/mem.rs	50.00%	2 Missing ⚠️

@@            Coverage Diff             @@
##             main    #1164      +/-   ##
==========================================
- Coverage   91.02%   91.02%   -0.01%     
==========================================
  Files         215      215              
  Lines       57457    57409      -48     
==========================================
- Hits        52300    52256      -44     
+ Misses       5157     5153       -4

Files with missing lines	Coverage Δ
storage/src/mmr/journaled.rs	`98.05% <100.00%> (-0.02%)`	⬇️
storage/src/adb/any.rs	`99.11% <75.00%> (-0.01%)`	⬇️
storage/src/mmr/hasher.rs	`87.47% <97.26%> (+0.62%)`	⬆️
storage/src/mmr/mem.rs	`92.37% <50.00%> (ø)`
storage/src/mmr/bitmap.rs	`96.70% <75.00%> (+0.21%)`	⬆️
storage/src/mmr/verification.rs	`95.16% <94.33%> (-0.12%)`	⬇️
storage/src/adb/current.rs	`96.21% <65.11%> (+0.04%)`	⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a2774e...9c29aa0. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

roberto-bayardo · 2025-06-27T20:29:24Z

Personally, I think Result<(), Error> is a bit more rusty, but not a sticking point for me. I think this is still an improvement over the existing code. Left some nits, otherwise LGTM.

Perhaps @patrick-ogrady has an opinion on this. I don't see this as much of a rusty/non-rusty thing but a "don't burden the user with an unnecessarily complex API" issue, but don't feel super strongly.

Went ahead and merged this so we can unblock Dan's PR. If Patrick likes the return value to be an Error maybe we can put that change in yours @danlaine ?

roberto-bayardo requested review from Copilot and danlaine June 27, 2025 17:03

This comment was marked as outdated.

Sign in to view

roberto-bayardo force-pushed the remove-error-from-verification branch from a830850 to ca8eef1 Compare June 27, 2025 17:05

remove Error being returned from proof verification functions

2a6bff3

roberto-bayardo force-pushed the remove-error-from-verification branch from ca8eef1 to 2a6bff3 Compare June 27, 2025 17:08

roberto-bayardo requested a review from Copilot June 27, 2025 17:08

Copilot AI reviewed Jun 27, 2025

View reviewed changes

roberto-bayardo mentioned this pull request Jun 27, 2025

[storage] Require proofs have at least 1 operation #1162

Closed

roberto-bayardo marked this pull request as ready for review June 27, 2025 17:14

roberto-bayardo requested a review from patrick-ogrady June 27, 2025 17:14

danlaine reviewed Jun 27, 2025

View reviewed changes

roberto-bayardo force-pushed the remove-error-from-verification branch from ac730ba to 7ce93b4 Compare June 27, 2025 19:03

roberto-bayardo requested a review from danlaine June 27, 2025 19:05

address review

9c29aa0

roberto-bayardo force-pushed the remove-error-from-verification branch from 7ce93b4 to 9c29aa0 Compare June 27, 2025 19:10

danlaine approved these changes Jun 27, 2025

View reviewed changes

roberto-bayardo merged commit fb59cd8 into main Jun 27, 2025
17 of 18 checks passed

roberto-bayardo deleted the remove-error-from-verification branch June 27, 2025 20:28

	pub enum ReconstructionError {
	pub(crate) enum ReconstructionError {

	/// of elements, or returns a `ReconstructionError` if the input data is invalid.
	/// of elements, or returns a [ReconstructionError] if the input data is invalid.

[storage/mmr/verification.rs] remove Error being returned from proof verification functions #1164

[storage/mmr/verification.rs] remove Error being returned from proof verification functions #1164

Uh oh!

Conversation

roberto-bayardo commented Jun 27, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

danlaine commented Jun 27, 2025

Uh oh!

roberto-bayardo commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danlaine left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roberto-bayardo commented Jun 27, 2025

Uh oh!

Uh oh!

codecov bot commented Jun 27, 2025

Codecov Report

Uh oh!

roberto-bayardo commented Jun 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

roberto-bayardo commented Jun 27, 2025 •

edited

Loading