Signer leader by jonathanMweiss · Pull Request #30 · XLabs/wormhole

jonathanMweiss · 2026-01-14T17:23:53Z

No description provided.

additional unit tests tssmock mocking + processor handling tmp unit-test for tss signature making inside processor change api wip: node receives the tss engine fix: leader mechanism to use ethaddress instead of indices connecting the node

Copilot

Pull request overview

This PR implements a leader-based protocol for TSS (Threshold Signature Scheme) signing where a designated leader guardian can forward VAAv1 messages with specific committees to other guardians for signing. The changes remove the old tsscomm protocol and replace it with a gossip-based approach.

Changes:

Removes the old tsscomm protocol service and replaces it with a gossip-based TSS communication mechanism
Implements leader-based VAA forwarding where leaders publish VAAv1 messages via gossip
Adds comprehensive test coverage for TSS gossip functionality and processor handling

Reviewed changes

Copilot reviewed 26 out of 27 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
proto/tsscomm/v1/tsscomm.proto	Removes entire tsscomm proto definition
proto/gossip/v1/gossip.proto	Adds TSSGossipMessage to gossip protocol
node/pkg/tss/gossiper.go	Implements leader VAA witnessing and gossip message handling
node/pkg/tss/client.go	Refactors SignerClient to support VAA handling
node/pkg/tss/api.go	Defines Gossiper interface and updates Signer interface
node/pkg/processor/processor.go	Implements TSS response handling and signature processing
node/pkg/p2p/p2p.go	Adds TSS gossip topic support
node/cmd/guardiand/node.go	Updates CLI flags for TSS configuration

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-14T17:32:54Z

+	p.logger.Info("TSS signing request stopped",
+		zap.String("hash", hash),
+		zap.String("status", status.GetProtocol()),
+		zap.String("code", codes.Code(status.GetCode()).String()),
+		zap.String("message", status.GetMessage()),
+	)
+
+	// TODO: consider additional handling based on status code


The TODO suggests that different status codes should be handled differently (e.g., retries for transient failures vs. permanent failures), but this logic is missing. Consider implementing appropriate error handling based on status codes.

Suggested change

p.logger.Info("TSS signing request stopped",

zap.String("hash", hash),

zap.String("status", status.GetProtocol()),

zap.String("code", codes.Code(status.GetCode()).String()),

zap.String("message", status.GetMessage()),

)

// TODO: consider additional handling based on status code

code := codes.Code(status.GetCode())

p.logger.Info("TSS signing request stopped",

zap.String("hash", hash),

zap.String("status", status.GetProtocol()),

zap.String("code", code.String()),

zap.String("message", status.GetMessage()),

)

// Additional handling based on status code. This is intentionally limited to

// logging/diagnostics to avoid changing existing functional behavior.

switch code {

case codes.Unavailable, codes.DeadlineExceeded, codes.ResourceExhausted, codes.Aborted:

// These errors are often transient and may be retried by the caller.

p.logger.Warn("transient TSS signer error; request may be retried by caller",

zap.String("hash", hash),

zap.String("status", status.GetProtocol()),

zap.String("code", code.String()),

zap.String("message", status.GetMessage()),

)

case codes.Canceled:

// The signing request was canceled (e.g., by the client); no retry implied.

p.logger.Debug("TSS signing request was canceled",

zap.String("hash", hash),

zap.String("status", status.GetProtocol()),

zap.String("code", code.String()),

zap.String("message", status.GetMessage()),

)

case codes.OK:

// A successful status should normally be represented as a signature response,

// but if it appears here, no additional action is required.

default:

// Non-transient, non-OK errors are considered non-recoverable.

p.logger.Error("non-recoverable TSS signer error",

zap.String("hash", hash),

zap.String("status", status.GetProtocol()),

zap.String("code", code.String()),

zap.String("message", status.GetMessage()),

)

}

fx: tss used wrong pubsub topic in p2p package fx: removed redundant check

