Make `payment_key` derivation deterministic #3391

tnull · 2024-10-31T10:33:22Z

Previously, KeysManager::derive_channel_keys would derive the channel's payment_key uniquely on a per-channel basis which would disallow users losing their channel_keys_id to recover funds. As there is no necessity to have payment_key derivation depend on channel_keys_id we can allow for easier recovery of any non-HTLC encumbered funds if we make payment_key derivation deterministic.

Here we do just this but also include a larger refactor introducing a ChannelKeysDerivationParameters struct holding a versioning field to be able to express this change in the derivation scheme. To this end we ensure that legacy channels will continue to use the old derivation scheme after upgrade, while new channels will derive the payment_key
deterministicly.

To this end, we use the first byte of channel_keys_id as a versioning byte indicating the version of the used channel keys derivation scheme. Note that Previously KeysManager::generate_channel_keys_id would with
very high likelyhood never have generated a channel_keys_id with a non-null first byte, which makes this a backwards-compatible change for any users that didn't run custom implementations of
SignerProvider::generate_channel_keys_id conflicting with this assumption.

Apart from this, we add some refactoring commits that drop unused logic (Writeable for InMemorySigner) and rename fields to align them with the BOLTs for clarity.

We previously also discussed abusing the first byte of channel_keys_id as a version byte, which wouldn't require the overhead of introducing ChannelKeysDerivationParameters and serializing the channel_keys_derivation_version field independently everywhere. However, as discussed I now chose to go with the latter approach as it seems cleaner and more future-proof.

~~That said, looking for an approach ACK here before continuing. Will be draft until then.~~

Should be good for review

TODO:

Fix one remaining failing test case.
Add test case for (partial) recovery from seed only.
Include commit adding a channel_keys_id argument to get_shutdown_scriptpubkey, just as for the destination case. (cc @alecchendev)

... which we haven't been using since 0.0.119 / commit 7a951b1.

.. as it doesn't use the actual signer's `payment_key`, but the associated public key.

.. to make the name more clear and since before we `clone` it in `derive_channel_keys` anyways.

tnull · 2024-11-06T11:39:18Z

After discussion this offline I now reverted this to simply use the first byte of channel_keys_id as a versioning field which considerably reduces the diff size. Note however that this approach might have backwards compatibility issues if users would have custom implementations of SignerProvider::generate_channel_keys_id for which the always-0-first-byte assumption doesn't hold.

Furthermore, this change will of course prohibit downgrading channel monitors to use the older derivation version. This is also the reason why currently monitor_tests::test_restored_packages_retry was failing before. Now added a workaround to use the legacy keys derivation for this test.

codecov · 2024-11-06T11:46:56Z

Codecov Report

Attention: Patch coverage is 94.95798% with 6 lines in your changes missing coverage. Please review.

Project coverage is 89.72%. Comparing base (8c086c7) to head (8991d74).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/sign/mod.rs	94.82%	2 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	81.81%	2 Missing ⚠️
lightning/src/util/test_utils.rs	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3391      +/-   ##
==========================================
+ Coverage   89.66%   89.72%   +0.05%     
==========================================
  Files         128      128              
  Lines      104887   105577     +690     
  Branches   104887   105577     +690     
==========================================
+ Hits        94052    94734     +682     
- Misses       8126     8140      +14     
+ Partials     2709     2703       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

.. to align the field everywhere.

.. to align the field naming with the spec for clarity.

Previously, `KeysManager::derive_channel_keys` would derive the channel's `payment_key` uniquely on a per-channel basis which would disallow users losing their `channel_keys_id` to recover funds. As it's no real necessity to have `payment_key` derivation depend on `channel_keys_id` we can allow for easier recovery of any non-HTLC encumbered funds if we make `payment_key` derivation deterministic. To this end, we use the first byte of `channel_keys_id` as a versioning byte indicating the version of the used channel keys derivation scheme. Note that Previously `KeysManager::generate_channel_keys_id` would with very high likelyhood never have generated a `channel_keys_id` with a non-null first byte, which makes this a backwards-compatible change for any users that didn't run custom implementations of `SignerProvider::generate_channel_keys_id` conflicting with this assumption.

Some test cases have hard-coded values which we change here (to be squashed in after review).

.. to allow users to return specific scripts per channel.

In 6a55dcc we fixed an issue where we'd just `unwrap` `InMemorySigner`'s `channel_parameters` field and added a comment explaining why we can't do that. However, in particular as related APIs were also changed in follow-up commits to return `Option`s, the introduced comment is now outdated and doesn't really make sense anymore without that historical context present. As it's more or less misleading by now, we just drop it.

TheBlueMatt

I know we discussed an alternative version where we use context to know which key type to derive, let me know when you've tried that to see if its practical.

TheBlueMatt · 2024-11-25T16:25:25Z

