Follow-ups to PR 3137 #3423

dunxen · 2024-11-25T13:18:18Z

Closes #3416.

jkczyz

Could you expand upon the commit messages? They should include both the what and the why (when not obvious) without relying in Github PRs or issues.

jkczyz · 2024-11-25T20:36:20Z

lightning/src/ln/channelmanager.rs

+				let channel_phase = chan_phase_entry.get_mut();
+				match channel_phase {
+					ChannelPhase::UnfundedOutboundV2(chan) => {
+						if chan.interactive_tx_signing_session.is_some() {
+							let (channel_id, mut channel_phase) = chan_phase_entry.remove_entry();
+							match channel_phase {
+								ChannelPhase::UnfundedOutboundV2(chan) => {
+									(channel_id, chan.into_channel())
+								}
+								_ => {
+									debug_assert!(false, "The channel phase was not UnfundedOutboundV2");
+									let err = ChannelError::close(
+										"Closing due to unexpected sender error".into());
+									return Err(convert_chan_phase_err!(self, peer_state, err, &mut channel_phase,
+										&channel_id).1)
+								}
+							}
 						} else {
-							let logger = WithChannelContext::from(&self.logger, &chan.context, None);
-							log_error!(logger, "Persisting initial ChannelMonitor failed, implying the funding outpoint was duplicated");
-							try_chan_phase_entry!(self, peer_state, Err(ChannelError::Close(
-								(
-									"Channel funding outpoint was a duplicate".to_owned(),
-									ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) },
-								)
-							)), chan_phase_entry)
+							return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
+								"Got a commitment_signed message for a V2 channel before funding transaction constructed!".into())), chan_phase_entry);
 						}
-					} else {
+					},
+					ChannelPhase::UnfundedInboundV2(chan) => {
+						// TODO(dual_funding): This should be somewhat DRYable when #3418 is merged.
+						if chan.interactive_tx_signing_session.is_some() {
+							let (channel_id, mut channel_phase) = chan_phase_entry.remove_entry();
+							match channel_phase {
+								ChannelPhase::UnfundedInboundV2(chan) => {
+									(channel_id, chan.into_channel())
+								}
+								_ => {
+									debug_assert!(false, "The channel phase was not UnfundedInboundV2");
+									let err = ChannelError::close(
+										"Closing due to unexpected sender error".into());
+									return Err(convert_chan_phase_err!(self, peer_state, err, &mut channel_phase,
+										&channel_id).1)
+								}
+							}
+						} else {
+							return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
+								"Got a commitment_signed message for a V2 channel before funding transaction constructed!".into())), chan_phase_entry);
+						}
+					},


Is it the case that only one of interactive_tx_signing_session or interactive_tx_constructor will be Some at a given time?

More and more it seems having phases for FundingTxConstruction and FundingTxSigning (with a property for inbound/outbound) may be more appropriate than phases for inbound/outbound. Probably not worth changing here, but we may want to consider it for post #3418.

Oh, I see we use interactive_tx_signing_session in the Funded phase but only for v2 channels. Would moving to the construction suggested above prevent needing interactive_tx_signing_session in Channel? That seems ideal given it isn't applicable for v1 channels.

@TheBlueMatt Do you see a problem with that? IIUC, we would transition to the Funded phase later for v2 channels. I'm not sure what implications that may have for #3137 (comment) and #3137 (comment). It seems we would need to persist both in the new FundingTxSigning phase and the Funded phase?

Also, just noticing when we read a Channel we unconditionally set interactive_tx_signing_session to None when reading. Won't this be a problem for v2 channels as currently written?

Yes that could prevent interactive_tx_signing_sesion from appearing in Channel. I'll need to look at the splicing work, but it seems we could reuse that phase for a new splice and for interactive RBF attempts.

Regarding, signing session persistence, yes that's needed in this PR... I believe we at least persist the funding_tx already, but I'll look at what else might need persistence and do that in a separate commit.

I think we can already drop the signing session after this PR because we move to Funded later.

We still rely on the signing session when receiving a remote's tx_signatures (e.g. InteractiveTxSigningSession::received_tx_signatures which depends on ｀ConstructedTransaction::add_remote_witnesses, otherwise I'd just lift it up to Channel` but not sure that's the right place for it). The other issues in this PR depend on figuring out what to do about this.

Ah, yea, damn. Indeed, I agree with @jkczyz that redoing the phases to have some "ChannelFundedAwaitingSignatures" phase (and maybe not have an outbound/inbound distinction) makes sense, but certainly not for this PR.

lightning/src/ln/interactivetxs.rs

dunxen

Cleaning up commits and adding explanations, and then we can discuss ac3fdbf#r1857415099 further.

dunxen · 2024-11-27T10:27:05Z

lightning/src/ln/channelmanager.rs

+				let channel_phase = chan_phase_entry.get_mut();
+				match channel_phase {
+					ChannelPhase::UnfundedOutboundV2(chan) => {
+						if chan.interactive_tx_signing_session.is_some() {
+							let (channel_id, mut channel_phase) = chan_phase_entry.remove_entry();
+							match channel_phase {
+								ChannelPhase::UnfundedOutboundV2(chan) => {
+									(channel_id, chan.into_channel())
+								}
+								_ => {
+									debug_assert!(false, "The channel phase was not UnfundedOutboundV2");
+									let err = ChannelError::close(
+										"Closing due to unexpected sender error".into());
+									return Err(convert_chan_phase_err!(self, peer_state, err, &mut channel_phase,
+										&channel_id).1)
+								}
+							}
 						} else {
-							let logger = WithChannelContext::from(&self.logger, &chan.context, None);
-							log_error!(logger, "Persisting initial ChannelMonitor failed, implying the funding outpoint was duplicated");
-							try_chan_phase_entry!(self, peer_state, Err(ChannelError::Close(
-								(
-									"Channel funding outpoint was a duplicate".to_owned(),
-									ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) },
-								)
-							)), chan_phase_entry)
+							return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
+								"Got a commitment_signed message for a V2 channel before funding transaction constructed!".into())), chan_phase_entry);
 						}
-					} else {
+					},
+					ChannelPhase::UnfundedInboundV2(chan) => {
+						// TODO(dual_funding): This should be somewhat DRYable when #3418 is merged.
+						if chan.interactive_tx_signing_session.is_some() {
+							let (channel_id, mut channel_phase) = chan_phase_entry.remove_entry();
+							match channel_phase {
+								ChannelPhase::UnfundedInboundV2(chan) => {
+									(channel_id, chan.into_channel())
+								}
+								_ => {
+									debug_assert!(false, "The channel phase was not UnfundedInboundV2");
+									let err = ChannelError::close(
+										"Closing due to unexpected sender error".into());
+									return Err(convert_chan_phase_err!(self, peer_state, err, &mut channel_phase,
+										&channel_id).1)
+								}
+							}
+						} else {
+							return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
+								"Got a commitment_signed message for a V2 channel before funding transaction constructed!".into())), chan_phase_entry);
+						}
+					},


Yes that could prevent interactive_tx_signing_sesion from appearing in Channel. I'll need to look at the splicing work, but it seems we could reuse that phase for a new splice and for interactive RBF attempts.

Regarding, signing session persistence, yes that's needed in this PR... I believe we at least persist the funding_tx already, but I'll look at what else might need persistence and do that in a separate commit.

dunxen · 2024-11-27T12:32:16Z

Still needs:

A commit to generate next_funding_txid for channel re-establish on-the-fly and get rid of the explicit ChannelContext::next_funding_txid field (Remove next_funding_txid tlv from Channel read/write #3417 (review)).
Consensus on Follow-ups to PR 3137 #3423 (comment).
A commit to populate interactive_tx_signing_session on Channel read. May or may not need new persisted fields.

codecov · 2024-11-29T13:19:40Z

Codecov Report

Attention: Patch coverage is 66.21160% with 99 lines in your changes missing coverage. Please review.

Project coverage is 89.11%. Comparing base (2d2c542) to head (15be515).

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	54.21%	69 Missing and 7 partials ⚠️
lightning/src/ln/interactivetxs.rs	43.47%	13 Missing ⚠️
lightning/src/ln/channelmanager.rs	60.00%	4 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3423      +/-   ##
==========================================
+ Coverage   88.60%   89.11%   +0.50%     
==========================================
  Files         149      150       +1     
  Lines      116877   117307     +430     
  Branches   116877   117307     +430     
==========================================
+ Hits       103564   104541     +977     
+ Misses      10805    10201     -604     
- Partials     2508     2565      +57

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lightning/src/ln/channel.rs

lightning/src/ln/interactivetxs.rs

TheBlueMatt · 2024-12-02T16:19:45Z

lightning/src/ln/channelmanager.rs

+				let channel_phase = chan_phase_entry.get_mut();
+				match channel_phase {
+					ChannelPhase::UnfundedOutboundV2(chan) => {
+						if chan.interactive_tx_signing_session.is_some() {
+							let (channel_id, mut channel_phase) = chan_phase_entry.remove_entry();
+							match channel_phase {
+								ChannelPhase::UnfundedOutboundV2(chan) => {
+									(channel_id, chan.into_channel())
+								}
+								_ => {
+									debug_assert!(false, "The channel phase was not UnfundedOutboundV2");
+									let err = ChannelError::close(
+										"Closing due to unexpected sender error".into());
+									return Err(convert_chan_phase_err!(self, peer_state, err, &mut channel_phase,
+										&channel_id).1)
+								}
+							}
 						} else {
-							let logger = WithChannelContext::from(&self.logger, &chan.context, None);
-							log_error!(logger, "Persisting initial ChannelMonitor failed, implying the funding outpoint was duplicated");
-							try_chan_phase_entry!(self, peer_state, Err(ChannelError::Close(
-								(
-									"Channel funding outpoint was a duplicate".to_owned(),
-									ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) },
-								)
-							)), chan_phase_entry)
+							return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
+								"Got a commitment_signed message for a V2 channel before funding transaction constructed!".into())), chan_phase_entry);
 						}
-					} else {
+					},
+					ChannelPhase::UnfundedInboundV2(chan) => {
+						// TODO(dual_funding): This should be somewhat DRYable when #3418 is merged.
+						if chan.interactive_tx_signing_session.is_some() {
+							let (channel_id, mut channel_phase) = chan_phase_entry.remove_entry();
+							match channel_phase {
+								ChannelPhase::UnfundedInboundV2(chan) => {
+									(channel_id, chan.into_channel())
+								}
+								_ => {
+									debug_assert!(false, "The channel phase was not UnfundedInboundV2");
+									let err = ChannelError::close(
+										"Closing due to unexpected sender error".into());
+									return Err(convert_chan_phase_err!(self, peer_state, err, &mut channel_phase,
+										&channel_id).1)
+								}
+							}
+						} else {
+							return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
+								"Got a commitment_signed message for a V2 channel before funding transaction constructed!".into())), chan_phase_entry);
+						}
+					},


I think we can already drop the signing session after this PR because we move to Funded later.

TheBlueMatt · 2024-12-02T16:30:04Z

lightning/src/ln/channelmanager.rs

+			let mut occupied_entry = peer_state.channel_by_id.entry(channel_id).insert(ChannelPhase::Funded(chan));
+			let channel_phase_entry = occupied_entry.get_mut();
+			match channel_phase_entry {
+				ChannelPhase::Funded(chan) => { handle_new_monitor_update!(self, persist_state, peer_state_lock, peer_state,


While we can definitely transition to Funded later (and IMO probably should, for consistency and to enable us to remove the interactive_tx_signing_session from Channel as discussed above), once we start adding our own inputs, we have to have the ChannelMonitor on disk before we can send our funding signatures, so this probably needs to move earlier.

lightning/src/ln/channel.rs

TheBlueMatt · 2024-12-02T16:35:28Z

lightning/src/ln/channel.rs

+					(None, None, Some(msgs::TxAbort { channel_id: self.context.channel_id(), data: vec![] }))
+				}
+			} else {
+				// Counterparty set `next_funding_txid` at incorrect state.


Did they? It could be that we sent our tx_signatures but they didn't receive them, no?

TheBlueMatt · 2024-12-02T16:42:18Z

lightning/src/ln/channel.rs

@@ -9898,6 +9908,28 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
 		let mut next_holder_commitment_point_opt: Option<PublicKey> = None;
 		let mut is_manual_broadcast = None;

+		let interactive_tx_signing_session =  if matches!(channel_state, ChannelState::FundingNegotiated) {


Its weird to me to recreate an empty interactive_tx_signing_session on startup just to trick some later code into thinking we have one. That probably indicates that we need to stop looking for the presence of interactive_tx_signing_session to determine the state the channels in and instead look at the existing state (channel_state and the commitment numbers).

Yes this is icky. I feel like we might need an intermediate state which may only make sense for dual-funded channels. I will try to avoid introducing that as much as possible, though.

TheBlueMatt · 2024-12-05T17:56:15Z

How's progress here looking? As we move towards an 0.1-beta hopefully in a week we should really make sure to get this landed!

dunxen · 2024-12-05T19:22:38Z

How's progress here looking? As we move towards an 0.1-beta hopefully in a week we should really make sure to get this landed!

Yeah it's pretty important. I should be able to get the latest fixes up by Saturday. Hopefully Friday, though, so it can get a final look before next week.

TheBlueMatt · 2024-12-09T15:26:35Z

Needs rebase. I think Use a WithChannelContextin unfunded branch ininternal_shutdown`` kinda got addressed upstream.

dunxen · 2024-12-09T15:36:18Z

Needs rebase. I think Use a WithChannelContextin unfunded branch ininternal_shutdown`` kinda got addressed upstream.

~~Yeah it did, the commit should have been removed on the last rebase where I solved some conflicts but I guess there was a diff that made it stay.~~ -nevermind, this was just me forgetting to push again.

What are your thoughts here? We actually do need to keep the signing session around as its methods for adding signatures use the instance of ConstructedTransaction to figure out which signatures correspond to which witnesses (as they are in order but could be interleaved with the counterparty so serial id is still needed)

TheBlueMatt · 2024-12-09T16:05:55Z

What are your thoughts #3423 (comment)?

Ah, sorry missed that. IMO we should just leave this PR as-is then and consider it later.

TheBlueMatt

I'm very skeptical of the reestablish logic without any test coverage, honestly. I'm kinda wondering if we shouldn't try to cfg-flag the v2 establish again for 0.1 :/

lightning/src/ln/channel.rs

TheBlueMatt · 2024-12-09T17:41:15Z

lightning/src/ln/channelmanager.rs

 						if let Some(tx_signatures) = tx_signatures_opt {
 							peer_state.pending_msg_events.push(events::MessageSendEvent::SendTxSignatures {
 								node_id: *counterparty_node_id,
 								msg: tx_signatures,
 							});
 						}
-						if let Some(ref funding_tx) = funding_tx_opt {
+						if let Some(ref funding_tx) = chan.context.unbroadcasted_funding() {


Do we have test coverage for ensuring we don't broadcast before the initial monitor is done for v2? This looks somewhat brittle (cc #3411).

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

TheBlueMatt · 2024-12-09T20:17:24Z

lightning/src/ln/channel.rs

+							// if it has already received commitment_signed and it should sign first, as specified in the tx_signatures requirements:
+							if session.received_commitment_signed && session.holder_sends_tx_signatures_first {
+								// MUST send its tx_signatures for that funding transaction.
+								(commitment_update, session.holder_tx_signatures.clone(), None)


Sometimes we may not get here when we need to send them. If we sent our TxSignatures first, then received theirs we'll be in AwaitingChannelReady, even if they didn't receive our TxSignatures and we need to resend them. We handle the AwaitingChannelReady case separately above, so won't even get this far.

lightning/src/ln/interactivetxs.rs

TheBlueMatt · 2024-12-09T20:24:54Z

lightning/src/ln/channel.rs

+				channel_state.set_peer_disconnected();
+			} else if matches!(channel_state, ChannelState::FundingNegotiated) &&
+				self.context.holder_commitment_point.transaction_number() == INITIAL_COMMITMENT_NUMBER - 1 {
+				// This is a V2 session which has an active signing session


Can we get test coverage for this?

dunxen · 2025-02-21T21:07:10Z

The commits for the interactive signing channel state flags and reload tests will be in another PR. Just slimming this one down for review.

TheBlueMatt

Ugh, sorry, one more.

TheBlueMatt · 2025-02-24T23:56:05Z

lightning/src/ln/channel.rs

-				self.context.monitor_pending_tx_signatures = tx_signatures_opt;
-				return Ok((None, None));
+				self.context.monitor_pending_tx_signatures = holder_tx_signatures_opt;
+				return Ok(None);


Ugh, this looks wrong. We set monitor_pending_tx_signatures and then return early (dropping funding_tx_opt and not setting context.funding_transaction) but in the next hunk (in monitor_updating_restored) we gate the new code by if let Some(..) = &self.context.funding_transaction.

TheBlueMatt · 2025-02-24T23:58:22Z

lightning/src/ln/dual_funding_tests.rs

-		let events = nodes[1].node.get_and_clear_pending_events();
-		assert!(events.is_empty());
+	// The funding transaction should not have been broadcast before persisting initial monitor has
+	// been completed.


I mean it also helps that we haven't received tx_signatures yet :p. We should move "Complete the persistence of the monitor" to below the handle_tx_signatures and check that we don't broadcast after the handler but that we do after the mon persist.

Yeah, you're right in that we should test it like that, but currently that ordering won't work.

We want to test that the acceptor only broadcasts the transaction after the monitor persist. However, the according to the spec, in this case, the acceptor needs to send tx_signatures first as it contributes the lowest input value (zero).

The acceptor's ChannelManager will only queue a SendTxMessage after initial monitor persist completes, so it's a bit of a chicken and egg problem. Technically the opener would only send their signatures after they receive the acceptor's in this case.

We could test this nicely if the acceptor contributes inputs, and then later also cover the opener case and remove the dummy opener messages when we add the opener tests for #2302.

However, I do see a way we could test something like this now without introducing the "acceptor contributes" functionality or having any opener logic in this PR, but that involves allowing the opener in this case to (erroneously) send their tx_signatures first and then us just responding with ours in our implementation.

I'm not sure if that's a dealbreaker really as it would just be for this test and the sender shouldn't actually screw up the order according to the spec.

Ah, okay, makes sense, yea we can leave figuring out how to test this to later.

dunxen · 2025-02-25T20:38:13Z

And did we want to remove the commit that removes the cfg flags?

TheBlueMatt · 2025-02-26T14:11:30Z

And did we want to remove the commit that removes the cfg flags?

Hmm, probably move it to the next PR if its not much hassle?

Also CI is quite sad.

dunxen · 2025-02-26T14:16:59Z

Also CI is quite sad.

Sigh, I need my own local push hooks of shame.

edit: nvm, just needed to rebase to see some changes to types.rs

This replaces the hard panic from this case.

Since we now have release branches, and dual-funded channels are expected in v0.2.0, we can remove the cfg flags. This should also speed up CI a bit as a bonus.

By moving the txid check to the start of `tx_signatures` handling, we can avoid spurious witness count mismatch errors. Also, it just makes more sense to check the txid earlier.

lightning/src/ln/channel.rs

jkczyz

Pretty much LGTM outside of a few minor concerns.

lightning/src/ln/channel.rs

jkczyz · 2025-02-26T21:41:52Z

lightning/src/ln/channel.rs

+			if funding_tx_opt.is_some() {
+				// We have a finalized funding transaction, so we can set the funding transaction and reset the
+				// signing session fields.
+				self.context.funding_transaction = funding_tx_opt;


Just noting that funding_transaction moves to FundingScope in #3604. There's a few other similar places, too, so we'll need to make sure to rebase there prior to merging.

Should this PR await 3604 and then rebase? Or do you mean rebasing 3604 once this PR is merged?

Ah, just saying whichever goes in second will need to rebase and resolve these before merging. We shouldn't hold up either one over the other, IMO.

jkczyz · 2025-02-26T21:54:10Z

lightning/src/ln/interactivetxs.rs

@@ -370,13 +370,8 @@ impl InteractiveTxSigningSession {
 			witnesses: witnesses.into_iter().collect(),


Unrelated to the PR, I think, but we don't need to allocate a new Vec here.

lightning/src/ln/channel.rs

We actually don't need to check if the counterparty had already sent their `tx_signatures` in `InteractiveTxSigningSession::received_tx_signatures`. Further, we can get rid of the clone of `funding_tx_opt` in `FundedChannel::tx_signatures` when setting the `ChannelContext::funding_transaction` as we don't actually need to propagate it through to `ChannelManager::internal_tx_complete` as we can use `ChannelContext::unbroadcasted_funding()` which clones the `ChannelContext::funding_transaction` anyway.

In a following commit we change the state at which a V2 channel is promoted to a `Channel` (after monitor persistence) to avoid a crash upon reading a `Channel` with no corresponding monitor persisted. This means that we will have no need for an optional (based on signing order) `TxSignatures` being returned when `provide_holder_witnesses` is called. The above is also the reason for removing the `received_commitment_signed` and signing order checks in the body of `provide_holder_witnesses`. These checks will only be important when we actually contribute inputs. Currently, we don't so we always send `tx_signatures` first in accordance with the Interactive Transaction Construction specification: https://github.com/lightning/bolts/blob/aa5207aea/02-peer-protocol.md?plain=1#L404

We don't yet support contibuting inputs to V2 channels, so we need to debug_assert and return an error if our signing session somehow has local inputs. We also return an error if for some mystical reason, in the no input contributions case, the input count does not equal the witness count of zero.

… receipt Before this commit, unfunded V2 channels were promoted to `FundedChannel`s in `PendingV2Channel::funding_tx_constructed`. Since a monitor is only created upon receipt of an initial `commitment_signed`, this would cause a crash if the channel was read from persisted data between those two events. Consequently, we also need to hold an `interactive_tx_signing_session` for both of our unfunded V2 channel structs.

jkczyz · 2025-02-27T16:16:59Z

lightning/src/ln/channel.rs

+					holder_commitment_point,
+					is_v2_established: true,
+				};
+				let res = funded_channel.commitment_signed_initial_v2(msg, best_block, signer_provider, logger)


Probably no need to hold up this PR, but is there a reason why commitment_signed_initial_v2 is a method on FundedChannel rather than PendingV2Channel? Seems that would be consistent with v1 channels calling initial_commitment_signed from an unfunded phase, only transitioning to FundedChannel after it returns.

jkczyz

Happy to land this now and leave the remaining comments for a follow-up.

TheBlueMatt

GOnna go ahead and land this but will open a followup for the second comment.

TheBlueMatt · 2025-02-27T19:12:22Z

lightning/src/ln/channel.rs

@@ -2242,15 +2242,19 @@ impl<SP: Deref> PendingV2Channel<SP> where SP::Target: SignerProvider {
 			},
 		};

-		let funding_ready_for_sig_event = None;
-		if signing_session.local_inputs_count() == 0 {
+		let funding_ready_for_sig_event = if signing_session.local_inputs_count() == 0 {


funding_ready_for_sig_event is always None, though it was already too.

Yeah it only makes sense for contributions. So will happen next.

lightning/src/ln/channel.rs

jkczyz requested review from TheBlueMatt and jkczyz November 25, 2024 14:11

dunxen added this to the 0.1 milestone Nov 25, 2024

dunxen added the Dual-funding label Nov 25, 2024

jkczyz mentioned this pull request Nov 25, 2024

Support async signing for V2 channel establishment #3411

Open

jkczyz reviewed Nov 25, 2024

View reviewed changes

dunxen mentioned this pull request Nov 26, 2024

Remove next_funding_txid tlv from Channel read/write #3417

Merged

dunxen marked this pull request as draft November 26, 2024 06:26

dunxen commented Nov 27, 2024

View reviewed changes

dunxen force-pushed the 2024-11-PR3137-followups branch from ac3fdbf to 0707e53 Compare November 27, 2024 12:27

dunxen mentioned this pull request Nov 27, 2024

Implement accepting dual-funded channels without contributing #3137

Merged

dunxen force-pushed the 2024-11-PR3137-followups branch 5 times, most recently from e29bc7d to 26fb59f Compare November 29, 2024 13:11

TheBlueMatt reviewed Dec 2, 2024

View reviewed changes

dunxen force-pushed the 2024-11-PR3137-followups branch 2 times, most recently from 8f5d100 to 38e43b2 Compare December 6, 2024 19:08

dunxen force-pushed the 2024-11-PR3137-followups branch from 38e43b2 to b729f2c Compare December 9, 2024 16:01

dunxen marked this pull request as ready for review December 9, 2024 16:07

TheBlueMatt reviewed Dec 9, 2024

View reviewed changes

jkczyz mentioned this pull request Dec 11, 2024

Dual funding extension, needed also for splicing: begin_interactive_funding_tx_construction #3443

Open

dunxen mentioned this pull request Feb 19, 2025

Can we merge Channel::tx_complete and Channel::funding_tx_constructed? #3609

Open

dunxen force-pushed the 2024-11-PR3137-followups branch from 15be515 to ba29649 Compare February 21, 2025 21:05

dunxen requested review from TheBlueMatt and jkczyz February 21, 2025 21:07

TheBlueMatt reviewed Feb 24, 2025

View reviewed changes

dunxen force-pushed the 2024-11-PR3137-followups branch from ba29649 to b9c8917 Compare February 26, 2025 13:46

dunxen added 3 commits February 26, 2025 17:50

Use debug_assert & error on unexpected initial commitment_signed

7fc2cf4

This replaces the hard panic from this case.

Remove dual_funding cfg-flags

ac060f9

Since we now have release branches, and dual-funded channels are expected in v0.2.0, we can remove the cfg flags. This should also speed up CI a bit as a bonus.

Move txid check to start of tx_signatures handling

eb18913

By moving the txid check to the start of `tx_signatures` handling, we can avoid spurious witness count mismatch errors. Also, it just makes more sense to check the txid earlier.

dunxen force-pushed the 2024-11-PR3137-followups branch 2 times, most recently from d8c6bd9 to 2630da8 Compare February 26, 2025 18:55

TheBlueMatt previously approved these changes Feb 26, 2025

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

jkczyz reviewed Feb 26, 2025

View reviewed changes

dunxen added 4 commits February 27, 2025 11:59

dunxen dismissed TheBlueMatt’s stale review via 9f9a7a9 February 27, 2025 10:38

dunxen force-pushed the 2024-11-PR3137-followups branch from 2630da8 to 9f9a7a9 Compare February 27, 2025 10:38

dunxen requested review from jkczyz and TheBlueMatt February 27, 2025 14:41

jkczyz reviewed Feb 27, 2025

View reviewed changes

jkczyz approved these changes Feb 27, 2025

View reviewed changes

TheBlueMatt approved these changes Feb 27, 2025

View reviewed changes

TheBlueMatt merged commit eaeed77 into lightningdevkit:main Feb 27, 2025
22 of 26 checks passed

dunxen mentioned this pull request Mar 3, 2025

Introduce interactive signing state flags for funded states. #3637

Open

		@@ -370,13 +370,8 @@ impl InteractiveTxSigningSession {
		witnesses: witnesses.into_iter().collect(),

Follow-ups to PR 3137 #3423

Follow-ups to PR 3137 #3423

Conversation

dunxen commented Nov 25, 2024

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunxen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunxen commented Nov 27, 2024 • edited Loading

codecov bot commented Nov 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Dec 5, 2024

dunxen commented Dec 5, 2024 • edited Loading

TheBlueMatt commented Dec 9, 2024 • edited Loading

dunxen commented Dec 9, 2024 • edited Loading

TheBlueMatt commented Dec 9, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunxen commented Feb 21, 2025 • edited Loading

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunxen commented Feb 25, 2025

TheBlueMatt commented Feb 26, 2025

dunxen commented Feb 26, 2025 • edited Loading

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunxen commented Nov 27, 2024 •

edited

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading

dunxen commented Dec 5, 2024 •

edited

Loading

TheBlueMatt commented Dec 9, 2024 •

edited

Loading

dunxen commented Dec 9, 2024 •

edited

Loading

dunxen commented Feb 21, 2025 •

edited

Loading

dunxen commented Feb 26, 2025 •

edited

Loading