rust-lightning icon indicating copy to clipboard operation
rust-lightning copied to clipboard

Introduce interactive signing state flags for funded states.

Open dunxen opened this issue 10 months ago • 32 comments

This PR includes some deferred follow-ups extracted from #3423 and introduces new state flags to track interactive signing along with persistence of the minimum information needed from a signing session to reconstruct it.

A top-level state flag was avoided so that this work is compatible with splicing as well as V2 channel establishment (dual-funding).

dunxen avatar Mar 03 '25 18:03 dunxen

👋 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 avatar Mar 03 '25 18:03 ldk-reviews-bot

Did you want to include test coverage for restarts here?

Not yet. Tracked in #3636. Will need to be able to contribute inputs first to test a useful order of message exchange + restart.

dunxen avatar Mar 04 '25 15:03 dunxen

Codecov Report

Attention: Patch coverage is 64.55026% with 67 lines in your changes missing coverage. Please review.

Project coverage is 90.01%. Comparing base (89f5217) to head (fb00cb6). Report is 19 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/channel.rs 59.55% 49 Missing and 6 partials :warning:
lightning/src/ln/interactivetxs.rs 46.66% 8 Missing :warning:
lightning/src/ln/channelmanager.rs 90.90% 1 Missing and 1 partial :warning:
lightning/src/ln/dual_funding_tests.rs 87.50% 1 Missing :warning:
lightning/src/util/ser.rs 85.71% 0 Missing and 1 partial :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3637      +/-   ##
==========================================
+ Coverage   89.37%   90.01%   +0.63%     
==========================================
  Files         157      157              
  Lines      124095   129376    +5281     
  Branches   124095   129376    +5281     
==========================================
+ Hits       110915   116452    +5537     
+ Misses      10469    10187     -282     
- Partials     2711     2737      +26     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Mar 05 '25 10:03 codecov[bot]

@dunxen re-request when this is ready for review again, feel free to squash as well

wpaulino avatar Mar 05 '25 21:03 wpaulino

🔔 1st Reminder

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

ldk-reviews-bot avatar Mar 06 '25 12:03 ldk-reviews-bot

Taking myself off since @wpaulino and @jkczyz are on this one. Aside from my first comment I don't have any more high-level feedback.

TheBlueMatt avatar Mar 06 '25 23:03 TheBlueMatt

🔔 1st Reminder

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

ldk-reviews-bot avatar Mar 15 '25 14:03 ldk-reviews-bot

🔔 2nd Reminder

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

ldk-reviews-bot avatar Mar 17 '25 14:03 ldk-reviews-bot

Just pushed the change for full persistence of InteractiveTxConstructor. Still addressing other feedback.

dunxen avatar Mar 19 '25 10:03 dunxen

@dunxen let us know when this is ready for another round

wpaulino avatar Mar 24 '25 17:03 wpaulino

@wpaulino, @jkczyz, sorry for the delay. This is ready for another round.

dunxen avatar Mar 27 '25 12:03 dunxen

🔔 1st Reminder

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

ldk-reviews-bot avatar Mar 29 '25 12:03 ldk-reviews-bot

🔔 2nd Reminder

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

ldk-reviews-bot avatar Mar 31 '25 12:03 ldk-reviews-bot

🔔 1st Reminder

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

ldk-reviews-bot avatar Apr 05 '25 11:04 ldk-reviews-bot

🔔 1st Reminder

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

ldk-reviews-bot avatar Apr 05 '25 11:04 ldk-reviews-bot

🔔 2nd Reminder

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

ldk-reviews-bot avatar Apr 07 '25 11:04 ldk-reviews-bot

🔔 2nd Reminder

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

ldk-reviews-bot avatar Apr 07 '25 11:04 ldk-reviews-bot

Looks like there's some breakages in the full_stack fuzz test.

Thanks, will address those. Honestly haven't paid much attention as fuzz had been broken for a while on main I believe.

dunxen avatar Apr 09 '25 08:04 dunxen

Hmm... when possible please avoid rebasing unless necessary. Otherwise, it makes it difficult to view new changes related to the PR. If needed, doing two separate pushes would be ideal.

jkczyz avatar Apr 09 '25 13:04 jkczyz

Thanks, will address those. Honestly haven't paid much attention as fuzz had been broken for a while on main I believe.

Locally, it still should work. CI has just been timing out (unfortunately) but will catch actual failures, AFAICT.

jkczyz avatar Apr 09 '25 13:04 jkczyz

Locally, it still should work. CI has just been timing out (unfortunately) but will catch actual failures, AFAICT.

Oh yeah I see this was just related to the new bool in config.

dunxen avatar Apr 09 '25 13:04 dunxen

Hmm... when possible please avoid rebasing unless necessary.

Sorry, bad habit I do sometimes. I'll keep it in mind again.

dunxen avatar Apr 09 '25 13:04 dunxen

Changes:

git diff-tree -U1 4f6192f 21a25fb3f
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b43e47e68..a60f3f293 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -5849,4 +5849,3 @@ impl<SP: Deref> FundedChannel<SP> where

-               let need_channel_ready = self.check_get_channel_ready(0, logger).is_some();
-               self.monitor_updating_paused(false, false, need_channel_ready, Vec::new(), Vec::new(), Vec::new());
+               self.monitor_updating_paused(false, false, false, Vec::new(), Vec::new(), Vec::new());

@@ -6496,3 +6495,3 @@ impl<SP: Deref> FundedChannel<SP> where
                if !matches!(self.context.channel_state, ChannelState::FundingNegotiated(flags) if flags.is_interactive_signing()) {
-                       return Err(ChannelError::close("Received tx_signatures in strange state!".to_owned()));
+                       return Err(ChannelError::Ignore("Ignoring tx_signatures received outside of interactive signing".to_owned()));
                }

dunxen avatar Apr 11 '25 06:04 dunxen

🔔 1st Reminder

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

ldk-reviews-bot avatar Apr 16 '25 10:04 ldk-reviews-bot

Changes:

git diff-tree -U1 7d19a76 21d3c78
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index ecec47387..93dd16e7d 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -11989,3 +11989,3 @@ where
        fn handle_open_channel_v2(&self, counterparty_node_id: PublicKey, msg: &msgs::OpenChannelV2) {
-               if !self.node_features().supports_dual_fund() {
+               if !self.init_features().supports_dual_fund() {
                        let _: Result<(), _> = handle_error!(self, Err(MsgHandleErrInternal::send_err_msg_no_close(

dunxen avatar Apr 16 '25 13:04 dunxen

🔔 2nd Reminder

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

ldk-reviews-bot avatar Apr 19 '25 00:04 ldk-reviews-bot

🔔 3rd Reminder

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

ldk-reviews-bot avatar Apr 21 '25 00:04 ldk-reviews-bot

🔔 4th Reminder

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

ldk-reviews-bot avatar Apr 23 '25 00:04 ldk-reviews-bot

🔔 5th Reminder

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

ldk-reviews-bot avatar Apr 26 '25 00:04 ldk-reviews-bot

🔔 6th Reminder

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

ldk-reviews-bot avatar Apr 28 '25 00:04 ldk-reviews-bot