Add networked-signer tests #3613

richardpringle · 2024-12-19T20:46:03Z

Move local-signer to its own package
Split bls tests into signing tests and other generic bls operations
Add test for networked signer

Why this should be merged

I moved the tests to make it easier to test multiple signer implementations.

How this works

I split the tests up. There are tests in the bls package as well as the individual signers. There are also a couple of tests in the blstest package as well.

How this was tested

See above

Need to be documented in RELEASES.md?

nope

utils/crypto/bls/signers/local/local.go

utils/crypto/bls/test/bls_benchmark_test.go

This reverts commit 65d61a0.

This reverts commit 67885b3.

This reverts commit 18d1f7c.

marun

(No action required) Would it make sense to separate the functional changes introduced by this PR from the considerable number of non-functional changes (e.g. renames) to simplify review of such an otherwise large PR?

richardpringle · 2025-01-08T15:49:08Z

(No action required) Would it make sense to separate the functional changes introduced by this PR from the considerable number of non-functional changes (e.g. renames) to simplify review of such an otherwise large PR?

Great question... maybe?

I would argue that the overall cost of review doesn't change that much as most files just contain renames. I would have had to change the tests no matter what. I could have added the json-rpc signer in a second PR, but is that really beneficial?

I can do that if you (@marun) or any other reviewers would like, I don't mind.

joshua-kim · 2025-01-09T06:35:42Z

config/config.md

@@ -399,7 +399,6 @@ specified. If not given, uses default genesis data.

 See the documentation for the genesis JSON format [here](https://github.com/ava-labs/avalanchego/blob/master/genesis/README.md) and an example used for the local network genesis [here](https://github.com/ava-labs/avalanchego/blob/master/genesis/).

-


nit: undo diff in this line since it's unrelated

joshua-kim · 2025-01-09T06:36:26Z

utils/crypto/bls/signers/localsigner/localsigner.go

nit: rename this path to bls/signer/localsigner. Plural package names read weirdly (e.g if we start writing code in signers and have an array of signers how do we refer to that? signerses?)

joshua-kim · 2025-01-09T06:44:46Z

utils/crypto/bls/signers/localsigner/localsigner.go

+	"errors"
+	"runtime"
+
+	"github.com/ava-labs/avalanchego/utils/crypto/bls" // Import the parent package


nit: Comments on why we're importing a package are unnecessary unless there's some side-effect of the import we depend on

joshua-kim · 2025-01-09T07:02:12Z

utils/crypto/bls/signer.go

+	Sign(msg []byte) *Signature
+	SignProofOfPossession(msg []byte) *Signature


I feel like this interface looks cleaner if we just merge these two function signatures and just add the ciphertext as a parameter. Having a separate function signature for each ciphertext feels like it just bloats the interface... but not sure what others think.

I actually think that we should have a ProofOfPossession() (no parameters) method. We shouldn't really be doing proof-of-possession signatures of anything other than the pubkey.

Regardless, that change would be beyond the scope of this PR.

Just to elaborate here, the reason that I prefer to have both APIs over a single function where one has to pass in the digest is because I think this should be opaque to the caller. It's an implementation detail that these two methods only differ in the digest used for signing.

joshua-kim · 2025-01-09T07:06:31Z

utils/crypto/bls/blstest/bls_benchmark_test.go

@@ -35,7 +37,7 @@ var (
 )

 func BenchmarkSign(b *testing.B) {


Something awkward about this test structure from the perspective of localsigner is that this test benchmarks the localsigner implementation (knows how to sign), not bls (knows operations across bls keys like aggregation/verification), so it feels like this belongs under something like signer/localsigner/local_signer_test.go since bls isn't even used here.

Maybe an alternative to this structure would be to have:

bls/bls_test.go - tests bls (operations across keys)

signer/signertest/signertest.go - has a common test suite that you can plug in any implementation of Signer into to verify that the interface implementation is correct

signer/localsigner/localsigner_test.go - uses the test suite in signertest

signer/jsonrpc - uses the test suite in signertest

joshua-kim · 2025-01-09T07:26:32Z

utils/crypto/bls/signers/json-rpc/messages.go

nit: message.go, or just define these types server.go

joshua-kim · 2025-01-09T07:27:29Z

utils/crypto/bls/signers/json-rpc/server.go

+	listener   net.Listener
+}
+
+func NewSignerService() *signerService {


Nit: export signerService since it's being used from an exported function

joshua-kim · 2025-01-09T07:33:32Z

utils/crypto/bls/signers/json-rpc/server.go

+	if err != nil {
+		panic(err)
+	}


Can we bubble up the error here instead of panic'ing? I think for the other panics it's reasonable if we don't want to make the diff bigger because it's an interface change + the panic's wont trigger because the network signer isn't actually used.

The constructor isn't actually part of the interface. Easy to add this, 👍

joshua-kim · 2025-01-09T07:49:03Z

utils/crypto/bls/blstest/bls_test.go

+var signerFns = []func() (Signer, error){
+	localSignerFn,
+	serverSignerFn,
+}
+


I think the tests in this file are a bit complex and also are now testing integration across the jsonrpc signer and localsigner which I feel like is unnecessary - if they correctly implement the Signer interface they're just returning plain data types and not interfaces so the operations in bls should still work across them. I think if we go with the earlier comment's suggestion we can probably get rid of a lot of these tests.

joshua-kim · 2025-01-09T07:50:23Z

utils/crypto/bls/signers/json-rpc/server.go

+// Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved.
+// See the file LICENSE for licensing terms.
+
+package jsonrpc


nit: this package name is inconsistent with localsigner... maybe we rename localsigner to local or jsonrpc to jsonrpcsigner. Alternatively we can pull this out of the PR and put it into a subsequent one.

🤔. If we're happy with how testing is split up, I don't mind pulling it out. The only thing is that the way I split up the testing might look a little silly with only a single implementation.

I'm easy though, either way works.

I also think jsonrpcsigner makes the most sense here and will make that change regardless.

richardpringle · 2025-01-14T18:06:26Z

utils/crypto/bls/blstest/blstest.go

+	}
+
+	return bls.Verify(aggPK, aggSig, message), nil
+}


