Conversation
Add infrastructure to run sync protocols through in-memory channels, enabling testing of actual message flow and state convergence. Key changes: - Add SyncTransport trait abstracting network operations (send/recv/close) - Add StreamTransport for production Stream wrapper - Add SimStream for in-memory channel-based transport - Add protocol.rs with execute_hash_comparison_sync for simulation - Add SimNode::new_in_context for shared context testing - Fix entity_count to use storage leaf_count (source of truth) The simulation now uses the exact same storage code path as production (Index<MainStorage>, Interface<MainStorage>, RuntimeEnv callbacks), with only the Database implementation differing (InMemoryDB vs RocksDB). Phase 1 of sim-transport-abstraction plan.
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Introduces a trait-based architecture for sync protocols that enables: - Same protocol code to run in production and simulation - Shared storage bridge (create_runtime_env) for both backends - Standalone HashComparisonProtocol implementation Changes: - Add SyncProtocolExecutor trait in node-primitives - Add create_runtime_env shared helper in storage_bridge.rs - Extract HashComparisonProtocol to standalone module - Clean up hash_comparison.rs (responder only, ~875 → ~285 lines) - Fix wire protocol to use u64 for sequence_id (portability) - Update simulation tests to use production protocol directly This removes ~730 lines of duplicated code and ensures the simulation tests exercise the exact same protocol logic as production.
- Bug 1: Preserve existing storage_type when updating entities in apply_leaf_with_crdt_merge to prevent 'Cannot change StorageType' errors for User/Frozen storage types during HashComparison sync - Bug 2: Extract get_local_tree_node helper to storage_bridge.rs to deduplicate identical Merkle tree lookup logic between hash_comparison_protocol.rs and hash_comparison.rs
|
Cursor Agent can help with this pull request. Just |
|
Your PR title does not adhere to the Conventional Commits convention: Common errors to avoid:
|
![meroreviewer[bot]](https://avatars.githubusercontent.com/in/2793509?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 100% | Review time: 154.4s
🟡 1 warnings, 💡 1 suggestions. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-37b8c80f
| metadata.crdt_type = Some(leaf.metadata.crdt_type.clone()); | ||
| metadata.updated_at = leaf.metadata.hlc_timestamp.into(); | ||
|
|
||
| // Preserve existing storage_type for updates to avoid "Cannot change StorageType" error. |
There was a problem hiding this comment.
🟡 Bug fix lacks regression test
The storage_type preservation fix addresses a specific failure mode but has no accompanying test to prevent regression.
Suggested fix:
Add a unit or integration test that syncs an entity with StorageType::User or StorageType::Frozen to verify the fix works and prevent future regression.
| }; | ||
|
|
||
| // Get the entity's index from the Merkle tree | ||
| let index = match Index::<MainStorage>::get_index(entity_id) { |
There was a problem hiding this comment.
💡 Error swallowing may mask storage failures
Returning Ok(None) on Index::get_index error makes it indistinguishable from 'entity not found', potentially masking real storage failures.
Suggested fix:
Consider returning the error or using a distinct return variant so callers can differentiate between 'not found' and 'storage error'.
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
2 participants
Fix: Sync for non-Public storage types and deduplicate tree node lookup
Description
This PR addresses two issues related to the hash comparison synchronization protocol:
apply_leaf_with_crdt_mergefunction was creatingMetadata::default()(which hasStorageType::Public) for updates. This caused sync failures for entities withStorageType::UserorStorageType::FrozenbecauseInterface::apply_actionwould reject the update due to aStorageTypemismatch. The fix ensures that the existing entity'sstorage_typeis read from the index and preserved in the update action's metadata, allowing correct synchronization of all storage types.hash_comparison.rsandhash_comparison_protocol.rs. This common logic has been extracted into a new shared helper function,get_local_tree_node, withincrates/node/primitives/src/sync/storage_bridge.rs. Both modules now import and utilize this single, shared implementation, improving code quality and maintainability.Test plan
The changes were verified by:
crates/node/primitives,crates/node/src/sync).cargo fmtandcargo clippyto ensure code style and catch potential issues.Interface::apply_action'sverify_action_updatefor non-Public storage types during sync. This would typically require an integration test withUserorFrozenentities.Documentation update
No public or internal documentation updates are required as these are internal code fixes and refactorings.