Draft
Conversation
There was a problem hiding this comment.
Pull request overview
This PR titled "Schnorr signer" represents a major refactoring that removes or comments out most of the TSS (Threshold Signature Scheme) implementation. The changes appear to be preparing for a migration from the existing FROST-based signing to a new Schnorr-based approach, but the implementation is incomplete.
Key Changes:
- Modified the processor to use a new
Response()method instead ofProducedSignature(), but left the handler unimplemented with a panic - Commented out all VAA v1 handling logic instead of removing it
- Deleted numerous test files, utility functions, and internal tooling related to the previous TSS implementation
Reviewed changes
Copilot reviewed 55 out of 56 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| node/pkg/processor/processor.go | Updated threshold signer interface call from ProducedSignature() to Response() but implementation is incomplete with a panic |
| node/pkg/tss/vaav1_handling.go | Entire file commented out (177 lines of VAA v1 handling logic) |
| node/pkg/tss/util.go | File deleted - contained utility functions for TSS operations |
| node/pkg/tss/setup_test.go | File deleted - contained guardian storage tests |
| node/pkg/tss/parse.go | File deleted - contained message parsing logic |
| node/pkg/tss/metrics.go | File deleted - contained signature metrics and monitoring |
| node/pkg/tss/messages.go | File deleted - contained message type definitions |
| node/pkg/tss/internal/timed_heap_test.go | File deleted - contained TTL heap tests |
| node/pkg/tss/internal/timed_heap.go | File deleted - contained TTL heap implementation |
| node/pkg/tss/internal/marshal_test.go | File deleted - contained cryptographic marshaling tests |
| node/pkg/tss/internal/cmd/* | Multiple files deleted - DKG setup tools and examples |
| node/pkg/tss/internal/certs.go | File deleted - certificate utilities |
| node/pkg/tss/implementation_test.go | File deleted - comprehensive integration tests (2122 lines) |
| node/pkg/tss/crypto.go | File deleted - cryptographic hashing utilities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
node/pkg/tss/vaav1_handling.go
Outdated
Comment on lines
3
to
179
| // var errNilGuardianSetState = errors.New("tss' guardianSetState nil") | ||
| // var errNilVaa = errors.New("nil VAA") | ||
| // var errNetworkOutputChannelFull = errors.New("network output channel buffer is full") | ||
|
|
||
| // if t == nil { | ||
| // return errNilTssEngine | ||
| // } | ||
|
|
||
| // if t.started.Load() != started { | ||
| // return errTssEngineNotStarted | ||
| // } | ||
|
|
||
| // // consider removing this check: Going leaderless, and letting everyone send VAAs they see | ||
| // // adds a layer of availability (no need to worry about leader missing VAAs or going offline), | ||
| // // but will spam the network with duplicated VAAs. | ||
| // if !t.isleader { | ||
| // return nil | ||
| // } | ||
|
|
||
| // if v == nil { | ||
| // return errNilVaa | ||
| // } | ||
|
|
||
| // if t.gst == nil { | ||
| // t.logger.Error("issue sending VAAv1 to others", zap.Error(errNilGuardianSetState)) | ||
|
|
||
| // return errNilGuardianSetState | ||
| // } | ||
|
|
||
| // dgst := digest{} | ||
| // copy(dgst[:], v.SigningDigest().Bytes()) | ||
|
|
||
| // if v.Version != vaa.VaaVersion1 { | ||
| // // not an error. but will not accept. | ||
| // return nil | ||
| // } | ||
|
|
||
| // gs := t.gst.Get() | ||
| // if err := v.Verify(gs.Keys); err != nil { | ||
| // return nil // won't send invalid VAAs. | ||
| // } | ||
|
|
||
| // bts, err := v.Marshal() | ||
| // if err != nil { | ||
| // err := fmt.Errorf("failed to marshal VAA: %w", err) | ||
|
|
||
| // t.logger.Error("issue sending VAAv1 to others", zap.Error(err)) | ||
| // return err | ||
| // } | ||
|
|
||
| // send := Unicast{ | ||
| // Unicast: &tsscommv1.Unicast{ | ||
| // Content: &tsscommv1.Unicast_Vaav1{ | ||
| // Vaav1: &tsscommv1.VaaV1Info{ | ||
| // Marshaled: bts, | ||
| // }, | ||
| // }, | ||
| // }, | ||
| // Receipients: t.GuardianStorage.Identities, // sending to all guardians. | ||
| // } | ||
|
|
||
| // select { | ||
| // case t.messageOutChan <- &send: | ||
| // t.logger.Info("informed all guardians on vaav1", | ||
| // zap.String("chainID", v.EmitterChain.String()), | ||
| // zap.String("digest", fmt.Sprintf("%x", v.SigningDigest())), | ||
| // ) | ||
| // default: | ||
| // t.logger.Error("issue sending VAAv1 to others", zap.Error(errNetworkOutputChannelFull)) | ||
| // } | ||
|
|
||
| // return nil | ||
| // } | ||
|
|
||
| // var errNilGuardianSet = fmt.Errorf("nil guardian set") | ||
| // var errNotVaaV1 = fmt.Errorf("not a v1 VAA") | ||
|
|
||
| // // handleUnicastVaaV1 expects to receive valid Vaav1 messages. | ||
| // // If the VAA is valid, it will trigger the TSS signing protocol too for that VAA (beginTSSSign, will ensure double signing for the same digest). | ||
| // func (t *Engine) handleUnicastVaaV1(v *tsscommv1.Unicast_Vaav1) error { | ||
| // if t == nil { | ||
| // return errNilTssEngine | ||
| // } | ||
|
|
||
| // if t.started.Load() != started { | ||
| // return errTssEngineNotStarted | ||
| // } | ||
|
|
||
| // if t.gst == nil { | ||
| // return errNilGuardianSetState | ||
| // } | ||
|
|
||
| // gs := t.gst.Get() | ||
| // if gs == nil { | ||
| // return errNilGuardianSet | ||
| // } | ||
|
|
||
| // if v == nil || v.Vaav1 == nil { | ||
| // return errNilVaa | ||
| // } | ||
|
|
||
| // newVaa, err := vaa.Unmarshal(v.Vaav1.Marshaled) | ||
| // if err != nil { | ||
| // return fmt.Errorf("unmarshal err: %w", err) | ||
| // } | ||
|
|
||
| // dgst := digest{} | ||
| // copy(dgst[:], newVaa.SigningDigest().Bytes()) | ||
|
|
||
| // if newVaa.Version != vaa.VaaVersion1 { | ||
| // return errNotVaaV1 | ||
| // } | ||
|
|
||
| // if err := newVaa.Verify(gs.Keys); err != nil { | ||
| // return err | ||
| // } | ||
|
|
||
| // signatureMeta := signingMeta{ | ||
| // isFromVaav1: true, | ||
| // verifiedVAAv1: newVaa, | ||
| // } | ||
|
|
||
| // return t.beginTSSSign(dgst[:], newVaa.EmitterChain, newVaa.ConsistencyLevel, signatureMeta) | ||
| // } | ||
|
|
||
| // var errCouldNotMapAllVaaV1Signers = fmt.Errorf("could not map all VAAv1 signers to senderIndexes") | ||
|
|
||
| // func (t *Engine) translateVaaV1Signers(v *vaa.VAA) (map[SenderIndex]*Identity, error) { | ||
| // gs := t.gst.Get() | ||
| // // translating the VAAv1 signers to senderIndexes to use. | ||
| // signersID := make(map[SenderIndex]*Identity, len(gs.Keys)) | ||
| // for _, s := range v.Signatures { | ||
| // if int(s.Index) >= len(gs.Keys) { | ||
| // // shouldn't happen, since the signature was verified. but just in case. | ||
| // return nil, fmt.Errorf("signature index %d out of bounds for guardian set size %d", s.Index, len(gs.Keys)) | ||
| // } | ||
|
|
||
| // id, err := t.GuardianStorage.fetchIdentityFromVaav1Pubkey(gs.Keys[s.Index]) | ||
| // if err != nil { | ||
| // return nil, errCouldNotMapAllVaaV1Signers | ||
| // } | ||
|
|
||
| // signersID[id.CommunicationIndex] = id | ||
| // } | ||
|
|
||
| // if len(signersID) != len(v.Signatures) { | ||
| // // make into an error: | ||
| // return nil, errCouldNotMapAllVaaV1Signers | ||
| // } | ||
|
|
||
| // return signersID, nil | ||
| // } | ||
|
|
||
| // func (t *Engine) getExcludedFromCommittee(mt signingMeta) []*common.PartyID { | ||
| // if !mt.isFromVaav1 || mt.verifiedVAAv1 == nil { | ||
| // return nil | ||
| // } | ||
|
|
||
| // signersID, err := t.translateVaaV1Signers(mt.verifiedVAAv1) | ||
| // if err != nil { | ||
| // return nil | ||
| // } | ||
|
|
||
| // if len(signersID) < t.GuardianStorage.Threshold { | ||
| // return nil // not enough guardians to form a committee. | ||
| // } | ||
|
|
||
| // // grab everyone that is not a signer in the VAAv1. | ||
| // var excludedSigners []*common.PartyID | ||
| // for _, id := range t.GuardianStorage.Identities { | ||
| // if _, ok := signersID[id.CommunicationIndex]; !ok { | ||
| // excludedSigners = append(excludedSigners, id.Pid) | ||
| // } | ||
| // } | ||
|
|
||
| // return excludedSigners | ||
| // } |
There was a problem hiding this comment.
The entire file content is commented out rather than deleted. If this code is no longer needed, it should be removed entirely from the codebase. If it's still needed, it should remain uncommented. Leaving large blocks of commented code reduces code maintainability.
node/pkg/processor/processor.go
Outdated
Comment on lines
393
to
395
| // p.processTssSignature(sig) // TODO: | ||
| _ = sig | ||
| panic("unimplemented") |
There was a problem hiding this comment.
This panic will cause the application to crash at runtime when a threshold signature is received. The implementation should be completed or this code path should be properly handled to avoid production crashes.
Suggested change
| // p.processTssSignature(sig) // TODO: | |
| _ = sig | |
| panic("unimplemented") | |
| // TODO: implement proper threshold signature processing, e.g. p.processTssSignature(sig). | |
| // For now, log and ignore to avoid crashing the node on receipt of a threshold signature. | |
| p.logger.Warn("received threshold signature response but processing is not implemented; ignoring") | |
| _ = sig |
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
fx: tss used wrong pubsub topic in p2p package fx: removed redundant check
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.