Should I just expose this function from the bls library?

richardpringle · 2025-01-14T18:32:29Z

utils/crypto/bls/bls_benchmark_test.go

-			}
-		})
-	}
-}


Moved to individual implementations

richardpringle · 2025-01-14T19:15:16Z

utils/crypto/bls/signature.go

+	}
+
+	return Verify(aggPK, aggSig, message), nil
+}


This function is only used in tests right now. I'm not sure if there's a better way to expose it

The preferred approach (in this repo) is to use *test packages for test-only code: #3238

As for this functions utility outside of tests, I can't think of a use case off the top of my head where the caller would be both the signer and verifier.

cam-schultz

The structure makes sense to me, and I appreciate the comprehensive test coverage you included. I left some comments that should hopefully be straightforward to answer/address.

cam-schultz · 2025-01-17T15:28:57Z

utils/crypto/bls/signature.go

+	}
+
+	return Verify(aggPK, aggSig, message), nil
+}


The preferred approach (in this repo) is to use *test packages for test-only code: #3238

As for this functions utility outside of tests, I can't think of a use case off the top of my head where the caller would be both the signer and verifier.

cam-schultz · 2025-01-17T15:32:45Z

utils/crypto/bls/signers/json-rpc/benchmark_test.go

+)
+
+func BenchmarkSign(b *testing.B) {
+	server, privateKey, _ := NewSigner(require.New(b))


What "server" is running here? Trying to understand what this benchmark measures distinct from the local signer benchmark.

cam-schultz · 2025-01-17T15:46:58Z

utils/crypto/bls/signers/json-rpc/client.go

What's the motivation to use gorilla/rpc/v2/json2 instead of avalanchego/utils/rpc to build out this package? Most other rpc packages in Avalanchego use avalanchego/utils/rpc/avalanchego/utils/json, both of which use gorilla internally.

richardpringle added 3 commits December 18, 2024 15:25

Move local-signer to its own package

321dd52

Prepare bls-tests for multiple signer-types

b731e7e

Add test for networked signer

435d116

richardpringle force-pushed the encapsulate-signer branch from ccdd538 to 435d116 Compare December 19, 2024 21:11

joshua-kim reviewed Dec 20, 2024

View reviewed changes

richardpringle added 23 commits January 6, 2025 12:54

Expose secret-key-length from the bls package

a2ab5d4

Rename signers/local to signers/localsigner

7f49c85

Remove redundant comment

9ad13b2

Rename local.go to localsigner.go

9fd0c12

Rename bls_test to blstest

67885b3

Rename bls/test dir to blstest

65d61a0

Revert "Rename bls/test dir to blstest"

77367f2

This reverts commit 65d61a0.

Revert "Rename bls_test to blstest"

eeb846a

This reverts commit 67885b3.

Fix bls tests

7027a47

Follow test package pattern from the rest of the repo

18d1f7c

Revert "Follow test package pattern from the rest of the repo"

c4170e7

This reverts commit 18d1f7c.

Use blstest convention

1532698

Merge remote-tracking branch 'origin/master' into encapsulate-signer

db266c6

Fix accidental search-and-replace

988920f

Fix remaining references to old bls methods

8885a5c

Add updated coreth

1ca1be1

Update tests to use localsigner

41830aa

Merge remote-tracking branch 'origin/master' into encapsulate-signer

6844aeb

Fix SecretKeyFromBytes

d92c69c

Fix linter errors

a679b44

More lint fixes

67bc45b

Reset tree.go file

44814bd

Add missing license header

695329f

richardpringle marked this pull request as ready for review January 7, 2025 22:30

richardpringle requested a review from marun as a code owner January 7, 2025 22:30

richardpringle requested review from meaghanfitzgerald and StephenButtolph as code owners January 7, 2025 22:30

richardpringle requested review from yacovm, joshua-kim and samliok January 7, 2025 22:30

marun reviewed Jan 7, 2025

View reviewed changes

richardpringle requested a review from marun January 8, 2025 15:49

richardpringle added 6 commits January 8, 2025 12:03

More lint-fixes

885230d

Merge remote-tracking branch 'origin/master' into encapsulate-signer

9fdfe99

Merge remote-tracking branch 'origin/master' into encapsulate-signer

9d25f68

Fix bls-tests

494864f

Make sure there are signers for the test

3b7858b

More bls-test fixes

75c98c5

joshua-kim reviewed Jan 9, 2025

View reviewed changes

Reset config/config.md

5835988

richardpringle marked this pull request as draft January 13, 2025 22:59

richardpringle added 2 commits January 14, 2025 12:59

Split bls tests

1bca3ef

Merge remote-tracking branch 'origin/master' into encapsulate-signer

a723269

richardpringle force-pushed the encapsulate-signer branch from 56c6f9a to a723269 Compare January 14, 2025 18:02

richardpringle commented Jan 14, 2025

View reviewed changes

richardpringle added 2 commits January 14, 2025 13:10

Move bls public_test.go back into the bls package

3c1ce6c

Move bls-tests back to bls package

ad10cb2

richardpringle commented Jan 14, 2025

View reviewed changes

utils/crypto/bls/bls_benchmark_test.go

}

})

}

}

