adds support for raw key instead of hashed key #64

rach-id · 2021-11-28T11:31:00Z

Fixes #40.
It adds a keySize field to all SMT related structs and uses it to access values in smt.values map.

Maybe we should add more checks on the keySize so that the treeHasher, SparseMerkleProof, SparseMerkleTree etc, have all the same key size.

Or, we can keep the keySize only inside the treeHasher and no checks will be required:
Pros
Simpler codebase

Cons
keySize is more related to the trees themselves than the hasher...

Also, other checks would be needed when doing operations like Get(key) checking if the key has the correct size.

Update:
The keySize is now part of the MapStore and it is being passed around as parameter wherever needed.

rach-id · 2021-11-28T13:57:08Z

@adlerjohn Please take a look and let me know if this is what is needed so I continue fixing the other tests etc.
Currently, only the TestSparseMerkleTreeUpdateBasic is working

codecov-commenter · 2021-12-19T15:52:50Z

Codecov Report

Merging #64 (a0cc481) into master (5f13c0f) will increase coverage by 3.54%.
The diff coverage is 95.29%.

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
+ Coverage   85.62%   89.16%   +3.54%     
==========================================
  Files           6        6              
  Lines         466      480      +14     
==========================================
+ Hits          399      428      +29     
+ Misses         39       26      -13     
+ Partials       28       26       -2

Impacted Files	Coverage Δ
deepsubtree.go	`71.92% <90.90%> (+10.81%)`	⬆️
proofs.go	`98.01% <92.59%> (-1.99%)`	⬇️
smt.go	`85.90% <94.73%> (+5.11%)`	⬆️
mapstore.go	`94.11% <100.00%> (+5.22%)`	⬆️
treehasher.go	`100.00% <100.00%> (ø)`
utils.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e634fe...a0cc481. Read the comment docs.

adds key size to map store + adds tests for it

rach-id · 2022-01-04T12:46:22Z

Questions:

Should the ErrWrongKeySize be also a struct ? So the error message also contains the wrong key. Or no need for that ?
Should we update the fuzz test or we no longer need it ? since it will always be a constant size key... What about generating tons of constant sized strings ?

tzdybal

I left some comments in the code.

In general, I think that default behavior of the library should be preserved (enable users to use arbitrary keys), and raw keys support should be actually added (instead of replacing current way of operation).

This can be added as an Option, that changes the behavior of treeHasher.path method to no-op.

.gitignore

helpers_test.go

tzdybal · 2022-01-04T21:53:11Z

mapstore.go

 	_, ok := sm.m[string(key)]
 	if ok {
 		delete(sm.m, string(key))
 		return nil
 	}
 	return &InvalidKeyError{Key: key}
 }
+
+func (sm *SimpleMap) checkKeySize(key []byte) error {


For SparseMerkleTree.values this function implies that all keys in SMT must have the same length. This is fine if user is going to hash keys before adding to SMT, but in more general case it's a very strong limitation.

I think the SMT is by construction assumed to be perfect. Allowing different key sizes...doesn't really make sense? How would that even work?

I thought it should only support a fixed size of keys, no ?
The checkKeySize is not exported (If I am not wrong), it is only used internaly. What it does is verify the length of the provided key if it matches the key size or return an error otherwise. I put it in a function to avoid rewriting the same code over and over again.

Do you mean we can have any key length ? I guess that wasn't the case even before this PR, since it was using a specific hashing algorithm to hash the keys and the depth of the tree was gotten via hasher.size()...

I totally agree, that SMT require fixed key size internally. My only concern is that, previously our SMT public interface was a black-box/high-level/general-purpose-container type of thing (see README.md with _, _ = tree.Update([]byte("foo"), []byte("bar"))). This change is a complete pivot, where de-facto part of implementation is exposed to the user, and user has to be aware of this.

true, it is a breaking change. However, it will impact the interface only at the time of creation of the map stores and verification of proofs.
For addBranch, it was a mistake from my side to put the keySize as a parameter since it should be checked against the dsmst's map store and not against the one passed in params (will make the change shortly).

Previously, I think when verifying a proof, we were passing a hasher, from which we extracted the keySize. So, we were verifying a proof against a certain hasher and a keySize. Except that the latter wasn't exposed since it was extracted from the hasher. Now that the key size doesn't relate to the hasher, it's alright to pass it as parameter.

What do you think ?

The actual function signatures of Get, Set, Update, Delete, etc are not changed, but there is an extra "contract" added (fixed length keys). While updating SMT dependency, user has to potentially update all invocations of those methods.

Aaah, got you now 👍 makes sense

rach-id · 2022-01-04T22:44:13Z

I left some comments in the code.

In general, I think that default behavior of the library should be preserved (enable users to use arbitrary keys), and raw keys support should be actually added (instead of replacing current way of operation).

This can be added as an Option, that changes the behavior of treeHasher.path method to no-op.

Thanks for the comments, will address them tomorrow.
For the functionality, I think the issue states replacing that behaviour, it's a breaking change, I believe.
Additionally, I think that the tree hasher should no longer be passed to these structures, as it is only hashing everything internally... What can be done is using a default hasher that can be changed using options, for example, but no longer be necessary.
What do you think ?

.gitignore

adlerjohn · 2022-01-04T22:52:07Z

smt.go

 			// This is a non-membership proof that involves showing a different leaf.
 			// Add the leaf data to the proof.
 			nonMembershipLeafData = leafData
 		}
 	}

