Support scalar tweak to rotate holder funding key during splicing #3624

wpaulino · 2025-02-26T19:01:57Z

We introduce a scalar tweak that can be applied to the base funding key to obtain the channel's funding key used in the 2-of-2 multisig. This is used to derive additional keys from the same secret backing the base funding_pubkey, as we have to rotate keys for each successful splice attempt.

The tweak is computed similar to existing tweaks used in BOLT-3, but rather than using the per_commitment_point, we use the txid of the funding transaction the splice transaction is spending to guarantee uniqueness, and the revocation_basepoint to guarantee only the channel participants can re-derive the new funding key.

tweak = SHA256(splice_parent_funding_txid || revocation_basepoint || base_funding_pubkey)
tweaked_funding_key = base_funding_key + tweak

Depends on #3604.
Fixes #3542.

lightning/src/ln/chan_utils.rs

wpaulino · 2025-02-27T20:27:12Z

Pushed an update addressing both of your comments @TheBlueMatt @jkczyz.

jkczyz

Just some minor comments. Feel free to ignore if you prefer. Otherwise, LGTM.

lightning/src/sign/mod.rs

lightning/src/ln/channel.rs

jkczyz · 2025-02-27T21:17:57Z

lightning/src/sign/mod.rs

+	/// NOTE: The [`ChannelPublicKeys::funding_pubkey`] returned will not necessarily correspond to
+	/// the channel's current holder public key, as it may have rotated due a splice. The value
+	/// should not be relied upon once the channel's initial funding transaction has been created.


I wonder if we should return the ChannelPublicKeys as part of the derive_channel_signer so that we can remove this method. Then we wouldn't need this caveat. Doesn't need to be done in this PR if it's going to be involved, though.

It would be nice to remove the method, but I think the caveat would still be there regardless?

lightning/src/ln/chan_utils.rs

codecov · 2025-02-28T00:05:53Z

Codecov Report

Attention: Patch coverage is 68.51852% with 51 lines in your changes missing coverage. Please review.

Project coverage is 89.16%. Comparing base (36ba27a) to head (08edfa7).

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	60.97%	31 Missing and 1 partial ⚠️
lightning/src/util/ser.rs	0.00%	10 Missing ⚠️
lightning/src/sign/mod.rs	80.95%	6 Missing and 2 partials ⚠️
lightning/src/ln/channel.rs	92.85%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3624      +/-   ##
==========================================
- Coverage   89.19%   89.16%   -0.04%     
==========================================
  Files         152      152              
  Lines      118681   118795     +114     
  Branches   118681   118795     +114     
==========================================
+ Hits       105860   105918      +58     
- Misses      10236    10289      +53     
- Partials     2585     2588       +3

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

lightning/src/sign/mod.rs

A scalar tweak applied to the base funding key to obtain the channel's funding key used in the 2-of-2 multisig. This is used to derive additional keys from the same secret backing the base `funding_pubkey`, as we have to rotate keys for each successful splice attempt. The tweak is computed similar to existing tweaks used in [BOLT-3](https://github.com/lightning/bolts/blob/master/03-transactions.md#key-derivation), but rather than using the `per_commitment_point`, we use the txid of the funding transaction the splice transaction is spending to guarantee uniqueness, and the `revocation_basepoint` to guarantee only the channel participants can re-derive the new funding key. tweak = SHA256(splice_parent_funding_txid || revocation_basepoint || base_funding_pubkey) tweaked_funding_key = base_funding_key + tweak

tankyleo · 2025-02-28T21:44:05Z

lightning/src/ln/chan_utils.rs

+// TODO: Expose a helper on `FundingScope` that calls this.
+pub fn compute_funding_key_tweak(base_funding_pubkey: &PublicKey, revocation_basepoint: &PublicKey, splice_parent_funding_txid: &Txid) -> Scalar {
+	let mut sha = Sha256::engine();
+	sha.input(splice_parent_funding_txid.as_byte_array());
+	sha.input(&revocation_basepoint.serialize());
+	sha.input(&base_funding_pubkey.serialize());
+	Scalar::from_be_bytes(Sha256::from_engine(sha).to_byte_array()).unwrap()
+}


I would be more comfortable with a roundtrip to the signer to ask for a fresh funding key to use for the new funding transaction, rather than FundingScope deriving this new public key. Could we expand the existing ChannelSigner::pubkeys call to take some id / idx of the funding transaction we need keys for? Then write the returned ChannelPublicKeys to ChannelTransactionParameters, similar to how we do it today for the initial funding transaction?

I am not sure that FundingScope should define the derivation scheme to use for new funding keys; the signer should do that.

In PR 3606 I propose that we avoid having channel objects impose a particular derivation scheme for TxCreationKeys on TxBuilder - this comment is in the same direction.

I consider this tweak a bit different than others because it's something we're doing locally, not something that both peers of the channel need to agree upon and therefore needs to be customized. While we could certainly go to the signer to ask for a new funding key, this mostly abstracts that away from them (the less work the signer has to do, the better IMO, especially for those not using InMemorySigner). The signer is still free to choose their derivation scheme of choice based on channel_keys_id, we are just proposing a tweak to be added to the funding key at the last mile.