lightning/src/ln/chan_utils.rs

 	} else {
-		ScriptBuf::new_p2wpkh(&WPubkeyHash::hash(&payment_key.serialize()))
+		ScriptBuf::new_p2wpkh(&WPubkeyHash::hash(&payment_basepoint.serialize()))


I'm quite confused by this. The point of calling it a "key" and not a "basepoint" is that the script is paid directly to the key (in this case as a P2WPKH) rather than incorporating the per_commitment_secret/per_commitment_point when building t the output.

TheBlueMatt · 2024-11-25T16:33:48Z

lightning/src/ln/chan_utils.rs

@@ -478,7 +478,7 @@ pub struct ChannelPublicKeys {
 	/// The public key on which the non-broadcaster (ie the countersignatory) receives an immediately
 	/// spendable primary channel balance on the broadcaster's commitment transaction. This key is
 	/// static across every commitment transaction.
-	pub payment_point: PublicKey,
+	pub payment_basepoint: PublicKey,


I think we discussed this elsewhere but IMO we should fix this in the spec, not here. The "base" in the "basepoint"s refer to the fact that its extended with the per_commitment_point to build a new point for each state. Since static_remotekey (which is now "ASSUMED" in the spec, and not described) its no longer used to derive a key but rather simply a key used as-is. The spec needs updating to remove references to its use as a base key.

tankyleo · 2024-12-11T16:49:31Z

lightning/src/sign/mod.rs

+				sha.input(&b"static payment key"[..]);
+				SecretKey::from_slice(&Sha256::from_engine(sha).to_byte_array())
+					.expect("SHA-256 is busted")
+			};


question: doesn't this derivation create the same payment_key across all the InMemorySigner's derived by this KeysManager ?

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.

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_remote output right ?

I know CLN hsm_tool allows people to recover their to_remote outputs via a trial-and-error method, we could take a look at how they derive their payment_key.

See hsmtool guesstoremote <P2WPKH address> <node id> <tries> "<path/to/hsm_secret>"

That guesstoremote method 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.

One more clarification: <node id> in that method is the node id of the remote party, not the local party.

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_remote output right ?

Yes, this is true and a good point, although note we currently have that issue with destination/shutdown scripts, 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_key derivation to a interface method we could consider including the counterparty_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 for StaticPaymentOutputs. 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.

Yes, this is true and a good point, although note we currently have that issue with destination/shutdown scripts, 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 see ! I had overlooked this code :) I let you determine the best path forward here.

I guess if we manage to completely isolate the payment_key derivation to a interface method we could consider including the counterparty_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 for StaticPaymentOutputs.

Can't the user already decide how payment_key is derived in SignerProvider::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.

But yeah, maybe you're right and we should try to do it 'the right way' right away.

Your call :)

tnull requested a review from TheBlueMatt October 31, 2024 10:33

tnull marked this pull request as draft October 31, 2024 10:33

tnull force-pushed the 2024-09-deterministic-payment-point branch 10 times, most recently from b8c5f77 to 5b17762 Compare October 31, 2024 14:28

tnull added 3 commits November 6, 2024 09:33

Drop InMemorySigner Writeable impl

6dfe913

... which we haven't been using since 0.0.119 / commit 7a951b1.

Rename payment_key to payment_basepoint in chan_utils

75c2b0a

.. as it doesn't use the actual signer's `payment_key`, but the associated public key.

Rename params to channel_keys_id and take it by value

611c1a3

.. to make the name more clear and since before we `clone` it in `derive_channel_keys` anyways.

tnull force-pushed the 2024-09-deterministic-payment-point branch from 5b17762 to a7db955 Compare November 6, 2024 11:14

tnull marked this pull request as ready for review November 6, 2024 11:14

tnull force-pushed the 2024-09-deterministic-payment-point branch 2 times, most recently from f9619b8 to 0c26baf Compare November 6, 2024 11:48

tnull added 4 commits November 6, 2024 13:56

Rename keys_id to channel_keys_id

6be731e

.. to align the field everywhere.

Rename payment_point to payment_basepoint

ed2d63e

.. to align the field naming with the spec for clarity.

f Adjust test cases to accommodate new derivation scheme

a781d23

Some test cases have hard-coded values which we change here (to be squashed in after review).

tnull force-pushed the 2024-09-deterministic-payment-point branch from 0c26baf to 57cadf2 Compare November 6, 2024 12:56

tnull added 2 commits November 6, 2024 14:25

f Allow legacy keys derivation for test_restored_packages_retry

bf14ec8

Have SignerProvider::get_shutdown_scriptpubkey take channel_keys_id

c70f7ec

.. to allow users to return specific scripts per channel.

tnull force-pushed the 2024-09-deterministic-payment-point branch from 57cadf2 to c70f7ec Compare November 6, 2024 13:26

TheBlueMatt reviewed Nov 25, 2024

View reviewed changes

TheBlueMatt added this to the 0.2 milestone Dec 5, 2024

TheBlueMatt linked an issue Dec 5, 2024 that may be closed by this pull request

Create fresh destination/shutdown scripts for each channel in KeysManager #1139

Open

TheBlueMatt removed a link to an issue Dec 5, 2024

Create fresh destination/shutdown scripts for each channel in KeysManager #1139

Open

tnull mentioned this pull request Dec 11, 2024

Let ChannelSigner set to_remote, to_local, htlc tx scriptpubkeys #3454

Open

tankyleo reviewed Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `payment_key` derivation deterministic #3391

Make `payment_key` derivation deterministic #3391

tnull commented Oct 31, 2024 •

edited

Loading

tnull commented Nov 6, 2024

codecov bot commented Nov 6, 2024 •

edited

Loading

TheBlueMatt left a comment

TheBlueMatt Nov 25, 2024

TheBlueMatt Nov 25, 2024

tankyleo Dec 11, 2024

tnull Dec 11, 2024

tankyleo Dec 11, 2024

tankyleo Dec 11, 2024

tankyleo Dec 11, 2024

tnull Dec 12, 2024

tankyleo Dec 17, 2024

Make payment_key derivation deterministic #3391

Are you sure you want to change the base?

Make payment_key derivation deterministic #3391

Conversation

tnull commented Oct 31, 2024 • edited Loading

tnull commented Nov 6, 2024

codecov bot commented Nov 6, 2024 • edited Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Make `payment_key` derivation deterministic #3391

Make `payment_key` derivation deterministic #3391

tnull commented Oct 31, 2024 •

edited

Loading

codecov bot commented Nov 6, 2024 •

edited

Loading