-	proof := SparseMerkleProof{
+	return SparseMerkleProof{


Is this change idiomatic Go? I prefer the old version personally since it's clearer what's being returned without having to parse two concepts at once. The compiler will optimize away the variable anyways, if performance was the concern.

I got this and similar ones flagged by a linter... Apparently, I am using a more exhaustive linter, maybe we should agree on a linter config for all the repos, and respect all the errors it is showing.

Hm. How about revert these changes for now, then we can open a separate issue (potentially also to be implemented across the org) on linting?

sounds good. Should I create an issue ? or just leave it for now ?

mapstore.go

tzdybal · 2022-01-05T12:01:26Z

Just for clarification: I totally agree, that raw keys can be very useful, I understand the logic behind this change. But IMHO it's a low hanging fruit, to be able to use SMT with both raw (advanced use, turned on by option) and hashed keys (by default).
This way, library can be more general-purpose, more developer/user friendly.

…apStore` instead

rach-id · 2022-01-05T12:26:13Z

@tzdybal sounds good to have both, yes. I can start digging into it as soon as I get the green light 👍

adlerjohn · 2022-01-05T14:08:30Z

Having the option for both modes isn't bad IMO. Having the option for raw keys is a must, beyond that extra features are gravy on top.

rach-id · 2022-01-05T14:42:51Z

@adlerjohn should I get going with the implementation ? in // with the explorer stuff ?
Which one should be the default behaviour ? raw keys or hashed keys ?

Also, what do you think about these ?

Questions:

Should the ErrWrongKeySize be also a struct ? So the error message also contains the wrong key. Or no need for that ?

Should we update the fuzz test or we no longer need it ? since it will always be a constant size key... What about generating tons of constant sized strings ?

I think that the tree hasher should no longer be passed to these structures, as it is only hashing everything internally... What can be done is using a default hasher that can be changed using options, for example, but no longer be necessary.

rach-id · 2022-01-05T17:08:13Z

Stopping work on this for now. Will pick it up later.

rach-id added 2 commits November 23, 2021 14:53

refactor: code deduplication

37c1633

refactor: code deduplication

e6faf80

rach-id marked this pull request as draft November 28, 2021 11:38

rach-id changed the title ~~WIP add support for raw key instead of hashed key~~ WIP adds support for raw key instead of hashed key Nov 28, 2021

adlerjohn assigned rach-id and adlerjohn Nov 29, 2021

adds key size to map store + adds tests for it

c88ac82

rach-id force-pushed the master branch from 3a62a7d to e6faf80 Compare January 4, 2022 11:26

rach-id added 4 commits January 4, 2022 12:26

Merge pull request #1 from SweeXordious/keySize_map_stores

092f129

adds key size to map store + adds tests for it

adds extra update in proofs key size check

315869a

cosmetics

19468c8

.gitignore

4529285

rach-id added 3 commits January 4, 2022 13:50

Merge branch 'celestiaorg:master' into master

c4225e4

revert unnecessary change

6e8bc4e

updates CHANGELOG-PENDING.md

d7836ba

rach-id marked this pull request as ready for review January 4, 2022 13:23

rach-id changed the title ~~WIP adds support for raw key instead of hashed key~~ adds support for raw key instead of hashed key Jan 4, 2022

liamsi requested review from adlerjohn and musalbas January 4, 2022 20:12

adlerjohn added the enhancement New feature or request label Jan 4, 2022

tzdybal reviewed Jan 4, 2022

View reviewed changes

adlerjohn reviewed Jan 4, 2022

View reviewed changes

rach-id added 3 commits January 5, 2022 10:27

adds go.sum and removes .gitignore

6537683

adds check on key size when creating a map store not to be < 0 + tests

1e162cb

stops seeding random everytime and rely on default Seed(1)

5926367

removes keySize from addBranch and checks against the one from `m…

9d8448d

…apStore` instead

reverts direct returns

a0cc481

rach-id marked this pull request as draft January 5, 2022 17:08

musalbas removed their request for review February 23, 2022 15:46

bvrooman mentioned this pull request Feb 28, 2022

Docs: Create Sparse Merkle Tree test spec FuelLabs/fuel-merkle#66

Merged

musalbas mentioned this pull request Sep 10, 2022

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds support for raw key instead of hashed key #64

adds support for raw key instead of hashed key #64

rach-id commented Nov 28, 2021 •

edited

Loading

rach-id commented Nov 28, 2021

codecov-commenter commented Dec 19, 2021 •

edited

Loading

rach-id commented Jan 4, 2022 •

edited

Loading

tzdybal left a comment

tzdybal Jan 4, 2022

adlerjohn Jan 4, 2022

rach-id Jan 5, 2022

tzdybal Jan 5, 2022

rach-id Jan 5, 2022

tzdybal Jan 5, 2022

rach-id Jan 5, 2022

rach-id commented Jan 4, 2022

adlerjohn Jan 4, 2022

rach-id Jan 5, 2022

adlerjohn Jan 5, 2022

rach-id Jan 5, 2022

tzdybal commented Jan 5, 2022

rach-id commented Jan 5, 2022

adlerjohn commented Jan 5, 2022

rach-id commented Jan 5, 2022

rach-id commented Jan 5, 2022

adds support for raw key instead of hashed key #64

Are you sure you want to change the base?

adds support for raw key instead of hashed key #64

Conversation

rach-id commented Nov 28, 2021 • edited Loading

rach-id commented Nov 28, 2021

codecov-commenter commented Dec 19, 2021 • edited Loading

Codecov Report

rach-id commented Jan 4, 2022 • edited Loading

tzdybal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rach-id commented Jan 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzdybal commented Jan 5, 2022

rach-id commented Jan 5, 2022

adlerjohn commented Jan 5, 2022

rach-id commented Jan 5, 2022

rach-id commented Jan 5, 2022

rach-id commented Nov 28, 2021 •

edited

Loading

codecov-commenter commented Dec 19, 2021 •

edited

Loading

rach-id commented Jan 4, 2022 •

edited

Loading