Refactor `ChannelTransactionParameters` into `FundingScope` #3604

jkczyz · 2025-02-14T21:18:39Z

Continues the work of #3592 by moving fields related to the funding transaction into FundingScope. ~~This includes fields from ChannelContext for the funding transaction and ChannelTransactionParameters for the funding outpoint and pubkeys.~~

This primarily involves moving the entire ChannelTransactionParameters into FundingScope and channel_value_satoshis into ChannelTransactionParameters. The latter result in less API churn at the cost of duplicating parameters that don't change across each FundingScope. To that end, this PR also updates the EcdsaChannelSigner to take ChannelTransactionParameters which removes the need for ChannelSigner::provide_channel_parameters.

Since ChannelTransactionParameters contains channel_value_satoshis, implementation like InMemorySigner no longer need it. Thus, the NodeSigner trait is updated to no longer take the value when deriving signers, which is a breaking change.

jkczyz · 2025-02-14T21:18:48Z

@wpaulino This primarily moves fields from ChannelContext and duplicates the funding outpoint pubkeys from ChannelTransactionParameters into FundingScope. I skimmed through sign.rs to see how this would be affected. Thinking on it a bit, I'm wondering if instead of duplicating those fields we instead move the entire ChannelTransactionParameters from ChannelContext into FundingScope.

The drawback is we'd be duplicating fields that wouldn't change. But this seems better than duplicating fields that would change and risk having inconsistent data. Also, it could prevent us from needing to worry about changing other uses of ChannelTransactionParameters (e.g., by ChannelMonitor and OnchainTxHandler), outside needing to pass the appropriate ChannelTransactionParameters. Maybe we could reduce the duplication when persisting ChannelMonitor, though.

What are your thoughts?

wpaulino · 2025-02-14T23:46:57Z

I was thinking we could deprecate the funding scope related fields from ChannelTransactionParameters and eventually remove them, so there wouldn't be any duplicate data in the long run.

jkczyz · 2025-02-18T19:11:51Z

I was thinking we could deprecate the funding scope related fields from ChannelTransactionParameters and eventually remove them, so there wouldn't be any duplicate data in the long run.

Yeah, this PR takes that approach. But when removing the fields we'd need to update all uses to take both ChannelTransactionParameters and FundingScope anywhere ChannelTransactionParameters is currently needed. So I'm wondering more based on the current uses if it makes sense to include the entire ChannelTransactionParameters in FundingScope?

For instance, writing an OnchainTxHandler writes ChannelTransactionParameters. So removing the deprecated fields would require a custom serialization to include the removed fields from FundingScope. And presumably OnchainTxHandler wouldn't need the other FundingScope fields since it currently doesn't.

There's also DirectedChannelTransactionParameters which has broadcaster_pubkeys and countersignatory_pubkeys methods where the returned ChannelPublicKeys needs to include the funding_pubkey. So we'd need to have AnchorDescriptor hold a FundingScope, for instance, and have a similar Directed variation. But all those additional fields in FundingScope aren't relevant.

I haven't looked exhaustively to see all usages, but it seems simpler to keep ChannelTransactionParameters as it is.

wpaulino · 2025-02-18T19:42:23Z

Hm, I'm not opposed to always writing ChannelTransactionParameters, especially given the name already conveys it's tied to a specific channel transaction. It would also cover for any other currently static parameters changing due to some future protocol feature/upgrade. We would still be breaking the API somewhat since they are currently intended to be static throughout the channel's lifetime, but we can think of something there.

As for OnchainTxHandler, it will already need to know of the scope (or a subset of its fields) somehow to carry out signing requests. I'd love to avoid all of that completely by having the auxiliary data come from the claim request being handled, but that seems like quite the big change.

jkczyz · 2025-02-19T16:41:03Z

Here's the alternative approach: https://github.com/jkczyz/rust-lightning/tree/2025-02-channel-funding-pubkeys-alt

wpaulino · 2025-02-19T17:57:00Z

Here's the alternative approach: https://github.com/jkczyz/rust-lightning/tree/2025-02-channel-funding-pubkeys-alt

Yeah this looks pretty simple, and would also allow us to handle transitioning to new channel types in a splice since it already tracks the channel type features. I don't think the duplicate data is all that bad here once we have multiple scopes since it will eventually go away when the splice confirms. We should at least be able to avoid persisting the duplicate data, if we choose to, by cloning it from the "main" (currently confirmed) scope when reading.

Thoughts on either of these approaches @TheBlueMatt?

jkczyz · 2025-02-20T16:25:29Z

@TheBlueMatt Added some more commits to the alternative branch after pairing with @wpaulino yesterday. They move channel_value_satoshis from FundingScope to ChannelTransactionParameters, which will be passed to EcdsaChannelSigner methods in lieu of calling provide_channel_parameters. This is done so far for sign_counterparty_commitment. The goal would be to get rid channel_value_satoshis and channel_parameters from InMemorySigner and instead access them through the passed ChannelTransactionParameters.

TheBlueMatt · 2025-02-21T22:14:37Z

Discussed this being supersceded by an alternative that tries to keep signer operations (across many splice versions) together as one call.

Rather than moving relevant fields of ChannelTransactionParameters to FundingScope, move the entire struct there instead. This prevents API churn wherever ChannelTransactionParameters is used, which otherwise would need a FundingScope in addition.

Since the funding transactions changes for each new FudningScope, include it there instead of ChannelContext.

Since channel_value_satoshis is needed by the signer and may change for each new FundingScope, included it in ChannelTransactionParameters so it can be passed to each signer call in the upcoming commits.

Now that ChannelTransactionParameters are in FundingScope, channel_value_satoshis can be dropped from the latter.

Now that channel_value_satoshis has been moved to ChannelTransactionParameters, pass the entire parameters when calling each method on EcdsaChannelSigner. This will remove the need for ChannelSigner::provide_channel_parameters. Instead, the parameters from the FundingScope will be passed in to each method. This simplifies the interaction with a ChannelSigner when needing to be called for more than one FundingScope, which will be the case for pending splices and RBF attempts.

codecov · 2025-02-25T01:04:51Z

Codecov Report

Attention: Patch coverage is 75.09579% with 65 lines in your changes missing coverage. Please review.

Project coverage is 88.59%. Comparing base (c9fd3a5) to head (aff70ae).

Files with missing lines	Patch %	Lines
lightning/src/util/test_channel_signer.rs	51.51%	9 Missing and 23 partials ⚠️
lightning/src/sign/mod.rs	76.13%	11 Missing and 10 partials ⚠️
lightning/src/ln/channelmanager.rs	85.29%	2 Missing and 3 partials ⚠️
lightning/src/util/test_utils.rs	50.00%	4 Missing ⚠️
lightning/src/chain/channelmonitor.rs	92.30%	0 Missing and 1 partial ⚠️
lightning/src/chain/onchaintx.rs	85.71%	0 Missing and 1 partial ⚠️
lightning/src/events/bump_transaction.rs	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3604      +/-   ##
==========================================
- Coverage   88.60%   88.59%   -0.02%     
==========================================
  Files         151      151              
  Lines      118409   118404       -5     
  Branches   118409   118404       -5     
==========================================
- Hits       104918   104900      -18     
+ Misses      10972    10968       -4     
- Partials     2519     2536      +17

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

jkczyz · 2025-02-25T01:05:05Z

Updated the branch to use the alternative version. See updated PR description for details.

devrandom · 2025-02-25T18:42:58Z

lightning/src/ln/channel.rs

+
+	/// The transaction which funds this channel. Note that for manually-funded channels (i.e.,
+	/// [`ChannelContext::is_manual_broadcast`] is true) this will be a dummy empty transaction.
+	funding_transaction: Option<Transaction>,


would it be possible to have just the transaction ID here? it's not so good to carry around the funding transaction for every signing operation in memory limited environments.

This won't be given to the signer, only ChannelTransactionParameters will

ah, of course, thank you

wpaulino · 2025-02-25T20:06:18Z

lightning/src/sign/ecdsa.rs

