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 tagged hash option to signer.SignMessage/VerifyMessage rpcs #8106

Merged

Conversation

orbitalturtle
Copy link
Contributor

@orbitalturtle orbitalturtle commented Oct 21, 2023

Adds an option to the signrpc.SignMessage rpc to sign a tagged hash of a message. Adds the same option to VerifyMessage so the signature can be properly verified. Replaces #8098.

@guggero guggero self-requested a review October 23, 2023 09:48
@ellemouton ellemouton self-requested a review October 23, 2023 09:56
Copy link
Collaborator

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

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

Thanks so much for this! This will be super useful for signing future gossip messages too 💪

lnrpc/signrpc/signer_server.go Outdated Show resolved Hide resolved
lnrpc/signrpc/signer_server.go Outdated Show resolved Hide resolved
lnrpc/signrpc/signer_server.go Outdated Show resolved Hide resolved
keychain/btcwallet.go Outdated Show resolved Hide resolved
lntest/mock/secretkeyring.go Outdated Show resolved Hide resolved
lnwallet/rpcwallet/rpcwallet.go Outdated Show resolved Hide resolved
@orbitalturtle orbitalturtle force-pushed the sign-tagged-hash-rpc branch 3 times, most recently from b50b879 to 1ee0a8b Compare October 24, 2023 23:32
Copy link
Collaborator

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

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

Really nice! Just a few nits from my side. Other than those, LGTM!

lnrpc/signrpc/signer.proto Outdated Show resolved Hide resolved
lnrpc/signrpc/signer_server.go Outdated Show resolved Hide resolved
lnrpc/signrpc/signer_server.go Show resolved Hide resolved
keychain/btcwallet.go Outdated Show resolved Hide resolved
itest/lnd_signer_test.go Outdated Show resolved Hide resolved
itest/lnd_signer_test.go Outdated Show resolved Hide resolved
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.

Looks good, pending @ellemouton's and my nits.

Since this is security related, I'm going to pull in @Crypt-iQ as well. Just to make sure we don't allow the user to shoot themselves in the foot with this.

keychain/btcwallet.go Outdated Show resolved Hide resolved
@guggero guggero requested a review from Crypt-iQ October 27, 2023 09:25
Copy link
Collaborator

@Crypt-iQ Crypt-iQ left a comment

Choose a reason for hiding this comment

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

LGTM once release notes fixed 🏎️

docs/release-notes/release-notes-0.18.0.md Outdated Show resolved Hide resolved
@orbitalturtle
Copy link
Contributor Author

Thanks for the reviews :) Fixed those nits and the release notes problem

Copy link
Collaborator

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

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

Very nice! 🔥

@saubyk saubyk added this to the v0.18.0 milestone Nov 6, 2023
@Roasbeef Roasbeef merged commit dacb86f into lightningnetwork:master Nov 7, 2023
24 of 25 checks 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.

6 participants