feat: add SyncTransaction endpoint by SantiagoPittella · Pull Request #1224 · 0xMiden/node

SantiagoPittella · 2025-09-10T20:44:08Z

closes #1207

bobbinth

Thank you! Looks good. I left some comments inline. The main things are:

We need to store more data in the transactions table to avoid parsing raw blocks.
We need to figure out how to limit response size (or at least send an error if the response is too big).

proto/proto/store/rpc.proto

crates/store/src/server/rpc_api.rs

crates/store/src/db/models/queries/transactions.rs

bobbinth

Thank you! Looks good. Not a full review but left some comments inline. The main thing is to fix the methodology for response size estimation. Once you come up with methodology, could you describe it before implementing?

crates/store/src/db/migrations/2025062000000_setup/up.sql

proto/proto/store/rpc.proto

crates/store/src/state.rs

crates/store/src/db/models/queries/transactions.rs

bobbinth

Looks good! Thank you. I left some more comments inline.

crates/store/src/db/mod.rs

crates/store/src/db/models/queries/transactions.rs

crates/store/src/db/migrations/2025062000000_setup/up.sql

bobbinth

Looks good! Thank you. I left some more comments inline.

crates/store/src/db/models/queries/transactions.rs

bobbinth · 2025-09-20T04:47:47Z

One more general comment: there is a much more efficient way to serialize note inclusion paths which would guarantee that we fit into 5MB limit even if a block contained $2^{16}$ output notes. It is quite a bit more involved though - instead of sending a Merkle path with every node, we'd need to send a single partial SMT for an entire block. So, it is not something we should do now, but let's create an issue for this.

SantiagoPittella · 2025-09-25T13:58:56Z

@SantiagoPittella - what's outstanding in this PR? I think maybe just #1224 (comment) is left?

I just addressed the offset comment, replacing it with a cursor-based approach. Every other comment was previously addressed 👌🏼 .
cc @Mirko-von-Leipzig

bobbinth

Looks good! Thank you. I left a couple more comments inline. The main one is that I'm not sure the logic for filtering out the last block is correct.

crates/store/src/db/models/queries/transactions.rs

bobbinth

Looks good! Thank you. I left a couple more comments inline.

Also, not for this PR, but it would be great to test this functionality somehow - but not really sure what's the best way to do it. Let's create an issue for this.

crates/store/src/db/models/queries/transactions.rs

igamigo · 2025-09-29T22:52:10Z

crates/store/src/db/models/queries/transactions.rs

+        //
+        // Note: 500 bytes per output note is an over-estimate but ensures we don't
+        // exceed memory limits when these transactions are later converted to proto records.
+        let header_base_size = 4 + 32 + 16 + 64; // block_num + tx_id + account_id + commitments


This could be const, right?

proto/proto/store/rpc.proto

crates/store/src/server/rpc_api.rs

igamigo · 2025-09-29T23:16:40Z

crates/store/src/server/rpc_api.rs

+            let note_records = self
+                .state
+                .get_notes_by_id(tx_header.output_notes.clone())
+                .await
+                .map_err(internal_error)?;


This might have been discussed before (maybe for a follow up) but going to the store for every single transaction seems potentially wasteful if in a single query you can fit note responses for more than one of them. I think joining the tables is also not very desirable here but that could be another alternative.

I'd add it to the list of follow-ups. We could make a single query that brings back note data for all transactions in the response - but that's a bit more involved and shouldn't make a huge difference for an in-process database (but would be very important if if the database was on a different machine).

igamigo · 2025-09-30T01:05:39Z

crates/store/src/db/models/queries/transactions.rs

+                last_block_num = Some(tx.block_num);
+                last_transaction_id = Some(tx.transaction_id.clone());


Looks like the clone() (and the assignment) happen per-row but are really only needed for the last row.

I split a bit this logic to avoid the extra clones.

igamigo · 2025-09-30T01:38:07Z

crates/store/src/db/models/queries/transactions.rs

+            all_transactions
+                .into_iter()
+                .take_while(|row| row.block_num != last_block_num)
+                .collect::<Vec<_>>(),


nit: I think this collect could be avoided as well, right?

igamigo · 2025-09-30T01:47:33Z

crates/store/src/db/models/queries/transactions.rs

+        let last_included_block = last_block_num.map_or(*block_range.end(), |block_num| {
+            BlockNumber::from_raw_sql(block_num).expect("valid block number from database")
+        });


Since this is the case where the max payload size was not reached, would this always be equal to the end of the block range?

Yes, makes sense. The response, in practice, is the same but in the next iteration we avoid going to blocks that we already checked and we saw that didn't matched our criteria. I;/m moving to block_range.end().

igamigo · 2025-09-30T01:51:09Z

crates/store/src/db/models/queries/transactions.rs

+#![allow(
+    clippy::cast_possible_wrap,
+    reason = "We will not approach the item count where i64 and usize cause issues"
+)]


Would this be better to put into the function itself?

igamigo · 2025-09-30T02:06:29Z

crates/store/src/db/models/queries/transactions.rs

+        // If no more data, we're done
+        if chunk.is_empty() {
+            break;
+        }


nit: I think I'd remove this if because it's functionally very similar to the if after the for loop and it doesn't seem to be too beneficial to return before it since chunk would contain no transactions to loop through

igamigo · 2025-09-30T02:11:04Z

