code review improvements

Author: Thegaram

rollup/internal/config/relayer.go

@@ -146,8 +146,9 @@ type AWSKMSSignerConfig struct {
 	// KeyID is the KMS key id, alias or ARN of an asymmetric ECC_SECG_P256K1 / SIGN_VERIFY key.
 	KeyID string `json:"key_id"`
 	// Region is the AWS region of the key. Optional; falls back to the ambient AWS config
-	// (AWS_REGION, shared config, instance/IRSA role) when empty.
-	Region string `json:"region,omitempty"`
+	// (AWS_REGION, shared config, instance/IRSA role) when empty, and startup fails if
+	// no region resolves from either.
+	Region string `json:"region"`
 	// SignerAddress is the expected Ethereum address of the key. Required: it is validated
 	// against the address derived from the KMS public key at startup so a misconfigured
 	// key id fails fast instead of signing from an unexpected account.

rollup/internal/controller/relayer/l2_relayer.go

@@ -1308,19 +1308,26 @@ func addrFromSignerConfig(config *config.SignerConfig) (common.Address, error) {
 		}
 		return crypto.PubkeyToAddress(privKey.PublicKey), nil
 	case sender.RemoteSignerType:
-		if config.RemoteSignerConfig.SignerAddress == "" {
-			return common.Address{}, fmt.Errorf("signer address is empty")
+		if config.RemoteSignerConfig == nil {
+			return common.Address{}, fmt.Errorf("remote_signer_config is missing")
 		}
-		return common.HexToAddress(config.RemoteSignerConfig.SignerAddress), nil
+		return parseSignerAddress(config.RemoteSignerConfig.SignerAddress)
 	case sender.AWSKMSSignerType:
-		if config.AWSKMSSignerConfig == nil || config.AWSKMSSignerConfig.SignerAddress == "" {
-			return common.Address{}, fmt.Errorf("aws kms signer address is empty")
+		if config.AWSKMSSignerConfig == nil {
+			return common.Address{}, fmt.Errorf("aws_kms_signer_config is missing")
 		}
-		if !common.IsHexAddress(config.AWSKMSSignerConfig.SignerAddress) {
-			return common.Address{}, fmt.Errorf("aws kms signer address %q is not a valid hex address", config.AWSKMSSignerConfig.SignerAddress)
-		}
-		return common.HexToAddress(config.AWSKMSSignerConfig.SignerAddress), nil
+		return parseSignerAddress(config.AWSKMSSignerConfig.SignerAddress)
 	default:
 		return common.Address{}, fmt.Errorf("failed to determine signer address, unknown signer type: %v", config.SignerType)
 	}
 }
+
+// parseSignerAddress parses a signer address configured for a signer that holds
+// its key outside the process. An empty string is not a valid hex address, so
+// this covers both a missing and a malformed value.
+func parseSignerAddress(addr string) (common.Address, error) {
+	if !common.IsHexAddress(addr) {
+		return common.Address{}, fmt.Errorf("signer address %q is not a valid hex address", addr)
+	}
+	return common.HexToAddress(addr), nil
+}

rollup/internal/controller/sender/README.md

@@ -33,7 +33,8 @@ so all transaction types — including blob transactions — are supported.
 - **`key_id`** — id, alias, or ARN of an asymmetric KMS key with key spec
   `ECC_SECG_P256K1` and key usage `SIGN_VERIFY`.
 - **`region`** — optional; falls back to the ambient AWS config
-  (`AWS_REGION`, shared config, instance/IRSA role) when empty.
+  (`AWS_REGION`, shared config, instance/IRSA role) when empty. Startup fails if
+  no region resolves from either.
 - **`signer_address`** — **required**. The expected Ethereum address of the key.
   It is validated at startup against the address derived from the KMS public key,
   so a misconfigured `key_id` fails fast instead of signing from an unexpected
@@ -56,9 +57,16 @@ role, or environment) — never put credentials in the config file.
 There are two ways to get a key into KMS. Whichever you use, the signer never
 needs the raw private key — it derives the Ethereum address from the KMS public
 key (`keccak256(pubkey)[12:]`) at startup, and you put that address in