Copy link

Contributor Author

richardpringle Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to individual implementations

richardpringle commented Jan 14, 2025

View reviewed changes

richardpringle marked this pull request as ready for review January 14, 2025 22:47

richardpringle added 2 commits January 14, 2025 17:58

Fix linting

0533b15

Add missing license headers

a429a32

cam-schultz reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add networked-signer tests #3613

Add networked-signer tests #3613

richardpringle commented Dec 19, 2024 •

edited

Loading

marun left a comment

richardpringle commented Jan 8, 2025

joshua-kim Jan 9, 2025

joshua-kim Jan 9, 2025

joshua-kim Jan 9, 2025

joshua-kim Jan 9, 2025

richardpringle Jan 14, 2025 •

edited

Loading

richardpringle Jan 15, 2025

joshua-kim Jan 9, 2025

joshua-kim Jan 9, 2025

joshua-kim Jan 9, 2025

joshua-kim Jan 9, 2025

richardpringle Jan 15, 2025

joshua-kim Jan 9, 2025

joshua-kim Jan 9, 2025

richardpringle Jan 15, 2025

richardpringle Jan 14, 2025

richardpringle Jan 14, 2025

richardpringle Jan 14, 2025

cam-schultz Jan 17, 2025

cam-schultz left a comment

cam-schultz Jan 17, 2025

cam-schultz Jan 17, 2025

cam-schultz Jan 17, 2025

		@@ -399,7 +399,6 @@ specified. If not given, uses default genesis data.

		See the documentation for the genesis JSON format [here](https://github.com/ava-labs/avalanchego/blob/master/genesis/README.md) and an example used for the local network genesis [here](https://github.com/ava-labs/avalanchego/blob/master/genesis/).

		Sign(msg []byte) *Signature
		SignProofOfPossession(msg []byte) *Signature

Add networked-signer tests #3613

Are you sure you want to change the base?

Add networked-signer tests #3613

Conversation

richardpringle commented Dec 19, 2024 • edited Loading

Why this should be merged

How this works

How this was tested

Need to be documented in RELEASES.md?

marun left a comment

Choose a reason for hiding this comment

richardpringle commented Jan 8, 2025

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

richardpringle Jan 14, 2025 • 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

cam-schultz 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

richardpringle commented Dec 19, 2024 •

edited

Loading

richardpringle Jan 14, 2025 •

edited

Loading