feat: stream SyncState response by TomasArrachea · Pull Request #751 · 0xMiden/miden-client

TomasArrachea · 2025-02-20T21:48:35Z

This PR updates the client's sync command to adapt to the new Streaming response introduced in 0xMiden/node#685.

The client is no longer needed to execute the sync request multiple times until the chain tip is reached, it just needs to read the multiple updates from the response Stream.

…into tomyrd-sync-component-alt

* feat: remove steps from state sync interface * feat: add block relevance in `StateSyncUpdate` * fix: web client build * chore: update docs * add failing test related to TODOs * fix: add nullifiers for new untracked notes in each step * use `Arc<RwLoc<NoteUpdates>>` in callbacks * feat: reduce the number of callbacks * remove `NoteUpdates` as callback parameter * refactor callbacks * update docs * change callbacks and move state transitions * fix: web store * refator: use partial mmr with current block in sync

tomyrd

Not a full review, I'll wait for the merge/rebase so the changes are a bit more clear.

tomyrd · 2025-02-21T19:21:52Z

crates/rust-client/src/sync/state_sync.rs

+        while let Some(res) = stream
+            .message()
+            .await
+            .map_err(|e| ClientError::RpcError(ConnectionError(e.message().to_string())))?
+        {
+            self.sync_state_step(
+                res.try_into().map_err(ClientError::RpcError)?,


I'm still not sure about having this logic in the client side vs the rpc side. We even have to transform between the rpc and domain types here which is something usually done inside the rpc client.

I think that the problem is that moving this logic to the rpc side would mean that the rpc_api.sync_state call will need to wait until the stream is closed and return all the collected sync responses. Those would be processed on the client side once the stream is closed. I'm not convinced if that is the best way to go, doesn't seem to go along with the streaming logic.

Maybe a good solution would be that the rpc side returns an iterator that lazily maps the response into a domain type, but not sure if that is possible

Agree with both of you! I think a wrapper over the stream that converts the responses appropriatly could work out well

Implemented this wrapper as SyncStateStream on 9dcdcac

tomyrd · 2025-02-21T19:23:44Z

crates/rust-client/src/sync/state_sync.rs

+        // New nullfiers should be added for new untracked notes
+        nullifiers_tags.append(
+            &mut state_sync_update
+                .note_updates
+                .updated_input_notes()
+                .map(|note| get_nullifier_prefix(&note.nullifier()))
+                .collect::<Vec<_>>(),
+        );


I would remove the declaration of the nullifier_tags at the start of this function and directly build the array here with the values from the note_updates

tomyrd · 2025-02-21T19:25:01Z

crates/rust-client/src/sync/state_sync.rs

+                }
+            }
        }
+        let transaction_updates = TransactionUpdates::new(transactions, discarded_transactions);


I would remove transactions if we don't use it:

Suggested change

let transaction_updates = TransactionUpdates::new(transactions, discarded_transactions);

let transaction_updates = TransactionUpdates::new(vec![], discarded_transactions);

tomyrd · 2025-02-21T19:29:31Z

crates/rust-client/src/sync/state_sync.rs

@@ -322,17 +331,14 @@ impl StateSync {
        &mut self,


We should remove the The transaction updates might include: part of the doc comment above as we don't update the transactions here anymore. I would move each bullet point to the section in the code where we make each update.

…onent-with-streaming

tomyrd

LGTM, left some minor comments

tomyrd · 2025-02-25T15:40:17Z

crates/rust-client/src/sync/state_sync.rs

+        let unspent_nullifiers = unspent_input_notes
+            .iter()
+            .map(InputNoteRecord::nullifier)
+            .chain(unspent_output_notes.iter().filter_map(OutputNoteRecord::nullifier));


I think this isn't needed. The unspent nullifiers should already be included in the state_sync_update.note_updates. (This also means that we could remove the .clone() from these vecs)

tomyrd · 2025-02-25T15:43:05Z

crates/rust-client/src/sync/state_sync.rs

+        // * Transactions that were committed. Some of these might be tracked by the client and need
+        //   to be marked as committed.


This part of the transaction update is note done here. This part only deals with discarded transactions. I would move this to line 214 where we add committed transactions.

tomyrd · 2025-02-25T15:46:29Z

crates/rust-client/src/sync/state_sync.rs

                    note_updates.insert_updates(Some(public_note), None);
                }

+                //


There's an empty comment here

igamigo

Looks great! Leaving some comments for now

igamigo · 2025-02-25T17:33:34Z

