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

Audit uses of .unwrap/.expect in shardtree #118

Open
daira opened this issue Nov 22, 2024 · 2 comments
Open

Audit uses of .unwrap/.expect in shardtree #118

daira opened this issue Nov 22, 2024 · 2 comments

Comments

@daira
Copy link
Contributor

daira commented Nov 22, 2024

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.

@str4d
Copy link
Contributor

str4d commented Nov 23, 2024

  • 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
Copy link
Contributor

str4d commented Nov 23, 2024

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 added a commit that referenced this issue Nov 23, 2024
A witness cannot exist for an empty tree.

Part of #118.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants