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

bugfix: TSMT failed to verify empty word for depth 64. #212

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

hackaugusto
Copy link
Contributor

When a prefix is pushed to the depth 64, the entry list includes only the values different than ZERO. This is required, since each block represents a 2^192 values.

The bug was in the proof membership code, that failed to handle the case of a key that was not in the list, because the depth is 64 and the value was not set.

When a prefix is pushed to the depth 64, the entry list includes only
the values different than ZERO. This is required, since each block
represents a 2^192 values.

The bug was in the proof membership code, that failed to handle the case
of a key that was not in the list, because the depth is 64 and the value
was not set.
@hackaugusto hackaugusto requested a review from bobbinth November 2, 2023 14:04
Copy link

sonarqubecloud bot commented Nov 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

// length
if self.is_value_empty()
&& get_common_prefix_tier_depth(key, &self.entries[0].0) < self.path.depth()
{
return false;
}
Copy link
Contributor Author

@hackaugusto hackaugusto Nov 2, 2023

Choose a reason for hiding this comment

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

I don't really understand why we have an entry [(key, EMPTY_WORD)] for empty leaves as an exception (meaning, even though the value is empty, there is an entry in the list). But I didn't want to change it, since that would be a large change.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need this entry to figure out which node the path is for (i.e., we use the key from the tuple to figure out the index. There may be a way to work around this (either by introducing a separate field for the index, or by some other means), but I haven't thought about the best way to do it.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Great catch! All looks good - thank you!

// length
if self.is_value_empty()
&& get_common_prefix_tier_depth(key, &self.entries[0].0) < self.path.depth()
{
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We need this entry to figure out which node the path is for (i.e., we use the key from the tuple to figure out the index. There may be a way to work around this (either by introducing a separate field for the index, or by some other means), but I haven't thought about the best way to do it.

@hackaugusto hackaugusto merged commit 801befa into next Dec 4, 2023
11 checks passed
@hackaugusto hackaugusto deleted the hacka-tsmt-bug-fix-empty-value-depth-64 branch December 4, 2023 16:00
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