Could we expand the existing ChannelSigner::pubkeys call to take some id / idx of the funding transaction we need keys for?

This would be nice to get rid of the caveat pointed out in the docs of ChannelSigner::pubkeys, but we'd want to guarantee the signer implementation only tweaks the funding key and not any of the others, as that would break the channel.

not something that both peers of the channel need to agree upon and therefore needs to be customized.

Interesting my thinking was - Spec does not dictate how to derive the new funding key, and the counterparty does not care which derivation we use, so LDK should let people customize this derivation.

the less work the signer has to do, the better IMO, especially for those not using InMemorySigner

Also interesting - I've been under the impression that signer does too little, and channel does too much :) At least too much that is specific to ecdsa signatures for example.

The signer is still free to choose their derivation scheme of choice based on channel_keys_id, we are just proposing a tweak to be added to the funding key at the last mile.

Yes I agree signer already has some choice of the derivation scheme. Though letting people customize the derivation of TxCreationKeys in TxBuilder could also be seen as a last mile operation - and at the moment we'd let people customize that part.

This would be nice to get rid of the caveat pointed out in the docs of ChannelSigner::pubkeys, but we'd want to guarantee the signer implementation only tweaks the funding key and not any of the others, as that would break the channel.

Certainly

I guess one benefit of having the signer derive the new funding key is it allows them to reuse that same funding key somewhere else within the commitment transaction as a spending condition or destination. I explored the changes you proposed in this separate branch: https://github.com/wpaulino/rust-lightning/tree/funding-key-tweak-alt

I appreciate you doing this thank you.

If we go for this direction, I would hope we can maintain the invariant on ChannelTransactionParameters that the holder_pubkeys matches the keys expected for the parent_txid.

So in src/chain/package, we could read the pubkeys directly from channel_parameters instead of doing a call to the signer.

TheBlueMatt · 2025-02-28T21:56:30Z

lightning/src/ln/chan_utils.rs

+	/// backing the base `funding_pubkey`, as we have to rotate keys for each successful splice
+	/// attempt. The tweak is computed as described in [`compute_funding_key_tweak`].
+	//
+	// TODO: Expose `splice_parent_funding_txid` instead so the signer can re-derive the tweak?


Yea, I think its fine, as you note the only invalid scalar would be the negation of the private key and calculating that requires knowing the private key, so....Just remove the TODO :)

TheBlueMatt · 2025-02-28T21:58:30Z

lightning/src/sign/mod.rs

@@ -776,6 +776,10 @@ pub trait ChannelSigner {
 	/// Returns the holder's channel public keys and basepoints.
 	///
 	/// This method is *not* asynchronous. Instead, the value must be cached locally.
+	///
+	/// NOTE: The [`ChannelPublicKeys::funding_pubkey`] returned will not necessarily correspond to


Then maybe lets remove it?

Remove funding_pubkey or ChannelSigner::pubkeys? The method is used on channel open to set the holder basepoints in ChannelTransactionParameters.

wpaulino added the weekly goal Someone wants to land this this week label Feb 26, 2025

TheBlueMatt reviewed Feb 26, 2025

View reviewed changes

lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved

jkczyz reviewed Feb 27, 2025

View reviewed changes

lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the funding-key-tweak branch from fda4235 to def03b5 Compare February 27, 2025 20:25

jkczyz reviewed Feb 27, 2025

View reviewed changes

wpaulino force-pushed the funding-key-tweak branch from def03b5 to 4bf1e8b Compare February 27, 2025 23:56

jkczyz reviewed Feb 28, 2025

View reviewed changes

lightning/src/sign/mod.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the funding-key-tweak branch from 4bf1e8b to 66b084a Compare February 28, 2025 17:51

wpaulino force-pushed the funding-key-tweak branch from 66b084a to 08edfa7 Compare February 28, 2025 17:52

tankyleo reviewed Feb 28, 2025

View reviewed changes

TheBlueMatt reviewed Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support scalar tweak to rotate holder funding key during splicing #3624

Support scalar tweak to rotate holder funding key during splicing #3624

wpaulino commented Feb 26, 2025 •

edited

Loading

wpaulino commented Feb 27, 2025

jkczyz left a comment

jkczyz Feb 27, 2025

wpaulino Feb 27, 2025

codecov bot commented Feb 28, 2025 •

edited

Loading

tankyleo Feb 28, 2025

wpaulino Feb 28, 2025

tankyleo Feb 28, 2025

wpaulino Mar 1, 2025

tankyleo Mar 1, 2025

TheBlueMatt Feb 28, 2025

TheBlueMatt Feb 28, 2025

wpaulino Feb 28, 2025

Support scalar tweak to rotate holder funding key during splicing #3624

Are you sure you want to change the base?

Support scalar tweak to rotate holder funding key during splicing #3624

Conversation

wpaulino commented Feb 26, 2025 • edited Loading

wpaulino commented Feb 27, 2025

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 28, 2025 • edited Loading

Codecov Report

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

wpaulino commented Feb 26, 2025 •

edited

Loading

codecov bot commented Feb 28, 2025 •

edited

Loading