incrementalmerkletree icon indicating copy to clipboard operation
incrementalmerkletree copied to clipboard

Audit uses of `.unwrap`/`.expect` in shardtree

Open daira opened this issue 1 year ago • 2 comments

For the unwrap calls, i see that :

  • batch.rs has 1 non-test unwrap
  • legacy.rs has 1 non-test unwrap
  • lib.rs has 3 non-test unwrap
  • prunable.rs has 8 non-test unwrap calls
  • caching.rs has 15 non-test unwrap calls, that are related to the DB access which could be unstable (for ex. due disk space issues)

~~At least the unwraps for DB access seem like a bug to me.~~

daira avatar Nov 22 '24 20:11 daira

  • batch.rs has 1 non-test unwrap

This (along with one unwrap in prunable.rs) is due to BatchInsertionResult.max_insert_position being an Option<Position>, but I cannot find anywhere this gets set to None.

  • legacy.rs has 1 non-test unwrap

This should be justifiable because of the invariant that you cannot construct a witness for an empty tree. However, incrementalmerkletree::witness::IncrementalWitness does permit construction from an empty tree, so we can't currently rely on that invariant. We should fix incrementalmerkletree.

  • lib.rs has 3 non-test unwrap
  • caching.rs has 15 non-test unwrap calls, that are related to the DB access which could be unstable (for ex. due disk space issues)

At least the unwraps for DB access seem like a bug to me.

These are all justifiable.

  • prunable.rs has 8 non-test unwrap calls

All except two of these (excluding the one mentioned above) are justifiable. The other two are technically justifiable in the context they get used, but their public APIs don't enforce the necessary invariant, so should be fixed (with the unwrap moving out into the place these APIs get used where the invariant can be justified).

str4d avatar Nov 23 '24 00:11 str4d

This should be justifiable because of the invariant that you cannot construct a witness for an empty tree. However, incrementalmerkletree::witness::IncrementalWitness does permit construction from an empty tree, so we can't currently rely on that invariant. We should fix incrementalmerkletree.

Opened #120. (Due to @nuttycom changing how we do crate development in #116, we can't change shardtree until after all changes to incrementalmerkletree are made.)

str4d avatar Nov 23 '24 01:11 str4d