Move SignedMapRoot fields to MapRootV1 #1060

gdbelvin · 2018-03-19T15:12:19Z

This PR moves the rest of the fields in SignedMapRoot into MapRoot and uses MapRootV1 throughout the code base.

codecov-io · 2018-03-19T15:34:30Z

Codecov Report

Merging #1060 into master will decrease coverage by 0.12%.
The diff coverage is 43.9%.

@@            Coverage Diff             @@
##           master    #1060      +/-   ##
==========================================
- Coverage    62.6%   62.47%   -0.13%     
==========================================
  Files         103      103              
  Lines        8455     8462       +7     
==========================================
- Hits         5293     5287       -6     
- Misses       2632     2642      +10     
- Partials      530      533       +3

Impacted Files	Coverage Δ
client/map_verifier.go	`0% <0%> (ø)`	⬆️
examples/ct/ctmapper/mapper/mapper.go	`11.81% <0%> (-0.19%)`	⬇️
integration/maptest/map.go	`72.05% <37.5%> (-0.75%)`	⬇️
crypto/verifier.go	`48.14% <42.85%> (-0.95%)`	⬇️
server/map_rpc_server.go	`30.58% <50%> (-1.13%)`	⬇️
storage/mysql/map_storage.go	`66.66% <61.11%> (-1.93%)`	⬇️
crypto/signer.go	`72% <77.77%> (+2.76%)`	⬆️
util/tracker.go	`96.61% <0%> (-3.39%)`	⬇️

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 6282acf...a1d851b. Read the comment docs.

phad · 2018-03-19T15:38:11Z

testonly/hammer/hammer.go

@@ -806,15 +838,20 @@ func (s *hammerState) getSMRRevInvalid(ctx context.Context) error {
 	return nil
 }

