feat: add validation to PartialMmr deserialization and from_parts#812
feat: add validation to PartialMmr deserialization and from_parts#812Farukest wants to merge 3 commits into0xMiden:nextfrom
PartialMmr deserialization and from_parts#812Conversation
Farukest
force-pushed
the
feat/partial-mmr-validation
branch
from
61461bb to
0724921
Compare
| ) -> Result<Self, MmrError> { | ||
| let forest = peaks.forest(); | ||
|
|
||
| // Validate track_latest: can only be true if forest has an odd element (single leaf tree) |
There was a problem hiding this comment.
The new check rejects inputs where track_latest=true but the forest has no single-leaf tree.
This breaks old data that set the flag loosely.
There was a problem hiding this comment.
This validation is intentional for security (untrusted deserialization from #802).
If track_latest=true but there's no single-leaf tree, the data is logically inconsistent, there's no "latest leaf" to track.
For trusted sources or migration paths, from_parts_unchecked() is available which skips all validation.
Should we add a note in the docs about this breaking change for invalid data ?
There was a problem hiding this comment.
We should perhaps just mark the change as breaking in the Changelog.
| let result = PartialMmr::from_parts(peaks_even.clone(), BTreeMap::new(), false); | ||
| assert!(result.is_ok()); | ||
|
|
||
| // Invalid case: node index out of bounds |
There was a problem hiding this comment.
The test only checks leaf nodes. Please add tests for:
- Index 0 (invalid)
- Large even indices beyond the forest range
- Bad inputs during deserialization
These matter because internal nodes are used for auth paths.
Farukest
force-pushed
the
feat/partial-mmr-validation
branch
from
2f03f9f to
d9d8176
Compare
|
All done @huitseeker. kindly need review again. |
huitseeker
left a comment
huitseeker
left a comment
There was a problem hiding this comment.
This is shaping up, thanks!
| // For an empty forest, no nodes are valid | ||
| if !nodes.is_empty() && forest.is_empty() { | ||
| return Err(MmrError::InconsistentPartialMmr( | ||
| "nodes present but forest is empty".into(), |
There was a problem hiding this comment.
The current validation only checks that indices are within rightmost_in_order_index(), but this includes "separator" positions between trees in multi-tree forests. For example, in a forest with 7 leaves (0b111), index 6 falls between trees but would pass validation even though it does not correspond to a real MMR node.
These separator indices are not valid for NodeMap. See:
track()only inserts atidx.sibling()whereidxstarts from a valid leaf positionadd()only inserts at positions derived fromrightmost_in_order_index()and its sibling- The
NodeMapdocumentation states it contains "authentication nodes" which are always real tree nodes, never separators
Consider adding a Forest::is_valid_in_order_index() helper that checks if an index falls within an actual tree span rather than a separator gap.
| let result = PartialMmr::from_parts(peaks.clone(), BTreeMap::new(), true); | ||
| assert!(result.is_ok()); | ||
|
|
||
| // Build an MMR with 8 leaves (no single leaf tree: 0b1000) |
There was a problem hiding this comment.
The tests don't cover the case where an index is in-range but points to a separator between trees. See the comment above on that topic.
| assert!(result.is_err()); | ||
|
|
||
| // Invalid case: index 0 (which is never valid for InOrderIndex) | ||
| let mut nodes_with_zero = BTreeMap::new(); |
There was a problem hiding this comment.
The comment says the rightmost in-order index for 7 leaves is 12, but Forest::new(0b0111).rightmost_in_order_index() returns 13. The test at line 191 in tests.rs confirms this. Please fix or remove the comment to avoid confusion.
| ) -> Result<Self, MmrError> { | ||
| let forest = peaks.forest(); | ||
|
|
||
| // Validate track_latest: can only be true if forest has an odd element (single leaf tree) |
There was a problem hiding this comment.
We should perhaps just mark the change as breaking in the Changelog.
|
All done @huitseeker 👍 |
This commit adds validation to `PartialMmr::from_parts()` and the `Deserializable` implementation to ensure consistency between components: - Validates that `track_latest` is only true when forest has a single leaf tree - Validates that all node indices are within forest bounds - Adds `from_parts_unchecked()` for performance-critical trusted code paths - Updates `Deserializable` to use the validating constructor This addresses security concerns when deserializing from untrusted sources. Closes 0xMiden#802
Address review feedback: - Reject index 0 as invalid (InOrderIndex starts at 1) - Check all indices against forest.rightmost_in_order_index() - Handle empty forest case explicitly - Add tests for index 0, large even indices, and deserialization
Farukest
force-pushed
the
feat/partial-mmr-validation
branch
from
9f3ce04 to
fbd1ce9
Compare
- Add Forest::is_valid_in_order_index() to check if an index points to an actual node (not a separator position between trees) - Update from_parts() to reject separator indices - Add tests for separator index validation (indices 8 and 12 for 7-leaf forest) - Fix comment: rightmost in-order index for 7 leaves is 13, not 12 - Mark PR as [BREAKING] in CHANGELOG
Farukest
force-pushed
the
feat/partial-mmr-validation
branch
from
fbd1ce9 to
f78b142
Compare
2 participants
Summary
PartialMmr::from_parts()to ensure consistency between componentsfrom_parts_unchecked()for performance-critical trusted code pathsDeserializableimplementation to use the validating constructorValidation Checks
track_latestcan only betruewhen forest has a single leaf treeNote
PartialMmr::from_partsandDeserializabledid not validate consistency between components. This is a security concern when deserializing from untrusted sources (e.g., viaProvenTransaction).Changes
InconsistentPartialMmrerror variantfrom_parts()now returnsResult<Self, MmrError>with validationfrom_parts_unchecked()for trusted/performance-critical pathsDeserializablenow validates viaMmrPeaks::new()andfrom_parts()Closes #802