Repository navigation
feat(platform-wallet)!: detect inconsistent shielded commitment trees and add explicit rebuild #5565
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
base: fix/shielded-sync-concurrent-append
Are you sure you want to change the base?
feat(platform-wallet)!: detect inconsistent shielded commitment trees and add explicit rebuild #5565
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1291,6 +1291,29 @@ impl NetworkShieldedCoordinator { | |
| Ok(()) | ||
| } | ||
|
|
||
| /// Empty only the commitment tree and rewind all subwallet scan cursors. | ||
| /// Notes, nullifiers, spent status, pending spends, and registrations survive. | ||
| /// The next sync performs a full rescan from zero, costing bandwidth and | ||
| /// trial decryption proportional to the entire pool. Never called automatically. | ||
| pub async fn rebuild_shielded_tree(&self) -> Result<(), PlatformWalletError> { | ||
| let _lifecycle = self.lifecycle.lock().await; | ||
| let mut store = self.store.write().await; | ||
| // Rewind even if the durable reset fails partway through. Rescanning | ||
| // an intact tree is harmless; skipping a partially reset tree is not. | ||
| store.rewind_scan_watermarks(); | ||
| if let Ok(mut caught_up) = self.last_caught_up_at.lock() { | ||
| *caught_up = None; | ||
| } | ||
| store.reset_commitment_tree()?; | ||
| let remaining = store.tree_size()?; | ||
| if remaining != 0 { | ||
| return Err(PlatformWalletError::ShieldedTreeUpdateFailed(format!( | ||
| "commitment tree still has {remaining} leaves after rebuild reset" | ||
| ))); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Run one shielded sync pass for every registered wallet on | ||
| /// this coordinator's network. Returns a per-wallet outcome | ||
| /// summary suitable for emission to UI / persistence layers | ||
|
|
@@ -1315,6 +1338,27 @@ impl NetworkShieldedCoordinator { | |
| /// [`PlatformEventManager::on_shielded_sync_completed`]: | ||
| /// crate::events::PlatformEventManager::on_shielded_sync_completed | ||
| pub async fn sync(&self, force: bool) -> ShieldedSyncPassSummary { | ||
| match self.try_sync(force).await { | ||
| Ok(summary) => summary, | ||
| Err(error) => { | ||
| let subwallets = self | ||
| .accounts | ||
| .read() | ||
| .await | ||
| .iter() | ||
| .map(|(id, views)| (*id, views.clone())) | ||
| .collect::<Vec<_>>(); | ||
| self.fail_all_wallets(&subwallets, &error) | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
1338
to
+1354
Collaborator
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. 🟡 Suggestion: Test background sync handling of ShieldedTreeInconsistent The typed source: |
||
|
|
||
| /// Run the same pass as [`Self::sync`], preserving typed failures for callers | ||
| /// that need to distinguish an inconsistent tree from a transient sync error. | ||
| pub async fn try_sync( | ||
| &self, | ||
| force: bool, | ||
| ) -> Result<ShieldedSyncPassSummary, PlatformWalletError> { | ||
| // Network-wide cooldown gate. Snapshot the remaining | ||
| // window into a local before any await — `std::sync::Mutex` | ||
| // is `!Send` across await points (clippy's | ||
|
|
@@ -1336,7 +1380,7 @@ impl NetworkShieldedCoordinator { | |
| cooldown_total_secs = CAUGHT_UP_COOLDOWN.as_secs(), | ||
| "Coordinator sync skipped — within caught-up cooldown" | ||
| ); | ||
| return Self::cooldown_skip_summary(self).await; | ||
| return Ok(Self::cooldown_skip_summary(self).await); | ||
| } | ||
|
|
||
| // Snapshot the flat subwallet registry. This Vec is both | ||
|
|
@@ -1350,7 +1394,7 @@ impl NetworkShieldedCoordinator { | |
| let mut summary = ShieldedSyncPassSummary::default(); | ||
| if subwallets.is_empty() { | ||
| summary.sync_unix_seconds = Self::now_unix(); | ||
| return summary; | ||
| return Ok(summary); | ||
| } | ||
|
|
||
| // Snapshot armed reservations and prefetch Platform's recorded | ||
|
|
@@ -1378,18 +1422,14 @@ impl NetworkShieldedCoordinator { | |
| // Second, distinct signal: commitments committed to the local | ||
| // tree as the interleaved consumer drains the SDK stream. | ||
| let on_tree_progress = self.tree_progress_handler(); | ||
| let notes = match super::sync::sync_notes_across( | ||
| let notes = super::sync::sync_notes_across( | ||
| &self.sdk, | ||
| &self.store, | ||
| &subwallets, | ||
| on_progress.as_ref(), | ||
| on_tree_progress.as_ref(), | ||
| ) | ||
| .await | ||
| { | ||
| Ok(r) => r, | ||
| Err(e) => return self.fail_all_wallets(&subwallets, &e), | ||
| }; | ||
| .await?; | ||
| // Scan-based spend detection now happens INSIDE | ||
| // `sync_notes_across`: every scanned action's nullifier is | ||
| // replayed against each subwallet's store as part of the note | ||
|
|
@@ -1441,10 +1481,7 @@ impl NetworkShieldedCoordinator { | |
| self.release_stranded_spends(snapshot, &recorded).await; | ||
| } | ||
|
|
||
| let balances_per_sub = match super::sync::balances_across(&self.store, &subwallets).await { | ||
| Ok(r) => r, | ||
| Err(e) => return self.fail_all_wallets(&subwallets, &e), | ||
| }; | ||
| let balances_per_sub = super::sync::balances_across(&self.store, &subwallets).await?; | ||
|
|
||
| // Restore-path activity derivation: reconstruct per-operation | ||
| // activity entries best-effort from the notes / outgoing notes | ||
|
|
@@ -1524,7 +1561,7 @@ impl NetworkShieldedCoordinator { | |
| summary = | ||
| build_per_wallet_summary(&subwallets, ¬es, &newly_spent_per_sub, &balances_per_sub); | ||
| summary.sync_unix_seconds = Self::now_unix(); | ||
| summary | ||
| Ok(summary) | ||
| } | ||
|
|
||
| /// Build a `ShieldedSyncPassSummary` where every registered | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -148,6 +148,13 @@ impl FileBackedShieldedStore { | |||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /// Rewind all scan cursors without discarding receipts or spend guards. | ||||||||||||||||||||||||||||||
| pub(super) fn rewind_scan_watermarks(&mut self) { | ||||||||||||||||||||||||||||||
| for state in self.subwallets.values_mut() { | ||||||||||||||||||||||||||||||
| state.last_synced_index = 0; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
Comment on lines
+151
to
+155
Collaborator
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. 🟡 Suggestion: Reset host-visible scan metadata during a tree rebuild
Suggested change
source: |
||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /// Open or create a shielded store at `path`. | ||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||
| /// SQLite is opened with **WAL journal + synchronous=NORMAL + temp_store=MEMORY** | ||||||||||||||||||||||||||||||
|
|
@@ -194,6 +201,19 @@ impl FileBackedShieldedStore { | |||||||||||||||||||||||||||||
| .map_err(|e| { | ||||||||||||||||||||||||||||||
| FileShieldedStoreError::Storage(format!("create pending_spends table: {e}")) | ||||||||||||||||||||||||||||||
| })?; | ||||||||||||||||||||||||||||||
| // The bound shares the tree's database/WAL: committing it before an | ||||||||||||||||||||||||||||||
| // append prevents a restart from pairing new leaves with an old bound. | ||||||||||||||||||||||||||||||
| pending_conn | ||||||||||||||||||||||||||||||
| .execute( | ||||||||||||||||||||||||||||||
| "CREATE TABLE IF NOT EXISTS shielded_tree_sync_height ( | ||||||||||||||||||||||||||||||
| id INTEGER PRIMARY KEY CHECK (id = 0), | ||||||||||||||||||||||||||||||
| height INTEGER NOT NULL CHECK (height >= 0) | ||||||||||||||||||||||||||||||
| )", | ||||||||||||||||||||||||||||||
| [], | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
| .map_err(|e| { | ||||||||||||||||||||||||||||||
| FileShieldedStoreError::Storage(format!("create tree sync height: {e}")) | ||||||||||||||||||||||||||||||
| })?; | ||||||||||||||||||||||||||||||
| // Additive, idempotent migration: old guards remain active. | ||||||||||||||||||||||||||||||
| let columns = { | ||||||||||||||||||||||||||||||
| let mut statement = pending_conn | ||||||||||||||||||||||||||||||
|
|
@@ -542,6 +562,17 @@ impl ShieldedStore for FileBackedShieldedStore { | |||||||||||||||||||||||||||||
| .unwrap_or_default()) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| fn note_spent_status( | ||||||||||||||||||||||||||||||
| &self, | ||||||||||||||||||||||||||||||
| id: SubwalletId, | ||||||||||||||||||||||||||||||
| nullifier: &[u8; 32], | ||||||||||||||||||||||||||||||
| ) -> Result<Option<bool>, Self::Error> { | ||||||||||||||||||||||||||||||
| Ok(self | ||||||||||||||||||||||||||||||
| .subwallets | ||||||||||||||||||||||||||||||
| .get(&id) | ||||||||||||||||||||||||||||||
| .and_then(|state| state.note_spent_status(nullifier))) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| fn mark_spent(&mut self, id: SubwalletId, nullifier: &[u8; 32]) -> Result<bool, Self::Error> { | ||||||||||||||||||||||||||||||
| let Some(sw) = self.subwallets.get_mut(&id) else { | ||||||||||||||||||||||||||||||
| return Ok(false); | ||||||||||||||||||||||||||||||
|
|
@@ -857,6 +888,43 @@ impl ShieldedStore for FileBackedShieldedStore { | |||||||||||||||||||||||||||||
| Ok(size) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| fn tree_sync_height(&self) -> Result<Option<u64>, Self::Error> { | ||||||||||||||||||||||||||||||
| let conn = self | ||||||||||||||||||||||||||||||
| .pending_conn | ||||||||||||||||||||||||||||||
| .lock() | ||||||||||||||||||||||||||||||
| .map_err(|e| FileShieldedStoreError::Storage(format!("tree sync height mutex: {e}")))?; | ||||||||||||||||||||||||||||||
| let height: Option<i64> = conn | ||||||||||||||||||||||||||||||
| .query_row( | ||||||||||||||||||||||||||||||
| "SELECT MAX(height) FROM shielded_tree_sync_height", | ||||||||||||||||||||||||||||||
| [], | ||||||||||||||||||||||||||||||
| |row| row.get(0), | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
| .map_err(|e| FileShieldedStoreError::Storage(format!("read tree sync height: {e}")))?; | ||||||||||||||||||||||||||||||
| height | ||||||||||||||||||||||||||||||
| .map(u64::try_from) | ||||||||||||||||||||||||||||||
| .transpose() | ||||||||||||||||||||||||||||||
| .map_err(|e| FileShieldedStoreError::Storage(format!("invalid tree sync height: {e}"))) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| fn set_tree_sync_height(&mut self, height: u64) -> Result<(), Self::Error> { | ||||||||||||||||||||||||||||||
| let height = i64::try_from(height).map_err(|e| { | ||||||||||||||||||||||||||||||
| FileShieldedStoreError::Storage(format!( | ||||||||||||||||||||||||||||||
| "tree sync height exceeds SQLite integer range: {e}" | ||||||||||||||||||||||||||||||
| )) | ||||||||||||||||||||||||||||||
| })?; | ||||||||||||||||||||||||||||||
| let conn = self | ||||||||||||||||||||||||||||||
| .pending_conn | ||||||||||||||||||||||||||||||
| .lock() | ||||||||||||||||||||||||||||||
| .map_err(|e| FileShieldedStoreError::Storage(format!("tree sync height mutex: {e}")))?; | ||||||||||||||||||||||||||||||
| conn.execute( | ||||||||||||||||||||||||||||||
| "INSERT INTO shielded_tree_sync_height (id, height) VALUES (0, ?1) | ||||||||||||||||||||||||||||||
| ON CONFLICT(id) DO UPDATE SET height = MAX(height, excluded.height)", | ||||||||||||||||||||||||||||||
| [height], | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
| .map_err(|e| FileShieldedStoreError::Storage(format!("write tree sync height: {e}")))?; | ||||||||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| fn last_synced_note_index(&self, id: SubwalletId) -> Result<u64, Self::Error> { | ||||||||||||||||||||||||||||||
| Ok(self | ||||||||||||||||||||||||||||||
| .subwallets | ||||||||||||||||||||||||||||||
|
|
@@ -974,7 +1042,8 @@ impl ShieldedStore for FileBackedShieldedStore { | |||||||||||||||||||||||||||||
| "DELETE FROM commitment_tree_checkpoint_marks_removed; | ||||||||||||||||||||||||||||||
| DELETE FROM commitment_tree_checkpoints; | ||||||||||||||||||||||||||||||
| DELETE FROM commitment_tree_shards; | ||||||||||||||||||||||||||||||
| DELETE FROM commitment_tree_cap;", | ||||||||||||||||||||||||||||||
| DELETE FROM commitment_tree_cap; | ||||||||||||||||||||||||||||||
| DELETE FROM shielded_tree_sync_height;", | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
| .map_err(|e| { | ||||||||||||||||||||||||||||||
| FileShieldedStoreError::Storage(format!("reset commitment tree tables: {e}")) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
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.
🔴 Blocking: Expose the preserving rebuild and typed inconsistency result through FFI
The only non-destructive recovery operation,
rebuild_shielded_tree, is reachable only through the Rust manager/coordinator API. No corresponding entry point exists inrs-platform-wallet-ffior the Swift/Kotlin wrappers. The FFI sync path mapsShieldedTreeInconsistentto the genericErrorWalletOperationwith a rendered string, so mobile hosts cannot reliably distinguish this terminal tree condition from a transient sync failure or invoke the required repair. The existingshielded_clearoperation is not equivalent because it removes registrations, notes, spent status, and related wallet state. Add a thin FFI marshal entry point for the Rust rebuild method and a stable result-code mapping forShieldedTreeInconsistent.source:
muse-spark-1.3-contributor(phase1-reviewer: general, rust-quality);gpt-6.1-sol(phase2-reviewer: general, architecture-layering, rust-quality, security-auditor)