-func timeFromNanos(nanos int64) time.Time {
-	return time.Unix(nanos/1e9, nanos%1e9)
+func timeFromNanos(nanos uint64) time.Time {


No need for this, you can pass nano >= 10^9 into https://golang.org/pkg/time/#Unix
i.e. your func is equivalent to

func timeFromNanos(nanos uint64) time.Time { return time.Unix(0, int64(nanos)) }

BTW do you need to use uint64 here?

using uint64 allows me to avoid a few typecasts.

phad · 2018-03-19T15:39:49Z

trillian.proto

+  reserved 8; // Deprecated: Was metadata bytes. Use map_root.
+
+  // map_root holds the TLS-serialization of the following
+  // structure (described in RFC5246 notation):


Should you be referencing types.MapRootV1 here, or is that considered an implementation detail?

That's a Go specific implementation detail and not technically part of the public API.

phad · 2018-03-19T15:40:48Z

testonly/hammer/hammer.go

@@ -167,6 +168,16 @@ func (c MapConfig) String() string {
 		c.MapID, c.EPBias, c.Operations, c.EmitInterval, c.IgnoreErrors, c.CheckSignatures)
 }

+// CheckSignature verifies the signature and returns the map root contents.
+func CheckSignature(ctf *MapConfig, r *trillian.SignedMapRoot) (*types.MapRootV1, error) {


What is ctf? Is it an abbreviation of config? I'd go for conf, config or mc here.

phad · 2018-03-19T15:42:01Z

testonly/hammer/hammer.go


 	s.pushSMR(rsp.MapRoot)
-	if err := s.updateContents(rsp.MapRoot.MapRevision, leaves); err != nil {
+	if err := s.updateContents(int64(root.Revision), leaves); err != nil {


How about changing updateContents to accept int64 instead?

phad · 2018-03-19T15:54:15Z

storage/mysql/map_storage_test.go

@@ -149,23 +161,27 @@ func TestMapReadWriteTransaction(t *testing.T) {
 			err := s.ReadWriteTransaction(ctx, tree, func(ctx context.Context, tx storage.MapTreeTX) error {
 				root, err := tx.LatestSignedMapRoot(ctx)
 				if err != nil && !strings.Contains(err.Error(), test.wantRootErr) {
-					t.Errorf("%v: LatestSignedMapRoot() returned err = %v", test.desc, err)
+					t.Errorf("LatestSignedMapRoot() returned err = %v", err)


How about return after this error? It'll abort this sub-test, allowing you to un-indent the main test in the if err == nil block further down.

phad · 2018-03-19T15:54:25Z

storage/mysql/map_storage_test.go

 				}
 				if err == nil && len(test.wantRootErr) != 0 {
-					t.Errorf("%v: LatestSignedMapRoot() returned err = %v, want: nil", test.desc, err)
+					t.Errorf("LatestSignedMapRoot() returned err = %v, want: nil", err)


Maybe you could return after this condition too, to abort the sub-test.

phad · 2018-03-19T15:56:21Z

storage/mysql/map_storage_test.go

 				}
 				if err == nil {
+					var mapRoot types.MapRootV1
+					if err := mapRoot.UnmarshalBinary(root.MapRoot); err != nil {
+						t.Errorf("UmarshalBinary(): %v", err)


Possibly return after this too: if you fail to unmarshal the maproot there'll be nothing worth testing about the fields you expected.

phad · 2018-03-20T10:53:54Z

crypto/verifier.go

-	j, err := json.Marshal(obj)
-	if err != nil {
-		return err
+// VerifySignedMapRoot verifies the signature on the SignedMapRoot.


Not clear from this documentation why it is also returning a MapRootV1.

gdbelvin · 2018-03-21T11:42:01Z

Updated and ready for a second look

* master: Move SignedMapRoot fields to MapRootV1 (google#1060)

googlebot added the cla: yes label Mar 19, 2018

gdbelvin requested a review from phad March 19, 2018 15:12

phad reviewed Mar 20, 2018

View reviewed changes

gdbelvin force-pushed the f/sig/signmaproot branch from fc448e8 to 97a8348 Compare March 21, 2018 09:51

gdbelvin added 12 commits March 21, 2018 11:22

SignMapRoot returns *trillian.SignedMapRoot

ac0c1ea

SignMapRoot accepts types.MapRootV1

4a6ca5f

Remove MapID from SignedMapRoot

d36119c

Set and verify MapRoot

5f2fd05

Remove SignObject

d37e141

Deprecate MapRoot fields

0f1ec5d

Add MapRootV1 to VerifySignedMapRoot

ffce607

Use new MapRoot in code

a076a0b

Remove SignedMapRoot fields

e620095

Remove remaining references to SigndMapRoot fields

2bb9dbd

fix map verifier

26e2e83

Reviewer comments

a1d851b

gdbelvin force-pushed the f/sig/signmaproot branch from 97a8348 to a1d851b Compare March 21, 2018 11:28

phad approved these changes Mar 21, 2018

View reviewed changes

gdbelvin merged commit 0c12c84 into google:master Mar 21, 2018

gdbelvin deleted the f/sig/signmaproot branch March 21, 2018 14:04

gdbelvin added a commit to gdbelvin/trillian that referenced this pull request Mar 21, 2018

Merge branch 'master' into f/sig/logroot_proto2

23860e0

* master: Move SignedMapRoot fields to MapRootV1 (google#1060)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move SignedMapRoot fields to MapRootV1 #1060

Move SignedMapRoot fields to MapRootV1 #1060

gdbelvin commented Mar 19, 2018

codecov-io commented Mar 19, 2018 •

edited

Loading

phad Mar 19, 2018

gdbelvin Mar 21, 2018

gdbelvin Mar 21, 2018

phad Mar 19, 2018

gdbelvin Mar 21, 2018

phad Mar 19, 2018

gdbelvin Mar 21, 2018

phad Mar 19, 2018

gdbelvin Mar 21, 2018

phad Mar 19, 2018

gdbelvin Mar 21, 2018

phad Mar 19, 2018

gdbelvin Mar 21, 2018

phad Mar 19, 2018

gdbelvin Mar 21, 2018

phad Mar 20, 2018

gdbelvin Mar 21, 2018

gdbelvin commented Mar 21, 2018

Move SignedMapRoot fields to MapRootV1 #1060

Move SignedMapRoot fields to MapRootV1 #1060

Conversation

gdbelvin commented Mar 19, 2018

codecov-io commented Mar 19, 2018 • edited Loading

Codecov Report

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

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

Choose a reason for hiding this comment

gdbelvin commented Mar 21, 2018

codecov-io commented Mar 19, 2018 •

edited

Loading