CHANGELOG.md


 * Add check for empty pay to id notes (#714).
 * [BREAKING] Refactored authentication out of the `Client` and added new separate authenticators (#718).
+* Read Sync State response from stream (#751).


nit: Let's write this in past tense

igamigo · 2025-02-25T17:50:41Z

crates/rust-client/src/sync/state_sync.rs

+        // Check for new nullifiers for input notes that were updated
+        let nullifiers_tags: Vec<u16> = state_sync_update
+            .note_updates
+            .updated_input_notes()
+            .map(|note| note.nullifier().prefix())
+            .collect();
+
+        let new_nullifiers = self
+            .rpc_api
+            .check_nullifiers_by_prefix(&nullifiers_tags, current_block_num)
+            .await?;
+
+        // Process nullifiers and track the updates of local tracked transactions that were
+        // discarded because the notes that they were processing were nullified by an
+        // another transaction.
+        let mut discarded_transactions = vec![];
+
+        for (nullifier, block_num) in new_nullifiers {
+            let nullifier_update = NullifierUpdate { nullifier, block_num };
+
+            let discarded_transaction =
+                state_sync_update.note_updates.apply_nullifiers_state_transitions(
+                    &nullifier_update,
+                    state_sync_update.transaction_updates.committed_transactions(),
+                )?;
+
+            if let Some(transaction_id) = discarded_transaction {
+                discarded_transactions.push(transaction_id);
+            }
        }
+        let transaction_updates = TransactionUpdates::new(vec![], discarded_transactions);
+        state_sync_update.transaction_updates.extend(transaction_updates);


I think we could factor this out to its own fucntion (sometihng like sync_nullifiers())

igamigo · 2025-02-25T17:52:06Z

crates/rust-client/src/sync/state_sync.rs

+        while let Some(res) = stream
+            .message()
+            .await
+            .map_err(|e| ClientError::RpcError(ConnectionError(e.message().to_string())))?
+        {
+            self.sync_state_step(
+                res.try_into().map_err(ClientError::RpcError)?,


Agree with both of you! I think a wrapper over the stream that converts the responses appropriatly could work out well

…onent-with-streaming

tomyrd · 2025-03-31T14:34:42Z

closing as it's too outdated with the client and most of the changes here are already included in #817 . We can open a new PR if we want to add the stream sync and use this as an example.

tomyrd and others added 30 commits December 30, 2024 17:17

feat: give interior mutability to NodeRpcClient

6a7579f

refactor: move client's rpc_api to Arc

991779e

feat: add StateSync component (wip)

d7c7fb6

feat: remove old sync structs

0812700

feat: update output notes on sync

c1b8c4f

feat: check for locked accounts in state sync

435682b

doc: improve documentation

b61262d

refactor: revert unnecessary changes

d54e186

Merge branch 'next' into tomyrd-sync-component-alt

922b862

refactor: move state transitions outside StateSync

a499748

feat: add update callbacks

b0930cf

refactor: change callbacks to deal with individual elements

170cc85

chore: improve code structure and documentation

599a801

fix: update web store

2645e7a

chore: update CHANGELOG

2f26007

Merge branch 'next' into tomyrd-sync-component-alt

e86c024

fix: Various stylistic fixes and refactors

b524978

refactor: move function outside of impl

2ff1c91

Merge branch 'next' into tomyrd-sync-component-alt

a14e73c

chore: fix clippy suggestiosn

da532db

chore: fix clippy suggestiosn

6b5b36e

chore: Merge next

6321312

chore: Merge next

268c9f5

chore: Format

77d91ce

Merge branch 'next' of https://github.com/0xPolygonMiden/miden-client …

fcaf826

…into tomyrd-sync-component-alt

Merge branch 'next' into tomyrd-sync-component-alt

aafa399

review: naming and sections

6b63d59

fix: move and update note tag test

c1281f0

review: improve StateSync comments

12e4706

tomyrd reviewed Feb 21, 2025

View reviewed changes

tomyrd mentioned this pull request Feb 24, 2025

feat: add StateSync component #650

Merged

5 tasks

fix: use Nullifier::prefix

d1c8490

TomasArrachea mentioned this pull request Feb 24, 2025

feat: add block number to CheckNullifiersByPrefix endpoint #749

Closed

tomyrd added 2 commits February 24, 2025 14:54

review: address suggestions

728f3f6

review: remove OnNullifierReceived callback

2090593

igamigo mentioned this pull request Feb 24, 2025

feat: Make sync compatible with node's next #758

Merged

TomasArrachea force-pushed the tomasarrachea-sync-component-with-streaming branch from c2b31ab to e6dc2f1 Compare February 24, 2025 20:15

TomasArrachea changed the title ~~WIP: stream SyncState response~~ feat: stream SyncState response Feb 24, 2025

feat: sync state refactor and testing

cf5d48b

TomasArrachea force-pushed the tomasarrachea-sync-component-with-streaming branch from 97ee6db to cf5d48b Compare February 24, 2025 20:47

Merge branch 'tomyrd-sync-component-alt' into tomasarrachea-sync-comp…

1b666fe

…onent-with-streaming

TomasArrachea marked this pull request as ready for review February 24, 2025 21:05

TomasArrachea requested review from SantiagoPittella, igamigo and tomyrd February 24, 2025 21:59

tomyrd approved these changes Feb 25, 2025

View reviewed changes

review: remove unnecessary nullifiers and update comments

e9ff81b

igamigo reviewed Feb 25, 2025

View reviewed changes

TomasArrachea added 7 commits February 25, 2025 14:53

Merge branch 'next' into new-state-sync

b9589a9

Merge branch 'new-state-sync' into tomyrd-sync-component-alt

9c99579

Merge branch 'tomyrd-sync-component-alt' into tomasarrachea-sync-comp…

bb15ea3

…onent-with-streaming

fix: merge tests

e2871ed

chore: clippy

bc6faf9

review: refactor sync stream and update CHANGELOG

9dcdcac

feat: add docs for SyncStateStream

4d4c439

Base automatically changed from tomyrd-sync-component-alt to next March 28, 2025 19:53

tomyrd closed this Mar 31, 2025

bobbinth deleted the tomasarrachea-sync-component-with-streaming branch July 3, 2025 05:57

	let transaction_updates = TransactionUpdates::new(transactions, discarded_transactions);
	let transaction_updates = TransactionUpdates::new(vec![], discarded_transactions);

		// * Transactions that were committed. Some of these might be tracked by the client and need
		// to be marked as committed.

Conversation

TomasArrachea commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomyrd 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!

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!

Choose a reason for hiding this comment

Uh oh!

tomyrd 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!

igamigo 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!

tomyrd commented Mar 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TomasArrachea commented Feb 20, 2025 •

edited

Loading