Enable Cubist Signer integration #3965

geoff-vball · 2025-05-15T16:13:41Z

Why this should be merged

The enables the rpc-signer configuration.

This is based off of this old PR.

How this works

I moved the instantiation of the signer from the config package to node.New() where we instantiate everything else that needs filesystem/networking. This way we don't need to propagate cleanup all over the place.

I also opted for a Shutdown() method on bls.Signer, which is how the other resources attached to node are managed. Currently, resources are not shutdown if we fail in another part of node.New(), but this is how the rest of the resources are also managed.

Out of scope for this PR, but we may want to consider some sort of registry of cleanup/shutdown functions that can be unwound whenever we fail.

How this was tested

Unit tests

Need to be documented in RELEASES.md?

Yes?

The code was changed without changing the behaviour. Instead of `os.Stat`ing the file, we just try to read the file (which returned the same error type).

Prior to this, we were checking if the file existed before attempting to readd it. It was unnecessary and the logic has been removed.

cam-schultz

Treating the signer the same way as the other Node composite fields makes sense to me. I left a few questions/comments about the config parsing pattern.

cam-schultz · 2025-05-28T16:53:36Z

config/config.go

+
+	default:
+		return node.SignerPathConfig{
+			SigningKeyPath:  getExpandedArg(v, StakingSignerKeyPathKey),


Just to confirm, if StakingSignerKeyPathKey is not set, do we use the default path?

That's my understanding? But I'm not 100% how this expands to the default path - I just took the existing logic 😅

config/node/config.go

cam-schultz · 2025-05-28T17:17:08Z

utils/crypto/bls/signer/signer.go

+		if cfg.SignerPathIsSet {
+			return nil, errMissingStakingSigningKeyFile
+		}


I don't love this pattern of embedding metadata into the config struct that's only used for internal control flow. One alternative would be to return a signerPathIsSet bool from getStakingSignerConfig and pass that around, but dedicating an entire return parameter that needs to be managed by the caller for one specific configuration type is also cumbersome. I mainly wanted to call this out as a slightly awkward implementation, that may not have a cleaner alternative.

Yeah, I don't think passing a return parameter around is a good choice. Would renaming the boolean make more sense? I just kind of went off what was there, but I think "fromExistingFile" might make more sense?

I think a private field and a getter makes sense. I agree that I wouldn't add a return value.

A private field in StakingConfig or in SignerPathConfig?

I was thinking in SignerPathConfig but I now realize that this is in node package instead of config which would require a constructor or a setter. I don't feel strongly about this either way but do prefer not serializing it (by omitting the json tag) if it's left public.

Another idea is to just make a different type called DefaultSignerConfig so we could switch on that instead of having to do this hack + avoid the ok var passing

iansuvak

The logic LGTM

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

joshua-kim · 2025-06-05T18:49:38Z

config/config.go

 	return config, nil
 }

+func getStakingSignerConfig(v *viper.Viper) (interface{}, error) {


This PR uses this a lot, but as of go 1.18 it's idiomatic to use the any alias for interface{}

joshua-kim · 2025-06-05T19:32:13Z

utils/crypto/bls/signer/signer.go

+var errMissingStakingSigningKeyFile = errors.New("missing staking signing key file")
+
+// GetStakingSigner returns a BLS signer based on the provided configuration.
+func GetStakingSigner(


go's idiom is to name factory functions New*, so I think this should be NewStakingSigner

Location looks a bit awkward to me - the package is the unit of abstraction and this is the node deciding which signer to use based off of the user-provided config, so I think this should live as a non-exported function in node + all of the config types we created should also be non-exported.

joshua-kim · 2025-06-05T19:32:48Z

config/config_test.go

@@ -549,6 +551,79 @@ func TestGetSubnetConfigsFromFlags(t *testing.T) {
 	}
 }

+func TestGetStakingSigner(t *testing.T) {
+	testKey := "HLimS3vRibTMk9lZD4b+Z+GLuSBShvgbsu0WTLt2Kd4="
+	type cfg map[string]any


nit: we don't need this type alias

joshua-kim · 2025-06-10T19:40:30Z

config/config_test.go

+			config, err := GetNodeConfig(v)
+
+			require.ErrorIs(err, tt.expectedErr)
+			require.IsType(tt.expectedSignerConfigType, config.StakingSignerConfig)


Should we just do an check on the expected config instead of just a type check?

This was slightly annoying because of the way temp dirs work in the test suite. Let me know if you think it's better or worse.

joshua-kim · 2025-06-10T19:44:02Z

config/config_test.go

+		expectedErr              error
+	}{
+		{
+			name:                     "default-signer",


Can we remove the kebab-casing in these test names? Plain strings work fine and there's no reason to format it this way IMO

joshua-kim · 2025-06-10T20:34:51Z

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

 	}

 	pkBytes := pubkeyResponse.GetPublicKey()
 	pk, err := bls.PublicKeyFromCompressedBytes(pkBytes)
 	if err != nil {
-		return nil, err
+		return nil, errors.Join(fmt.Errorf("failed to uncompress public key bytes: %w", err), conn.Close())


joshua-kim · 2025-06-10T20:36:25Z

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


-	return bls.SignatureFromBytes(signature)
+	sigBytes := resp.GetSignature()
+	return bls.SignatureFromBytes(sigBytes)


Since we're now wrapping the previous error I feel like it's weird for us to not wrap this error

joshua-kim · 2025-06-10T20:36:40Z

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


-	return bls.SignatureFromBytes(signature)
+	sigBytes := resp.GetSignature()
+	return bls.SignatureFromBytes(sigBytes)


Same nit here

joshua-kim · 2025-06-10T20:37:09Z

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

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


Similar nit here

Does wrapping an error in a function that only has one possible return provide any meaningful information?

Although there aren't any other errors in this scope, if we don't wrap this error it's not disambiguated from other grpc connection close errors. As an example, if we had another grpc conn in avalanchego and its conn close failed, we might lose visibility into which conn was the one we had to debug.

joshua-kim · 2025-06-10T20:37:28Z

utils/crypto/bls/signer/signer.go

+func createSignerFromFile(signingKeyPath string) (bls.Signer, error) {
+	signingKeyBytes, err := os.ReadFile(signingKeyPath)
+	if err != nil {
+		return nil, err


same wrapping nit

joshua-kim · 2025-06-10T20:42:27Z

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

 	}, nil
 }

 func (c *Client) PublicKey() *bls.PublicKey {
 	return c.pk
 }

+// Sign a message. The [Client] already handles transient connection errors.


I would move the comment w.r.t transient conn errors to NewClient or Client since it's applicable to the type and not just Sign

joshua-kim · 2025-06-10T20:46:13Z

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

 }

+// [SignProofOfPossession] has the same behavior as [Sign] but will product a different signature.


Square brackets are so confusing... my understanding is:

This is a newer convention that auto links in godoc

It only works if it's linked properly + exported out of the package, so stuff a package foo that can do [Foo] (if Foo is an exported type in foo) and [bar.Bar], but if Foo.Baz is a method then you have to specify the type and can't just reference Baz.

So if you want to use square brackets I think you need to prefix the type (i.e Client.SignProofOfPossession and Client.Sign), or it'll break in godoc. I think you could alternatively just not use square brackets.

Regarding the actual comment, I think we can just say it produces a ProofOfPossession signature instead of different, since it's not clear what that difference is + we document PoP elsewhere in the repo iirc.

I think I removed them all. I haven't used these at all before - they're from the portion of the code Richard wrote

Co-authored-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Signed-off-by: Geoff Stuart <geoff.vball@gmail.com>

… into signers-config-wip

richardpringle and others added 30 commits March 12, 2025 14:05

Add comments to signer-config setup

dd5176e

Extend siging configuration to include RPC

ed0c5f8

Add default behaviour to switch

dd99c36

Add timeout to signer instantiation

9f61b1c

Make rpc-signer client handle the connection

f106e92

Fix linter error

de574a4

Close the connection on any error

a7e895a

Remove special character from error message

15a53f0

Use Go casing conventions in json config

cc11cac

Fix default signing config behaviour

eeb3644

Cleanup type-check in config-test

47e37fa

Simply variable name for url

505c7fb

Only use public config api in test

0d66c3a

Add test for default config signer with multiple inits

f8837e8

Merge remote-tracking branch 'origin/master' into signers-config-wip

97ffb21

Merge remote-tracking branch 'origin/master' into signers-config-wip

7327d0f

Bubble up cleanup function

bb56fd5

Merge remote-tracking branch 'origin/master' into signers-config-wip

0b22c0a

Merge remote-tracking branch 'origin/master' into signers-config-wip

d3e2677

Merge remote-tracking branch 'origin/master' into signers-config-wip

193ccd4

Simplify default signer creation code

bd77f88

The code was changed without changing the behaviour. Instead of `os.Stat`ing the file, we just try to read the file (which returned the same error type).

Cleanup signer creation when key path is set

5bc1bc0

Prior to this, we were checking if the file existed before attempting to readd it. It was unnecessary and the logic has been removed.

Remove redundant word in error message

64f1a1d

Fix config test name

b55f05d

Small rpc-signer-client cleanup refactor

e85e815

Fix the min-connect-timeout for grpc signer

c8b3714

Merge branch 'master' into signers-config-wip

c46209e

Merge branch 'master' into signers-config-wip

8a4cb7c

Reconfigure signer setup

99c3a48

Merge branch 'master' into signers-config-wip

e23e982

geoff-vball and others added 4 commits May 27, 2025 13:50

Merge branch 'master' into signers-config-wip

649447d

Fix ordering

d79916b

Lint

14c5525

Merge branch 'master' into signers-config-wip

fac9f79

geoff-vball marked this pull request as ready for review May 28, 2025 13:14

geoff-vball requested a review from StephenButtolph as a code owner May 28, 2025 13:14

StephenButtolph requested review from a team and maru-ava and removed request for a team May 28, 2025 14:53

geoff-vball changed the title ~~Signers config wip~~ Enable Cubist Signer integration May 28, 2025

cam-schultz reviewed May 28, 2025

View reviewed changes

Remove json tags from fields that are not user supplied

8fa5525

iansuvak reviewed May 29, 2025

View reviewed changes

utils/crypto/bls/signer/localsigner/localsigner.go Outdated Show resolved Hide resolved

geoff-vball added 2 commits May 29, 2025 10:01

Remove stray comment

0c15985

Merge branch 'master' into signers-config-wip

15db2ad

StephenButtolph requested a review from a team June 5, 2025 18:13

joshua-kim requested review from joshua-kim and removed request for maru-ava June 5, 2025 18:43

joshua-kim moved this to In Progress 🏗️ in avalanchego Jun 10, 2025

joshua-kim reviewed Jun 10, 2025

View reviewed changes

geoff-vball and others added 5 commits June 17, 2025 11:59

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

58cc0c1

Co-authored-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Signed-off-by: Geoff Stuart <geoff.vball@gmail.com>

Review fixes

f92d3be

Merge branch 'signers-config-wip' of github.com:ava-labs/avalanche-go…

270d59a

… into signers-config-wip

Move signer to node package

60526b1

Merge branch 'master' into signers-config-wip

9791ff8

geoff-vball requested a review from joshua-kim June 17, 2025 20:25

geoff-vball added 3 commits June 18, 2025 10:54

Add missing files

ba4977a

Wrap error

3283340

Lint

d6512c2

		}

		// [SignProofOfPossession] has the same behavior as [Sign] but will product a different signature.

Enable Cubist Signer integration #3965

Are you sure you want to change the base?

Enable Cubist Signer integration #3965

Uh oh!

Conversation

geoff-vball commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this should be merged

How this works

How this was tested

Need to be documented in RELEASES.md?

Uh oh!

cam-schultz left a comment

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!

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!

iansuvak left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

geoff-vball commented May 15, 2025 •

edited

Loading