Compact Precise Proofs #743

mikiquantum · 2019-02-12T16:00:14Z

Issue Link:

…mpact

mikiquantum · 2019-02-12T16:00:30Z

coredocument/coredocument.go

@@ -16,6 +16,41 @@ import (
 	"github.com/ethereum/go-ethereum/common/hexutil"
 )

+const (


Disregard this file.

mikiquantum · 2019-02-12T16:00:44Z

coredocument/coredocument_test.go

@@ -215,10 +215,11 @@ func TestNewWithCollaborators(t *testing.T) {



Disregard this file.

mikiquantum · 2019-02-12T16:02:10Z

documents/model.go

@@ -18,6 +18,22 @@ import (
 	"github.com/ethereum/go-ethereum/common/hexutil"
 )

+const (


As sometimes we create the tree manually, we have to provide a deterministic compact name to each of the fields. Some of them might have a correspondent field in the coredocument and some dont. This keeps a map of the literal name and the compact name.

Not an immediate action item, but we have to make sure that we don't accidentally create two fields with the same property (one in invoice and one in CoreDoc for example).

We could achieve this using reserved statements and putting field numbers into there for the fields of the other documents.

message Foo { reserved 2, 15, 9 to 11; reserved "foo", "bar"; }

Yes, agree the initial definition of the protocol documents should have that defined upfront. In this particular case, for low level general fields, I would argue to have those fields defined in the coredoc proto message, even though they are calculated every time.

mikiquantum · 2019-02-12T16:02:39Z

documents/model.go

@@ -53,6 +69,27 @@ const (
 	ErrZeroCollaborators = errors.Error("require at least one collaborator")
 )

+// NewDefaultTree returns a DocumentTree with default opts
+func NewDefaultTree(salts *proofs.Salts) *proofs.DocumentTree {


I wanted to enforce a single way of creating a tree with the supported defaults. Here is the methods we should be using.

mikiquantum · 2019-02-12T16:03:06Z

documents/model.go

+}
+
+// NewLeafProperty returns a proof property with the literal and the compact
+func NewLeafProperty(literal string, compact []byte) proofs.Property {


Same for creating custom properties, I wanted to enforce that we always pass the compact name as well

mikiquantum · 2019-02-12T16:03:42Z

documents/model_test.go

@@ -287,18 +287,20 @@ func TestGetDocumentRootTree(t *testing.T) {
 	tree, err := dm.GetDocumentRootTree()

 	// Manually constructing the two node tree:
-	signaturesLengthLeaf := sha256.Sum256(append([]byte("signatures.length0"), make([]byte, 32)...))
-	expectedRootHash := sha256.Sum256(append(signaturesLengthLeaf[:], dm.Document.SigningRoot...))
+	signaturesLengthLeaf := sha256.Sum256(append(append(compactProperties[SignaturesField], []byte{48}...), make([]byte, 32)...))


Looks like with the new precise proof update the order of hashing leaves has changed.

But why did you say the order has changed?

It's property, value, salt. Was it different before?

Actually []byte{48} -> seems to be the wrong length value here, that to me looks like an ASCII encoded "SPACE" character and not a 0. Though this will be fixed when changing the way we generate the signatures. So not worth fixing I think.

within leaves, not within a leaf. Not sure why but if you look at the next line 291 I had to change the order.
Regarding the ascii code, int(48) is 0 in ASCII, as the node logic for length fields does: []byte("0"). I could transform that to encode an int in bytes today.

Actually this should be addressed once we create the signatures as a tree soon.

lucasvo

None of the changes need to be addressed in this PR but definitely need to be fixed pretty soon I think in follow up work.

lucasvo · 2019-02-12T22:06:02Z

documents/model.go

@@ -18,6 +18,22 @@ import (
 	"github.com/ethereum/go-ethereum/common/hexutil"
 )

+const (


Not an immediate action item, but we have to make sure that we don't accidentally create two fields with the same property (one in invoice and one in CoreDoc for example).

We could achieve this using reserved statements and putting field numbers into there for the fields of the other documents.

message Foo { reserved 2, 15, 9 to 11; reserved "foo", "bar"; }

lucasvo · 2019-02-12T22:13:11Z

coredocument/coredocument.go

+}
+
+func NewDefaultTreeWithPrefix(salts *proofs.Salts, prefix string) *proofs.DocumentTree {
+	var prop proofs.Property


Technically we also have to prefix the compact version of the properties so we'll need to have a byte and string prefix. I think it's fine to implement this in a later PR but it's something we should think about. Perhaps we actually can pass in a Property as a prefix instead.

True, I will add another entry in the map for our prefixes used as a "reserved" prefix.

The lengthProp method needs to add as well the compact property for length: https://github.com/centrifuge/precise-proofs/blob/13d3af957299c614237c42cdc331f7acd7c7d201/proofs/property.go#L127-L133

lucasvo · 2019-02-12T22:22:16Z

documents/model_test.go

@@ -287,18 +287,20 @@ func TestGetDocumentRootTree(t *testing.T) {
 	tree, err := dm.GetDocumentRootTree()

 	// Manually constructing the two node tree:
-	signaturesLengthLeaf := sha256.Sum256(append([]byte("signatures.length0"), make([]byte, 32)...))
-	expectedRootHash := sha256.Sum256(append(signaturesLengthLeaf[:], dm.Document.SigningRoot...))
+	signaturesLengthLeaf := sha256.Sum256(append(append(compactProperties[SignaturesField], []byte{48}...), make([]byte, 32)...))


But why did you say the order has changed?

lucasvo · 2019-02-12T22:24:30Z

documents/model_test.go

@@ -287,18 +287,20 @@ func TestGetDocumentRootTree(t *testing.T) {
 	tree, err := dm.GetDocumentRootTree()

 	// Manually constructing the two node tree:
-	signaturesLengthLeaf := sha256.Sum256(append([]byte("signatures.length0"), make([]byte, 32)...))
-	expectedRootHash := sha256.Sum256(append(signaturesLengthLeaf[:], dm.Document.SigningRoot...))
+	signaturesLengthLeaf := sha256.Sum256(append(append(compactProperties[SignaturesField], []byte{48}...), make([]byte, 32)...))


It's property, value, salt. Was it different before?

Actually []byte{48} -> seems to be the wrong length value here, that to me looks like an ASCII encoded "SPACE" character and not a 0. Though this will be fixed when changing the way we generate the signatures. So not worth fixing I think.

codecov · 2019-02-13T12:58:37Z

Codecov Report

Merging #743 into develop will increase coverage by 0.06%.
The diff coverage is 94%.

@@             Coverage Diff             @@
##           develop     #743      +/-   ##
===========================================
+ Coverage     52.2%   52.26%   +0.06%     
===========================================
  Files          119      119              
  Lines         9110     9216     +106     
===========================================
+ Hits          4756     4817      +61     
- Misses        3802     3842      +40     
- Partials       552      557       +5

Impacted Files	Coverage Δ
documents/invoice/model.go	`88.18% <100%> (-0.06%)`	⬇️
documents/handler.go	`85.45% <100%> (ø)`	⬆️
documents/purchaseorder/model.go	`83.41% <100%> (-0.08%)`	⬇️
coredocument/coredocument.go	`69.29% <89.47%> (-0.19%)`	⬇️
documents/model.go	`73.07% <95.83%> (+0.47%)`	⬆️
identity/did/service.go	`42.4% <0%> (-12.39%)`	⬇️
identity/did/key.go	`46.15% <0%> (-8.4%)`	⬇️
identity/did/factory.go	`45.33% <0%> (-7.7%)`	⬇️
bootstrap/bootstrappers/bootstrapper.go	`5.12% <0%> (-0.14%)`	⬇️
identity/identity.go	`87.69% <0%> (ø)`	⬆️
... and 1 more

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 5e3cb95...5bdb1fe. Read the comment docs.

mikiquantum added 5 commits February 11, 2019 18:56

latest precise proofs

6d86d19

latest precise proofs

d82b506

Merge branch 'pp' into compact

6657884

Merge branch 'develop' of github.com:centrifuge/go-centrifuge into co…

bae0c24

…mpact

using compact properties

4d4270d

mikiquantum commented Feb 12, 2019

View reviewed changes

mikiquantum requested review from vedhavyas and lucasvo February 12, 2019 16:04

lucasvo approved these changes Feb 12, 2019

View reviewed changes

mikiquantum added 3 commits February 13, 2019 11:24

added compact prefix

8e652ec

fix client test

1836b97

fix client test

5bdb1fe

mikiquantum merged commit 0956e7a into centrifuge:develop Feb 13, 2019

mikiquantum deleted the compact branch February 13, 2019 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compact Precise Proofs #743

Compact Precise Proofs #743

mikiquantum commented Feb 12, 2019

mikiquantum Feb 12, 2019

mikiquantum Feb 12, 2019

mikiquantum Feb 12, 2019

lucasvo Feb 12, 2019

mikiquantum Feb 13, 2019

mikiquantum Feb 12, 2019

mikiquantum Feb 12, 2019

mikiquantum Feb 12, 2019

lucasvo Feb 12, 2019

lucasvo Feb 12, 2019

mikiquantum Feb 13, 2019

mikiquantum Feb 13, 2019

lucasvo left a comment

lucasvo Feb 12, 2019

lucasvo Feb 12, 2019

mikiquantum Feb 13, 2019

mikiquantum Feb 13, 2019

lucasvo Feb 12, 2019

lucasvo Feb 12, 2019

codecov bot commented Feb 13, 2019

		@@ -215,10 +215,11 @@ func TestNewWithCollaborators(t *testing.T) {

Compact Precise Proofs #743

Compact Precise Proofs #743

Conversation

mikiquantum commented Feb 12, 2019

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

lucasvo 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

codecov bot commented Feb 13, 2019

Codecov Report