refactor(sync): split `sync_state` into `sync_chain` and `sync_note_transport` for granularity by JereSalo · Pull Request #2091 · 0xMiden/rust-sdk

JereSalo · 2026-04-21T18:52:13Z

Splits sync_state into two functions that can be called independently. Users that want to sync to the tip of the chain but don't care about the NTL can now call sync_chain, or if user wants to sync the NTL directly they could call sync_note_transport.

Closes (Partially) 0xMiden/web-sdk#92

sync_state previously returned Err when the Note Transport Layer fetch failed, taking down the on-chain portion of the sync with it. The NTL fetch is now treated as best-effort: its failure is caught, logged, and surfaced via a new SyncSummary.ntl_error: Option<String> field (ntlError() on the web client). Applications can check the field to disable private-note features while keeping public-note flows working.

Previously the whole ClientError was stringified, which produced the terse top-level Display ("note transport error") because ClientError's variants delegate detail to the error source chain rather than their own Display impl. Pattern-match ClientError::NoteTransportError and stringify the inner error directly, which carries the informative Display (e.g. "note transport network error: Fetch notes failed: ..."). Also stop silently swallowing non-NTL errors from fetch_transport_notes (store failures from cursor/import/sync-height lookups). Those are real issues unrelated to NTL infra and now propagate as before.

igamigo · 2026-04-22T18:04:18Z

+    /// Error message from the Note Transport Layer if its fetch failed during the sync.
+    /// `None` means NTL was either disabled or succeeded.
+    pub ntl_error: Option<String>,


Why not pass the error directly here, as opposed to a full string?

The problem is we'd have to implement PartialEq and Clone for NoteTransportError, and it has inside a Connection variant that has Box<dyn Error + Send + Sync>, which can implement neither and isn't serializable either.

Having the typed error there would be nice on the rust side but the WebClient will end up consuming a string anyway, right? I think it'd only be worth to do the necessary to have the error type only if we want to pattern match with it, until then a String would be simpler for now.

Let me know if we really don't want it to be a String though and we can take the necessary measures.

SantiagoPittella

It is conceptually odd continuing despite the error here. In case that the NTL fails, how can we continue fetch that later? Should this behavior be removed entirely into its own sync function?

SantiagoPittella · 2026-04-22T18:28:09Z

Lets update this function;s documentation including this behavior

igamigo · 2026-04-22T19:14:08Z

It is conceptually odd continuing despite the error here.

