feat: Add FPI support to NtxDataStore by sergerad · Pull Request #1521 · 0xMiden/node

sergerad · 2026-01-16T00:18:33Z

Context

The NTX Builder's NtxDataStore does not handle foreign accounts for the purposes of the following functions of DataStore trait:

get_foreign_account_inputs()
get_foreign_account_inputs()
get_vault_asset_witnesses()

Closes #1387.

Changes

Updates NtxDataStore to handle foreign accounts in the above-listed functions.
Update NTX Builder store client to support get_account() like the RPC client.

…x-builder-fpi

bobbinth

Looks good! Thank you! I left some comments inline - the main ones are about making the code a bit more NTX builder-specific, and also adding helper endpoints on the store to retrieve asset/map witnesses (in a different PR).

crates/store/src/server/ntx_builder.rs

crates/proto/src/errors/mod.rs

crates/ntx-builder/src/store.rs

crates/ntx-builder/src/actor/execute.rs

crates/proto/src/domain/account.rs

…x-builder-fpi

bobbinth

Looks good! Thank you! I left a few more comments inline - all a pretty small.

crates/ntx-builder/src/actor/execute.rs

crates/ntx-builder/src/store.rs

crates/proto/src/errors/mod.rs

bobbinth

All looks good! Thank you!

Mirko-von-Leipzig

Code looks good; I still dislike how datastore forces arcs and mutex's despite being run effectively sequentially. But that's not this PRs problem.

Mirko-von-Leipzig · 2026-01-22T10:07:07Z

crates/ntx-builder/src/actor/execute.rs

+            // The slot name that corresponds to the given account ID and map root must have been
+            // registered during previous calls of this data store.
+            let storage_slots = self.storage_slots.lock().await;
+            let Some(slot_name) = storage_slots.get(&(account_id, map_root)) else {
+                return Err(DataStoreError::other(
+                    "requested storage slot has not been registered",
+                ));
+            };


Are we essentially relying on knowing the order of operations of the caller of data store?

nit: Seems like you can drop the lock by this point

bobbinth · 2026-01-22T18:10:11Z

crates/ntx-builder/src/actor/execute.rs

