Introduce composite key to delegate features to each function key by torao · Pull Request #101 · Finschia/ostracon

torao · 2020-07-03T14:42:49Z

Closes: #95

Summary

Added BLS and Composite key definitions.
- Changed the default key generation to Composite (BLS for signature and Ed25519 for VRF).
- Added definitions to serialize BLS and Composite key to Amino and ProtocolBuffers.
- Added methods to the PrivKey and PubKey interfaces to abstract VRF proving and verifying operation.
  - Move Proof and Output from vrf to crypto to avoid import circular references.
  - Added default implementations of those methods to existing ECDSA, Ed25519, Sr25519, and MultisigThreshold keys.
- Changed the size of keys, signatures, and blocks that were hard-corded as expectations in the test.
- Changed from a Ed25519-fixed part to a Composite key (maybe test only).
- Added build tasks of the BLS native library to make it work properly on the target environment, Alpine.
  - CircleCI, Github workflow, Makefile, and Dockerfile.

Description

Introduces the BLS and Composite keys into the LINE Blockchain. This PR enables the BLS signature on the LINE Blockchain (VRF functionality remains with the Ed25519 key). It also enables the Composite key to differentiate between BSL and Ed25519 keys depending on their function.

Please note that BLS signature aggregation is a modification of the following.

For contributor use:

Wrote tests
Updated CHANGELOG_PENDING.md
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Updated relevant documentation (docs/) and code comments
Re-reviewed Files changed in the Github PR explorer

zemyblue · 2020-07-10T06:08:45Z

crypto/crypto.go

+	Identity PrivKey `json:"identity"`
+	SignKey  PrivKey `json:"sign"`
+	VrfKey   PrivKey `json:"vrf"`


Do we need three keys?
As I said in the previous meeting, we're not going to use HSM, so I think we'll only need 2 keys.
And three keys are too many.

Fixed VRF key to be the role of Identity.

zemyblue · 2020-07-10T06:26:32Z

crypto/crypto.go

