Use absolute addressing in HStar2 by gdbelvin · Pull Request #765 · google/trillian

gdbelvin · 2017-07-28T13:09:32Z

Secure versions of HashEmpty take the node's address into account. See #670.
This PR modifies HStar2 and its callers to use NodeIDs for absolute addressing in order to supply full paths to the hashing functions.

HStar2 Changes

HStar2 in its current form only uses subtree relative addressing.
This PR modifies the HStar2 algorithm from operating in terms of subtree height to absolute depth.

from
hStar2b(n int, values []HStar2LeafHash, offset *big.Int)
to
hStar2b(depth, maxDepth int, values []HStar2LeafHash, offset *big.Int)

And it also provides HStar2Nodes with the prefix of the subtree it is operating on.

NodeID Changes

In order to provide correct conversions to/from Hstar2LeafHash, NodeID, Prefix and Suffix the following functions are added and tested:

ParseSuffix
NodeID.BigInt
NewNodeIDFromBigInt
NewNodeIDFromPrefixSuffix

Verifier Changes

On the verification side, the verification algorithm is also modified to provide a correct path to HashEmpty.

Closes #670
Rebased on #761 and #772

gdbelvin · 2017-07-28T15:24:21Z

integration/maptest/map_test.go

 			desc:         "maphasher batch",
 			HashStrategy: trillian.HashStrategy_TEST_MAP_HASHER,
-			batchSize:    64, numBatches: 32,
+			batchSize:    1, numBatches: 1,


gdbelvin · 2017-07-28T15:27:15Z

merkle/hstar2.go

+	depth := len(prefix) * 8
+	totalDepth := depth + subtreeDepth
+	if totalDepth > s.hasher.BitLen() {
 		return nil, ErrNegativeTreeLevelOffset


change error

gdbelvin · 2017-07-28T15:28:04Z

merkle/hstar2.go

-		func(depth int, index *big.Int, hash []byte) error {
-			return set(treeDepth-depth, index, hash)
-		})
+	offset := new(big.Int).SetBytes(prefix)


Use NewFromPreifx

gdbelvin · 2017-07-28T15:31:58Z

merkle/hstar2.go

+			return h, nil
+		}
+	}
+	height := s.hasher.BitLen() - depth


TODO: convert hashers back to accepting depth arguments

gdbelvin · 2017-07-28T15:33:39Z

merkle/hstar2_test.go

 		}
-		ret = append(ret, HStar2LeafHash{prefix, root})
+
+		index := new(big.Int).SetBytes(prefix)


Use NewNode Functions

gdbelvin · 2017-07-28T15:35:48Z

merkle/sparse_merkle_tree.go

 			return
 		}
-		leaves = append(leaves, HStar2LeafHash{Index: new(big.Int).SetBytes(ih.index), LeafHash: ih.hash})
+		node := storage.NewNodeIDFromPrefixSuffix(ih.index, storage.Suffix{}, s.hasher.BitLen())


s/node/nodeID/

gdbelvin · 2017-07-28T15:38:49Z

merkle/sparse_merkle_tree.go

 		nodesToStore = append(nodesToStore,
 			storage.Node{
-				NodeID:       storage.NewNodeIDFromHash(bytes.Join([][]byte{s.prefix, ih.index}, []byte{})),
+				NodeID:       storage.NewNodeIDFromHash(ih.index),


XXX: should this be the above node?

Martin2112 · 2017-07-31T11:18:12Z

Can we drop the test timeout change from the PR?

gdbelvin · 2017-07-31T11:34:38Z

Now that we're running BatchTest as part of the integration tests on travis, we're going to need more time. We have a couple of options:

Set the timeout to 9 minutes, so it's less than Travis's timeout.
Annotate tests with short / long and only run the short tests on travis. (use the go test -short flag)
Increase the Travis timeout to be > 10 min.

WDYT?

Martin2112 · 2017-07-31T11:51:29Z

Can we get a rough breakdown of where the time is being spent? I'm reluctant to have people waiting > 10 mins for test runs but if it's necessary to ensure correctness then we might have to.

gdbelvin · 2017-07-31T12:40:57Z

Here's the output of a run on my local machine (which is a bit faster than travis). It's clear that the large batch test is taking up almost all of the time. The batch test is running 32 batches of size 64 = 2048 leaves being written and read back out.

=== RUN TestInclusion
--- PASS: TestInclusion (0.11s)
=== RUN TestInclusionBatch
--- PASS: TestInclusionBatch (30.80s)
=== RUN TestNonExistentLeaf
--- PASS: TestNonExistentLeaf (0.02s)
PASS

Martin2112 · 2017-07-31T12:44:22Z

Do they run at this speed on Travis? I think 30 seconds isn't unreasonable. But we keep adding tests the cumulatively take a long time to run.

gdbelvin · 2017-07-31T13:29:41Z

They run much slower on travis. I've set the timeout back to 5 minutes and set travis to only run the short version of tests.

I should note here that the full integration tests also get run without a timeout through ./integration/integration_test.sh

Martin2112 · 2017-08-03T07:45:46Z

I think this PR needs a rebase / merge now?

gdbelvin · 2017-08-03T10:45:49Z

Rebased and ready for review

Martin2112 · 2017-07-31T14:01:10Z

storage/suffix.go

+func ParseSuffix(s string) (Suffix, error) {
+	b, err := base64.StdEncoding.DecodeString(s)
+	if err != nil {
+		return Suffix{}, err


Does nil, err not work?

nil doesn't work here because the return argument is Suffix rather than *Suffix.
Fear not, I have more pull requests if you so desire :-)

Yep, I can't wait!

Martin2112 · 2017-08-03T10:56:47Z

integration/maptest/map_test.go

+			leaves: []*trillian.MapLeaf{
+				{Index: h2b("0000000000000000000000000000000000000000000000000000000000000000"), LeafValue: []byte("A")},
+				{Index: h2b("0000000000000000000000000000000000000000000000000000000000000001"), LeafValue: []byte("B")},
+				{Index: h2b("0000000000000000000000000000000000000000000000000000000000000002"), LeafValue: []byte("C")},


How about a multi byte leaf value test, just for paranoia. Might also want to test what happens if the leaf value is empty.

The empty leaf case is both in the just merged TestLeafHistory and in TestNonexistantLeaf. I can add here as well.

Martin2112 · 2017-08-03T11:21:31Z

merkle/hstar2_test.go

-			b = append([]byte{0}, b...)
+		subtreeDepth := s.hasher.BitLen() - prefixSize
+		prefix := lh[i].Index.Bytes()
+		// ensure we've got any chopped off leading zero bytes


I don't think that comment is fully clear. I know it was there before but how about "left pad prefix with any zero bytes that were previously removed".

Martin2112 · 2017-08-03T11:27:19Z

storage/suffix_test.go

+		want      []byte
+	}{
+		{h2b(""), 1, h2b("0801")},
+		{h2b("00"), 1, h2b("0801")},


Not testing any invalid cases?

gdbelvin · 2017-08-04T11:05:39Z

PTAL

googlebot added cla: yes labels Jul 28, 2017

gdbelvin added WIP bug labels Jul 28, 2017

gdbelvin force-pushed the map_clean branch from 4408212 to ef85e41 Compare July 28, 2017 14:45

gdbelvin removed the WIP label Jul 28, 2017

gdbelvin changed the title ~~Map clean~~ Use absolute addressing in HStar2 Jul 28, 2017

gdbelvin commented Jul 28, 2017

View reviewed changes

gdbelvin requested review from AlCutter, Martin2112, RJPercival and pphaneuf July 28, 2017 15:43

gdbelvin force-pushed the map_clean branch 5 times, most recently from b1e3f57 to 0248a22 Compare July 31, 2017 10:31

gdbelvin assigned Martin2112 Jul 31, 2017

This was referenced Jul 31, 2017

Move NodeID to it's own package #768

Closed

Remove depth parameter from MapHasher.HashLeaf #769

Merged

gdbelvin force-pushed the map_clean branch from 0248a22 to 09c5fe2 Compare July 31, 2017 13:27

gdbelvin force-pushed the map_clean branch 4 times, most recently from 07bb675 to 23edc87 Compare August 2, 2017 21:04

gdbelvin force-pushed the map_clean branch from 23edc87 to acf6f4d Compare August 3, 2017 10:44

Martin2112 reviewed Aug 3, 2017

View reviewed changes

gdbelvin added 11 commits August 3, 2017 17:21

Add index to HStar2Nodes. Tests pass

70d7b4e

Use absolute indexes

60470f0

Add MaskIndex

450e2d2

ParseSuffix

906591d

Tests pass - proper indexes to HStar2

0995623

NewNodeIDFromPrefixSuffix

11e8de1

Remove NewNodeIDFromRelativeBigInt

c64bbdf

Use NewFromPrefixSuffix

dc2adc4

Define short and long tests

f025f06

remove extra logging

6bd4d89

reapply subtree cache fix for roots

57d04a7

gdbelvin force-pushed the map_clean branch from acf6f4d to 57d04a7 Compare August 3, 2017 16:27

Add error tests to ParseSuffix, reviewer comments

f2c5b37

Martin2112 approved these changes Aug 4, 2017

View reviewed changes

gdbelvin merged commit 0ef00b0 into google:master Aug 4, 2017

gdbelvin deleted the map_clean branch August 4, 2017 12:31

gdbelvin mentioned this pull request Aug 9, 2017

Map: CONIKS map hasher #670

Closed

Conversation

gdbelvin commented Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Martin2112 commented Jul 31, 2017

Uh oh!

gdbelvin commented Jul 31, 2017

Uh oh!

Martin2112 commented Jul 31, 2017

Uh oh!

gdbelvin commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Martin2112 commented Jul 31, 2017

Uh oh!

gdbelvin commented Jul 31, 2017

Uh oh!

Martin2112 commented Aug 3, 2017

Uh oh!

gdbelvin commented Aug 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdbelvin commented Aug 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gdbelvin commented Jul 28, 2017 •

edited

Loading

gdbelvin commented Jul 31, 2017 •

edited

Loading