Conversation
|
Also replaces tendermint/tmlibs#211 |
|
referencing discussion on hashing inner & leaf nodes differently; as well as the removed encoding (as both might or might not have security implications): |
| ) | ||
|
|
||
| // Hash(left | right). The basic operation of the Merkle tree. | ||
| func SimpleHashFromTwoHashes(left, right []byte) []byte { |
There was a problem hiding this comment.
If the size of left and right is now fixed, can we use the size in the args:
SimpleHashFromTwoHashes(left, right [Size]byte)
see also: tendermint/tmlibs#211 (comment)
There was a problem hiding this comment.
Making this explicit via left, right [tmhash.Size]byte makes it necessary to change a lot of places. I propose to keep the encoding (called length prefix here: tendermint/tmlibs#211) and potentially refactor this at a later point.
There was a problem hiding this comment.
damn ok. my motivation was to simplify the tree description by removing unecessary length prefixing. but i agree that if we do so, we should introduce some better type safety, and doing so will require ALOT of changes ... so maybe its not that important right now ? Just feels like now is a good time to make wanted breaking changes to the data structures :P
There was a problem hiding this comment.
Also having length prefixes doesnt solve the 2nd pre-image attack right ?
There was a problem hiding this comment.
Nope. It just mitigates similar but simpler collisions (e.g., I guess where you just append 2 byte slices together and yield the same root hash as with two leafs ?). Let's re-add the encoding (which allows for variable sized byte slices to be hashed) and add proper prefixes in a separate issue.
There was a problem hiding this comment.
so maybe its not that important right now ?
it's definitely easier to simply re-introduce the encoding. I've already started adding fixed lengths everywhere in another branch. There seems to be sth wrong though (didn't investigate but I pushed it now: https://github.com/Liamsi/go-crypto/compare/bucky_fixed...Liamsi:fixed_size?expand=1)
There was a problem hiding this comment.
also if we go through with tendermint/tmlibs#213 we'll need the length prefix in here.
I guess so long as we use []byte, we should use a length prefix, at least so people don't use it wrong.
There was a problem hiding this comment.
sth wrong how ?
TestSimpleProof fails on every index
I guess so long as we use []byte, we should use a length prefix, at least so people don't use it wrong.
Totally.
merkle/simple_tree.go
Outdated
| / \ / \ / \ | ||
| h0 h1 h2 h3 h4 h5 | ||
|
|
||
| */ |
There was a problem hiding this comment.
Can we add a warning here about pre-image attacks, or, more general a warning on how to use all this safely (in our context, but also in general). See for instance: https://github.com/bitcoin/bitcoin/blob/5961b23898ee7c0af2626c46d5d70e80136578d3/src/consensus/merkle.cpp#L9-L43
There was a problem hiding this comment.
I can take care of this later too (I would open an issue though referencing relevant comments)
There was a problem hiding this comment.
Would love it if you could pick this up! Thanks!
|
@liamsi also worth noting how we use these functions from See PR to update tendermint dependent on this in tendermint/tendermint#1606 |
|
Thanks for the pointer! If you like, we can fix the test-vectors here and merge this (so you can reference the development branch there instead: https://github.com/tendermint/tendermint/pull/1606/files#diff-bd247e83efc3c45ae9e8c47233249f18R261). |
merkle/simple_map_test.go
Outdated
| type strHasher string | ||
|
|
||
| func (str strHasher) Hash() []byte { | ||
| return SimpleHashFromBytes([]byte(str)) |
There was a problem hiding this comment.
Any reason SimpleHashFromBytes disappeared?
https://github.com/tendermint/tmlibs/blob/50ad19541d61a60bdd6be6f407af3ed7abd6ba44/merkle/simple_tree.go#L70-L74
There was a problem hiding this comment.
meh - trying to simplify the exposed interface and it got replaced with tmhash.Sum
|
Cool I'm going to squash some commits and then use amino for the encodeByteSlice (didn't want to do that before to avoid an amino dependency in tmlibs) |
Ref tendermint/tmlibs#210 (comment)