Support splice shared input signing by wpaulino · Pull Request #4024 · lightningdevkit/rust-lightning

wpaulino · 2025-08-21T17:02:58Z

This also addresses follow-ups from #3889.

ldk-reviews-bot · 2025-08-21T17:03:00Z

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

TheBlueMatt

LGTM, aside from the last commit.

TheBlueMatt · 2025-08-22T17:42:14Z

Also CI is sad.

The `handle_channel_resumption` path is reachable from both channel reestablish and monitor update completion. Since we only want to sign once we know the monitor update has completed, it's possible we could have unintentionally attempted to sign if we were still pending the monitor update but had a channel reestablish occur.

This is reachable if the event doesn't get handled and a channel reestablish occurs.

codecov · 2025-08-26T16:18:23Z

Codecov Report

❌ Patch coverage is 61.42857% with 81 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.74%. Comparing base (9724d19) to head (e664b7e).
⚠️ Report is 71 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/interactivetxs.rs	59.54%	42 Missing and 11 partials ⚠️
lightning/src/sign/mod.rs	0.00%	12 Missing ⚠️
lightning/src/ln/channelmanager.rs	40.00%	8 Missing and 1 partial ⚠️
lightning/src/util/test_channel_signer.rs	0.00%	4 Missing ⚠️
lightning/src/ln/channel.rs	93.75%	3 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4024 +/- ##
==========================================
- Coverage 88.77% 88.74% -0.03% 
==========================================
Files 175 176 +1 Lines 127846 128638 +792 Branches 127846 128638 +792 ==========================================
+ Hits 113492 114164 +672 - Misses 11788 11877 +89 - Partials 2566 2597 +31

Flag	Coverage Δ
fuzzing	`21.83% <12.74%> (-0.26%)`	⬇️
tests	`88.58% <61.42%> (-0.03%)`	⬇️

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.

jkczyz · 2025-08-26T22:10:11Z

