Implement go bindings to secp256k1 #1

elichai · 2020-02-24T11:50:17Z

Hi,
These are go bindings for the https://github.com/kaspanet/secp256k1 repository.
the repo is included as a subtree with the script ./depend/update-subtree.sh to pull updates from the subtree.

There are currently some integration problems into kaspad, mostly because kaspad does utxo multiset diffs. which aren't supported by the code, but this should probably be changed in kaspad if I understand things correctly.

As usual review is done best commit by commit :)

git-subtree-dir: depend/secp256k1 git-subtree-split: ee3ab072d625da9f3418f94c62ef9dcc1ff1be0c

…cp256k1'

svarogg

Please do go mod init.

Than run golint and go vet and fix all warnings they output (note: In some rare cases go vet might have false positives. This is not something we have every encountered, but unlike with golint there's no guarantee against them. If you think it does output any false positives, let's discuss it)

multiset.go

svarogg · 2020-02-25T13:19:10Z

multiset.go

+func (multiset *MultiSet) Reset() {
+	ret := C.secp256k1_multiset_init(context, &multiset.set)
+	if ret != 1 {
+		panic("failed intializing a multiset. should never happen")


This is reset, not initialize (I guess copy-paste error?)

multiset.go

svarogg · 2020-02-25T13:25:31Z

multiset_test.go

+	for _, test := range testVectors {
+		data, err := hex.DecodeString(test.dataElementHex)
+		if err != nil {
+			t.Fatal(err)


In all Fatals - please include a short message explaining where the failure happened.
We have stack traces, but it's still very convenient when there's a short textual explanation on what has happened.

Note: good chance that after you execute the comment a few lines above, this will already be obsolete.

sure. FYI these are a copy-paste from: https://github.com/kaspanet/kaspad/blob/master/ecc/ecmh_test.go#L39

svarogg · 2020-02-25T13:27:12Z

multiset_test.go

+	}
+	m1.Combine(m2)
+	if m1.Finalize() != zeroHash {
+		t.Fatalf("m1 was expected to return to have zero hash, but was %s instead", m1.Finalize())


Grammar: to return to have

+1 (FYI https://github.com/kaspanet/kaspad/blob/master/ecc/ecmh_test.go#L117 ;) )

svarogg · 2020-02-25T13:51:52Z

oldschnorr.go

+func (key *SchnorrPublicKey) Negate() {
+	ret := C.secp256k1_ec_pubkey_negate(C.secp256k1_context_no_precomp, &key.pubkey)
+	if ret != 1 {
+		panic("Failed Negating the public key. should never happen")


Error messages shouldn't start with capital letters

svarogg · 2020-02-25T13:53:23Z

secp256k1.go

@@ -0,0 +1,92 @@
+package secp256k1
+
+// // **This is CGO's build system. yes, in comments.**


Complaining about the language's quirks in the code is very understandable, but undesirable

It's both complaining and being highly explicit that these are not comments. suggested language?

How about something like:
"CGO defs. CGO parses the following comments as build instructions."

svarogg · 2020-02-25T13:54:49Z

secp256k1.go

+	}
+}
+
+type PrivateKey struct {


type PrivateKey [32]byte is much more concise and nicer to use

Also, it might be a good idea to move PrivateKey/PublicKey and all related methods to a separate file/s.

Didn't known that type PrivateKey [32]byte is a thing.

Seems like you missed fixing these comments.

I think it's better to not let users access the private key directly like that, the struct adds a visibility barrier (with no cost I assume).

And this actually increase the line count, because it requires me to assign temporaries before returning(because you can't take the address of an array without assigning it)

svarogg · 2020-02-25T13:57:20Z

secp256k1_test.go

+	"testing"
+)
+
+// TODO: Add test vectors. (even though they're included in libsecp itself)


Here too: no TODOs in the code, create a ticket instead

svarogg · 2020-02-25T13:59:18Z

secp256k1_test.go

+
+const LoopsN = 150
+
+var Secp256k1Order, _ = new(big.Int).SetString("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141", 16)


Don't ignore errors.

You can do initialization logic in TestMain instead.

What I'd do is have the string representation as a const, than have TestMain parse it and assign global var Secp256k1Order

What do you think about that vs doing this?
var Secp256k1Order = new(big.Int).SetBytes([]byte{255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 254, 186, 174, 220, 230, 175, 72, 160, 59, 191, 210, 94, 140, 208, 54, 65, 65})

This is way longer and not readable, but it skips the need for TestMain

I guess this can work. It is indeed ugly and not readible, but A) this is a test, so not as critical. and B) it's not as though the hexa was in any way more readable, just shorter.

svarogg

Another note about govet/golint: we can help setup Jenkins to run this automatically on PRs, to make sure we are always clear.

depend/update-subtree.sh

stasatdaglabs · 2020-02-26T09:23:17Z

secp256k1.go

@@ -0,0 +1,92 @@
+package secp256k1
+
+// // **This is CGO's build system. yes, in comments.**


How about something like:
"CGO defs. CGO parses the following comments as build instructions."

stasatdaglabs · 2020-02-26T09:31:22Z

oldschnorr.go

+}
+
+func isZeroed(slice []C.uchar) bool {
+	for _, v := range slice {


Please avoid one-character names. "v" makes readers needlessly stop to think "wait, what's v? Is it something special?" for a moment before realizing that "oh yeah, it's just value"

stasatdaglabs · 2020-02-26T09:34:42Z

secp256k1_test.go

+
+// TODO: Add test vectors. (even though they're included in libsecp itself)
+
+const LoopsN = 150


The name LoopsN is a bit unclear. Please either rename it or add a short explanation.

Also, does LoopsN need to be exported? If you aren't using it anywhere outside of this package, please unexport it.

It's a test variable for how many times to run the loops in the tests. it's used both in secp256k1_test and in multiset_test, I can put different ones in each file though

Unlike Rust, files don't define scope. Meaning that since both files are inside the package "secp256k1" if you unexport LoopsN here it will still be available in multiset_test.go.

stasatdaglabs · 2020-02-26T10:00:40Z

secp256k1_test.go

+	}
+}
+
+func TestPrivateKey_Add_fail(t *testing.T) {


Please capitalize the f in fail.

stasatdaglabs · 2020-02-26T10:09:56Z

secp256k1_test.go

+		t.Errorf("A zeroed key is invalid")
+	}
+	if err4 != nil || err5 != nil {
+		t.Errorf("It should be possible to add zero to a key")


Please check each error individually and add it to the error string here and everywhere. That way when the test fails we'll know exactly and immediately where it fails instead of having to connect a debugger.

stasatdaglabs · 2020-02-26T10:11:45Z

secp256k1_test.go

+		copy(msg32[:], test.message)
+		valid := pubkey.SchnorrVerify(msg32, sig)
+		if valid != test.valid {
+			t.Errorf("Schnorr test vector %d didn't produce correct result", i)


Please specify in the error what was the result and what was the expected result.

stasatdaglabs · 2020-02-26T10:12:31Z

secp256k1_test.go

+		msg := [32]byte{}
+		n, err := rand.Read(msg[:])
+		if err != nil || n != 32 {
+			panic("benchmark failed")


Please add the error to the string.

stasatdaglabs · 2020-02-26T10:15:52Z

multiset_test.go

+	copy(data[:], Secp256k1Order.Bytes())
+	_, err := ParseMultiSet(data)
+	if err == nil {
+		t.Errorf("Shouldn't be able to parse a multiset bigger with x bigger than the field size")


Please add the error itself to the string.

stasatdaglabs · 2020-02-26T10:20:55Z

multiset_test.go

+		list[i] = data
+	}
+	if set.Finalize() == set2.Finalize() {
+		t.Errorf("sets are the same when they should be different: set %x\n", set.Finalize())


Please print both sets.

The if only executes if they're the same though

Oops, yeah, my bad.

elichai · 2020-02-26T16:06:23Z

I think I applied all your comments, you can review the diff here: https://github.com/kaspanet/go-secp256k1/compare/8cf663fb9f1b1d9b47af041bb487eae97c57ffe4..bca4c0d5a9eae14ddd24f4638bf83271df464495

stasatdaglabs · 2020-02-27T11:26:27Z

multiset_test.go

+		data := [100]byte{}
+		n, err := rand.Read(data[:])
+		if err != nil || n != len(data) {
+			t.Fatalf("Failed generating random data ''%v'  ''%d' ", err, n)


Please switch the order of err and n, and annotate n. Likewise in other tests.

stasatdaglabs · 2020-02-27T11:28:35Z

multiset_test.go

+		list[i] = data
+	}
+	if set.Finalize() == set2.Finalize() {
+		t.Errorf("sets are the same when they should be different: set %x\n", set.Finalize())


Oops, yeah, my bad.

stasatdaglabs · 2020-02-27T11:29:25Z

multiset_test.go

+	}
+	parsedSet, err := ParseMultiSet(serializedEmpty)
+	if err != nil {
+		t.Errorf("error: '%v' happened when parsing: ''%x'", err, serializedEmpty)


Please put the error at the end of the string. This is because the error string might be quite long.

Good idea. Thanks.

multiset.go

svarogg · 2020-03-01T14:44:16Z

multiset.go

+// which is a rolling(homomorphic) hash that you can add and remove elements from
+// and receiving the same resulting hash as-if you never hashed that element.
+// Because of that the order of adding and removing elements doesn't matter.
+// The type should only be created via the supplied methods.


I'd revise to something like "Use NewMultiset to initialize a MultiSet"

multiset.go

svarogg · 2020-03-01T15:28:47Z

multiset_test.go

+			panic("bad benchmark")
+		}
+	}
+	if set == NewMultiset() { // To prevent optimizing out the loop


Is this a real scenario that really happens?

If yes, please note that once you do that NewMultiset returns a pointer, you might had this functionality broken.

I honestly don't know :)
I know this is in fact a problem I've encountered many times in C/C++ and Rust, don't know how aggressive is Go's optimizer and if it gives special treatment for benchmarks

Example: bitcoin-core/secp256k1#667 bitcoin-core/secp256k1#678

Hmmmmm... I really doubt that's going to happen.
The benchmark loop is a standard golang construct. I have written a ton of benchmark this way and never encountered a similar problem.

I'd rather remove this piece of code, and re-introduce only if we find it's needed.

oldschnorr.go

svarogg · 2020-03-01T15:42:53Z

oldschnorr.go

+
+// SchnorrVerify verifies a schnorr signature using the public key and the input hashed message.
+// Notice: the [32] byte array *MUST* be a hash of a message you hashed yourself.
+func (key *SchnorrPublicKey) SchnorrVerify(hash [32]byte, signature SchnorrSignature) bool {


Please add type Hash [32]byte and use pointer to it instead of direct [32]byte.
Also use it wherever ecmh returns a hash, and define similar type for ecmh's [64]byte serialization format.

What does that help/give?

Than you will be able to pass by reference easily.

oldschnorr.go

svarogg · 2020-03-01T15:49:09Z

secp256k1.go

+	}
+}
+
+type PrivateKey struct {


Seems like you missed fixing these comments.

svarogg · 2020-03-22T11:03:03Z

multiset.go

+
+// NewMultiset return an empty initialized set.
+// when finalized it should be equal to a finalized set with all elements removed.
+func NewMultiset() MultiSet {


It should always be a pointer except for simple types (ints, bools, etc) or types that are reference-types already (strings, maps, slices, interfaces).
In other words - if it's a struct, you should probably pass it around as a pointer.

Indeed, in some cases deep examination would prove that some structs are better of passed-by-value. However, these are the kinds of micro-optimizations that we would rather refrain from, for the sake of simplicity.

That said, in cases where there's a proven bottleneck that would benefit a lot from passing something by-value, no doubt we would consider doing it. Nevertheless - the default go-to should be pass-by-pointer.

svarogg · 2020-03-22T11:08:11Z

multiset.go

+
+// NewMultiset return an empty initialized set.
+// when finalized it should be equal to a finalized set with all elements removed.
+func NewMultiset() MultiSet {


Also note that all MultiSet functions have a pointer to MultiSet as the receiver.
If you wanted MultiSet to be a value-type for some reason, you would also want it's methods to have a value receiver)

svarogg · 2020-03-22T11:11:48Z

multiset.go

+	}
+}
+
+// Add a data element to the multiset. This will hash the data onto the curve and add it.


Comments in go should be in the form "[Function name] [does blablabla]".
Yes, what you did satisfies the linter, because comment indeed starts with the name of the function, but it misses the point.

Please fix in all the codebase

Are the new ones ok? not the best in wording :)

The new ones are looking fine. Wording can be always improved, but it is absolutely fine.

multiset.go

multiset_test.go

svarogg · 2020-03-22T13:03:57Z

multiset_test.go

+			panic("bad benchmark")
+		}
+	}
+	if set == NewMultiset() { // To prevent optimizing out the loop


Hmmmmm... I really doubt that's going to happen.
The benchmark loop is a standard golang construct. I have written a ton of benchmark this way and never encountered a similar problem.

I'd rather remove this piece of code, and re-introduce only if we find it's needed.

svarogg · 2020-03-22T13:48:09Z

secp256k1.go

+	return key.privateKey
+}
+
+// Negate a private key in place.


Why is Negate needed?

Asking more out of curiosity than anything else

I thought it was needed for BIP32 but I think that's not true.
But I guess there are other more complex applications that require this bitcoin-core/secp256k1#408

secp256k1_test.go

oldschnorr.go

svarogg · 2020-03-22T14:13:54Z

secp256k1_test.go

+
+const LoopsN = 150
+
+var Secp256k1Order, _ = new(big.Int).SetString("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141", 16)


I guess this can work. It is indeed ugly and not readible, but A) this is a test, so not as critical. and B) it's not as though the hexa was in any way more readable, just shorter.

svarogg · 2020-03-22T14:33:53Z

oldschnorr.go

+// DeserializeSchnorrSignature deserializes a 64 byte serialized schnorr signature into a SchnorrSignature type.
+func DeserializeSchnorrSignature(serializedSignature [64]byte) SchnorrSignature {
+	signature := SchnorrSignature{}
+	signature.signature = serializedSignature


You can write this whole function in one line return SchnorrSignature{signature:serializedSignature}

oldschnorr.go

elichai · 2020-03-22T19:51:58Z

Should I just implement IsEqual(), Clone() and String() on all the structs?

svarogg · 2020-03-23T10:24:54Z

Should I just implement

svarogg · 2020-03-23T10:29:00Z

Should I just implement IsEqual(), Clone() and String() on all the structs?

It is redundant in many cases, but for hardcore-library-level code that you are writing:
String() - in 99% cases yes
IsEqual() - in all cases that comparing makes sense
Clone() - only for objects where cloning is something that will happen

elichai added 4 commits February 20, 2020 18:53

Add script to pull secp256k1 subtree

61b0758

Squashed 'depend/secp256k1/' content from commit ee3ab07

e16d7d7

git-subtree-dir: depend/secp256k1 git-subtree-split: ee3ab072d625da9f3418f94c62ef9dcc1ff1be0c

Merge commit 'e16d7d7dadbfa60149cb6f8f35858846fffa4b1f' as 'depend/se…

ddfd306

…cp256k1'

Add compile instructions in Go for libsecp256k1

656d365

elichai requested a review from stasatdaglabs February 24, 2020 11:50

stasatdaglabs requested a review from svarogg February 25, 2020 12:24

svarogg reviewed Feb 25, 2020

View reviewed changes

svarogg requested changes Feb 25, 2020

View reviewed changes

stasatdaglabs requested changes Feb 26, 2020

View reviewed changes

elichai force-pushed the subtree branch from 8cf663f to bca4c0d Compare February 26, 2020 16:03

stasatdaglabs reviewed Feb 27, 2020

View reviewed changes

stasatdaglabs requested changes Feb 27, 2020

View reviewed changes

elichai force-pushed the subtree branch 2 times, most recently from a20b377 to 41cded5 Compare February 27, 2020 12:59

stasatdaglabs approved these changes Mar 1, 2020

View reviewed changes

svarogg requested changes Mar 1, 2020

View reviewed changes

elichai force-pushed the subtree branch from 41cded5 to 4974405 Compare March 1, 2020 17:00

elichai force-pushed the subtree branch from 4974405 to b266fa3 Compare March 22, 2020 10:33

svarogg reviewed Mar 22, 2020

View reviewed changes

svarogg requested changes Mar 22, 2020

View reviewed changes

elichai force-pushed the subtree branch from b266fa3 to b19d7be Compare March 22, 2020 19:49

elichai commented Mar 22, 2020

View reviewed changes

oldschnorr.go Outdated Show resolved Hide resolved

elichai force-pushed the subtree branch from b19d7be to 5141566 Compare March 22, 2020 20:03

svarogg closed this Mar 23, 2020

svarogg reopened this Mar 23, 2020

elichai force-pushed the subtree branch from 5141566 to c4f4086 Compare March 23, 2020 12:09

elichai added 6 commits March 23, 2020 14:12

Implement global context and private key related functions

9601edc

Implement bindings to oldschnorr signature and PublicKey

2964dce

Add tests for oldschnorr and general privatekey operations

1882032

Add bindings to multiset

b3f4fe0

Add tests for the multiset

efd56bb

Add go.mod and document the update-subtree.sh script

cf2c103

elichai force-pushed the subtree branch from c4f4086 to cf2c103 Compare March 23, 2020 12:13

svarogg approved these changes Mar 24, 2020

View reviewed changes

svarogg merged commit 5bc7b08 into master Mar 24, 2020

elichai deleted the subtree branch March 24, 2020 12:16

		@@ -0,0 +1,92 @@
		package secp256k1

		// // This is CGO's build system. yes, in comments.


		const LoopsN = 150

		var Secp256k1Order, _ = new(big.Int).SetString("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141", 16)


		// TODO: Add test vectors. (even though they're included in libsecp itself)

		const LoopsN = 150

Implement go bindings to secp256k1 #1

Implement go bindings to secp256k1 #1

Conversation

elichai commented Feb 24, 2020

svarogg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichai Feb 25, 2020 • edited Loading

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

svarogg 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

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

elichai commented Feb 26, 2020

stasatdaglabs Feb 27, 2020 • edited Loading

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

Choose a reason for hiding this comment

elichai commented Mar 22, 2020 • edited Loading

svarogg commented Mar 23, 2020

svarogg commented Mar 23, 2020

elichai Feb 25, 2020 •

edited

Loading

stasatdaglabs Feb 27, 2020 •

edited

Loading

elichai commented Mar 22, 2020 •

edited

Loading