Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow the user to use custom signing scheme and multiple keys in vtxo tree signatures #422

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

louisinger
Copy link
Collaborator

@louisinger louisinger commented Jan 15, 2025

This PR adds a way for the user to sign only its branch during the vtxo tree signing process.

now RegisterInputsForNextRound RPC expects the following request:

message RegisterInputsForNextRoundRequest {
  repeated Input inputs = 1;
  repeated string signer_pubkeys = 2;
  repeated string notes = 3;
  uint32 signing_type = 4;
}
  • ephemeral_pubkey has been replaced by signer_pubkeys. It allows setting multiple signing keys for the same request. Useful for collaborative VTXO contracts.
  • signing_type is a flag signalling to the server the type of signing scheme the user wants to use, 2 are supported:
    • SignAll = 0 is the existing scheme where one signs all branches of the tree.
    • SignBranch = 1 is the new scheme where one signs only the branch(es) related to his VTXO(s) in the tree.

BREAKING CHANGES:

With this, the tree's PSBTs don't embed the sweep tapscript leaf and internal taproot key as unknown fields anymore. They are replaced instead with the round lifetime, as clients know all other info to reconstruct the sweep leaf on their own.
All clients must be updated once this is merged.

@tiero @altafan @sekulicd please review


AI TL;DR

Looking at the changes in the PR, the main changes are:

  1. Refactoring of the signing process for vtxo trees:

    • Removed the single ephemeral key concept
    • Introduced multiple signer keys and signing types (ALL or BRANCH)
    • Added better error handling and validation
  2. Changes to the signing protocol:

    • Each vtxo leaf can now be signed by different signers
    • Added support for signing ALL branches or just specific branches
    • Server now generates its own signing key pair
    • Simplified coordinator session by removing unnecessary error returns
  3. Core API changes:

    • Modified RegisterInputsForNextRound RPC to take signer_pubkeys and signing_type instead of ephemeral_pubkey
    • Updated vtxo leaf format to include signer pubkeys and signing type
    • Removed cosigners_pubkeys from RoundSigningEvent

@louisinger louisinger requested a review from altafan January 15, 2025 16:38
repeated string cosigners_pubkeys = 2;
Tree unsigned_vtxo_tree = 3;
string unsigned_round_tx = 4;
Tree unsigned_vtxo_tree = 2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's keep the list of cosigners, this way it's easier for clients to understand if they are participating this round or have to wait for the next one.

common/locktime.go Outdated Show resolved Hide resolved
common/tree/type.go Outdated Show resolved Hide resolved
UnsignedTree tree.VtxoTree
CosignersPubKeys []*secp256k1.PublicKey
UnsignedRoundTx string
ID string
Copy link
Collaborator

Choose a reason for hiding this comment

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

revert changes

}

// WithCustomTreeSigner allows to use a set of custom signer for the vtxo tree signing process
func WithCustomTreeSigner(privKeys []*secp256k1.PrivateKey) Option {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I took a look at handleOptions and i think this should be come WithExtraSigner to be useful for the user.. this way he just need to add extra keys, without taking care of creating one for himself, that should always be done by the SDK.

Suggested change
func WithCustomTreeSigner(privKeys []*secp256k1.PrivateKey) Option {
func WithExtraSigner(privKeys []*secp256k1.PrivateKey) Option {

}

// if no custom signer priv keys are provided, we generate a new ephemeral key
if len(signerPubKeys) == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

in light of this comment, we should always generate the keypair for the sdk wallet

if err = signerSession.SetKeys(event.CosignersPubKeys); err != nil {
return
}
for _, privKey := range signerPrivKeys {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be done in parallel?

if err := signerSession.SetAggregatedNonces(event.Nonces); err != nil {
return err
}
for _, session := range signerSessions {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here?

Comment on lines +2794 to +2797
ephemeralKey, err := secp256k1.GeneratePrivateKey()
if err != nil {
return nil, nil, tree.SignBranch, err
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we should use an ephemeral key only when signing ALL, otherwise we should use a wallet key to be sure we can re-sign the branch if needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants