Support scalar tweak to rotate holder funding key during splicing by wpaulino · Pull Request #3624 · lightningdevkit/rust-lightning

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.

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.

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

+	/// 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?

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

Codecov Report

Attention: Patch coverage is 84.31373% with 24 lines in your changes missing coverage. Please review.

Project coverage is 89.18%. Comparing base (7188d5b) to head (bbd89a2).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/sign/mod.rs	63.15%	16 Missing and 5 partials ⚠️
lightning/src/ln/channel.rs	86.95%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3624 +/- ##
==========================================
- Coverage 89.19% 89.18% -0.02% 
==========================================
Files 155 155 Lines 118974 119068 +94 Branches 118974 119068 +94 ==========================================
+ Hits 106119 106188 +69 - Misses 10265 10285 +20 - Partials 2590 2595 +5

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

+// 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.

wpaulino · 2025-03-03T20:37:47Z

Ended up reworking this a bit thanks to @tankyleo's suggestion to allow the signer to customize the tweak if they wish to.

jkczyz · 2025-03-03T21:12:55Z

 current_chain_height: u32,
 outbound_scid_alias: u64,
-temporary_channel_id: Option<ChannelId>,
+temporary_channel_id_fn: Option<impl Fn(&ChannelPublicKeys) -> ChannelId>,


Isn't clear to me why this is needed for the PR or why it needs to be a Fn.

Because we need a secp_ctx now to call ChannelSigner::pubkeys, which is instantiated in ChannelContext::new_outbound_channel.

jkczyz · 2025-03-03T21:13:40Z

+/// While the use of this tweak is not required (signers may choose to compute a tweak of their
+/// choice), signers must ensure their tweak guarantees the two properties mentioned above:
+/// uniqueness and derivable only by one or both of the channel participants.


Seems like this should live on the trait somewhere?

This is just an example tweak so I figured this would be the best place to mention it. We already link to this from the trait method anyway.

jkczyz · 2025-03-03T21:28:54Z


+	/// Holder secret key in the 2-of-2 multisig script of a channel. This key also backs the
+	/// holder's anchor output in a commitment transaction, if one is present.
+	pub fn funding_key(&self, splice_parent_funding_txid: Option<Txid>) -> SecretKey {


nit: Might be worth passing the entire ChannelTransactionParameters to avoid mistakenly passing None. Your call.

This is also called by ChannelSigner::pubkeys which cannot have access to ChannelTransactionParameters before it is called before they are created.

jkczyz · 2025-03-03T21:47:31Z

+	fn pubkeys(
+ &self, splice_parent_funding_txid: Option<Txid>, secp_ctx: &Secp256k1<secp256k1::All>,
+	) -> ChannelPublicKeys;


Looks like this only used externally now. Isn't clear to me we should keep this as one method or make two methods -- one for initial funding and one for splicing -- to avoid needing to pass secp_ctx when None. Not sure if it matters all that much, but it would make what's happening at the call sites explicit.

Your call.
Maybe someone wants to do a derivation away from the "origin" keys of ChannelSigner for the 0th funding transaction ? A bit like we have m/0 then m/1 in BIP 32, instead of m then m/1
Also if we consider the 0th funding transaction to be a special case in a channel where many many splices will occur over its lifetime, maybe it's not worth breaking out a standalone function for a single case ?

Looks like this only used externally now.
It's only called during channel open currently, and we'll need to call it for the new ChannelTransactionParameters of a splice's FundingScope.
one for initial funding and one for splicing -- to avoid needing to pass secp_ctx when None.
If it's just for this purpose, I'd say it's not worth it. You're expected to have a secp_ctx available when interacting with the signer. If anything, I'd argue secp_ctx was missing from this method as it's required when going from secret keys to public keys. This isn't a problem for InMemorySigner because it is done on initialization to cache them.

tankyleo

Just have some nits, thank you!

tankyleo · 2025-03-04T03:50:31Z

 (8, self.funding_outpoint, option),
 (10, legacy_deserialization_prevention_marker, option),
 (11, self.channel_type_features, required),
+ (12, self.splice_parent_funding_txid, option),


Checking myself: once a user has made a splice, we cannot downgrade to some release before this commit here - that's why we assign an even field ?

tankyleo · 2025-03-04T04:23:58Z

+	fn pubkeys(
+ &self, splice_parent_funding_txid: Option<Txid>, secp_ctx: &Secp256k1<secp256k1::All>,
+	) -> ChannelPublicKeys;


Your call.
Maybe someone wants to do a derivation away from the "origin" keys of ChannelSigner for the 0th funding transaction ? A bit like we have m/0 then m/1 in BIP 32, instead of m then m/1
Also if we consider the 0th funding transaction to be a special case in a channel where many many splices will occur over its lifetime, maybe it's not worth breaking out a standalone function for a single case ?

wpaulino · 2025-03-04T21:16:59Z

Had to push again to fix CI

jkczyz · 2025-03-04T22:11:52Z

Looks like CI is sad still.

wpaulino · 2025-03-05T15:44:48Z

Somehow I didn't rebase on latest main the first time, so had to rebase again to fix the dyn_signer.rs build issues

jkczyz · 2025-03-05T16:46:05Z

Hmm... seems there's a test using --cfg=ldk_test_vectors that needs updating.

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](https://github.com/lightning/bolts/blob/master/03-transactions.md#key-derivation): 1. We use the txid of the funding transaction the splice transaction is spending instead of the `per_commitment_point` to guarantee uniqueness. 2. We include the private key instead of the public key to guarantee only those with knowledge of it can re-derive the new funding key. tweak = SHA256(splice_parent_funding_txid || base_funding_secret_key) tweaked_funding_key = base_funding_key + tweak While the use of this tweak is not required (signers may choose to compute a tweak of their choice), signers must ensure their tweak guarantees the two properties mentioned above: uniqueness and derivable only by one or both of the channel participants.

TheBlueMatt · 2025-03-06T00:14:04Z

 	fn get_change_destination_script(&self) -> Result<ScriptBuf, ()>;
 }

