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

Feat/237 mpt trie ext to branch collapse error #455

Merged
merged 3 commits into from
Aug 9, 2024

Conversation

BGluth
Copy link
Contributor

@BGluth BGluth commented Jul 30, 2024

Resolves #237.

Also slipped in a QoL method Nibbles::empty(), which maybe should have been a separate PR (sorry!). I felt that an explicit empty method was a bit more clear than calling Nibbles::default() whenever we wanted an empty Nibbles.

- While `Nibbles::default()` already does this, I think having an
  explicit method that makes an empty `Nibbles` is a bit cleaner, even
  if it's a bit redundant.
- Unsafe, as if the extension was pointing to a (hashed) leaf, then we must
  collapse the extension into the leaf.
- However, because the leaf is hashed, we can not tell that the hash
  node is from a leaf and also can not extract the leaf's key.
- It's the user's responsibility to ensure that this scenario never
  occurs, so we need to return an error when it does.
@github-actions github-actions bot added the crate: mpt_trie Anything related to the mpt_trie crate. label Jul 30, 2024
@BGluth BGluth added this to the Testing and Validation milestone Jul 30, 2024
@muursh muursh requested a review from 0xaatif August 5, 2024 18:09
Copy link
Contributor

@0xaatif 0xaatif left a comment

Choose a reason for hiding this comment

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

I find the "collapse into" phrasing a bit overloaded/confusing, but I think I understand

mpt_trie/src/nibbles.rs Outdated Show resolved Hide resolved
mpt_trie/src/debug_tools/diff.rs Outdated Show resolved Hide resolved
mpt_trie/src/nibbles.rs Outdated Show resolved Hide resolved
BGluth added a commit that referenced this pull request Aug 9, 2024
@BGluth BGluth requested review from 0xaatif and Nashtare August 9, 2024 17:20
@Nashtare
Copy link
Collaborator

Nashtare commented Aug 9, 2024

@BGluth I think you deleted the changes in commit 9b3e1b5? Now it's only displaying the Default impl.

@BGluth
Copy link
Contributor Author

BGluth commented Aug 9, 2024

Ugh, how did I do that? I meant too just remove the changes of the calls to default. I'll fix this.

BGluth added a commit that referenced this pull request Aug 9, 2024
@BGluth BGluth force-pushed the feat/237_mpt_trie_ext_to_branch_collapse_error branch from 9b3e1b5 to 1c47d09 Compare August 9, 2024 18:55
@BGluth
Copy link
Contributor Author

BGluth commented Aug 9, 2024

Should be good now. 👍

BGluth added a commit that referenced this pull request Aug 9, 2024
@BGluth BGluth force-pushed the feat/237_mpt_trie_ext_to_branch_collapse_error branch from 1c47d09 to 08543c2 Compare August 9, 2024 18:57
@BGluth BGluth force-pushed the feat/237_mpt_trie_ext_to_branch_collapse_error branch from 08543c2 to 610c00b Compare August 9, 2024 19:03
Copy link
Collaborator

@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!

@Nashtare Nashtare enabled auto-merge (squash) August 9, 2024 19:13
@Nashtare Nashtare merged commit c0e0351 into develop Aug 9, 2024
15 checks passed
@Nashtare Nashtare deleted the feat/237_mpt_trie_ext_to_branch_collapse_error branch August 9, 2024 19:16
atanmarko pushed a commit that referenced this pull request Aug 14, 2024
* Added `Nibbles::empty()`

- While `Nibbles::default()` already does this, I think having an
  explicit method that makes an empty `Nibbles` is a bit cleaner, even
  if it's a bit redundant.

* Now returns an error if we collapse an `E --> H`

- Unsafe, as if the extension was pointing to a (hashed) leaf, then we must
  collapse the extension into the leaf.
- However, because the leaf is hashed, we can not tell that the hash
  node is from a leaf and also can not extract the leaf's key.
- It's the user's responsibility to ensure that this scenario never
  occurs, so we need to return an error when it does.

* Requested PR changes for #455
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate: mpt_trie Anything related to the mpt_trie crate.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Create an error if we try to collapse E --> H
3 participants