+func (pk PubKeyComposite) Validate() bool {
+	msg := b.NewBuffer(pk.SignKey.Bytes())
+	msg.Write(pk.VrfKey.Bytes())
+	return pk.Identity.VerifyBytes(msg.Bytes(), pk.Seal) &&


Is it works well?
Because the msg to be made with SignKey's PubKey byte and VrfKey's Pubkey byte is difference the msg to be made with SIgnKey's PrivKey bytes and VrfKey's PrivKey bytes.

(SignKey PubKe bytes + VrfKey PubKey bytes) != (SignKey PrivKey bytes + VrfKey PrivKey bytes)

The place to create the msg to generate the seal is is https://github.com/line/tendermint/pull/101/files#diff-31c4aa3a4249d4755fc652d3e0087b98R121-R122

The source is out of date, but this part was a bug. Now it doesn't use the seal (the signature of two-keys) and instead calculates its Address from the two-keys. Therefore, if one of the keys is tampered with, the address will always change.

zemyblue · 2020-07-10T07:25:44Z

I think it's better that PubKeyComposite and PrivKeyComposite in the crypto.go file move to composite directory under cryto directory. If then, we can resolve the import cycle problem.

torao · 2020-07-10T10:08:39Z

I was concerned about where to define this Composite key type, but I put it in crypto.go because it's not itself an implementation of a specific public key algorithm. However, the latest sources also seem to sub-package keys like multisig, similar to composite, so I'll do the same.

And there were two reasons for the cyclic import error. One will resolve by such a deployment, but another was due to the strong coupling between VRF and ed25519, which I solved both by interfacing the Proof and Output (thus, the current vrf probably should be placed under ed25519).

zemyblue · 2020-07-10T10:36:55Z

types/block.go

@@ -688,6 +687,8 @@ type Commit struct {
 	BlockID    BlockID     `json:"block_id"`
 	Signatures []CommitSig `json:"signatures"`


Do you remove signature of CommitSig later?

The CommitSig contains the address of "who signed" information, so I'll leave it in the block. I'm considering to set the Signature []byte field in the CommitSig to nil or byte[0]after aggregating the signatures.

I misunderstood a little bit; The CommitSig is not only a meta-information of the block, but also used by each Voter to send their signatures, so I'm considering to leave the Signature field in CommitSig.

I think it is better to define and use different types explicitly.
How about this

type Commit struct { Height int64 `json:"height"` Round int `json:"round"` BlockID BlockID `json:"block_id"` Signers []CommitSigner `json:"signers"` AggregatedSignature []byte `json:"aggregated_signature"` hash tmbytes.HexBytes bitArray *bits.BitArray } type CommitSigner struct { BlockIDFlag BlockIDFlag `json:"block_id_flag"` ValidatorAddress Address `json:"validator_address"` Timestamp time.Time `json:"timestamp"` }

torao · 2020-07-17T06:34:25Z

Here are the errors that are currently occurring in the CircleCI environment. The same errors will appear if you use docker on your Mac OS to run tendermint/localnet, build the binary in that container, and to execute.

init() in bls package is always successful. Therefore, it's not that libbls384_256.a cannot refer.
SIGTRAP is triggered in the C library called by PrivKeyBLS12.Sign(). But this doesn't always occur at the time of PrivKeyBLS12.Sign(), and often the signing is successful.

$ cd networks/local
$ make
$ cd ../..
$ docker run --rm -it -v `pwd`:/tendermint --entrypoint '' tendermint/localnode /bin/sh
/tendermint # apk add git gcc g++ go
/tendermint # make build
/tendermint # make test

...
fatal: morestack on g0
SIGTRAP: trace trap
PC=0x56054755e692 m=4 sigcode=128

goroutine 0 [idle]:
runtime.abort()
        /usr/lib/go/src/runtime/asm_amd64.s:859 +0x2
runtime.morestack()
        /usr/lib/go/src/runtime/asm_amd64.s:416 +0x22

goroutine 16 [syscall]:
runtime.cgocall(0x560547863df0, 0xc00003cda0, 0xc00001ae40)
        /usr/lib/go/src/runtime/cgocall.go:128 +0x5d fp=0xc00003cd70 sp=0xc00003cd38 pc=0x560547502edd
github.com/herumi/bls-eth-go-binary/bls._Cfunc_blsSign(0xc00010cea0, 0xc000019140, 0xc00001ae40, 0xc)
        _cgo_gotypes.go:815 +0x47 fp=0xc00003cda0 sp=0xc00003cd70 pc=0x56054765d7d7
github.com/herumi/bls-eth-go-binary/bls.(*SecretKey).SignByte.func1(0xc00003ce40, 0xc000019140, 0xc00003ce28)
        /root/go/pkg/mod/github.com/herumi/bls-eth-go-binary@v0.0.0-20200522010937-01d282b5380b/bls/bls.go:691 +0x107 fp=0xc00003cdf8 sp=0xc00003cda0 pc=0x560547663787
...
FAIL    github.com/tendermint/tendermint/crypto/bls     0.484s
...

…tion

torao · 2020-08-07T06:20:25Z

I'll write down the problems that took a long time to solve.

A problem with multi-node tests (localnet, contract_test) that caused the test to fail due to a SIGTRAP issue was avoided by building a BLS native library on Alpine Docker container with nodes running. The cause is unclear, but it seems to be a conflict between Alpine and BLS pre-compiled binary, as this doesn't occur in Debian even in the same Docker container environment.
The actions/cache@v1 was used in test_abci_cli on Github workflow. However, the fix was not tested correctly because the cache-key was unchanged so previously built binaries were restored. This has been fixed to rebuild the cache by adding a hash value with hashFiles() to the key. This problem has existed since Tendermint release v0.33.4 and is not fixed in the latest v0.33.7. However, in the master branch, test_abci_cli itself has been largely fixed.

zemyblue · 2020-08-07T10:14:48Z

abci/example/kvstore/persistent_kvstore.go


 	// update
-	return app.updateValidator(types.Ed25519ValidatorUpdate(pubkey, power))
+	// TODO The type of public key to be restored should be kept its original type.


Do you have any plan to change the type of public key later?
Because you point it as TODO.

This TODO is a comment intended for #107. It'll be fixed when #107 fixed.

I fixed the relevant part, but since there is no NewValidatorUpdate in the develop branch, the PR of 107 needs to wait for the source code of 101 to be merged.

zemyblue · 2020-08-07T10:21:29Z

abci/tests/server/client.go

+		pubkey := tmtypes.TM2PB.PubKey(pk).Data
 		power := tmrand.Int()
-		vals[i] = types.Ed25519ValidatorUpdate(pubkey, int64(power))
+		vals[i] = types.NewValidatorUpdate("composite", pubkey, int64(power))


I know ABCIPubKeyTypeComposite is same string const type with "composite".
So, I think it's the better to use ABCIPubKeyTypeComposite if possible.

Exactly, I'll fix it.

zemyblue · 2020-08-07T10:23:17Z

cmd/contract_tests/main.go

 	"fmt"
 	"strings"

+	"github.com/tendermint/tendermint/cmd/contract_tests/unmarshaler"


Do you know why this import sorting is changed?
lint version is changed? or golang version is changed?

Do different versions of goimports give different results? I'm not sure why it was corrected, but this is the result of applying make format (gofmt) locally.

% go version go version go1.14.4 darwin/amd64

zemyblue · 2020-08-07T11:11:53Z

crypto/encoding/codec_test.go

+	if !cpk.Equals(pk) {
+		t.Fatalf("The retrieved composite public key was not match: %+v != %+v", cpk, pk)
+	}
+}


How about adding more test for BLS12?

I'll fix it.

zemyblue · 2020-08-07T11:18:28Z

types/priv_validator.go

+	//signKey := bls.GenPrivKey()
+	//vrfKey := ed25519.GenPrivKey()
+	//return composite.NewPrivKeyComposite(signKey, vrfKey).PubKey(), nil


If this lines don't need anymore, please remove this comment.

egonspace · 2020-08-10T02:57:13Z

abci/example/kvstore/helpers.go

 // from the input value
 func RandVal(i int) types.ValidatorUpdate {
-	pubkey := tmrand.Bytes(32)
+	pk := composite.GenPrivKey().PubKey()


I think choosing which key to use should be handled in one place. The same is true in codes for testing.

I've defined another function that generates a private key for the example and tests.

egonspace · 2020-08-11T00:39:40Z

crypto/vrf/vrf.go

 type vrfEd25519 interface {
-	Prove(privateKey ed25519.PrivKeyEd25519, message []byte) (Proof, error)
-	Verify(publicKey ed25519.PubKeyEd25519, proof Proof, message []byte) (bool, error)
+	Prove(privateKey []byte, publicKey []byte, message []byte) (Proof, error)


Can I ask the reason why you modified more specific parameter type(ed25519.PrivKeyEd25519) to general type([]byte)? I think it's better that vrfEd25519 interface methods use the type concerned with Ed25519.

This is a fix intended to eliminate circular references in import: since crypto/ed25519 must reference crypt/vrf, a reference to crypto/ed25519 in crypt/vrf will cause a circular reference. Indeed, passing the Ed25519 key type is better abstraction and natural, but we have to give preference to one over the other.

If golang supported type-class or generics, there would be a better way to write this, but for now, we use the generic notation as byte[].

Okay I see.

egonspace

LGTM

torao added C: enhancement Classification: New feature or its request, or improvement in maintainability of code WIP labels Jul 3, 2020

torao self-assigned this Jul 3, 2020

torao force-pushed the feature/signature_aggregation branch 3 times, most recently from 4edece2 to e6d875b Compare July 6, 2020 07:56

torao changed the base branch from master to develop July 6, 2020 07:58

torao force-pushed the feature/signature_aggregation branch 11 times, most recently from 477d819 to b6715d4 Compare July 9, 2020 12:27

zemyblue reviewed Jul 10, 2020

View reviewed changes

torao force-pushed the feature/signature_aggregation branch from d2a2bbb to 9499629 Compare July 10, 2020 09:30

torao force-pushed the feature/signature_aggregation branch from a28dab3 to f8d41a8 Compare July 10, 2020 10:35

zemyblue reviewed Jul 10, 2020

View reviewed changes

torao force-pushed the feature/signature_aggregation branch from 0f3c118 to 642e48c Compare July 16, 2020 06:03

torao mentioned this pull request Jul 16, 2020

Missing public-key type in sending ValidatorUpdate Tx #107

Closed

torao force-pushed the feature/signature_aggregation branch 2 times, most recently from 6a504de to f72afd4 Compare July 17, 2020 04:51

torao force-pushed the feature/signature_aggregation branch 10 times, most recently from 8eda5fc to 2838143 Compare August 6, 2020 08:18

introduced a composite-key that delegates processing to each key-func…

6994d02

…tion

torao force-pushed the feature/signature_aggregation branch from 2838143 to 6994d02 Compare August 7, 2020 05:45

torao changed the title ~~WIP: Introduce composite key to delegate features to each function key~~ Introduce composite key to delegate features to each function key Aug 7, 2020

torao removed the WIP label Aug 7, 2020

torao requested review from egonspace, shiki-tak and zemyblue August 7, 2020 06:04

zemyblue reviewed Aug 7, 2020

View reviewed changes

egonspace reviewed Aug 10, 2020

View reviewed changes

egonspace reviewed Aug 11, 2020

View reviewed changes

torao force-pushed the feature/signature_aggregation branch from 70cead1 to 0b9f890 Compare August 13, 2020 09:31

fix the points made in the review

c6e66bc

torao force-pushed the feature/signature_aggregation branch from 0b9f890 to c6e66bc Compare August 13, 2020 09:56

egonspace approved these changes Aug 14, 2020

View reviewed changes

tnasu approved these changes Aug 14, 2020

View reviewed changes

torao merged commit 40a2944 into develop Aug 17, 2020

torao mentioned this pull request Sep 3, 2020

BLS Signature Aggregation and Verification #117

Merged

5 tasks

tnasu deleted the feature/signature_aggregation branch December 24, 2020 08:06

		@@ -688,6 +687,8 @@ type Commit struct {
		BlockID BlockID `json:"block_id"`
		Signatures []CommitSig `json:"signatures"`

Conversation

torao commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Description

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!

zemyblue commented Jul 10, 2020

Uh oh!

torao commented Jul 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torao Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torao commented Jul 17, 2020

Uh oh!

torao commented Aug 7, 2020

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!

egonspace Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

egonspace left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

torao commented Jul 3, 2020 •

edited

Loading

torao Jul 10, 2020 •

edited

Loading

egonspace Aug 11, 2020 •

edited

Loading