diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ca032a5f5..93d8f89901 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,14 @@ * [FEATURE][cli] Added a `--payback-note-type` option to `swap` so the payback note can be created as public or private (defaults to private). Public payback works without any off-band advice now that SWAP derives the payback recipient deterministically ([#2190](https://github.com/0xMiden/rust-sdk/pull/2190)). +### Changes + +* [rust] `AccountUpdates` records the local account states superseded by a same-nonce network transaction alongside the public and mismatched-private account updates, exposed via `AccountUpdates::superseded_local_states`. State sync discards the affected transactions as part of account sync rather than relying on its caller to forward a separate return value. `miden_client::store::AccountUpdates` and `miden_client::sync::AccountUpdates` now name the same type. + +### Fixes + +* [FIX][rust] `SyncSummary::locked_accounts` now reports only the private accounts a sync actually locked. A mismatched commitment that is already present in local history is stale network data and leaves the account usable, but every mismatch was previously reported as a lock — so `miden-client-cli sync` could print a non-zero "Locked accounts" count for accounts that remained usable. The count also no longer re-reports accounts that were already locked before the sync. + ## 0.16.0-alpha.1 (2026-07-17) ### Breaking Changes diff --git a/crates/rust-client/src/store/account.rs b/crates/rust-client/src/store/account.rs index 788e4699fd..149807c6dd 100644 --- a/crates/rust-client/src/store/account.rs +++ b/crates/rust-client/src/store/account.rs @@ -1,13 +1,11 @@ // ACCOUNT RECORD // ================================================================================================ -use alloc::vec::Vec; use core::fmt::Display; -use miden_protocol::account::{Account, AccountId, PartialAccount}; +use miden_protocol::account::{Account, PartialAccount}; use miden_protocol::{Felt, Word}; use crate::ClientError; -use crate::sync::PublicAccountUpdate; // ACCOUNT RECORD DATA // ================================================================================================ @@ -175,38 +173,3 @@ impl Display for AccountStatus { } } } - -// ACCOUNT UPDATES -// ================================================================================================ - -/// Contains account changes to apply to the store. -pub struct AccountUpdates { - /// Updated public accounts, either as full state replacements or incremental deltas. - updated_public_accounts: Vec, - /// Network account commitments that don't match the current tracked state for private - /// accounts. - mismatched_private_accounts: Vec<(AccountId, Word)>, -} - -impl AccountUpdates { - /// Creates a new instance of `AccountUpdates`. - pub fn new( - updated_public_accounts: Vec, - mismatched_private_accounts: Vec<(AccountId, Word)>, - ) -> Self { - Self { - updated_public_accounts, - mismatched_private_accounts, - } - } - - /// Returns the updated public accounts. - pub fn updated_public_accounts(&self) -> &[PublicAccountUpdate] { - &self.updated_public_accounts - } - - /// Returns the mismatched private accounts. - pub fn mismatched_private_accounts(&self) -> &[(AccountId, Word)] { - &self.mismatched_private_accounts - } -} diff --git a/crates/rust-client/src/store/mod.rs b/crates/rust-client/src/store/mod.rs index 882fdebbf5..74d7394576 100644 --- a/crates/rust-client/src/store/mod.rs +++ b/crates/rust-client/src/store/mod.rs @@ -68,15 +68,9 @@ mod smt_forest; pub use smt_forest::AccountSmtForest; mod account; -pub use account::{ - AccountRecord, - AccountRecordData, - AccountStatus, - AccountUpdates, - ClientAccountType, -}; +pub use account::{AccountRecord, AccountRecordData, AccountStatus, ClientAccountType}; -pub use crate::sync::PublicAccountUpdate; +pub use crate::sync::{AccountUpdates, PublicAccountUpdate}; mod note_record; pub use note_record::{ InputNoteRecord, diff --git a/crates/rust-client/src/sync/mod.rs b/crates/rust-client/src/sync/mod.rs index b571ffc3b3..853efe9035 100644 --- a/crates/rust-client/src/sync/mod.rs +++ b/crates/rust-client/src/sync/mod.rs @@ -131,8 +131,18 @@ where // Get the sync update from the network let state_sync_update = state_sync.sync_state(&mut partial_mmr, input).await?; - let sync_summary: SyncSummary = (&state_sync_update).into(); - debug!(sync_summary = ?sync_summary, "Sync summary computed"); + let mut sync_summary: SyncSummary = (&state_sync_update).into(); + + // Accounts whose network commitment diverges from the local state. The store locks such an + // account only when the diverging commitment is absent from local history, so which of + // these actually get locked is only observable by reading their status back afterwards. + let lock_candidates: Vec = state_sync_update + .account_updates + .mismatched_private_accounts() + .iter() + .map(|(id, _)| *id) + .collect(); + let already_locked = self.locked_accounts_among(&lock_candidates).await?; // Post-sync observer hooks; run before persisting. Per-observer errors are logged, not // propagated. @@ -146,6 +156,15 @@ where .await .map_err(ClientError::StoreError)?; + sync_summary.locked_accounts = self + .locked_accounts_among(&lock_candidates) + .await? + .into_iter() + .filter(|account_id| !already_locked.contains(account_id)) + .collect(); + + debug!(sync_summary = ?sync_summary, "Sync summary computed"); + // Cache MMR so pruning can reuse in-memory MMR. self.cache_partial_mmr(partial_mmr).await?; @@ -202,6 +221,24 @@ where Ok(summary) } + /// Returns which of the given accounts are currently locked. + /// + /// Accounts missing from the store are treated as not locked. + async fn locked_accounts_among( + &self, + account_ids: &[AccountId], + ) -> Result, ClientError> { + let mut locked = BTreeSet::new(); + for account_id in account_ids { + let header = self.store.get_account_header(*account_id).await?; + if header.is_some_and(|(_, status)| status.is_locked()) { + locked.insert(*account_id); + } + } + + Ok(locked) + } + /// Builds a default [`StateSyncInput`] from the current client state. /// /// This includes all tracked account headers, all unique note tags, all unspent input and @@ -357,7 +394,12 @@ pub struct SyncSummary { pub consumed_notes: Vec, /// IDs of on-chain accounts that have been updated. pub updated_accounts: Vec, - /// IDs of private accounts that have been locked. + /// IDs of private accounts that this sync locked because their on-chain commitment diverged + /// from the local state. + /// + /// A divergence that resolves to a commitment already present in local history is stale + /// network data rather than a real conflict; it leaves the account usable and is not reported + /// here. Accounts already locked before this sync are also excluded. pub locked_accounts: Vec, /// IDs of committed transactions. pub committed_transactions: Vec, diff --git a/crates/rust-client/src/sync/state_sync.rs b/crates/rust-client/src/sync/state_sync.rs index 254ef49e69..485df778ac 100644 --- a/crates/rust-client/src/sync/state_sync.rs +++ b/crates/rust-client/src/sync/state_sync.rs @@ -283,22 +283,14 @@ impl StateSync { state_sync_update.block_num = sync_data.chain_tip_header.block_num(); let new_commitments = derive_account_commitments(&sync_data.transactions); - let superseded_states = self - .account_state_sync( - &mut state_sync_update.account_updates, - &accounts, - &new_commitments, - block_num, - &sync_data.chain_tip_header, - ) - .await?; - - // Discard the local transactions whose result lost a same-nonce race against the network. - for superseded_state in superseded_states { - state_sync_update - .transaction_updates - .apply_superseded_account_state(superseded_state); - } + self.account_state_sync( + &mut state_sync_update, + &accounts, + &new_commitments, + block_num, + &sync_data.chain_tip_header, + ) + .await?; // Apply local changes: update the MMR, screen notes, and apply state transitions. self.apply_sync_result(sync_data, &mut state_sync_update, current_partial_mmr) @@ -672,35 +664,32 @@ impl StateSync { /// * Private accounts that have been marked as mismatched because the current commitment /// doesn't match the one received from the node. The client will need to handle these cases /// as they could be a stale account state or a reason to lock the account. - /// - /// Returns the local states that were superseded by a same-nonce network transaction; the - /// caller must discard the transactions that produced them. + /// * Local account states superseded by a same-nonce network transaction, whose producing + /// transactions are discarded before this method returns. async fn account_state_sync( &self, - account_updates: &mut AccountUpdates, + state_sync_update: &mut StateSyncUpdate, accounts: &[AccountHeader], account_commitment_updates: &[(AccountId, Word)], block_from: BlockNumber, chain_tip_header: &BlockHeader, - ) -> Result, ClientError> { + ) -> Result<(), ClientError> { // "Public" here includes both Public and Network accounts, since both have // their state stored on-chain and follow the same sync path. let (public_accounts, private_accounts): (Vec<_>, Vec<_>) = accounts.iter().partition(|header| !header.id().is_private()); - let superseded_states = self - .sync_public_accounts( - account_updates, - account_commitment_updates, - &public_accounts, - block_from, - chain_tip_header, - ) - .await?; + self.sync_public_accounts( + &mut state_sync_update.account_updates, + account_commitment_updates, + &public_accounts, + block_from, + chain_tip_header, + ) + .await?; // If a private account commitment differs between the node and local then we verify the // commitment from the node before flagging the account as mismatched. - let mut mismatched_private_accounts = Vec::new(); for header in &private_accounts { let account_id = header.id(); let local_commitment = header.to_commitment(); @@ -715,13 +704,15 @@ impl StateSync { .verify_private_account_mismatch(account_id, local_commitment, chain_tip_header) .await? { - mismatched_private_accounts.push((account_id, proven_commitment)); + state_sync_update + .account_updates + .push_mismatched_private_account(account_id, proven_commitment); } } - account_updates.extend(AccountUpdates::new(Vec::new(), mismatched_private_accounts)); + state_sync_update.discard_superseded_transactions(); - Ok(superseded_states) + Ok(()) } /// Verifies a private account commitment against an account witness from the node. @@ -781,7 +772,7 @@ impl StateSync { /// single `get_account` call that requests every storage map and the vault. /// /// Accounts whose vault or maps are too large to fit in a single response fall back to the - /// incremental [`PublicAccountUpdate::Delta`] path, which fetches vault and storage map + /// incremental [`PublicAccountUpdate::Patch`] path, which fetches vault and storage map /// updates over the synced block range. async fn sync_public_accounts( &self, @@ -790,11 +781,9 @@ impl StateSync { current_public_accounts: &[&AccountHeader], block_from: BlockNumber, chain_tip_header: &BlockHeader, - ) -> Result, ClientError> { + ) -> Result<(), ClientError> { let local_headers: BTreeMap = current_public_accounts.iter().map(|header| (header.id(), *header)).collect(); - // Local states that lost a same-nonce race; their transactions must be discarded. - let mut superseded_states = Vec::new(); for (id, commitment) in commitment_updates { let Some(local_header) = local_headers.get(id).copied() else { continue; @@ -809,16 +798,16 @@ impl StateSync { .await? { PublicAccountSync::Apply(public_update) => { - account_updates.extend(AccountUpdates::new(vec![*public_update], Vec::new())); + account_updates.push_public_update(*public_update); }, PublicAccountSync::Superseded => { - superseded_states.push(local_header.to_commitment()); + account_updates.push_superseded_local_state(local_header.to_commitment()); }, PublicAccountSync::Ignore => {}, } } - Ok(superseded_states) + Ok(()) } // SYNC PUBLIC ACCOUNTS HELPERS @@ -1373,7 +1362,7 @@ mod tests { let commitment_updates = vec![(account.id(), account.to_commitment())]; let mut account_updates = AccountUpdates::default(); - let superseded = state_sync + state_sync .sync_public_accounts( &mut account_updates, &commitment_updates, @@ -1389,7 +1378,7 @@ mod tests { "public account sync should ignore node snapshots that are older than local" ); assert!( - superseded.is_empty(), + account_updates.superseded_local_states().is_empty(), "an older node snapshot must not supersede the local state" ); } @@ -1410,7 +1399,7 @@ mod tests { let commitment_updates = vec![(account.id(), account.to_commitment())]; let mut account_updates = AccountUpdates::default(); - let superseded = state_sync + state_sync .sync_public_accounts( &mut account_updates, &commitment_updates, @@ -1426,8 +1415,8 @@ mod tests { "a same-nonce fork must not overwrite the account while its tx is still pending" ); assert_eq!( - superseded, - vec![local_header.to_commitment()], + account_updates.superseded_local_states(), + [local_header.to_commitment()], "the superseded local state should be reported so its transaction is discarded" ); } @@ -1594,7 +1583,7 @@ mod tests { let commitment_updates = vec![(account.id(), account.to_commitment())]; let mut account_updates = AccountUpdates::default(); - let superseded = state_sync + state_sync .sync_public_accounts( &mut account_updates, &commitment_updates, @@ -1605,7 +1594,10 @@ mod tests { .await .unwrap(); - assert!(superseded.is_empty(), "the transaction must not be superseded"); + assert!( + account_updates.superseded_local_states().is_empty(), + "the transaction must not be superseded" + ); assert!( account_updates.updated_public_accounts().is_empty(), "the target state must not overwrite the local account" diff --git a/crates/rust-client/src/sync/state_sync_update.rs b/crates/rust-client/src/sync/state_sync_update.rs index 009e9bbaa0..b4a68e1747 100644 --- a/crates/rust-client/src/sync/state_sync_update.rs +++ b/crates/rust-client/src/sync/state_sync_update.rs @@ -45,6 +45,19 @@ pub struct StateSyncUpdate { pub account_updates: AccountUpdates, } +impl StateSyncUpdate { + /// Discards the local transactions whose resulting account state was superseded by a + /// same-nonce network transaction. + /// + /// Such a transaction can never commit, so leaving it pending would keep it pending forever. + /// Repeated calls are harmless: only pending transactions are discarded. + pub(crate) fn discard_superseded_transactions(&mut self) { + for superseded_state in &self.account_updates.superseded_local_states { + self.transaction_updates.apply_superseded_account_state(*superseded_state); + } + } +} + impl From<&StateSyncUpdate> for SyncSummary { fn from(value: &StateSyncUpdate) -> Self { let new_public_note_ids = value @@ -104,12 +117,11 @@ impl From<&StateSyncUpdate> for SyncSummary { .iter() .map(PublicAccountUpdate::id) .collect(), - value - .account_updates - .mismatched_private_accounts() - .iter() - .map(|(id, _)| *id) - .collect(), + // A mismatched private account is only a *candidate* for locking: the store leaves it + // usable when the diverging commitment is already in local history. The verdict is + // knowable only once the update has been applied, so `Client::sync_chain` fills this + // in afterwards. + Vec::new(), value.transaction_updates.committed_transactions().map(|t| t.id).collect(), ) } @@ -440,6 +452,11 @@ pub struct AccountUpdates { /// hasn't been committed). If this is not the case, the account may be locked until the state /// is restored manually. mismatched_private_accounts: Vec<(AccountId, Word)>, + /// Local account states that lost a same-nonce race against a network transaction. + /// + /// The transactions that produced these states can never commit, so they are discarded by + /// [`StateSyncUpdate::discard_superseded_transactions`]. + superseded_local_states: Vec, } impl AccountUpdates { @@ -451,6 +468,7 @@ impl AccountUpdates { Self { updated_public_accounts, mismatched_private_accounts, + superseded_local_states: Vec::new(), } } @@ -464,9 +482,34 @@ impl AccountUpdates { &self.mismatched_private_accounts } + /// Returns the local account states that were superseded by a same-nonce network transaction. + pub fn superseded_local_states(&self) -> &[Word] { + &self.superseded_local_states + } + pub fn extend(&mut self, other: AccountUpdates) { self.updated_public_accounts.extend(other.updated_public_accounts); self.mismatched_private_accounts.extend(other.mismatched_private_accounts); + self.superseded_local_states.extend(other.superseded_local_states); + } + + /// Records an updated public account. + pub(crate) fn push_public_update(&mut self, update: PublicAccountUpdate) { + self.updated_public_accounts.push(update); + } + + /// Records a private account whose network commitment diverges from the local state. + pub(crate) fn push_mismatched_private_account( + &mut self, + account_id: AccountId, + proven_commitment: Word, + ) { + self.mismatched_private_accounts.push((account_id, proven_commitment)); + } + + /// Records a local account state that lost a same-nonce race against a network transaction. + pub(crate) fn push_superseded_local_state(&mut self, superseded_state: Word) { + self.superseded_local_states.push(superseded_state); } } diff --git a/crates/sqlite-store/src/account/tests.rs b/crates/sqlite-store/src/account/tests.rs index f6d8a4e0e6..4c4ee745b6 100644 --- a/crates/sqlite-store/src/account/tests.rs +++ b/crates/sqlite-store/src/account/tests.rs @@ -1468,6 +1468,94 @@ async fn lock_account_affects_latest_and_historical() -> anyhow::Result<()> { Ok(()) } +/// Verifies that a mismatched commitment already present in local history leaves the account +/// unlocked: the divergence is stale network data reporting a state the client already knows, not +/// a real conflict. +/// +/// `Client::sync_chain` reads the lock status back after applying a sync to decide which accounts +/// it actually locked, so this negative branch is what keeps `SyncSummary::locked_accounts` from +/// reporting a lock that never happened. +#[tokio::test] +async fn lock_account_ignores_commitment_present_in_history() -> anyhow::Result<()> { + let store = create_test_store().await; + let map_slot_name = StorageSlotName::new("test::lock::stale").expect("valid slot name"); + + // Insert account (nonce 1) and record the commitment of that state. + let mut account = setup_account_with_map(&store, 3, &map_slot_name).await?; + let account_id = account.id(); + let historical_commitment = account.to_commitment(); + + // Advance the account to nonce 2, which archives the nonce-1 header into the historical table. + let mut map_entries = StorageMapPatchEntries::new(); + map_entries.insert( + StorageMapKey::new([Felt::from(1u32), ZERO, ZERO, ZERO].into()), + [Felt::from(2000u32), ZERO, ZERO, ZERO].into(), + ); + let storage_patch = AccountStoragePatch::from_entries([( + map_slot_name.clone(), + StorageSlotPatch::Map(StorageMapPatch::Update { entries: map_entries }), + )])?; + let patch = AccountPatch::new( + account.id(), + storage_patch, + AccountVaultPatch::default(), + None, + Some(Felt::from(2u32)), + )?; + let prev_header: AccountHeader = (&account).into(); + account.apply_patch(&patch)?; + let final_header: AccountHeader = (&account).into(); + + let smt_forest = store.smt_forest.clone(); + let patch_clone = patch.clone(); + store + .interact_with_connection(move |conn| { + let old_map_roots = SqliteStore::get_storage_map_roots_for_patch( + conn, + account_id, + patch_clone.storage(), + )?; + let tx = conn.transaction().into_store_error()?; + let mut smt_forest = smt_forest.write().expect("smt_forest write lock not poisoned"); + SqliteStore::apply_account_patch( + &tx, + &mut smt_forest, + &prev_header, + &final_header, + &old_map_roots, + &patch, + )?; + tx.commit().into_store_error()?; + Ok(()) + }) + .await?; + + // The node reports the nonce-1 commitment, which the client already has in history. + store + .interact_with_connection(move |conn| { + let tx = conn.transaction().into_store_error()?; + SqliteStore::lock_account_on_unexpected_commitment( + &tx, + &account_id, + &historical_commitment, + )?; + tx.commit().into_store_error()?; + Ok(()) + }) + .await?; + + let (_header, status) = store + .interact_with_connection(move |conn| SqliteStore::get_account_header(conn, account_id)) + .await? + .expect("account should exist"); + assert!( + !status.is_locked(), + "a commitment already in local history must not lock the account" + ); + + Ok(()) +} + /// Verifies that undoing a patch after `update_account_state` does not resurrect entries that /// were removed by the update. This exercises the archival logic in `update_account_state`. ///