[feature/jmt 1/4] Jmt: Integration of Storage traits with crate jmt

[acerone85]

In order to integrate Jellyfish Merkle Trees from the jmt crate, we must implement three traits from that crate:

• TreeReader - used to read and cache updates to the tree in memory
• TreeWriter - used to flush updates from the tree cache into the underlying storage backend
• HasPreimage - used for proof generation and verification.

This PR connects the storage traits on the fuel-vm side with jmt crate as follows:

• It defines a data structure parametric in several types that require an implementation of Mappable each,
• It implements the three storage traits above for mappable

Tests are provided in the last PR of the feature branch.

[Short description of the changes.]

Checklist

• Breaking changes are clearly marked as such in the PR description and changelog
• New behavior is reflected in tests
• If performance characteristic of an instruction change, update gas costs as well or make a follow-up PR for that
• The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

• I have reviewed the code myself
• I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

• Rust SDK
• Sway compiler
• Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
• Someone else?

[rymnc]

perhaps

Suggested change

	.map_err(|_e| anyhow::anyhow!("Version Storage Error"))?
	.map_err(|e| anyhow::anyhow!("could not get value: {}", e.err()))?

[rymnc]

is there any way we can optimize this?

[netrome]

Some initial questions

[netrome]

While benchmarking I think this is fine, but I'd advocate for using dedicated error types before merging to master (or as a follow-up)

[acerone85]

this is the jmt crate that exposes traits whose signature return a anyhow::Result value.

See https://github.com/FuelLabs/fuel-vm/blob/68bbb4c52d4a749832cb07c11a9f467b54ec6b1f/fuel-merkle/src/jellyfish/jmt_integration.rs#L47 for example

Otherwise I'd be very happy to remove it :)

[netrome]

It feels weird to dictate the lock type over the Storage. Shouldn't we just use traits to dictate the storage behavior, and let any potential locking mechanism be an implementation detail of the particular storage implementation?

[acerone85]

agreed. In practice I don't think we'll need a Lock, and we can use RefCell instead.
On the other hand, it could be wise to have a trait for the "locking/borrowing" functions and parameterise over it. WIll amend

[netrome]

Do we need a cell even? Can't this just hold the StorageType directly the same way we do in the SMT struct:

#[derive(Debug)]
pub struct MerkleTree<TableType, StorageType> {
    root_node: Node,
    storage: StorageType,
    phantom_table: PhantomData<TableType>,
}

[acerone85]

I don't think we should, but again it's because of a constraint on the Jmt trait signatures.
Specifically TreeWriter::write_node_batch(&self, _: &NodeBatch) takes a non-mutable reference, but its implementation requires to call StorageInspect::inspect, which instead requires a mutable reference.

If we decide to fork jmt at some point, we can amend the trait signatures to avoid having to use cells or locks

[netrome]

Yeah commented above. I don't think we should have to care about locks in this context.

[netrome]

Perhaps it will be more obvious when I look at the following PRs, but why do we need to declare a specific storage type for the JMTs? I don't find anything directly equivalent in our SMT implementation.

[acerone85]

Maybe JellyFishMerkleTreeStorage is not the best name, but the Sparse merkle tree implementation follows the same pattern:

pub struct MerkleTree<TableType, StorageType> {
    root_node: Node,
    storage: StorageType,
    phantom_table: PhantomData<TableType>,
}

and at a later point StorageType is required to implement StorageInspect<TableType>, with TableType: Mappable<...> see

fuel-vm/fuel-merkle/src/sparse/merkle_tree.rs

Lines 193 to 196 in c329ea7

	impl<TableType, StorageType, StorageError> MerkleTree<TableType, StorageType>
	where
	TableType: Mappable<Key = Bytes32, Value = Primitive, OwnedValue = Primitive>,
	StorageType: StorageInspect<TableType, Error = StorageError>,

[netrome]

Right yeah, perhaps we should just omit Storage from the name then?