Fix/756 Fix ListHistory command#761
Conversation
liamsi
left a comment
liamsi
left a comment
There was a problem hiding this comment.
A few suggestions/questions. But LGTM
|
|
||
| // Initialize inserts the object hash of an empty struct into the log if it is empty. | ||
| // This keeps the log leaves in-sync with the map which starts off with an | ||
| // empty log root at map revision 0. |
There was a problem hiding this comment.
That is a cool and simple idea!
core/signer/signer.go
Outdated
| if logRoot.GetSignedLogRoot().GetTreeSize() == 0 && | ||
| mapRoot.GetMapRoot().GetMapRevision() == 0 { | ||
| smrJSON, err := json.Marshal(mapRoot.GetMapRoot()) | ||
| idHash := sha256.Sum256(smrJSON) |
There was a problem hiding this comment.
What is the reason behind not calling ObjectHash() on the map root?
There was a problem hiding this comment.
The log server sets the leaf hash for us. It's only duplicate work on our end to do it here.
core/signer/signer.go
Outdated
| }, | ||
| }); err != nil { | ||
| return fmt.Errorf("trillianLog.QueueLeaf(logID: %v, leaf: %v): %v", | ||
| s.logID, smrJSON, err) |
There was a problem hiding this comment.
This is exactly the same code as in https://github.com/google/keytransparency/pull/761/files#diff-98fddaa13ed78860afbe32b4e92dc431R116 (only the two TODOs are different).
What do you think about refactoring this into a private helper writeToLog(tlog *LogClient, smr *trillian.SignedMapRoot)
There was a problem hiding this comment.
Good call. Eventually we will replace this code with a proper Verifying Log Client that auto configures itself
README.md
Outdated
| ``` | ||
|
|
||
| 3. Run Key Transparency | ||
| - `docker-compose build kt-signer` |
There was a problem hiding this comment.
This looks like we might need to update the deploy script. I'll take care of that in case this is necessary.
gdbelvin
changed the title
Cleans up a few off by on errors and poor stopping condition logic in the list history command. Fixes google#763
Codecov Report
@@ Coverage Diff @@
## master #761 +/- ##
=========================================
Coverage ? 48.42%
=========================================
Files ? 28
Lines ? 2480
Branches ? 0
=========================================
Hits ? 1201
Misses ? 1093
Partials ? 186
Continue to review full report at Codecov.
|
3 participants
Cleans up a few off by on errors and poor stopping condition logic in the list history command.
Fixes #756
Rebased on #760 and #762. Merge them first.