+        async move {
+            debug_assert_eq!(ref_block, self.reference_block.block_num());
+
+            // Get foreign account inputs from store.
+            let account_inputs =
+                self.store.get_account_inputs(foreign_account_id, ref_block).await.map_err(
+                    |err| DataStoreError::other_with_source("failed to get account inputs", err),
+                )?;
+
+            // Register slot names from the foreign account for later use.
+            self.register_storage_map_slots(foreign_account_id, account_inputs.storage().header())
+                .await;
+
+            Ok(account_inputs)


Following up on the conversation we had during the call, I actually don't think we need to add foreign account code to the MAST store in this function (i.e., nothing needs to change). The reason for this is that transaction host does this after calling DataStore::get_foreign_account_inputs().

Technically, I think we didn't have to do this in the validator either - but in the validator, we are planning to change how re-execution works and so, I'd leave it as is.

cc @igamigo to confirm.

Looking at the code, I think this is right. But also, not sure why the validator was erroring during client integration FPI tests then (which prompted #1561).

Oh actually it's because it never even calls DataStore::get_foreign_account_inputs().

Although then I wonder how you plan on doing away with adding the MAST forest in the validator.

bobbinth · 2026-01-22T18:29:53Z

crates/ntx-builder/src/actor/execute.rs

+    /// Mapping of storage map roots to storage slot names observed during various calls.
+    ///
+    /// The registered slot names are subsequently used to retrieve storage map witnesses from the
+    /// store. We need this because the store interface (and the underling SMT forest) use storage
+    /// slot names, but the `DataStore` interface works with tree roots. To get around this problem
+    /// we populate this map when:
+    /// - The the native account is loaded (in `get_transaction_inputs()`).
+    /// - When a foreign account is loaded (in `get_foreign_account_inputs`).
+    ///
+    /// The assumption here are:
+    /// - Once an account is loaded, the mapping between `(account_id, map_root)` and slot names
+    ///   do not change. This is always the case.
+    /// - New storage slots created during transaction execution will not be accesses in the same
+    ///   transaction. The mechanism for adding new storage slots is not implemented yet, but the
+    ///   plan for it is consistent with this assumption.
+    ///
+    /// One nuance worth mentioning: it is possible that there could be a root collision where an
+    /// account has two storage maps with the same root. In this case, the map will contain only a
+    /// single entry with the storage slot name that was added last. Thus, technically, requests
+    /// to the store could be "wrong", but given that two identical maps have identical witnesses
+    /// this does not cause issues in practice.
+    storage_slots: Arc<Mutex<BTreeMap<(AccountId, Word), StorageSlotName>>>,


Expanded this comment. cc @Mirko-von-Leipzig.

igamigo

Sorry for the delay, LGTM! I left some comments, not sure if it's worth addressing any of them right now

igamigo · 2026-01-22T21:37:40Z

crates/ntx-builder/src/actor/execute.rs

+        async move {
+            debug_assert_eq!(ref_block, self.reference_block.block_num());
+
+            // Get foreign account inputs from store.
+            let account_inputs =
+                self.store.get_account_inputs(foreign_account_id, ref_block).await.map_err(
+                    |err| DataStoreError::other_with_source("failed to get account inputs", err),
+                )?;
+
+            // Register slot names from the foreign account for later use.
+            self.register_storage_map_slots(foreign_account_id, account_inputs.storage().header())
+                .await;
+
+            Ok(account_inputs)


Looking at the code, I think this is right. But also, not sure why the validator was erroring during client integration FPI tests then (which prompted #1561).

igamigo · 2026-01-22T21:38:24Z

crates/ntx-builder/src/actor/execute.rs

+        async move {
+            debug_assert_eq!(ref_block, self.reference_block.block_num());
+
+            // Get foreign account inputs from store.
+            let account_inputs =
+                self.store.get_account_inputs(foreign_account_id, ref_block).await.map_err(
+                    |err| DataStoreError::other_with_source("failed to get account inputs", err),
+                )?;
+
+            // Register slot names from the foreign account for later use.
+            self.register_storage_map_slots(foreign_account_id, account_inputs.storage().header())
+                .await;
+
+            Ok(account_inputs)


Oh actually it's because it never even calls DataStore::get_foreign_account_inputs().

Although then I wonder how you plan on doing away with adding the MAST forest in the validator.

igamigo · 2026-01-22T21:47:52Z

crates/ntx-builder/src/store.rs

+            self.inner.clone().get_vault_asset_witnesses(request).await?.into_inner();
+
+        // Convert the response to domain type.
+        let mut asset_witnesses = Vec::new();


nit: can use Vec::with_capacity here

igamigo · 2026-01-22T21:52:53Z

crates/ntx-builder/src/actor/execute.rs

+            // The slot name that corresponds to the given account ID and map root must have been
+            // registered during previous calls of this data store.
+            let storage_slots = self.storage_slots.lock().await;
+            let Some(slot_name) = storage_slots.get(&(account_id, map_root)) else {
+                return Err(DataStoreError::other(
+                    "requested storage slot has not been registered",
+                ));
+            };


nit: Seems like you can drop the lock by this point

igamigo · 2026-01-22T21:53:44Z

crates/ntx-builder/src/actor/execute.rs

    TransactionMastStore,
    TransactionProverError,
 };
+use tokio::sync::Mutex;


Because we know this lock is low-contention and also not held across awaits, I think an std::sync::Mutex would be lower overhead and overall faster

sergerad added 2 commits January 16, 2026 12:42

Add ntx data store impl and grpc client/server changes

523347b

Update get_storage_map_witness

5f071e3

sergerad mentioned this pull request Jan 16, 2026

feat: Support foreign accounts #1486

Closed

sergerad added 4 commits January 16, 2026 13:20

Changelog

5af082a

Add partial storage logic

9b49133

Merge branch 'next' of github.com:0xMiden/miden-node into sergerad-nt…

eb26ce2

…x-builder-fpi

Comment and collect nits

8f3c1c6

sergerad requested review from Mirko-von-Leipzig and bobbinth January 16, 2026 03:46

sergerad added 2 commits January 19, 2026 07:01

RM account id branch

220385f

Simplify datastore other errs

a517beb

bobbinth reviewed Jan 18, 2026

View reviewed changes

Rename request variable

d706d3f

sergerad mentioned this pull request Jan 18, 2026

Refactor ConversionError enum #1528

Open

sergerad added 2 commits January 19, 2026 12:12

Fix more variable names

66e94cd

Fill details request with todos

71571bd

This was referenced Jan 19, 2026

chore: Add serde for StorageMapWitness 0xMiden/protocol#2305

Closed

feat: Add store endpoints GetVaultAssetWitnesses and GetStorageMapWitness #1529

Merged

sergerad added 11 commits January 20, 2026 13:15

Register slot names in ntx data store

4f01a6d

Use arc mutex

6c4463c

Add temporary snippet with todo

00c352d

Fix details request fields

a124390

Merge branch 'next' of github.com:0xMiden/miden-node into sergerad-nt…

9ea0855

…x-builder-fpi

Merge branch 'next' of github.com:0xMiden/miden-node into sergerad-nt…

bbc38bd

…x-builder-fpi

Add ntx builder client fns for store witness endpoints and integrate

801cda0

Replace try_from

d4b3a77

Vault root only

0ce950b

Return account inputs

705ba3a

Fix error message for witness not found

a9c4d3a

sergerad requested a review from bobbinth January 21, 2026 03:21

sergerad added 8 commits January 22, 2026 06:29

Treat native/foreign the same

5873fb8

Pass ref block

89b9725

Explainer comment for slot name cache

2e066ed

RM account request

828ac4d

Doc comment

2763610

Use account header vault root

f068918

add register_storage_map_slots fn

7bd5258

Misc cleanup

a8781ea

sergerad requested a review from bobbinth January 21, 2026 21:29

bobbinth reviewed Jan 22, 2026

View reviewed changes

sergerad added 6 commits January 22, 2026 15:32

Lock out of loop

a6d5b97

Rename reference header

0cbf820

Fix details request

f7eacbe

Expand get_account_inputs comment

9448396

Reprhase build_minimal_foreign_account fn

75a17f0

RM unused variants

c9c658b

sergerad requested a review from bobbinth January 22, 2026 03:27

bobbinth approved these changes Jan 22, 2026

View reviewed changes

Mirko-von-Leipzig approved these changes Jan 22, 2026

View reviewed changes

bobbinth mentioned this pull request Jan 22, 2026

fix(validator): load foreign code from inputs #1570

Merged

Mirko-von-Leipzig mentioned this pull request Jan 22, 2026

Loading code in the validator #1571

Closed

bobbinth requested a review from igamigo January 22, 2026 16:24

bobbinth reviewed Jan 22, 2026

View reviewed changes

chore: update comment

e133005

bobbinth reviewed Jan 22, 2026

View reviewed changes

chore: fix lints

fd3c646

bobbinth merged commit b7a458c into next Jan 22, 2026
7 checks passed

bobbinth deleted the sergerad-ntx-builder-fpi branch January 22, 2026 18:44

igamigo reviewed Jan 22, 2026

View reviewed changes

SantiagoPittella mentioned this pull request Jan 22, 2026

DataStore error while re-executing network transactions #1581

Closed

Conversation

sergerad commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes

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!

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!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Mirko-von-Leipzig 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!

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!

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!

4 participants

sergerad commented Jan 16, 2026 •

edited

Loading