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

Trie debug tooling #6

Merged
merged 20 commits into from
Feb 5, 2024
Merged

Trie debug tooling #6

merged 20 commits into from
Feb 5, 2024

Conversation

BGluth
Copy link
Collaborator

@BGluth BGluth commented Feb 2, 2024

I think it's probably a good idea to get this tooling back into main as the LoC count is starting to get pretty high.

There are two tools in this PR:

  • Trie diffing --> Attempts to find divergences between two tries.
  • Trie path querying --> Gives detailed information on the nodes hit when searching for a key in a trie.

I've only finished 1/2 of what I originally was planning for the diff tooling (I haven't implemented bottom-up diffs), but top-down diffs were enough for me to figure out the issue that I built it for, so I'll come back and finish bottom-up later on. The query logic I think is done. The way this information is printed out (the Display impls) could be a bit better, but this is something that I'll clean up in a separate PR down the road.

Both of these tools are placed behind the new feature flag trie_debug.

Copy link
Contributor

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

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

Mostly nitpicky comments, some are purely for my (and possibly others) understanding when diving into the codebase. I'll probably do another pass later.

src/nibbles.rs Outdated Show resolved Hide resolved
src/debug_tools/query.rs Outdated Show resolved Hide resolved
src/debug_tools/query.rs Show resolved Hide resolved
src/debug_tools/query.rs Show resolved Hide resolved
src/debug_tools/query.rs Outdated Show resolved Hide resolved

// It might seem a bit weird to say a branch has no key piece, but this function
// is used to detect two nodes of the same type that have different keys.
pub(super) fn get_key_piece_from_node_no_branch_key<T: PartialTrie>(n: &Node<T>) -> Nibbles {
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming is a bit hard to decrypt, without reading the comment (which won't show up in rustdoc). Though I don't have a better suggestion in mind.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll come up with something better...

Copy link
Collaborator Author

@BGluth BGluth Feb 2, 2024

Choose a reason for hiding this comment

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

Potentially a bit better?

/// Get the key piece from the given node if applicable.
///
/// Note that there is no specific [`Nibble`] associated with a branch like
/// there is with [Extension][`Node::Extension`] and [Leaf][`Node::Leaf`] nodes,
/// and the only way to get the nibble "associated" with branches is to look at
/// the next Nibble in the current key as we traverse down it.
pub(super) fn get_key_piece_from_node_pulling_from_key_for_branches<T: PartialTrie>(
    n: &Node<T>,
    curr_key: &Nibbles,
) -> Nibbles {
    ...
}

/// Get the key piece from the given node if applicable. Note that
/// [branch][`Node::Branch`]s have no [`Nibble`] directly associated with them.
pub(super) fn get_key_piece_from_node<T: PartialTrie>(n: &Node<T>) -> Nibbles {
    ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

So yes, it's easier to understand, but now I think we've hit another issue 😆 I think we usually should try to not have function names with more than 5/6 words max. But let's go with it, we can always do a pass on renaming long functions into clearer names later on.

src/debug_tools/diff.rs Outdated Show resolved Hide resolved
src/debug_tools/diff.rs Outdated Show resolved Hide resolved
src/debug_tools/diff.rs Outdated Show resolved Hide resolved
src/debug_tools/diff.rs Outdated Show resolved Hide resolved
Robin's suggested changes for #6

Co-authored-by: Robin Salen <[email protected]>
@BGluth BGluth requested a review from Nashtare February 2, 2024 22:55
Copy link
Contributor

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

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

Thanks! Overall looks good to me, we may do some light cleanup / refactoring later on, but that's already pretty cool to get this into main 😃

@BGluth BGluth merged commit ce863e5 into main Feb 5, 2024
2 checks passed
@BGluth BGluth deleted the debug_tools branch February 5, 2024 22:28
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.

2 participants