Allow constructing partial structs from a known root

[igamigo]

Today a partial structure (map or vault) effectively needs at least one SMT proof to exist. In the miden-base tests that use lazy loading, partial structs are created by minimizing a full object and keeping a proof by opening the empty value, which preserves the SMT root. This makes it impossible to start from just the root when you do not have any proofs yet.

We should add APIs to instantiate PartialStorageMap and PartialVault from a known SMT root with no proofs loaded, for supporting workflows where we only know the roots at start time and fetch proofs lazily (although I think in the future we'd always want to add at least the fee token proof for the vault specifically).

[PhilippGackstatter]

This would be nice to have but depends on PartialSmt to support that. Since it builds on Smt, setting a root without any merkle paths present may break some assumptions, but we should indeed look into if we can make that happen.

[igamigo]

I thought that could be the case, but also thought that if this required a big refactor on Smt, as an alternative we could set the root in the partial structs separately (and validate it against added proofs)

[bobbinth]

Yeah, because PartialSmt uses Smt internally, it may be tricky to have it work without any proofs (though, I haven't spent too much time thinking about it - and so, there may be a way). So, for now, it may be simpler to handle this case separately in the partial structs.

But let's create an issue in miden-crypto to enable this functionality for PartialSmt in the future.

[PhilippGackstatter]

As part of this issue, we should update PartialVault::new_minimal and PartialStorageMap::new_minimal to use the new "from root" constructor.

[PhilippGackstatter]

In general, I think it would be preferable to handle this directly in PartialSmt. Let's say we change the PartialVault to the following:

pub struct PartialVault {
    root: Word,
    partial_smt: PartialSmt,
}

Then, after every mutating operation, we need to assert!(self.partial_smt.root(), self.root);. The PartialSmt could essentially do the same and then we'd have this handled in PartialVault and PartialStorageMap automatically. I left a comment in 0xMiden/crypto#560 (comment) that should allow for this.

[bobbinth]

The PartialSmt could essentially do the same and then we'd have this handled in PartialVault and PartialStorageMap automatically. I left a comment in 0xMiden/crypto#560 (comment) that should allow for this.

I agree with this, but my understanding is that this would be a breaking change in miden-crypto, and propagating it here my take some time (e.g., about 3 weeks). So, assuming it doesn't take too much effort, I think we can still update PartialVault and PartialStorageMap to work as if the Smt works as you described, and then, once we make the changes in miden-crypto and propagate them here, we could get rid of the extra machinery and just use the Smt interfaces directly.

[PhilippGackstatter]

It might actually be a good idea to introduce this for 0.12 still, because doing AssetVault::open(&Word::empty()) is not actually very safe, because the empty word is not a valid asset key. In other places we rely on the fact that the vault or the partial vault contain valid asset keys and assets, and so I can't say for sure that this doesn't trigger a panic somewhere somehow. I think it's probably fine, but again, not sure. So constructing a partial vault from a root is much safer.

Related: #1978 (comment)

[bobbinth]

It might actually be a good idea to introduce this for 0.12 still, because doing AssetVault::open(&Word::empty()) is not actually very safe, because the empty word is not a valid asset key. In other places we rely on the fact that the vault or the partial vault contain valid asset keys and assets, and so I can't say for sure that this doesn't trigger a panic somewhere somehow. I think it's probably fine, but again, not sure. So constructing a partial vault from a root is much safer.

Agreed. I've assigned 0xMiden/crypto#560 to you. There are a couple of other things I'd like to get merged in miden-crypto, and then we can make another release there and then it will be included in v0.19 release of the VM, which we'll upgrade miden-base v0.12.0 towards the end of of October.

[PhilippGackstatter]

As part of this, we can also try to make the partial struct constructors consistent as mentioned here (0xMiden/crypto#572 (comment)). This should only be a rename to PartialStorageMap::from_witnesses and maybe a small refactor of PartialVault::new.

Question for maybe @igamigo, in the client, is PartialVault::new essential to have in its current form or would having PartialVault::default + PartialVault::with_witnesses(witnesses: impl IntoIterator<Item = AssetWitness>) be sufficient (in addition to the new "from root" constructors)?

[igamigo]

Right now I don't think we need PartialVault::new in its current form. If any node endpoint ends up compressing responses into SMTs, maybe it'd be useful but right now there are no hard requirements.