+mod sealed {


nit: this isn't really "sealed" in the traditional rust definition (which only applies to traits), this is just private...

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

TheBlueMatt reviewed Feb 26, 2025
View reviewed changes

Comment threadlightning/src/ln/chan_utils.rs Outdated

jkczyz reviewed Feb 27, 2025
View reviewed changes

Comment threadlightning/src/ln/chan_utils.rs Outdated

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

jkczyz reviewed Feb 27, 2025
View reviewed changes

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

jkczyz reviewed Feb 28, 2025
View reviewed changes

Comment threadlightning/src/sign/mod.rs Outdated

wpaulino force-pushed the funding-key-tweak branch 2 times, most recently from 66b084a to 08edfa7Compare February 28, 2025 17:52

tankyleo reviewed Feb 28, 2025
View reviewed changes

TheBlueMatt reviewed Feb 28, 2025
View reviewed changes

Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment threadlightning/src/sign/mod.rs Outdated

wpaulino force-pushed the funding-key-tweak branch from 08edfa7 to 21e3cccCompare March 3, 2025 20:36

wpaulino requested review from TheBlueMatt and jkczyz March 3, 2025 20:37

jkczyz reviewed Mar 3, 2025
View reviewed changes

tankyleo reviewed Mar 4, 2025
View reviewed changes

wpaulino force-pushed the funding-key-tweak branch from 21e3ccc to 2afbb41Compare March 4, 2025 16:03

jkczyz previously approved these changes Mar 4, 2025
View reviewed changes

wpaulino dismissed jkczyz’s stale review via c8f7f16March 4, 2025 21:16

wpaulino force-pushed the funding-key-tweak branch from 2afbb41 to c8f7f16Compare March 4, 2025 21:16

wpaulino force-pushed the funding-key-tweak branch from c8f7f16 to 257d91fCompare March 5, 2025 15:44

wpaulino mentioned this pull request Mar 5, 2025
lightning-liquidity: Introduce EventQueue notifier and wake BP for message processing #3509
Merged

wpaulino force-pushed the funding-key-tweak branch from 257d91f to bbd89a2Compare March 5, 2025 18:10

jkczyz approved these changes Mar 5, 2025
View reviewed changes

TheBlueMatt approved these changes Mar 6, 2025
View reviewed changes

TheBlueMatt merged commit f25709c into lightningdevkit:mainMar 6, 2025

wpaulino deleted the funding-key-tweak branch March 6, 2025 01:26

jkczyz mentioned this pull request Jun 21, 2025
Dual-funded channels and Splicing Project Tracking #1621
Closed
7 tasks

Uh oh!

Conversation

wpaulino commented Feb 26, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wpaulino commented Feb 27, 2025

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecovBot commented Feb 28, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wpaulino commented Mar 3, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wpaulino commented Mar 4, 2025

Uh oh!

jkczyz commented Mar 4, 2025

Uh oh!

wpaulino commented Mar 5, 2025

Uh oh!

jkczyz commented Mar 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

wpaulino commented Feb 26, 2025•
edited
Loading

codecovBot commented Feb 28, 2025•
edited
Loading