-
Notifications
You must be signed in to change notification settings - Fork 422
Make payment_key derivation deterministic
#3391
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
Closed
tnull
wants to merge
10
commits into
lightningdevkit:main
from
tnull:2024-09-deterministic-payment-point
Closed
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
6dfe913
Drop `InMemorySigner` `Writeable` impl
tnull 75c2b0a
Rename `payment_key` to `payment_basepoint` in `chan_utils`
tnull 611c1a3
Rename `params` to `channel_keys_id` and take it by value
tnull 6be731e
Rename `keys_id` to `channel_keys_id`
tnull ed2d63e
Rename `payment_point` to `payment_basepoint`
tnull aea8e5e
Make `payment_key` derivation deterministic
tnull a781d23
f Adjust test cases to accommodate new derivation scheme
tnull bf14ec8
f Allow legacy keys derivation for `test_restored_packages_retry`
tnull c70f7ec
Have `SignerProvider::get_shutdown_scriptpubkey` take `channel_keys_id`
tnull 8991d74
Drop outdated comment
tnull File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: doesn't this derivation create the same
payment_keyacross all theInMemorySigner's derived by thisKeysManager?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is essentially the intended outcome. Or rather, we want to be able to derive a payment key without including additional data such as channel keys id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I'm concerned about a privacy leak here - in case of force close of two channels onchain, these commit tx would both have the same scriptpubkey in the
to_remoteoutput right ?I know CLN
hsm_toolallows people to recover theirto_remoteoutputs via a trial-and-error method, we could take a look at how they derive theirpayment_key.See
hsmtoolguesstoremote <P2WPKH address> <node id> <tries> "<path/to/hsm_secret>"There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That
guesstoremotemethod is intended to be used in case the local party loses all channel state, and the remote party force closes the channel - the same scenario addressed in this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more clarification:
<node id>in that method is the node id of the remote party, not the local party.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is true and a good point, although note we currently have that issue with
destination/shutdownscripts, too. Though we plan to change this with #1139 which I considered to pick up as part / as a follow-up of this PR.I guess if we manage to completely isolate the
payment_keyderivation to a interface method we could consider including thecounterparty_node_id, or, even better have the user override that method so that it returns a fresh address from their wallet, which would also get rid of the additional spending step forStaticPaymentOutputs. We considered that refactor before, but so far intended to punt on it as it's likely going to be a larger one and maintaining backwards compatibility for all of this might get tricky. But yeah, maybe you're right and we should try to do it 'the right way' right away.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see ! I had overlooked this code :) I let you determine the best path forward here.
Can't the user already decide how
payment_keyis derived inSignerProvider::derive_channel_signer?We are also constrained by the LN spec if the channel has anchors. But I agree that it would be very cool in the non-anchor case to have the funds land directly in the on-chain wallet from the commitment transaction.
I took a brief look at the API of BDK's
Wallet, and they don't offer an easy way to ask for raw public keys - just addresses - which makes sense considering that the core of the wallet is an output descriptor.Your call :)