Make `bls.Signer` api fallible #3696

richardpringle · 2025-02-03T23:27:16Z

Make ProofOfPossession creation fallible
Make bls.Signer api fallible

Why this should be merged

If we want to add any networking in between the keys, we'll need a fallible API.

I've implemented the rpcsigner here as a grpc client with a server implemented in a _test.go file. The idea here is that anyone who implements the interface.

Open Question:

There's one little gotchya here for anyone implementing the interface. It's that we expect the keys and signatures to be compressed when sent over the wire. I'm open to using the more general serialization OR adding the compressed prefix to the proto-message field names.

TODO:

Further, we should probably add a minimal test suite that an external implementor can run, however, I think could probably be implemented in a follow up PR as it is not a priority at the moment.

How this works

How this was tested

Need to be documented in RELEASES.md?

utils/crypto/bls/signer.go

chains/manager.go

network/peer/ip.go

richardpringle · 2025-02-04T23:43:29Z

utils/crypto/bls/signer/rpcsigner/server_test.go

please help with naming!?

Shouldn't this just be server.go? I don't think this is a test file - this is the server implementation

I thought I already responded to this (maybe I forgot to submit the comment). My issue here is that I actually don't want people to use this. There's no benefit to running this on the same server vs running a local-signer and there's no auth mechanism implementation for running it on a separate server.

Maybe the mistake is testing the client at all?

Maybe we can just have a set of tests that implementors can run to make sure this works as expected? Curious to hear your thoughts

joshua-kim · 2025-02-05T18:43:32Z

network/peer/peer_test.go

+					BLSSignature: (func() *bls.Signature {
+						sig, err := blsKey.SignProofOfPossession([]byte("wrong message"))
+						require.NoError(t, err)
+						return sig
+					})(),


Suggested change

BLSSignature: (func() *bls.Signature {

sig, err := blsKey.SignProofOfPossession([]byte("wrong message"))

require.NoError(t, err)

return sig

})(),

BLSSignature: func() *bls.Signature {

sig, err := blsKey.SignProofOfPossession([]byte("wrong message"))

require.NoError(t, err)

return sig

}(),

We wrap this with () in a few spots in this PR, but I don't think it's necessary?

joshua-kim · 2025-02-05T18:45:21Z

node/node.go

+	if err != nil {
+		return err
+	}


I don't have a strong preference for wrapping this error or not, but I think we should be consistent w/ whatever we end up doing for this and the above error earlier in this diff

joshua-kim · 2025-02-05T18:48:23Z

proto/signer/signer.proto

+service Signer {
+  rpc PublicKey(PublicKeyRequest) returns (PublicKeyResponse) {}
+  rpc Sign(SignatureRequest) returns (SignatureResponse) {}
+  rpc ProofOfPossession(ProofOfPossessionSignatureRequest) returns (ProofOfPossessionSignatureResponse) {}


Maybe prefix this + the corresponding request/response types with Sign* to align this more closely with the existing Sign rpc, as they are similar apis with differing ciphertexts? i.e
rpc SignProofOfPossession(SignProofOfPossessionRequest) returns (SignProofOfPossessionResponse) {}?

joshua-kim · 2025-02-05T21:30:16Z

utils/crypto/bls/signer/localsigner/bls_test.go

+func collectN[T any](n int, fn func() T) []T {
+	result := make([]T, n)
+	for i := 0; i < n; i++ {
+		result[i] = fn()
+	}
+	return result
+}
+
+func mapWithError[T any, U any](arr []T, fn func(T) (U, error)) ([]U, error) {
+	result := make([]U, len(arr))
+	for i, val := range arr {
+		newVal, err := fn(val)
+		if err != nil {
+			return nil, err
+		}
+		result[i] = newVal
+	}
+	return result, nil
 }


I would probably not use these functions... the way we're using collectN I think is over-engineering/not improving the readability of the tests. mapWithError is more useful than collectN so if you want to keep it in I'm fine with it, but I don't think writing out:

sig1, err := sk1.Sign(msg) require.NoError(err) sig2, err := sk2.Sign(msg) require.NoError(err) sig3, err := sk3.Sign(msg) require.NoError(err) sigs := []*bls.SIgnature{sig1, sig2, sig3}

is actually that bad IMO.

joshua-kim · 2025-02-05T21:32:23Z

utils/crypto/bls/signer/localsigner/bls_test.go

+	wrongSig, err := signers[0].Sign(differentMsg)
+	require.NoError(err)
+
+	signatures[0] = wrongSig


This line is an example where I feel like the mapWithError util looks worse, instead of just writing out:

wrongSig, err := sk1.Sign(differentMsg) require.NoError(err) sig2, err := sk2.Sign(msg) require.NoError(err) sig3, err := sk3.Sign(msg) require.NoError(err) sigs := []*bls.SIgnature{wrongSig, sig2, sig3}

joshua-kim · 2025-02-05T23:14:11Z

utils/crypto/bls/signer/rpcsigner/bls_test.go

+	isValid := bls.Verify(signer.PublicKey(), sig, msg)
+	require.True(isValid)


This can be done in a one-liner

Generally Go uses the ok var name for bool return values

Suggested change

isValid := bls.Verify(signer.PublicKey(), sig, msg)

require.True(isValid)

require.True(bls.Verify(signer.PublicKey(), sig, msg))

joshua-kim · 2025-02-05T23:15:15Z

utils/crypto/bls/signer/rpcsigner/bls_test.go

+	msg := []byte("TestVerifyWrongMessageSignature local signer")
+	wrongMsg := []byte("TestVerifyWrongMessageSignature local signer with wrong message")


Nit: The payloads of these messages in this test file don't need to be related to the test name + why make them longer when the contents don't matter? Ex. my IDE will pick up on this + we have to keep the test names/strings consistent

Suggested change

msg := []byte("TestVerifyWrongMessageSignature local signer")

wrongMsg := []byte("TestVerifyWrongMessageSignature local signer with wrong message")

msg := []byte("foo")

wrongMsg := []byte("bar")

joshua-kim · 2025-02-05T23:17:51Z

utils/crypto/bls/signer/rpcsigner/bls_test.go

+	require.True(isValid)
+}
+
+func TestVerifyWrongMessageSignature(t *testing.T) {


This can be refactored w/ TestVerifyValidSignature into a test-table if we the expected message a parameter

Is it really worth combining two tests into a table test? The logic is different for each test. In the first test, we have a message stored at variable msg. We sign the message and pass that same message and signature for verification. In the second test we have two separate inputs. We sign msg again, but we pass wrongMsg in for signature verification. Therefore the logic for each of the two tests are subtly different.

I could make a table test (with only two tests) that has a msg field as well as a verificationMsg field, but now I have to either copy/paste the same thing into one of the test-structs or define a variable outside. I'm kind of shoehorning things to save a few lines of code.

joshua-kim · 2025-02-06T00:16:06Z

utils/crypto/bls/signer/rpcsigner/bls_test.go

+func TestValidAggregation(t *testing.T) {
+	require := require.New(t)
+	keyPairs := collectN(2, func() bls.Signer {
+		return NewLocalSigner(require)
+	})
+
+	keyPairs = append(keyPairs, NewLocalSigner(require))
+
+	msg := []byte("TestValidAggregation local signer")
+
+	sigs, err := mapWithError(keyPairs, func(s bls.Signer) (*bls.Signature, error) {
+		return s.Sign(msg)
+	})
+	require.NoError(err)
+
+	pks, _ := mapWithError(keyPairs, func(s bls.Signer) (*bls.PublicKey, error) {
+		return s.PublicKey(), nil
+	})
+
+	isValid, err := AggregateAndVerify(pks, sigs, msg)
+	require.NoError(err)
+	require.True(isValid)
+}
+
+func TestSingleKeyAggregation(t *testing.T) {
+	require := require.New(t)
+	signer := NewRpcSigner(require)
+
+	pks := []*bls.PublicKey{signer.PublicKey()}
+
+	msg := []byte("TestSingleKeyAggregation local signer")
+
+	sig, err := signer.Sign(msg)
+	require.NoError(err)
+
+	isValid, err := AggregateAndVerify(pks, []*bls.Signature{sig}, msg)
+	require.NoError(err)
+	require.True(isValid)
+}


Do we need tests on aggregation working? I feel like we already know aggregation will work because we are just implementing a type that produces bls.Signature and bls.Publickey, and those types + aggregation are already tested in the upstream package bls. I feel like coverage for rpcsigner only needs to cover that the Sign* functions produce signatures that are verifiable off of the PublicKey.

utils/crypto/bls/signer/rpcsigner/server_test.go

joshua-kim · 2025-02-06T00:20:53Z

utils/crypto/bls/signer/rpcsigner/bls_test.go

+	require.True(isValid)
+}
+
+func TestVerifyValidProofOfPossession(t *testing.T) {


Similar comment regarding test tables w/ the following test

joshua-kim · 2025-02-07T19:07:45Z

proto/signer/signer.proto

+message SignatureRequest {
+  bytes message = 1;
+}
+message SignatureResponse {


nit: SignRequest/SignResponse

joshua-kim · 2025-02-07T19:08:01Z

proto/signer/signer.proto

+message ProofOfPossessionSignatureRequest {
+  bytes message = 1;
+}
+message ProofOfPossessionSignatureResponse {


nit: SignProofOfPossessionSignatureRequest/Response

joshua-kim · 2025-02-07T19:15:48Z

utils/crypto/bls/signer/rpcsigner/client.go

+	return getSignatureFromResponse(resp)
+}
+
+func (c *Client) PublicKey() *bls.PublicKey {


nit: put this before Sign to be consistent w/ ordering on the interface

joshua-kim · 2025-02-07T19:26:07Z

utils/crypto/bls/signer/rpcsigner/client.go

+}
+
+func (c *Client) Sign(message []byte) (*bls.Signature, error) {
+	resp, err := c.client.Sign(context.Background(), &pb.SignatureRequest{Message: message})


Use TODO instead of Background

joshua-kim · 2025-02-10T17:55:35Z

utils/crypto/bls/signer/rpcsigner/client.go

+
+	pubkeyResponse, err := client.PublicKey(context.TODO(), &pb.PublicKeyRequest{})
+	if err != nil {
+		conn.Close()


I feel weird about this code owning the lifecycle of the connection since it's provided by the caller. Some of these errors don't look persistent + you can still re-use the connection since you can retry rpcs on different streams on the same http/2 connection even if this errored out.

You could probably make a strong case for some of these errors being persistent (i.e the empty public key case, the invalid pk case), but I would still think that you could handle all of these errors at the call-site through a connection close and have all the connection lifecycle managed there instead of having mixed ownership

joshua-kim · 2025-02-10T17:56:10Z

utils/crypto/bls/signer/rpcsigner/client.go

+func (c *Client) Close() error {
+	return c.conn.Close()
+}


Related to my last comment but I think this can be removed as well

joshua-kim · 2025-02-10T17:57:38Z

utils/crypto/bls/signer/rpcsigner/client.go

+type signatureResponse interface {
+	GetSignature() []byte
+}
+
+func getSignatureFromResponse[T signatureResponse](resp T) (*bls.Signature, error) {
+	signature := resp.GetSignature()
+	if len(signature) == 0 {
+		return nil, ErrorEmptySignature
+	}
+
+	return bls.SignatureFromBytes(signature)
+}


Can't tell if this is hacky or clever but it works

joshua-kim · 2025-02-10T17:59:39Z