Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion firewood/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ pub use firewood_storage::logger;
/// In the event of unexpected behavior in testing, disable this first before
/// looking elsewhere.
fn init_logger() {
env_logger::Builder::from_env(env_logger::Env::default().default_filter_or("trace"))
env_logger::Builder::from_env(env_logger::Env::default().default_filter_or("info"))
Copy link
Member Author

Choose a reason for hiding this comment

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

trace logging for all tests is very expensive, noticed a big improvement here.

Sorry to sneak this in to this PR, really it belongs as a separate PR. Complain if you agree and I'll move it.

.is_test(true)
.try_init()
.ok();
Expand Down
62 changes: 57 additions & 5 deletions firewood/src/merkle/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,12 +262,64 @@ impl<T: TrieReader> Merkle<T> {
/// incremental range proof verification
pub fn verify_range_proof(
&self,
_first_key: Option<impl KeyType>,
_last_key: Option<impl KeyType>,
_root_hash: &TrieHash,
_proof: &RangeProof<impl KeyType, impl ValueType, impl ProofCollection>,
first_key: Option<impl KeyType>,
last_key: Option<impl KeyType>,
root_hash: &TrieHash,
proof: &RangeProof<impl KeyType, impl ValueType, impl ProofCollection>,
) -> Result<(), api::Error> {
todo!()
// check that the keys are in ascending order
let key_values = proof.key_values();
if !key_values
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Using is_sorted_by is slightly simpler:

        if !key_values
            .iter()
            .is_sorted_by(|a, b| a.0.as_ref() < b.0.as_ref())

.iter()
.zip(key_values.iter().skip(1))
.all(|(a, b)| a.0.as_ref() < b.0.as_ref())
{
return Err(api::Error::ProofError(
ProofError::NonMonotonicIncreaseRange,
));
}

// check that the start and end proofs are valid
let left = key_values
.first()
.ok_or(api::Error::ProofError(ProofError::Empty))?;
Copy link
Contributor

@demosdemon demosdemon Dec 18, 2025

Choose a reason for hiding this comment

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

This is incorrect. An empty set of key-values is only invalid if both key bounds are empty. An empty set of key-values with a bounded range correctly represents an empty set of key-values between the two keys.


// Verify that first_key (if provided) is <= the first key in the proof
if let Some(ref requested_first) = first_key
&& requested_first.as_ref() > left.0.as_ref()
{
return Err(api::Error::InvalidRange {
start_key: requested_first.as_ref().to_vec().into(),
end_key: left.0.as_ref().to_vec().into(),
});
}

proof
.start_proof()
.verify(&left.0, Some(&left.1), root_hash)?;
Comment on lines +297 to +299
Copy link
Contributor

Choose a reason for hiding this comment

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

This is incorrect. When we generate proofs, we always generate a proof for the lower bound. The proof for the lower bound may or may not prove the first key provided in the range. If the lower bound does not exist, the provided proof may not have all the information required to prove the inclusion of the first key.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like we should perhaps construct the key provided and make sure its <= to the first key.


let right = key_values
.last()
.ok_or(api::Error::ProofError(ProofError::Empty))?;

// Verify that last_key (if provided) is >= the last key in the proof
if let Some(ref requested_last) = last_key
&& requested_last.as_ref() < right.0.as_ref()
{
return Err(api::Error::InvalidRange {
start_key: right.0.as_ref().to_vec().into(),
end_key: requested_last.as_ref().to_vec().into(),
Comment on lines +310 to +311
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

Similar to the first_key validation, this uses InvalidRange incorrectly. The start_key is set to right.0 and end_key to requested_last, which reverses the logical ordering and misrepresents the nature of the error (proof boundary validation vs request validation).

Suggested change
start_key: right.0.as_ref().to_vec().into(),
end_key: requested_last.as_ref().to_vec().into(),
start_key: requested_last.as_ref().to_vec().into(),
end_key: right.0.as_ref().to_vec().into(),

Copilot uses AI. Check for mistakes.
});
}

proof
.end_proof()
.verify(&right.0, Some(&right.1), root_hash)?;
Comment on lines +315 to +317
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly here, when we generate the upper bound proof, we generate a proof for the final key in the yielded range only if we truncated the range. Otherwise, we generate a proof for the requested upper bound. This proof may or may not be sufficient to verify the final yielded key.


// TODO: build a merkle and reshape it, filling in hashes from the
// provided proofs on the left and right edges, then verify the root hash

Ok(())
}

/// Merges a sequence of key-value pairs with the base merkle trie, yielding
Expand Down
9 changes: 6 additions & 3 deletions firewood/src/merkle/tests/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ fn test_missing_key_proof() {
#[test]
// Tests normal range proof with both edge proofs as the existent proof.
// The test cases are generated randomly.
#[ignore = "https://github.com/ava-labs/firewood/issues/738"]
fn test_range_proof() {
let rng = firewood_storage::SeededRng::from_env_or_random();

Expand Down Expand Up @@ -69,7 +68,6 @@ fn test_range_proof() {
#[test]
// Tests a few cases which the proof is wrong.
// The prover is expected to detect the error.
#[ignore = "https://github.com/ava-labs/firewood/issues/738"]
fn test_bad_range_proof() {
Copy link
Contributor

@RodrigoVillar RodrigoVillar Dec 18, 2025

Choose a reason for hiding this comment

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

(q / feel-free-to-ignore): I wonder if it's cleaner to split this test into two - one test that tests with verification and one test that doesn't. Having the 'skip_test loop label is hard to follow considering that we also have continue statements which should be kept around.

let rng = firewood_storage::SeededRng::from_env_or_random();

Expand All @@ -78,7 +76,7 @@ fn test_bad_range_proof() {
items.sort_unstable();
let merkle = init_merkle(items.clone());

for _ in 0..10 {
'skip_test: for _ in 0..10 {
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The label 'skip_test' is misleading since it doesn't skip the test itself, but rather continues to the next iteration. Consider renaming to 'next_iteration' or 'retry' to better reflect its purpose.

Suggested change
'skip_test: for _ in 0..10 {
'next_iteration: for _ in 0..10 {

Copilot uses AI. Check for mistakes.
let start = rng.random_range(0..items.len());
let end = rng.random_range(0..items.len() - start) + start - 1;

Expand All @@ -104,10 +102,12 @@ fn test_bad_range_proof() {
0 => {
// Modified key
keys[index] = rng.random::<[u8; 32]>(); // In theory it can't be same
continue 'skip_test;
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

These continue statements skip validation of intentionally corrupted proofs. The test cases for modified keys, modified values, gapped entries, empty keys, and nil values are no longer being verified as invalid, which defeats the purpose of test_bad_range_proof. These should either be properly validated or the test should remain ignored until full verification is implemented.

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

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

The whole point of this PR is to enable the parts of the test that do work, and out of order is an integral part of this test. The others will get skipped.

}
1 => {
// Modified val
vals[index] = rng.random::<[u8; 20]>(); // In theory it can't be same
continue 'skip_test;
}
2 => {
// Gapped entry slice
Expand All @@ -116,6 +116,7 @@ fn test_bad_range_proof() {
}
keys.remove(index);
vals.remove(index);
continue 'skip_test;
}
3 => {
// Out of order
Expand All @@ -130,10 +131,12 @@ fn test_bad_range_proof() {
4 => {
// Set random key to empty, do nothing
keys[index] = [0; 32];
continue 'skip_test;
}
5 => {
// Set random value to nil
vals[index] = [0; 20];
continue 'skip_test;
}
_ => unreachable!(),
}
Expand Down