Wipe splice state upon failed interactive funding construction

[wpaulino]

An interactive funding construction can be considered failed upon a disconnect or a tx_abort message. So far, we've consumed the InteractiveTxConstructor in the latter case, but not the former. Additionally, we may have splice-specific state that needs to be consumed as well to allow us to negotiate another splice later on.

This commit ensures that we properly consume all splice and interactive funding state whenever possible upon a disconnect or tx_abort.

The interactive funding state is safe to consume as long as we have either yet to reach AwaitingSignatures, or we have but tx_signatures has not been sent/received.

The splice state is safe to consume as long as we don't have a pending FundingNegotiation::AwaitingSignatures with a tx_signatures sent/received and we don't have any negotiated candidates. Note that until splice RBF is supported, it is not currently possible to have any negotiated candidates with a pending interactive funding transaction.

[ldk-reviews-bot]

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

[codecov]

Codecov Report

❌ Patch coverage is 88.14815% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.68%. Comparing base (3e21ba3) to head (6d2b110).
⚠️ Report is 82 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	60.00%	30 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4120 +/- ##
==========================================
- Coverage 88.72% 88.68% -0.04% 
==========================================
Files 177 180 +3 Lines 133404 135148 +1744 Branches 133404 135148 +1744 ==========================================
+ Hits 118365 119860 +1495 - Misses 12325 12523 +198 - Partials 2714 2765 +51 

Flag	Coverage Δ
fuzzing	21.79% <33.75%> (+0.05%)	⬆️
tests	88.52% <88.14%> (-0.04%)	⬇️

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]

Does it matter if we received their signatures if we haven't sent ours? Or is it because if they sent theirs then they would not have reset their state?

[wpaulino]

You're supposed to remember the channel as soon as one side has sent tx_signatures, because you can't be sure the other side didn't just sign and broadcast without sending their own.

[ldk-reviews-bot]

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

[jkczyz]

Looks like we aren't calling fail_interactive_tx_negotiation when failing to handle a splice_ack. #4077 updates fail_interactive_tx_negotiation to take a NegotiationError, which it uses to construct a SpliceFundingFailed struct.

For FundingNegotiation::ConstructingTransaction, the NegotiationError is formed by copying the inputs from the InteractiveTxConstructor. For FundingNegotiation::AwaitingAck when handling splice_ack, we'd need to do something similar?

Either way we are cloning the inputs just to later take the FundingNegotiation here. Maybe that is ok for now? Any thoughts on a better way of doing this?

[jkczyz]

Hmmm... so we are also already take'ing the FundingNegotiation in one place in splice_ack handling (when FundingNegotiation::into_interactive_tx_constructor fails), but not earlier when validating the splice_ack message.

[wpaulino]

We shouldn't need to call fail_interactive_tx_negotiation whenever we fail handling a message by sending a warning and disconnecting.

[jkczyz]

Ah, so failing to process a splice_ack would just disconnect, and the peer could re-send it after reconnecting. Do we eventually timeout quiescence somewhere?

[wpaulino]

Quiescence is also implicitly terminated upon disconnection, but we do have a timeout enforced at should_disconnect_peer_awaiting_response.

[jkczyz]

Oh, so we'd need to produce a SpliceFailed event in some other way? I'm thinking when we are in FundingNegotiation::AwaitingAck and fail processing the splice_ack.

[wpaulino]

Yeah sounds like that logic might need to live in the disconnection handler, since we'll need to emit an event anyway if a peer disconnects mid-splice negotiation for whatever reason.

[wpaulino]

An interactive funding construction can be considered failed upon a disconnect or a tx_abort message. So far, we've consumed the InteractiveTxConstructor in the latter case, but not the former. Additionally, we may have splice-specific state that needs to be consumed as well to allow us to negotiate another splice later on.

This commit ensures that we properly consume all splice and interactive funding state whenever possible upon a disconnect or tx_abort.

The interactive funding state is safe to consume as long as we have either yet to reach AwaitingSignatures, or we have but tx_signatures has not been sent/received.

The splice state is safe to consume as long as we don't have a pending FundingNegotiation::AwaitingSignatures with a tx_signatures sent/received and we don't have any negotiated candidates. Note that until splice RBF is supported, it is not currently possible to have any negotiated candidates with a pending interactive funding transaction.

[ldk-reviews-bot]

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

[codecov]

Codecov Report

❌ Patch coverage is 88.14815% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.68%. Comparing base (3e21ba3) to head (6d2b110).
⚠️ Report is 82 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	60.00%	30 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4120 +/- ##
==========================================
- Coverage 88.72% 88.68% -0.04% 
==========================================
Files 177 180 +3 Lines 133404 135148 +1744 Branches 133404 135148 +1744 ==========================================
+ Hits 118365 119860 +1495 - Misses 12325 12523 +198 - Partials 2714 2765 +51 

Flag	Coverage Δ
fuzzing	21.79% <33.75%> (+0.05%)	⬆️
tests	88.52% <88.14%> (-0.04%)	⬇️

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]

Does it matter if we received their signatures if we haven't sent ours? Or is it because if they sent theirs then they would not have reset their state?

[wpaulino]

You're supposed to remember the channel as soon as one side has sent tx_signatures, because you can't be sure the other side didn't just sign and broadcast without sending their own.

[ldk-reviews-bot]

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

[jkczyz]

Looks like we aren't calling fail_interactive_tx_negotiation when failing to handle a splice_ack. #4077 updates fail_interactive_tx_negotiation to take a NegotiationError, which it uses to construct a SpliceFundingFailed struct.

For FundingNegotiation::ConstructingTransaction, the NegotiationError is formed by copying the inputs from the InteractiveTxConstructor. For FundingNegotiation::AwaitingAck when handling splice_ack, we'd need to do something similar?

Either way we are cloning the inputs just to later take the FundingNegotiation here. Maybe that is ok for now? Any thoughts on a better way of doing this?

[jkczyz]

Hmmm... so we are also already take'ing the FundingNegotiation in one place in splice_ack handling (when FundingNegotiation::into_interactive_tx_constructor fails), but not earlier when validating the splice_ack message.

[wpaulino]

We shouldn't need to call fail_interactive_tx_negotiation whenever we fail handling a message by sending a warning and disconnecting.

[jkczyz]

Ah, so failing to process a splice_ack would just disconnect, and the peer could re-send it after reconnecting. Do we eventually timeout quiescence somewhere?

[wpaulino]

Quiescence is also implicitly terminated upon disconnection, but we do have a timeout enforced at should_disconnect_peer_awaiting_response.

[jkczyz]

Oh, so we'd need to produce a SpliceFailed event in some other way? I'm thinking when we are in FundingNegotiation::AwaitingAck and fail processing the splice_ack.

[wpaulino]

Yeah sounds like that logic might need to live in the disconnection handler, since we'll need to emit an event anyway if a peer disconnects mid-splice negotiation for whatever reason.

[wpaulino]

An interactive funding construction can be considered failed upon a disconnect or a tx_abort message. So far, we've consumed the InteractiveTxConstructor in the latter case, but not the former. Additionally, we may have splice-specific state that needs to be consumed as well to allow us to negotiate another splice later on.

This commit ensures that we properly consume all splice and interactive funding state whenever possible upon a disconnect or tx_abort.

The interactive funding state is safe to consume as long as we have either yet to reach AwaitingSignatures, or we have but tx_signatures has not been sent/received.

The splice state is safe to consume as long as we don't have a pending FundingNegotiation::AwaitingSignatures with a tx_signatures sent/received and we don't have any negotiated candidates. Note that until splice RBF is supported, it is not currently possible to have any negotiated candidates with a pending interactive funding transaction.

[ldk-reviews-bot]

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

[codecov]

Codecov Report

