Client with (partially) verified QueueLeaf #320

gdbelvin · 2017-01-25T09:57:08Z

Client AddLeaf:

Fetches current Signed Tree Root
Queues the leaf
Waits for an STR update
TODO: Verify STR
Fetch and verify inclusion proof

Integration test improvements:

LogEnv also creates a signer
Graceful server shutdown
Adjust VerifyMerkleTree to accept a LeafHash rather than LeafData.

Contributes to #297

codingllama · 2017-01-30T18:46:11Z

client/client.go

+	}
+
+	// Wait for TreeSize to update.
+	if err := c.waitForSTRUpdate(ctx, 2); err != nil {


I think you need to pass in the first tree size, which must be read before queueLeaf.

Suppose the new leaf gets integrated really quickly. The tree size could change before the first call to waitForSTRUpdate(), which makes you block needlessly.

This won't block needlessly because it uses current STR in the LogClient as it's base line rather than the first fetch. I've added an UpdateSTR call before queue leaf

I'm now passing the current tree size

codingllama · 2017-01-30T18:46:28Z

client/client.go

+
+	// Wait for TreeSize to update.
+	if err := c.waitForSTRUpdate(ctx, 2); err != nil {
+		return nil


codingllama · 2017-01-30T18:47:36Z

client/client.go

+	}
+
+	// Get proof by hash.
+	err := c.getInclusionProof(ctx, leaf.MerkleLeafHash, c.STR.TreeSize)


Suggestion:

Either do "if err != ...; err != nil { return err }" and "return nil" at line 64, or remove the if and "return err" (as you do now)

now using if err:= foo; ... { format

codingllama · 2017-01-30T18:55:22Z

client/client.go


 	"github.com/google/trillian"
+	"github.com/jpillora/backoff"


I'll push back on jpillora's library due to the lack of an explicit license. Let's talk about this in person if you like.

Looks like he has an MIT license in the README. Opened a ticket for him to create a LICENSE file in his repo.

codingllama · 2017-01-30T19:00:16Z

client/client.go

+// waitForSTRUpdate repeatedly fetches the STR until the TreeSize changes
+// or until a maximum number of attempts (10) has been tried.
+func (c *LogClient) waitForSTRUpdate(ctx context.Context, maxRetries int) error {
+	startTreeSize := c.STR.TreeSize


As stated above, I think we need the STR from immediately before queueLeaf. If AddLeaf is called on a new client, the STR will be zeroed, which will cause this to detect a change even if the actual STR didn't change.

yep. Added UpdateSTR before queue leaf

codingllama · 2017-01-30T19:03:51Z

client/client.go

+		if err := c.UpdateSTR(ctx); err != nil {
+			return err
+		}
+		if i >= maxRetries {


I think this should be factored into the stop condition of the loop.

We've got 2 issues here:

The last attempt will always cause an error, regardless of a successful update

The loop does maxRetries+1 attempts, not maxRetries

Please add tests for the scenarios above before changing it.

Updated algorithm to be correct.

codingllama · 2017-01-30T19:05:17Z

client/client.go

+		LeafHash: hash,
+		TreeSize: treeSize,
+	}
+	_, err := c.client.GetInclusionProofByHash(ctx, req)


nit: Scope err in the if.

if _, err := ...; err != nil { return err }

codingllama · 2017-01-30T19:06:25Z

client/client_test.go

@@ -33,18 +34,38 @@ func TestAddLeaf(t *testing.T) {
 	if err := mysql.CreateTree(logID, env.DB); err != nil {
 		t.Errorf("Failed to create log: %v", err)
 	}
-
 	client := New(logID, env.ClientConn)

 	if err := client.AddLeaf(ctx, []byte("foo")); err != nil {
 		t.Errorf("Failed to add Leaf: %v", err)
 	}


Verify that the leaf was really added in the log? An empty AddLeaf() function would pass this test.

We need a GetLeaf don't we?

True. Add a TODO?

codingllama · 2017-01-30T19:07:51Z

client/client_test.go

 	client := New(logID, env.ClientConn)

 	if err := client.AddLeaf(ctx, []byte("foo")); err != nil {
 		t.Errorf("Failed to add Leaf: %v", err)
 	}
 }

-func TestAddSameLeaf(t *testing.T) {
+func TestUpdateSTR(t *testing.T) {
+	t.Skip("Need to add a leaf first")


Remove the skip? We have AddLeaf(), let's test it.

codingllama · 2017-01-30T19:09:23Z

client/client_test.go

+	}
+}
+
+func TestAddSameLeaf(t *testing.T) {
 	t.Skip("Submitting two leaves currently breaks")


Remove the skip here too?

codingllama

Thanks for the changes.

It seems that we still have a few open comments, plus Travis is broken. Could you verify?

codingllama · 2017-02-01T16:03:11Z

client/client_test.go

@@ -33,18 +34,38 @@ func TestAddLeaf(t *testing.T) {
 	if err := mysql.CreateTree(logID, env.DB); err != nil {
 		t.Errorf("Failed to create log: %v", err)
 	}
-
 	client := New(logID, env.ClientConn)

 	if err := client.AddLeaf(ctx, []byte("foo")); err != nil {
 		t.Errorf("Failed to add Leaf: %v", err)
 	}


True. Add a TODO?

gdbelvin · 2017-02-09T16:20:25Z

Updated and ready for review.

codingllama

I have a few requests:

Please don't squash commits on ongoing reviews. It makes it hard to figure out what you changed.
You have (had?) unit test failures on Travis, PTAL.
As we discussed offline, could you remove the dependency to jpillora's library? I think it buys too little to justify yet another dependency (plus licensing and so on).

Meanwhile I'll have another go at it and see if I have any further comments.

gdbelvin · 2017-02-10T15:21:51Z

Sorry for the squash. There was a large rebase which cased this.
Travis errors were in the past. These have been fixed.
Created a backoff library.

codingllama

Thanks for addressing the comments.

Could you add a test for Backoff, please?

codingllama · 2017-02-10T16:19:23Z

client/backoff/backoff.go

+	maxNanos := float64(b.Max.Nanoseconds())
+	nanos := math.Min(minNanos*math.Pow(b.Factor, float64(b.x)), maxNanos)
+	if b.Jitter {
+		// Generate a number in the range [b.Min, b.Max]


I think the range should be [b.Min,nanos), otherwise you may end up with a <= 0 number. Alternatively, you could just have "nanos = r".

TBH I don't exactly how you want Jitter to work, so I'm not entirely sure which approach is better.

codingllama · 2017-02-10T16:19:47Z

client/backoff/backoff.go

+)
+
+// Backoff specifies the parameters of the backoff algorithm.
+type Backoff struct {


Could you expand the documentation with a simple usage example, plus a brief explanation of what Jitter does?

codingllama · 2017-02-10T16:23:18Z

client/backoff/backoff.go

+	x      int
+}
+
+// Duration returns the time to wait on duration x.


Add:

"Every time Duration() is called the returned value will exponentially increase, up to Backoff.Max. If Jitter is true, instead of increasing exponentially, a random Duration will be picked in the interval [insert interval here]".

codingllama

Please add a test for Backoff; other than that I'm happy to approve.

gdbelvin · 2017-02-13T14:21:14Z

Added backoff test.

codingllama

Please address the remaining comment. Other than that, as long as Travis is happy, LGTM.

codingllama · 2017-02-13T14:57:44Z

client/backoff/backoff_test.go

+		{b, 2, time.Duration(2) * time.Second, time.Duration(4) * time.Second},
+		{b, 3, time.Duration(4) * time.Second, time.Duration(8) * time.Second},
+		{b, 4, time.Duration(8) * time.Second, time.Duration(16) * time.Second},
+		{b, 8, time.Duration(100) * time.Second, time.Duration(200) * time.Second},


Should we limit Jitter to Max? That would be my working assumption.

If we don't, please document it on Backoff.

I thought about capping Jitter at Max, but I didn't want the max to cap things at a static value when jitter is on lets clients all clients wait the same amount of time.
The current algorithm uses a geometrically increasing delay [A, B, C, D] = [ca^0, ca^1, c^a*2..]. Jitter selects a value from next higher interval. To keep max constant, jitter would have to select from the next lower interval, which would cause problems for x=0.

Fair enough. I'm happy with the documentation change.

Client AddLeaf: - Fetches current Signed Tree Root - Queues the leaf - Waits for an STR update - TODO: Verify STR - Fetch and verify inclusion proof Integration test improvements: - LogEnv also creates a signer

- Created backoff library - Rebased and removed signinterval

daviddrysdale · 2017-02-23T09:31:31Z

Looking at this commit after the fact, a couple of things:

The commit message / summary could be more helpful. The summary implies it's all about client verification, but there are other changes that either only get mentioned in the body of the commit message (adding a signer), or are not mentioned at all (modifying storage to use gRPC errors). Please separate distinct chunks into separate commits (they can still be in the same PR, but you'll need to get used to git rebase -i etc.)
Adding a dependency from storage/mysql to grpc just to avoid a bit of error mapping seems like a bad idea. I think we should back that out.

googlebot added the cla: yes label Jan 25, 2017

gdbelvin force-pushed the client2 branch 3 times, most recently from 29a2dc7 to 040b197 Compare January 27, 2017 15:49

codingllama self-requested a review January 30, 2017 18:43

codingllama suggested changes Jan 30, 2017

View reviewed changes

gdbelvin force-pushed the client2 branch from 040b197 to 5e62c16 Compare February 1, 2017 15:52

codingllama self-assigned this Feb 1, 2017

codingllama requested a review from AlCutter February 1, 2017 16:00

codingllama suggested changes Feb 1, 2017

View reviewed changes

gdbelvin mentioned this pull request Feb 1, 2017

Create log verifier client #297

Closed

9 tasks

gdbelvin force-pushed the client2 branch 4 times, most recently from 8cd31e3 to 09ce4c9 Compare February 9, 2017 16:16

gdbelvin changed the title ~~Client with getProofByHash~~ Client with (partially) verified QueueLeaf Feb 9, 2017

gdbelvin force-pushed the client2 branch from 74c1da0 to dfdec76 Compare February 9, 2017 17:00

codingllama suggested changes Feb 9, 2017

View reviewed changes

gdbelvin force-pushed the client2 branch from dfdec76 to f30c5e4 Compare February 10, 2017 14:58

codingllama reviewed Feb 10, 2017

View reviewed changes

codingllama approved these changes Feb 13, 2017

View reviewed changes

gdbelvin added 5 commits February 13, 2017 15:16

Client with (partially) verified AddLeaf

b483b9f

Client AddLeaf: - Fetches current Signed Tree Root - Queues the leaf - Waits for an STR update - TODO: Verify STR - Fetch and verify inclusion proof Integration test improvements: - LogEnv also creates a signer

Reviewer comments

f08395a

- Created backoff library - Rebased and removed signinterval

Better jitter algo + comments

18520a4

backoff tests

19d0e2f

documentation

853620e

fix golint

82888fb

gdbelvin force-pushed the client2 branch from 3595611 to 82888fb Compare February 13, 2017 15:33

gdbelvin merged commit a716680 into google:master Feb 13, 2017

gdbelvin deleted the client2 branch July 4, 2017 12:01

Client with (partially) verified QueueLeaf #320

Client with (partially) verified QueueLeaf #320

Conversation

gdbelvin commented Jan 25, 2017 • edited

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

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

codingllama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdbelvin commented Feb 9, 2017

codingllama left a comment

Choose a reason for hiding this comment

gdbelvin commented Feb 10, 2017

codingllama left a 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

codingllama left a comment

Choose a reason for hiding this comment

gdbelvin commented Feb 13, 2017

codingllama left a 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

daviddrysdale commented Feb 23, 2017

gdbelvin commented Jan 25, 2017 •

edited