FuelLabs / fuel-merkle

Fuel Merkle trees in Rust.
Apache License 2.0
7 stars 5 forks source link

feat: Load BMT from MMR peaks #123

Closed bvrooman closed 1 year ago

bvrooman commented 1 year ago

Related PRs:

bvrooman commented 1 year ago

Thank you for the thorough review @freesig!

In general, this crate was not designed with arithmetic safety as a high priority. Specifically, there are instances of additions (increments) and multiplication that may overflow when we reach the extreme upper bounds of the u64 space. We use u64 in the following (likely not exhaustive):

Leaves count is simply an incremental counter of the number of leaves in a tree. It starts at 0 and increments with each push.

The proof index is a value that must lie between [0, leaves_count) for a BMT.

Positions store the in-order index of a tree node. The highest in-order index will be (leaves_count - 1) * 2. This means we require an index space of 2^(N + 1) where N is the log of the leaves_count. Therefore, we may limit the leaves count to 2^63 in order to support the index space.

We should consider the risk this poses in the context of real use. Do we suspect it is possible to reach the upper limits of the u64 space?

If we want to review the arithmetic safety of this crate, we can do that in a separate, focused effort.

Voxelot commented 1 year ago

We could do a similar effort to the VM where we use the new clippy lint arithmetic_side_effects to protect against unexpected arithmetic behaviors. Typically the client will manage the interactions with the merkle tree so the risks should be low, but if we ever allow user provided input to select parameters like a "tree version" we'd want to make sure there are no uncaught edge cases.

freesig commented 1 year ago

If we want to review the arithmetic safety of this crate, we can do that in a separate, focused effort.

Yep that sounds like a good approach.

bvrooman commented 1 year ago

I'm creating a new issue to address overall arithmetic safety: https://github.com/FuelLabs/fuel-merkle/issues/124.