Das merkle accumulator - #739
Conversation
|
I talked to Rachel out of band to make sure I understand how this works. LGTM. I see there are some tests failing currently. |
edfelten
left a comment
edfelten
left a comment
There was a problem hiding this comment.
A few minor suggestions.
Also, we should think carefully about the fact that the same bytes can be encoded into different trees, having different hashes. Does that break anything? And even if it doesn't, should we dictate a single canonical tree shape in the interest of future-proofing?
edfelten
left a comment
edfelten
left a comment
There was a problem hiding this comment.
One small change in a comment suggested. Otherwise looks good.
Tristan-Wilson
left a comment
Tristan-Wilson
left a comment
There was a problem hiding this comment.
Do we want to have upgrade scripts for the other storage types (S3 and Database) in this PR or should that be in a different PR?
| return nil, err | ||
| } | ||
| if !bytes.Equal(crypto.Keccak256(keysetBytes), cert.KeysetHash[:]) { | ||
| if dastree.Hash(keysetBytes) != cert.KeysetHash { |
There was a problem hiding this comment.
What about keysets that were stored before the DASes were upgraded? I suppose we'll just fall back to getting it from the chain so it's probably fine...
There was a problem hiding this comment.
We now accept both :)
PlasmaPower
left a comment
PlasmaPower
left a comment
There was a problem hiding this comment.
Some minor recommendations but generally LGTM
PlasmaPower
left a comment
PlasmaPower
left a comment
There was a problem hiding this comment.
LGTM. I'm going to approve and merge this to include it in consensus v3, but I'd appreciate another review from @edfelten and/or @Tristan-Wilson, and worst case we'll make another consensus release with any fixes to it before we put it to use.
5 participants
This PR
common.Hasheverywhere