yossigi · 2026-01-24T09:38:50Z

+	if p.GuardianSigner == nil {
+		return nil, errors.New("guardian signer must not be nil")
+	}
+	if p.SocketPath == "" {


should this also make sure the p.LeaderAddress is in the guardianset?

I'm not sure it is relevant. When this function actually runs it is possible a valid guardianSetState doesn't exist yet.

Copilot

Pull request overview

Copilot reviewed 28 out of 29 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-25T14:44:31Z

+func (dt vaaHandling) verifyGossipSig(msg *gossipv1.TSSGossipMessage, gs *common.GuardianSet) error {
+	pubKey, err := ethcrypto.Ecrecover(ethcrypto.Keccak256(msg.Message), msg.Signature)
+	if err != nil {
+		return fmt.Errorf("failed to recover public key: %w", err)
+	}
+
+	signerAddr := ethcommon.BytesToAddress(ethcrypto.Keccak256(pubKey[1:])[12:])
+
+	if signerAddr != dt.leaderAddress {
+		return fmt.Errorf("signature not from leader: got %s, want %s", signerAddr.Hex(), dt.leaderAddress.Hex())
+	}
+
+	if _, ok := gs.KeyIndex(signerAddr); !ok {
+		return fmt.Errorf("leader address %s not in guardian set", signerAddr.Hex())
+	}


The signature recovery process appears incomplete. The code recovers the public key and derives an address, but the comparison with dt.leaderAddress seems insufficient for proper authentication. The recovered address should ideally be cross-verified with the guardian set to ensure it's a valid guardian, not just the leader. Additionally, the signature format should be validated to ensure it follows the expected ECDSA signature structure (65 bytes with recovery ID).

this line if _, ok := gs.KeyIndex(signerAddr); !ok { ensures the guardian is part of the guardian set.
The ecrecover checks the structure of the signature.

Copilot · 2026-01-25T14:44:32Z

+		case s.conn.signResponses <- resp: // forward response to consumer.
+		default:
+			// drop response if channel is full to avoid blocking. This is not ideal, but prevents deadlocks.
+			// log as an error, since it indicates that the consumer is not keeping up.
+			logger.Error("signResponses channel full, dropping response", zap.Stringer("response", resp))
 		}


When the channel is full, the code logs an error but drops the response silently. This could lead to lost signatures in high-throughput scenarios. Consider using a buffered channel with monitoring/alerting when approaching capacity, or implementing backpressure to slow down the sender rather than silently dropping critical signature responses.

Copilot · 2026-01-25T14:44:32Z

 	p.storeSignedVAA(signed)
-	p.thresholdSigner.WitnessNewVaa(signed)
+	// TODO: handle ctx.
+	if err := p.thresholdSigner.WitnessNewVaaV1(context.Background(), signed); err != nil {


Using context.Background() here bypasses the cancellation chain and could cause the WitnessNewVaaV1 call to continue even after the processor is shutting down. Consider passing through the context from the HandleQuorum caller or maintaining a processor-level context that gets canceled on shutdown.

Copilot · 2026-01-25T14:44:32Z

+func (s *SignerClient) gossipListener(ctx context.Context, logger *zap.Logger) {
+	logger = logger.Named("gossipListener")
+	dt := s.vaaData
+	for {
+		select {
+		case <-ctx.Done():
+			return
+		case msg := <-dt.incomingGossip:
+			gs := dt.gst.Get()
+			if gs == nil {
+				logger.Debug("nil guardian set")
+				continue
+			}
+
+			if err := dt.verifyGossipSig(msg, gs); err != nil {
+				logger.Warn("invalid gossip signature", zap.Error(err))
+				continue
+			}
+
+			newVaa, err := vaa.Unmarshal(msg.Message)
+			if err != nil {
+				logger.Warn("malformed VAA", zap.Error(err))
+				continue
+			}
+
+			if newVaa.Version != vaa.VaaVersion1 {
+				continue
+			}
+
+			if err := newVaa.Verify(gs.Keys); err != nil {
+				logger.Warn("invalid VAA", zap.Error(err))
+				continue
+			}
+
+			if err := s.vaaToSignRequest(newVaa, gs); err != nil {
+				logger.Error("failed to create sign request from VAA", zap.Error(err))
+				continue
+			}
+		}
+	}
+}


The gossipListener goroutine doesn't have any mechanism to signal that it has started or is running. If the goroutine fails to start or exits unexpectedly, there's no way for the caller to detect this. Consider using an error channel or a sync.WaitGroup to ensure the goroutine has started successfully before proceeding.

yossigi · 2026-01-26T19:39:51Z

+		rq.Committee = append(rq.Committee, key)
+	}
+
+	if len(rq.Committee) < s.configurations.ThresholdSize {


Should this support ThresholdSize == 0? If not, can assert it when loading a config

yossigi · 2026-01-26T19:45:55Z

-	return &signerClient{}
+type Parameters struct {
+	// configurations for the signer client.
+	Configurations `json:"configurations"`


Is the Parameters struct serialized somewhere? (using the json tags)

…uncs

jonathanMweiss added 7 commits January 7, 2026 06:25

wip: observing VAAv1

497ff04

vaav1 leader's witness and gossip

4c56d93

tssGossiper api. toward leader

19cb522

gossipListener

f026f2a

p2p signerconnection integration

8a3d56a

p2p integration

756b3e5

signer integrated fully to guardian.

0caf760

additional unit tests tssmock mocking + processor handling tmp unit-test for tss signature making inside processor change api wip: node receives the tss engine fix: leader mechanism to use ethaddress instead of indices connecting the node

jonathanMweiss requested review from Copilot and yossigi January 14, 2026 17:23

jonathanMweiss self-assigned this Jan 14, 2026

Copilot started reviewing on behalf of jonathanMweiss January 14, 2026 17:24 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

fx: p2p unit test for TssGossipListener

bced272

fx: tss used wrong pubsub topic in p2p package fx: removed redundant check

yossigi requested changes Jan 24, 2026

View reviewed changes

jonathanMweiss added 4 commits January 25, 2026 07:05

added unary call for key update

4484bad

fix: verifyGossipSig ensures leader is in current guardianSet

7944431

fix nits: import with two different names and debug log

d006ee6

fix: ensure guardian set is not nil

dfb475f

jonathanMweiss marked this pull request as ready for review January 25, 2026 10:21

jonathanMweiss added 8 commits January 25, 2026 10:23

added comment

cd45545

context.TODO removed

866d244

dropping incoming messages from server (with log) on full channels.

44f0757

improved unit tests

14d5f01

adding config to the signer client

4ba956c

integrating protocol fetching from signer config

f2cd165

fix: gossiper checks committee size

397c2b2

removed unused value from proto

733dc47

jonathanMweiss requested review from Copilot and yossigi January 25, 2026 14:36

Copilot started reviewing on behalf of jonathanMweiss January 25, 2026 14:37 View session

Copilot AI reviewed Jan 25, 2026

View reviewed changes

jonathanMweiss added 4 commits January 25, 2026 14:54

removed unneeded line

846f9bc

removed unneeded line

bcc36ba

removed double space from error message

5dd38d0

removed todo

9827142

yossigi approved these changes Jan 26, 2026

View reviewed changes

jonathanMweiss added 5 commits January 27, 2026 06:03

fix comment

54ce082

removed context.Background, passing ctx from Processor.Run to inner f…

5eb5633

…uncs

removed todo

c4dc0ab

removed empty line

e950081

allowing leader from outside guardianset. changing comments

758d480

jonathanMweiss merged commit da59334 into schnorr-signer Jan 27, 2026

jonathanMweiss deleted the signer-leader branch January 27, 2026 08:54

Conversation

jonathanMweiss commented Jan 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yossigi Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

jonathanMweiss Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

jonathanMweiss Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yossigi Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yossigi Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants