Refactor BroadcasterInterface to include TransactionType

[tnull]

Fixes#3566.

Add a `BroadcastType` enum to provide context about the type of
transaction being broadcast. This information can be useful for
logging, filtering, or prioritization purposes.
The `BroadcastType` variants are:
- `Funding`: A funding transaction establishing a new channel
- `CooperativeClose`: A cooperative close transaction
- `UnilateralClose`: A transaction for force-close
- `AnchorBump`: An anchor transaction for CPFP fee-bumping
- `Claim`: A transaction claiming outputs from commitment tx
- `Sweep`: A transaction sweeping spendable outputs to wallet
Co-Authored-By: HAL 9000

and:

We add the `ChannelId` as context to the just-added `BroadcastType`
enum.
Co-Authored-By: HAL 9000

[ldk-reviews-bot]

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

[TheBlueMatt]

docs need a lot of love

[TheBlueMatt]

I mean all transactions are mutually agreed upon by both parties? Maybe describe what a coop close tx means

[tnull]

Hmm, changed it up a bit, but let me know what exactly is missing for you.

[codecov]

Codecov Report

❌ Patch coverage is 84.09091% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.99%. Comparing base (8679d8d) to head (0715f4a).
⚠️ Report is 39 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/util/sweep.rs	79.59%	6 Missing and 4 partials ⚠️
lightning/src/ln/channelmanager.rs	72.72%	8 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	46.15%	6 Missing and 1 partial ⚠️
lightning-liquidity/src/lsps2/service.rs	85.71%	0 Missing and 1 partial ⚠️
lightning/src/chain/onchaintx.rs	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4353 +/- ##
==========================================
+ Coverage 85.98% 85.99% +0.01% 
==========================================
Files 156 156 Lines 102641 102734 +93 Branches 102641 102734 +93 ==========================================
+ Hits 88258 88349 +91 - Misses 11873 11875 +2 
Partials 2510 2510 

Flag	Coverage Δ
tests	85.99% <84.09%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[wpaulino]

Splice transactions should have their own variant, no?

[tnull]

Right, hence the question above regarding which variants we want. So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?

And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?

[TheBlueMatt]

So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?

If we want more splicing context we should include the splice metadata in the splice variant rather than having a separate variant for each splice init logic. This feels like a followup though.

[TheBlueMatt]

To clarify a bit - the differentiation between "splice out" and "splice in" at the top-level API is a bug that we're fixing - there is only a "splice" which can have constituent parts that are in + out. Its a useful differentiation when building splicing instructions but after that it should dissapear.

[wpaulino]

And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?

The raw transaction is already available since we're broadcasting it so they can inspect its inputs and outputs if they want to distinguish between splice in/out.

[tnull]

The raw transaction is already available since we're broadcasting it so they can inspect its inputs and outputs if they want to distinguish between splice in/out.

Will this also work going forward with dual funding, given that then we might reuse the same broadcast_interactive_funding path? Or would we get wrong classifications then, if we don't track the 'intent' from the beginning?

[TheBlueMatt]

Hmm, but for the user it still makes a fundamental difference, no? So in the actual API we still want to discern in/out, right?

Sure at the constructor level we have utilities that allow you to only do one, but the right API definitely isn't a tri-state where the third state is "both". Rather, where we want to expose it it should be a "does this splice have inputs from me" and "does this splice have outputs I added" methods.

[wpaulino]

Will this also work going forward with dual funding, given that then we might reuse the same broadcast_interactive_funding path? Or would we get wrong classifications then, if we don't track the 'intent' from the beginning?

Dual-funding should keep the Funding type and we can just return that along with the transaction since the ChannelManager doesn't have enough context to determine the correct one.

[tnull]

Discussed offline: for now, just add a Splice variant. While we want to add more context (in/out etc) to the Splice variant soon, for now we just need to return and hand through the TransactionType together with the Transaction from its origin.

[tnull]

Now updated accordingly.

[tnull]

FWIW, I also now pushed a fixup renaming to TransactionType which seemed like a slightly better naming, let me know if you object.

[wpaulino]

This needs a rebase now, feel free to squash. There's one pending comment from Matt left that still needs to be addressed.

[tnull]

This needs a rebase now, feel free to squash. There's one pending comment from Matt left that still needs to be addressed.

Rebased and also added a commit that adds some test coverage for the TransactionType::Splice case.

[TheBlueMatt]

few comments on the docs, otherwise feel free to squash.

[tnull]

few comments on the docs, otherwise feel free to squash.

Squashed with the following changes:

diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs
index cbac84aaf..90f4aa13e 100644
--- a/lightning/src/chain/chaininterface.rs+++ b/lightning/src/chain/chaininterface.rs@@ -41,5 +41,6 @@ pub enum TransactionType {
/// A transaction cooperatively closing a channel.
///
-	/// A transaction of this type will be broadcast when cooperatively closing a channel via [`ChannelManager::close_channel`].+	/// A transaction of this type will be broadcast when cooperatively closing a channel via+	/// [`ChannelManager::close_channel`] or if the counterparty closes the channel.
///
/// [`ChannelManager::close_channel`]: crate::ln::channelmanager::ChannelManager::close_channel
@@ -50,5 +51,7 @@ pub enum TransactionType {
/// A transaction being broadcast to force-close the channel.
///
-	/// A transaction of this type will be broadcast when unilaterally closing a channel via [`ChannelManager::force_close_broadcasting_latest_txn`].+	/// A transaction of this type will be broadcast when unilaterally closing a channel via+	/// [`ChannelManager::force_close_broadcasting_latest_txn`] or if the counterparty force-closes+	/// the channel..
///
/// [`ChannelManager::force_close_broadcasting_latest_txn`]: crate::ln::channelmanager::ChannelManager::force_close_broadcasting_latest_txn
@@ -67,5 +70,15 @@ pub enum TransactionType {
channel_id: ChannelId,
},
-	/// A transaction claiming outputs from a commitment transaction (HTLC claims, penalty/justice).+	/// A transaction which is resolving an output spendable by both us and our counterparty.+	///+	/// When a channel closes via the unilateral close path, there may be transaction outputs which+	/// are spendable by either our counterparty or us and represent some lightning state. In order+	/// to resolve that state, the [`ChannelMonitor`] will spend any such outputs, ensuring funds+	/// are only available to us prior to generating an [`Event::SpendableOutputs`]. This+	/// transaction is one such transaction - resolving in-flight HTLCs or punishing our+	/// counterparty if they broadcasted an outdated state.+	///+	/// [`ChannelMonitor`]: crate::chain::ChannelMonitor+	/// [`Event::SpendableOutputs`]: crate::events::Event::SpendableOutputs
Claim {
/// The ID of the channel from which outputs are being claimed.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[TheBlueMatt]

Gonna just land this so the bulk of the code is out of the way, but we really need counterparty node ids if we're gonna use channel ids.

[TheBlueMatt]

oops, note that because we're still not living in a funding-v2-required world ChannelIds aren't the right unique indicator of a specific channel. We need the counterparty node id as well. We should also (obviously) provide the user_channel_id here.

[tnull]

Hmm, good point regarding the counterparty_node_id, though it seems we then first need it to add to a few other places. For example, we don't track it in the SpendableOutputs event (only channel_id) and therefore we also don't track it in OutputSweeper.

[tnull]

Done in #4393

[tnull]

Fixes#3566.

Add a `BroadcastType` enum to provide context about the type of
transaction being broadcast. This information can be useful for
logging, filtering, or prioritization purposes.
The `BroadcastType` variants are:
- `Funding`: A funding transaction establishing a new channel
- `CooperativeClose`: A cooperative close transaction
- `UnilateralClose`: A transaction for force-close
- `AnchorBump`: An anchor transaction for CPFP fee-bumping
- `Claim`: A transaction claiming outputs from commitment tx
- `Sweep`: A transaction sweeping spendable outputs to wallet
Co-Authored-By: HAL 9000

and:

We add the `ChannelId` as context to the just-added `BroadcastType`
enum.
Co-Authored-By: HAL 9000

[ldk-reviews-bot]

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

[TheBlueMatt]

docs need a lot of love

[TheBlueMatt]

I mean all transactions are mutually agreed upon by both parties? Maybe describe what a coop close tx means

[tnull]

Hmm, changed it up a bit, but let me know what exactly is missing for you.

[codecov]

Codecov Report

❌ Patch coverage is 84.09091% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.99%. Comparing base (8679d8d) to head (0715f4a).
⚠️ Report is 39 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/util/sweep.rs	79.59%	6 Missing and 4 partials ⚠️
lightning/src/ln/channelmanager.rs	72.72%	8 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	46.15%	6 Missing and 1 partial ⚠️
lightning-liquidity/src/lsps2/service.rs	85.71%	0 Missing and 1 partial ⚠️
lightning/src/chain/onchaintx.rs	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4353 +/- ##
==========================================
+ Coverage 85.98% 85.99% +0.01% 
==========================================
Files 156 156 Lines 102641 102734 +93 Branches 102641 102734 +93 ==========================================
+ Hits 88258 88349 +91 - Misses 11873 11875 +2 
Partials 2510 2510 

Flag	Coverage Δ
tests	85.99% <84.09%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[wpaulino]

Splice transactions should have their own variant, no?

[tnull]

Right, hence the question above regarding which variants we want. So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?

And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?

[TheBlueMatt]

So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?

If we want more splicing context we should include the splice metadata in the splice variant rather than having a separate variant for each splice init logic. This feels like a followup though.

[TheBlueMatt]

To clarify a bit - the differentiation between "splice out" and "splice in" at the top-level API is a bug that we're fixing - there is only a "splice" which can have constituent parts that are in + out. Its a useful differentiation when building splicing instructions but after that it should dissapear.

[wpaulino]

And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?

The raw transaction is already available since we're broadcasting it so they can inspect its inputs and outputs if they want to distinguish between splice in/out.

[tnull]

The raw transaction is already available since we're broadcasting it so they can inspect its inputs and outputs if they want to distinguish between splice in/out.

Will this also work going forward with dual funding, given that then we might reuse the same broadcast_interactive_funding path? Or would we get wrong classifications then, if we don't track the 'intent' from the beginning?

[TheBlueMatt]

Hmm, but for the user it still makes a fundamental difference, no? So in the actual API we still want to discern in/out, right?

Sure at the constructor level we have utilities that allow you to only do one, but the right API definitely isn't a tri-state where the third state is "both". Rather, where we want to expose it it should be a "does this splice have inputs from me" and "does this splice have outputs I added" methods.

[wpaulino]

Will this also work going forward with dual funding, given that then we might reuse the same broadcast_interactive_funding path? Or would we get wrong classifications then, if we don't track the 'intent' from the beginning?

Dual-funding should keep the Funding type and we can just return that along with the transaction since the ChannelManager doesn't have enough context to determine the correct one.

[tnull]

Discussed offline: for now, just add a Splice variant. While we want to add more context (in/out etc) to the Splice variant soon, for now we just need to return and hand through the TransactionType together with the Transaction from its origin.

[tnull]

Now updated accordingly.

[tnull]

FWIW, I also now pushed a fixup renaming to TransactionType which seemed like a slightly better naming, let me know if you object.

[wpaulino]

This needs a rebase now, feel free to squash. There's one pending comment from Matt left that still needs to be addressed.

[tnull]

This needs a rebase now, feel free to squash. There's one pending comment from Matt left that still needs to be addressed.

Rebased and also added a commit that adds some test coverage for the TransactionType::Splice case.

[TheBlueMatt]

few comments on the docs, otherwise feel free to squash.

[tnull]

few comments on the docs, otherwise feel free to squash.

Squashed with the following changes:

diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs
index cbac84aaf..90f4aa13e 100644
--- a/lightning/src/chain/chaininterface.rs+++ b/lightning/src/chain/chaininterface.rs@@ -41,5 +41,6 @@ pub enum TransactionType {
/// A transaction cooperatively closing a channel.
///
-	/// A transaction of this type will be broadcast when cooperatively closing a channel via [`ChannelManager::close_channel`].+	/// A transaction of this type will be broadcast when cooperatively closing a channel via+	/// [`ChannelManager::close_channel`] or if the counterparty closes the channel.
///
/// [`ChannelManager::close_channel`]: crate::ln::channelmanager::ChannelManager::close_channel
@@ -50,5 +51,7 @@ pub enum TransactionType {
/// A transaction being broadcast to force-close the channel.
///
-	/// A transaction of this type will be broadcast when unilaterally closing a channel via [`ChannelManager::force_close_broadcasting_latest_txn`].+	/// A transaction of this type will be broadcast when unilaterally closing a channel via+	/// [`ChannelManager::force_close_broadcasting_latest_txn`] or if the counterparty force-closes+	/// the channel..
///
/// [`ChannelManager::force_close_broadcasting_latest_txn`]: crate::ln::channelmanager::ChannelManager::force_close_broadcasting_latest_txn
@@ -67,5 +70,15 @@ pub enum TransactionType {
channel_id: ChannelId,
},
-	/// A transaction claiming outputs from a commitment transaction (HTLC claims, penalty/justice).+	/// A transaction which is resolving an output spendable by both us and our counterparty.+	///+	/// When a channel closes via the unilateral close path, there may be transaction outputs which+	/// are spendable by either our counterparty or us and represent some lightning state. In order+	/// to resolve that state, the [`ChannelMonitor`] will spend any such outputs, ensuring funds+	/// are only available to us prior to generating an [`Event::SpendableOutputs`]. This+	/// transaction is one such transaction - resolving in-flight HTLCs or punishing our+	/// counterparty if they broadcasted an outdated state.+	///+	/// [`ChannelMonitor`]: crate::chain::ChannelMonitor+	/// [`Event::SpendableOutputs`]: crate::events::Event::SpendableOutputs
Claim {
/// The ID of the channel from which outputs are being claimed.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[TheBlueMatt]

Gonna just land this so the bulk of the code is out of the way, but we really need counterparty node ids if we're gonna use channel ids.

[TheBlueMatt]

oops, note that because we're still not living in a funding-v2-required world ChannelIds aren't the right unique indicator of a specific channel. We need the counterparty node id as well. We should also (obviously) provide the user_channel_id here.

[tnull]

Hmm, good point regarding the counterparty_node_id, though it seems we then first need it to add to a few other places. For example, we don't track it in the SpendableOutputs event (only channel_id) and therefore we also don't track it in OutputSweeper.

[tnull]

Done in #4393

[tnull]

Fixes#3566.

Add a `BroadcastType` enum to provide context about the type of
transaction being broadcast. This information can be useful for
logging, filtering, or prioritization purposes.
The `BroadcastType` variants are:
- `Funding`: A funding transaction establishing a new channel
- `CooperativeClose`: A cooperative close transaction
- `UnilateralClose`: A transaction for force-close
- `AnchorBump`: An anchor transaction for CPFP fee-bumping
- `Claim`: A transaction claiming outputs from commitment tx
- `Sweep`: A transaction sweeping spendable outputs to wallet
Co-Authored-By: HAL 9000

and:

We add the `ChannelId` as context to the just-added `BroadcastType`
enum.
Co-Authored-By: HAL 9000

[ldk-reviews-bot]

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

[TheBlueMatt]

docs need a lot of love

[TheBlueMatt]

I mean all transactions are mutually agreed upon by both parties? Maybe describe what a coop close tx means

[tnull]

Hmm, changed it up a bit, but let me know what exactly is missing for you.

[codecov]

Codecov Report

❌ Patch coverage is 84.09091% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.99%. Comparing base (8679d8d) to head (0715f4a).
⚠️ Report is 39 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/util/sweep.rs	79.59%	6 Missing and 4 partials ⚠️
lightning/src/ln/channelmanager.rs	72.72%	8 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	46.15%	6 Missing and 1 partial ⚠️
lightning-liquidity/src/lsps2/service.rs	85.71%	0 Missing and 1 partial ⚠️
lightning/src/chain/onchaintx.rs	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4353 +/- ##
==========================================
+ Coverage 85.98% 85.99% +0.01% 
==========================================
Files 156 156 Lines 102641 102734 +93 Branches 102641 102734 +93 ==========================================
+ Hits 88258 88349 +91 - Misses 11873 11875 +2 
Partials 2510 2510 

Flag	Coverage Δ
tests	85.99% <84.09%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[wpaulino]

Splice transactions should have their own variant, no?

[tnull]

Right, hence the question above regarding which variants we want. So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?

And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?

[TheBlueMatt]

So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?

If we want more splicing context we should include the splice metadata in the splice variant rather than having a separate variant for each splice init logic. This feels like a followup though.

[TheBlueMatt]

To clarify a bit - the differentiation between "splice out" and "splice in" at the top-level API is a bug that we're fixing - there is only a "splice" which can have constituent parts that are in + out. Its a useful differentiation when building splicing instructions but after that it should dissapear.

[wpaulino]

And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?

The raw transaction is already available since we're broadcasting it so they can inspect its inputs and outputs if they want to distinguish between splice in/out.

[tnull]

The raw transaction is already available since we're broadcasting it so they can inspect its inputs and outputs if they want to distinguish between splice in/out.

Will this also work going forward with dual funding, given that then we might reuse the same broadcast_interactive_funding path? Or would we get wrong classifications then, if we don't track the 'intent' from the beginning?

[TheBlueMatt]

Hmm, but for the user it still makes a fundamental difference, no? So in the actual API we still want to discern in/out, right?

Sure at the constructor level we have utilities that allow you to only do one, but the right API definitely isn't a tri-state where the third state is "both". Rather, where we want to expose it it should be a "does this splice have inputs from me" and "does this splice have outputs I added" methods.

[wpaulino]

Will this also work going forward with dual funding, given that then we might reuse the same broadcast_interactive_funding path? Or would we get wrong classifications then, if we don't track the 'intent' from the beginning?

Dual-funding should keep the Funding type and we can just return that along with the transaction since the ChannelManager doesn't have enough context to determine the correct one.

[tnull]

Discussed offline: for now, just add a Splice variant. While we want to add more context (in/out etc) to the Splice variant soon, for now we just need to return and hand through the TransactionType together with the Transaction from its origin.

[tnull]

Now updated accordingly.

[tnull]

FWIW, I also now pushed a fixup renaming to TransactionType which seemed like a slightly better naming, let me know if you object.

[wpaulino]

This needs a rebase now, feel free to squash. There's one pending comment from Matt left that still needs to be addressed.

[tnull]

This needs a rebase now, feel free to squash. There's one pending comment from Matt left that still needs to be addressed.

Rebased and also added a commit that adds some test coverage for the TransactionType::Splice case.

[TheBlueMatt]

few comments on the docs, otherwise feel free to squash.

[tnull]

few comments on the docs, otherwise feel free to squash.

Squashed with the following changes:

diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs
index cbac84aaf..90f4aa13e 100644
--- a/lightning/src/chain/chaininterface.rs+++ b/lightning/src/chain/chaininterface.rs@@ -41,5 +41,6 @@ pub enum TransactionType {
/// A transaction cooperatively closing a channel.
///
-	/// A transaction of this type will be broadcast when cooperatively closing a channel via [`ChannelManager::close_channel`].+	/// A transaction of this type will be broadcast when cooperatively closing a channel via+	/// [`ChannelManager::close_channel`] or if the counterparty closes the channel.
///
/// [`ChannelManager::close_channel`]: crate::ln::channelmanager::ChannelManager::close_channel
@@ -50,5 +51,7 @@ pub enum TransactionType {
/// A transaction being broadcast to force-close the channel.
///
-	/// A transaction of this type will be broadcast when unilaterally closing a channel via [`ChannelManager::force_close_broadcasting_latest_txn`].+	/// A transaction of this type will be broadcast when unilaterally closing a channel via+	/// [`ChannelManager::force_close_broadcasting_latest_txn`] or if the counterparty force-closes+	/// the channel..
///
/// [`ChannelManager::force_close_broadcasting_latest_txn`]: crate::ln::channelmanager::ChannelManager::force_close_broadcasting_latest_txn
@@ -67,5 +70,15 @@ pub enum TransactionType {
channel_id: ChannelId,
},
-	/// A transaction claiming outputs from a commitment transaction (HTLC claims, penalty/justice).+	/// A transaction which is resolving an output spendable by both us and our counterparty.+	///+	/// When a channel closes via the unilateral close path, there may be transaction outputs which+	/// are spendable by either our counterparty or us and represent some lightning state. In order+	/// to resolve that state, the [`ChannelMonitor`] will spend any such outputs, ensuring funds+	/// are only available to us prior to generating an [`Event::SpendableOutputs`]. This+	/// transaction is one such transaction - resolving in-flight HTLCs or punishing our+	/// counterparty if they broadcasted an outdated state.+	///+	/// [`ChannelMonitor`]: crate::chain::ChannelMonitor+	/// [`Event::SpendableOutputs`]: crate::events::Event::SpendableOutputs
Claim {
/// The ID of the channel from which outputs are being claimed.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[TheBlueMatt]

Gonna just land this so the bulk of the code is out of the way, but we really need counterparty node ids if we're gonna use channel ids.

[TheBlueMatt]

oops, note that because we're still not living in a funding-v2-required world ChannelIds aren't the right unique indicator of a specific channel. We need the counterparty node id as well. We should also (obviously) provide the user_channel_id here.

[tnull]

Hmm, good point regarding the counterparty_node_id, though it seems we then first need it to add to a few other places. For example, we don't track it in the SpendableOutputs event (only channel_id) and therefore we also don't track it in OutputSweeper.

[tnull]

Done in #4393

[tnull]

Fixes#3566.

Add a `BroadcastType` enum to provide context about the type of
transaction being broadcast. This information can be useful for
logging, filtering, or prioritization purposes.
The `BroadcastType` variants are:
- `Funding`: A funding transaction establishing a new channel
- `CooperativeClose`: A cooperative close transaction
- `UnilateralClose`: A transaction for force-close
- `AnchorBump`: An anchor transaction for CPFP fee-bumping
- `Claim`: A transaction claiming outputs from commitment tx
- `Sweep`: A transaction sweeping spendable outputs to wallet
Co-Authored-By: HAL 9000

and:

We add the `ChannelId` as context to the just-added `BroadcastType`
enum.
Co-Authored-By: HAL 9000

[ldk-reviews-bot]

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

[TheBlueMatt]

docs need a lot of love

[TheBlueMatt]

I mean all transactions are mutually agreed upon by both parties? Maybe describe what a coop close tx means

[tnull]

Hmm, changed it up a bit, but let me know what exactly is missing for you.

[codecov]

Codecov Report

❌ Patch coverage is 84.09091% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.99%. Comparing base (8679d8d) to head (0715f4a).
⚠️ Report is 39 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/util/sweep.rs	79.59%	6 Missing and 4 partials ⚠️
lightning/src/ln/channelmanager.rs	72.72%	8 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	46.15%	6 Missing and 1 partial ⚠️
lightning-liquidity/src/lsps2/service.rs	85.71%	0 Missing and 1 partial ⚠️
lightning/src/chain/onchaintx.rs	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4353 +/- ##
==========================================
+ Coverage 85.98% 85.99% +0.01% 
==========================================
Files 156 156 Lines 102641 102734 +93 Branches 102641 102734 +93 ==========================================
+ Hits 88258 88349 +91 - Misses 11873 11875 +2 
Partials 2510 2510 

Flag	Coverage Δ
tests	85.99% <84.09%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[wpaulino]

Splice transactions should have their own variant, no?

[tnull]

Right, hence the question above regarding which variants we want. So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?

And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?

[TheBlueMatt]

So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?

If we want more splicing context we should include the splice metadata in the splice variant rather than having a separate variant for each splice init logic. This feels like a followup though.

[TheBlueMatt]

To clarify a bit - the differentiation between "splice out" and "splice in" at the top-level API is a bug that we're fixing - there is only a "splice" which can have constituent parts that are in + out. Its a useful differentiation when building splicing instructions but after that it should dissapear.

[wpaulino]

And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?

The raw transaction is already available since we're broadcasting it so they can inspect its inputs and outputs if they want to distinguish between splice in/out.

[tnull]

The raw transaction is already available since we're broadcasting it so they can inspect its inputs and outputs if they want to distinguish between splice in/out.

Will this also work going forward with dual funding, given that then we might reuse the same broadcast_interactive_funding path? Or would we get wrong classifications then, if we don't track the 'intent' from the beginning?

[TheBlueMatt]

Hmm, but for the user it still makes a fundamental difference, no? So in the actual API we still want to discern in/out, right?

Sure at the constructor level we have utilities that allow you to only do one, but the right API definitely isn't a tri-state where the third state is "both". Rather, where we want to expose it it should be a "does this splice have inputs from me" and "does this splice have outputs I added" methods.

[wpaulino]

Will this also work going forward with dual funding, given that then we might reuse the same broadcast_interactive_funding path? Or would we get wrong classifications then, if we don't track the 'intent' from the beginning?

Dual-funding should keep the Funding type and we can just return that along with the transaction since the ChannelManager doesn't have enough context to determine the correct one.

[tnull]

Discussed offline: for now, just add a Splice variant. While we want to add more context (in/out etc) to the Splice variant soon, for now we just need to return and hand through the TransactionType together with the Transaction from its origin.

[tnull]

Now updated accordingly.

[tnull]

FWIW, I also now pushed a fixup renaming to TransactionType which seemed like a slightly better naming, let me know if you object.

[wpaulino]

This needs a rebase now, feel free to squash. There's one pending comment from Matt left that still needs to be addressed.

[tnull]

This needs a rebase now, feel free to squash. There's one pending comment from Matt left that still needs to be addressed.

Rebased and also added a commit that adds some test coverage for the TransactionType::Splice case.

[TheBlueMatt]

few comments on the docs, otherwise feel free to squash.

[tnull]

few comments on the docs, otherwise feel free to squash.

Squashed with the following changes:

diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs
index cbac84aaf..90f4aa13e 100644
--- a/lightning/src/chain/chaininterface.rs+++ b/lightning/src/chain/chaininterface.rs@@ -41,5 +41,6 @@ pub enum TransactionType {
/// A transaction cooperatively closing a channel.
///
-	/// A transaction of this type will be broadcast when cooperatively closing a channel via [`ChannelManager::close_channel`].+	/// A transaction of this type will be broadcast when cooperatively closing a channel via+	/// [`ChannelManager::close_channel`] or if the counterparty closes the channel.
///
/// [`ChannelManager::close_channel`]: crate::ln::channelmanager::ChannelManager::close_channel
@@ -50,5 +51,7 @@ pub enum TransactionType {
/// A transaction being broadcast to force-close the channel.
///
-	/// A transaction of this type will be broadcast when unilaterally closing a channel via [`ChannelManager::force_close_broadcasting_latest_txn`].+	/// A transaction of this type will be broadcast when unilaterally closing a channel via+	/// [`ChannelManager::force_close_broadcasting_latest_txn`] or if the counterparty force-closes+	/// the channel..
///
/// [`ChannelManager::force_close_broadcasting_latest_txn`]: crate::ln::channelmanager::ChannelManager::force_close_broadcasting_latest_txn
@@ -67,5 +70,15 @@ pub enum TransactionType {
channel_id: ChannelId,
},
-	/// A transaction claiming outputs from a commitment transaction (HTLC claims, penalty/justice).+	/// A transaction which is resolving an output spendable by both us and our counterparty.+	///+	/// When a channel closes via the unilateral close path, there may be transaction outputs which+	/// are spendable by either our counterparty or us and represent some lightning state. In order+	/// to resolve that state, the [`ChannelMonitor`] will spend any such outputs, ensuring funds+	/// are only available to us prior to generating an [`Event::SpendableOutputs`]. This+	/// transaction is one such transaction - resolving in-flight HTLCs or punishing our+	/// counterparty if they broadcasted an outdated state.+	///+	/// [`ChannelMonitor`]: crate::chain::ChannelMonitor+	/// [`Event::SpendableOutputs`]: crate::events::Event::SpendableOutputs
Claim {
/// The ID of the channel from which outputs are being claimed.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[TheBlueMatt]

Gonna just land this so the bulk of the code is out of the way, but we really need counterparty node ids if we're gonna use channel ids.

[TheBlueMatt]

oops, note that because we're still not living in a funding-v2-required world ChannelIds aren't the right unique indicator of a specific channel. We need the counterparty node id as well. We should also (obviously) provide the user_channel_id here.

[tnull]

Hmm, good point regarding the counterparty_node_id, though it seems we then first need it to add to a few other places. For example, we don't track it in the SpendableOutputs event (only channel_id) and therefore we also don't track it in OutputSweeper.

[tnull]

Done in #4393

[tnull]

Fixes#3566.

Add a `BroadcastType` enum to provide context about the type of
transaction being broadcast. This information can be useful for
logging, filtering, or prioritization purposes.
The `BroadcastType` variants are:
- `Funding`: A funding transaction establishing a new channel
- `CooperativeClose`: A cooperative close transaction
- `UnilateralClose`: A transaction for force-close
- `AnchorBump`: An anchor transaction for CPFP fee-bumping
- `Claim`: A transaction claiming outputs from commitment tx
- `Sweep`: A transaction sweeping spendable outputs to wallet
Co-Authored-By: HAL 9000

and:

We add the `ChannelId` as context to the just-added `BroadcastType`
enum.
Co-Authored-By: HAL 9000

[ldk-reviews-bot]

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

[TheBlueMatt]

docs need a lot of love

[TheBlueMatt]

I mean all transactions are mutually agreed upon by both parties? Maybe describe what a coop close tx means

[tnull]

Hmm, changed it up a bit, but let me know what exactly is missing for you.

[codecov]

Codecov Report

❌ Patch coverage is 84.09091% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.99%. Comparing base (8679d8d) to head (0715f4a).
⚠️ Report is 39 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/util/sweep.rs	79.59%	6 Missing and 4 partials ⚠️
lightning/src/ln/channelmanager.rs	72.72%	8 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	46.15%	6 Missing and 1 partial ⚠️
lightning-liquidity/src/lsps2/service.rs	85.71%	0 Missing and 1 partial ⚠️
lightning/src/chain/onchaintx.rs	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4353 +/- ##
==========================================
+ Coverage 85.98% 85.99% +0.01% 
==========================================
Files 156 156 Lines 102641 102734 +93 Branches 102641 102734 +93 ==========================================
+ Hits 88258 88349 +91 - Misses 11873 11875 +2 
Partials 2510 2510 

Flag	Coverage Δ
tests	85.99% <84.09%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[wpaulino]

Splice transactions should have their own variant, no?

[tnull]

Right, hence the question above regarding which variants we want. So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?

And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?

[TheBlueMatt]

So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?

If we want more splicing context we should include the splice metadata in the splice variant rather than having a separate variant for each splice init logic. This feels like a followup though.

[TheBlueMatt]

To clarify a bit - the differentiation between "splice out" and "splice in" at the top-level API is a bug that we're fixing - there is only a "splice" which can have constituent parts that are in + out. Its a useful differentiation when building splicing instructions but after that it should dissapear.

[wpaulino]

And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?

The raw transaction is already available since we're broadcasting it so they can inspect its inputs and outputs if they want to distinguish between splice in/out.

[tnull]

The raw transaction is already available since we're broadcasting it so they can inspect its inputs and outputs if they want to distinguish between splice in/out.

Will this also work going forward with dual funding, given that then we might reuse the same broadcast_interactive_funding path? Or would we get wrong classifications then, if we don't track the 'intent' from the beginning?

[TheBlueMatt]

Hmm, but for the user it still makes a fundamental difference, no? So in the actual API we still want to discern in/out, right?

Sure at the constructor level we have utilities that allow you to only do one, but the right API definitely isn't a tri-state where the third state is "both". Rather, where we want to expose it it should be a "does this splice have inputs from me" and "does this splice have outputs I added" methods.

[wpaulino]

Will this also work going forward with dual funding, given that then we might reuse the same broadcast_interactive_funding path? Or would we get wrong classifications then, if we don't track the 'intent' from the beginning?

Dual-funding should keep the Funding type and we can just return that along with the transaction since the ChannelManager doesn't have enough context to determine the correct one.

[tnull]

Discussed offline: for now, just add a Splice variant. While we want to add more context (in/out etc) to the Splice variant soon, for now we just need to return and hand through the TransactionType together with the Transaction from its origin.

[tnull]

Now updated accordingly.

[tnull]

FWIW, I also now pushed a fixup renaming to TransactionType which seemed like a slightly better naming, let me know if you object.

[wpaulino]

This needs a rebase now, feel free to squash. There's one pending comment from Matt left that still needs to be addressed.

[tnull]

This needs a rebase now, feel free to squash. There's one pending comment from Matt left that still needs to be addressed.

Rebased and also added a commit that adds some test coverage for the TransactionType::Splice case.

[TheBlueMatt]

few comments on the docs, otherwise feel free to squash.

[tnull]

few comments on the docs, otherwise feel free to squash.

Squashed with the following changes:

diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs
index cbac84aaf..90f4aa13e 100644
--- a/lightning/src/chain/chaininterface.rs+++ b/lightning/src/chain/chaininterface.rs@@ -41,5 +41,6 @@ pub enum TransactionType {
/// A transaction cooperatively closing a channel.
///
-	/// A transaction of this type will be broadcast when cooperatively closing a channel via [`ChannelManager::close_channel`].+	/// A transaction of this type will be broadcast when cooperatively closing a channel via+	/// [`ChannelManager::close_channel`] or if the counterparty closes the channel.
///
/// [`ChannelManager::close_channel`]: crate::ln::channelmanager::ChannelManager::close_channel
@@ -50,5 +51,7 @@ pub enum TransactionType {
/// A transaction being broadcast to force-close the channel.
///
-	/// A transaction of this type will be broadcast when unilaterally closing a channel via [`ChannelManager::force_close_broadcasting_latest_txn`].+	/// A transaction of this type will be broadcast when unilaterally closing a channel via+	/// [`ChannelManager::force_close_broadcasting_latest_txn`] or if the counterparty force-closes+	/// the channel..
///
/// [`ChannelManager::force_close_broadcasting_latest_txn`]: crate::ln::channelmanager::ChannelManager::force_close_broadcasting_latest_txn
@@ -67,5 +70,15 @@ pub enum TransactionType {
channel_id: ChannelId,
},
-	/// A transaction claiming outputs from a commitment transaction (HTLC claims, penalty/justice).+	/// A transaction which is resolving an output spendable by both us and our counterparty.+	///+	/// When a channel closes via the unilateral close path, there may be transaction outputs which+	/// are spendable by either our counterparty or us and represent some lightning state. In order+	/// to resolve that state, the [`ChannelMonitor`] will spend any such outputs, ensuring funds+	/// are only available to us prior to generating an [`Event::SpendableOutputs`]. This+	/// transaction is one such transaction - resolving in-flight HTLCs or punishing our+	/// counterparty if they broadcasted an outdated state.+	///+	/// [`ChannelMonitor`]: crate::chain::ChannelMonitor+	/// [`Event::SpendableOutputs`]: crate::events::Event::SpendableOutputs
Claim {
/// The ID of the channel from which outputs are being claimed.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[TheBlueMatt]

Gonna just land this so the bulk of the code is out of the way, but we really need counterparty node ids if we're gonna use channel ids.

[TheBlueMatt]

oops, note that because we're still not living in a funding-v2-required world ChannelIds aren't the right unique indicator of a specific channel. We need the counterparty node id as well. We should also (obviously) provide the user_channel_id here.

[tnull]

Hmm, good point regarding the counterparty_node_id, though it seems we then first need it to add to a few other places. For example, we don't track it in the SpendableOutputs event (only channel_id) and therefore we also don't track it in OutputSweeper.

[tnull]

Done in #4393

[tnull]

Fixes#3566.

Add a `BroadcastType` enum to provide context about the type of
transaction being broadcast. This information can be useful for
logging, filtering, or prioritization purposes.
The `BroadcastType` variants are:
- `Funding`: A funding transaction establishing a new channel
- `CooperativeClose`: A cooperative close transaction
- `UnilateralClose`: A transaction for force-close
- `AnchorBump`: An anchor transaction for CPFP fee-bumping
- `Claim`: A transaction claiming outputs from commitment tx
- `Sweep`: A transaction sweeping spendable outputs to wallet
Co-Authored-By: HAL 9000

and:

We add the `ChannelId` as context to the just-added `BroadcastType`
enum.
Co-Authored-By: HAL 9000

[ldk-reviews-bot]

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

[TheBlueMatt]

docs need a lot of love

[TheBlueMatt]

I mean all transactions are mutually agreed upon by both parties? Maybe describe what a coop close tx means

[tnull]

Hmm, changed it up a bit, but let me know what exactly is missing for you.

[codecov]

Codecov Report

❌ Patch coverage is 84.09091% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.99%. Comparing base (8679d8d) to head (0715f4a).
⚠️ Report is 39 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/util/sweep.rs	79.59%	6 Missing and 4 partials ⚠️
lightning/src/ln/channelmanager.rs	72.72%	8 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	46.15%	6 Missing and 1 partial ⚠️
lightning-liquidity/src/lsps2/service.rs	85.71%	0 Missing and 1 partial ⚠️
lightning/src/chain/onchaintx.rs	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4353 +/- ##
==========================================
+ Coverage 85.98% 85.99% +0.01% 
==========================================
Files 156 156 Lines 102641 102734 +93 Branches 102641 102734 +93 ==========================================
+ Hits 88258 88349 +91 - Misses 11873 11875 +2 
Partials 2510 2510 

Flag	Coverage Δ
tests	85.99% <84.09%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[wpaulino]

Splice transactions should have their own variant, no?

[tnull]

Right, hence the question above regarding which variants we want. So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?

And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?

[TheBlueMatt]

So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?

If we want more splicing context we should include the splice metadata in the splice variant rather than having a separate variant for each splice init logic. This feels like a followup though.

[TheBlueMatt]

To clarify a bit - the differentiation between "splice out" and "splice in" at the top-level API is a bug that we're fixing - there is only a "splice" which can have constituent parts that are in + out. Its a useful differentiation when building splicing instructions but after that it should dissapear.

[wpaulino]

And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?

The raw transaction is already available since we're broadcasting it so they can inspect its inputs and outputs if they want to distinguish between splice in/out.

[tnull]

The raw transaction is already available since we're broadcasting it so they can inspect its inputs and outputs if they want to distinguish between splice in/out.

Will this also work going forward with dual funding, given that then we might reuse the same broadcast_interactive_funding path? Or would we get wrong classifications then, if we don't track the 'intent' from the beginning?

[TheBlueMatt]

Hmm, but for the user it still makes a fundamental difference, no? So in the actual API we still want to discern in/out, right?

Sure at the constructor level we have utilities that allow you to only do one, but the right API definitely isn't a tri-state where the third state is "both". Rather, where we want to expose it it should be a "does this splice have inputs from me" and "does this splice have outputs I added" methods.

[wpaulino]

Will this also work going forward with dual funding, given that then we might reuse the same broadcast_interactive_funding path? Or would we get wrong classifications then, if we don't track the 'intent' from the beginning?

Dual-funding should keep the Funding type and we can just return that along with the transaction since the ChannelManager doesn't have enough context to determine the correct one.

[tnull]

Discussed offline: for now, just add a Splice variant. While we want to add more context (in/out etc) to the Splice variant soon, for now we just need to return and hand through the TransactionType together with the Transaction from its origin.

[tnull]

Now updated accordingly.

[tnull]

FWIW, I also now pushed a fixup renaming to TransactionType which seemed like a slightly better naming, let me know if you object.

[wpaulino]

This needs a rebase now, feel free to squash. There's one pending comment from Matt left that still needs to be addressed.

[tnull]

This needs a rebase now, feel free to squash. There's one pending comment from Matt left that still needs to be addressed.

Rebased and also added a commit that adds some test coverage for the TransactionType::Splice case.

[TheBlueMatt]

few comments on the docs, otherwise feel free to squash.

[tnull]

few comments on the docs, otherwise feel free to squash.

Squashed with the following changes:

diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs
index cbac84aaf..90f4aa13e 100644
--- a/lightning/src/chain/chaininterface.rs+++ b/lightning/src/chain/chaininterface.rs@@ -41,5 +41,6 @@ pub enum TransactionType {
/// A transaction cooperatively closing a channel.
///
-	/// A transaction of this type will be broadcast when cooperatively closing a channel via [`ChannelManager::close_channel`].+	/// A transaction of this type will be broadcast when cooperatively closing a channel via+	/// [`ChannelManager::close_channel`] or if the counterparty closes the channel.
///
/// [`ChannelManager::close_channel`]: crate::ln::channelmanager::ChannelManager::close_channel
@@ -50,5 +51,7 @@ pub enum TransactionType {
/// A transaction being broadcast to force-close the channel.
///
-	/// A transaction of this type will be broadcast when unilaterally closing a channel via [`ChannelManager::force_close_broadcasting_latest_txn`].+	/// A transaction of this type will be broadcast when unilaterally closing a channel via+	/// [`ChannelManager::force_close_broadcasting_latest_txn`] or if the counterparty force-closes+	/// the channel..
///
/// [`ChannelManager::force_close_broadcasting_latest_txn`]: crate::ln::channelmanager::ChannelManager::force_close_broadcasting_latest_txn
@@ -67,5 +70,15 @@ pub enum TransactionType {
channel_id: ChannelId,
},
-	/// A transaction claiming outputs from a commitment transaction (HTLC claims, penalty/justice).+	/// A transaction which is resolving an output spendable by both us and our counterparty.+	///+	/// When a channel closes via the unilateral close path, there may be transaction outputs which+	/// are spendable by either our counterparty or us and represent some lightning state. In order+	/// to resolve that state, the [`ChannelMonitor`] will spend any such outputs, ensuring funds+	/// are only available to us prior to generating an [`Event::SpendableOutputs`]. This+	/// transaction is one such transaction - resolving in-flight HTLCs or punishing our+	/// counterparty if they broadcasted an outdated state.+	///+	/// [`ChannelMonitor`]: crate::chain::ChannelMonitor+	/// [`Event::SpendableOutputs`]: crate::events::Event::SpendableOutputs
Claim {
/// The ID of the channel from which outputs are being claimed.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[TheBlueMatt]

Gonna just land this so the bulk of the code is out of the way, but we really need counterparty node ids if we're gonna use channel ids.

[TheBlueMatt]

oops, note that because we're still not living in a funding-v2-required world ChannelIds aren't the right unique indicator of a specific channel. We need the counterparty node id as well. We should also (obviously) provide the user_channel_id here.

[tnull]

Hmm, good point regarding the counterparty_node_id, though it seems we then first need it to add to a few other places. For example, we don't track it in the SpendableOutputs event (only channel_id) and therefore we also don't track it in OutputSweeper.

[tnull]

Done in #4393

[tnull]

Fixes#3566.

Add a `BroadcastType` enum to provide context about the type of
transaction being broadcast. This information can be useful for
logging, filtering, or prioritization purposes.
The `BroadcastType` variants are:
- `Funding`: A funding transaction establishing a new channel
- `CooperativeClose`: A cooperative close transaction
- `UnilateralClose`: A transaction for force-close
- `AnchorBump`: An anchor transaction for CPFP fee-bumping
- `Claim`: A transaction claiming outputs from commitment tx
- `Sweep`: A transaction sweeping spendable outputs to wallet
Co-Authored-By: HAL 9000

and:

We add the `ChannelId` as context to the just-added `BroadcastType`
enum.
Co-Authored-By: HAL 9000

[ldk-reviews-bot]

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

[TheBlueMatt]

docs need a lot of love

[TheBlueMatt]

I mean all transactions are mutually agreed upon by both parties? Maybe describe what a coop close tx means

[tnull]

Hmm, changed it up a bit, but let me know what exactly is missing for you.

[codecov]

Codecov Report

❌ Patch coverage is 84.09091% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.99%. Comparing base (8679d8d) to head (0715f4a).
⚠️ Report is 39 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/util/sweep.rs	79.59%	6 Missing and 4 partials ⚠️
lightning/src/ln/channelmanager.rs	72.72%	8 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	46.15%	6 Missing and 1 partial ⚠️
lightning-liquidity/src/lsps2/service.rs	85.71%	0 Missing and 1 partial ⚠️
lightning/src/chain/onchaintx.rs	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4353 +/- ##
==========================================
+ Coverage 85.98% 85.99% +0.01% 
==========================================
Files 156 156 Lines 102641 102734 +93 Branches 102641 102734 +93 ==========================================
+ Hits 88258 88349 +91 - Misses 11873 11875 +2 
Partials 2510 2510 

Flag	Coverage Δ
tests	85.99% <84.09%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[wpaulino]

Splice transactions should have their own variant, no?

[tnull]

Right, hence the question above regarding which variants we want. So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?

And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?

[TheBlueMatt]

So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?

If we want more splicing context we should include the splice metadata in the splice variant rather than having a separate variant for each splice init logic. This feels like a followup though.

[TheBlueMatt]

To clarify a bit - the differentiation between "splice out" and "splice in" at the top-level API is a bug that we're fixing - there is only a "splice" which can have constituent parts that are in + out. Its a useful differentiation when building splicing instructions but after that it should dissapear.

[wpaulino]

And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?

The raw transaction is already available since we're broadcasting it so they can inspect its inputs and outputs if they want to distinguish between splice in/out.

[tnull]

The raw transaction is already available since we're broadcasting it so they can inspect its inputs and outputs if they want to distinguish between splice in/out.

Will this also work going forward with dual funding, given that then we might reuse the same broadcast_interactive_funding path? Or would we get wrong classifications then, if we don't track the 'intent' from the beginning?

[TheBlueMatt]

Hmm, but for the user it still makes a fundamental difference, no? So in the actual API we still want to discern in/out, right?

Sure at the constructor level we have utilities that allow you to only do one, but the right API definitely isn't a tri-state where the third state is "both". Rather, where we want to expose it it should be a "does this splice have inputs from me" and "does this splice have outputs I added" methods.

[wpaulino]

Will this also work going forward with dual funding, given that then we might reuse the same broadcast_interactive_funding path? Or would we get wrong classifications then, if we don't track the 'intent' from the beginning?

Dual-funding should keep the Funding type and we can just return that along with the transaction since the ChannelManager doesn't have enough context to determine the correct one.

[tnull]

Discussed offline: for now, just add a Splice variant. While we want to add more context (in/out etc) to the Splice variant soon, for now we just need to return and hand through the TransactionType together with the Transaction from its origin.

[tnull]

Now updated accordingly.

[tnull]

FWIW, I also now pushed a fixup renaming to TransactionType which seemed like a slightly better naming, let me know if you object.

[wpaulino]

This needs a rebase now, feel free to squash. There's one pending comment from Matt left that still needs to be addressed.

[tnull]

This needs a rebase now, feel free to squash. There's one pending comment from Matt left that still needs to be addressed.

Rebased and also added a commit that adds some test coverage for the TransactionType::Splice case.

[TheBlueMatt]

few comments on the docs, otherwise feel free to squash.

[tnull]

few comments on the docs, otherwise feel free to squash.

Squashed with the following changes:

diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs
index cbac84aaf..90f4aa13e 100644
--- a/lightning/src/chain/chaininterface.rs+++ b/lightning/src/chain/chaininterface.rs@@ -41,5 +41,6 @@ pub enum TransactionType {
/// A transaction cooperatively closing a channel.
///
-	/// A transaction of this type will be broadcast when cooperatively closing a channel via [`ChannelManager::close_channel`].+	/// A transaction of this type will be broadcast when cooperatively closing a channel via+	/// [`ChannelManager::close_channel`] or if the counterparty closes the channel.
///
/// [`ChannelManager::close_channel`]: crate::ln::channelmanager::ChannelManager::close_channel
@@ -50,5 +51,7 @@ pub enum TransactionType {
/// A transaction being broadcast to force-close the channel.
///
-	/// A transaction of this type will be broadcast when unilaterally closing a channel via [`ChannelManager::force_close_broadcasting_latest_txn`].+	/// A transaction of this type will be broadcast when unilaterally closing a channel via+	/// [`ChannelManager::force_close_broadcasting_latest_txn`] or if the counterparty force-closes+	/// the channel..
///
/// [`ChannelManager::force_close_broadcasting_latest_txn`]: crate::ln::channelmanager::ChannelManager::force_close_broadcasting_latest_txn
@@ -67,5 +70,15 @@ pub enum TransactionType {
channel_id: ChannelId,
},
-	/// A transaction claiming outputs from a commitment transaction (HTLC claims, penalty/justice).+	/// A transaction which is resolving an output spendable by both us and our counterparty.+	///+	/// When a channel closes via the unilateral close path, there may be transaction outputs which+	/// are spendable by either our counterparty or us and represent some lightning state. In order+	/// to resolve that state, the [`ChannelMonitor`] will spend any such outputs, ensuring funds+	/// are only available to us prior to generating an [`Event::SpendableOutputs`]. This+	/// transaction is one such transaction - resolving in-flight HTLCs or punishing our+	/// counterparty if they broadcasted an outdated state.+	///+	/// [`ChannelMonitor`]: crate::chain::ChannelMonitor+	/// [`Event::SpendableOutputs`]: crate::events::Event::SpendableOutputs
Claim {
/// The ID of the channel from which outputs are being claimed.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[TheBlueMatt]

Gonna just land this so the bulk of the code is out of the way, but we really need counterparty node ids if we're gonna use channel ids.

[TheBlueMatt]

oops, note that because we're still not living in a funding-v2-required world ChannelIds aren't the right unique indicator of a specific channel. We need the counterparty node id as well. We should also (obviously) provide the user_channel_id here.

[tnull]

Hmm, good point regarding the counterparty_node_id, though it seems we then first need it to add to a few other places. For example, we don't track it in the SpendableOutputs event (only channel_id) and therefore we also don't track it in OutputSweeper.

[tnull]

Done in #4393

[tnull]

Fixes#3566.

Add a `BroadcastType` enum to provide context about the type of
transaction being broadcast. This information can be useful for
logging, filtering, or prioritization purposes.
The `BroadcastType` variants are:
- `Funding`: A funding transaction establishing a new channel
- `CooperativeClose`: A cooperative close transaction
- `UnilateralClose`: A transaction for force-close
- `AnchorBump`: An anchor transaction for CPFP fee-bumping
- `Claim`: A transaction claiming outputs from commitment tx
- `Sweep`: A transaction sweeping spendable outputs to wallet
Co-Authored-By: HAL 9000

and:

We add the `ChannelId` as context to the just-added `BroadcastType`
enum.
Co-Authored-By: HAL 9000

[ldk-reviews-bot]

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

[TheBlueMatt]

docs need a lot of love

[TheBlueMatt]

I mean all transactions are mutually agreed upon by both parties? Maybe describe what a coop close tx means

[tnull]

Hmm, changed it up a bit, but let me know what exactly is missing for you.

[codecov]

Codecov Report

❌ Patch coverage is 84.09091% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.99%. Comparing base (8679d8d) to head (0715f4a).
⚠️ Report is 39 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/util/sweep.rs	79.59%	6 Missing and 4 partials ⚠️
lightning/src/ln/channelmanager.rs	72.72%	8 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	46.15%	6 Missing and 1 partial ⚠️
lightning-liquidity/src/lsps2/service.rs	85.71%	0 Missing and 1 partial ⚠️
lightning/src/chain/onchaintx.rs	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4353 +/- ##
==========================================
+ Coverage 85.98% 85.99% +0.01% 
==========================================
Files 156 156 Lines 102641 102734 +93 Branches 102641 102734 +93 ==========================================
+ Hits 88258 88349 +91 - Misses 11873 11875 +2 
Partials 2510 2510 

Flag	Coverage Δ
tests	85.99% <84.09%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[wpaulino]

Splice transactions should have their own variant, no?

[tnull]

Right, hence the question above regarding which variants we want. So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?

And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?

[TheBlueMatt]

So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?

If we want more splicing context we should include the splice metadata in the splice variant rather than having a separate variant for each splice init logic. This feels like a followup though.

[TheBlueMatt]

To clarify a bit - the differentiation between "splice out" and "splice in" at the top-level API is a bug that we're fixing - there is only a "splice" which can have constituent parts that are in + out. Its a useful differentiation when building splicing instructions but after that it should dissapear.

[wpaulino]

And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?

The raw transaction is already available since we're broadcasting it so they can inspect its inputs and outputs if they want to distinguish between splice in/out.

[tnull]

The raw transaction is already available since we're broadcasting it so they can inspect its inputs and outputs if they want to distinguish between splice in/out.

Will this also work going forward with dual funding, given that then we might reuse the same broadcast_interactive_funding path? Or would we get wrong classifications then, if we don't track the 'intent' from the beginning?

[TheBlueMatt]

Hmm, but for the user it still makes a fundamental difference, no? So in the actual API we still want to discern in/out, right?

Sure at the constructor level we have utilities that allow you to only do one, but the right API definitely isn't a tri-state where the third state is "both". Rather, where we want to expose it it should be a "does this splice have inputs from me" and "does this splice have outputs I added" methods.

[wpaulino]

Will this also work going forward with dual funding, given that then we might reuse the same broadcast_interactive_funding path? Or would we get wrong classifications then, if we don't track the 'intent' from the beginning?

Dual-funding should keep the Funding type and we can just return that along with the transaction since the ChannelManager doesn't have enough context to determine the correct one.

[tnull]

Discussed offline: for now, just add a Splice variant. While we want to add more context (in/out etc) to the Splice variant soon, for now we just need to return and hand through the TransactionType together with the Transaction from its origin.

[tnull]

Now updated accordingly.

[tnull]

FWIW, I also now pushed a fixup renaming to TransactionType which seemed like a slightly better naming, let me know if you object.

[wpaulino]

This needs a rebase now, feel free to squash. There's one pending comment from Matt left that still needs to be addressed.

[tnull]

This needs a rebase now, feel free to squash. There's one pending comment from Matt left that still needs to be addressed.

Rebased and also added a commit that adds some test coverage for the TransactionType::Splice case.

[TheBlueMatt]

few comments on the docs, otherwise feel free to squash.

[tnull]

few comments on the docs, otherwise feel free to squash.

Squashed with the following changes:

diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs
index cbac84aaf..90f4aa13e 100644
--- a/lightning/src/chain/chaininterface.rs+++ b/lightning/src/chain/chaininterface.rs@@ -41,5 +41,6 @@ pub enum TransactionType {
/// A transaction cooperatively closing a channel.
///
-	/// A transaction of this type will be broadcast when cooperatively closing a channel via [`ChannelManager::close_channel`].+	/// A transaction of this type will be broadcast when cooperatively closing a channel via+	/// [`ChannelManager::close_channel`] or if the counterparty closes the channel.
///
/// [`ChannelManager::close_channel`]: crate::ln::channelmanager::ChannelManager::close_channel
@@ -50,5 +51,7 @@ pub enum TransactionType {
/// A transaction being broadcast to force-close the channel.
///
-	/// A transaction of this type will be broadcast when unilaterally closing a channel via [`ChannelManager::force_close_broadcasting_latest_txn`].+	/// A transaction of this type will be broadcast when unilaterally closing a channel via+	/// [`ChannelManager::force_close_broadcasting_latest_txn`] or if the counterparty force-closes+	/// the channel..
///
/// [`ChannelManager::force_close_broadcasting_latest_txn`]: crate::ln::channelmanager::ChannelManager::force_close_broadcasting_latest_txn
@@ -67,5 +70,15 @@ pub enum TransactionType {
channel_id: ChannelId,
},
-	/// A transaction claiming outputs from a commitment transaction (HTLC claims, penalty/justice).+	/// A transaction which is resolving an output spendable by both us and our counterparty.+	///+	/// When a channel closes via the unilateral close path, there may be transaction outputs which+	/// are spendable by either our counterparty or us and represent some lightning state. In order+	/// to resolve that state, the [`ChannelMonitor`] will spend any such outputs, ensuring funds+	/// are only available to us prior to generating an [`Event::SpendableOutputs`]. This+	/// transaction is one such transaction - resolving in-flight HTLCs or punishing our+	/// counterparty if they broadcasted an outdated state.+	///+	/// [`ChannelMonitor`]: crate::chain::ChannelMonitor+	/// [`Event::SpendableOutputs`]: crate::events::Event::SpendableOutputs
Claim {
/// The ID of the channel from which outputs are being claimed.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[TheBlueMatt]

Gonna just land this so the bulk of the code is out of the way, but we really need counterparty node ids if we're gonna use channel ids.

[TheBlueMatt]

oops, note that because we're still not living in a funding-v2-required world ChannelIds aren't the right unique indicator of a specific channel. We need the counterparty node id as well. We should also (obviously) provide the user_channel_id here.

[tnull]

Hmm, good point regarding the counterparty_node_id, though it seems we then first need it to add to a few other places. For example, we don't track it in the SpendableOutputs event (only channel_id) and therefore we also don't track it in OutputSweeper.

[tnull]

Done in #4393