feat: stream SyncState response #751

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

This PR updates the client's sync command to adapt to the new Streaming response introduced in 0xPolygonMiden/miden-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.

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

@@ -332,25 +338,13 @@ impl StateSync {
                    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

@@ -10,6 +10,7 @@

 * 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

TomasArrachea mentioned this pull request Feb 20, 2025

feat: stream SyncState response #734

Closed

TomasArrachea force-pushed the tomasarrachea-sync-component-with-streaming branch from 11fa97f to 986fb53 Compare February 21, 2025 17:57

TomasArrachea added 4 commits February 21, 2025 15:38

feat: udpate rpc interface

2b75a77

feat: read sync state stream response

38602aa

feat: get nullifiers after state sync

e26d512

fix: use ProvenBlocks in mock

fee56fe

TomasArrachea force-pushed the tomasarrachea-sync-component-with-streaming branch from 65140f6 to fee56fe Compare February 21, 2025 18:51

fix: merge

5dcb17e

tomyrd reviewed Feb 21, 2025

View reviewed changes

tomyrd mentioned this pull request Feb 24, 2025

feat: add StateSync component #650

Open

5 tasks

TomasArrachea mentioned this pull request Feb 24, 2025

feat: add block number to CheckNullifiersByPrefix endpoint #749

Closed

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 tomyrd, igamigo and SantiagoPittella 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 5 commits February 25, 2025 16:35

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: stream SyncState response #751

feat: stream SyncState response #751

TomasArrachea commented Feb 20, 2025 •

edited

Loading

tomyrd left a comment

tomyrd Feb 21, 2025

TomasArrachea Feb 21, 2025

TomasArrachea Feb 21, 2025

igamigo Feb 25, 2025

TomasArrachea Feb 25, 2025

tomyrd Feb 21, 2025

tomyrd Feb 21, 2025

tomyrd Feb 21, 2025

tomyrd left a comment

tomyrd Feb 25, 2025

tomyrd Feb 25, 2025

tomyrd Feb 25, 2025

igamigo left a comment

igamigo Feb 25, 2025

igamigo Feb 25, 2025

igamigo Feb 25, 2025

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

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

feat: stream SyncState response #751

Are you sure you want to change the base?

feat: stream SyncState response #751

Conversation

TomasArrachea commented Feb 20, 2025 • edited Loading

tomyrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomyrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igamigo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasArrachea commented Feb 20, 2025 •

edited

Loading