I agree a bit. This is why I suggested potentially having separate APIs for this (instead of putting everything behind Client::sync_state(). The other alternative is to make this an option of the StateSync component, which users can then use to decide whether to sync with NTL or not (StateSync::with_ntl_sync() or similar).
Although, all in all, this depends on to what extent we can assume the NTL service will be up in average.

decouple NTL fetching from Client::sync_state. sync_state now only performs the on-chain sync; sync_note_transport fetches private notes and sync_all runs both sequentially, failing fast on either. drop the ntl_error field from SyncSummary and expose syncNoteTransport / syncAll on the web client, with the sync lock made method-aware so same-method coalescing is preserved while different methods serialize.

JereSalo · 2026-04-22T21:47:41Z

It is conceptually odd continuing despite the error here. In case that the NTL fails, how can we continue fetch that later? Should this behavior be removed entirely into its own sync function?

I agree a bit. This is why I suggested potentially having separate APIs for this (instead of putting everything behind Client::sync_state(). The other alternative is to make this an option of the StateSync component, which users can then use to decide whether to sync with NTL or not (StateSync::with_ntl_sync() or similar).
Although, all in all, this depends on to what extent we can assume the NTL service will be up in average.

My bad, I just thought that this was going to be good enough for the user but after thinking about it I see that it's not the best UX because the user doesn't have the possibility to sync the NTL without syncing the rest if he wants to.
For now I'm converging towards having different methods, sync_state now doesn't sync the NTL, sync_note_transport syncs the NTL and sync_all is a wrapper for both (so it's like the previous sync_state). I'll see if names can be improved though, sync_all may not be ideal. The good thing is that we now have granularity.

I have pushed changes regarding this, the WebClient part is kinda dirty for now so I'll see to make it cleaner and perhaps if I (or you) think that the approach should be different I'll change it.

replace the two-phase acquire/release lock with a single withSyncLock(dbId, methodId, fn, timeoutMs) callback. native Promise sharing handles coalescing and error propagation, removing the manual waiter queues, syncGeneration tracking, and dual timeout layers. same semantics: same-method coalesces, different-method serializes via the Web Lock (or the WASM mutex as fallback), cross-tab coordination preserved.

the tests were calling sync_state expecting it to also fetch NTL notes, which no longer holds after the sync_state/sync_note_transport split. replace sync_state with sync_note_transport in the four pure-NTL tests. in fetch_private_notes_finds_note_committed_at_sync_height keep the first sync_state (setup: advances sync_height while NTL is empty, which is what creates the race) and swap only the second call to sync_note_transport (the behavior under test: NTL lookback).

…ault Client::sync_state now runs NTL then chain (failing fast on either) — the original combined semantics preserved at the original name. The on-chain-only path is exposed as Client::sync_chain. Client::sync_all is dropped since it was redundant with the new sync_state. JS parity: MidenClient.sync() stays combined, syncChain() is new, syncAll() is removed.

The earlier edits to sync_note_transport were needed while sync_state was chain-only. With sync_state back to its combined semantics, the original calls work unchanged, matching next.

The timeout only rejected the caller's promise; the underlying sync kept running in the background and eventually wrote to the store anyway. Drop the misleading API rather than keep a contract we cannot deliver. Callers who want a bail-out can wrap with Promise.race themselves.

The previous commit removed the `timeout` option from MidenClient.sync/syncChain/syncNoteTransport at runtime but missed the public-facing .d.ts. No CI job compares api-types.d.ts parameter shapes against the js implementation (check:standalone-types only verifies return types of forwarders in standalone.js), so the drift slipped through.

JereSalo · 2026-04-23T23:27:19Z

It wasn't my goal to make those WebClient changes in this PR but had to because I realized that otherwise it'd ended in very complex logic given that we now have 3 methods for syncing instead of just one. I think that making another PR for those changes as a follow-up would've been more churn for everyone.

juan518munoz

I'd probably rename SyncSummary to something like SyncChainSummary and create a wrapper type like:
SyncSummary = SyncChainSummary

This is just a nit that can be ommitted, but in my opinion helps with code clarity.

juan518munoz · 2026-04-28T14:50:37Z

   * @returns {Promise<SyncSummary>} The sync summary.
   */
-  async sync(opts) {
+  async sync() {


why have we removed the opts (timeout) here?

ropped the web timeout option (and syncStateWithTimeout) since it never cancelled the underlying sync.

Shouldn't we instead focus on making the timeout work?

I evaluated doing that in this PR but it implicating making a lot of changes. I will give it another try though but we gotta find out if it's actually necessary and how to make it work properly (and if it's even worth it to do so because of the complexity cost). I'll probably open an issue in the new web-sdk repository after evaluating this. I will have to migrate half of this PR over there anyway.

It can be left for a later revision, if so let's be sure to track it.

WiktorStarczewski · 2026-04-28T19:08:10Z

Heads-up: the web-sdk-related changes from this PR have been migrated to a new PR on the web-sdk repo: 0xMiden/web-sdk#30.

This is part of the ongoing split of web/WASM components from miden-client into a dedicated repo (#1992 / #2135). Once that split lands, the crates/web-client/ and crates/idxdb-store/ directories will no longer exist in miden-client, so the migration moves those changes to the right home now.

Please continue this PR with the miden-client-only changes; the web-sdk-side changes are tracked in 0xMiden/web-sdk#30. If you have write access to your branch, the cleanest follow-up is to drop the web-sdk file changes from this PR. Otherwise they'll naturally fall out when you rebase after #1992 / #2135 merges.

Note: the migrated web-sdk PR contains 3-way merge conflicts (drift between miden-client next and web-sdk next).

…couple-1930 # Conflicts: # crates/web-client/js/client.js # crates/web-client/js/constants.js # crates/web-client/js/index.js # crates/web-client/js/node/napi-compat.js # crates/web-client/js/resources/transactions.js # crates/web-client/js/syncLock.js # crates/web-client/js/types/api-types.d.ts # crates/web-client/js/types/index.d.ts # crates/web-client/js/workers/web-client-methods-worker.js # crates/web-client/scripts/check-method-classification.js # crates/web-client/src/sync.rs # crates/web-client/test/node-adapter.ts # crates/web-client/test/sync_lock.test.ts # crates/web-client/test/test-helpers.ts # docs/typedoc/web-client/classes/MidenClient.md

SantiagoPittella

Looks good, maybe we can improve a bit the documentation of

    pub async fn sync_state(&mut self) -> Result<SyncSummary, ClientError> {

With a somewhat brief explanation of what it does

TomasArrachea

LGTM.

Agreed with @SantiagoPittella that we could expand the Client::sync_state docs to explain briefly what is the sync state and mention the two internal methods in case the user needs them. BTW the now sync_chain method docs need updating.

…couple-1930

…' into jere/syncstate-ntl-decouple-1930

JereSalo · 2026-05-07T14:15:59Z

You're right, I expanded the docs for sync_state to give more info about what it does, the heavy part of the sync docs still lies in sync_chain because it's specific of that method.

BTW the now sync_chain method docs need updating.

I think it's up to date, it has the docs that sync_state used to have but with the added caveat that it doesn't fetch from the NTL and that for that goal it's best to use sync_state for combined sync or call sync_note_transport.

…mary Rewrite the step lists in StateSync::sync_state and Client::sync_chain so they match the actual order of operations and reference OnNoteReceived / NoteScreener. Make Client::sync_chain delegate to StateSync for the detailed pipeline to avoid duplicating the bullet list across both layers. Add a new_private_notes field to SyncSummary, populated from the IDs that sync_note_transport now returns (sourced from import_notes). This restores visibility into NTL-imported notes that would otherwise be missed by committed_notes when the lookback window catches a note already committed before the current sync height. Cover the field in the existing transport test.

TomasArrachea

Thanks!

igamigo · 2026-05-15T18:52:33Z


        println!("State synced to block {}", new_details.block_num);
        println!("New public notes: {}", new_details.new_public_notes.len());
+        println!("New private notes: {}", new_details.new_private_notes.len());


This is a bit ambiguous: you can get private notes from the chain itself (or rather, we get the inclusion information for a private note that the client knew about), but I believe this just really displays the private notes retrieved from the NTL, right? If so, we'd need to make this clearer IMO.

igamigo · 2026-05-15T18:55:45Z

+        let new_private_notes = self.sync_note_transport().await?;
+        let mut summary = self.sync_chain().await?;
+        summary.new_private_notes = new_private_notes;
+        Ok(summary)


nit: We could add something like SyncSummary::from_ntl_sync(fetched_note_ids: Vec<NoteId>) and have SyncSummary::from_chain_sync(/* all other parameters */) and then combine them with SyncSummary::combine_with() here to avoid mutating the internal field, and also avoid having to initialize the SyncSummary with an empty list of private notes every time. I wonder if we could also make all fields private instead of pub by default.

JereSalo added 3 commits April 21, 2026 12:37

tighten changelog entry and ntlError docstring

ee50832

JereSalo self-assigned this Apr 21, 2026

JereSalo added the maintainer PRs that come from internal contributors or integration partners. They should be given priority. label Apr 21, 2026

JereSalo marked this pull request as ready for review April 21, 2026 21:44

igamigo requested review from SantiagoPittella, WiktorStarczewski and igamigo April 22, 2026 18:03

igamigo reviewed Apr 22, 2026

View reviewed changes

SantiagoPittella reviewed Apr 22, 2026

View reviewed changes

JereSalo marked this pull request as draft April 22, 2026 19:40

JereSalo added 10 commits April 23, 2026 10:33

Merge branch 'next' into jere/syncstate-ntl-decouple-1930

eb087b6

add Node proxy aliases for syncNoteTransport and syncAll

7838054

run NTL before chain sync in sync_all

4987e2e

allow syncNoteTransportImpl and syncAllImpl in method classification

20c4688

silence unhandled rejection from sync lock cleanup chain

2d235d2

rename sync timeout error to reflect scope

6561ac3

restore transport tests to use sync_state

6ab4564

The earlier edits to sync_note_transport were needed while sync_state was chain-only. With sync_state back to its combined semantics, the original calls work unchanged, matching next.

JereSalo changed the title ~~feat(sync): keep Sync State running when the NTL fetch fails~~ refactor(sync): split sync_state into sync_chain and sync_note_transport for granularity Apr 23, 2026

JereSalo added 3 commits April 23, 2026 19:26

compress sync split changelog entries into one

37c65e1

JereSalo marked this pull request as ready for review April 23, 2026 23:39

regenerate typedoc after removing timeout option

8ad4274

JereSalo added this to the v0.15 milestone Apr 27, 2026

TomasArrachea self-requested a review April 28, 2026 03:08

juan518munoz reviewed Apr 28, 2026

View reviewed changes

document sync lock helpers

e212878

WiktorStarczewski mentioned this pull request Apr 28, 2026

refactor(sync): split sync_state into sync_chain and sync_note_transport for granularity 0xMiden/web-sdk#30

Merged

JereSalo added 3 commits April 29, 2026 16:42

reframe sync split changelog entry as enhancement

e34cc46

trim redundant sync_state line from changelog entry

f0d1b94

JereSalo requested a review from juan518munoz April 29, 2026 20:07

Merge branch 'next' into jere/syncstate-ntl-decouple-1930

b33b7f0

SantiagoPittella approved these changes May 5, 2026

View reviewed changes

expand sync state documentation

ab17474

TomasArrachea approved these changes May 6, 2026

View reviewed changes

JereSalo added 3 commits May 6, 2026 16:51

Merge remote-tracking branch 'origin/next' into jere/syncstate-ntl-de…

b9ad816

…couple-1930

Merge remote-tracking branch 'origin/jere/syncstate-ntl-decouple-1930…

57ea066

…' into jere/syncstate-ntl-decouple-1930

link sync_note_transport and sync_chain in sync_state docs

ccb3f4f

TomasArrachea reviewed May 7, 2026

View reviewed changes

Comment thread crates/rust-client/src/sync/mod.rs Outdated

Comment thread crates/rust-client/src/sync/mod.rs

TomasArrachea reviewed May 8, 2026

View reviewed changes

Comment thread crates/rust-client/src/sync/mod.rs

print new_private_notes count in CLI sync command

5fe0968

TomasArrachea approved these changes May 8, 2026

View reviewed changes

SantiagoPittella merged commit bca5f5f into next May 8, 2026
20 checks passed

SantiagoPittella deleted the jere/syncstate-ntl-decouple-1930 branch May 8, 2026 19:44

igamigo reviewed May 15, 2026

View reviewed changes

Uh oh!

Conversation

JereSalo commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SantiagoPittella left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igamigo commented Apr 22, 2026

Uh oh!

JereSalo commented Apr 22, 2026

Uh oh!

JereSalo commented Apr 23, 2026

Uh oh!

juan518munoz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

WiktorStarczewski commented Apr 28, 2026

Uh oh!

SantiagoPittella left a comment

Choose a reason for hiding this comment

Uh oh!

TomasArrachea left a comment

Choose a reason for hiding this comment

Uh oh!

JereSalo commented May 7, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TomasArrachea left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JereSalo commented Apr 21, 2026 •

edited

Loading