Overall logic looking good. Left a few comments mainly related to avoiding unnecessary allocations and a few minor nits (feel free to disregard if you think they don't apply)

bobbinth

Looks good! Thank you! I think after @igamigo's comments are addressed, this is good to merge.

bobbinth · 2025-09-30T20:21:16Z

proto/proto/store/rpc.proto

+    // List of transaction records.
+    repeated TransactionRecord transaction_records = 2;


I'd name this just transactions - let's add it to the follow-up.

bobbinth · 2025-09-30T20:22:21Z

proto/proto/store/rpc.proto

+    // A transaction header.
+    transaction.TransactionHeader transaction_header = 2;


nit: maybe I'd change this to just header. Let's add this to the follow-ups as well.

SantiagoPittella force-pushed the santiagopittella-sync-transaction-endpoint branch from f8b6e1e to 2f04a15 Compare September 10, 2025 20:44

feat: add SyncTransaction endpoint

cea5e7f

SantiagoPittella force-pushed the santiagopittella-sync-transaction-endpoint branch from 2f04a15 to cea5e7f Compare September 10, 2025 20:45

SantiagoPittella requested review from Mirko-von-Leipzig, bobbinth and igamigo and removed request for bobbinth September 10, 2025 20:45

bobbinth reviewed Sep 11, 2025

View reviewed changes

proto/proto/store/rpc.proto Show resolved Hide resolved

crates/store/src/server/rpc_api.rs Outdated Show resolved Hide resolved

crates/store/src/db/models/queries/transactions.rs Outdated Show resolved Hide resolved

SantiagoPittella added 4 commits September 11, 2025 17:18

review: add missing fields to transaction table in DB

1dac0c4

review: add raw sql query

cee3a73

review: add docs to proto message

8547c82

review: add limit to transaction headers in response

30478f8

bobbinth reviewed Sep 12, 2025

View reviewed changes

SantiagoPittella added 5 commits September 12, 2025 11:17

review: add docs to transaction table fields

71825ed

review: update comments on proto message

f5a1c77

chore: remove load_block_data

be0b5cb

refactor: entries limit

d030abb

chore: remove unwrap

8934058

bobbinth reviewed Sep 13, 2025

View reviewed changes

SantiagoPittella added 4 commits September 15, 2025 16:22

Merge branch 'next' into santiagopittella-sync-transaction-endpoint

dd6a216

review: rename TransacionHeader to TransactionRecord

3195c66

review: improve size estimation, improve sql docs

c639e82

review: use chunks loading approach

9881ca5

This comment was marked as outdated.

Sign in to view

SantiagoPittella requested a review from bobbinth September 17, 2025 20:40

Merge branch 'next' into santiagopittella-sync-transaction-endpoint

a9d8580

bobbinth reviewed Sep 20, 2025

View reviewed changes

Merge branch 'next' into santiagopittella-sync-transaction-endpoint

c70ee41

Mirko-von-Leipzig approved these changes Sep 25, 2025

View reviewed changes

review: use cursor-based pagination

ee10573

SantiagoPittella requested a review from bobbinth September 25, 2025 13:59

SantiagoPittella mentioned this pull request Sep 25, 2025

Benchmark and improve performance of SyncTransaction query #1243

Open

bobbinth reviewed Sep 25, 2025

View reviewed changes

crates/store/src/db/models/queries/transactions.rs Outdated Show resolved Hide resolved

crates/store/src/db/models/queries/transactions.rs Outdated Show resolved Hide resolved

review: use more standard approach

3accb18

bobbinth reviewed Sep 28, 2025

View reviewed changes

crates/store/src/db/models/queries/transactions.rs Outdated Show resolved Hide resolved

crates/store/src/db/models/queries/transactions.rs Outdated Show resolved Hide resolved

crates/store/src/db/models/queries/transactions.rs Outdated Show resolved Hide resolved

Review: use if/else approach

3e3869a

igamigo mentioned this pull request Sep 29, 2025

Complete RPC endpoint implementations 0xMiden/miden-client#1349

Closed

SantiagoPittella requested a review from bobbinth September 29, 2025 16:40

igamigo reviewed Sep 30, 2025

View reviewed changes

bobbinth approved these changes Sep 30, 2025

View reviewed changes

SantiagoPittella added 11 commits September 30, 2025 14:41

review: add TransactionHeader message & HEAD_BASE_SIZE constant

cad1550

review: create vector with capacity

70e8ed0

review: reduce type conversions

b6c27e3

review: remove duplicated reference

7061553

review: rename to into_proto_with_note_recods

91f94d2

review: use inclusive range

92d8d27

review: remove unused clones

b4f9197

review: remove collect call

1971e4b

review: use block_range.end

23fe9a3

review: allow lint rule only in function

275f804

review: remove if

9597125

SantiagoPittella mentioned this pull request Sep 30, 2025

SyncTransactions follow-ups #1258

Closed

SantiagoPittella merged commit 7f51765 into next Sep 30, 2025
6 checks passed

SantiagoPittella deleted the santiagopittella-sync-transaction-endpoint branch September 30, 2025 20:13

bobbinth reviewed Sep 30, 2025

View reviewed changes

		last_block_num = Some(tx.block_num);
		last_transaction_id = Some(tx.transaction_id.clone());

		// List of transaction records.
		repeated TransactionRecord transaction_records = 2;

		// A transaction header.
		transaction.TransactionHeader transaction_header = 2;

Conversation

SantiagoPittella commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bobbinth commented Sep 20, 2025

Uh oh!

SantiagoPittella commented Sep 25, 2025

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

igamigo commented Sep 30, 2025

Uh oh!

bobbinth left a comment

SantiagoPittella commented Sep 10, 2025 •

edited

Loading