-
Notifications
You must be signed in to change notification settings - Fork 35
feat(sync): SyncProtocolExecutor trait for unified protocol testing #1972
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4662a28
f955b9f
6a7d56a
0d73005
eeb8087
6186741
78cd972
20f49bb
a7eacd0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| //! Common trait for sync protocol implementations. | ||
| //! | ||
| //! This module defines the [`SyncProtocolExecutor`] trait that all sync protocols | ||
| //! implement. This enables: | ||
| //! | ||
| //! - Protocol implementation details contained within each protocol module | ||
| //! - Common interface for `SyncManager` to invoke any protocol | ||
| //! - Same code path for production and simulation (only `Store` backend differs) | ||
| //! | ||
| //! # Architecture | ||
| //! | ||
| //! ```text | ||
| //! ┌─────────────────────────────────────────────────────────────────┐ | ||
| //! │ SyncProtocolExecutor trait │ | ||
| //! │ ┌─────────────────┐ ┌─────────────────┐ ┌─────────────────┐ │ | ||
| //! │ │ HashComparison │ │ Snapshot │ │ BloomFilter │ │ | ||
| //! │ │ Protocol │ │ Protocol │ │ Protocol │ │ | ||
| //! │ └────────┬────────┘ └────────┬────────┘ └────────┬────────┘ │ | ||
| //! │ │ │ │ │ | ||
| //! │ └────────────────────┼────────────────────┘ │ | ||
| //! │ │ │ | ||
| //! │ ┌───────────┴───────────┐ │ | ||
| //! │ │ SyncTransport │ │ | ||
| //! │ │ (Stream or SimStream) │ │ | ||
| //! │ └───────────────────────┘ │ | ||
| //! └─────────────────────────────────────────────────────────────────┘ | ||
| //! ``` | ||
| //! | ||
| //! # Example | ||
| //! | ||
| //! ```ignore | ||
| //! use calimero_node_primitives::sync::{SyncProtocolExecutor, HashComparisonProtocol}; | ||
| //! | ||
| //! // Production | ||
| //! let mut transport = StreamTransport::new(&mut stream); | ||
| //! let stats = HashComparisonProtocol::run_initiator( | ||
| //! &mut transport, | ||
| //! &store, | ||
| //! context_id, | ||
| //! identity, | ||
| //! HashComparisonConfig { remote_root_hash }, | ||
| //! ).await?; | ||
| //! | ||
| //! // Simulation (exact same call, different transport/store) | ||
| //! let mut transport = SimStream::new(); | ||
| //! let stats = HashComparisonProtocol::run_initiator( | ||
| //! &mut transport, | ||
| //! &store, // Store<InMemoryDB> | ||
| //! context_id, | ||
| //! identity, | ||
| //! HashComparisonConfig { remote_root_hash }, | ||
| //! ).await?; | ||
| //! ``` | ||
|
|
||
| use async_trait::async_trait; | ||
| use calimero_primitives::context::ContextId; | ||
| use calimero_primitives::identity::PublicKey; | ||
| use calimero_store::Store; | ||
| use eyre::Result; | ||
|
|
||
| use super::SyncTransport; | ||
|
|
||
| /// Trait for sync protocol implementations. | ||
| /// | ||
| /// Each sync protocol (HashComparison, Snapshot, BloomFilter, etc.) implements | ||
| /// this trait. The protocol logic is generic over: | ||
| /// | ||
| /// - `T: SyncTransport` - the transport layer (production streams or simulation channels) | ||
| /// - `Store` - the storage backend (RocksDB or InMemoryDB) | ||
| /// | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Trait bounds may be overly restrictive for Config type The Suggested fix: |
||
| /// This enables the same protocol code to run in both production and simulation. | ||
| /// | ||
| /// Note: Uses `?Send` because `RuntimeEnv` (used for storage access) contains `Rc` | ||
| /// which is not `Send`. Callers must not spawn these futures across threads. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 Nit: Associated type bounds may be overly restrictive Both Suggested fix: |
||
| #[async_trait(?Send)] | ||
| pub trait SyncProtocolExecutor { | ||
| /// Protocol-specific configuration for the initiator. | ||
| /// | ||
| /// For example, HashComparison needs the remote root hash. | ||
| type Config: Send; | ||
|
|
||
| /// Protocol-specific statistics/results. | ||
| type Stats: Send + Default; | ||
|
|
||
| /// Run the initiator (pulling) side of the protocol. | ||
| /// | ||
| /// The initiator requests data from the responder and applies it locally. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `transport` - The transport for sending/receiving messages | ||
| /// * `store` - The local storage (works with both RocksDB and InMemoryDB) | ||
| /// * `context_id` - The context being synced | ||
| /// * `identity` - Our identity for this context | ||
| /// * `config` - Protocol-specific configuration | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// Protocol-specific statistics on success. | ||
| async fn run_initiator<T: SyncTransport>( | ||
| transport: &mut T, | ||
| store: &Store, | ||
| context_id: ContextId, | ||
| identity: PublicKey, | ||
| config: Self::Config, | ||
| ) -> Result<Self::Stats>; | ||
|
|
||
| /// Run the responder side of the protocol. | ||
| /// | ||
| /// The responder answers requests from the initiator. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `transport` - The transport for sending/receiving messages | ||
| /// * `store` - The local storage | ||
| /// * `context_id` - The context being synced | ||
| /// * `identity` - Our identity for this context | ||
| async fn run_responder<T: SyncTransport>( | ||
| transport: &mut T, | ||
| store: &Store, | ||
| context_id: ContextId, | ||
| identity: PublicKey, | ||
| ) -> Result<()>; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,184 @@ | ||
| //! Storage bridge utilities for sync protocols. | ||
| //! | ||
| //! This module provides helpers to bridge `calimero-storage` APIs (which use | ||
| //! the `RuntimeEnv` thread-local) to the underlying `calimero-store` backend. | ||
| //! | ||
| //! # Why This Exists | ||
| //! | ||
| //! The `calimero-storage` crate provides high-level storage APIs (`Index`, `Interface`) | ||
| //! that operate through a thread-local `RuntimeEnv`. This `RuntimeEnv` contains | ||
| //! callbacks that route read/write/remove operations to the actual database. | ||
| //! | ||
| //! This module provides a single, reusable way to create these callbacks from | ||
| //! a `Store`, regardless of the backend (RocksDB or InMemoryDB). | ||
| //! | ||
| //! # Usage | ||
| //! | ||
| //! ```ignore | ||
| //! use calimero_node_primitives::sync::storage_bridge::create_runtime_env; | ||
| //! | ||
| //! // Works with any Store backend (RocksDB or InMemoryDB) | ||
| //! let runtime_env = create_runtime_env(&store, context_id, identity); | ||
| //! | ||
| //! // Use with storage APIs | ||
| //! with_runtime_env(runtime_env, || { | ||
| //! let index = Index::<MainStorage>::get_index(entity_id)?; | ||
| //! // ... | ||
| //! }); | ||
| //! ``` | ||
|
|
||
| use std::cell::RefCell; | ||
| use std::rc::Rc; | ||
|
|
||
| use calimero_primitives::context::ContextId; | ||
| use calimero_primitives::identity::PublicKey; | ||
| use calimero_storage::env::RuntimeEnv; | ||
| use calimero_storage::store::Key; | ||
| use calimero_store::{key, types, Store}; | ||
| use tracing::warn; | ||
|
|
||
| /// Create a `RuntimeEnv` that bridges `calimero-storage` to a `Store`. | ||
| /// | ||
| /// This is the canonical way to set up storage access for sync protocols. | ||
| /// The returned `RuntimeEnv` can be used with `with_runtime_env()` to enable | ||
| /// `Index<MainStorage>` and `Interface<MainStorage>` operations. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `store` - The underlying store (works with both RocksDB and InMemoryDB) | ||
| /// * `context_id` - The context being accessed | ||
| /// * `executor_id` - The identity executing operations | ||
| /// | ||
| /// # Example | ||
| /// | ||
| /// ```ignore | ||
| /// let env = create_runtime_env(&store, context_id, identity); | ||
| /// let result = with_runtime_env(env, || { | ||
| /// Index::<MainStorage>::get_index(entity_id) | ||
| /// }); | ||
| /// ``` | ||
| pub fn create_runtime_env( | ||
| store: &Store, | ||
| context_id: ContextId, | ||
| executor_id: PublicKey, | ||
| ) -> RuntimeEnv { | ||
| let callbacks = create_storage_callbacks(store, context_id); | ||
| RuntimeEnv::new( | ||
| callbacks.read, | ||
| callbacks.write, | ||
| callbacks.remove, | ||
| *context_id.as_ref(), | ||
| *executor_id.as_ref(), | ||
| ) | ||
| } | ||
|
|
||
| /// Storage callback closures that bridge `calimero-storage` Key API to the Store. | ||
| /// | ||
| /// These closures translate `calimero-storage::Key` (Index/Entry) to | ||
| /// `calimero-store::ContextStateKey` for access to the actual database. | ||
| #[expect( | ||
| clippy::type_complexity, | ||
| reason = "Matches RuntimeEnv callback signatures" | ||
| )] | ||
| struct StorageCallbacks { | ||
| read: Rc<dyn Fn(&Key) -> Option<Vec<u8>>>, | ||
| write: Rc<dyn Fn(Key, &[u8]) -> bool>, | ||
| remove: Rc<dyn Fn(&Key) -> bool>, | ||
| } | ||
|
|
||
| /// Create storage callbacks for a context. | ||
| /// | ||
| /// These bridge the `calimero-storage` Key-based API to the underlying | ||
| /// `calimero-store` ContextStateKey-based storage. | ||
| #[expect( | ||
| clippy::type_complexity, | ||
| reason = "Matches RuntimeEnv callback signatures" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Three separate store handles created for read/write/remove Each callback creates its own Suggested fix: |
||
| )] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Multiple separate store handles created when one could be shared
Suggested fix: |
||
| fn create_storage_callbacks(store: &Store, context_id: ContextId) -> StorageCallbacks { | ||
| let read: Rc<dyn Fn(&Key) -> Option<Vec<u8>>> = { | ||
| let handle = store.handle(); | ||
| let ctx_id = context_id; | ||
| Rc::new(move |key: &Key| { | ||
| let storage_key = key.to_bytes(); | ||
| let state_key = key::ContextState::new(ctx_id, storage_key); | ||
| match handle.get(&state_key) { | ||
| Ok(Some(state)) => Some(state.value.into_boxed().into_vec()), | ||
| Ok(None) => None, | ||
| Err(e) => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Storage read errors are silently masked When storage read fails, the error is logged but Suggested fix: |
||
| warn!( | ||
| %ctx_id, | ||
| storage_key = %hex::encode(storage_key), | ||
| error = ?e, | ||
| "Storage read failed" | ||
| ); | ||
| None | ||
| } | ||
| } | ||
| }) | ||
| }; | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Silent failure on storage write operations The write callback returns Suggested fix: |
||
| let write: Rc<dyn Fn(Key, &[u8]) -> bool> = { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Storage errors logged but operation appears successful to caller The Suggested fix: |
||
| let handle_cell: Rc<RefCell<_>> = Rc::new(RefCell::new(store.handle())); | ||
| let ctx_id = context_id; | ||
| Rc::new(move |key: Key, value: &[u8]| { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Silent write failures may cause data loss The write callback returns false on error without logging, unlike the read callback which logs errors; this inconsistency makes debugging storage failures difficult and could mask data corruption. Suggested fix: |
||
| let storage_key = key.to_bytes(); | ||
| let state_key = key::ContextState::new(ctx_id, storage_key); | ||
| let slice: calimero_store::slice::Slice<'_> = value.to_vec().into(); | ||
| let state_value = types::ContextState::from(slice); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Write/remove callbacks could panic on recursive borrow Using Suggested fix: |
||
| handle_cell | ||
| .borrow_mut() | ||
| .put(&state_key, &state_value) | ||
| .is_ok() | ||
| }) | ||
| }; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Silent remove failures may cause data inconsistency The remove callback returns false on error without logging, which could mask failed deletions and lead to stale data persisting unnoticed. Suggested fix: |
||
|
|
||
| let remove: Rc<dyn Fn(&Key) -> bool> = { | ||
| let handle_cell: Rc<RefCell<_>> = Rc::new(RefCell::new(store.handle())); | ||
| let ctx_id = context_id; | ||
| Rc::new(move |key: &Key| { | ||
| let storage_key = key.to_bytes(); | ||
| let state_key = key::ContextState::new(ctx_id, storage_key); | ||
| handle_cell.borrow_mut().delete(&state_key).is_ok() | ||
| }) | ||
| }; | ||
|
|
||
| StorageCallbacks { | ||
| read, | ||
| write, | ||
| remove, | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use std::sync::Arc; | ||
|
|
||
| use super::*; | ||
| use calimero_storage::env::with_runtime_env; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Test coverage only verifies read callback, not write or remove The test only verifies that Suggested fix: |
||
| use calimero_storage::index::Index; | ||
| use calimero_storage::store::MainStorage; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Test only verifies non-panic, not callback behavior The test Suggested fix: |
||
| use calimero_store::db::InMemoryDB; | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Test only verifies no panic, not actual functionality The test Suggested fix: |
||
| #[test] | ||
| fn test_create_runtime_env_with_inmemory() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Test only verifies read callback, not write/remove The unit test creates a RuntimeEnv and calls get_index but doesn't exercise the write or remove callbacks, leaving those code paths untested. Suggested fix: |
||
| // Create an in-memory store | ||
| let db = InMemoryDB::owned(); | ||
| let store = Store::new(Arc::new(db)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Test assertion doesn't verify expected return value The comment states 'should be Ok(None)' but the assertion only checks Suggested fix: |
||
|
|
||
| // Create a context ID and identity | ||
| let context_id = ContextId::from([1u8; 32]); | ||
| let identity = PublicKey::from([2u8; 32]); | ||
|
|
||
| // Create RuntimeEnv - should not panic | ||
| let env = create_runtime_env(&store, context_id, identity); | ||
|
|
||
| // Use it with storage APIs | ||
| let result = with_runtime_env(env, || { | ||
| // Try to get a non-existent index - should return None, not panic | ||
| Index::<MainStorage>::get_index(calimero_storage::address::Id::root()) | ||
| }); | ||
|
|
||
| // Root index doesn't exist yet, should be Ok(None) | ||
| assert!(result.is_ok()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Doc example references non-existent type
The example uses
HashComparisonProtocolbut no struct implementingSyncProtocolExecutoris introduced in this PR, making the example misleading.Suggested fix: