-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
docs: Update ADR-040 to store hash(value) in SMT leaf #9680
Merged
mergify
merged 8 commits into
cosmos:master
from
vulcanize:roysc/adr-040-update-smt-leaf
Sep 15, 2021
Merged
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
ab7ee3e
Update ADR-40 to store hash(value) in SMT leaf
roysc a805516
Improve wording
roysc f071c61
Specify SMT structure
roysc bf2a783
"celestia"
roysc 265d0f6
nits
roysc 537f1bd
correct wording
roysc 5901119
Merge branch 'master' into roysc/adr-040-update-smt-leaf
roysc 0973b86
Merge branch 'master' into roysc/adr-040-update-smt-leaf
roysc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed - in the leaf we need to commit to the key as well (it's not enough that it is in a path).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hash(key)/path
is stored in the leaf node, even if not as part of the "leaf value" (i.e. the value returned from callingGet
on the SMT)leaf node ==
prefix || path || leaf_value
==prefix || hash(key) || hash(value_provided_to_the_SMT)
When calling
Set(key, value_provided_to_the_SMT)
it hashes thekey
into thepath
and thevalue_provided_to_the_SMT
into theleaf_value
. When callingGet(key)
it returnsvalue_provided_to_the_SMT
.Note that
value_provided_to_the_SMT
currently ishash(key || value_in_the_StateStore)
so that when we callGet
we retrievehash(key || value_in_the_StateStore)
which is the key we need for the (current) inverse index.So the SMT leaf node is, in current practice,
prefix || hash(key) || hash(hash(key || value_in_the_StateStore))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so we can add
prefix +
to my suggestion. We can use||
operator instead of+
if you prefer.Why is that? We should provide
key
andobj_value
without modifying it. SMT will do all necessary operations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was just a misunderstanding of the ADR language, I think. We didn't think it was describing the SMT's internal structure, since the hashed values are not exposed by the SMT interface (so we assumed the hashed value should be passed in). But we can add methods for that and fork the code if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like Roy said it is because the current implementation does not expose the hashed values.
We did it like this so that when we would
Get
from the SMT we retrieved the value we needed for the old inverse indexhash(key || value)
.Set
takes a key and value,Get
only returns the value provided toSet
not some internal transformation of key and/or value- this would be really odd behavior for a Setter and Getter interface.Set
was the unhashed "value" (key || obj_value
), then when we wouldGet
from the SMT we would get that unhashed value (again, not some internal- hashed- transformation of the value we provided) and we would have to hash it again at the level above the SMT before we could use it in the inverse index. This would have worked but would mean we were duplicating hashing efforts at the two levels.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are on the same page. In my suggestion I added
hash(key)
to the leaf (I'm using+
operator rather than||
). It seams we need to update it to (as you noted in the comment above):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping, @roysc , @i-norden - let's update the paragraph to what's in the SMT leaf and merge this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated my suggestion based on the SMT spec: https://github.com/celestiaorg/celestia-specs/blob/ec98170398dfc6394423ee79b00b71038879e211/src/specs/data_structures.md#sparse-merkle-tree and John response.