Conversation
7e21770 to
79b11ce
Compare
bobbinth
left a comment
There was a problem hiding this comment.
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
SmtProoffor 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.
|
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. |
igamigo
left a comment
There was a problem hiding this comment.
Left some minor comments, but overall looks great!
| return Ok(Vec::new()); | ||
| } | ||
|
|
||
| let vault_root = self.base_host.native_account_header().vault_root(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Load assets lazily. Part of #401.
Changes
{AssetVault, PartialVault}::open.AssetWitnessas a type returned byAssetVault::openand for use in theDataStoreinterface.AccountTreeandNullifierTree.TransactionExecutorHostto require aSTORE: DataStorerather thanSTORE: MastForestStore. This is needed so the host can request data from the store.native_account_headerin 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.TransactionContextBuilder::enable_partial_loadingto selectively "enable" lazy loading for testing purposes. Enabling it immediately would break 50+ tests and not all of them are easily fixable (see below).advice_inputs_from_transaction_witness_are_sufficient_to_reexecute_transactiontest, which has frequently been difficult to maintain. Here also, due to the host requiring aDataStorerather than just aMastForestStoreimpl 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.mock::utillibrary for creating notes more easily. This can be extended with many more test functionality in the future, such asexec-utable wrappers around mock account APIs, which is currently annoying to use, due to thecallconvention.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:
TransactionKernelErrorto miden-tx. It's defined inmiden-libbut really should be inmiden-tx(seeDataStoreErrorissue in the code).PartialVault::addshould takeAssetWitnessinstead. This is a bit too much to do to include in this PR so can be a follow-up.AssetVault::asset_tree. Instead, I think we should replace its usages withAssetVault::openor add missing, type-safer alternatives toAssetVaultdirectly. 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 withAccountTreeandNullifierTree.Wordinto theFeltthat represents an SMT'sLeafIndex? If not, should there be something like that inmiden-crypto? It'd be much better than havingvault_key[3]in places which is basically a magic constant. Basically, makingSmt::key_to_leaf_indexpublicly available would be great.TransactionContext::execute_codewith lazy loading, we'd have to makeMockHostalso 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 replaceMockHostwithTransactionExecutorHostforCodeExecutor. Otherwise we'll have to duplicate the lazy loading things.FastProcessormemory after execution miden-vm#2052, because we rely on accessingProcessmemory afterexecute_codein a few places.SmtProoffor this?