Zero Conf Channels #1401

TheBlueMatt · 2022-04-01T21:51:30Z

🎉 not sure what else to say. See individual commits.

codecov-commenter · 2022-04-01T22:15:59Z

Codecov Report

Merging #1401 (ffaf9bc) into main (342698f) will increase coverage by 0.91%.
The diff coverage is 93.58%.

@@            Coverage Diff             @@
##             main    #1401      +/-   ##
==========================================
+ Coverage   90.89%   91.80%   +0.91%     
==========================================
  Files          76       80       +4     
  Lines       42075    50226    +8151     
  Branches    42075    50226    +8151     
==========================================
+ Hits        38244    46111    +7867     
- Misses       3831     4115     +284

Impacted Files	Coverage Δ
lightning/src/util/config.rs	`43.13% <0.00%> (-0.87%)`	⬇️
lightning/src/util/events.rs	`33.56% <ø> (ø)`
lightning/src/ln/channelmanager.rs	`84.71% <78.94%> (+0.05%)`	⬆️
lightning/src/ln/priv_short_conf_tests.rs	`97.09% <95.61%> (-0.87%)`	⬇️
lightning/src/ln/channel.rs	`88.59% <96.15%> (+0.01%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`96.77% <100.00%> (+1.22%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.34% <100.00%> (+1.16%)`	⬆️
lightning/src/routing/router.rs	`93.17% <100.00%> (+0.65%)`	⬆️
lightning/src/util/scid_utils.rs	`99.19% <100.00%> (ø)`
lightning/src/util/test_utils.rs	`78.15% <100.00%> (+0.18%)`	⬆️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 342698f...ffaf9bc. Read the comment docs.

TheBlueMatt · 2022-04-02T02:32:04Z

Note things not implemented that we may want eventually (will open followup issues after this makes some progress) - (a) always send a funding_locked for outbound channels immediately whether its 0conf or not, (b) allowing users to lock in an inbound channel before the min-depth even if they didn't follow the manual-acceptance flow (assuming the peer implements (a)).

ariard · 2022-04-04T00:04:51Z

lightning/src/util/config.rs

+	/// If this is set, we assume that our own funding transactions are *never* double-spent, and
+	/// thus we can trust them without any confirmations. This is generally a reasonable
+	/// assumption, given we're the only ones who could ever double-spend it (assuming we have sole
+	/// control of the signing keys).


Should we make the comment even more conservative and say "The funding transaction should not have inputs spendable by a non-trusted third-party" ? The notion of "sole control of the signing keys" can be quickly confusing as there could be an alternative script branch spendable. The "sole knowledge of any satisfying witness of the inputs" would be better imo.

Hmm, it already is very explicit - "given we're the only ones who could ever double-spend it". The comment about control of keys is just a parenthetical to make it concrete. I'm not sure its worth clarifying a clarification example.

You might have an output locked by a Taproot with 2 script path spends : you own key in the script path but not the ones in the second script path, or they're also locked with a timelock and you think it's safe but the timelock expires. I find the "sole knowledge of any satisfying witness of the inputs" a stronger requirement.

Its also much less readable for people not super deep in Bitcoin - most folks looking at LDK know what a private key is, but not all of them know what "any satisfying witness of the inputs" would mean, or at least would have to think hard about it.

ariard · 2022-04-04T00:05:58Z

lightning/src/util/config.rs

+	///
+	/// You may wish to un-set this if you allow the user to (or do in an automated fashion)
+	/// double-spend the funding transaction with an alternative channel open to RBF a channel
+	/// opening transaction.


If a 0conf user is willing to fee-bump, should we advice them to CPFP the funding transaction through a change output, if any ?

Sure, that seems implied, no? You can of course RBF a funding transaction by simply opening a new channel and closing the original, but that requires non-0-conf.

Hmmm you might be willing to use CPFP to get an instant usage effect that you loose with RBF as the replacement transaction invalids the commitment transactions.

I'm not sure what specifically you want changed here - it talks explicitly about RBF, implying that CPFP is still allowed, IMO.

ariard · 2022-04-04T00:27:05Z

lightning/src/ln/channelmanager.rs

+	///
+	/// [`Event::OpenChannelRequest`]: events::Event::OpenChannelRequest
+	/// [`Event::ChannelClosed::user_channel_id`]: events::Event::ChannelClosed::user_channel_id
+	pub fn accept_inbound_channel_from_trusted_peer_0conf(&self, temporary_channel_id: &[u8; 32], user_channel_id: u64) -> Result<(), APIError> {


I think one step to reduce trust peer with 0conf inbound channel would be to only authorize payment forwarding decreasing our own balance, then authorize payment reception once we have 1 conf.

If we think it's relevant, we could have another config boolean 0conf_authorize_forward_until_conf.

(iirc there is an issue somewhere we're already discussed this but can't find it rn..)

That feels like its somewhat useless, no? Unless you have push_msat you won't have any balance to forward out over.

Well, in that precise case where you've a substantial push_msat though dunno if we see any instant outbound liquidity channel in the wild.

Yea, I just don't think that's all that common...Unless we have reason to believe that will change I'm not sure we should really cater to it.

We're much better off letting users manually decide what to do with each HTLC for such cases, I think.

valentinewallace · 2022-04-26T16:02:40Z

lightning/src/util/config.rs

@@ -128,6 +128,23 @@ pub struct ChannelHandshakeLimits {
 	///
 	/// Default value: 144, or roughly one day and only applies to outbound channels.
 	pub max_minimum_depth: u32,
+	/// Whether we implicitly trust our own funding transactions to not be double-spent.


"our own funding transaction" = a transaction where we're the channel initiator?

Tried some tweaked wording.

lightning/src/ln/channel.rs

valentinewallace · 2022-04-26T18:04:11Z

lightning/src/ln/channel.rs

+		// (re-)broadcast the funding transaction as we may have declined to broadcast it when we
+		// first received the funding_signed.
+		let mut funding_broadcastable =
+			if self.is_outbound() && self.channel_state & !MULTI_STATE_FLAGS >= ChannelState::FundingSent as u32 {


This change almost seems like it belongs in a separate commit or be documented in the commit message, seemed a bit unrelated at first

Updated the commit message.

valentinewallace · 2022-04-26T18:28:53Z

lightning/src/util/config.rs

+	/// A lower-bound of 1 is applied, requiring all channels to have a confirmed commitment
+	/// transaction before operation. If you wish to accept channels with zero confirmations, see
+	/// [`UserConfig::manually_accept_inbound_channels`] and
+	/// [`ChannelManager::accept_inbound_channel`].


Should we link to accept_inbound_channel_from_trusted_peer_0conf? I think the API might be cleaner if we aggregate the two methods .. Also so weird to me that you can't initiate 0-conf channels, only accept them 🤔

This is "applied only for inbound channels" :)

As an improvement, perhaps the struct-level docs could be expanded to be on par with ChannelHandshakeLimits docs.

I'm not really sure what we could add to make it more detailed. Do you have a specific suggestion?

I guess not entirely applicable to the comment, but perhaps explaining why a config parameter would go in ChannelHandshakeConfig vs ChannelHandshakeLimits. The comments on the fields in the UserConfig make it clear, but ChannelHandshakeConfig's docs are super vague. Feel free to disregard as it is unrelated to this PR.

Yea, lets do that as a part of #1247/#216 instead of here - those intend to change the definition slightly.

valentinewallace · 2022-04-27T18:05:17Z

lightning/src/ln/channelmanager.rs

@@ -4370,6 +4422,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				if !channel.get().inbound_is_awaiting_accept() {
 					return Err(APIError::APIMisuseError { err: "The channel isn't currently awaiting to be accepted.".to_owned() });
 				}
+				if accept_0conf { channel.get_mut().set_0conf(); }


Curious why we don't check the channel features before setting it to 0-conf?

The 0conf feature is a "I DEMAND ZEROCONF" feature not "I must set this in order to use the channel at zeroconf". Originally we had no such feature at all, but lnd demanded it for reasons that are still somewhat unclear to me. You're welcome to send a funding_locked at 0 conf without the zeroconf feature bit being set.

lightning/src/ln/channelmanager.rs

valentinewallace · 2022-04-27T19:24:41Z

lightning/src/ln/channel.rs

+		let mut funding_broadcastable =
+			if self.is_outbound() && self.channel_state & !MULTI_STATE_FLAGS >= ChannelState::FundingSent as u32 {
+				self.funding_transaction.take()
+			} else { None };
+		// That said, if the funding transaction is already confirmed (ie we're active with a
+		// minimum_depth over 0, don't bother re-broadcasting the confirmed funding tx.
+		if self.channel_state & !MULTI_STATE_FLAGS >= ChannelState::ChannelFunded as u32 && self.minimum_depth != Some(0) {
+			funding_broadcastable = None;
+		}


These checks seem like they can be combined?

Actually it also addresses a subtle bug - if we confirmed the channel a week ago, we should be removing the funding_transaction field and no longer re-serializing it in the Channel.

lightning/src/ln/channel.rs

valentinewallace · 2022-04-29T17:37:36Z

Hmm, intuitively the 0-conf feature seems incomplete to me until we add the ability to call open_channel in a way that demands 0-conf, or something like that?

valentinewallace · 2022-04-29T18:07:42Z

Seems we'll currently reject inbound channels with option_zeroconf set, is this saved for follow-up or?

valentinewallace

Left some questions, but this looks good and the test coverage seems solid (tried commenting out different stuff, all caused failures). Would prefer to take a last look after another reviewer

lightning/src/ln/channel.rs

lightning/src/ln/priv_short_conf_tests.rs

lightning/src/ln/channelmanager.rs

valentinewallace · 2022-04-29T19:20:04Z

lightning/src/ln/channelmanager.rs

@@ -4908,7 +4965,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					break Ok((raa_updates.accepted_htlcs, raa_updates.failed_htlcs,
 							raa_updates.finalized_claimed_htlcs,
 							chan.get().get_short_channel_id()
-								.expect("RAA should only work on a short-id-available channel"),
+								.unwrap_or(chan.get().outbound_scid_alias()),


It looks like this could cause us to set short_channel_id in PaymentPathFailed to the alias (maybe elsewhere too), may want to document

I don't think this causes that? I added two comments on the fields I see getting copied from here, but I don't see how it ends up in PaymentPathFailed. That said, PaymentPathFailed can just copy the SCID from the first hop in the route into short_channel_id, which would indeed be an alias.

TheBlueMatt · 2022-05-02T04:37:36Z

Gonna let #1461 go first as it will fix #1401 (comment)

TheBlueMatt · 2022-05-04T02:35:09Z

Rebased after #1461, added a new test and addressed feedback.

TheBlueMatt · 2022-05-04T20:33:14Z

Hmm, intuitively the 0-conf feature seems incomplete to me until we add the ability to call open_channel in a way that demands 0-conf, or something like that?

I don't think there is ever a reason anyone should use the 0-conf channel type. It really just exists cause lnd demanded it really vocally, I'm really not at all sure why.

Seems we'll currently reject inbound channels with option_zeroconf set, is this saved for follow-up or?

Oh, good point lol, yea, we should support this. Maybe we can do this as a follow-up?

valentinewallace · 2022-05-04T22:41:15Z

Hmm, intuitively the 0-conf feature seems incomplete to me until we add the ability to call open_channel in a way that demands 0-conf, or something like that?

I don't think there is ever a reason anyone should use the 0-conf channel type. It really just exists cause lnd demanded it really vocally, I'm really not at all sure why.

I'm just wondering, couldn't there be a scenario where I want to open a channel but only if I can use it immediately? Is there any way to do that with the way this is implemented?

Seems we'll currently reject inbound channels with option_zeroconf set, is this saved for follow-up or?

Oh, good point lol, yea, we should support this. Maybe we can do this as a follow-up?

Good with a follow-up for this!

TheBlueMatt · 2022-05-04T22:47:59Z

I'm just wondering, couldn't there be a scenario where I want to open a channel but only if I can use it immediately? Is there any way to do that with the way this is implemented?

The feature doesn't really provide that, though, you're never going to just open a channel with someone and have them accept zeroconf, you're always going to do some kind of out-of-band communication with the peer before you go to open the channel, negotiating zeroconf and that the counterparty is willing to accept zeroconf from you specifically. Once you've done that, the feature doesn't really add anything...

jkczyz

High-level comment: not sure if I have the whole context around feature bits, but do we at least want to advertise that we support option_zeroconf?

lightning/src/ln/channel.rs

lightning/src/util/config.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/priv_short_conf_tests.rs

lightning/src/ln/channel.rs

jkczyz · 2022-05-13T04:57:43Z

lightning/src/ln/functional_tests.rs

-		let mut as_chan = a_channel_lock.by_id.get_mut(&open_chan_2_msg.temporary_channel_id).unwrap();
+		// Note that we drop the channel entirely here as its in a somewhat nonsense state (leaving
+		// the channelmanager in a possibly nonsense state instead).
+		let mut as_chan = a_channel_lock.by_id.remove(&open_chan_2_msg.temporary_channel_id).unwrap();


Not sure I understand what's happening here and how it relates the other changes in the commit.

It only tangentially has to do with the commit - previously we'd not even consider the funding transaction because we weren't in the right state, but now we do, and thus get into an even more nonsensical state with the changes in the commit. I updated the comment to be a bit more explicit.

jkczyz · 2022-05-13T05:03:14Z

lightning/src/ln/priv_short_conf_tests.rs

-	// should fly immediately and the channel should be available for use as soon as they are
-	// received.
+// Receiver must have been initialized with manually_accept_inbound_channels set to true.
+fn open_zero_conf_channel<'a, 'b, 'c, 'd>(initiator: &'a Node<'b, 'c, 'd>, receiver: &'a Node<'b, 'c, 'd>, initiator_config: Option<UserConfig>) -> bitcoin::Transaction {


Is this general enough to move to functional_test_utils?

I guess? Its not currently used outside of this file, and functional_test_utils is already 2300 LoC, so I'd kinda prefer to leave it here if possible.

I guess this file is already near 10k lines anyway lol

Hmm? priv_short_conf_tests is 1k lines?

Whoops, confused this for functional_tests 😅

lightning/src/ln/priv_short_conf_tests.rs

jkczyz · 2022-05-13T05:13:41Z

lightning/src/ln/priv_short_conf_tests.rs

+	// At this point the channel no longer has an SCID again. In the future we should likely
+	// support simply un-setting the SCID and waiting until the channel gets re-confirmed, but for
+	// now we force-close the channel here.
+	check_closed_event!(&nodes[0], 1, ClosureReason::ProcessingError {
+		err: "Funding transaction was un-confirmed. Locked at 0 confs, now have 0 confs.".to_owned()
+	});
+	check_closed_broadcast!(nodes[0], true);
+	check_closed_event!(&nodes[1], 1, ClosureReason::ProcessingError {
+		err: "Funding transaction was un-confirmed. Locked at 0 confs, now have 0 confs.".to_owned()
+	});


Would this be difficult to support? Don't need to do it here but probably could open an issue once merged.

TheBlueMatt · 2022-05-16T22:08:33Z

Rebased to address merge conflict, lmk if I should squash.

ariard

Still reviewing - would like to look at test coverage in depth at least.

ariard · 2022-05-17T00:29:34Z

lightning/src/util/config.rs

+	/// control of the signing keys).
+	///
+	/// You may wish to un-set this if you allow the user to (or do in an automated fashion)
+	/// double-spend the funding transaction to RBF with an alternative channel open.


In fact replace-by-fee is a node policy allowing to replace an original transaction with a newer one, implying a change in the txid and thus nullifying the first commitment transactions validity. However, this is only one fee-bumping technique destructive of the txid, you could imagine a (not) smart user relying on some out-of-band miners API for their fee-bumping, shrugging about our RBF warning as not understanding they're concerned.

"You may wish to un-set this if you allow the user a) to halt the channel open by double-spending the funding inputs or b) to fee-bump the channel open with a replacement transaction (e.g RBF)". Or another wording, but current one is weird as RBF implies a double-spend.

Yes I'm picky but as we're not covering the funding transaction, there are margins for the users to screw up there.

elying on some out-of-band miners API for their fee-bumping, shrugging about our RBF warning as not understanding they're concerned.

sure, but if they're doing that its totally safe to use 0conf? I'm not sure why we'd want to specify that? Or are you saying we'd also like to include the "cancel a channel open with an RBF" bit?

Well they can still propose a newer transaction through out-of-band miners API, that's just a communication channel. What I would like to underscore is that you have more ways to screw up your 0-conf fee-bumping than leveraging RBF like using ANYONECANPAY...so warning : Be sure of you know well what you're doing if you fee-bump !

Thinking more I'm not even sure we explain somewhere how to insert RBF in our funding flow, as we assume the funding_txid to be stable (e.g in transactions_confirmed) so a user would have to re-start from scratch the funding sequence and force-close the previous channel open (though you would have to wait at least 1 conf as a previous channel open might end up confirmed). Maybe to open an issue that we should document cleanly "how to implement RBF with our current API".

Well they can still propose a newer transaction through out-of-band miners API, that's just a communication channel.

Right, but that's still RBF? No matter how it ends up there its replace-by-fee, even if the fee is out-of-band. Would you prefer if I just remove the "to RBF..." part or say "e.g. to RBF"? That way its just a more general "double-spend".

Thinking more I'm not even sure we explain somewhere how to insert RBF in our funding flow,

Right, the only way to do funding with RBF is to literally open a new channel with a conflicting tx, and then force-close the old one once the rbf'd one confirms. Indeed, I don't think its documented anywhere, and its pretty gross.

ariard · 2022-05-17T01:47:16Z

lightning/src/ln/channelmanager.rs

+							// un-confirmed we force-close the channel, ensuring short_to_id is
+							// always consistent.
+							let scid_insert = short_to_id.insert(real_scid, channel.channel_id());
+							assert!(scid_insert.is_none() || scid_insert.unwrap() == channel.channel_id(),


Could be nice to call the variable name fake_scid in clear opposition to real_scid and stick to it as a convention. In that direction the error message could also be "fake SCID should never collide with real SCID"

Not sure what you want to rename fake_scid here? scid_insert is a result, not an SCID specifically.

ariard · 2022-05-17T02:01:23Z

lightning/src/util/config.rs

+	/// If this is set, we assume that our own funding transactions are *never* double-spent, and
+	/// thus we can trust them without any confirmations. This is generally a reasonable
+	/// assumption, given we're the only ones who could ever double-spend it (assuming we have sole
+	/// control of the signing keys).


You might have an output locked by a Taproot with 2 script path spends : you own key in the script path but not the ones in the second script path, or they're also locked with a timelock and you think it's safe but the timelock expires. I find the "sole knowledge of any satisfying witness of the inputs" a stronger requirement.

ariard · 2022-05-17T02:05:39Z

lightning/src/util/config.rs

+	///
+	/// You may wish to un-set this if you allow the user to (or do in an automated fashion)
+	/// double-spend the funding transaction with an alternative channel open to RBF a channel
+	/// opening transaction.


Hmmm you might be willing to use CPFP to get an instant usage effect that you loose with RBF as the replacement transaction invalids the commitment transactions.

ariard · 2022-05-17T02:07:46Z

lightning/src/ln/channelmanager.rs

+	///
+	/// [`Event::OpenChannelRequest`]: events::Event::OpenChannelRequest
+	/// [`Event::ChannelClosed::user_channel_id`]: events::Event::ChannelClosed::user_channel_id
+	pub fn accept_inbound_channel_from_trusted_peer_0conf(&self, temporary_channel_id: &[u8; 32], user_channel_id: u64) -> Result<(), APIError> {


Well, in that precise case where you've a substantial push_msat though dunno if we see any instant outbound liquidity channel in the wild.

wpaulino · 2022-05-18T00:08:39Z

lightning/src/ln/channel.rs

+		if peer_limits.trust_own_funding_0conf {
+			self.minimum_depth = Some(msg.minimum_depth);
+		} else {
+			self.minimum_depth = Some(cmp::max(1, msg.minimum_depth));


This would previously return an error when min_depth == 0, whereas now we'll enforce a minimum of one, so they may think we've agreed to proceed with zero confs once we send funding_created back.

Right, this is kinda a 0conf philosophy question. This PR doesn't actually support the 0conf channel type (as it was written before the 0conf channel type existed) which is the "I require 0conf" option, but we'll add that in a followup.

In general, if you're doing a 0conf channel, you've proably pre-negotiated it in some way - most likely the recipient of the channel is pre-configured to trust an LSP. There's arguably not a lot of value in negotiating 0conf in the open/accept channel flow - by the time you're there both sides of the channel should really already know if the channel should be 0conf or not.

More generally, nodes can always delay channel_ready/funding_locked as long as they want - the minimum_depth field is just a hint, and the protocol can't enforce it in any way. We're not strictly-speaking "out of spec" by delaying more than the channel recipient wanted.

lightning/src/ln/channelmanager.rs

lightning/src/ln/priv_short_conf_tests.rs

TheBlueMatt · 2022-05-19T16:54:45Z

Squashed fixups without other changes.

$ git diff-tree -U1 32df62aa 2269c409
$

ariard

See potential test coverage holes. Otherwise, SGTM, I don't think any comment is a blocker.

ariard · 2022-05-22T20:32:11Z

lightning/src/ln/channelmanager.rs

@@ -136,6 +136,8 @@ pub(super) enum HTLCForwardInfo {
 		// `process_pending_htlc_forwards()` for constructing the
 		// `HTLCSource::PreviousHopData` for failed and forwarded
 		// HTLCs.
+		//
+		// Note that this may be an outbound SCID alias for the associated channel.


I believe that comment should be dup on PendingHTLCRouting::Forward's short_channel_id

Ah, a similar comment applies there as well, yes.

ariard · 2022-05-22T20:47:00Z

lightning/src/ln/channelmanager.rs

@@ -1398,7 +1401,7 @@ macro_rules! handle_chan_restoration_locked {
 		let res = loop {
 			let forwards: Vec<(PendingHTLCInfo, u64)> = $pending_forwards; // Force type-checking to resolve
 			if !forwards.is_empty() {
-				htlc_forwards = Some(($channel_entry.get().get_short_channel_id().expect("We can't have pending forwards before funding confirmation"),
+				htlc_forwards = Some(($channel_entry.get().get_short_channel_id().unwrap_or($channel_entry.get().outbound_scid_alias()),


Not covering the get_short_channel_id() in list_channels_with_filter, deliberate ?

I'm not sure what this is in reference to?

ariard · 2022-05-22T22:36:29Z

lightning/src/ln/channel.rs

-			// inconsistent, so we currently have to.
+			// Note that ideally we wouldn't force-close if we see *any* reorg on a 1-conf or
+			// 0-conf channel, but not doing so may lead to the `ChannelManager::short_to_id` map
+			// being inconsistent, so we currently have to.


Apart of more code complexity, I don't understand why we would force-close a 0-conf channel as we already assume it's safe without block inclusion ? And I'm not sure there is a real inconsistency risk with the short_to_id being inserted at funding_locked being a fake SCID ? Though one concern would be the funding inputs unconfirmed themselves.

No, we should fix that eventually - #867

ariard · 2022-05-22T22:56:28Z

lightning/src/ln/channelmanager.rs

+					if channel.is_our_funding_locked() {
+						if let Some(real_scid) = channel.get_short_channel_id() {
+							// If we sent a 0conf funding_locked, and now have an SCID, we add it
+							// to the short_to_id map here. Note that we check whether we can relay


Is that true of option_scid_alias private chan where if the short_channel_id is different from the outbound scid alias we fail the forward ?

rust-lightning/lightning/src/ln/channelmanager.rs

Line 2196 in 46e58ae

if chan.get_channel_type().supports_scid_privacy() && *short_channel_id != chan.outbound_scid_alias() {

Do you have test coverage of combining a private 0-conf and fake SCID routing over it ?

Is that true of option_scid_alias private chan where if the short_channel_id is different from the outbound scid alias we fail the forward ?

Yes, I updated the comment to be clearer.

Do you have test coverage of combining a private 0-conf and fake SCID routing over it ?

Yes? I'm not sure exactly what combo you're asking for but there's good testing of the reject-real-scid-on-aliases.

ariard · 2022-05-22T23:13:19Z

lightning/src/ln/channelmanager.rs

@@ -2291,6 +2291,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				action: msgs::ErrorAction::IgnoreError
 			});
 		}
+		if chan.get_short_channel_id().is_none() {


Doesn't seem to have test coverage ? Commenting out doesn't break anything.

Hmm, good point, its kinda hard to hit, though - its only used on force-close (which I added a trivial test for) and in broadcasting our node announcement (which we can't really hit from here because we don't have our counterparty announcement sigs).

In the next few commits we add support for 0conf channels, allowing us to have an active channel with HTLC and other updates flying prior to having an SCID available. This would break several assumptions made in `ChannelManager`, which we address here by looking at SCID aliases in addition to SCIDs.

If our peer sets a minimum depth of 0, and we're set to trusting ourselves to not double-spend our own funding transactions, send a funding_locked message immediately after funding signed. Note that some special care has to be taken around the `channel_state` values - `ChannelFunded` no longer implies the funding transaction is confirmed on-chain. Thus, for example, the should-we-re-broadcast logic has to now accept `channel_state` values greater than `ChannelFunded` as indicating we may still need to re-broadcast our funding tranasction, unless `minimum_depth` is greater than 0. Further note that this starts writing `Channel` objects with a `MIN_SERIALIZATION_VERSION` of 2. Thus, LDK versions prior to 0.0.99 (July 2021) will now refuse to read serialized Channels/ChannelManagers.

This supports routing outbound over 0-conf channels by utilizing the outbound SCID alias that we assign to all channels to refer to the selected channel when routing.

This tests a few cases of monitor failure updates that were broken in earlier versions of the 0conf patchset.

In a previous version of the 0-conf code we did not correctly handle 0-conf channels getting the funding transaction reorg'd out (and the real SCID possibly changing on us).

This isn't specific to 0-conf but is useful for users to note as the network moves towards more SCID aliases.

TheBlueMatt · 2022-05-27T22:40:43Z

Squashed without diff:

$ git diff-tree -U1 d58b8b05 ffaf9bc1
$

jkczyz · 2022-05-27T23:10:50Z

lightning/src/ln/channel.rs

-				if non_shutdown_state & !(ChannelState::TheirFundingLocked as u32) == ChannelState::FundingSent as u32 {
+				// Check if the transaction is the expected funding transaction, and if it is,
+				// check that it pays the right amount to the right script.
+				if self.funding_tx_confirmation_height == 0 {


I guess this predated the PR, but it's unclear to me what funding_tx_confirmation_height of 0 means.

Ah, it means "not yet confirmed", as certainly no channel is going to be confirmed at height 0 :) I kinda figured it was an obvious default but I can add docs if you prefer?

Meh, maybe an is_funding_confirmed function would be more readable, but no need to do anything here.

TheBlueMatt added this to the 0.0.107 milestone Apr 1, 2022

TheBlueMatt force-pushed the 2022-02-0conf-round-two branch from fabf567 to 6e30722 Compare April 2, 2022 02:18

TheBlueMatt force-pushed the 2022-02-0conf-round-two branch from 6e30722 to 3083a30 Compare April 2, 2022 23:26

ariard reviewed Apr 4, 2022

View reviewed changes

TheBlueMatt force-pushed the 2022-02-0conf-round-two branch from 3083a30 to c508890 Compare April 11, 2022 18:29

TheBlueMatt added the Seeking Code Review label Apr 18, 2022

valentinewallace reviewed Apr 26, 2022

View reviewed changes

valentinewallace reviewed Apr 27, 2022

View reviewed changes

TheBlueMatt force-pushed the 2022-02-0conf-round-two branch 2 times, most recently from 093d75e to fb24768 Compare April 28, 2022 22:02

valentinewallace reviewed Apr 29, 2022

View reviewed changes

TheBlueMatt added blocked on dependent pr and removed Seeking Code Review labels May 2, 2022

TheBlueMatt added Seeking Code Review and removed blocked on dependent pr labels May 4, 2022

TheBlueMatt force-pushed the 2022-02-0conf-round-two branch from fb24768 to f894386 Compare May 4, 2022 02:34

jkczyz reviewed May 13, 2022

View reviewed changes

TheBlueMatt force-pushed the 2022-02-0conf-round-two branch from f894386 to 6ffb633 Compare May 13, 2022 18:30

TheBlueMatt mentioned this pull request May 13, 2022

Drop the during-reorg channel closure behavior #867

Open

TheBlueMatt force-pushed the 2022-02-0conf-round-two branch 2 times, most recently from 7415bb3 to 433e487 Compare May 13, 2022 18:40

ariard reviewed May 17, 2022

View reviewed changes

wpaulino reviewed May 18, 2022

View reviewed changes

TheBlueMatt force-pushed the 2022-02-0conf-round-two branch 2 times, most recently from 32df62a to 2269c40 Compare May 19, 2022 16:54

ariard reviewed May 22, 2022

View reviewed changes

TheBlueMatt force-pushed the 2022-02-0conf-round-two branch from 2269c40 to d58b8b0 Compare May 24, 2022 20:56

TheBlueMatt assigned jkczyz, wpaulino and valentinewallace May 27, 2022

TheBlueMatt removed the Seeking Code Review label May 27, 2022

TheBlueMatt added 10 commits May 27, 2022 22:40

Define a crate-public constant for max fake SCID in blocks offset

168b3a5

Add API and signaling to accept incoming channels at 0conf

9569bfe

Send funding_locked immediately for inbound channels with 0conf

ff9e457

Expose outbound SCID alias in ChannelDetails and use in routing

26288e3

This supports routing outbound over 0-conf channels by utilizing the outbound SCID alias that we assign to all channels to refer to the selected channel when routing.

Add a new test for 0conf-with-monitor-update-failures

8be97f0

This tests a few cases of monitor failure updates that were broken in earlier versions of the 0conf patchset.

Correctly handle sending announcement sigs on public 0conf channels

7ed7a7d

Add test of 0conf channels getting the funding transaction reorg'd

86ad252

In a previous version of the 0-conf code we did not correctly handle 0-conf channels getting the funding transaction reorg'd out (and the real SCID possibly changing on us).

Add note about SCID collisions in PaymentPathFailed

ffaf9bc

This isn't specific to 0-conf but is useful for users to note as the network moves towards more SCID aliases.

TheBlueMatt force-pushed the 2022-02-0conf-round-two branch from d58b8b0 to ffaf9bc Compare May 27, 2022 22:40

valentinewallace approved these changes May 27, 2022

View reviewed changes

jkczyz reviewed May 27, 2022

View reviewed changes

jkczyz approved these changes May 27, 2022

View reviewed changes

TheBlueMatt merged commit ce7b0b4 into lightningdevkit:main May 27, 2022

TonyGiorgio mentioned this pull request May 28, 2022

June topics austinbitdevs/austinbitdevs.com#7

Closed

valentinewallace mentioned this pull request Nov 28, 2022

Intercept HTLC forwards for JIT channels #1835

Merged

Zero Conf Channels #1401

Zero Conf Channels #1401

Conversation

TheBlueMatt commented Apr 1, 2022

codecov-commenter commented Apr 1, 2022 • edited Loading

Codecov Report

TheBlueMatt commented Apr 2, 2022

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

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

valentinewallace Apr 26, 2022 • edited Loading

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

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

valentinewallace commented Apr 29, 2022

valentinewallace commented Apr 29, 2022

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented May 2, 2022

TheBlueMatt commented May 4, 2022

TheBlueMatt commented May 4, 2022

valentinewallace commented May 4, 2022

TheBlueMatt commented May 4, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented May 16, 2022

ariard 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

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

TheBlueMatt commented May 19, 2022

ariard 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

codecov-commenter commented Apr 1, 2022 •

edited

Loading

valentinewallace Apr 26, 2022 •

edited

Loading