Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions fuel-merkle/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,16 @@ repository = { workspace = true }
description = "Fuel Merkle tree libraries."

[dependencies]
anyhow = { version = "1.0", default-features = false }
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

See

fn write_node_batch(&self, node_batch: &JmtNodeBatch) -> anyhow::Result<()> {
for example

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

derive_more = { version = "0.99", default-features = false, features = ["display"] }
digest = { version = "0.10", default-features = false }
fuel-storage = { workspace = true, default-features = false }
hashbrown = "0.13"
hex = { version = "0.4", default-features = false, features = ["alloc"] }
jmt = { version = "0.11.0" }
serde = { version = "1.0", default-features = false, optional = true }
sha2 = { version = "0.10", default-features = false }
spin = { version = "0.9.8" }

[dev-dependencies]
criterion = { workspace = true }
Expand Down
6 changes: 6 additions & 0 deletions fuel-merkle/src/jellyfish.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/// Integration of jmt::JellyfishMerkleTree with the Storage traits.
pub mod jmt_integration;

// Re-export dependencies from the jmt crate necessary for defining implementations
// of the Mappable trait required by the JellyfishMerkleTree integration.
pub use jmt;
245 changes: 245 additions & 0 deletions fuel-merkle/src/jellyfish/jmt_integration.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,245 @@
use core::marker::PhantomData;

use crate::storage::{
Mappable,
StorageInspect,
StorageMutate,
};

use alloc::{
sync::Arc,
vec::Vec,
};

use jmt::storage::{
HasPreimage,
LeafNode as JmtLeafNode,
Node as JmtNode,
NodeBatch as JmtNodeBatch,
NodeKey as JmtNodeKey,
TreeReader,
TreeWriter,
};
use spin::{
RwLock,
RwLockReadGuard,
RwLockWriteGuard,
};

#[derive(Debug, Clone)]
pub struct JellyfishMerkleTreeStorage<
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

NodeTableType,
ValueTableType,
LatestRootVersionTableType,
StorageType,
StorageError,
> {
inner: Arc<RwLock<StorageType>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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>,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

phantom_table: PhantomData<(
NodeTableType,
ValueTableType,
LatestRootVersionTableType,
StorageError,
)>,
}

impl<
NodeTableType,
ValueTableType,
LatestRootVersionTableType,
StorageType,
StorageError,
>
JellyfishMerkleTreeStorage<
NodeTableType,
ValueTableType,
LatestRootVersionTableType,
StorageType,
StorageError,
>
{
pub fn storage_read(&self) -> RwLockReadGuard<StorageType> {
self.inner.read()
}

pub fn storage_write(&self) -> RwLockWriteGuard<StorageType> {
self.inner.write()
}
}

impl<
NodeTableType,
ValueTableType,
LatestRootVersionTableType,
StorageType,
StorageError,
> TreeWriter
for JellyfishMerkleTreeStorage<
NodeTableType,
ValueTableType,
LatestRootVersionTableType,
StorageType,
StorageError,
>
where
NodeTableType: Mappable<Key = JmtNodeKey, Value = JmtNode, OwnedValue = JmtNode>,
ValueTableType: Mappable<
Key = jmt::KeyHash,
Value = (jmt::Version, jmt::OwnedValue),
OwnedValue = (jmt::Version, jmt::OwnedValue),
>,
LatestRootVersionTableType: Mappable<Key = (), Value = u64, OwnedValue = u64>,
StorageType: StorageMutate<NodeTableType, Error = StorageError>
+ StorageMutate<ValueTableType, Error = StorageError>
+ StorageMutate<LatestRootVersionTableType, Error = StorageError>,
{
fn write_node_batch(&self, node_batch: &JmtNodeBatch) -> anyhow::Result<()> {
for (key, node) in node_batch.nodes() {
// TODO: Do we really need locks here?
Copy link
Contributor

Choose a reason for hiding this comment

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

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

let mut storage = self.storage_write();
<StorageType as StorageMutate<NodeTableType>>::insert(
&mut *storage,
key,
node,
)
.map_err(|_err| anyhow::anyhow!("Node table write Storage Error"))?;
if key.nibble_path().is_empty() {
// If the nibble path is empty, we are updating the root node.
// We must also update the latest root version
let newer_version = <StorageType as StorageInspect<
LatestRootVersionTableType,
>>::get(&*storage, &())
.map_err(|_e| anyhow::anyhow!("Latest root version read storage error"))?
.map(|v| *v)
.filter(|v| *v >= key.version());
// To check: it should never be the case that this check fails
if newer_version.is_none() {
StorageMutate::<LatestRootVersionTableType>::insert(
&mut *storage,
&(),
&key.version(),
)
.map_err(|_e| {
anyhow::anyhow!("Latest root version write storage error")
})?;
}
}

for ((version, key_hash), value) in node_batch.values() {
match value {
None => {
let _old = <StorageType as StorageMutate<ValueTableType>>::take(
&mut *storage,
key_hash,
)
.map_err(|_e| anyhow::anyhow!("Version Storage Error"))?;
}
Some(value) => {
let _old =
<StorageType as StorageMutate<ValueTableType>>::replace(
&mut *storage,
key_hash,
&(*version, value.clone()),
)
.map_err(|_e| anyhow::anyhow!("Version Storage Error"))?;
}
}
}
}

Ok(())
}
}

impl<
NodeTableType,
ValueTableType,
LatestRootVersionTableType,
StorageType,
StorageError,
> TreeReader
for JellyfishMerkleTreeStorage<
NodeTableType,
ValueTableType,
LatestRootVersionTableType,
StorageType,
StorageError,
>
where
NodeTableType: Mappable<Key = JmtNodeKey, Value = JmtNode, OwnedValue = JmtNode>,
ValueTableType: Mappable<
Key = jmt::KeyHash,
Value = (jmt::Version, jmt::OwnedValue),
OwnedValue = (jmt::Version, jmt::OwnedValue),
>,
StorageType: StorageInspect<NodeTableType, Error = StorageError>
+ StorageInspect<ValueTableType, Error = StorageError>,
{
fn get_node_option(&self, node_key: &JmtNodeKey) -> anyhow::Result<Option<JmtNode>> {
let storage = self.storage_read();
let get_result =
<StorageType as StorageInspect<NodeTableType>>::get(&*storage, node_key)
.map_err(|_e| anyhow::anyhow!("Storage Error"))?;
let node = get_result.map(|node| node.into_owned());

Ok(node)
}

fn get_value_option(
&self,
max_version: jmt::Version,
key_hash: jmt::KeyHash,
) -> anyhow::Result<Option<jmt::OwnedValue>> {
let storage = self.storage_read();
let Some(value) =
<StorageType as StorageInspect<ValueTableType>>::get(&*storage, &key_hash)
.map_err(|_e| anyhow::anyhow!("Version Storage Error"))?
Copy link
Member

Choose a reason for hiding this comment

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

perhaps

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

.filter(|v| v.0 <= max_version)
Copy link
Member

Choose a reason for hiding this comment

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

is there any way we can optimize this?

.map(|v| v.into_owned().1)
else {
return Ok(None)
};
// Retrieve current version of key

return Ok(Some(value))
}

fn get_rightmost_leaf(&self) -> anyhow::Result<Option<(JmtNodeKey, JmtLeafNode)>> {
unimplemented!(
"Righmost leaf is used only when restoring the tree, which we do not support"
)
}
}

impl<
NodeTableType,
ValueTableType,
LatestRootVersionTableType,
StorageType,
StorageError,
> HasPreimage
for JellyfishMerkleTreeStorage<
NodeTableType,
ValueTableType,
LatestRootVersionTableType,
StorageType,
StorageError,
>
where
ValueTableType: Mappable<
Key = jmt::KeyHash,
Value = (jmt::Version, jmt::OwnedValue),
OwnedValue = (jmt::Version, jmt::OwnedValue),
>,
StorageType: StorageInspect<ValueTableType, Error = StorageError>,
{
fn preimage(&self, key_hash: jmt::KeyHash) -> anyhow::Result<Option<Vec<u8>>> {
let storage = self.storage_read();
let preimage =
<StorageType as StorageInspect<ValueTableType>>::get(&*storage, &key_hash)
.map_err(|_e| anyhow::anyhow!("Preimage storage error"))?
.map(|v| v.into_owned().1);

Ok(preimage)
}
}
1 change: 1 addition & 0 deletions fuel-merkle/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ extern crate alloc;

pub mod binary;
pub mod common;
pub mod jellyfish;
pub mod sparse;
pub mod storage;

Expand Down
Loading