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

Expose leaf node data #3

Closed
wants to merge 2 commits into from
Closed

Conversation

roysc
Copy link
Collaborator

@roysc roysc commented Jul 20, 2021

Expose leaf node data with a GetLeaf() method.

This will allow us to avoid an external hashing step, and use the hashed key (path) and value directly from within a Cosmos KV store.

@i-norden
Copy link
Collaborator

@roysc can you rebase vulcanize:master, so that this PR is a bit cleaner.

@i-norden
Copy link
Collaborator

Nvm I rebased vulcanize:master, can you rebase roysc:cosmos-get-leaf? Thanks!

Copy link
Collaborator

@i-norden i-norden left a comment

Choose a reason for hiding this comment

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

Now that I think about it, I think we should open this PR against something other than our master branch as we should keep that even with the upstream repo's master branch. When you get the chance rebase roysc:cosmos-get-leaf onto master, and then reopen this PR against https://github.com/vulcanize/smt/tree/main

@roysc roysc changed the base branch from master to main July 23, 2021 07:14
@i-norden i-norden self-requested a review July 27, 2021 00:16
Copy link
Collaborator

@i-norden i-norden left a comment

Choose a reason for hiding this comment

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

LGTM!


path := smt.th.path(key)
currentHash := smt.root
for i := 0; i < smt.depth(); i++ {
Copy link
Collaborator

@i-norden i-norden Jul 27, 2021

Choose a reason for hiding this comment

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

This is outside the scope of this PR butI wonder if it would be worthwhile to create a secondary index, internal to the SMT, that maps a key/path directly to the associated leaf node so that we don't have to iterate down a branch from the root- performing multiple db lookups- to get to the leaf (or discover it is nonexistent). Or rather than internal to the SMT this could be done similar to the state snapshot in Ethereum: ethereum/go-ethereum#20152, where it is handled at levels above the tree.

@roysc
Copy link
Collaborator Author

roysc commented Aug 13, 2021

As discussed here, this PR is probably useless due to the performance cost of accessing the tree and the fact this access pattern likely won't be needed. We should close this and add a way to iterate the SMT.

@roysc roysc closed this Aug 13, 2021
@roysc roysc deleted the cosmos-get-leaf branch April 15, 2022 16:52
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.

None yet

2 participants