❌ Patch coverage is 88.14815% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.68%. Comparing base (3e21ba3) to head (6d2b110).
⚠️ Report is 82 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	60.00%	30 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4120 +/- ##
==========================================
- Coverage 88.72% 88.68% -0.04% 
==========================================
Files 177 180 +3 Lines 133404 135148 +1744 Branches 133404 135148 +1744 ==========================================
+ Hits 118365 119860 +1495 - Misses 12325 12523 +198 - Partials 2714 2765 +51 

Flag	Coverage Δ
fuzzing	21.79% <33.75%> (+0.05%)	⬆️
tests	88.52% <88.14%> (-0.04%)	⬇️

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]

Does it matter if we received their signatures if we haven't sent ours? Or is it because if they sent theirs then they would not have reset their state?

[wpaulino]

You're supposed to remember the channel as soon as one side has sent tx_signatures, because you can't be sure the other side didn't just sign and broadcast without sending their own.

[ldk-reviews-bot]

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

[jkczyz]

Looks like we aren't calling fail_interactive_tx_negotiation when failing to handle a splice_ack. #4077 updates fail_interactive_tx_negotiation to take a NegotiationError, which it uses to construct a SpliceFundingFailed struct.

For FundingNegotiation::ConstructingTransaction, the NegotiationError is formed by copying the inputs from the InteractiveTxConstructor. For FundingNegotiation::AwaitingAck when handling splice_ack, we'd need to do something similar?

Either way we are cloning the inputs just to later take the FundingNegotiation here. Maybe that is ok for now? Any thoughts on a better way of doing this?

[jkczyz]

Hmmm... so we are also already take'ing the FundingNegotiation in one place in splice_ack handling (when FundingNegotiation::into_interactive_tx_constructor fails), but not earlier when validating the splice_ack message.

[wpaulino]

We shouldn't need to call fail_interactive_tx_negotiation whenever we fail handling a message by sending a warning and disconnecting.

[jkczyz]

Ah, so failing to process a splice_ack would just disconnect, and the peer could re-send it after reconnecting. Do we eventually timeout quiescence somewhere?

[wpaulino]

Quiescence is also implicitly terminated upon disconnection, but we do have a timeout enforced at should_disconnect_peer_awaiting_response.

[jkczyz]

Oh, so we'd need to produce a SpliceFailed event in some other way? I'm thinking when we are in FundingNegotiation::AwaitingAck and fail processing the splice_ack.

[wpaulino]

Yeah sounds like that logic might need to live in the disconnection handler, since we'll need to emit an event anyway if a peer disconnects mid-splice negotiation for whatever reason.

[wpaulino]

An interactive funding construction can be considered failed upon a disconnect or a tx_abort message. So far, we've consumed the InteractiveTxConstructor in the latter case, but not the former. Additionally, we may have splice-specific state that needs to be consumed as well to allow us to negotiate another splice later on.

This commit ensures that we properly consume all splice and interactive funding state whenever possible upon a disconnect or tx_abort.

The interactive funding state is safe to consume as long as we have either yet to reach AwaitingSignatures, or we have but tx_signatures has not been sent/received.

The splice state is safe to consume as long as we don't have a pending FundingNegotiation::AwaitingSignatures with a tx_signatures sent/received and we don't have any negotiated candidates. Note that until splice RBF is supported, it is not currently possible to have any negotiated candidates with a pending interactive funding transaction.

[ldk-reviews-bot]

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

[codecov]

Codecov Report

❌ Patch coverage is 88.14815% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.68%. Comparing base (3e21ba3) to head (6d2b110).
⚠️ Report is 82 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	60.00%	30 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4120 +/- ##
==========================================
- Coverage 88.72% 88.68% -0.04% 
==========================================
Files 177 180 +3 Lines 133404 135148 +1744 Branches 133404 135148 +1744 ==========================================
+ Hits 118365 119860 +1495 - Misses 12325 12523 +198 - Partials 2714 2765 +51 

Flag	Coverage Δ
fuzzing	21.79% <33.75%> (+0.05%)	⬆️
tests	88.52% <88.14%> (-0.04%)	⬇️

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]

Does it matter if we received their signatures if we haven't sent ours? Or is it because if they sent theirs then they would not have reset their state?

[wpaulino]

You're supposed to remember the channel as soon as one side has sent tx_signatures, because you can't be sure the other side didn't just sign and broadcast without sending their own.

[ldk-reviews-bot]

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

[jkczyz]

Looks like we aren't calling fail_interactive_tx_negotiation when failing to handle a splice_ack. #4077 updates fail_interactive_tx_negotiation to take a NegotiationError, which it uses to construct a SpliceFundingFailed struct.

For FundingNegotiation::ConstructingTransaction, the NegotiationError is formed by copying the inputs from the InteractiveTxConstructor. For FundingNegotiation::AwaitingAck when handling splice_ack, we'd need to do something similar?

Either way we are cloning the inputs just to later take the FundingNegotiation here. Maybe that is ok for now? Any thoughts on a better way of doing this?

[jkczyz]

Hmmm... so we are also already take'ing the FundingNegotiation in one place in splice_ack handling (when FundingNegotiation::into_interactive_tx_constructor fails), but not earlier when validating the splice_ack message.

[wpaulino]

We shouldn't need to call fail_interactive_tx_negotiation whenever we fail handling a message by sending a warning and disconnecting.

[jkczyz]

Ah, so failing to process a splice_ack would just disconnect, and the peer could re-send it after reconnecting. Do we eventually timeout quiescence somewhere?

[wpaulino]

Quiescence is also implicitly terminated upon disconnection, but we do have a timeout enforced at should_disconnect_peer_awaiting_response.

[jkczyz]

Oh, so we'd need to produce a SpliceFailed event in some other way? I'm thinking when we are in FundingNegotiation::AwaitingAck and fail processing the splice_ack.

[wpaulino]

Yeah sounds like that logic might need to live in the disconnection handler, since we'll need to emit an event anyway if a peer disconnects mid-splice negotiation for whatever reason.

[wpaulino]

An interactive funding construction can be considered failed upon a disconnect or a tx_abort message. So far, we've consumed the InteractiveTxConstructor in the latter case, but not the former. Additionally, we may have splice-specific state that needs to be consumed as well to allow us to negotiate another splice later on.

This commit ensures that we properly consume all splice and interactive funding state whenever possible upon a disconnect or tx_abort.

The interactive funding state is safe to consume as long as we have either yet to reach AwaitingSignatures, or we have but tx_signatures has not been sent/received.

The splice state is safe to consume as long as we don't have a pending FundingNegotiation::AwaitingSignatures with a tx_signatures sent/received and we don't have any negotiated candidates. Note that until splice RBF is supported, it is not currently possible to have any negotiated candidates with a pending interactive funding transaction.

[ldk-reviews-bot]

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

[codecov]

Codecov Report

❌ Patch coverage is 88.14815% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.68%. Comparing base (3e21ba3) to head (6d2b110).
⚠️ Report is 82 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	60.00%	30 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4120 +/- ##
==========================================
- Coverage 88.72% 88.68% -0.04% 
==========================================
Files 177 180 +3 Lines 133404 135148 +1744 Branches 133404 135148 +1744 ==========================================
+ Hits 118365 119860 +1495 - Misses 12325 12523 +198 - Partials 2714 2765 +51 

Flag	Coverage Δ
fuzzing	21.79% <33.75%> (+0.05%)	⬆️
tests	88.52% <88.14%> (-0.04%)	⬇️

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]

Does it matter if we received their signatures if we haven't sent ours? Or is it because if they sent theirs then they would not have reset their state?

[wpaulino]

You're supposed to remember the channel as soon as one side has sent tx_signatures, because you can't be sure the other side didn't just sign and broadcast without sending their own.

[ldk-reviews-bot]

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

[jkczyz]

Looks like we aren't calling fail_interactive_tx_negotiation when failing to handle a splice_ack. #4077 updates fail_interactive_tx_negotiation to take a NegotiationError, which it uses to construct a SpliceFundingFailed struct.

For FundingNegotiation::ConstructingTransaction, the NegotiationError is formed by copying the inputs from the InteractiveTxConstructor. For FundingNegotiation::AwaitingAck when handling splice_ack, we'd need to do something similar?

Either way we are cloning the inputs just to later take the FundingNegotiation here. Maybe that is ok for now? Any thoughts on a better way of doing this?

[jkczyz]

Hmmm... so we are also already take'ing the FundingNegotiation in one place in splice_ack handling (when FundingNegotiation::into_interactive_tx_constructor fails), but not earlier when validating the splice_ack message.

[wpaulino]

We shouldn't need to call fail_interactive_tx_negotiation whenever we fail handling a message by sending a warning and disconnecting.

[jkczyz]

Ah, so failing to process a splice_ack would just disconnect, and the peer could re-send it after reconnecting. Do we eventually timeout quiescence somewhere?

[wpaulino]

Quiescence is also implicitly terminated upon disconnection, but we do have a timeout enforced at should_disconnect_peer_awaiting_response.

[jkczyz]

Oh, so we'd need to produce a SpliceFailed event in some other way? I'm thinking when we are in FundingNegotiation::AwaitingAck and fail processing the splice_ack.

[wpaulino]

Yeah sounds like that logic might need to live in the disconnection handler, since we'll need to emit an event anyway if a peer disconnects mid-splice negotiation for whatever reason.

[wpaulino]

An interactive funding construction can be considered failed upon a disconnect or a tx_abort message. So far, we've consumed the InteractiveTxConstructor in the latter case, but not the former. Additionally, we may have splice-specific state that needs to be consumed as well to allow us to negotiate another splice later on.

This commit ensures that we properly consume all splice and interactive funding state whenever possible upon a disconnect or tx_abort.

The interactive funding state is safe to consume as long as we have either yet to reach AwaitingSignatures, or we have but tx_signatures has not been sent/received.

The splice state is safe to consume as long as we don't have a pending FundingNegotiation::AwaitingSignatures with a tx_signatures sent/received and we don't have any negotiated candidates. Note that until splice RBF is supported, it is not currently possible to have any negotiated candidates with a pending interactive funding transaction.

[ldk-reviews-bot]

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

[codecov]

Codecov Report

❌ Patch coverage is 88.14815% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.68%. Comparing base (3e21ba3) to head (6d2b110).
⚠️ Report is 82 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	60.00%	30 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4120 +/- ##
==========================================
- Coverage 88.72% 88.68% -0.04% 
==========================================
Files 177 180 +3 Lines 133404 135148 +1744 Branches 133404 135148 +1744 ==========================================
+ Hits 118365 119860 +1495 - Misses 12325 12523 +198 - Partials 2714 2765 +51 

Flag	Coverage Δ
fuzzing	21.79% <33.75%> (+0.05%)	⬆️
tests	88.52% <88.14%> (-0.04%)	⬇️

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]

Does it matter if we received their signatures if we haven't sent ours? Or is it because if they sent theirs then they would not have reset their state?

[wpaulino]

You're supposed to remember the channel as soon as one side has sent tx_signatures, because you can't be sure the other side didn't just sign and broadcast without sending their own.

[ldk-reviews-bot]

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

[jkczyz]

Looks like we aren't calling fail_interactive_tx_negotiation when failing to handle a splice_ack. #4077 updates fail_interactive_tx_negotiation to take a NegotiationError, which it uses to construct a SpliceFundingFailed struct.

For FundingNegotiation::ConstructingTransaction, the NegotiationError is formed by copying the inputs from the InteractiveTxConstructor. For FundingNegotiation::AwaitingAck when handling splice_ack, we'd need to do something similar?

Either way we are cloning the inputs just to later take the FundingNegotiation here. Maybe that is ok for now? Any thoughts on a better way of doing this?

[jkczyz]

