Test channel reestablish during splice lifecycle by wpaulino · Pull Request #4079 · lightningdevkit/rust-lightning

wpaulino · 2025-09-18T00:20:28Z

This also includes a couple small bug fixes found along the way.

ldk-reviews-bot · 2025-09-18T00:20:31Z

👋 Thanks for assigning @jkczyz 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.

ldk-reviews-bot · 2025-09-20T00:20:51Z

🔔 1st Reminder

Hey @jkczyz! 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.

ldk-reviews-bot · 2025-09-22T00:21:02Z

🔔 2nd Reminder

Hey @jkczyz! 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.

ldk-reviews-bot · 2025-09-24T00:21:48Z

🔔 3rd Reminder

Hey @jkczyz! 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.

codecov · 2025-09-24T16:46:01Z

Codecov Report

❌ Patch coverage is 93.96325% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.72%. Comparing base (fec434b) to head (5594f9b).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	73.68%	14 Missing and 1 partial ⚠️
lightning/src/ln/functional_test_utils.rs	93.54%	2 Missing and 2 partials ⚠️
lightning/src/ln/splicing_tests.rs	98.08%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4079 +/- ##
==========================================
+ Coverage 88.63% 88.72% +0.09% 
==========================================
Files 180 180 Lines 135243 135528 +285 Branches 135243 135528 +285 ==========================================
+ Hits 119873 120252 +379 + Misses 12597 12506 -91 + Partials 2773 2770 -3

