Refactor `ChannelContext` value fields into `FundingScope` #3592

jkczyz · 2025-02-07T23:13:56Z

When establishing a channel, the funding transaction may be replaced either:

after the funding transaction has confirmed using splicing,
before the funding transaction has confirmed for v2 channel
establishment using tx_init_rbf, or
before the splice's funding transaction has confirmed using
tx_init_rbf.

In each of these cases, fields in ChannelContext will need to be updated once the funding transaction confirms. Additionally, the same fields for a pending attempt may need to be considered instead of a previously confirmed funding.

This PR introduces a FundingScope to hold the aforementioned fields. It lives next to ChannelContext and will be needed whenever these fields are accessed. It limits the the fields moved to those related to the channel's value. Other fields will be moved in follow-up PRs.

jkczyz · 2025-02-07T23:29:16Z

@wpaulino Would like some early feedback on this approach of using a ScopedChannelContext which wraps references to FundingScope and ChannelContext. The first few commits introduce this structural refactoring.

The last commit and its fixups moves channel_value_satoshis to FundingScope. This was mostly straight-forward except for build_commitment_transaction as its return value (and that of it's callers) has a lifetime of one of ChannelContext's fields which is used to populate CommitmentStats. See commit message for details. These were the compilation errors:

error[E0515]: cannot return value referencing local variable `funding_context`
    --> lightning/src/ln/channel.rs:8556:3
     |
8535 | ...   let commitment_stats = funding_context.build_commitment_transaction(self.context.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, true, logge...
     |                              -------------------------------------------------------------------------------------------------------------------------------------------------- `funding_context` is borrowed here
...
8556 | ...   (commitment_stats.htlcs_included, counterparty_commitment_tx)
     |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ returns a value referencing data owned by the current function

error[E0515]: cannot return value referencing local variable `funding_context`
    --> lightning/src/ln/channel.rs:8604:5
     |
8568 |           let commitment_stats = funding_context.build_commitment_transaction(self.context.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, true, logge...
     |                                  -------------------------------------------------------------------------------------------------------------------------------------------------- `funding_context` is borrowed here
...
8604 | /                 Ok((msgs::CommitmentSigned {
8605 | |                     channel_id: self.context.channel_id,
8606 | |                     signature,
8607 | |                     htlc_signatures,
...    |
8610 | |                     partial_signature_with_nonce: None,
8611 | |                 }, (counterparty_commitment_txid, commitment_stats.htlcs_included)))
     | |____________________________________________________________________________________^ returns a value referencing data owned by the current function

For more information about this error, try `rustc --explain E0515`.
error: could not compile `lightning` due to 2 previous errors

The commit is later reverted and FundingScope is passed in directly instead.

Compiles with RUSTFLAGS="--cfg=dual_funding" cargo +1.63.0 check -p lightning. It's possible the test may have some compilation errors.

jkczyz · 2025-02-10T19:54:36Z

Chatted with @wpaulino offline about attempting to pass FundingScope everywhere instead of having a ScopedChannelContext. It was mostly straightforward, but it became a bit more complicated with force_shutdown and convert_channel_err!. In particularly, needing a new Channel::funding_and_context_mut method, since the compiler needs to be convinced we aren't taking both a &mut and & to a channel. Here's the relevant commit with branch linked: jkczyz@431f7a4

jkczyz · 2025-02-10T23:11:19Z

Chatted with @wpaulino offline about attempting to pass FundingScope everywhere instead of having a ScopedChannelContext. It was mostly straightforward, but it became a bit more complicated with force_shutdown and convert_channel_err!. In particularly, needing a new Channel::funding_and_context_mut method, since the compiler needs to be convinced we aren't taking both a &mut and & to a channel. Here's the relevant commit with branch linked: jkczyz@431f7a4

FYI, I pushed some more commits to the branch that move some more fields into FundedScope. https://github.com/jkczyz/rust-lightning/tree/2025-02-channel-funding-scope-alt

jkczyz · 2025-02-11T18:27:12Z

FYI, I pushed some more commits to the branch that move some more fields into FundedScope. https://github.com/jkczyz/rust-lightning/tree/2025-02-channel-funding-scope-alt

Went ahead with this approach as discussed offline. This PR limits the the fields moved to those related to the channel's value. Other fields will be moved in follow-up PRs.

jkczyz · 2025-02-11T20:31:16Z

Pushed a small change to cheer up CI compiling under --cfg=ldk_test_vectors.

TheBlueMatt · 2025-02-11T21:25:33Z

Is the thinking here that basically we'd implement things on the ScopedChannelContext and then run message handling twice, once on each context? Then we'd have to implement "undoing" the message on the first context, right? Or would we split most handling into a test then an accept method?

jkczyz · 2025-02-11T22:00:10Z

Is the thinking here that basically we'd implement things on the ScopedChannelContext and then run message handling twice, once on each context? Then we'd have to implement "undoing" the message on the first context, right? Or would we split most handling into a test then an accept method?

Note that the updated branch drops ScopedChannelContext in favor of explicitly passing in a FundingScope. (For reference, the previous version can be found here: https://github.com/jkczyz/rust-lightning/tree/2025-02-channel-funding-scope-old.)

Generally, the idea for splice/rbf would be to maintain an additional FundingScopes for each outstanding splices/rbf. But, IIUC, you're saying there may be a field on ChannelContext that could change during handling, thus we'd need to account for that. Yeah, if that's the case, I'd prefer to update handling to use a test and accept approach to avoid undoing, which seems error prone.

wpaulino · 2025-02-11T22:56:16Z

lightning/src/ln/channel.rs

+impl<SP: Deref> FundedChannel<SP>
+where
+	SP::Target: SignerProvider,
+	<SP::Target as SignerProvider>::EcdsaSigner: EcdsaChannelSigner,
+{


Up to you, but I'd rather move the method to keep the number of impl blocks to a minimum so we don't have to keep redefining all the generics

Maybe we tack on an extra commit at the end that is purely moving code to make reviewing easier?

Added fixups along the way to revert to a single impl block.

wpaulino · 2025-02-11T23:00:59Z

lightning/src/ln/channel.rs

@@ -4257,7 +4313,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 			unbroadcasted_batch_funding_txid,
 			channel_id: self.channel_id,
 			user_channel_id: self.user_id,
-			channel_capacity_satoshis: self.channel_value_satoshis,
+			channel_capacity_satoshis: funding.channel_value_satoshis,


Unfortunately it seems like some of these fields will not be accurate if we tried closing the channel with the currently confirmed scope, but then a pending splice ends up confirming and we have to spend from the spliced channel instead.

Discussed this a bit more offline. Seems when processing ShutdownResult in ChannelManager::finish_close_channel, instead of generating a ChannelClosed event there we should do so in ChannelMonitor. This will require including the ClosureReason in ChannelMonitorUpdateStep::ChannelForceClosed.

@TheBlueMatt Any thoughts on this? @wpaulino Feel free to fill in any more details that I may have left out.

Right, obviously the ChannelMonitor will know how to spend any splice at any point, but I think we already have the issue that the ChannelClosed event can be subtly off once we hit chain - its always generated immediately, the amounts in it are intended to be informative (maybe we should clarify that more in the docs?), but eg the counterparty might broadcast a state that's one old (but not revoked...or revoked) and we'll get more or less and have to claim HTLCs on chain... I don't think we want to wait until the channel is confirmed on chain before we generate the event.

If we can't/don't want to rely on the values being correct then we might as well not include them in the event?

I'm definitely open to removing them, but we should keep the info around somewhere - currently Balances in ChannelMonitor don't tell you how big the channel was/what your msat balance was. The original motivation for the balance field (which may be wrong) is #1898.

How about adding a closed_channel_balance API to the monitor that users can use as long as they keep it around? We'd only return Some once the closing transaction is no longer under reorg risk and it could include channel value, local/remote balance, and pending HTLC balance.

Hmm, yea, that would work, I guess a related question is how Balances are going to work with splicing anyway - I guess we'll have to have redundant Balances for different splice cases?

The HTLC balances could probably stay the same? For the local balance, when there's a splice pending, maybe we use a new variant over ClaimableOnChannelClose (e.g., MaybeClaimableOnChannelClose?) that gives you the balance for a particular FundingScope. Once the splice is no longer pending, we go back to ClaimableOnChannelClose.

The HTLC balances could probably stay the same?

Hmm, is it possible we splice and change the commitment transaction format? That would impact the balances

For the local balance, when there's a splice pending, maybe we use a new variant over ClaimableOnChannelClose (e.g., MaybeClaimableOnChannelClose?) that gives you the balance for a particular FundingScope. Once the splice is no longer pending, we go back to ClaimableOnChannelClose.

Something like that, yea, it'd be nice to expose a vec of the possible options but makes sense.

Hmm, is it possible we splice and change the commitment transaction format? That would impact the balances

It is possible with splicing (e.g., anchors to taproot), but the spec as is today does not mention anything regarding it

lightning/src/ln/channel.rs

wpaulino · 2025-02-11T23:21:44Z

Is the thinking here that basically we'd implement things on the ScopedChannelContext and then run message handling twice, once on each context? Then we'd have to implement "undoing" the message on the first context, right? Or would we split most handling into a test then an accept method?

Yeah we basically want to guarantee immutability whenever we have multiple FundingScopes and are handling a message. It may end up in some large-ish refactors, but it seems better than having to undo state transitions? For example, with a pending splice, we'll receive a batch of two commit_sigs for each state update. For each commit_sig, we'll do the validation required before advancing our state, and only after all of them succeed, we advance our state after processing the last commit_sig of the batch.

TheBlueMatt · 2025-02-12T02:29:01Z

Generally, the idea for splice/rbf would be to maintain an additional FundingScopes for each outstanding splices/rbf. But, IIUC, you're saying there may be a field on ChannelContext that could change during handling, thus we'd need to account for that. Yeah, if that's the case, I'd prefer to update handling to use a test and accept approach to avoid undoing, which seems error prone.

Right, okay, yea, makes sense, just gonna be a lot of methods that we have to split into two.

TheBlueMatt · 2025-02-12T02:40:35Z

Conceptually I think that's probably the right approach, though, not sure how else we'd do it, just a pretty awkward API.

jkczyz

Addressed the feedback. I'm going to add another commit to move next_local_commitment_tx_fee_info_cached and next_remote_commitment_tx_fee_info_cached soon.

jkczyz · 2025-02-12T18:10:12Z

lightning/src/ln/channel.rs

+impl<SP: Deref> FundedChannel<SP>
+where
+	SP::Target: SignerProvider,
+	<SP::Target as SignerProvider>::EcdsaSigner: EcdsaChannelSigner,
+{


Added fixups along the way to revert to a single impl block.

jkczyz · 2025-02-12T20:20:11Z

lightning/src/ln/channel.rs

@@ -4257,7 +4313,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 			unbroadcasted_batch_funding_txid,
 			channel_id: self.channel_id,
 			user_channel_id: self.user_id,
-			channel_capacity_satoshis: self.channel_value_satoshis,
+			channel_capacity_satoshis: funding.channel_value_satoshis,


Discussed this a bit more offline. Seems when processing ShutdownResult in ChannelManager::finish_close_channel, instead of generating a ChannelClosed event there we should do so in ChannelMonitor. This will require including the ClosureReason in ChannelMonitorUpdateStep::ChannelForceClosed.

@TheBlueMatt Any thoughts on this? @wpaulino Feel free to fill in any more details that I may have left out.

optout21

LGTM so far, it's the right abstraction, in the good direction.

When establishing a channel, the funding transaction may be replaced either: - after the funding transaction has confirmed using splicing, - before the funding transaction has confirmed for v2 channel establishment using tx_init_rbf, or - before the splice's funding transaction has confirmed using tx_init_rbf. In each of these cases, fields in ChannelContext will need to be updated once the funding transaction confirms. Additionally, the same fields for a pending attempt may need to be considered instead of a previously confirmed funding. This commit introduces a FundingScope to hold the aforementioned fields. It lives next to ChannelContext and will be needed whenever these fields are accessed. The next few commits will move the relevant fields to FundingScope and provide access to them whenever needed, allowing to swap in another FundingScope when necessary.

jkczyz · 2025-02-18T20:11:01Z

Rebased to resolve merge conflict.

wpaulino

LGTM after squash

wpaulino · 2025-02-19T00:11:01Z

lightning/src/ln/channel.rs

+		#[cfg(any(test, fuzzing))]
+		funding: &FundingScope,


This feels a bit weird, should we move these methods to be on FundedChannel instead?

Hmm... this is called from ChannelContext::get_available_balances, though.

That's also only called on funded channels, but always passing it in now is fine

TheBlueMatt

Basically LGTM. I think ideally we walk back exposing the Funding to channelmanager.rs at all, but assuming we can we can also do that as this develops over coming PRs.

TheBlueMatt · 2025-02-19T01:14:57Z

lightning/src/ln/channel.rs

@@ -3805,9 +3868,17 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 			}

 			let htlc_above_dust = HTLCCandidate::new(real_dust_limit_timeout_sat * 1000, HTLCInitiator::LocalOffered);
-			let mut max_reserved_commit_tx_fee_msat = context.next_local_commit_tx_fee_msat(htlc_above_dust, Some(()));
+			let mut max_reserved_commit_tx_fee_msat = context.next_local_commit_tx_fee_msat(
+				#[cfg(any(test, fuzzing))]


Ha, we should probably either remove the fields and associated testing or always pass the funding. I think either is probably fine.

Going to always pass FundingScope since the follow-up PR alternative branch needs it.

TheBlueMatt · 2025-02-19T01:20:54Z

lightning/src/ln/channelmanager.rs

@@ -3075,7 +3075,7 @@ macro_rules! convert_channel_err {
 			ChannelError::Close((msg, reason)) => {
 				let logger = WithChannelContext::from(&$self.logger, &$context, None);
 				log_error!(logger, "Closing channel {} due to close-required error: {}", $channel_id, msg);
-				let mut shutdown_res = $context.force_shutdown(true, reason);
+				let mut shutdown_res = $context.force_shutdown($funding, true, reason);


Do we have to expose the Funding to channelmanager.rs at all? It seems like very much a Channel-internal thing that ideally we wouldn't have to deal with in channelmanager.rs (we just might get a Vec of things like channel amounts when we go to look).

We could probably introduce a force_shutdown method on Channel and have it delegate to ChannelContext using the underlying FundingScope. Likewise, with other methods. Here, we don't have a Channel, though. Might be a way to restructure these macros, but they are a bit of a maze, TBH.

Also, note, the use of locked_close_channel below will need a FundingScope in the follow-up to call get_funding_txo. That doesn't have a Channel, either, just a ChannelContext.

Once we finish all of the work here, there may be a nice way to clean this up.

Yea, I guess looking at it we really shouldn't be exposing the ChannelContext at all, either. After all this work is done we should really go through channel.rs, break it up a bit (now that we have several different structs we can draw neater lines!), clean up the API to channelmanager.rs (one struct, with channel.rs and new friends deciding whether it makes sense in the channel's state).

TheBlueMatt · 2025-02-19T22:44:11Z

Feel free to squash from my PoV. Where are you on this @wpaulino?

jkczyz · 2025-02-20T14:48:25Z

Squashed given @wpaulino seemed good with it earlier.

codecov · 2025-02-20T14:56:34Z

Codecov Report

Attention: Patch coverage is 97.56098% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.92%. Comparing base (2d2c542) to head (79ce104).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	96.87%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3592      +/-   ##
==========================================
+ Coverage   88.61%   88.92%   +0.30%     
==========================================
  Files         149      149              
  Lines      116877   119290    +2413     
  Branches   116877   119290    +2413     
==========================================
+ Hits       103572   106074    +2502     
+ Misses      10798    10775      -23     
+ Partials     2507     2441      -66

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

TheBlueMatt

Yea, reflecting on it more I'm not very happy with the state of our channel state (pun intended)...We've ended up with a ChannelState which describes the channel's state stored in an enum that describes the channel's state while exposing much too much to channelmanager.rs. As we make more progress here it would be great to clean this up as much as possible and if required do a "finish cleaning up" PR at the end to unify the state tracking, split files, and ensure a simple API to channelmanager as much as possible.

jkczyz requested a review from wpaulino February 7, 2025 23:14

jkczyz mentioned this pull request Feb 10, 2025

Follow-ups to PR 3137 #3423

Open

jkczyz force-pushed the 2025-02-channel-funding-scope branch from a8e46b7 to 18b4eac Compare February 11, 2025 18:07

jkczyz changed the title ~~Refactor ChannelContext fields into FundingScope~~ Refactor ChannelContext value fields into FundingScope Feb 11, 2025

jkczyz marked this pull request as ready for review February 11, 2025 18:27

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

jkczyz requested review from dunxen, optout21 and TheBlueMatt and removed request for dunxen February 11, 2025 18:30

jkczyz force-pushed the 2025-02-channel-funding-scope branch from 18b4eac to 5beaa01 Compare February 11, 2025 20:29

wpaulino reviewed Feb 11, 2025

View reviewed changes

jkczyz commented Feb 12, 2025

View reviewed changes

jkczyz force-pushed the 2025-02-channel-funding-scope branch from 5beaa01 to d2a32a7 Compare February 12, 2025 20:22

optout21 reviewed Feb 13, 2025

View reviewed changes

jkczyz mentioned this pull request Feb 14, 2025

Refactor funding-tx-related fields into FundingScope #3604

Draft

jkczyz force-pushed the 2025-02-channel-funding-scope branch from 9a11051 to 04abb58 Compare February 18, 2025 20:10

wpaulino reviewed Feb 19, 2025

View reviewed changes

TheBlueMatt reviewed Feb 19, 2025

View reviewed changes

jkczyz mentioned this pull request Feb 19, 2025

[Splicing] Clone for ChannelContext #3332

Closed

jkczyz added 5 commits February 20, 2025 08:47

Move channel_value_satoshis to FundingScope

770f90e

Move value_to_self_msat to FundingScope

5c7cf11

Move *_selected_channel_reserve_satoshis to FundedScope

6bfb5fb

Move *_max_commitment_tx_output to FundingScope

2d452fb

Move next_*_commitment_tx_fee_info_cached to FundingScope

79ce104

jkczyz force-pushed the 2025-02-channel-funding-scope branch from 15f1c21 to 79ce104 Compare February 20, 2025 14:47

TheBlueMatt approved these changes Feb 20, 2025

View reviewed changes

wpaulino approved these changes Feb 20, 2025

View reviewed changes

wpaulino merged commit 6cf270d into lightningdevkit:main Feb 20, 2025
24 of 26 checks passed

Refactor ChannelContext value fields into FundingScope #3592

Refactor ChannelContext value fields into FundingScope #3592

Conversation

jkczyz commented Feb 7, 2025 • edited Loading

jkczyz commented Feb 7, 2025

jkczyz commented Feb 10, 2025

jkczyz commented Feb 10, 2025

jkczyz commented Feb 11, 2025

jkczyz commented Feb 11, 2025

TheBlueMatt commented Feb 11, 2025

jkczyz commented Feb 11, 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

wpaulino commented Feb 11, 2025

TheBlueMatt commented Feb 12, 2025

TheBlueMatt commented Feb 12, 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

optout21 left a comment

Choose a reason for hiding this comment

jkczyz commented Feb 18, 2025

wpaulino 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

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

TheBlueMatt commented Feb 19, 2025

jkczyz commented Feb 20, 2025

codecov bot commented Feb 20, 2025 • edited Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

Refactor `ChannelContext` value fields into `FundingScope` #3592

Refactor `ChannelContext` value fields into `FundingScope` #3592

jkczyz commented Feb 7, 2025 •

edited

Loading

codecov bot commented Feb 20, 2025 •

edited

Loading