enhance(LargeSmtForest tests): expand LargeSmtForest property test coverage#886
enhance(LargeSmtForest tests): expand LargeSmtForest property test coverage#886AlexWaker wants to merge 18 commits into0xMiden:nextfrom
Conversation
AlexWaker
changed the title
| let bad_version = forest.update_tree(lineage, version, extra_entries); | ||
| prop_assert!(bad_version.is_err()); | ||
| prop_assert_eq!( | ||
| bad_version.unwrap_err().to_string(), |
There was a problem hiding this comment.
Would it be safer to assert the error variant instead of exact to_string() text? Matching full message strings can make this test fail on wording-only changes.
|
Resolved the merge conflict by aligning property_tests.rs with the upstream API changes, including the new fallible entries() usage. Also addressed the two review comments:
Finally, ran formatting checks and applied rustfmt fixes (cargo +nightly fmt --all and cargo +nightly fmt --all --check). |
|
Would this PR need a changelog entry? My understanding is that it probably does not, since this change only expands internal test coverage and adapts the tests to upstream API changes, without introducing any user-facing behavior change or public API change. From the current CHANGELOG.md, it also seems like entries are generally reserved for externally visible features, fixes, and breaking changes. That said, if you would still prefer one to be added, I can update the changelog right away. |
huitseeker
added
the
no changelog
huitseeker
left a comment
huitseeker
left a comment
There was a problem hiding this comment.
This is shaping up nicely! See comments inline.
| TreeId::new(lineage, version), | ||
| &tree_v1, | ||
| &sample_keys, | ||
| false, |
There was a problem hiding this comment.
This helper skips open() on the historical tree by passing false here. Can we also property-test historical open() against the reference Smt? That feels like the trickiest query path in LargeSmtForest, and right now it still looks like it only has the hand-written unit test coverage in tests.rs.
There was a problem hiding this comment.
Yeah this should 100% test historical openings as well.
| @@ -0,0 +1 @@ | |||
| cc abf493f7aece0d6db6c370e3c95651effc9f46f7a2f97e2dccddacfce2afd16d No newline at end of file | |||
There was a problem hiding this comment.
I don't see any behavioral fixes in this PR. We usually only keep regression seeds like this when they allowed us to identify an interesting failure, which required fixing. This seems like it was just a development artifact and so does not need keeping.
There was a problem hiding this comment.
Looks reasonable in general! I've left a few comments in-line.
One thing I would love to see for every single potential modification is that we assert complete correctness with regards to the state change. Some of these—like the one @huitseeker commented on—do not assert the complete soundness of all propertiess should a bug arise.
Perhaps this is out of scope for this PR, but I would also be interested in expanding the See #911 for a separate batch of work in this vein.FallibleEntriesBackend to be a more general mechanism for testing state coherence in the face of a number of different kinds of failure. @huitseeker, thoughts?
| }; | ||
|
|
||
| // ENTRIES | ||
| // HELPERS |
There was a problem hiding this comment.
All of these helpers should be moved to large_forest::test_utils like the other strategies are. I would also expect them to have doc comments, as that is the first line of understanding for someone reading the property test after a failure.
|
|
||
| fn apply_batch(tree: &mut Smt, batch: SmtUpdateBatch) -> core::result::Result<(), TestCaseError> { | ||
| let mutations = tree | ||
| .compute_mutations(Vec::<(Word, Word)>::from(batch).into_iter()) |
There was a problem hiding this comment.
This should call batch.consume().into_iter() instead. That guarantees the correct deduplication behaviouor.
| } | ||
|
|
||
| fn sorted_forest_entries( | ||
| forest: &LargeSmtForest<ForestInMemoryBackend>, |
There was a problem hiding this comment.
Realistically, this and all subsequent cases that take a forest should be parametric in the backend. While we currently only do these with the in-memory backend, there is no reason that has to remain the case forever.
| .collect::<crate::merkle::smt::large_forest::Result<Vec<_>>>() | ||
| .map_err(to_fail)? | ||
| .into_iter() | ||
| .sorted() |
There was a problem hiding this comment.
What is the purpose of sorting them? Does the default derived Ord implementation order them correctly?
| Ok(()) | ||
| } | ||
|
|
||
| // PROPERTY TESTS |
There was a problem hiding this comment.
In general, please reorder these property tests to match the method order in the large forest itself. It makes for easier code navigation.
| TreeId::new(lineage, version), | ||
| &tree_v1, | ||
| &sample_keys, | ||
| false, |
There was a problem hiding this comment.
Yeah this should 100% test historical openings as well.
|
Ended up filing #911 for my ask above. |
|
Thanks for the review. I’ve addressed the suggestions raised in this PR, with the exception of the broader failure-injection work described in #911, which I have not tackled in this patch. Changes made in this round:
I also expanded the property coverage for historical
My current understanding is that this was a real bug exposed by the stronger historical After these changes, the targeted I have not implemented the more general failure-injection / “FallibleBackend” work from #911 in this PR, and I’m leaving that follow-up scoped to the separate issue. |
huitseeker
left a comment
huitseeker
left a comment
There was a problem hiding this comment.
Thanks a lot for this update, this looks super nice!
There was a problem hiding this comment.
Good catch on the bug, but we definitely need a more principled fix that doesn't incur unnecessary performance penalties for correctness.
Once my suggested fix is applied, I'd also like to see comparative benchmarks for before and after this commit, now that hot-path code has been touched.
| let sibling_key = Word::new([ | ||
| Felt::new(0), | ||
| Felt::new(0), | ||
| Felt::new(0), | ||
| Felt::new(sibling_leaf_index.position()), | ||
| ]); |
There was a problem hiding this comment.
This seems like a hack to me. My suggestion would be to add a new method to Backend that is get_leaf that simply gets the entire leaf for a given leaf index. That would also be a lot more performant than having to do the work to compute a full opening in the backend, which can be quite expensive.
Something like this.
fn get_leaf(&self, index: LeafIndex<SMT_DEPTH>) -> Option<SmtLeaf>;| // Third item should be the simulated error. | ||
| let third = iter.next(); | ||
| assert_matches!(&third, Some(Err(LargeSmtForestError::Unspecified(msg))) if msg == "simulated read failure"); | ||
| assert_matches!(&third, Some(Err(LargeSmtForestError::Unspecified(_)))); |
There was a problem hiding this comment.
I don't think removing the message assertion is the correct thing to do. We might get spurious passes for other errors that way. I would create a constant for the error message and use it at the emit site and then check for it here in the test.
| // This takes the WithHistory iteration path, which will hit our fallible iterator. | ||
| let result = forest.entry_count(TreeId::new(lineage, version_1)); | ||
| assert_matches!(result, Err(LargeSmtForestError::Unspecified(msg)) if msg == "simulated read failure"); | ||
| assert_matches!(result, Err(LargeSmtForestError::Unspecified(_))); |
iamrecursion's suggestions
| Ok(tree.tree.open(&key)) | ||
| } | ||
|
|
||
| /// Returns the leaf stored at `leaf_index` in the SMT with the specified `lineage`. |
There was a problem hiding this comment.
This comment should also mention something about it explicitly always returning a leaf, even if said leaf is not stored explicitly.
There was a problem hiding this comment.
Oh, I just saw your new comment. I’ve been busy running benchmark tests, but I’ll fix your latest suggestion shortly.
| } | ||
|
|
||
| path.push(if is_right { left } else { right }); | ||
| /// Returns the leaf stored at `leaf_index` in the SMT with the specified `lineage`. |
| let key = Word::new([ | ||
| EMPTY_WORD[0], | ||
| EMPTY_WORD[1], | ||
| EMPTY_WORD[2], | ||
| Felt::new(leaf_index.position()), | ||
| ]); |
There was a problem hiding this comment.
This is still not the way to do it. You should add a load_leaf_raw akin to this that you would just call with LeafKey { lineage, index: leaf_index.position() }.
/// Gets the leaf with the provided `key` from disk, or returns [`None`] if it is not stored.
///
/// # Errors
///
/// - [`BackendError::Internal`] if the database cannot be successfully queried.
#[inline(always)]
fn load_leaf_raw(&self, key: &LeafKey) -> Result<Option<SmtLeaf>> {
let col = self.cf(LEAVES_CF)?;
let key_bytes = key.to_bytes();
let leaf_bytes = self.db.get_cf(col, key_bytes)?;
let leaf = match leaf_bytes {
Some(bytes) => Some(SmtLeaf::read_from_bytes_with_budget(&bytes, bytes.len())?),
None => None,
};
Ok(leaf)
}You can then use this inside load_leaf_for to avoid code duplication:
/// Gets the leaf from disk in the provided `lineage` that would contain `key`.
///
/// # Errors
///
/// - [`BackendError::Internal`] if the database cannot be successfully queried.
fn load_leaf_for(&self, lineage: LineageId, key: Word) -> Result<Option<SmtLeaf>> {
let key = LeafKey {
lineage,
index: LeafIndex::from(key).position(),
};
self.load_leaf_raw(&key)
}The current approach is too directly tied to the internal representation and is brittle, while this is principled.
| // top of the current state. | ||
| let new_leaf = self.merge_leaves(opening.leaf(), view)?; | ||
| let new_leaf = Self::merge_leaves(opening.leaf(), view)?; | ||
| let new_path = self.merge_paths(tree.lineage(), leaf_index, opening.path(), view)?; |
There was a problem hiding this comment.
Not technically part of this PR, but merge_paths should also be a static method rather than an instance method. It never touches self. Would you be able to fix that?
| let mut entries = leaf_entries.into_iter().collect::<Vec<_>>(); | ||
| // We sort the entries to ensure a consistent ordering, as the map above is a HashMap | ||
| // which does not guarantee iteration order. | ||
| let mut entries: Vec<_> = leaf_entries.into_iter().collect(); |
There was a problem hiding this comment.
Please use the turbofish style for the collect type.
|
Thanks for the suggestion @iamrecursion . I addressed this by adding a In particular:
I also reran the existing
I think this level of regression is acceptable. |
|
Hm. That regression is small, sure, but Are there regressions in any other benchmarks or just the open ones? |
I just addressed the inline comments. I haven’t had time yet to dig deeply into the performance regression, but my initial assessment is that it may be caused by repeated scans of the historical change set. I’ll continue with a deeper analysis and try to fix it shortly |
|
A fix would be appreciated as this is very hot-path code. |
I reran the broader The benchmarks I checked were:
For reference, these were the median times I got across the repeated runs. This branch:
Across those repeated runs, I did not see evidence of a broad regression. The only regression signal that remained reasonably consistent was on My current understanding is that this matches the code changes: the fix is specifically on the historical Would you consider this level of performance cost acceptable, or do you think it still needs to be optimized further? If further optimization is needed, my current idea is a low-risk local change: during a single historical Original data:My branch:=== pr run 1 === large_smt_forest_persistent_open_historical_tree/benchmark large_smt_forest_persistent_get_full_tree/benchmark large_smt_forest_persistent_get_historical_tree/benchmark large_smt_forest_persistent_entry_count_current_tree/benchmark large_smt_forest_persistent_entry_count_historical_tree/benchmark large_smt_forest_persistent_entries_current_tree/benchmark large_smt_forest_persistent_entries_historical_tree/benchmark === pr run 2 === large_smt_forest_persistent_open_historical_tree/benchmark large_smt_forest_persistent_get_full_tree/benchmark large_smt_forest_persistent_get_historical_tree/benchmark large_smt_forest_persistent_entry_count_current_tree/benchmark large_smt_forest_persistent_entry_count_historical_tree/benchmark large_smt_forest_persistent_entries_current_tree/benchmark large_smt_forest_persistent_entries_historical_tree/benchmark === pr run 3 === large_smt_forest_persistent_open_historical_tree/benchmark large_smt_forest_persistent_get_full_tree/benchmark large_smt_forest_persistent_get_historical_tree/benchmark large_smt_forest_persistent_entry_count_current_tree/benchmark large_smt_forest_persistent_entry_count_historical_tree/benchmark large_smt_forest_persistent_entries_current_tree/benchmark large_smt_forest_persistent_entries_historical_tree/benchmark origin/next=== next run 1 === large_smt_forest_persistent_open_historical_tree/benchmark large_smt_forest_persistent_get_full_tree/benchmark large_smt_forest_persistent_get_historical_tree/benchmark large_smt_forest_persistent_entry_count_current_tree/benchmark large_smt_forest_persistent_entry_count_historical_tree/benchmark large_smt_forest_persistent_entries_current_tree/benchmark large_smt_forest_persistent_entries_historical_tree/benchmark === next run 2 === large_smt_forest_persistent_open_historical_tree/benchmark large_smt_forest_persistent_get_full_tree/benchmark large_smt_forest_persistent_get_historical_tree/benchmark large_smt_forest_persistent_entry_count_current_tree/benchmark large_smt_forest_persistent_entry_count_historical_tree/benchmark large_smt_forest_persistent_entries_current_tree/benchmark large_smt_forest_persistent_entries_historical_tree/benchmark === next run 3 === large_smt_forest_persistent_open_historical_tree/benchmark large_smt_forest_persistent_get_full_tree/benchmark large_smt_forest_persistent_get_historical_tree/benchmark large_smt_forest_persistent_entry_count_current_tree/benchmark large_smt_forest_persistent_entry_count_historical_tree/benchmark large_smt_forest_persistent_entries_current_tree/benchmark large_smt_forest_persistent_entries_historical_tree/benchmark |
|
I think that performance delta is acceptable for this PR. Would you file an issue for your performance improvement idea? It's worth exploring regardless. |
Thanks, I agree that this performance delta is acceptable for this PR. I’ve filed a follow-up issue for the optimization work here: #923. I’ll continue the performance investigation there and look into optimizing the repeated history-scan path afterward. |
iamrecursion
left a comment
iamrecursion
left a comment
There was a problem hiding this comment.
LGTM. Just a couple of minor documentation fixes please!
|
|
||
| /// Returns the leaf stored at `leaf_index` in the SMT with the specified `lineage`. | ||
| /// Returns the leaf stored at `leaf_index` in the SMT with the specified `lineage`. If no | ||
| /// leaf is explicitly stored at the given index, an empty leaf for that index is returned. |
There was a problem hiding this comment.
| /// leaf is explicitly stored at the given index, an empty leaf for that index is returned. | |
| /// Returns the leaf stored at `leaf_index` in the SMT with the specified `lineage`. | |
| /// | |
| /// If no leaf is explicitly stored at the given index, an empty leaf for that index | |
| /// is returned. |
You may need to wrap this correctly by hand. I wrote it in the GH PR editor. :'D
| } | ||
|
|
||
| /// Returns the leaf stored at `leaf_index` in the SMT with the specified `lineage`. | ||
| /// Returns the leaf stored at `leaf_index` in the SMT with the specified `lineage`. If no |
There was a problem hiding this comment.
Please make the suggested doc fix here too.
|
I resolved the conflict by keeping the incoming side, since historical entry_count now uses the stored count directly instead of iterating through entries. That makes the success assertion the correct behavior here. I will pay closer attention to these doc formatting nits details going forward. |
3 participants
This PR expands the property-based test coverage for
LargeSmtForest.Previously, the forest-level property tests were mostly limited to
entries(). This change adds a broader set of reference-model-driven property tests covering core query, mutation, metadata, constructor, and truncation behavior.The new tests now exercise:
get(),entries(), andentry_count()open()consistency checks against a referenceSmtlatest_version(),latest_root(),lineage_roots(),roots(), androot_info()add_lineage(),update_tree(), andupdate_forest()with_config()andtruncate()In addition, the property test helpers and generators were refactored to make the coverage easier to extend and maintain.
Closes #879.