Introduce FundingScope abstraction to ChannelMonitor by wpaulino · Pull Request #3663 · lightningdevkit/rust-lightning

wpaulino · 2025-03-12T00:22:23Z

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 the ChannelMonitor will need to be updated once the new funding transaction confirms. This commit introduces a FundingScope to hold the aforementioned fields, allowing to swap in another FundingScope when necessary.

This is the same abstraction as in channel.rs representing a different set of data.

ldk-reviews-bot · 2025-03-12T00:22:26Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-03-12T00:32:04Z

Codecov Report

Attention: Patch coverage is 92.66055% with 16 lines in your changes missing coverage. Please review.

Project coverage is 89.18%. Comparing base (00ee0ef) to head (ef4e6f5).
Report is 27 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/chain/channelmonitor.rs	92.34%	5 Missing and 11 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3663      +/-   ##
==========================================
- Coverage   89.23%   89.18%   -0.06%     
==========================================
  Files         155      155              
  Lines      119327   119414      +87     
  Branches   119327   119414      +87     
==========================================
+ Hits       106482   106495      +13     
- Misses      10242    10301      +59     
- Partials     2603     2618      +15

☔ 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.

TheBlueMatt · 2025-03-12T18:07:27Z

lightning/src/chain/channelmonitor.rs

+	/// The set of outpoints in each counterparty commitment transaction. We always need at least
+	/// the payment hash from `HTLCOutputInCommitment` to claim even a revoked commitment
+	/// transaction broadcast as we need to be able to construct the witness script in all cases.
+	counterparty_claimable_outpoints: HashMap<Txid, Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>>,


Hmm, is there a way to avoid duplicating these for all state updates while a splice is pending? In theory it should be short but it seems like a DoS issue if someone can get 100 splices in-flight at once and then never confirm any and our memory usage goes up 100x.

Good call, we're definitely gonna need a new way of tracking these to avoid that. We could keep them at the ChannelMonitorImpl level and index them by the counterparty commitment number (since all scopes will be at the same commitment number). Then, we add a new map in FundingScope that maps txid -> commitment number.

Is there a reason for us using the txid over the commitment number in current main?

Hm, doing this will actually break the transaction_output_index, they are not guaranteed to remain consistent across all commitments/scopes.

It does seem like "HTLCOutputInCommiemnt's output_index is not like the other fields" keeps coming up....maybe time to finally refactor the type so that output_index is separate (cc @tankyleo)

yes I can certainly take a look. Hopefully this allows us to delete the brute force search we talked about last week, Matt, to populate the output indices.

Added a TODO(splicing) to make sure we don't miss this.

jkczyz

IIUC, a follow-up PR will have ChannelMonitor hold a FundingScope for each pending transaction?

lightning/src/chain/channelmonitor.rs

jkczyz · 2025-03-12T20:11:53Z

lightning/src/chain/channelmonitor.rs

-			channel_value_satoshis, channel_keys_id, destination_script.into(), keys,
-			channel_parameters.clone(), initial_holder_commitment_tx, secp_ctx
+			channel_parameters.channel_value_satoshis, channel_keys_id, destination_script.into(),
+			keys, channel_parameters.clone(), initial_holder_commitment_tx, secp_ctx


Will OnchainTxHandler hold parameters for each funding scope?

Unrelated to this PR, but I wonder if we need to write channel_parameters twice if we can pass it to OnchainTxHandler on read.

No, I plan to move all channel-specific data from OnchainTxHandler to ChannelMonitor, and provide it as needed to OnchainTxHandler via the outputs API.

wpaulino · 2025-03-12T22:31:18Z

IIUC, a follow-up PR will have ChannelMonitor hold a FundingScope for each pending transaction?

Yeah it will happen in the PR introducing splice monitor updates.

lightning/src/chain/channelmonitor.rs

jkczyz

LGTM modulo #3663 (comment).

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 the `ChannelMonitor` will need to be updated once the new funding transaction confirms. This commit introduces a `FundingScope` to hold the aforementioned fields, allowing to swap in another `FundingScope` when necessary. This is the same abstraction as in `channel.rs` representing a different set of data.

The `ChannelTransactionParameters` will change across `FundingScope`s, so we make sure to track it to ensure it is updated accordingly when a new `FundingScope` is applied/considered. Along the way, we also clean up some unnecessary function arguments to `ChannelMonitor::new` that we can just obtain from the `ChannelTransactionParameters` already provided.

wpaulino · 2025-03-13T17:22:14Z

Tacked on some more changes to the second commit to reference the ChannelTransactionParameters in the FundingScope as opposed to the OnchainTxHandler.

TheBlueMatt · 2025-03-14T19:39:18Z

lightning/src/chain/channelmonitor.rs

@@ -1623,7 +1650,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {

 	/// Gets the channel type of the corresponding channel.
 	pub fn channel_type_features(&self) -> ChannelTypeFeatures {


Honestly even the pub version can return a reference.

wpaulino requested a review from jkczyz March 12, 2025 00:22

wpaulino added the weekly goal Someone wants to land this this week label Mar 12, 2025

wpaulino force-pushed the channel-monitor-funding-scope branch from 114340b to 8b47754 Compare March 12, 2025 17:08

wpaulino mentioned this pull request Mar 12, 2025

Replace use of HolderSignedTx with HolderCommitment #3664

Merged

TheBlueMatt reviewed Mar 12, 2025

View reviewed changes

jkczyz reviewed Mar 12, 2025

View reviewed changes

wpaulino force-pushed the channel-monitor-funding-scope branch from 8b47754 to 6097917 Compare March 12, 2025 22:31

wpaulino requested review from TheBlueMatt and jkczyz March 12, 2025 22:32

TheBlueMatt reviewed Mar 13, 2025

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

jkczyz reviewed Mar 13, 2025

View reviewed changes

wpaulino added 2 commits March 13, 2025 10:20

wpaulino force-pushed the channel-monitor-funding-scope branch from 6097917 to ef4e6f5 Compare March 13, 2025 17:21

jkczyz approved these changes Mar 13, 2025

View reviewed changes

wpaulino requested a review from TheBlueMatt March 13, 2025 21:09

TheBlueMatt approved these changes Mar 14, 2025

View reviewed changes

TheBlueMatt merged commit 1fc2726 into lightningdevkit:main Mar 14, 2025
25 of 27 checks passed

wpaulino deleted the channel-monitor-funding-scope branch March 14, 2025 20:47

jkczyz mentioned this pull request Jun 21, 2025

Dual-funded channels and Splicing Project Tracking #1621

Closed

7 tasks

jkczyz mentioned this pull request Dec 12, 2025

Splicing / Dual Funding #4284

Open

3 tasks

		@@ -1623,7 +1650,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {

		/// Gets the channel type of the corresponding channel.
		pub fn channel_type_features(&self) -> ChannelTypeFeatures {

Conversation

wpaulino commented Mar 12, 2025

Uh oh!

ldk-reviews-bot commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Choose a reason for hiding this comment

Uh oh!

jkczyz 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!

wpaulino commented Mar 12, 2025

Uh oh!

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

wpaulino commented Mar 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ldk-reviews-bot commented Mar 12, 2025 •

edited

Loading

codecov bot commented Mar 12, 2025 •

edited

Loading