Skip to content

signer_client: correctly use 32-byte key for schnorr #225

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

Merged
merged 1 commit into from
Jun 2, 2025

Conversation

guggero
Copy link
Member

@guggero guggero commented Jun 2, 2025

This was incorrect all along, seems like we haven't actually used the VerifySchnorr() option in any project yet.

This is needed for lightninglabs/taproot-assets#1502.

Pull Request Checklist

  • PR is opened against correct version branch.
  • Version compatibility matrix in the README and minimal required version
    in lnd_services.go are updated.
  • Update macaroon_recipes.go if your PR adds a new method that is called
    differently than the RPC method it invokes.

This was incorrect all along, seems like we haven't actually used the
VerifySchnorr option in any project yet.
@guggero guggero requested review from bhandras and GeorgeTsagk June 2, 2025 15:42
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

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

LGTM, one nit

// If the signature is a Schnorr signature, we need to set the public
// key as the 32-byte x-only key, as mentioned in the RPC docs.
if rpcIn.IsSchnorrSig {
rpcIn.Pubkey = pubkey[1:]
Copy link
Member

Choose a reason for hiding this comment

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

could also check for the len(pubkey) here to avoid a potential panic?

Copy link
Member

@ellemouton ellemouton Jun 2, 2025

Choose a reason for hiding this comment

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

i dont think it can panic. The param is [33]byte and so is guaranteed to have 33 bytes

@guggero guggero merged commit fe0bdd6 into master Jun 2, 2025
1 check passed
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.

4 participants