Hmmm... so we are also already take'ing the FundingNegotiation in one place in splice_ack handling (when FundingNegotiation::into_interactive_tx_constructor fails), but not earlier when validating the splice_ack message.

[wpaulino]

We shouldn't need to call fail_interactive_tx_negotiation whenever we fail handling a message by sending a warning and disconnecting.

[jkczyz]

Ah, so failing to process a splice_ack would just disconnect, and the peer could re-send it after reconnecting. Do we eventually timeout quiescence somewhere?

[wpaulino]

Quiescence is also implicitly terminated upon disconnection, but we do have a timeout enforced at should_disconnect_peer_awaiting_response.

[jkczyz]

Oh, so we'd need to produce a SpliceFailed event in some other way? I'm thinking when we are in FundingNegotiation::AwaitingAck and fail processing the splice_ack.

[wpaulino]

Yeah sounds like that logic might need to live in the disconnection handler, since we'll need to emit an event anyway if a peer disconnects mid-splice negotiation for whatever reason.

[wpaulino]

An interactive funding construction can be considered failed upon a disconnect or a tx_abort message. So far, we've consumed the InteractiveTxConstructor in the latter case, but not the former. Additionally, we may have splice-specific state that needs to be consumed as well to allow us to negotiate another splice later on.

This commit ensures that we properly consume all splice and interactive funding state whenever possible upon a disconnect or tx_abort.

The interactive funding state is safe to consume as long as we have either yet to reach AwaitingSignatures, or we have but tx_signatures has not been sent/received.

The splice state is safe to consume as long as we don't have a pending FundingNegotiation::AwaitingSignatures with a tx_signatures sent/received and we don't have any negotiated candidates. Note that until splice RBF is supported, it is not currently possible to have any negotiated candidates with a pending interactive funding transaction.

[ldk-reviews-bot]

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

[codecov]

Codecov Report

❌ Patch coverage is 88.14815% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.68%. Comparing base (3e21ba3) to head (6d2b110).
⚠️ Report is 82 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	60.00%	30 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4120 +/- ##
==========================================
- Coverage 88.72% 88.68% -0.04% 
==========================================
Files 177 180 +3 Lines 133404 135148 +1744 Branches 133404 135148 +1744 ==========================================
+ Hits 118365 119860 +1495 - Misses 12325 12523 +198 - Partials 2714 2765 +51 

Flag	Coverage Δ
fuzzing	21.79% <33.75%> (+0.05%)	⬆️
tests	88.52% <88.14%> (-0.04%)	⬇️

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]

Does it matter if we received their signatures if we haven't sent ours? Or is it because if they sent theirs then they would not have reset their state?

[wpaulino]

You're supposed to remember the channel as soon as one side has sent tx_signatures, because you can't be sure the other side didn't just sign and broadcast without sending their own.

[ldk-reviews-bot]

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

[jkczyz]

Looks like we aren't calling fail_interactive_tx_negotiation when failing to handle a splice_ack. #4077 updates fail_interactive_tx_negotiation to take a NegotiationError, which it uses to construct a SpliceFundingFailed struct.

For FundingNegotiation::ConstructingTransaction, the NegotiationError is formed by copying the inputs from the InteractiveTxConstructor. For FundingNegotiation::AwaitingAck when handling splice_ack, we'd need to do something similar?

Either way we are cloning the inputs just to later take the FundingNegotiation here. Maybe that is ok for now? Any thoughts on a better way of doing this?

[jkczyz]

Hmmm... so we are also already take'ing the FundingNegotiation in one place in splice_ack handling (when FundingNegotiation::into_interactive_tx_constructor fails), but not earlier when validating the splice_ack message.

[wpaulino]

We shouldn't need to call fail_interactive_tx_negotiation whenever we fail handling a message by sending a warning and disconnecting.

[jkczyz]

Ah, so failing to process a splice_ack would just disconnect, and the peer could re-send it after reconnecting. Do we eventually timeout quiescence somewhere?

