Skip to content

feat: Load assets lazily#1848

Merged
PhilippGackstatter merged 31 commits intonextfrom
pgackst-lazy-asset-loading
Sep 8, 2025
Merged

feat: Load assets lazily#1848
PhilippGackstatter merged 31 commits intonextfrom
pgackst-lazy-asset-loading

Conversation

@PhilippGackstatter
Copy link
Copy Markdown
Contributor

@PhilippGackstatter PhilippGackstatter commented Sep 3, 2025

Load assets lazily. Part of #401.

Changes

  • Add {AssetVault, PartialVault}::open.
  • Introduces AssetWitness as a type returned by AssetVault::open and for use in the DataStore interface.
    • This results in a type safe API and makes the API more consistent with AccountTree and NullifierTree.
  • Changes TransactionExecutorHost to require a STORE: DataStore rather than STORE: MastForestStore. This is needed so the host can request data from the store.
  • React to before add/remove asset events by checking if the merkle store already contains a merkle path for this asset and:
    • if so, continue
    • if not, request it from the data store.
  • Tracks the native_account_header in the base host to be able to access the initial vault root of the account. With lazy storage map loading we'd also need the account storage header for similar reasons.
  • Adds TransactionContextBuilder::enable_partial_loading to selectively "enable" lazy loading for testing purposes. Enabling it immediately would break 50+ tests and not all of them are easily fixable (see below).
  • Removes the advice_inputs_from_transaction_witness_are_sufficient_to_reexecute_transaction test, which has frequently been difficult to maintain. Here also, due to the host requiring a DataStore rather than just a MastForestStore impl it was difficult to keep up to date. I think what this test asserts is essentially implicitly covered by the fact that we can prove, which doesn't have access to a data store. It would definitely be nice to keep the test, but it is difficult to maintain.
  • Add mock::util library for creating notes more easily. This can be extended with many more test functionality in the future, such as exec-utable wrappers around mock account APIs, which is currently annoying to use, due to the call convention.

Note that the scope of this PR is asset witness for the native account. For now I'm assuming we only request native account witnesses in the code, but this would eventually be changed to handle foreign accounts as well.

Follow-Ups:

  • Input Vault currently requests missing assets one by one, causing many individual data store lookups. These should be requested before the transaction and in bulk. We could probably do it in this PR, but I just left it like that for now as other things were more important to get done first.
  • Move TransactionKernelError to miden-tx. It's defined in miden-lib but really should be in miden-tx (see DataStoreError issue in the code).
  • PartialVault::add should take AssetWitness instead. This is a bit too much to do to include in this PR so can be a follow-up.
  • We should consider whether we still need to expose AssetVault::asset_tree. Instead, I think we should replace its usages with AssetVault::open or add missing, type-safer alternatives to AssetVault directly. It's still fine to be able to access the underlying SMT, but generally, there should be a better API for this. Again, makes it type safer and more in line with AccountTree and NullifierTree.
  • Is there a way to convert a Word into the Felt that represents an SMT's LeafIndex? If not, should there be something like that in miden-crypto? It'd be much better than having vault_key[3] in places which is basically a magic constant. Basically, making Smt::key_to_leaf_index publicly available would be great.
  • To use TransactionContext::execute_code with lazy loading, we'd have to make MockHost also request missing witnesses from a store. It currently doesn't have a store at all. I'm not sure the mock host makes sense any longer if we have to add more logic to it that duplicates the logic in the tx host. I think we should check if we can replace MockHost with TransactionExecutorHost for CodeExecutor. Otherwise we'll have to duplicate the lazy loading things.
  • Should we have a convenience method on SmtProof for this?
let smt_proof = SmtProof::from(asset_witness);
let authenticated_nodes = smt_proof
.path()
.authenticated_nodes(smt_proof.leaf().index().value(), smt_proof.leaf().hash())
.expect("leaf index is u64 and should be less than 2^SMT_DEPTH");

Base automatically changed from pgackst-transaction-inputs-partial-account to next September 3, 2025 21:05
@PhilippGackstatter PhilippGackstatter force-pushed the pgackst-lazy-asset-loading branch from 7e21770 to 79b11ce Compare September 4, 2025 05:58
Copy link
Copy Markdown
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you! I left just a few small comments inline - but I reviewed primarily non-test code and so would be good to have another set of eyes look at it (I tagged @igamigo for review).

Also, I pretty much agree with all the follow-ups. One comment:

  • Should we have a convenience method on SmtProof for this?
let smt_proof = SmtProof::from(asset_witness);
let authenticated_nodes = smt_proof
.path()
.authenticated_nodes(smt_proof.leaf().index().value(), smt_proof.leaf().hash())
.expect("leaf index is u64 and should be less than 2^SMT_DEPTH");

Could we put this as a convenience method on the AssetWitness for now? And maybe in the longer term we can consider moving it to miden-crypto.

@PhilippGackstatter
Copy link
Copy Markdown
Contributor Author

I discovered a few small problems while working on the follow-up PR to enable lazy loading for storage maps, so I fixed them here, but no major changes.

@bobbinth
Copy link
Copy Markdown
Contributor

bobbinth commented Sep 6, 2025

I discovered a few small problems while working on the follow-up PR to enable lazy loading for storage maps, so I fixed them here, but no major changes.

Looks good! Thank you for making the updates here. I think this is basically ready to go - so, once @igamigo had a chance to take a look, we can merge.

Copy link
Copy Markdown
Collaborator

@igamigo igamigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some minor comments, but overall looks great!

return Ok(Vec::new());
}

let vault_root = self.base_host.native_account_header().vault_root();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I fully understand why vault_root is passed to the function and then ignored. The explanation about always fetching data for the initial vault root makes sense, but in what scenarios would this change?

In the meantime, should there be an assert_eq!() for checking that the passed in root is the same as this one or does this not make sense?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good question! Basically, this code is somewhat temporary and will change when we implement lazy loading for foreign accounts. Then we have the situation that the vault root passed to this function is the current vault root of the current account:

  • for native accounts, the current vault root can change throughout transaction execution. The root that we need to pass to the data store is however always the initial one, which is why we take it from the base host's stored header.
  • for foreign accounts, which are read-only, the vault root does not change and so this is always the initial vault root, so we can pass exactly that vault root to the data store.

The alternative, and probably the one that's easier to understand, is that we could handle both native and foreign accounts in the same way: Store the account headers (+ storage headers for storage maps) in the host and fetch the initial vault root from there. That would be valid, but since we already have the initial vault root for foreign accounts, we can avoid storing this extra data and optimize a little bit.

The trade-off is that we wouldn't handle native and foreign accounts the same way but we would handle foreign accounts a bit more efficiently. This is something we can discuss more on the foreign account PR.

@PhilippGackstatter PhilippGackstatter merged commit 9ab59d1 into next Sep 8, 2025
18 checks passed
@PhilippGackstatter PhilippGackstatter deleted the pgackst-lazy-asset-loading branch September 8, 2025 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants