Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow log server to return consistency proofs where first=second #819

Merged
merged 4 commits into from
Sep 12, 2017
Merged

Allow log server to return consistency proofs where first=second #819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2b075ac
Allow log server to return proofs where first=second
Martin2112 Sep 6, 2017
186cae1
lint fix
Martin2112 Sep 6, 2017
f978d78
Fix typo
Martin2112 Sep 6, 2017
4e5abde
Fix inconsistent error message.
Martin2112 Sep 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 71 additions & 15 deletions server/log_rpc_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ var (

getConsistencyProofRequest25 = trillian.GetConsistencyProofRequest{LogId: logID1, FirstTreeSize: 10, SecondTreeSize: 25}
getConsistencyProofRequest7 = trillian.GetConsistencyProofRequest{LogId: logID1, FirstTreeSize: 4, SecondTreeSize: 7}
getConsistencyProofRequest44 = trillian.GetConsistencyProofRequest{LogId: logID1, FirstTreeSize: 4, SecondTreeSize: 4}
getConsistencyProofRequest48 = trillian.GetConsistencyProofRequest{LogId: logID1, FirstTreeSize: 4, SecondTreeSize: 8}
getConsistencyProofRequest54 = trillian.GetConsistencyProofRequest{LogId: logID1, FirstTreeSize: 5, SecondTreeSize: 4}

nodeIdsInclusionSize7Index2 = []storage.NodeID{
stestonly.MustCreateNodeIDForTreeCoords(0, 3, 64),
Expand Down Expand Up @@ -1231,38 +1234,91 @@ func TestGetConsistencyProofCommitFails(t *testing.T) {
test.executeCommitFailsTest(t, getConsistencyProofRequest7.LogId)
}

func TestGetConsistencyProof(t *testing.T) {
// Ask for a proof from size 4 to 8 but the tree is only size 7. This should fail.
func TestGetConsistencyProofOutsideTree(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

mockStorage := storage.NewMockLogStorage(ctrl)
mockTx := storage.NewMockLogTreeTX(ctrl)
mockStorage.EXPECT().SnapshotForTree(gomock.Any(), getConsistencyProofRequest7.LogId).Return(mockTx, nil)
mockStorage.EXPECT().SnapshotForTree(gomock.Any(), getConsistencyProofRequest48.LogId).Return(mockTx, nil)

mockTx.EXPECT().LatestSignedLogRoot(gomock.Any()).Return(signedRoot1, nil)
mockTx.EXPECT().ReadRevision().Return(signedRoot1.TreeRevision)
mockTx.EXPECT().GetMerkleNodes(gomock.Any(), revision1, nodeIdsConsistencySize4ToSize7).Return([]storage.Node{{NodeID: stestonly.MustCreateNodeIDForTreeCoords(2, 1, 64), NodeRevision: 3, Hash: []byte("nodehash")}}, nil)
mockTx.EXPECT().Commit().Return(nil)
mockTx.EXPECT().Close().Return(nil)

registry := extension.Registry{
AdminStorage: mockAdminStorage(ctrl, getConsistencyProofRequest7.LogId),
AdminStorage: mockAdminStorage(ctrl, getConsistencyProofRequest48.LogId),
LogStorage: mockStorage,
}
server := NewTrillianLogRPCServer(registry, fakeTimeSource)

response, err := server.GetConsistencyProof(context.Background(), &getConsistencyProofRequest7)
if err != nil {
t.Fatalf("failed to get consistency proof: %v", err)
_, err := server.GetConsistencyProof(context.Background(), &getConsistencyProofRequest48)
if err == nil {
t.Fatal("incorrectly returned a proof for out of range tree size")
}
}

// Ensure we got the expected proof
expectedProof := trillian.Proof{
LeafIndex: 0,
Hashes: [][]byte{[]byte("nodehash")},
// Ask for a proof from size 5 to 4, testing the boundary condition. This request should fail
// before making any storage requests.
func TestGetConsistencyProofRangeError(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

mockStorage := storage.NewMockLogStorage(ctrl)
registry := extension.Registry{
AdminStorage: mockAdminStorage(ctrl, getConsistencyProofRequest54.LogId),
LogStorage: mockStorage,
}
if !proto.Equal(response.Proof, &expectedProof) {
t.Fatalf("expected proof: %v but got: %v", expectedProof, response.Proof)
server := NewTrillianLogRPCServer(registry, fakeTimeSource)

_, err := server.GetConsistencyProof(context.Background(), &getConsistencyProofRequest54)
if err == nil {
t.Fatal("incorrectly returned a proof for out of range tree size")
}
}

func TestGetConsistencyProof(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a lot of these tests could be collapsed into a table-driven one which includes a wantError flag, no?
Perhaps that could be done in a follow-up, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it could be done but seemed out of scope for this change.

ctrl := gomock.NewController(t)
defer ctrl.Finish()

// getConsistencyProofRequest7 - tests a normal request which should succeed
// getConsistencyProofRequest44 - tests an edge condition we used to reject but now support
// for compatibility with the older C++ log servers.
testCases := []trillian.GetConsistencyProofRequest{getConsistencyProofRequest7, getConsistencyProofRequest44}
nodeIDs := [][]storage.NodeID{nodeIdsConsistencySize4ToSize7, {}}
hashes := [][][]byte{{[]byte("nodehash")}, {}}
nodes := [][]storage.Node{{{NodeID: stestonly.MustCreateNodeIDForTreeCoords(2, 1, 64), NodeRevision: 3, Hash: []byte("nodehash")}}, {}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to put all the per-test case info into a single struct:

vars tests = []struct {
  req trillian.GetConsistencyProofRequest
  nodeIDs []storage.NodeID
  hashes [][]byte
  nodes []storage.Node
}{
  {
    req: &getConsistencyProofRequest7,
    nodeIDs: nodeIdsConsistencySize4ToSize7,
    hashes: [][]byte{{[]byte("nodehash")},
    nodes: []storage.Node{{NodeID: stestonly.MustCreateNodeIDForTreeCoords(2, 1, 64), NodeRevision: 3, Hash: []byte("nodehash")}},
  },
  {
    req: &getConsistencyProofRequest44,
  },
}
for _, test := range tests {
   ...

(drop if this is coming in follow-up PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I've got this done but the branch is based off this one.


for i := range testCases {
mockStorage := storage.NewMockLogStorage(ctrl)
mockTx := storage.NewMockLogTreeTX(ctrl)
mockStorage.EXPECT().SnapshotForTree(gomock.Any(), testCases[i].LogId).Return(mockTx, nil)

mockTx.EXPECT().LatestSignedLogRoot(gomock.Any()).Return(signedRoot1, nil)
mockTx.EXPECT().ReadRevision().Return(signedRoot1.TreeRevision)
mockTx.EXPECT().GetMerkleNodes(gomock.Any(), revision1, nodeIDs[i]).Return(nodes[i], nil)
mockTx.EXPECT().Commit().Return(nil)
mockTx.EXPECT().Close().Return(nil)

registry := extension.Registry{
AdminStorage: mockAdminStorage(ctrl, testCases[i].LogId),
LogStorage: mockStorage,
}
server := NewTrillianLogRPCServer(registry, fakeTimeSource)

response, err := server.GetConsistencyProof(context.Background(), &testCases[i])
if err != nil {
t.Fatalf("failed to get consistency proof: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t.Errorf and continue so the loop continues
(OK to defer if you've got a follow-up PR that makes things more table-driven overall)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, upcoming PR moves the consistency proof tests into one table test.

}

// Ensure we got the expected proof
expectedProof := trillian.Proof{
LeafIndex: 0,
Hashes: hashes[i],
}
if !proto.Equal(response.Proof, &expectedProof) {
t.Fatalf("expected proof: %v but got: %v", expectedProof, response.Proof)
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions server/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ func validateGetConsistencyProofRequest(req *trillian.GetConsistencyProofRequest
if req.SecondTreeSize <= 0 {
return status.Errorf(codes.InvalidArgument, "GetConsistencyProofRequest.SecondTreeSize: %v, want > 0", req.SecondTreeSize)
}
if req.SecondTreeSize <= req.FirstTreeSize {
return status.Errorf(codes.InvalidArgument, "GetConsistencyProofRequest.FirstTreeSize: %v < GetConsistencyProofRequest.SecondTreeSize: %v, want > ", req.FirstTreeSize, req.SecondTreeSize)
if req.SecondTreeSize < req.FirstTreeSize {
return status.Errorf(codes.InvalidArgument, "GetConsistencyProofRequest.FirstTreeSize: %v < GetConsistencyProofRequest.SecondTreeSize: %v, want >= ", req.FirstTreeSize, req.SecondTreeSize)
}
return nil
}
Expand Down