-`signer_address`. Get the address from `GetPublicKey` (the relayer logs it on
-startup, or derive it yourself from the returned point) and **fund it** before
-the sender goes live.
+`signer_address`. To get it, take the `PublicKey` returned by
+`aws kms get-public-key` (base64 SPKI DER), whose last 65 bytes are the
+uncompressed point `0x04 || X || Y`, and keccak256-hash the 64 bytes after the
+`0x04` prefix; the address is the last 20 bytes of that hash. Alternatively, start
+the relayer with a placeholder `signer_address`: startup fails with an error that
+names the address derived from the KMS key, which you then put in the config.
+
+**Fund the address** before the sender goes live. Once configured correctly, the
+relayer logs `initialized AWS KMS signer` with the key id and address, so you can
+confirm which account a workload signs from.
 
 **Option A — generate in KMS (recommended).** The private key is created inside
 the KMS HSM and is non-exportable: it provably never exists outside KMS.

rollup/internal/controller/sender/kms_signer.go

@@ -12,7 +12,9 @@ import (
 	kmstypes "github.com/aws/aws-sdk-go-v2/service/kms/types"
 
 	"github.com/scroll-tech/go-ethereum/common"
+	gethTypes "github.com/scroll-tech/go-ethereum/core/types"
 	"github.com/scroll-tech/go-ethereum/crypto"
+	"github.com/scroll-tech/go-ethereum/log"
 
 	"scroll-tech/rollup/internal/config"
 )
@@ -24,24 +26,29 @@ type kmsAPI interface {
 	Sign(ctx context.Context, params *kms.SignInput, optFns ...func(*kms.Options)) (*kms.SignOutput, error)
 }
 
-// kmsSigner produces ECDSA secp256k1 signatures over transaction hashes using an
-// AWS KMS asymmetric key. The private key never leaves KMS: only the 32-byte
-// signing hash is sent, and KMS returns a DER-encoded signature which we turn
-// into Ethereum's 65-byte [R || S || V] form locally.
+// kmsSigner signs transactions with an AWS KMS asymmetric secp256k1 key. The
+// private key never leaves KMS: only the 32-byte signing hash is sent, and KMS
+// returns a DER-encoded signature which we turn into Ethereum's 65-byte
+// [R || S || V] form and apply to the transaction locally. Because the tx is
+// assembled locally, every tx type the sender builds is supported, including BlobTx.
 type kmsSigner struct {
-	client kmsAPI
-	keyID  string
-	addr   common.Address
+	client   kmsAPI
+	keyID    string
+	addr     common.Address
+	txSigner gethTypes.Signer
+}
+
+// asn1AlgorithmIdentifier is the AlgorithmIdentifier of a SubjectPublicKeyInfo.
+type asn1AlgorithmIdentifier struct {
+	Algorithm  asn1.ObjectIdentifier
+	Parameters asn1.ObjectIdentifier
 }
 
 // asn1Spki mirrors the SubjectPublicKeyInfo DER structure returned by KMS
 // GetPublicKey for an ECC_SECG_P256K1 key. PublicKey holds the uncompressed
 // point (0x04 || X || Y).
 type asn1Spki struct {
-	Algorithm struct {
-		Algorithm  asn1.ObjectIdentifier
-		Parameters asn1.ObjectIdentifier
-	}
+	Algorithm asn1AlgorithmIdentifier
 	PublicKey asn1.BitString
 }
 