Flag	Coverage Δ
fuzzing	`21.72% <17.50%> (-0.04%)`	⬇️
tests	`88.57% <93.96%> (+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.

TheBlueMatt · 2025-09-25T13:36:34Z

Needs rebase :(

ldk-reviews-bot · 2025-09-25T16:49:02Z

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

TheBlueMatt · 2025-10-08T12:23:01Z

+ .interactive_tx_signing_session
+ .as_ref()
+ .map(|signing_session| signing_session.unsigned_tx().compute_txid());
+ self.pending_splice.is_some() && their_next_funding_txid == our_next_funding_txid


This is much stricter than the spec update proposed here - the spec update is simple (did the messages contain the fields) but this is checking that indirectly and in a complicated way. I'm not entirely sure why we need to bother with this, in any case, a redundant channel_ready isn't really a big deal and this is a bunch of annoying code.

The spec does note that they must be "for a splice transaction". I found a way to simplify this though, we can just check for existence of pending_splice on reestablish.

jkczyz

Latest push looks good modulo addressing @TheBlueMatt's comments.

We may produce duplicate `FundingTransactionReadyForSigning` events if the user has processed an initial event but has not yet called back with `funding_transaction_signed` and a peer reconnection occurs. If the user also handles the duplicate events, any duplicate calls to `funding_transaction_signed` after an initial successful one would return an error. This doesn't make sense, as the API should remain idempotent, so we return early on any duplicate calls.

We only want to produce `tx_signatures` once we know that the monitor update (either the initial one for a dual-funded channel, or a `RenegotiatedFunding` one for a splice) has been persisted. If we haven't received the counterparty's `commitment_signed` yet, then the monitor update hasn't been created, leading us to pass the `!awaiting_monitor_update` condition and produce a holder `tx_signatures` message.

If nodes have started a splice, this means they have both sent and received `channel_ready` already: in that case, it's unnecessary to retransmit `channel_ready` on reconnection.

Otherwise, we won't ever be able to resume a pending negotiation after a reconnection via `channel_reestablish`. Along the way, we also merge `should_reset_pending_splice_state` into `PendingFunding::can_abandon_state` to simplify the logic around when we're able to reset specific parts of the pending splice state.

We'll use this in the next commit to test the resend logic for `announcement_signatures` when reestablishing a channel that had a pending splice become locked.

This test captures all the new spec requirements introduced for a splice to the channel reestablish flow.

wpaulino · 2025-09-18T00:20:28Z

This also includes a couple small bug fixes found along the way.

ldk-reviews-bot · 2025-09-18T00:20:31Z

👋 Thanks for assigning @jkczyz 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.

ldk-reviews-bot · 2025-09-20T00:20:51Z

🔔 1st Reminder

Hey @jkczyz! 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.

ldk-reviews-bot · 2025-09-22T00:21:02Z

🔔 2nd Reminder

Hey @jkczyz! 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.

ldk-reviews-bot · 2025-09-24T00:21:48Z

🔔 3rd Reminder

Hey @jkczyz! 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.

codecov · 2025-09-24T16:46:01Z

Codecov Report

❌ Patch coverage is 93.96325% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.72%. Comparing base (fec434b) to head (5594f9b).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	73.68%	14 Missing and 1 partial ⚠️
lightning/src/ln/functional_test_utils.rs	93.54%	2 Missing and 2 partials ⚠️
lightning/src/ln/splicing_tests.rs	98.08%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4079 +/- ##
==========================================
+ Coverage 88.63% 88.72% +0.09% 
==========================================
Files 180 180 Lines 135243 135528 +285 Branches 135243 135528 +285 ==========================================
+ Hits 119873 120252 +379 + Misses 12597 12506 -91 + Partials 2773 2770 -3

Flag	Coverage Δ
fuzzing	`21.72% <17.50%> (-0.04%)`	⬇️
tests	`88.57% <93.96%> (+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.

TheBlueMatt · 2025-09-25T13:36:34Z

Needs rebase :(

ldk-reviews-bot · 2025-09-25T16:49:02Z

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

TheBlueMatt · 2025-10-08T12:23:01Z

+ .interactive_tx_signing_session
+ .as_ref()
+ .map(|signing_session| signing_session.unsigned_tx().compute_txid());
+ self.pending_splice.is_some() && their_next_funding_txid == our_next_funding_txid


This is much stricter than the spec update proposed here - the spec update is simple (did the messages contain the fields) but this is checking that indirectly and in a complicated way. I'm not entirely sure why we need to bother with this, in any case, a redundant channel_ready isn't really a big deal and this is a bunch of annoying code.

The spec does note that they must be "for a splice transaction". I found a way to simplify this though, we can just check for existence of pending_splice on reestablish.

jkczyz

Latest push looks good modulo addressing @TheBlueMatt's comments.

We may produce duplicate `FundingTransactionReadyForSigning` events if the user has processed an initial event but has not yet called back with `funding_transaction_signed` and a peer reconnection occurs. If the user also handles the duplicate events, any duplicate calls to `funding_transaction_signed` after an initial successful one would return an error. This doesn't make sense, as the API should remain idempotent, so we return early on any duplicate calls.

We only want to produce `tx_signatures` once we know that the monitor update (either the initial one for a dual-funded channel, or a `RenegotiatedFunding` one for a splice) has been persisted. If we haven't received the counterparty's `commitment_signed` yet, then the monitor update hasn't been created, leading us to pass the `!awaiting_monitor_update` condition and produce a holder `tx_signatures` message.

If nodes have started a splice, this means they have both sent and received `channel_ready` already: in that case, it's unnecessary to retransmit `channel_ready` on reconnection.

Otherwise, we won't ever be able to resume a pending negotiation after a reconnection via `channel_reestablish`. Along the way, we also merge `should_reset_pending_splice_state` into `PendingFunding::can_abandon_state` to simplify the logic around when we're able to reset specific parts of the pending splice state.

We'll use this in the next commit to test the resend logic for `announcement_signatures` when reestablishing a channel that had a pending splice become locked.

This test captures all the new spec requirements introduced for a splice to the channel reestablish flow.

wpaulino · 2025-09-18T00:20:28Z

This also includes a couple small bug fixes found along the way.

ldk-reviews-bot · 2025-09-18T00:20:31Z

👋 Thanks for assigning @jkczyz 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.

ldk-reviews-bot · 2025-09-20T00:20:51Z

🔔 1st Reminder

Hey @jkczyz! 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.

ldk-reviews-bot · 2025-09-22T00:21:02Z

🔔 2nd Reminder

Hey @jkczyz! 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.

ldk-reviews-bot · 2025-09-24T00:21:48Z

🔔 3rd Reminder

Hey @jkczyz! 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.

codecov · 2025-09-24T16:46:01Z

Codecov Report

❌ Patch coverage is 93.96325% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.72%. Comparing base (fec434b) to head (5594f9b).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	73.68%	14 Missing and 1 partial ⚠️
lightning/src/ln/functional_test_utils.rs	93.54%	2 Missing and 2 partials ⚠️
lightning/src/ln/splicing_tests.rs	98.08%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4079 +/- ##
==========================================
+ Coverage 88.63% 88.72% +0.09% 
==========================================
Files 180 180 Lines 135243 135528 +285 Branches 135243 135528 +285 ==========================================
+ Hits 119873 120252 +379 + Misses 12597 12506 -91 + Partials 2773 2770 -3

Flag	Coverage Δ
fuzzing	`21.72% <17.50%> (-0.04%)`	⬇️
tests	`88.57% <93.96%> (+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.

TheBlueMatt · 2025-09-25T13:36:34Z

Needs rebase :(

ldk-reviews-bot · 2025-09-25T16:49:02Z

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

TheBlueMatt · 2025-10-08T12:23:01Z

+ .interactive_tx_signing_session
+ .as_ref()
+ .map(|signing_session| signing_session.unsigned_tx().compute_txid());
+ self.pending_splice.is_some() && their_next_funding_txid == our_next_funding_txid


This is much stricter than the spec update proposed here - the spec update is simple (did the messages contain the fields) but this is checking that indirectly and in a complicated way. I'm not entirely sure why we need to bother with this, in any case, a redundant channel_ready isn't really a big deal and this is a bunch of annoying code.

The spec does note that they must be "for a splice transaction". I found a way to simplify this though, we can just check for existence of pending_splice on reestablish.

jkczyz

Latest push looks good modulo addressing @TheBlueMatt's comments.

We may produce duplicate `FundingTransactionReadyForSigning` events if the user has processed an initial event but has not yet called back with `funding_transaction_signed` and a peer reconnection occurs. If the user also handles the duplicate events, any duplicate calls to `funding_transaction_signed` after an initial successful one would return an error. This doesn't make sense, as the API should remain idempotent, so we return early on any duplicate calls.

We only want to produce `tx_signatures` once we know that the monitor update (either the initial one for a dual-funded channel, or a `RenegotiatedFunding` one for a splice) has been persisted. If we haven't received the counterparty's `commitment_signed` yet, then the monitor update hasn't been created, leading us to pass the `!awaiting_monitor_update` condition and produce a holder `tx_signatures` message.

If nodes have started a splice, this means they have both sent and received `channel_ready` already: in that case, it's unnecessary to retransmit `channel_ready` on reconnection.

Otherwise, we won't ever be able to resume a pending negotiation after a reconnection via `channel_reestablish`. Along the way, we also merge `should_reset_pending_splice_state` into `PendingFunding::can_abandon_state` to simplify the logic around when we're able to reset specific parts of the pending splice state.

We'll use this in the next commit to test the resend logic for `announcement_signatures` when reestablishing a channel that had a pending splice become locked.

This test captures all the new spec requirements introduced for a splice to the channel reestablish flow.

wpaulino · 2025-09-18T00:20:28Z

This also includes a couple small bug fixes found along the way.

ldk-reviews-bot · 2025-09-18T00:20:31Z

👋 Thanks for assigning @jkczyz 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.

ldk-reviews-bot · 2025-09-20T00:20:51Z

🔔 1st Reminder

Hey @jkczyz! 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.

ldk-reviews-bot · 2025-09-22T00:21:02Z

🔔 2nd Reminder

Hey @jkczyz! 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.

ldk-reviews-bot · 2025-09-24T00:21:48Z

🔔 3rd Reminder

Hey @jkczyz! 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.

codecov · 2025-09-24T16:46:01Z

Codecov Report

❌ Patch coverage is 93.96325% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.72%. Comparing base (fec434b) to head (5594f9b).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	73.68%	14 Missing and 1 partial ⚠️
lightning/src/ln/functional_test_utils.rs	93.54%	2 Missing and 2 partials ⚠️
lightning/src/ln/splicing_tests.rs	98.08%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4079 +/- ##
==========================================
+ Coverage 88.63% 88.72% +0.09% 
==========================================
Files 180 180 Lines 135243 135528 +285 Branches 135243 135528 +285 ==========================================
+ Hits 119873 120252 +379 + Misses 12597 12506 -91 + Partials 2773 2770 -3

Flag	Coverage Δ
fuzzing	`21.72% <17.50%> (-0.04%)`	⬇️
tests	`88.57% <93.96%> (+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.

TheBlueMatt · 2025-09-25T13:36:34Z

Needs rebase :(

ldk-reviews-bot · 2025-09-25T16:49:02Z

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

TheBlueMatt · 2025-10-08T12:23:01Z

+ .interactive_tx_signing_session
+ .as_ref()
+ .map(|signing_session| signing_session.unsigned_tx().compute_txid());
+ self.pending_splice.is_some() && their_next_funding_txid == our_next_funding_txid


This is much stricter than the spec update proposed here - the spec update is simple (did the messages contain the fields) but this is checking that indirectly and in a complicated way. I'm not entirely sure why we need to bother with this, in any case, a redundant channel_ready isn't really a big deal and this is a bunch of annoying code.

The spec does note that they must be "for a splice transaction". I found a way to simplify this though, we can just check for existence of pending_splice on reestablish.

jkczyz

Latest push looks good modulo addressing @TheBlueMatt's comments.

We may produce duplicate `FundingTransactionReadyForSigning` events if the user has processed an initial event but has not yet called back with `funding_transaction_signed` and a peer reconnection occurs. If the user also handles the duplicate events, any duplicate calls to `funding_transaction_signed` after an initial successful one would return an error. This doesn't make sense, as the API should remain idempotent, so we return early on any duplicate calls.

We only want to produce `tx_signatures` once we know that the monitor update (either the initial one for a dual-funded channel, or a `RenegotiatedFunding` one for a splice) has been persisted. If we haven't received the counterparty's `commitment_signed` yet, then the monitor update hasn't been created, leading us to pass the `!awaiting_monitor_update` condition and produce a holder `tx_signatures` message.

If nodes have started a splice, this means they have both sent and received `channel_ready` already: in that case, it's unnecessary to retransmit `channel_ready` on reconnection.

Otherwise, we won't ever be able to resume a pending negotiation after a reconnection via `channel_reestablish`. Along the way, we also merge `should_reset_pending_splice_state` into `PendingFunding::can_abandon_state` to simplify the logic around when we're able to reset specific parts of the pending splice state.

We'll use this in the next commit to test the resend logic for `announcement_signatures` when reestablishing a channel that had a pending splice become locked.

This test captures all the new spec requirements introduced for a splice to the channel reestablish flow.

wpaulino · 2025-09-18T00:20:28Z

This also includes a couple small bug fixes found along the way.

ldk-reviews-bot · 2025-09-18T00:20:31Z

👋 Thanks for assigning @jkczyz 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.

ldk-reviews-bot · 2025-09-20T00:20:51Z

🔔 1st Reminder

Hey @jkczyz! 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.

ldk-reviews-bot · 2025-09-22T00:21:02Z

🔔 2nd Reminder

Hey @jkczyz! 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.

ldk-reviews-bot · 2025-09-24T00:21:48Z

🔔 3rd Reminder

Hey @jkczyz! 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.

codecov · 2025-09-24T16:46:01Z

Codecov Report

❌ Patch coverage is 93.96325% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.72%. Comparing base (fec434b) to head (5594f9b).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	73.68%	14 Missing and 1 partial ⚠️
lightning/src/ln/functional_test_utils.rs	93.54%	2 Missing and 2 partials ⚠️
lightning/src/ln/splicing_tests.rs	98.08%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4079 +/- ##
==========================================
+ Coverage 88.63% 88.72% +0.09% 
==========================================
Files 180 180 Lines 135243 135528 +285 Branches 135243 135528 +285 ==========================================
+ Hits 119873 120252 +379 + Misses 12597 12506 -91 + Partials 2773 2770 -3

Flag	Coverage Δ
fuzzing	`21.72% <17.50%> (-0.04%)`	⬇️
tests	`88.57% <93.96%> (+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.

TheBlueMatt · 2025-09-25T13:36:34Z

Needs rebase :(

ldk-reviews-bot · 2025-09-25T16:49:02Z

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

TheBlueMatt · 2025-10-08T12:23:01Z

+ .interactive_tx_signing_session
+ .as_ref()
+ .map(|signing_session| signing_session.unsigned_tx().compute_txid());
+ self.pending_splice.is_some() && their_next_funding_txid == our_next_funding_txid


This is much stricter than the spec update proposed here - the spec update is simple (did the messages contain the fields) but this is checking that indirectly and in a complicated way. I'm not entirely sure why we need to bother with this, in any case, a redundant channel_ready isn't really a big deal and this is a bunch of annoying code.

The spec does note that they must be "for a splice transaction". I found a way to simplify this though, we can just check for existence of pending_splice on reestablish.

jkczyz

Latest push looks good modulo addressing @TheBlueMatt's comments.

We may produce duplicate `FundingTransactionReadyForSigning` events if the user has processed an initial event but has not yet called back with `funding_transaction_signed` and a peer reconnection occurs. If the user also handles the duplicate events, any duplicate calls to `funding_transaction_signed` after an initial successful one would return an error. This doesn't make sense, as the API should remain idempotent, so we return early on any duplicate calls.

We only want to produce `tx_signatures` once we know that the monitor update (either the initial one for a dual-funded channel, or a `RenegotiatedFunding` one for a splice) has been persisted. If we haven't received the counterparty's `commitment_signed` yet, then the monitor update hasn't been created, leading us to pass the `!awaiting_monitor_update` condition and produce a holder `tx_signatures` message.

If nodes have started a splice, this means they have both sent and received `channel_ready` already: in that case, it's unnecessary to retransmit `channel_ready` on reconnection.

Otherwise, we won't ever be able to resume a pending negotiation after a reconnection via `channel_reestablish`. Along the way, we also merge `should_reset_pending_splice_state` into `PendingFunding::can_abandon_state` to simplify the logic around when we're able to reset specific parts of the pending splice state.

We'll use this in the next commit to test the resend logic for `announcement_signatures` when reestablishing a channel that had a pending splice become locked.

This test captures all the new spec requirements introduced for a splice to the channel reestablish flow.

wpaulino · 2025-09-18T00:20:28Z

This also includes a couple small bug fixes found along the way.

ldk-reviews-bot · 2025-09-18T00:20:31Z

👋 Thanks for assigning @jkczyz 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.

ldk-reviews-bot · 2025-09-20T00:20:51Z

🔔 1st Reminder

Hey @jkczyz! 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.

ldk-reviews-bot · 2025-09-22T00:21:02Z

🔔 2nd Reminder

Hey @jkczyz! 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.

ldk-reviews-bot · 2025-09-24T00:21:48Z

🔔 3rd Reminder

Hey @jkczyz! 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.

codecov · 2025-09-24T16:46:01Z

Codecov Report

❌ Patch coverage is 93.96325% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.72%. Comparing base (fec434b) to head (5594f9b).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	73.68%	14 Missing and 1 partial ⚠️
lightning/src/ln/functional_test_utils.rs	93.54%	2 Missing and 2 partials ⚠️
lightning/src/ln/splicing_tests.rs	98.08%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4079 +/- ##
==========================================
+ Coverage 88.63% 88.72% +0.09% 
==========================================
Files 180 180 Lines 135243 135528 +285 Branches 135243 135528 +285 ==========================================
+ Hits 119873 120252 +379 + Misses 12597 12506 -91 + Partials 2773 2770 -3

Flag	Coverage Δ
fuzzing	`21.72% <17.50%> (-0.04%)`	⬇️
tests	`88.57% <93.96%> (+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.

TheBlueMatt · 2025-09-25T13:36:34Z

Needs rebase :(

ldk-reviews-bot · 2025-09-25T16:49:02Z

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

TheBlueMatt · 2025-10-08T12:23:01Z

+ .interactive_tx_signing_session
+ .as_ref()
+ .map(|signing_session| signing_session.unsigned_tx().compute_txid());
+ self.pending_splice.is_some() && their_next_funding_txid == our_next_funding_txid


This is much stricter than the spec update proposed here - the spec update is simple (did the messages contain the fields) but this is checking that indirectly and in a complicated way. I'm not entirely sure why we need to bother with this, in any case, a redundant channel_ready isn't really a big deal and this is a bunch of annoying code.

The spec does note that they must be "for a splice transaction". I found a way to simplify this though, we can just check for existence of pending_splice on reestablish.

jkczyz

Latest push looks good modulo addressing @TheBlueMatt's comments.

We may produce duplicate `FundingTransactionReadyForSigning` events if the user has processed an initial event but has not yet called back with `funding_transaction_signed` and a peer reconnection occurs. If the user also handles the duplicate events, any duplicate calls to `funding_transaction_signed` after an initial successful one would return an error. This doesn't make sense, as the API should remain idempotent, so we return early on any duplicate calls.

We only want to produce `tx_signatures` once we know that the monitor update (either the initial one for a dual-funded channel, or a `RenegotiatedFunding` one for a splice) has been persisted. If we haven't received the counterparty's `commitment_signed` yet, then the monitor update hasn't been created, leading us to pass the `!awaiting_monitor_update` condition and produce a holder `tx_signatures` message.

If nodes have started a splice, this means they have both sent and received `channel_ready` already: in that case, it's unnecessary to retransmit `channel_ready` on reconnection.

Otherwise, we won't ever be able to resume a pending negotiation after a reconnection via `channel_reestablish`. Along the way, we also merge `should_reset_pending_splice_state` into `PendingFunding::can_abandon_state` to simplify the logic around when we're able to reset specific parts of the pending splice state.

We'll use this in the next commit to test the resend logic for `announcement_signatures` when reestablishing a channel that had a pending splice become locked.

This test captures all the new spec requirements introduced for a splice to the channel reestablish flow.

wpaulino · 2025-09-18T00:20:28Z

This also includes a couple small bug fixes found along the way.

ldk-reviews-bot · 2025-09-18T00:20:31Z

👋 Thanks for assigning @jkczyz 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.

ldk-reviews-bot · 2025-09-20T00:20:51Z

🔔 1st Reminder

Hey @jkczyz! 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.

ldk-reviews-bot · 2025-09-22T00:21:02Z

🔔 2nd Reminder

Hey @jkczyz! 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.

ldk-reviews-bot · 2025-09-24T00:21:48Z

🔔 3rd Reminder

Hey @jkczyz! 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.

codecov · 2025-09-24T16:46:01Z

Codecov Report

❌ Patch coverage is 93.96325% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.72%. Comparing base (fec434b) to head (5594f9b).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	73.68%	14 Missing and 1 partial ⚠️
lightning/src/ln/functional_test_utils.rs	93.54%	2 Missing and 2 partials ⚠️
lightning/src/ln/splicing_tests.rs	98.08%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4079 +/- ##
==========================================
+ Coverage 88.63% 88.72% +0.09% 
==========================================
Files 180 180 Lines 135243 135528 +285 Branches 135243 135528 +285 ==========================================
+ Hits 119873 120252 +379 + Misses 12597 12506 -91 + Partials 2773 2770 -3

Flag	Coverage Δ
fuzzing	`21.72% <17.50%> (-0.04%)`	⬇️
tests	`88.57% <93.96%> (+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.

TheBlueMatt · 2025-09-25T13:36:34Z

Needs rebase :(

ldk-reviews-bot · 2025-09-25T16:49:02Z

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

TheBlueMatt · 2025-10-08T12:23:01Z

+ .interactive_tx_signing_session
+ .as_ref()
+ .map(|signing_session| signing_session.unsigned_tx().compute_txid());
+ self.pending_splice.is_some() && their_next_funding_txid == our_next_funding_txid


This is much stricter than the spec update proposed here - the spec update is simple (did the messages contain the fields) but this is checking that indirectly and in a complicated way. I'm not entirely sure why we need to bother with this, in any case, a redundant channel_ready isn't really a big deal and this is a bunch of annoying code.

The spec does note that they must be "for a splice transaction". I found a way to simplify this though, we can just check for existence of pending_splice on reestablish.

jkczyz

Latest push looks good modulo addressing @TheBlueMatt's comments.

We may produce duplicate `FundingTransactionReadyForSigning` events if the user has processed an initial event but has not yet called back with `funding_transaction_signed` and a peer reconnection occurs. If the user also handles the duplicate events, any duplicate calls to `funding_transaction_signed` after an initial successful one would return an error. This doesn't make sense, as the API should remain idempotent, so we return early on any duplicate calls.

We only want to produce `tx_signatures` once we know that the monitor update (either the initial one for a dual-funded channel, or a `RenegotiatedFunding` one for a splice) has been persisted. If we haven't received the counterparty's `commitment_signed` yet, then the monitor update hasn't been created, leading us to pass the `!awaiting_monitor_update` condition and produce a holder `tx_signatures` message.

If nodes have started a splice, this means they have both sent and received `channel_ready` already: in that case, it's unnecessary to retransmit `channel_ready` on reconnection.

Otherwise, we won't ever be able to resume a pending negotiation after a reconnection via `channel_reestablish`. Along the way, we also merge `should_reset_pending_splice_state` into `PendingFunding::can_abandon_state` to simplify the logic around when we're able to reset specific parts of the pending splice state.

We'll use this in the next commit to test the resend logic for `announcement_signatures` when reestablishing a channel that had a pending splice become locked.

This test captures all the new spec requirements introduced for a splice to the channel reestablish flow.

wpaulino · 2025-09-18T00:20:28Z

This also includes a couple small bug fixes found along the way.

ldk-reviews-bot · 2025-09-18T00:20:31Z

👋 Thanks for assigning @jkczyz 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.

ldk-reviews-bot · 2025-09-20T00:20:51Z

🔔 1st Reminder

Hey @jkczyz! 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.

ldk-reviews-bot · 2025-09-22T00:21:02Z

🔔 2nd Reminder

Hey @jkczyz! 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.

ldk-reviews-bot · 2025-09-24T00:21:48Z

🔔 3rd Reminder

Hey @jkczyz! 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.

codecov · 2025-09-24T16:46:01Z

Codecov Report

❌ Patch coverage is 93.96325% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.72%. Comparing base (fec434b) to head (5594f9b).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	73.68%	14 Missing and 1 partial ⚠️
lightning/src/ln/functional_test_utils.rs	93.54%	2 Missing and 2 partials ⚠️
lightning/src/ln/splicing_tests.rs	98.08%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4079 +/- ##
==========================================
+ Coverage 88.63% 88.72% +0.09% 
==========================================
Files 180 180 Lines 135243 135528 +285 Branches 135243 135528 +285 ==========================================
+ Hits 119873 120252 +379 + Misses 12597 12506 -91 + Partials 2773 2770 -3

Flag	Coverage Δ
fuzzing	`21.72% <17.50%> (-0.04%)`	⬇️
tests	`88.57% <93.96%> (+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.

TheBlueMatt · 2025-09-25T13:36:34Z

Needs rebase :(

ldk-reviews-bot · 2025-09-25T16:49:02Z

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

TheBlueMatt · 2025-10-08T12:23:01Z

+ .interactive_tx_signing_session
+ .as_ref()
+ .map(|signing_session| signing_session.unsigned_tx().compute_txid());
+ self.pending_splice.is_some() && their_next_funding_txid == our_next_funding_txid


This is much stricter than the spec update proposed here - the spec update is simple (did the messages contain the fields) but this is checking that indirectly and in a complicated way. I'm not entirely sure why we need to bother with this, in any case, a redundant channel_ready isn't really a big deal and this is a bunch of annoying code.

The spec does note that they must be "for a splice transaction". I found a way to simplify this though, we can just check for existence of pending_splice on reestablish.

jkczyz

Latest push looks good modulo addressing @TheBlueMatt's comments.

We may produce duplicate `FundingTransactionReadyForSigning` events if the user has processed an initial event but has not yet called back with `funding_transaction_signed` and a peer reconnection occurs. If the user also handles the duplicate events, any duplicate calls to `funding_transaction_signed` after an initial successful one would return an error. This doesn't make sense, as the API should remain idempotent, so we return early on any duplicate calls.

We only want to produce `tx_signatures` once we know that the monitor update (either the initial one for a dual-funded channel, or a `RenegotiatedFunding` one for a splice) has been persisted. If we haven't received the counterparty's `commitment_signed` yet, then the monitor update hasn't been created, leading us to pass the `!awaiting_monitor_update` condition and produce a holder `tx_signatures` message.

If nodes have started a splice, this means they have both sent and received `channel_ready` already: in that case, it's unnecessary to retransmit `channel_ready` on reconnection.

Otherwise, we won't ever be able to resume a pending negotiation after a reconnection via `channel_reestablish`. Along the way, we also merge `should_reset_pending_splice_state` into `PendingFunding::can_abandon_state` to simplify the logic around when we're able to reset specific parts of the pending splice state.

We'll use this in the next commit to test the resend logic for `announcement_signatures` when reestablishing a channel that had a pending splice become locked.

This test captures all the new spec requirements introduced for a splice to the channel reestablish flow.

wpaulino added this to the 0.2 milestone Sep 18, 2025

wpaulino requested a review from jkczyz September 18, 2025 00:20

wpaulino self-assigned this Sep 18, 2025

jkczyz mentioned this pull request Sep 19, 2025
Dual-funded channels and Splicing Project Tracking #1621
Closed
7 tasks

wpaulino force-pushed the test-splice-reestablish branch from aa5ab88 to e841721Compare September 24, 2025 16:36

wpaulino mentioned this pull request Sep 25, 2025
Allow outgoing splice request while disconnected #4122
Merged

wpaulino force-pushed the test-splice-reestablish branch from e841721 to 9588403Compare September 25, 2025 15:56

jkczyz reviewed Sep 25, 2025
View reviewed changes

Comment threadlightning/src/ln/channel.rs

jkczyz reviewed Sep 30, 2025
View reviewed changes

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated

wpaulino force-pushed the test-splice-reestablish branch from 9588403 to 02acd71Compare October 7, 2025 21:56

wpaulino requested review from TheBlueMatt and jkczyz October 7, 2025 22:15

TheBlueMatt reviewed Oct 8, 2025
View reviewed changes

jkczyz reviewed Oct 8, 2025
View reviewed changes

wpaulino force-pushed the test-splice-reestablish branch from 02acd71 to 5d2e474Compare October 8, 2025 17:43

wpaulino mentioned this pull request Oct 8, 2025
Emit SplicePending and SpliceFailed events #4077
Merged

wpaulino added 6 commits October 8, 2025 11:55

Avoid initial commitment channel_ready retransmission while splicing
fd4d052
If nodes have started a splice, this means they have both sent and received `channel_ready` already: in that case, it's unnecessary to retransmit `channel_ready` on reconnection.

Capture announcement signatures resend in reconnection tests
ac6be68
We'll use this in the next commit to test the resend logic for `announcement_signatures` when reestablishing a channel that had a pending splice become locked.

Test channel reestablish during splice lifecycle
5594f9b
This test captures all the new spec requirements introduced for a splice to the channel reestablish flow.

wpaulino force-pushed the test-splice-reestablish branch from 5d2e474 to 5594f9bCompare October 8, 2025 18:55

TheBlueMatt approved these changes Oct 8, 2025
View reviewed changes

jkczyz approved these changes Oct 8, 2025
View reviewed changes

jkczyz merged commit 3ce8204 into lightningdevkit:mainOct 8, 2025
24 checks passed

wpaulino deleted the test-splice-reestablish branch October 8, 2025 23:29