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

Replace justified unwraps in shardtree with expects #119

Merged
merged 4 commits into from
Dec 2, 2024

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Nov 23, 2024

Part of #118.

Rust 1.82 adds support for omitting empty types in pattern matching,
which would make these much clearer.
@@ -406,13 +406,16 @@ impl<

/// Adds a checkpoint at the rightmost leaf state of the tree.
pub fn checkpoint(&mut self, checkpoint_id: C) -> Result<bool, ShardTreeError<S::Error>> {
/// Pre-condition: `root_addr` must be the address of `root`.
Copy link
Contributor Author

@str4d str4d Nov 23, 2024

Choose a reason for hiding this comment

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

This pre-condition (and all similar others in this PR) is verifiably upheld:

  • We call this method initially with go(subtree.root_addr, &subtree.root); it is a programming error for these to be inconsistent.
  • We recurse by calling either go(r_addr, right) or go(l_addr, left) with values obtained from root_addr and root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is a programming error for these to be inconsistent.

I note that the public API LocatedTree::from_parts does not enforce that its arguments are consistent; we could potentially strengthen this by making that method fallible and checking that the actual encoded depth of the provided tree matches the provided root address (which would ensure that all parents within the provided tree correspond to a subtree node of the provided root address that can have children).

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've updated this PR to add the consistency check to LocatedTree::from_parts.

@str4d str4d force-pushed the shardtree-justify-unwraps branch from 33e7aa1 to 166872e Compare November 23, 2024 03:48
// the authentication path on the way back up.
/// Traverse down to the desired leaf position, and then construct
/// the authentication path on the way back up.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking nit:

Suggested change
//
///

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't think that /// comments were allowed on internal documentation? I suspect this whole comment will produce a linting or doc error.

Copy link
Contributor

Choose a reason for hiding this comment

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

I verified that cargo doc accepts these without complaint.

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

utACK modulo doc/comment linting c05f2fa

// the authentication path on the way back up.
/// Traverse down to the desired leaf position, and then construct
/// the authentication path on the way back up.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't think that /// comments were allowed on internal documentation? I suspect this whole comment will produce a linting or doc error.

@nuttycom nuttycom merged commit db4ad58 into main Dec 2, 2024
15 checks passed
@nuttycom nuttycom deleted the shardtree-justify-unwraps branch December 2, 2024 17:22
nuttycom added a commit that referenced this pull request Dec 9, 2024
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

Successfully merging this pull request may close these issues.

3 participants