test: make FakeStateManager return a valid height witness#10364
Open
pierugo-dfinity wants to merge 4 commits into
Open
test: make FakeStateManager return a valid height witness#10364pierugo-dfinity wants to merge 4 commits into
FakeStateManager return a valid height witness#10364pierugo-dfinity wants to merge 4 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the FakeStateManager (test utility) to produce a valid height witness for list_state_hashes_to_certify, aligning its behavior with StateManagerImpl so certification can progress in upcoming consensus integration tests.
Changes:
- Compute
FakeStateManager’s height witness using canonical-state lazy tree hashing (replicated_state_as_lazy_tree→hash_lazy_tree→compute_state_height_witness) instead ofWitness::new_for_testing. - Factor the shared witness-construction logic into
ic-canonical-state-tree-hashaswitness::compute_state_height_witnessand reuse it fromStateManagerImpl. - Add required Rust and Bazel dependencies to
rs/test_utilities.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rs/test_utilities/src/state_manager.rs | Generates real height witnesses + matching partial hashes for certification metadata. |
| rs/test_utilities/Cargo.toml | Adds canonical-state crates needed to compute witnesses in tests. |
| rs/test_utilities/BUILD.bazel | Adds Bazel deps for the same new canonical-state crates. |
| rs/state_manager/src/lib.rs | Reuses the new shared compute_state_height_witness helper instead of duplicating logic. |
| rs/canonical_state/tree_hash/src/witness.rs | Introduces shared compute_state_height_witness helper. |
| rs/canonical_state/tree_hash/src/lib.rs | Exposes the new witness module. |
| Cargo.lock | Records the new dependency edges. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR makes
FakeStateManagerreturn a valid height witness in its implementation oflist_state_hashes_to_certify. This change is driven by an upcoming new consensus integration test which depends on the certified height increasing. Though as of today, nodes in the integration test keeps invalidating certification shares (preventing the certified height to increase) because the height witness is not valid.To do so, the
FakeStateManagernow calls the same functions asStateManagerImpl(i.e.replicated_state_as_lazy_tree,hash_lazy_tree,compute_state_height_witness) to compute the witness instead of using a::new_for_testing.The common part of the code was thus moved to the
ic_canonical_state_tree_hashcrate, whereHashTreeandLazyTreeare defined and already depends onic_crypto_tree_hashforWitness. Moving it to the latter crate was a possibility but we would have had to addic_canonical_state_tree_hashas dependency forHashTreeandLazyTree.