Skip to content
This repository has been archived by the owner on Feb 27, 2023. It is now read-only.

Use raw key as path instead of hash(key) #40

Open
adlerjohn opened this issue Jul 11, 2021 · 8 comments · May be fixed by #64
Open

Use raw key as path instead of hash(key) #40

adlerjohn opened this issue Jul 11, 2021 · 8 comments · May be fixed by #64

Comments

@adlerjohn
Copy link
Member

adlerjohn commented Jul 11, 2021

Note: this is a backwards-incompatible change in the interface, but shouldn't change the commitment so long as the library is used the same way.

Currently (as of #37), values are stored in a map hash(key) -> value. This allows constant-time reads. However, an outstanding change is using the raw key instead of the hash of the key: #14 (comment)

This is needed to allow applications to store certain entries at specific keys in the tree, which also allows for algorithms that operate over subtrees (see above-linked comment for more context). Users who do not want to use this functionality can simply hash their keys before passing them to the library, resulting in an identical tree as before the change proposed here.

@roysc
Copy link
Contributor

roysc commented Aug 26, 2021

Is there an ETA or general priority set for this?

@adlerjohn
Copy link
Member Author

I will start working on this at the end of September if no one else has picked it up before then.

@musalbas
Copy link
Member

musalbas commented Sep 10, 2022

Wouldn't a simpler way for an end-user to implement this than #64 would be to implement a treeHasher with a custom path() function that simply returns the same bytes as output?

func (th *treeHasher) path(key []byte) []byte {

@adlerjohn
Copy link
Member Author

adlerjohn commented Sep 10, 2022

Hm, that's a good suggestion. Off the top of my head I don't see why that wouldn't work. If there are no objections from @sweexordious who worked on this most recently, we can instead just document this property and close the issue.

@rach-id
Copy link
Member

rach-id commented Sep 11, 2022

That's a good idea. The only thing I am thinking about is the key size, the library expects it to be constant, if I remember correctly. Thus, additional key size checks will need to be added.

@adlerjohn
Copy link
Member Author

Those checks should probably be required orthogonally to the issue at hand.

@rach-id
Copy link
Member

rach-id commented Sep 11, 2022

Sounds good 👍.

If I remember correctly, my PR was only doing the following:

  • Adding constant key size checks
  • Ability to track the key size not to panic

If these are orthogonal, or can be added as preconditions, then the PR would be way smaller.

@universe2439
Copy link

Agreed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Status: TODO
Development

Successfully merging a pull request may close this issue.

5 participants