@@ -238,7 +238,8 @@ pub trait EcdsaChannelSigner: ChannelSigner {
 	///
 	/// [`NodeSigner::sign_gossip_message`]: crate::sign::NodeSigner::sign_gossip_message
 	fn sign_channel_announcement_with_funding_key(
-		&self, msg: &UnsignedChannelAnnouncement, secp_ctx: &Secp256k1<secp256k1::All>,
+		&self, channel_parameters: &ChannelTransactionParameters,


We can probably omit this one, instead we'd just have to pass the optional funding pubkey tweak once that's implemented.

Ah, I assumed we'd get it from ChannelTransactionParameters. Happy to drop this if you'd prefer.

Yeah let's drop it for now, one less thing to break

wpaulino · 2025-02-25T20:11:21Z

lightning/src/sign/mod.rs

+		// TODO: Is this ok?
+		let channel_parameters =
+			descriptor.channel_transaction_parameters.as_ref().expect(MISSING_PARAMS_ERR);


Only if the descriptor was produced in v0.0.117 or later. Probably best to err on the safe side and keep the handling as before.

What about for the payment point? The earlier version was using self.pubkeys(). I figured we get rid of this since it's duplicative with channel_parameters.holder_pubkeys. But maybe we use that as a fallback instead when looking up the payment point. Will add a fixup doing so.

I'd say just keep using self.pubkeys() since it doesn't depend on the channel parameters. It might get removed eventually with some of the custom commitment work to be done, so we can worry about it then.

wpaulino · 2025-02-25T20:16:00Z

lightning/src/sign/mod.rs

@@ -1804,7 +1734,7 @@ impl Writeable for InMemorySigner {
 		self.delayed_payment_base_key.write(writer)?;
 		self.htlc_base_key.write(writer)?;
 		self.commitment_seed.write(writer)?;
-		self.channel_parameters.write(writer)?;


I think we can remove all serialization code for the signer now, it hasn't been used for many releases.

Looks like deserialization is indirectly used via read_chan_signer called when a deserialized FundedChannel doesn't contain channel_keys_id:

rust-lightning/lightning/src/ln/channel.rs

Line 10592 in 1610854

let holder_signer = signer_provider.read_chan_signer(&keys_data)?;

Isn't clear to me when that wouldn't be present, though. If that's old, maybe we should error instead?

I think channel_keys_id can be expected on now. keys_data is only Some for versions <= 2 (we're on 4 now), so someone upgrading from back then would be pretty rare. And if they are, they can just upgrade to an older version first.

Gotcha. Removed this now.

lightning/src/events/bump_transaction.rs

lightning/src/sign/mod.rs

lightning/src/ln/chan_utils.rs

wpaulino · 2025-02-25T20:20:35Z

lightning/src/ln/channel.rs

@@ -1684,7 +1684,7 @@ impl FundingScope {
 	/// pays to get_funding_redeemscript().to_p2wsh()).
 	/// Panics if called before accept_channel/InboundV1Channel::new
 	pub fn get_funding_redeemscript(&self) -> ScriptBuf {
-		make_funding_redeemscript(&self.get_holder_pubkeys().funding_pubkey, self.counterparty_funding_pubkey())
+		self.channel_transaction_parameters.make_funding_redeemscript()


Any reason this was done? Seems fine to keep it as it was

I had a note from our meeting to move this to ChannelTransactionParameters. I ended up keeping the method on FundingScope but moving the body.

Now that channel_value_satoshis has been moved to ChannelTransactionParameters, pass the entire parameters when calling each method on EcdsaChannelSigner. This will remove the need for ChannelSigner::provide_channel_parameters. Instead, the parameters from the FundingScope will be passed in to each method. This simplifies the interaction with a ChannelSigner when needing to be called for more than one FundingScope, which will be the case for pending splices and RBF attempts.

Now that channel_value_satoshis has been moved to ChannelTransactionParameters, InMemorySigner no longer needs a copy. Remove uses of the copy from sign_counterparty_payment_input.

Now that channel_value_satoshis has been moved to ChannelTransactionParameters, InMemorySigner no longer needs a copy. Remove indirect uses of the copy from TestChannelSigner.

InMemorySigner no longer holds channel_value_satoshis and channel_parameters. Instead of writing 0 and None, respectively, drop (de-)serialization support entirely since InMemorySigner hasn't been serialized since SERIALIZATION_VERSION 2.

Now that the copy of ChannelTransactionParameters is no longer used by InMemorySigner, remove it and all remaining uses. Since it was set by ChannelSigner::provide_channel_parameters, this method is no longer needed and can also be removed.

Now that channel_value_satoshis has been moved to ChannelTransactionParameters, it no longer needs to be used when deriving a signer. This is a breaking API change, though InMemorySigner did not make use of channel_value_satoshis when being derived.

The custom derive_channel_signer methods on AnchorDescriptor and HTLCDescriptor simply delegate to the passed SignerProvider now that providing ChannelTransactionParameters is no longer needed. Drop these methods and replace them with the method bodies at the call sites.

jkczyz mentioned this pull request Feb 19, 2025

[Splicing] Clone for ChannelContext #3332

Closed

wpaulino mentioned this pull request Feb 24, 2025

allow functional tests to be used externally with a dynamic signer factory #3016

Open

6 tasks

jkczyz force-pushed the 2025-02-channel-funding-pubkeys branch from e788ac1 to df840cb Compare February 24, 2025 23:18

jkczyz added 12 commits February 24, 2025 18:27

Reduce ChannelContext::channel_transaction_parameters visibility

e360922

Move funding_transaction to FundingScope

0e499a5

Since the funding transactions changes for each new FudningScope, include it there instead of ChannelContext.

Include channel_value_satoshis in ChannelTransactionParameters

a53f454

Since channel_value_satoshis is needed by the signer and may change for each new FundingScope, included it in ChannelTransactionParameters so it can be passed to each signer call in the upcoming commits.

Remove channel_value_satoshis from FundingScope

acc5318

Now that ChannelTransactionParameters are in FundingScope, channel_value_satoshis can be dropped from the latter.

jkczyz force-pushed the 2025-02-channel-funding-pubkeys branch from df840cb to aff70ae Compare February 25, 2025 00:56

jkczyz changed the title ~~Refactor funding-tx-related fields into FundingScope~~ Refactor ChannelTransactionParameters into FundingScope Feb 25, 2025

jkczyz marked this pull request as ready for review February 25, 2025 01:05

jkczyz requested review from TheBlueMatt and wpaulino February 25, 2025 01:05

jkczyz added the weekly goal Someone wants to land this this week label Feb 25, 2025

devrandom reviewed Feb 25, 2025

View reviewed changes

wpaulino reviewed Feb 25, 2025

View reviewed changes

jkczyz force-pushed the 2025-02-channel-funding-pubkeys branch 2 times, most recently from ad8ddcb to 339e0ca Compare February 26, 2025 01:19

jkczyz added 9 commits February 25, 2025 19:22

Use channel parameters in sign_counterparty_payment_input

d694900

Now that channel_value_satoshis has been moved to ChannelTransactionParameters, InMemorySigner no longer needs a copy. Remove uses of the copy from sign_counterparty_payment_input.

f - remove expect

2316fb3

Use channel parameters in sign_holder_htlc_transaction

721defd

Now that channel_value_satoshis has been moved to ChannelTransactionParameters, InMemorySigner no longer needs a copy. Remove indirect uses of the copy from TestChannelSigner.

Move get_funding_redeemscript implementation

a55149f

jkczyz force-pushed the 2025-02-channel-funding-pubkeys branch from 339e0ca to a55149f Compare February 26, 2025 01:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `ChannelTransactionParameters` into `FundingScope` #3604

Refactor `ChannelTransactionParameters` into `FundingScope` #3604

jkczyz commented Feb 14, 2025 •

edited

Loading

jkczyz commented Feb 14, 2025

wpaulino commented Feb 14, 2025

jkczyz commented Feb 18, 2025

wpaulino commented Feb 18, 2025

jkczyz commented Feb 19, 2025

wpaulino commented Feb 19, 2025

jkczyz commented Feb 20, 2025

TheBlueMatt commented Feb 21, 2025

codecov bot commented Feb 25, 2025

jkczyz commented Feb 25, 2025

devrandom Feb 25, 2025

wpaulino Feb 25, 2025

devrandom Feb 26, 2025

wpaulino Feb 25, 2025

jkczyz Feb 25, 2025

wpaulino Feb 26, 2025

jkczyz Feb 26, 2025

wpaulino Feb 25, 2025

jkczyz Feb 25, 2025

wpaulino Feb 26, 2025

jkczyz Feb 26, 2025

wpaulino Feb 25, 2025

jkczyz Feb 25, 2025

wpaulino Feb 26, 2025

jkczyz Feb 26, 2025

wpaulino Feb 25, 2025

jkczyz Feb 26, 2025

Refactor ChannelTransactionParameters into FundingScope #3604

Are you sure you want to change the base?

Refactor ChannelTransactionParameters into FundingScope #3604

Conversation

jkczyz commented Feb 14, 2025 • edited Loading

jkczyz commented Feb 14, 2025

wpaulino commented Feb 14, 2025

jkczyz commented Feb 18, 2025

wpaulino commented Feb 18, 2025

jkczyz commented Feb 19, 2025

wpaulino commented Feb 19, 2025

jkczyz commented Feb 20, 2025

TheBlueMatt commented Feb 21, 2025

codecov bot commented Feb 25, 2025

Codecov Report

jkczyz commented Feb 25, 2025

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

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

Refactor `ChannelTransactionParameters` into `FundingScope` #3604

Refactor `ChannelTransactionParameters` into `FundingScope` #3604

jkczyz commented Feb 14, 2025 •

edited

Loading