Refactor interactive-tx construction and uses by jkczyz · Pull Request #4123 · lightningdevkit/rust-lightning

jkczyz · 2025-09-25T00:44:41Z

Interactive transaction construction currently fails with an AbortReason. However, in order to produce SpliceFailed events in #4077, the contributed inputs and outputs need to be accessible. That is, they cannot be consumed if returning an error. This PR refactors interactive-tx construction code and uses to avoid consuming InteractiveTxConstruction until all error checking has completed.

An upcoming commit will include the contributed inputs and outputs in an error whenever ConstructedTransaction::new fails. In order to DRY up that logic, this commit updates the constructor to create the resulting object prior to performing any checks. This way a conversion method can be added that extracts the necessary input and output data.

Both NegotiationContext::validate_tx and ConstructedTransaction::new contain validity checks. Move the former into the latter in order to consolidate the checks in a single place. This will also allow for reusing error construction in an upcoming commit.

ldk-reviews-bot · 2025-09-25T00:44:44Z

👋 Thanks for assigning @wpaulino 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-09-25T02:31:36Z

Codecov Report

❌ Patch coverage is 61.01695% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.59%. Comparing base (dea125a) to head (0956efd).
⚠️ Report is 64 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/interactivetxs.rs	65.43%	55 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	51.35%	34 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4123 +/- ##
==========================================
+ Coverage 88.50% 88.59% +0.08% 
==========================================
Files 179 180 +1 Lines 134270 134977 +707 Branches 134270 134977 +707 ==========================================
+ Hits 118837 119584 +747 + Misses 12682 12631 -51 - Partials 2751 2762 +11

Flag	Coverage Δ
fuzzing	`21.73% <0.00%> (-0.06%)`	⬇️
tests	`88.43% <61.01%> (+0.09%)`	⬆️

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 · 2025-09-25T16:11:09Z

- None => Err(AbortReason::InternalError(
- "Received unexpected interactive transaction negotiation message",
- )),
+ None => Err(NegotiationError {


Relevant for the follow-up PR: we should be careful to not emit a SpliceFailed event here. We probably only should when pending_splice.is_some().

ldk-reviews-bot · 2025-09-25T16:15:33Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

Instead of popping each input and output to contribute during an interactive tx session, clone the necessary parts and keep around the original inputs and outputs. This will let us reuse them later when constructing an error. The tradeoff is using additional memory to avoid more code complexity required to extract the sent input and outputs from NegotiationContext.

Currently, only the shared input index is stored in ConstructedTransaction. This will be used later to filter out the shared input when constructing an error during interactive tx negotiation. Store the shared output index as well so that the shared output can be filtered out as well.

The number of inputs allowed during an interactive-tx construction session is limited to 4096, so a u16 can be used instead of u32 when serializing ConstructedTransaction.

InteractiveTxConstructor contains the users contributed inputs. When an interactive tx sessions is aborted, the user will need to be notified with an event indicating which inputs and outputs were contributed. This allows them to re-use inputs that are no longer in use. This commit ensures the InteractiveTxConstructor is only consumed after all error checking. That way, in the case of a failure, we're able to produce an event from its input data.

jkczyz · 2025-09-25T00:44:41Z

Interactive transaction construction currently fails with an AbortReason. However, in order to produce SpliceFailed events in #4077, the contributed inputs and outputs need to be accessible. That is, they cannot be consumed if returning an error. This PR refactors interactive-tx construction code and uses to avoid consuming InteractiveTxConstruction until all error checking has completed.

An upcoming commit will include the contributed inputs and outputs in an error whenever ConstructedTransaction::new fails. In order to DRY up that logic, this commit updates the constructor to create the resulting object prior to performing any checks. This way a conversion method can be added that extracts the necessary input and output data.

Both NegotiationContext::validate_tx and ConstructedTransaction::new contain validity checks. Move the former into the latter in order to consolidate the checks in a single place. This will also allow for reusing error construction in an upcoming commit.

ldk-reviews-bot · 2025-09-25T00:44:44Z

👋 Thanks for assigning @wpaulino 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-09-25T02:31:36Z

Codecov Report

❌ Patch coverage is 61.01695% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.59%. Comparing base (dea125a) to head (0956efd).
⚠️ Report is 64 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/interactivetxs.rs	65.43%	55 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	51.35%	34 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4123 +/- ##
==========================================
+ Coverage 88.50% 88.59% +0.08% 
==========================================
Files 179 180 +1 Lines 134270 134977 +707 Branches 134270 134977 +707 ==========================================
+ Hits 118837 119584 +747 + Misses 12682 12631 -51 - Partials 2751 2762 +11

Flag	Coverage Δ
fuzzing	`21.73% <0.00%> (-0.06%)`	⬇️
tests	`88.43% <61.01%> (+0.09%)`	⬆️

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 · 2025-09-25T16:11:09Z

- None => Err(AbortReason::InternalError(
- "Received unexpected interactive transaction negotiation message",
- )),
+ None => Err(NegotiationError {


Relevant for the follow-up PR: we should be careful to not emit a SpliceFailed event here. We probably only should when pending_splice.is_some().

ldk-reviews-bot · 2025-09-25T16:15:33Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

Instead of popping each input and output to contribute during an interactive tx session, clone the necessary parts and keep around the original inputs and outputs. This will let us reuse them later when constructing an error. The tradeoff is using additional memory to avoid more code complexity required to extract the sent input and outputs from NegotiationContext.

Currently, only the shared input index is stored in ConstructedTransaction. This will be used later to filter out the shared input when constructing an error during interactive tx negotiation. Store the shared output index as well so that the shared output can be filtered out as well.

The number of inputs allowed during an interactive-tx construction session is limited to 4096, so a u16 can be used instead of u32 when serializing ConstructedTransaction.

InteractiveTxConstructor contains the users contributed inputs. When an interactive tx sessions is aborted, the user will need to be notified with an event indicating which inputs and outputs were contributed. This allows them to re-use inputs that are no longer in use. This commit ensures the InteractiveTxConstructor is only consumed after all error checking. That way, in the case of a failure, we're able to produce an event from its input data.

jkczyz · 2025-09-25T00:44:41Z

Interactive transaction construction currently fails with an AbortReason. However, in order to produce SpliceFailed events in #4077, the contributed inputs and outputs need to be accessible. That is, they cannot be consumed if returning an error. This PR refactors interactive-tx construction code and uses to avoid consuming InteractiveTxConstruction until all error checking has completed.

An upcoming commit will include the contributed inputs and outputs in an error whenever ConstructedTransaction::new fails. In order to DRY up that logic, this commit updates the constructor to create the resulting object prior to performing any checks. This way a conversion method can be added that extracts the necessary input and output data.

Both NegotiationContext::validate_tx and ConstructedTransaction::new contain validity checks. Move the former into the latter in order to consolidate the checks in a single place. This will also allow for reusing error construction in an upcoming commit.

ldk-reviews-bot · 2025-09-25T00:44:44Z

👋 Thanks for assigning @wpaulino 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-09-25T02:31:36Z

Codecov Report

❌ Patch coverage is 61.01695% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.59%. Comparing base (dea125a) to head (0956efd).
⚠️ Report is 64 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/interactivetxs.rs	65.43%	55 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	51.35%	34 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4123 +/- ##
==========================================
+ Coverage 88.50% 88.59% +0.08% 
==========================================
Files 179 180 +1 Lines 134270 134977 +707 Branches 134270 134977 +707 ==========================================
+ Hits 118837 119584 +747 + Misses 12682 12631 -51 - Partials 2751 2762 +11

Flag	Coverage Δ
fuzzing	`21.73% <0.00%> (-0.06%)`	⬇️
tests	`88.43% <61.01%> (+0.09%)`	⬆️

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 · 2025-09-25T16:11:09Z

- None => Err(AbortReason::InternalError(
- "Received unexpected interactive transaction negotiation message",
- )),
+ None => Err(NegotiationError {


Relevant for the follow-up PR: we should be careful to not emit a SpliceFailed event here. We probably only should when pending_splice.is_some().

ldk-reviews-bot · 2025-09-25T16:15:33Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

Instead of popping each input and output to contribute during an interactive tx session, clone the necessary parts and keep around the original inputs and outputs. This will let us reuse them later when constructing an error. The tradeoff is using additional memory to avoid more code complexity required to extract the sent input and outputs from NegotiationContext.

Currently, only the shared input index is stored in ConstructedTransaction. This will be used later to filter out the shared input when constructing an error during interactive tx negotiation. Store the shared output index as well so that the shared output can be filtered out as well.

The number of inputs allowed during an interactive-tx construction session is limited to 4096, so a u16 can be used instead of u32 when serializing ConstructedTransaction.

InteractiveTxConstructor contains the users contributed inputs. When an interactive tx sessions is aborted, the user will need to be notified with an event indicating which inputs and outputs were contributed. This allows them to re-use inputs that are no longer in use. This commit ensures the InteractiveTxConstructor is only consumed after all error checking. That way, in the case of a failure, we're able to produce an event from its input data.

jkczyz · 2025-09-25T00:44:41Z

Interactive transaction construction currently fails with an AbortReason. However, in order to produce SpliceFailed events in #4077, the contributed inputs and outputs need to be accessible. That is, they cannot be consumed if returning an error. This PR refactors interactive-tx construction code and uses to avoid consuming InteractiveTxConstruction until all error checking has completed.

An upcoming commit will include the contributed inputs and outputs in an error whenever ConstructedTransaction::new fails. In order to DRY up that logic, this commit updates the constructor to create the resulting object prior to performing any checks. This way a conversion method can be added that extracts the necessary input and output data.

Both NegotiationContext::validate_tx and ConstructedTransaction::new contain validity checks. Move the former into the latter in order to consolidate the checks in a single place. This will also allow for reusing error construction in an upcoming commit.

ldk-reviews-bot · 2025-09-25T00:44:44Z

👋 Thanks for assigning @wpaulino 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-09-25T02:31:36Z

Codecov Report

❌ Patch coverage is 61.01695% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.59%. Comparing base (dea125a) to head (0956efd).
⚠️ Report is 64 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/interactivetxs.rs	65.43%	55 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	51.35%	34 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4123 +/- ##
==========================================
+ Coverage 88.50% 88.59% +0.08% 
==========================================
Files 179 180 +1 Lines 134270 134977 +707 Branches 134270 134977 +707 ==========================================
+ Hits 118837 119584 +747 + Misses 12682 12631 -51 - Partials 2751 2762 +11

Flag	Coverage Δ
fuzzing	`21.73% <0.00%> (-0.06%)`	⬇️
tests	`88.43% <61.01%> (+0.09%)`	⬆️

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 · 2025-09-25T16:11:09Z

- None => Err(AbortReason::InternalError(
- "Received unexpected interactive transaction negotiation message",
- )),
+ None => Err(NegotiationError {


Relevant for the follow-up PR: we should be careful to not emit a SpliceFailed event here. We probably only should when pending_splice.is_some().

ldk-reviews-bot · 2025-09-25T16:15:33Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

Instead of popping each input and output to contribute during an interactive tx session, clone the necessary parts and keep around the original inputs and outputs. This will let us reuse them later when constructing an error. The tradeoff is using additional memory to avoid more code complexity required to extract the sent input and outputs from NegotiationContext.

Currently, only the shared input index is stored in ConstructedTransaction. This will be used later to filter out the shared input when constructing an error during interactive tx negotiation. Store the shared output index as well so that the shared output can be filtered out as well.

The number of inputs allowed during an interactive-tx construction session is limited to 4096, so a u16 can be used instead of u32 when serializing ConstructedTransaction.

InteractiveTxConstructor contains the users contributed inputs. When an interactive tx sessions is aborted, the user will need to be notified with an event indicating which inputs and outputs were contributed. This allows them to re-use inputs that are no longer in use. This commit ensures the InteractiveTxConstructor is only consumed after all error checking. That way, in the case of a failure, we're able to produce an event from its input data.

jkczyz · 2025-09-25T00:44:41Z

Interactive transaction construction currently fails with an AbortReason. However, in order to produce SpliceFailed events in #4077, the contributed inputs and outputs need to be accessible. That is, they cannot be consumed if returning an error. This PR refactors interactive-tx construction code and uses to avoid consuming InteractiveTxConstruction until all error checking has completed.

An upcoming commit will include the contributed inputs and outputs in an error whenever ConstructedTransaction::new fails. In order to DRY up that logic, this commit updates the constructor to create the resulting object prior to performing any checks. This way a conversion method can be added that extracts the necessary input and output data.

Both NegotiationContext::validate_tx and ConstructedTransaction::new contain validity checks. Move the former into the latter in order to consolidate the checks in a single place. This will also allow for reusing error construction in an upcoming commit.

ldk-reviews-bot · 2025-09-25T00:44:44Z

👋 Thanks for assigning @wpaulino 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-09-25T02:31:36Z

Codecov Report

❌ Patch coverage is 61.01695% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.59%. Comparing base (dea125a) to head (0956efd).
⚠️ Report is 64 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/interactivetxs.rs	65.43%	55 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	51.35%	34 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4123 +/- ##
==========================================
+ Coverage 88.50% 88.59% +0.08% 
==========================================
Files 179 180 +1 Lines 134270 134977 +707 Branches 134270 134977 +707 ==========================================
+ Hits 118837 119584 +747 + Misses 12682 12631 -51 - Partials 2751 2762 +11

Flag	Coverage Δ
fuzzing	`21.73% <0.00%> (-0.06%)`	⬇️
tests	`88.43% <61.01%> (+0.09%)`	⬆️

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 · 2025-09-25T16:11:09Z

- None => Err(AbortReason::InternalError(
- "Received unexpected interactive transaction negotiation message",
- )),
+ None => Err(NegotiationError {


Relevant for the follow-up PR: we should be careful to not emit a SpliceFailed event here. We probably only should when pending_splice.is_some().

ldk-reviews-bot · 2025-09-25T16:15:33Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

Instead of popping each input and output to contribute during an interactive tx session, clone the necessary parts and keep around the original inputs and outputs. This will let us reuse them later when constructing an error. The tradeoff is using additional memory to avoid more code complexity required to extract the sent input and outputs from NegotiationContext.

Currently, only the shared input index is stored in ConstructedTransaction. This will be used later to filter out the shared input when constructing an error during interactive tx negotiation. Store the shared output index as well so that the shared output can be filtered out as well.

The number of inputs allowed during an interactive-tx construction session is limited to 4096, so a u16 can be used instead of u32 when serializing ConstructedTransaction.

InteractiveTxConstructor contains the users contributed inputs. When an interactive tx sessions is aborted, the user will need to be notified with an event indicating which inputs and outputs were contributed. This allows them to re-use inputs that are no longer in use. This commit ensures the InteractiveTxConstructor is only consumed after all error checking. That way, in the case of a failure, we're able to produce an event from its input data.

jkczyz · 2025-09-25T00:44:41Z

Interactive transaction construction currently fails with an AbortReason. However, in order to produce SpliceFailed events in #4077, the contributed inputs and outputs need to be accessible. That is, they cannot be consumed if returning an error. This PR refactors interactive-tx construction code and uses to avoid consuming InteractiveTxConstruction until all error checking has completed.

An upcoming commit will include the contributed inputs and outputs in an error whenever ConstructedTransaction::new fails. In order to DRY up that logic, this commit updates the constructor to create the resulting object prior to performing any checks. This way a conversion method can be added that extracts the necessary input and output data.

Both NegotiationContext::validate_tx and ConstructedTransaction::new contain validity checks. Move the former into the latter in order to consolidate the checks in a single place. This will also allow for reusing error construction in an upcoming commit.

ldk-reviews-bot · 2025-09-25T00:44:44Z

👋 Thanks for assigning @wpaulino 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-09-25T02:31:36Z

Codecov Report

❌ Patch coverage is 61.01695% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.59%. Comparing base (dea125a) to head (0956efd).
⚠️ Report is 64 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/interactivetxs.rs	65.43%	55 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	51.35%	34 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4123 +/- ##
==========================================
+ Coverage 88.50% 88.59% +0.08% 
==========================================
Files 179 180 +1 Lines 134270 134977 +707 Branches 134270 134977 +707 ==========================================
+ Hits 118837 119584 +747 + Misses 12682 12631 -51 - Partials 2751 2762 +11

Flag	Coverage Δ
fuzzing	`21.73% <0.00%> (-0.06%)`	⬇️
tests	`88.43% <61.01%> (+0.09%)`	⬆️

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 · 2025-09-25T16:11:09Z

- None => Err(AbortReason::InternalError(
- "Received unexpected interactive transaction negotiation message",
- )),
+ None => Err(NegotiationError {


Relevant for the follow-up PR: we should be careful to not emit a SpliceFailed event here. We probably only should when pending_splice.is_some().

ldk-reviews-bot · 2025-09-25T16:15:33Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

Instead of popping each input and output to contribute during an interactive tx session, clone the necessary parts and keep around the original inputs and outputs. This will let us reuse them later when constructing an error. The tradeoff is using additional memory to avoid more code complexity required to extract the sent input and outputs from NegotiationContext.

Currently, only the shared input index is stored in ConstructedTransaction. This will be used later to filter out the shared input when constructing an error during interactive tx negotiation. Store the shared output index as well so that the shared output can be filtered out as well.

The number of inputs allowed during an interactive-tx construction session is limited to 4096, so a u16 can be used instead of u32 when serializing ConstructedTransaction.

InteractiveTxConstructor contains the users contributed inputs. When an interactive tx sessions is aborted, the user will need to be notified with an event indicating which inputs and outputs were contributed. This allows them to re-use inputs that are no longer in use. This commit ensures the InteractiveTxConstructor is only consumed after all error checking. That way, in the case of a failure, we're able to produce an event from its input data.

jkczyz · 2025-09-25T00:44:41Z

Interactive transaction construction currently fails with an AbortReason. However, in order to produce SpliceFailed events in #4077, the contributed inputs and outputs need to be accessible. That is, they cannot be consumed if returning an error. This PR refactors interactive-tx construction code and uses to avoid consuming InteractiveTxConstruction until all error checking has completed.

An upcoming commit will include the contributed inputs and outputs in an error whenever ConstructedTransaction::new fails. In order to DRY up that logic, this commit updates the constructor to create the resulting object prior to performing any checks. This way a conversion method can be added that extracts the necessary input and output data.

Both NegotiationContext::validate_tx and ConstructedTransaction::new contain validity checks. Move the former into the latter in order to consolidate the checks in a single place. This will also allow for reusing error construction in an upcoming commit.

ldk-reviews-bot · 2025-09-25T00:44:44Z

👋 Thanks for assigning @wpaulino 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-09-25T02:31:36Z

Codecov Report

❌ Patch coverage is 61.01695% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.59%. Comparing base (dea125a) to head (0956efd).
⚠️ Report is 64 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/interactivetxs.rs	65.43%	55 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	51.35%	34 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4123 +/- ##
==========================================
+ Coverage 88.50% 88.59% +0.08% 
==========================================
Files 179 180 +1 Lines 134270 134977 +707 Branches 134270 134977 +707 ==========================================
+ Hits 118837 119584 +747 + Misses 12682 12631 -51 - Partials 2751 2762 +11

Flag	Coverage Δ
fuzzing	`21.73% <0.00%> (-0.06%)`	⬇️
tests	`88.43% <61.01%> (+0.09%)`	⬆️

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 · 2025-09-25T16:11:09Z

- None => Err(AbortReason::InternalError(
- "Received unexpected interactive transaction negotiation message",
- )),
+ None => Err(NegotiationError {


Relevant for the follow-up PR: we should be careful to not emit a SpliceFailed event here. We probably only should when pending_splice.is_some().

ldk-reviews-bot · 2025-09-25T16:15:33Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

Instead of popping each input and output to contribute during an interactive tx session, clone the necessary parts and keep around the original inputs and outputs. This will let us reuse them later when constructing an error. The tradeoff is using additional memory to avoid more code complexity required to extract the sent input and outputs from NegotiationContext.

Currently, only the shared input index is stored in ConstructedTransaction. This will be used later to filter out the shared input when constructing an error during interactive tx negotiation. Store the shared output index as well so that the shared output can be filtered out as well.

The number of inputs allowed during an interactive-tx construction session is limited to 4096, so a u16 can be used instead of u32 when serializing ConstructedTransaction.

InteractiveTxConstructor contains the users contributed inputs. When an interactive tx sessions is aborted, the user will need to be notified with an event indicating which inputs and outputs were contributed. This allows them to re-use inputs that are no longer in use. This commit ensures the InteractiveTxConstructor is only consumed after all error checking. That way, in the case of a failure, we're able to produce an event from its input data.

jkczyz · 2025-09-25T00:44:41Z

Interactive transaction construction currently fails with an AbortReason. However, in order to produce SpliceFailed events in #4077, the contributed inputs and outputs need to be accessible. That is, they cannot be consumed if returning an error. This PR refactors interactive-tx construction code and uses to avoid consuming InteractiveTxConstruction until all error checking has completed.

An upcoming commit will include the contributed inputs and outputs in an error whenever ConstructedTransaction::new fails. In order to DRY up that logic, this commit updates the constructor to create the resulting object prior to performing any checks. This way a conversion method can be added that extracts the necessary input and output data.

Both NegotiationContext::validate_tx and ConstructedTransaction::new contain validity checks. Move the former into the latter in order to consolidate the checks in a single place. This will also allow for reusing error construction in an upcoming commit.

ldk-reviews-bot · 2025-09-25T00:44:44Z

👋 Thanks for assigning @wpaulino 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-09-25T02:31:36Z

Codecov Report

❌ Patch coverage is 61.01695% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.59%. Comparing base (dea125a) to head (0956efd).
⚠️ Report is 64 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/interactivetxs.rs	65.43%	55 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	51.35%	34 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4123 +/- ##
==========================================
+ Coverage 88.50% 88.59% +0.08% 
==========================================
Files 179 180 +1 Lines 134270 134977 +707 Branches 134270 134977 +707 ==========================================
+ Hits 118837 119584 +747 + Misses 12682 12631 -51 - Partials 2751 2762 +11

Flag	Coverage Δ
fuzzing	`21.73% <0.00%> (-0.06%)`	⬇️
tests	`88.43% <61.01%> (+0.09%)`	⬆️

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 · 2025-09-25T16:11:09Z

- None => Err(AbortReason::InternalError(
- "Received unexpected interactive transaction negotiation message",
- )),
+ None => Err(NegotiationError {


Relevant for the follow-up PR: we should be careful to not emit a SpliceFailed event here. We probably only should when pending_splice.is_some().

ldk-reviews-bot · 2025-09-25T16:15:33Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

Instead of popping each input and output to contribute during an interactive tx session, clone the necessary parts and keep around the original inputs and outputs. This will let us reuse them later when constructing an error. The tradeoff is using additional memory to avoid more code complexity required to extract the sent input and outputs from NegotiationContext.

Currently, only the shared input index is stored in ConstructedTransaction. This will be used later to filter out the shared input when constructing an error during interactive tx negotiation. Store the shared output index as well so that the shared output can be filtered out as well.

The number of inputs allowed during an interactive-tx construction session is limited to 4096, so a u16 can be used instead of u32 when serializing ConstructedTransaction.

InteractiveTxConstructor contains the users contributed inputs. When an interactive tx sessions is aborted, the user will need to be notified with an event indicating which inputs and outputs were contributed. This allows them to re-use inputs that are no longer in use. This commit ensures the InteractiveTxConstructor is only consumed after all error checking. That way, in the case of a failure, we're able to produce an event from its input data.

jkczyz added 2 commits September 24, 2025 19:30

jkczyz requested a review from wpaulino September 25, 2025 00:45

jkczyz self-assigned this Sep 25, 2025

jkczyz mentioned this pull request Sep 25, 2025
Emit SplicePending and SpliceFailed events #4077
Merged

jkczyz added this to the 0.2 milestone Sep 25, 2025

jkczyz force-pushed the 2025-09-negotiation-error branch from a7638af to 59f7debCompare September 25, 2025 02:15

wpaulino previously approved these changes Sep 25, 2025
View reviewed changes

jkczyz added this to Weekly Goals Sep 25, 2025

jkczyz added 3 commits September 29, 2025 16:32

Use u16 for ConstructedTransaction::shared_input_index
0ecbe62
The number of inputs allowed during an interactive-tx construction session is limited to 4096, so a u16 can be used instead of u32 when serializing ConstructedTransaction.

jkczyz dismissed wpaulino’s stale review via 18d3bd7September 30, 2025 13:09

jkczyz force-pushed the 2025-09-negotiation-error branch from 59f7deb to 18d3bd7Compare September 30, 2025 13:09

jkczyz force-pushed the 2025-09-negotiation-error branch from 18d3bd7 to 0956efdCompare September 30, 2025 14:16

jkczyz changed the title ~~Introduce NegotiationError for interactive-tx constructions~~Refactor interactive-tx construction and usesSep 30, 2025

wpaulino approved these changes Oct 1, 2025
View reviewed changes

wpaulino merged commit 81495c6 into lightningdevkit:mainOct 1, 2025
25 checks passed

github-project-automationBot moved this to Done in Weekly Goals Oct 1, 2025