Conversation
|
|
||
| const treeID = int64(0) | ||
|
|
||
| var hasher = maphasher.Default |
There was a problem hiding this comment.
I moved the hasher variable to a package variable in order to make testing with a different hasher easier - one place to change it.
There was a problem hiding this comment.
Wouldn't we need a test matrix for that though?
There was a problem hiding this comment.
To do it properly, yes.
In the next PR, I'll be adding a MapEnv we can can do this with a table driven test.
| if err != nil { | ||
| t.Fatalf("Failed to calculate root: %v", err) | ||
| // createHStar2Leaves returns a []HStar2LeafHash formed by the mapping of index, value ... | ||
| // createHStar2Leaves panics if len(iv) is odd. Duplicate i/v pairs get over written. |
There was a problem hiding this comment.
Can you reference the doc that this came from and/or explain how we know the results are reliable?
There was a problem hiding this comment.
This algorithms is inspired by gRPC's [metadata.Pairs](https://github.com/grpc/grpc-go/blob/master/metadata/metadata.go#L65) which also has a key / value pair format.
I use a map internally to also guarantee that emitted values don't have duplicate keys.
There was a problem hiding this comment.
I think we're at cross purposes. I meant that before we had hardcoded values that couldn't go wrong. If this leaf generation code code is basically a copy of the code we're testing for example then it's not ideal.
There was a problem hiding this comment.
oh, hmm.
This isn't a new function - it replaces the previous createHStar2Leaves which was also in the test. It's just a test helper function. There's nothing like this function in the non-test codebase.
merkle/hstar2_test.go
Outdated
| } | ||
| } | ||
|
|
||
| // b64 converts a base64 string into []byte. |
There was a problem hiding this comment.
The name b64 sounds like an encoder rather than a decoder.
There was a problem hiding this comment.
would fromB64 be better? I was hoping to keep a short name because this function gets used a lot when forming table driven tests and a long name just pushes the test vectors off the screen.
| }, | ||
| testonly.MustDecodeBase64("U6ANU1en3BSbbnWqhV2nTGtQ+scBlaZf9kRPEEDZsHM="), | ||
| }, { | ||
| "key-1-848", |
There was a problem hiding this comment.
Are we reducing coverage by removing these cases?
There was a problem hiding this comment.
We're not losing any coverage. The cases weren't being used, and I don't think they were exercising any additional code paths.
In the next PR, I'll be adding a fuller suite of merkle tree tests that don't rely on test vectors, and can thus be used with any hasher.
|
Can you fix Travis and then it's probably ready. |
|
Looks like it was the |
3 participants
In order to facilitate easier debugging and prepare for the needed changes to support #691, I did some preliminary code cleanup:
hasher.BitLen()in place ofhasher.Size()*8testonly.HashKeymap[key]value->[][]byteof alternating index / value pairs.EmptyRoot*tests intosimpleTestVector