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

lnrpc: add NoHash option to signer.SignMessage rpc #8098

Conversation

orbitalturtle
Copy link
Contributor

This PR adds the option to not hash the message before signing it with signer.SignMessage. I'm running into a scenario where the message I need to pass into this API call is already hashed.

I'm need this when needing a Schnorr signature of the message, so I made the change for that particular circumstance. But if this would be useful in a non-Schnorr scenario as well, I'd be happy to expand the change.

Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Though I'm not sure this is safe. Can you add more context to why the message can't be given to the RPC in a non-hashed form?

lnrpc/signrpc/signer.proto Show resolved Hide resolved
keychain/btcwallet.go Show resolved Hide resolved
@orbitalturtle
Copy link
Contributor Author

@guggero Gotcha, that makes sense, thanks for taking a look. So I'm working on this for LNDK. We're currently working on supporting paying bolt 12 offers, and part of the process is to sign the invoice request... the data LDK needs to pass to LND is already hashed. I had first looked into changing this on LDK's side -- to try to pass in an unhashed value into the SignMessage api. But the hashing formula for is a bit more complicated just a SHA256 (it requires a merkle root as well alongside it) https://github.com/lightningdevkit/rust-lightning/blob/6cafba9f5bc5c67d2ba302d2f53082ef22685b42/lightning/src/offers/merkle.rs#L104 so it doesn't quite work.

Just brainstorming here but... Would it be safer if the noHash option can only be taken on a particular piece of data that can be deserialized into a struct on LND's side? I suppose another option would be to support the full bolt 12 hash making process on LND's side instead.

Oi, and sorry for the red X marks in the CI checks. I'll fix those up once we discuss what the best option is here. :)

@guggero
Copy link
Collaborator

guggero commented Oct 17, 2023

Ah, so it's just a tagged hash? I guess then instead of allowing the message not to be hashed, we could allow the tag to be set that should be used when hashing. Then we just need to disallow any specific BIP-340 tags ("BIP0340/*") to avoid transaction hashes to be signed.

@orbitalturtle
Copy link
Contributor Author

@guggero Sorry for the slow reply, was making sure I can refactor to expose the tag data in LDK. Looks like it works so I'll rework this PR in that direction :)

@orbitalturtle
Copy link
Contributor Author

Replacing this with a new PR to use a more accurate branch name.

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