@@ -67,7 +74,7 @@ var (
 // newKMSSigner constructs a signer backed by a live AWS KMS key. cfg.SignerAddress
 // is required and validated against the address derived from the KMS public key,
 // so a wrong key id fails fast instead of signing from an unexpected account.
-func newKMSSigner(ctx context.Context, cfg *config.AWSKMSSignerConfig) (*kmsSigner, error) {
+func newKMSSigner(ctx context.Context, cfg *config.AWSKMSSignerConfig, chainID *big.Int) (*kmsSigner, error) {
 	if cfg == nil {
 		return nil, fmt.Errorf("aws_kms_signer_config is nil")
 	}
@@ -89,13 +96,16 @@ func newKMSSigner(ctx context.Context, cfg *config.AWSKMSSignerConfig) (*kmsSign
 	if err != nil {
 		return nil, fmt.Errorf("aws kms signer: failed to load aws config: %w", err)
 	}
+	if awsCfg.Region == "" {
+		return nil, fmt.Errorf("aws kms signer: aws region is not set (configure region or AWS_REGION)")
+	}
 
-	return newKMSSignerWithClient(ctx, kms.NewFromConfig(awsCfg), cfg.KeyID, common.HexToAddress(cfg.SignerAddress))
+	return newKMSSignerWithClient(ctx, kms.NewFromConfig(awsCfg), cfg.KeyID, common.HexToAddress(cfg.SignerAddress), chainID)
 }
 
 // newKMSSignerWithClient is the testable core: it derives the key's address from
 // the KMS public key and asserts it matches expectedAddr.
-func newKMSSignerWithClient(ctx context.Context, client kmsAPI, keyID string, expectedAddr common.Address) (*kmsSigner, error) {
+func newKMSSignerWithClient(ctx context.Context, client kmsAPI, keyID string, expectedAddr common.Address, chainID *big.Int) (*kmsSigner, error) {
 	pub, err := publicKeyFromKMS(ctx, client, keyID)
 	if err != nil {
 		return nil, err
@@ -104,7 +114,13 @@ func newKMSSignerWithClient(ctx context.Context, client kmsAPI, keyID string, ex
 	if derivedAddr != expectedAddr {
 		return nil, fmt.Errorf("aws kms signer: configured signer_address %s does not match address %s derived from KMS key %s", expectedAddr.Hex(), derivedAddr.Hex(), keyID)
 	}
-	return &kmsSigner{client: client, keyID: keyID, addr: derivedAddr}, nil
+	log.Info("initialized AWS KMS signer", "keyID", keyID, "address", derivedAddr.Hex(), "chainID", chainID)
+	return &kmsSigner{
+		client:   client,
+		keyID:    keyID,
+		addr:     derivedAddr,
+		txSigner: gethTypes.LatestSignerForChainID(chainID),
+	}, nil
 }
 
 func publicKeyFromKMS(ctx context.Context, client kmsAPI, keyID string) (*ecdsa.PublicKey, error) {
@@ -140,6 +156,19 @@ func (k *kmsSigner) address() common.Address {
 	return k.addr
 }
 
+// signTx returns tx signed by the KMS key.
+func (k *kmsSigner) signTx(ctx context.Context, tx *gethTypes.Transaction) (*gethTypes.Transaction, error) {
+	sig, err := k.sign(ctx, k.txSigner.Hash(tx).Bytes())
+	if err != nil {
+		return nil, err
+	}
+	signedTx, err := tx.WithSignature(k.txSigner, sig)
+	if err != nil {
+		return nil, fmt.Errorf("aws kms signer: failed to apply signature to tx: %w", err)
+	}
+	return signedTx, nil
+}
+
 // sign returns the 65-byte [R || S || V] Ethereum signature over the given 32-byte hash.
 func (k *kmsSigner) sign(ctx context.Context, hash []byte) ([]byte, error) {
 	out, err := k.client.Sign(ctx, &kms.SignInput{

rollup/internal/controller/sender/kms_signer_test.go

@@ -34,10 +34,7 @@ type fakeKMS struct {
 func (f *fakeKMS) GetPublicKey(_ context.Context, _ *kms.GetPublicKeyInput, _ ...func(*kms.Options)) (*kms.GetPublicKeyOutput, error) {
 	pubBytes := crypto.FromECDSAPub(&f.priv.PublicKey) // 0x04 || X || Y
 	der, err := asn1.Marshal(asn1Spki{
-		Algorithm: struct {
-			Algorithm  asn1.ObjectIdentifier
-			Parameters asn1.ObjectIdentifier
-		}{Algorithm: ecPublicKeyOID, Parameters: secp256k1OID},
+		Algorithm: asn1AlgorithmIdentifier{Algorithm: ecPublicKeyOID, Parameters: secp256k1OID},
 		PublicKey: asn1.BitString{Bytes: pubBytes, BitLength: len(pubBytes) * 8},
 	})
 	if err != nil {
@@ -66,10 +63,10 @@ func (f *fakeKMS) Sign(_ context.Context, in *kms.SignInput, _ ...func(*kms.Opti
 	return &kms.SignOutput{Signature: der}, nil
 }
 
-func newTestKMSSigner(t *testing.T, fake *fakeKMS) *kmsSigner {
+func newTestKMSSigner(t *testing.T, fake *fakeKMS, chainID *big.Int) *kmsSigner {
 	t.Helper()
 	expected := crypto.PubkeyToAddress(fake.priv.PublicKey)
-	ks, err := newKMSSignerWithClient(context.Background(), fake, "test-key-id", expected)
+	ks, err := newKMSSignerWithClient(context.Background(), fake, "test-key-id", expected, chainID)
 	require.NoError(t, err)
 	assert.Equal(t, expected, ks.address())
 	return ks
@@ -81,10 +78,10 @@ func TestKMSSigner_AddressValidation(t *testing.T) {
 	fake := &fakeKMS{priv: priv}
 
 	// matching address succeeds
-	newTestKMSSigner(t, fake)
+	newTestKMSSigner(t, fake, big.NewInt(534352))
 
 	// mismatching address fails fast
-	_, err = newKMSSignerWithClient(context.Background(), fake, "test-key-id", common.HexToAddress("0xdeadbeef00000000000000000000000000000000"))
+	_, err = newKMSSignerWithClient(context.Background(), fake, "test-key-id", common.HexToAddress("0xdeadbeef00000000000000000000000000000000"), big.NewInt(534352))
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "does not match")
 }
@@ -134,12 +131,11 @@ func TestKMSSigner_SignAllTxTypes(t *testing.T) {
 
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
-			ks := newTestKMSSigner(t, &fakeKMS{priv: priv, forceHi: tc.forceHi})
+			ks := newTestKMSSigner(t, &fakeKMS{priv: priv, forceHi: tc.forceHi}, chainID)
 			ts := &TransactionSigner{
-				config:      &config.SignerConfig{SignerType: AWSKMSSignerType},
-				kmsSigner:   ks,
-				kmsTxSigner: gethTypes.LatestSignerForChainID(chainID),
-				addr:        ks.address(),
+				config:    &config.SignerConfig{SignerType: AWSKMSSignerType},
+				kmsSigner: ks,
+				addr:      ks.address(),
 			}
 
 			tx := gethTypes.NewTx(tc.txData)
@@ -183,10 +179,7 @@ func (f *rawKMS) Sign(_ context.Context, _ *kms.SignInput, _ ...func(*kms.Option
 func marshalSPKI(t *testing.T, algo, curve asn1.ObjectIdentifier, point []byte, bitLen int) []byte {
 	t.Helper()
 	der, err := asn1.Marshal(asn1Spki{
-		Algorithm: struct {
-			Algorithm  asn1.ObjectIdentifier
-			Parameters asn1.ObjectIdentifier
-		}{Algorithm: algo, Parameters: curve},
+		Algorithm: asn1AlgorithmIdentifier{Algorithm: algo, Parameters: curve},
 		PublicKey: asn1.BitString{Bytes: point, BitLength: bitLen},
 	})
 	require.NoError(t, err)
@@ -207,7 +200,7 @@ func TestKMSSigner_MalformedPublicKey(t *testing.T) {
 	evenPoint[len(evenPoint)-1] &^= 1
 
 	// control: a well-formed SPKI must still be accepted.
-	_, err = newKMSSignerWithClient(context.Background(), &rawKMS{pub: marshalSPKI(t, ecPublicKeyOID, secp256k1OID, point, full)}, "k", addr)
+	_, err = newKMSSignerWithClient(context.Background(), &rawKMS{pub: marshalSPKI(t, ecPublicKeyOID, secp256k1OID, point, full)}, "k", addr, big.NewInt(534352))
 	require.NoError(t, err)
 
 	cases := []struct {
@@ -222,7 +215,7 @@ func TestKMSSigner_MalformedPublicKey(t *testing.T) {
 	}
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
-			_, err := newKMSSignerWithClient(context.Background(), &rawKMS{pub: tc.pub}, "k", addr)
+			_, err := newKMSSignerWithClient(context.Background(), &rawKMS{pub: tc.pub}, "k", addr, big.NewInt(534352))
 			require.Error(t, err)
 			assert.Contains(t, err.Error(), tc.want)
 		})
@@ -264,7 +257,7 @@ func TestKMSSigner_InvalidSignerAddress(t *testing.T) {
 	_, err := newKMSSigner(context.Background(), &config.AWSKMSSignerConfig{
 		KeyID:         "some-key-id",
 		SignerAddress: "not-a-hex-address",
-	})
+	}, big.NewInt(534352))
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "not a valid hex address")
 }

rollup/internal/controller/sender/transaction_signer.go

@@ -29,13 +29,12 @@ const (
 
 // TransactionSigner signs given transactions
 type TransactionSigner struct {
-	config      *config.SignerConfig
-	auth        *bind.TransactOpts
-	rpcClient   *rpc.Client
-	kmsSigner   *kmsSigner
-	kmsTxSigner gethTypes.Signer
-	nonce       uint64
-	addr        common.Address
+	config    *config.SignerConfig
+	auth      *bind.TransactOpts
+	rpcClient *rpc.Client
+	kmsSigner *kmsSigner
+	nonce     uint64
+	addr      common.Address
 }
 
 func NewTransactionSigner(ctx context.Context, config *config.SignerConfig, chainID *big.Int) (*TransactionSigner, error) {
@@ -68,15 +67,14 @@ func NewTransactionSigner(ctx context.Context, config *config.SignerConfig, chai
 			addr:      common.HexToAddress(config.RemoteSignerConfig.SignerAddress),
 		}, nil
 	case AWSKMSSignerType:
-		ks, err := newKMSSigner(ctx, config.AWSKMSSignerConfig)
+		ks, err := newKMSSigner(ctx, config.AWSKMSSignerConfig, chainID)
 		if err != nil {
 			return nil, fmt.Errorf("failed to create AWS KMS signer, err: %w", err)
 		}
 		return &TransactionSigner{
-			config:      config,
-			kmsSigner:   ks,
-			kmsTxSigner: gethTypes.LatestSignerForChainID(chainID),
-			addr:        ks.address(),
+			config:    config,
+			kmsSigner: ks,
+			addr:      ks.address(),
 		}, nil
 	default:
 		return nil, fmt.Errorf("failed to create new transaction signer, unknown type: %v", config.SignerType)
@@ -109,18 +107,7 @@ func (ts *TransactionSigner) SignTransaction(ctx context.Context, tx *gethTypes.
 		}
 		return signedTx, nil
 	case AWSKMSSignerType:
-		// KMS signs the transaction hash, so we construct the signed tx locally.
-		// This supports every tx type the sender builds, including BlobTx.
-		sig, err := ts.kmsSigner.sign(ctx, ts.kmsTxSigner.Hash(tx).Bytes())
-		if err != nil {
-			log.Info("failed to sign tx with AWS KMS", "address", ts.addr.String(), "err", err)
-			return nil, err
-		}
-		signedTx, err := tx.WithSignature(ts.kmsTxSigner, sig)
-		if err != nil {
-			return nil, fmt.Errorf("failed to apply KMS signature to tx, err: %w", err)
-		}
-		return signedTx, nil
+		return ts.kmsSigner.signTx(ctx, tx)
 	default:
 		// this shouldn't happen, because SignerType is checked during creation
 		return nil, fmt.Errorf("shouldn't happen, unknown signer type")