Thinking more about this, there are some better solutions:

• Using the sequential hash would be optimal, but we somehow need to get that hash into account_delta::add_storage_map_entries_to_delta, which would have to be either provided as part of the prologue or pushed onto the advice stack by an event. This would require an extra field to track this in the host, which feels a bit unclean.
• Alternatively, we could also use the hash of the root and do the same in add_storage_map_entries_to_delta. This would probably be the simplest approach.
• We could also have account_delta::add_storage_map_entries_to_delta emit an event and the host pushes the data to the advice stack in response. The host would need to be changed to contain PartialAccount instead of AccountHeader, which means we need to clone the partial account whenever we construct a tx host.

I think I will change it to the second one, as that seems like the minimal fix, unless anyone has other preferences.

Is there something fundamentally wrong with the current approach?

I'm not sure if overwriting the SMT's root in the advice provider is fine or not. I don't know if the smt module sets it somewhere. This is a general problem with the advice map. You don't really know how others are using it. That's why we have this soft rule of "data in the advice map should have its hash as the key", but it's a bit unclean here.

but as we're using roots as keys, which result from cryptographic hashes, we shouldn't have to worry about collisions, right?

so if that's the only concern, I think the current approach works well 👍🏼

I think option 2 is not that much different from the current approach - so, I'd keep the code as is.

But I do agree that not relying on STORAGE_MAP_ROOT |-> [MAP ENTRIES] entry in the advice map would be a bit better, and maybe the 3rd option is the right way to do it. Let's create an issue to potentially come back to this in the future.

I've looked into the call sites of validate() a bit, and aside from tests (and deserialization*) it seems to be used only in ProvenTransacationBuilder::build().
And build(), in turn, is only meaningfully used within LocalTransactionProver::build_proven_transaction, where we have the following logic:

// Compute the commitment of the pre-fee delta, which goes into the proven transaction,
// since it is the output of the transaction and so is needed for proof verification.
let pre_fee_delta_commitment: Word = pre_fee_account_delta.to_commitment();

let builder = ProvenTransactionBuilder::new(
    ...,
    pre_fee_delta_commitment,
    ...
)...;

// The full transaction delta is the pre fee delta with the fee asset removed.
let mut post_fee_account_delta = pre_fee_account_delta;
post_fee_account_delta
    .vault_mut()
    .remove_asset(Asset::from(tx_outputs.fee))
    .map_err(TransactionProverError::RemoveFeeAssetFromDelta)?;

So it seems that fee manipulation within validate basically replicates that same logic.

*I haven't thought too much whether this should be done for deserialization, i.e. in what scenarios this would happen.

I think option 2 is not that much different from the current approach - so, I'd keep the code as is.

But I do agree that not relying on STORAGE_MAP_ROOT |-> [MAP ENTRIES] entry in the advice map would be a bit better, and maybe the 3rd option is the right way to do it. Let's create an issue to potentially come back to this in the future.

Thinking more about this, I wonder if this approach is completely secure. IIUC, here we check that the set of entries provided via the advice map is present in the the storage map with the specified root. However, we do not check that this set of entries is contains all entries in the storage map.

For example, let's say a storage map has 2 entries: [KEY1 |-> VAL1, KEY2 |-> VAL2] but the advice map contains an entry MAP_ROOT |-> [VAL2, KEY2]. As far as I can tell, this procedure will still pass and build the delta as if the map only contained KEY2 |-> VAL2 entry.

I'm not sure of the exact way to exploit this - but it feels a bit dangerous. Maybe the approach could be:

1. Start with an empty storage map
2. Read key value pairs from the advice map (as we do now)
3. Insert these key-value pairs into the map.

Essentially, for new accounts, we'd be running a kind of a "deploy" procedure where we'd start with an empty account, and then insert the values into the storage (this should naturally update the delta, I think). Then, we'd check that the resulting commitment matches the initial account commitment, and only then we'd call mem_copy_initial_storage_slots.

Thanks for pointing that out, I agree that's a problem with the current approach.

I changed it to roughly how you described it, with a minor difference. The initial slots are still copied before all entries are inserted, because while iterating the storage, we need to know what the type of each slot is, and so that must be done on initialized memory.
I'm also not sure if you meant comparing the storage commitment against the initial one or each storage map's root against the committed one, but I went with the latter. Both should give us the same validation, but the per-map assertion can help more with debugging (e.g. to figure out which one of multiple maps are incorrect), but that's a minor thing. It also means we don't have to recompute the storage commitment.