[wpaulino]

Quiescence is also implicitly terminated upon disconnection, but we do have a timeout enforced at should_disconnect_peer_awaiting_response.

[jkczyz]

Oh, so we'd need to produce a SpliceFailed event in some other way? I'm thinking when we are in FundingNegotiation::AwaitingAck and fail processing the splice_ack.

[wpaulino]

Yeah sounds like that logic might need to live in the disconnection handler, since we'll need to emit an event anyway if a peer disconnects mid-splice negotiation for whatever reason.

[wpaulino]

An interactive funding construction can be considered failed upon a disconnect or a tx_abort message. So far, we've consumed the InteractiveTxConstructor in the latter case, but not the former. Additionally, we may have splice-specific state that needs to be consumed as well to allow us to negotiate another splice later on.

This commit ensures that we properly consume all splice and interactive funding state whenever possible upon a disconnect or tx_abort.

The interactive funding state is safe to consume as long as we have either yet to reach AwaitingSignatures, or we have but tx_signatures has not been sent/received.

The splice state is safe to consume as long as we don't have a pending FundingNegotiation::AwaitingSignatures with a tx_signatures sent/received and we don't have any negotiated candidates. Note that until splice RBF is supported, it is not currently possible to have any negotiated candidates with a pending interactive funding transaction.

[ldk-reviews-bot]

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

[codecov]

Codecov Report

❌ Patch coverage is 88.14815% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.68%. Comparing base (3e21ba3) to head (6d2b110).
⚠️ Report is 82 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	60.00%	30 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4120 +/- ##
==========================================
- Coverage 88.72% 88.68% -0.04% 
==========================================
Files 177 180 +3 Lines 133404 135148 +1744 Branches 133404 135148 +1744 ==========================================
+ Hits 118365 119860 +1495 - Misses 12325 12523 +198 - Partials 2714 2765 +51 

Flag	Coverage Δ
fuzzing	21.79% <33.75%> (+0.05%)	⬆️
tests	88.52% <88.14%> (-0.04%)	⬇️

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]

Does it matter if we received their signatures if we haven't sent ours? Or is it because if they sent theirs then they would not have reset their state?

[wpaulino]

You're supposed to remember the channel as soon as one side has sent tx_signatures, because you can't be sure the other side didn't just sign and broadcast without sending their own.

[ldk-reviews-bot]

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

[jkczyz]

Looks like we aren't calling fail_interactive_tx_negotiation when failing to handle a splice_ack. #4077 updates fail_interactive_tx_negotiation to take a NegotiationError, which it uses to construct a SpliceFundingFailed struct.

For FundingNegotiation::ConstructingTransaction, the NegotiationError is formed by copying the inputs from the InteractiveTxConstructor. For FundingNegotiation::AwaitingAck when handling splice_ack, we'd need to do something similar?

Either way we are cloning the inputs just to later take the FundingNegotiation here. Maybe that is ok for now? Any thoughts on a better way of doing this?

[jkczyz]

Hmmm... so we are also already take'ing the FundingNegotiation in one place in splice_ack handling (when FundingNegotiation::into_interactive_tx_constructor fails), but not earlier when validating the splice_ack message.

[wpaulino]

We shouldn't need to call fail_interactive_tx_negotiation whenever we fail handling a message by sending a warning and disconnecting.

[jkczyz]

Ah, so failing to process a splice_ack would just disconnect, and the peer could re-send it after reconnecting. Do we eventually timeout quiescence somewhere?

[wpaulino]

Quiescence is also implicitly terminated upon disconnection, but we do have a timeout enforced at should_disconnect_peer_awaiting_response.

[jkczyz]

Oh, so we'd need to produce a SpliceFailed event in some other way? I'm thinking when we are in FundingNegotiation::AwaitingAck and fail processing the splice_ack.

[wpaulino]

Yeah sounds like that logic might need to live in the disconnection handler, since we'll need to emit an event anyway if a peer disconnects mid-splice negotiation for whatever reason.