+ inputs
+ .iter()
+ .position(|input| {
+ input.txin.previous_output == shared_funding_input.input.previous_output


Shouldn't we fail or assert if we can't find the shared input?

We just checked for it at the top of the method.

Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.

jkczyz · 2025-08-26T22:33:02Z

+ let mut witness = Witness::new();
+ witness.push(Vec::new());


Could you comment on why an empty Vec is pushed?

There's a bug in OP_CHECKMULTISIG that pops an extra argument from the stack than what is required, so all witnesses for it need this.

Right, but a comment in the code would be useful.

jkczyz · 2025-08-26T22:47:29Z

+ })
+ .unwrap_or(false)
+ {
+ debug_assert!(false);


Can't this be reached?

I guess it could? I was thinking it was already covered by the state checks above, so the assert here is really to make sure we catch a mismatch in the splice state.

This commit tracks all data related to the shared input of a splice, such that a valid witness can be formed upon the splice transaction finalization.

We also remove the `Result` to make it clear that this method does not support async operations yet and rename the method to clarify that it is only intended to be used for the shared input of a splice.

jkczyz · 2025-08-27T16:18:56Z

+ inputs
+ .iter()
+ .position(|input| {
+ input.txin.previous_output == shared_funding_input.input.previous_output


Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.

jkczyz · 2025-08-27T16:19:54Z

+ let mut witness = Witness::new();
+ witness.push(Vec::new());


Right, but a comment in the code would be useful.

wpaulino · 2025-08-27T17:21:27Z

Merging this since the changes after @TheBlueMatt's approval are trivial.

wpaulino · 2025-08-21T17:02:58Z

This also addresses follow-ups from #3889.

ldk-reviews-bot · 2025-08-21T17:03:00Z

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

TheBlueMatt

LGTM, aside from the last commit.

TheBlueMatt · 2025-08-22T17:42:14Z

Also CI is sad.

The `handle_channel_resumption` path is reachable from both channel reestablish and monitor update completion. Since we only want to sign once we know the monitor update has completed, it's possible we could have unintentionally attempted to sign if we were still pending the monitor update but had a channel reestablish occur.

This is reachable if the event doesn't get handled and a channel reestablish occurs.

codecov · 2025-08-26T16:18:23Z

Codecov Report

❌ Patch coverage is 61.42857% with 81 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.74%. Comparing base (9724d19) to head (e664b7e).
⚠️ Report is 71 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/interactivetxs.rs	59.54%	42 Missing and 11 partials ⚠️
lightning/src/sign/mod.rs	0.00%	12 Missing ⚠️
lightning/src/ln/channelmanager.rs	40.00%	8 Missing and 1 partial ⚠️
lightning/src/util/test_channel_signer.rs	0.00%	4 Missing ⚠️
lightning/src/ln/channel.rs	93.75%	3 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4024 +/- ##
==========================================
- Coverage 88.77% 88.74% -0.03% 
==========================================
Files 175 176 +1 Lines 127846 128638 +792 Branches 127846 128638 +792 ==========================================
+ Hits 113492 114164 +672 - Misses 11788 11877 +89 - Partials 2566 2597 +31

Flag	Coverage Δ
fuzzing	`21.83% <12.74%> (-0.26%)`	⬇️
tests	`88.58% <61.42%> (-0.03%)`	⬇️

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.

jkczyz · 2025-08-26T22:10:11Z

+ inputs
+ .iter()
+ .position(|input| {
+ input.txin.previous_output == shared_funding_input.input.previous_output


Shouldn't we fail or assert if we can't find the shared input?

We just checked for it at the top of the method.

Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.

jkczyz · 2025-08-26T22:33:02Z

+ let mut witness = Witness::new();
+ witness.push(Vec::new());


Could you comment on why an empty Vec is pushed?

There's a bug in OP_CHECKMULTISIG that pops an extra argument from the stack than what is required, so all witnesses for it need this.

Right, but a comment in the code would be useful.

jkczyz · 2025-08-26T22:47:29Z

+ })
+ .unwrap_or(false)
+ {
+ debug_assert!(false);


Can't this be reached?

I guess it could? I was thinking it was already covered by the state checks above, so the assert here is really to make sure we catch a mismatch in the splice state.

This commit tracks all data related to the shared input of a splice, such that a valid witness can be formed upon the splice transaction finalization.

We also remove the `Result` to make it clear that this method does not support async operations yet and rename the method to clarify that it is only intended to be used for the shared input of a splice.

jkczyz · 2025-08-27T16:18:56Z

+ inputs
+ .iter()
+ .position(|input| {
+ input.txin.previous_output == shared_funding_input.input.previous_output


Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.

jkczyz · 2025-08-27T16:19:54Z

+ let mut witness = Witness::new();
+ witness.push(Vec::new());


Right, but a comment in the code would be useful.

wpaulino · 2025-08-27T17:21:27Z

Merging this since the changes after @TheBlueMatt's approval are trivial.

wpaulino · 2025-08-21T17:02:58Z

This also addresses follow-ups from #3889.

ldk-reviews-bot · 2025-08-21T17:03:00Z

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

TheBlueMatt

LGTM, aside from the last commit.

TheBlueMatt · 2025-08-22T17:42:14Z

Also CI is sad.

The `handle_channel_resumption` path is reachable from both channel reestablish and monitor update completion. Since we only want to sign once we know the monitor update has completed, it's possible we could have unintentionally attempted to sign if we were still pending the monitor update but had a channel reestablish occur.

This is reachable if the event doesn't get handled and a channel reestablish occurs.

codecov · 2025-08-26T16:18:23Z

Codecov Report

❌ Patch coverage is 61.42857% with 81 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.74%. Comparing base (9724d19) to head (e664b7e).
⚠️ Report is 71 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/interactivetxs.rs	59.54%	42 Missing and 11 partials ⚠️
lightning/src/sign/mod.rs	0.00%	12 Missing ⚠️
lightning/src/ln/channelmanager.rs	40.00%	8 Missing and 1 partial ⚠️
lightning/src/util/test_channel_signer.rs	0.00%	4 Missing ⚠️
lightning/src/ln/channel.rs	93.75%	3 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4024 +/- ##
==========================================
- Coverage 88.77% 88.74% -0.03% 
==========================================
Files 175 176 +1 Lines 127846 128638 +792 Branches 127846 128638 +792 ==========================================
+ Hits 113492 114164 +672 - Misses 11788 11877 +89 - Partials 2566 2597 +31

Flag	Coverage Δ
fuzzing	`21.83% <12.74%> (-0.26%)`	⬇️
tests	`88.58% <61.42%> (-0.03%)`	⬇️

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.

jkczyz · 2025-08-26T22:10:11Z

+ inputs
+ .iter()
+ .position(|input| {
+ input.txin.previous_output == shared_funding_input.input.previous_output


Shouldn't we fail or assert if we can't find the shared input?

We just checked for it at the top of the method.

Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.

jkczyz · 2025-08-26T22:33:02Z

+ let mut witness = Witness::new();
+ witness.push(Vec::new());


Could you comment on why an empty Vec is pushed?

There's a bug in OP_CHECKMULTISIG that pops an extra argument from the stack than what is required, so all witnesses for it need this.

Right, but a comment in the code would be useful.

jkczyz · 2025-08-26T22:47:29Z

+ })
+ .unwrap_or(false)
+ {
+ debug_assert!(false);


Can't this be reached?

I guess it could? I was thinking it was already covered by the state checks above, so the assert here is really to make sure we catch a mismatch in the splice state.

This commit tracks all data related to the shared input of a splice, such that a valid witness can be formed upon the splice transaction finalization.

We also remove the `Result` to make it clear that this method does not support async operations yet and rename the method to clarify that it is only intended to be used for the shared input of a splice.

jkczyz · 2025-08-27T16:18:56Z

+ inputs
+ .iter()
+ .position(|input| {
+ input.txin.previous_output == shared_funding_input.input.previous_output


Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.

jkczyz · 2025-08-27T16:19:54Z

+ let mut witness = Witness::new();
+ witness.push(Vec::new());


Right, but a comment in the code would be useful.

wpaulino · 2025-08-27T17:21:27Z

Merging this since the changes after @TheBlueMatt's approval are trivial.

wpaulino · 2025-08-21T17:02:58Z

This also addresses follow-ups from #3889.

ldk-reviews-bot · 2025-08-21T17:03:00Z

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

TheBlueMatt

LGTM, aside from the last commit.

TheBlueMatt · 2025-08-22T17:42:14Z

Also CI is sad.

The `handle_channel_resumption` path is reachable from both channel reestablish and monitor update completion. Since we only want to sign once we know the monitor update has completed, it's possible we could have unintentionally attempted to sign if we were still pending the monitor update but had a channel reestablish occur.

This is reachable if the event doesn't get handled and a channel reestablish occurs.

codecov · 2025-08-26T16:18:23Z

Codecov Report

❌ Patch coverage is 61.42857% with 81 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.74%. Comparing base (9724d19) to head (e664b7e).
⚠️ Report is 71 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/interactivetxs.rs	59.54%	42 Missing and 11 partials ⚠️
lightning/src/sign/mod.rs	0.00%	12 Missing ⚠️
lightning/src/ln/channelmanager.rs	40.00%	8 Missing and 1 partial ⚠️
lightning/src/util/test_channel_signer.rs	0.00%	4 Missing ⚠️
lightning/src/ln/channel.rs	93.75%	3 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4024 +/- ##
==========================================
- Coverage 88.77% 88.74% -0.03% 
==========================================
Files 175 176 +1 Lines 127846 128638 +792 Branches 127846 128638 +792 ==========================================
+ Hits 113492 114164 +672 - Misses 11788 11877 +89 - Partials 2566 2597 +31

Flag	Coverage Δ
fuzzing	`21.83% <12.74%> (-0.26%)`	⬇️
tests	`88.58% <61.42%> (-0.03%)`	⬇️

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.

jkczyz · 2025-08-26T22:10:11Z

+ inputs
+ .iter()
+ .position(|input| {
+ input.txin.previous_output == shared_funding_input.input.previous_output


Shouldn't we fail or assert if we can't find the shared input?

We just checked for it at the top of the method.

Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.

jkczyz · 2025-08-26T22:33:02Z

+ let mut witness = Witness::new();
+ witness.push(Vec::new());


Could you comment on why an empty Vec is pushed?

There's a bug in OP_CHECKMULTISIG that pops an extra argument from the stack than what is required, so all witnesses for it need this.

Right, but a comment in the code would be useful.

jkczyz · 2025-08-26T22:47:29Z

+ })
+ .unwrap_or(false)
+ {
+ debug_assert!(false);


Can't this be reached?

I guess it could? I was thinking it was already covered by the state checks above, so the assert here is really to make sure we catch a mismatch in the splice state.

This commit tracks all data related to the shared input of a splice, such that a valid witness can be formed upon the splice transaction finalization.

We also remove the `Result` to make it clear that this method does not support async operations yet and rename the method to clarify that it is only intended to be used for the shared input of a splice.

jkczyz · 2025-08-27T16:18:56Z

+ inputs
+ .iter()
+ .position(|input| {
+ input.txin.previous_output == shared_funding_input.input.previous_output


Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.

jkczyz · 2025-08-27T16:19:54Z

+ let mut witness = Witness::new();
+ witness.push(Vec::new());


Right, but a comment in the code would be useful.

wpaulino · 2025-08-27T17:21:27Z

Merging this since the changes after @TheBlueMatt's approval are trivial.

wpaulino · 2025-08-21T17:02:58Z

This also addresses follow-ups from #3889.

ldk-reviews-bot · 2025-08-21T17:03:00Z

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

TheBlueMatt

LGTM, aside from the last commit.

TheBlueMatt · 2025-08-22T17:42:14Z

Also CI is sad.

The `handle_channel_resumption` path is reachable from both channel reestablish and monitor update completion. Since we only want to sign once we know the monitor update has completed, it's possible we could have unintentionally attempted to sign if we were still pending the monitor update but had a channel reestablish occur.

This is reachable if the event doesn't get handled and a channel reestablish occurs.

codecov · 2025-08-26T16:18:23Z

Codecov Report

❌ Patch coverage is 61.42857% with 81 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.74%. Comparing base (9724d19) to head (e664b7e).
⚠️ Report is 71 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/interactivetxs.rs	59.54%	42 Missing and 11 partials ⚠️
lightning/src/sign/mod.rs	0.00%	12 Missing ⚠️
lightning/src/ln/channelmanager.rs	40.00%	8 Missing and 1 partial ⚠️
lightning/src/util/test_channel_signer.rs	0.00%	4 Missing ⚠️
lightning/src/ln/channel.rs	93.75%	3 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4024 +/- ##
==========================================
- Coverage 88.77% 88.74% -0.03% 
==========================================
Files 175 176 +1 Lines 127846 128638 +792 Branches 127846 128638 +792 ==========================================
+ Hits 113492 114164 +672 - Misses 11788 11877 +89 - Partials 2566 2597 +31

Flag	Coverage Δ
fuzzing	`21.83% <12.74%> (-0.26%)`	⬇️
tests	`88.58% <61.42%> (-0.03%)`	⬇️

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.

jkczyz · 2025-08-26T22:10:11Z

+ inputs
+ .iter()
+ .position(|input| {
+ input.txin.previous_output == shared_funding_input.input.previous_output


Shouldn't we fail or assert if we can't find the shared input?

We just checked for it at the top of the method.

Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.

jkczyz · 2025-08-26T22:33:02Z

+ let mut witness = Witness::new();
+ witness.push(Vec::new());


Could you comment on why an empty Vec is pushed?

There's a bug in OP_CHECKMULTISIG that pops an extra argument from the stack than what is required, so all witnesses for it need this.

Right, but a comment in the code would be useful.

jkczyz · 2025-08-26T22:47:29Z

+ })
+ .unwrap_or(false)
+ {
+ debug_assert!(false);


Can't this be reached?

I guess it could? I was thinking it was already covered by the state checks above, so the assert here is really to make sure we catch a mismatch in the splice state.

This commit tracks all data related to the shared input of a splice, such that a valid witness can be formed upon the splice transaction finalization.

We also remove the `Result` to make it clear that this method does not support async operations yet and rename the method to clarify that it is only intended to be used for the shared input of a splice.

jkczyz · 2025-08-27T16:18:56Z

+ inputs
+ .iter()
+ .position(|input| {
+ input.txin.previous_output == shared_funding_input.input.previous_output


Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.

jkczyz · 2025-08-27T16:19:54Z

+ let mut witness = Witness::new();
+ witness.push(Vec::new());


Right, but a comment in the code would be useful.

wpaulino · 2025-08-27T17:21:27Z

Merging this since the changes after @TheBlueMatt's approval are trivial.

wpaulino · 2025-08-21T17:02:58Z

This also addresses follow-ups from #3889.

ldk-reviews-bot · 2025-08-21T17:03:00Z

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

TheBlueMatt

LGTM, aside from the last commit.

TheBlueMatt · 2025-08-22T17:42:14Z

Also CI is sad.

The `handle_channel_resumption` path is reachable from both channel reestablish and monitor update completion. Since we only want to sign once we know the monitor update has completed, it's possible we could have unintentionally attempted to sign if we were still pending the monitor update but had a channel reestablish occur.

This is reachable if the event doesn't get handled and a channel reestablish occurs.

codecov · 2025-08-26T16:18:23Z

Codecov Report

❌ Patch coverage is 61.42857% with 81 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.74%. Comparing base (9724d19) to head (e664b7e).
⚠️ Report is 71 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/interactivetxs.rs	59.54%	42 Missing and 11 partials ⚠️
lightning/src/sign/mod.rs	0.00%	12 Missing ⚠️
lightning/src/ln/channelmanager.rs	40.00%	8 Missing and 1 partial ⚠️
lightning/src/util/test_channel_signer.rs	0.00%	4 Missing ⚠️
lightning/src/ln/channel.rs	93.75%	3 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4024 +/- ##
==========================================
- Coverage 88.77% 88.74% -0.03% 
==========================================
Files 175 176 +1 Lines 127846 128638 +792 Branches 127846 128638 +792 ==========================================
+ Hits 113492 114164 +672 - Misses 11788 11877 +89 - Partials 2566 2597 +31

Flag	Coverage Δ
fuzzing	`21.83% <12.74%> (-0.26%)`	⬇️
tests	`88.58% <61.42%> (-0.03%)`	⬇️

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.

jkczyz · 2025-08-26T22:10:11Z

+ inputs
+ .iter()
+ .position(|input| {
+ input.txin.previous_output == shared_funding_input.input.previous_output


Shouldn't we fail or assert if we can't find the shared input?

We just checked for it at the top of the method.

Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.

jkczyz · 2025-08-26T22:33:02Z

+ let mut witness = Witness::new();
+ witness.push(Vec::new());


Could you comment on why an empty Vec is pushed?

There's a bug in OP_CHECKMULTISIG that pops an extra argument from the stack than what is required, so all witnesses for it need this.

Right, but a comment in the code would be useful.

jkczyz · 2025-08-26T22:47:29Z

+ })
+ .unwrap_or(false)
+ {
+ debug_assert!(false);


Can't this be reached?

I guess it could? I was thinking it was already covered by the state checks above, so the assert here is really to make sure we catch a mismatch in the splice state.

This commit tracks all data related to the shared input of a splice, such that a valid witness can be formed upon the splice transaction finalization.

We also remove the `Result` to make it clear that this method does not support async operations yet and rename the method to clarify that it is only intended to be used for the shared input of a splice.

jkczyz · 2025-08-27T16:18:56Z

+ inputs
+ .iter()
+ .position(|input| {
+ input.txin.previous_output == shared_funding_input.input.previous_output


Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.

jkczyz · 2025-08-27T16:19:54Z

+ let mut witness = Witness::new();
+ witness.push(Vec::new());


Right, but a comment in the code would be useful.

wpaulino · 2025-08-27T17:21:27Z

Merging this since the changes after @TheBlueMatt's approval are trivial.

wpaulino · 2025-08-21T17:02:58Z

This also addresses follow-ups from #3889.

ldk-reviews-bot · 2025-08-21T17:03:00Z

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

TheBlueMatt

LGTM, aside from the last commit.

TheBlueMatt · 2025-08-22T17:42:14Z

Also CI is sad.

The `handle_channel_resumption` path is reachable from both channel reestablish and monitor update completion. Since we only want to sign once we know the monitor update has completed, it's possible we could have unintentionally attempted to sign if we were still pending the monitor update but had a channel reestablish occur.

This is reachable if the event doesn't get handled and a channel reestablish occurs.

codecov · 2025-08-26T16:18:23Z

Codecov Report

❌ Patch coverage is 61.42857% with 81 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.74%. Comparing base (9724d19) to head (e664b7e).
⚠️ Report is 71 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/interactivetxs.rs	59.54%	42 Missing and 11 partials ⚠️
lightning/src/sign/mod.rs	0.00%	12 Missing ⚠️
lightning/src/ln/channelmanager.rs	40.00%	8 Missing and 1 partial ⚠️
lightning/src/util/test_channel_signer.rs	0.00%	4 Missing ⚠️
lightning/src/ln/channel.rs	93.75%	3 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4024 +/- ##
==========================================
- Coverage 88.77% 88.74% -0.03% 
==========================================
Files 175 176 +1 Lines 127846 128638 +792 Branches 127846 128638 +792 ==========================================
+ Hits 113492 114164 +672 - Misses 11788 11877 +89 - Partials 2566 2597 +31

Flag	Coverage Δ
fuzzing	`21.83% <12.74%> (-0.26%)`	⬇️
tests	`88.58% <61.42%> (-0.03%)`	⬇️

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.

jkczyz · 2025-08-26T22:10:11Z

+ inputs
+ .iter()
+ .position(|input| {
+ input.txin.previous_output == shared_funding_input.input.previous_output


Shouldn't we fail or assert if we can't find the shared input?

We just checked for it at the top of the method.

Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.

jkczyz · 2025-08-26T22:33:02Z

+ let mut witness = Witness::new();
+ witness.push(Vec::new());


Could you comment on why an empty Vec is pushed?

There's a bug in OP_CHECKMULTISIG that pops an extra argument from the stack than what is required, so all witnesses for it need this.

Right, but a comment in the code would be useful.

jkczyz · 2025-08-26T22:47:29Z

+ })
+ .unwrap_or(false)
+ {
+ debug_assert!(false);


Can't this be reached?

I guess it could? I was thinking it was already covered by the state checks above, so the assert here is really to make sure we catch a mismatch in the splice state.

This commit tracks all data related to the shared input of a splice, such that a valid witness can be formed upon the splice transaction finalization.

We also remove the `Result` to make it clear that this method does not support async operations yet and rename the method to clarify that it is only intended to be used for the shared input of a splice.

jkczyz · 2025-08-27T16:18:56Z

+ inputs
+ .iter()
+ .position(|input| {
+ input.txin.previous_output == shared_funding_input.input.previous_output


Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.

jkczyz · 2025-08-27T16:19:54Z

+ let mut witness = Witness::new();
+ witness.push(Vec::new());


Right, but a comment in the code would be useful.

wpaulino · 2025-08-27T17:21:27Z

Merging this since the changes after @TheBlueMatt's approval are trivial.

wpaulino · 2025-08-21T17:02:58Z

This also addresses follow-ups from #3889.

ldk-reviews-bot · 2025-08-21T17:03:00Z

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

TheBlueMatt

LGTM, aside from the last commit.

TheBlueMatt · 2025-08-22T17:42:14Z

Also CI is sad.

The `handle_channel_resumption` path is reachable from both channel reestablish and monitor update completion. Since we only want to sign once we know the monitor update has completed, it's possible we could have unintentionally attempted to sign if we were still pending the monitor update but had a channel reestablish occur.

This is reachable if the event doesn't get handled and a channel reestablish occurs.

codecov · 2025-08-26T16:18:23Z

Codecov Report

❌ Patch coverage is 61.42857% with 81 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.74%. Comparing base (9724d19) to head (e664b7e).
⚠️ Report is 71 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/interactivetxs.rs	59.54%	42 Missing and 11 partials ⚠️
lightning/src/sign/mod.rs	0.00%	12 Missing ⚠️
lightning/src/ln/channelmanager.rs	40.00%	8 Missing and 1 partial ⚠️
lightning/src/util/test_channel_signer.rs	0.00%	4 Missing ⚠️
lightning/src/ln/channel.rs	93.75%	3 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4024 +/- ##
==========================================
- Coverage 88.77% 88.74% -0.03% 
==========================================
Files 175 176 +1 Lines 127846 128638 +792 Branches 127846 128638 +792 ==========================================
+ Hits 113492 114164 +672 - Misses 11788 11877 +89 - Partials 2566 2597 +31

Flag	Coverage Δ
fuzzing	`21.83% <12.74%> (-0.26%)`	⬇️
tests	`88.58% <61.42%> (-0.03%)`	⬇️

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.

jkczyz · 2025-08-26T22:10:11Z

+ inputs
+ .iter()
+ .position(|input| {
+ input.txin.previous_output == shared_funding_input.input.previous_output


Shouldn't we fail or assert if we can't find the shared input?

We just checked for it at the top of the method.

Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.

jkczyz · 2025-08-26T22:33:02Z

+ let mut witness = Witness::new();
+ witness.push(Vec::new());


Could you comment on why an empty Vec is pushed?

There's a bug in OP_CHECKMULTISIG that pops an extra argument from the stack than what is required, so all witnesses for it need this.

Right, but a comment in the code would be useful.

jkczyz · 2025-08-26T22:47:29Z

+ })
+ .unwrap_or(false)
+ {
+ debug_assert!(false);


Can't this be reached?

I guess it could? I was thinking it was already covered by the state checks above, so the assert here is really to make sure we catch a mismatch in the splice state.

This commit tracks all data related to the shared input of a splice, such that a valid witness can be formed upon the splice transaction finalization.

We also remove the `Result` to make it clear that this method does not support async operations yet and rename the method to clarify that it is only intended to be used for the shared input of a splice.

jkczyz · 2025-08-27T16:18:56Z

+ inputs
+ .iter()
+ .position(|input| {
+ input.txin.previous_output == shared_funding_input.input.previous_output


Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.

jkczyz · 2025-08-27T16:19:54Z

+ let mut witness = Witness::new();
+ witness.push(Vec::new());


Right, but a comment in the code would be useful.

wpaulino · 2025-08-27T17:21:27Z

Merging this since the changes after @TheBlueMatt's approval are trivial.

wpaulino requested review from TheBlueMatt and jkczyz August 21, 2025 17:02

wpaulino self-assigned this Aug 21, 2025

wpaulino added this to Weekly Goals Aug 21, 2025

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

TheBlueMatt reviewed Aug 22, 2025
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

jkczyz reviewed Aug 22, 2025
View reviewed changes

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

wpaulino added 4 commits August 26, 2025 09:11

Remove reachable FundingTransactionReadyForSigning assertion
dfd7779
This is reachable if the event doesn't get handled and a channel reestablish occurs.

Fix empty witness filter in funding_transaction_signed
bbe590d

Assume splicing input value from channel parameters
712b385

wpaulino force-pushed the sign-splice-shared-input branch from 1512cd9 to 8c9cc05Compare August 26, 2025 16:13

wpaulino requested review from TheBlueMatt and jkczyz August 26, 2025 16:14

TheBlueMatt previously approved these changes Aug 26, 2025
View reviewed changes

jkczyz reviewed Aug 26, 2025
View reviewed changes

wpaulino added 2 commits August 26, 2025 16:49

Support splice shared input signing
1170e1b
This commit tracks all data related to the shared input of a splice, such that a valid witness can be formed upon the splice transaction finalization.

Sign splice shared input when producing holder tx_signatures
e664b7e
We also remove the `Result` to make it clear that this method does not support async operations yet and rename the method to clarify that it is only intended to be used for the shared input of a splice.

wpaulino dismissed TheBlueMatt’s stale review via e664b7eAugust 26, 2025 23:50

wpaulino force-pushed the sign-splice-shared-input branch from 8c9cc05 to e664b7eCompare August 26, 2025 23:50

jkczyz approved these changes Aug 27, 2025
View reviewed changes

wpaulino merged commit 2772bfd into lightningdevkit:mainAug 27, 2025
25 checks passed

wpaulino deleted the sign-splice-shared-input branch August 27, 2025 17:21

github-project-automationBot moved this to Done in Weekly Goals Aug 27, 2025