Expose More Information about Channels and structs #984

TheBlueMatt · 2021-07-03T01:59:25Z

No description provided.

codecov · 2021-07-03T03:17:35Z

Codecov Report

Merging #984 (46c3ba4) into main (0c57018) will increase coverage by 0.25%.
The diff coverage is 93.61%.

@@            Coverage Diff             @@
##             main     #984      +/-   ##
==========================================
+ Coverage   90.72%   90.98%   +0.25%     
==========================================
  Files          60       60              
  Lines       30640    31478     +838     
==========================================
+ Hits        27798    28639     +841     
+ Misses       2842     2839       -3

Impacted Files	Coverage Δ
lightning-background-processor/src/lib.rs	`94.87% <ø> (ø)`
lightning/src/chain/channelmonitor.rs	`90.80% <ø> (ø)`
lightning/src/ln/functional_test_utils.rs	`94.83% <ø> (ø)`
lightning/src/chain/mod.rs	`58.82% <75.00%> (+38.82%)`	⬆️
lightning/src/ln/channel.rs	`88.50% <93.75%> (-0.02%)`	⬇️
lightning/src/ln/channelmanager.rs	`84.73% <100.00%> (+0.09%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.32% <100.00%> (+0.04%)`	⬆️
lightning/src/routing/router.rs	`97.41% <100.00%> (+1.43%)`	⬆️
... and 1 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 0c57018...46c3ba4. Read the comment docs.

jkczyz · 2021-07-03T21:44:33Z

lightning/src/ln/channel.rs

+			if local {
+				self.get_counterparty_selected_contest_delay().unwrap()
+			} else {
+				self.get_holder_selected_contest_delay()


I'd prefer if we could make this a local variable while you're touching the code as it makes the calls site much more readable. Hmm.. but it looks like build_htlc_transaction is only ever called with local set true.

I'm just gonna drop the utility function. It made sense when it was a real function but its such a thing wrapper around chan_utils that we can inline it at the callsites.

jkczyz · 2021-07-03T22:01:10Z

lightning/src/ln/channel.rs

-			counterparty_selected_channel_reserve_satoshis: 0,
+			counterparty_selected_channel_reserve_satoshis: None,
 			counterparty_htlc_minimum_msat: 0,
 			holder_htlc_minimum_msat: if config.own_channel_config.our_htlc_minimum_msat == 0 { 1 } else { config.own_channel_config.our_htlc_minimum_msat },
 			counterparty_max_accepted_htlcs: 0,
-			minimum_depth: 0, // Filled in in accept_channel
+			minimum_depth: None, // Filled in in accept_channel


Would be nice if Channel had a state that held this data rather than knowing to call unwrap when we know it is safe. But I guess there's a lot more work to get there. 😕

Yea, I've thought a bit before about inlining a ton of state variables inside ChannelState or something similar, but as you note its a lot of change. At least these unwraps are pretty trivial - we don't have to be very far along in the state machine for them to be known-good.

jkczyz · 2021-07-03T22:08:29Z

lightning/src/ln/channel.rs

+		if minimum_depth == Some(0) {
+			// Versions up to 0.0.98 had minimum_depth as a non-option, writing 0 for what we now
+			// consider None.
+			minimum_depth = None;
+		}


For 0-confirmation channels, will 0 be a legitimate depth?

Unclear? Per Rusty's spec I assume not (it just changes the state machine...), but we should probably support it, yea. Note that if the second serialized value is Some(0) then we will store Some(0) its only when the first value is 0 that we will consider it None.

jkczyz · 2021-07-03T22:13:30Z

lightning/src/ln/channel.rs

+			(1, self.minimum_depth, option),
+			(3, self.counterparty_selected_channel_reserve_satoshis, option),


Just so I'm clear, we are writing these twice (here and above), right? Wouldn't we want to increment our version instead?

By writing it twice (with an odd TLV) old versions can always read the Channel, and will do so correctly. New versions will let the TLV value override the old copy, allowing for a distinction between Some(0) and None, while old versions will use 0 for both.

I'm not really sure what the criteria should be for bumping the version number - my gut was kinda "only if we need to to understand the new fields correctly, which I think you can argue is fine here - the old code will run fine no matter what value is serialized, though if we start using Some(0) to mean something, then we need to bump the version so that we can flag to old versions that they don't know what our minimum_depth means.

I added a check at open to reject minimum_depth of 0 and a comment noting that the serialization version should be updated.

By writing it twice (with an odd TLV) old versions can always read the Channel, and will do so correctly. New versions will let the TLV value override the old copy, allowing for a distinction between Some(0) and None, while old versions will use 0 for both.

Could be useful to document this in a comment

if we start using Some(0) to mean something, then we need to bump the version so that we can flag to old versions that they don't know what our minimum_depth means.

And this

Added a comment on the write side here, on the read end there's a new rejection of 0 as a value and a comment in accept_channel to note that we should update the version when we do so.

jkczyz · 2021-07-03T22:22:50Z

lightning/src/ln/channelmanager.rs

+	/// that if we broadcast a revoked state, our counterparty can punish us by claiming at least
+	/// this value on chain.
+	///
+	/// This value is not included in [`outbound_capacity_msat`] as it can never be spent.


I feel this should be included in outbound_capacity_msat as otherwise we are redefining "capacity". Or at least we need a term for capacity minus reserve (spending capacity?).

I'm happy to rename it to spending capacity, but its used by the router to decide if it can fit an htlc into this channel so it should consider all the various limits, IMO.

What about just outbound_spendable_msat and inbound_spendable_msat? I agree capacity is strange.

outbound_spendable_msat and inbound_spendable_msat

Fine with this solution, though arguably that would ideally take into account the fee spike buffer and such?

Yea, we could try to include more things, but ultimately any use of this as an "exact" value is race-y - a send/forward could happen between when its read and when we go to send, and I feel like we'll screw it up pretty easily.

Isn't this mixing up what the users is looking for (channel value = inbound + outbound capacity) with what we want to supply to the router interface (routable value)? I feel these are separate concepts and our ChannelDetails abstraction is being reused where maybe it shouldn't be.

Isn't this mixing up what the users is looking for (channel value = inbound + outbound capacity) with what we want to supply to the router interface (routable value)?

If that's what users are looking for, we've never provided that. These values have always not included pending HTLCs, meaning you can't meaningfully add them together to get anything. Still, I think what we expose makes sense - users want to know how much money they can currently send over a channel (modulo race conditions), that's a really useful number. It just so happens to also be useful for the router. We could debate whether the inbound_capacity value is useful, but the outbound one I'd argue is definitely right.

Ah, I see. I was thinking more in terms of useful information to show an end-user (not necessarily the LDK developer) as we are doing in the sample node, which could be what is my channel balance?

I guess it is more of a question of what makes a good router interface -- which I'm happy to mull over -- and should it be part of a general ChannelDetails interface. Seems like we were using it for both but it wasn't satisfactory for either use case.

Ah, I see. I was thinking more in terms of useful information to show an end-user (not necessarily the LDK developer) as we are doing in the sample node, which could be what is my channel balance?

I'd still argue this is a reasonable thing, though maybe the name could be changed? As an end user I would very much like to know how much money I can send over a channel before I hit a limit. Phrasing the question as "how much of the channel is my balance?" seems vague to me - I'm not really sure that we can reasonably answer that question.

I guess it is more of a question of what makes a good router interface -- which I'm happy to mull over -- and should it be part of a general ChannelDetails interface. Seems like we were using it for both but it wasn't satisfactory for either use case.

I think I disagree - the details of available channels is exactly what the router wants, and whether those details also include things that are human-readable or not is kinda beside the point.

lightning/src/ln/channelmanager.rs

lightning/src/chain/mod.rs

jkczyz · 2021-07-03T22:35:43Z

lightning/src/ln/channelmanager.rs

+	pub counterparty_selected_channel_reserve_satoshis: Option<u64>,
+	/// The value, in satoshis, which must always be held in the channel for our counterparty. This
+	/// value ensures that if our counterparty broadcasts a revoked state, we can punish them by
+	/// claiming at least this value on chain.
+	///
+	/// This value is not included in [`inbound_capacity_msat`] as it can never be spent.
+	///
+	/// [`inbound_capacity_msat`]: ChannelDetails::inbound_capacity_msat
+	pub holder_selected_channel_reserve_satoshis: u64,


I think we should avoid "holder" (which isn't well known and may change) and "counterparty" in this interface. Could we use "inbound" and "outbound" to be consistent with the capacity fields? Who selected the value isn't very meaningful to the user.

Yea, I agree, but I'm not sure "inbound" and "outbound" is more clear, its a really awkward parameter in that its selected by the opposite party from whom it applies to. Maybe to_self_min_satoshis and to_remote_min_satoshis?

I like to_self_channel_reserve_satoshis and remote_channel_reserve_satoshis. IMO the fact that it was "selected by the opposite party" just needs a comment, and adding that to the naming is potentially misleading for people not reading carefully.

I went with to_remote_reserve_satoshis and to_self_reserve_satoshis

Well I think we're loosing in clarity re-introducing to_self/to_remote mentions, which have revealed themselves quite error-prone in the past and was the motivation of #633. I agree that "holder" meaning and usage isn't stabilized yet, though there we're just adding more confusions. I believe adopted naming here doesn't resolve the confusion on whom transactions the limits are applied.

What do you think on_holder_tx_reserve_sats/on_counterparty_tx_reserve_sats ? It's quite matching on_counterparty_tx_csv already used in channelmonitor.rs.

Sorry about falling off the conversation, but I don't quite agree with the decision here. If these reserve values are subtracted to produce inbound_capacity_msat and outbound_capacity_msat, why are "inbound" and "outbound" not appropriate modifiers for them?

Hmm, that feels off? The capacity (which we could rename to spendable, if relevant) fields are "the amount you can send", here we're talking about a static amount which is always in the channel "to" each side. I'm happy to rename them, but I'm kinda at a loss for what is better here.

And, maybe more to the point, if we are subtracting the reserve values to produce inbound_capacity_msat and outbound_capacity_msat, why do we need to expose them at all?

Because a user asked for them. I dunno why specifically but you could reasonably imagine decisions around when/which channel to force-close including the reserve values.

Somewhat related -- but maybe outside the scope of this change -- we are overloading "inbound" and "outbound" for channel creation as seen in the documentation for the reserve fields. Perhaps "acceptor" (i.e. fundee) and "initiator" (i.e. funder) are more appropriate there, which would leave "inbound" and "outbound" as a consistent modifier here for the "side" of the channel. At least for user-facing interfaces? Might need to think about this more.

Happy to do that, that makes sense, I think.

If "inbound" and "outbound" are only specific for capacities -- rather than generally for the "side" -- then an alternative, which I believe I proposed awhile ago, is to use no modifier for "our" side and "counterparty" for "their" side.

I'm happy with that too, I think, would need to see it in practice I think.

And, maybe more to the point, if we are subtracting the reserve values to produce inbound_capacity_msat and outbound_capacity_msat, why do we need to expose them at all?

Because a user asked for them. I dunno why specifically but you could reasonably imagine decisions around when/which channel to force-close including the reserve values.

Shouldn't we find out why before going forward with an interface change? 🙂 Especially if it means re-introducing terminology we decided against using?

The user doesn't always know what they want. We have an opportunity to learn what they are trying to do and how we can better serve them. Let's not let the opportunity pass us by!

Shouldn't we find out why before going forward with an interface change? slightly_smiling_face Especially if it means re-introducing terminology we decided against using?

Heh, fair, I also figured it was a reasonable request and something that could be used programmatically so went ahead with it. Concretely, all of LDK is a programmatic interface, not a human one, so providing more information never really seems like a particularly bad thing to me, as long as the documentation is clear on what its useful for and the variable names are clear. Do you have a concrete rename here?

I'd say we should provide interfaces that have a distinct purpose. The naming issues may partly be arising because we are trying to put too much into one interface.

If we limit one interface to routing payments, then much of the naming issues disappear: we would only need to expose one capacity (outbound) and one reserve, so there would be no need to come up with suitable prefixes.

I don't think "just split the ChannelDetails into two nearly-identical interfaces" solves the issue? We'll still debate about the use and names of particular variables. Ultimately I really think that the router and the "user" here want the same thing - information about how much money they can send/receive over a channel, as well as whether it is active and usable, so I really don't think two interfaces is "right". The user may want more information, but they still want the same basic information that the router wants. I agree we should have specific uses for individual variables, as well as clear documentation that describes how or why a user may wish to use them, though.

We don't support turbo channels so this is a pretty clear indication that there is some incompatibility.

valentinewallace

I'm good with this PR mod outstanding comments

valentinewallace · 2021-07-04T19:08:05Z

lightning/src/ln/channel.rs

@@ -4317,7 +4323,7 @@ impl<Signer: Sign> Channel<Signer> {
 	}

 	pub fn channel_update(&mut self, msg: &msgs::ChannelUpdate) -> Result<(), ChannelError> {
-		let usable_channel_value_msat = (self.channel_value_satoshis - self.counterparty_selected_channel_reserve_satoshis) * 1000;
+		let usable_channel_value_msat = (self.channel_value_satoshis - self.counterparty_selected_channel_reserve_satoshis.unwrap()) * 1000;


Is it possible for our counterparty to send send a channel_update before we're ready for this unwrap?

Grrrrr, yes, you're totally right. Further, I don't think we should really be rejecting channel_updates based on the reserve - I mean, sure, they shouldn't include it, but a node could reasonably just be ignoring it and we shouldn't close the channel for that.

valentinewallace · 2021-07-04T19:17:57Z

lightning/src/ln/channelmanager.rs

+	pub counterparty_selected_channel_reserve_satoshis: Option<u64>,
+	/// The value, in satoshis, which must always be held in the channel for our counterparty. This
+	/// value ensures that if our counterparty broadcasts a revoked state, we can punish them by
+	/// claiming at least this value on chain.
+	///
+	/// This value is not included in [`inbound_capacity_msat`] as it can never be spent.
+	///
+	/// [`inbound_capacity_msat`]: ChannelDetails::inbound_capacity_msat
+	pub holder_selected_channel_reserve_satoshis: u64,


I like to_self_channel_reserve_satoshis and remote_channel_reserve_satoshis. IMO the fact that it was "selected by the opposite party" just needs a comment, and adding that to the naming is potentially misleading for people not reading carefully.

fuzz/src/router.rs

lightning/src/chain/mod.rs

valentinewallace · 2021-07-05T16:49:30Z

lightning/src/ln/channel.rs

+		let mut minimum_depth = Some(Readable::read(reader)?);
+		if minimum_depth == Some(0) {
+			// Versions up to 0.0.98 had minimum_depth as a non-option, writing 0 for what we now
+			// consider None.
+			minimum_depth = None;
+		}


Thinking out loud: unsure if these special cases don't bode well for the complexity of our backwards compat going forward 🤔

Right, I'm not sure how to do it particularly better - its nice to be backwards compatible while still being able to add new things. At least in this case if we ever need to write 0s we can just bump the version number and then ignore this value when reading a new channel. I figured just shoving the "real" value in the TLV is nice and simple, but we could do this another way if we prefer.

If we get review on #975, we could use the version bump included there to just decide whether to read the value here or in the TLV (in the same way we do there) to also change the read here. That way we'd avoid two version bumps as well, which is nice.

I see! I don't really mind, up to you

I added a commit from #975 to do this, makes sense if we merge for 0.0.99.

valentinewallace · 2021-07-05T16:54:40Z

lightning/src/ln/channel.rs

+			(1, self.minimum_depth, option),
+			(3, self.counterparty_selected_channel_reserve_satoshis, option),


By writing it twice (with an odd TLV) old versions can always read the Channel, and will do so correctly. New versions will let the TLV value override the old copy, allowing for a distinction between Some(0) and None, while old versions will use 0 for both.

Could be useful to document this in a comment

if we start using Some(0) to mean something, then we need to bump the version so that we can flag to old versions that they don't know what our minimum_depth means.

And this

ariard · 2021-07-05T18:58:45Z

lightning/src/chain/mod.rs

@@ -28,6 +30,35 @@ pub mod keysinterface;
 pub(crate) mod onchaintx;
 pub(crate) mod package;

+/// The best known block as identified by its hash and height.


Should we precise further the meaning of "best known block", like the longuest-chain and not the most-PoW one which is an assumption beyond library scope ?

I think that's wayyyyy beyond the scope of our lightning library :).

lightning/src/ln/channel.rs

ariard · 2021-07-05T19:10:14Z

lightning/src/ln/channel.rs

@@ -1488,6 +1482,12 @@ impl<Signer: Sign> Channel<Signer> {
 		if msg.minimum_depth > config.peer_channel_config_limits.max_minimum_depth {
 			return Err(ChannelError::Close(format!("We consider the minimum depth to be unreasonably large. Expected minimum: ({}). Actual: ({})", config.peer_channel_config_limits.max_minimum_depth, msg.minimum_depth)));
 		}
+		if msg.minimum_depth == 0 {
+			// Note that if this changes we should update the serialization minimum version to


Following check_get_funding_locked, this new check actually prevent the creation of zero-conf chan, which were allowed previous to this PR ? I guess this is intentional waiting for upcoming specification of "turbo-channels" ?

Correct, we previously ditn't handle the 0-conf option, though. We'd accept a message with a 0 set, but then we'd still wait for a confirmation, so easier to just reject it outright.

ariard · 2021-07-05T19:31:51Z

lightning/src/ln/channel.rs

@@ -1785,8 +1785,22 @@ impl<Signer: Sign> Channel<Signer> {
 	/// corner case properly.
 	pub fn get_inbound_outbound_available_balance_msat(&self) -> (u64, u64) {


I think this function name and its usage in list_channels_with_filter are confusing balance/liquidity/capacity. IMHO, you have :

capacity : the size of the funding output value as seen on-chain, already mentioned in this sense by our API in ChannelInfo

liquidity : the in-flight, directed, available value to transfer HTLC, as defined by channel policy (htlc_maximum_msat or probabilistic assumptions), already mentioned through get_route

balance : the to_local output claimable by the holder without competition, assuming we're not broadcasting revoked state

So as a suggestion i would suggest to recall this function get_inbound_outbound_available_liquidity_msat though here we have not providing information on the max_accepted_htlc/max_htlc_value_in_flight which could effectively restrain the effective liquidity lawfully usable for forwarding/receiving ?

Maybe I would also suggest to drop the "not including pending HTLCs" are in fact they're subtracted to reflect that you can't reuse liquidity occupied by get_inbound_pending_htlc_stats to receive more ?

And last point, maybe precise yielded values should be consider "rough" and not considered to apply strict, additional policy enforcement ? E.g close channel if ratio inbound/outbound reaches X ?

The docs already indicate that these values are approximate and not exact, so we agree there :). Can we rename an existing private function in a later PR? 0.0.99 is already late enough.

ariard · 2021-07-05T19:46:53Z

lightning/src/ln/channelmanager.rs

+	pub counterparty_selected_channel_reserve_satoshis: Option<u64>,
+	/// The value, in satoshis, which must always be held in the channel for our counterparty. This
+	/// value ensures that if our counterparty broadcasts a revoked state, we can punish them by
+	/// claiming at least this value on chain.
+	///
+	/// This value is not included in [`inbound_capacity_msat`] as it can never be spent.
+	///
+	/// [`inbound_capacity_msat`]: ChannelDetails::inbound_capacity_msat
+	pub holder_selected_channel_reserve_satoshis: u64,


Well I think we're loosing in clarity re-introducing to_self/to_remote mentions, which have revealed themselves quite error-prone in the past and was the motivation of #633. I agree that "holder" meaning and usage isn't stabilized yet, though there we're just adding more confusions. I believe adopted naming here doesn't resolve the confusion on whom transactions the limits are applied.

What do you think on_holder_tx_reserve_sats/on_counterparty_tx_reserve_sats ? It's quite matching on_counterparty_tx_csv already used in channelmonitor.rs.

ariard

See my points around to_self/to_remote` naming and liquidity/capacity blurring, which have been also raised by the other reviewers. I don't think solving them are blockers for this PR, though it would be nice if we agree and observe a consistent naming for the future.

ariard · 2021-07-05T19:48:08Z

lightning/src/ln/channelmanager.rs

 	/// The user_id passed in to create_channel, or 0 if the channel was inbound.
 	pub user_id: u64,
 	/// The available outbound capacity for sending HTLCs to the remote peer. This does not include
 	/// any pending HTLCs which are not yet fully resolved (and, thus, who's balance is not
 	/// available for inclusion in new outbound HTLCs). This further does not include any pending
 	/// outgoing HTLCs which are awaiting some other resolution to be sent.
+	///
+	/// This value is not exact. Due to various in-flight changes, feerate changes, and our
+	/// conflict-avoidance policy, exactly this amount is not likely to be spendable. However, we


What do you mean by conflict-avoidance policy ? The HTLC fee buffer?

Yes, the commitment tx fee buffer. Its a conflict-avoidance thing where you race the counterparty. I'm not sure how to specify that more clearly, but I don't think users need to care too much exactly what it means.

ariard · 2021-07-05T19:51:12Z

lightning/src/ln/channelmanager.rs

+	/// The number of required confirmations on the funding transaction before the funding will be
+	/// considered "locked". This number is selected by the channel fundee (i.e. us if
+	/// [`is_outbound`] is *not* set), and can be selected for inbound channels with
+	/// [`ChannelHandshakeConfig::minimum_depth`] or limited for outbound channels with


Instead of "limited", what do you think of "upper-bounded" ?

A min_minimum_depth superior to our default of 6 confs could make sense if you only plan to accept sizeable outbound channels (and quite complementary to min_funding_satoshis)

I think its pretty clear. A user won't be confused by the time they go read the docs for the config options, which is the way it should be, we don't have to explicitly say every detail of a different parameter here.

ariard · 2021-07-05T19:52:48Z

lightning/src/ln/channelmanager.rs

+	/// until we can claim our funds after we force-close the channel. During this time our
+	/// counterparty is allowed to punish us if we broadcasted a stale state. If our counterparty
+	/// force-closes the channel and broadcasts a commitment transaction we do not have to wait any
+	/// time to claim our non-HTLC-encumbered funds.


At least on legacy chan, consider CSV 1 on anchor chan.

CSV 1 is ~not having to wait, IMO. its certainly close enough from users' point-of-view, IMO, especially that then moment you see the commitment transaction confirmed on-chain you can broadcast the spend.

lightning/src/ln/channelmanager.rs

ariard · 2021-07-05T23:33:19Z

Code Review ACK eae4386

These fields are set with a dummy value, which we should generally be avoiding since Rust gives us a nice `Option` type to use instead. Further, we stop rejecting channel_update messages outright when the htlc_maximum_msat field includes the reserve values, which nodes could reasonably do without it meriting a channel closure.

This adds four new fields in `ChannelDetails`: 1. holder_selected_ and counterparty_selected_channel_reserve_delay are useful to determine what amount of the channel is unavailable for payments. 2. confirmations_required is useful when awaiting funding confirmation to determine how long you will need to wait. 3. to_self_delay is useful to determine how long it will take to receive funds after a force-close. Fixes lightningdevkit#983.

Fixes lightningdevkit#979

TheBlueMatt · 2021-07-06T00:21:37Z

Squashed with only a rename of to_self_delay as requested at #984 (comment). All the changes from previous acks are quite trivial so will merge after CI.

Diff from Val's ACK to ariard's:

$ git diff-tree -U1  928c33f eae4386
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 27f1938d..d8a284fa 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -612,3 +612,3 @@ impl<Signer: Sign> Channel<Signer> {
 			counterparty_max_htlc_value_in_flight_msat: 0,
-			counterparty_selected_channel_reserve_satoshis: None,
+			counterparty_selected_channel_reserve_satoshis: None, // Filled in in accept_channel
 			counterparty_htlc_minimum_msat: 0,
$

Diff from ariard's to HEAD:

$ git diff-tree -U1 eae4386 46c3ba49
diff --git a/fuzz/src/router.rs b/fuzz/src/router.rs
index d3eb6eba..fabad11a 100644
--- a/fuzz/src/router.rs
+++ b/fuzz/src/router.rs
@@ -217,3 +217,4 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 								to_remote_reserve_satoshis: 0,
-								confirmations_required: None, to_self_delay: None,
+								confirmations_required: None,
+								spend_csv_on_our_commitment_funds: None,
 								is_outbound: true, is_funding_locked: true,
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index e98981f6..21d13eae 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -687,3 +687,3 @@ pub struct ChannelDetails {
 	/// This value will be `None` for outbound channels until the counterparty accepts the channel.
-	pub to_self_delay: Option<u16>,
+	pub spend_csv_on_our_commitment_funds: Option<u16>,
 	/// True if the channel was initiated (and thus funded) by us.
@@ -1183,3 +1183,3 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					confirmations_required: channel.minimum_depth(),
-					to_self_delay: channel.get_counterparty_selected_contest_delay(),
+					spend_csv_on_our_commitment_funds: channel.get_counterparty_selected_contest_delay(),
 					is_outbound: channel.is_outbound(),
diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index 96559d65..aa1c3c11 100644
--- a/lightning/src/routing/router.rs
+++ b/lightning/src/routing/router.rs
@@ -1646,3 +1646,4 @@ mod tests {
 			to_remote_reserve_satoshis: 0,
-			confirmations_required: None, to_self_delay: None,
+			confirmations_required: None,
+			spend_csv_on_our_commitment_funds: None,
 			is_outbound: true, is_funding_locked: true,
@@ -1969,3 +1970,4 @@ mod tests {
 			to_remote_reserve_satoshis: 0,
-			confirmations_required: None, to_self_delay: None,
+			confirmations_required: None,
+			spend_csv_on_our_commitment_funds: None,
 			is_outbound: true, is_funding_locked: true,
@@ -2022,3 +2024,4 @@ mod tests {
 			to_remote_reserve_satoshis: 0,
-			confirmations_required: None, to_self_delay: None,
+			confirmations_required: None,
+			spend_csv_on_our_commitment_funds: None,
 			is_outbound: true, is_funding_locked: true,
@@ -2092,3 +2095,4 @@ mod tests {
 			to_remote_reserve_satoshis: 0,
-			confirmations_required: None, to_self_delay: None,
+			confirmations_required: None,
+			spend_csv_on_our_commitment_funds: None,
 			is_outbound: true, is_funding_locked: true,
@@ -2234,3 +2238,4 @@ mod tests {
 			to_remote_reserve_satoshis: 0,
-			confirmations_required: None, to_self_delay: None,
+			confirmations_required: None,
+			spend_csv_on_our_commitment_funds: None,
 			is_outbound: true, is_funding_locked: true,
@@ -2368,3 +2373,4 @@ mod tests {
 			to_remote_reserve_satoshis: 0,
-			confirmations_required: None, to_self_delay: None,
+			confirmations_required: None,
+			spend_csv_on_our_commitment_funds: None,
 			is_outbound: true, is_funding_locked: true,
@@ -2505,3 +2511,4 @@ mod tests {
 			to_remote_reserve_satoshis: 0,
-			confirmations_required: None, to_self_delay: None,
+			confirmations_required: None,
+			spend_csv_on_our_commitment_funds: None,
 			is_outbound: true, is_funding_locked: true,
$

After the merge of lightningdevkit#984, Jeff pointed out that `ChannelDetails` has become a bit of a "bag of variables", and that a few of the variable names in lightningdevkit#984 were more confusing than necessary in context. This addresses several issues by: * Splitting counterparty parameters into a separate `ChannelCounterpartyParameters` struct, * using the name `unspendable_punishment_reserve` for both outbound and inbound channel reserves, differentiating them based on their position in the counterparty parameters struct or not, * Using the name `force_close_spend_delay` instead of `spend_csv_on_our_commitment_funds` to better communicate what is occurring.

TheBlueMatt added this to the 0.0.99 milestone Jul 3, 2021

TheBlueMatt force-pushed the 2021-06-more-chan-data branch 4 times, most recently from 57cc55b to d5d37d4 Compare July 3, 2021 03:00

TheBlueMatt force-pushed the 2021-06-more-chan-data branch from d5d37d4 to 6e16062 Compare July 3, 2021 15:35

jkczyz reviewed Jul 3, 2021

View reviewed changes

Reject minimum_depth of 0 on channel opens

fbb36a0

We don't support turbo channels so this is a pretty clear indication that there is some incompatibility.

TheBlueMatt force-pushed the 2021-06-more-chan-data branch from 6e16062 to 82021a2 Compare July 4, 2021 14:20

valentinewallace reviewed Jul 4, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-06-more-chan-data branch from 82021a2 to 6d9cefd Compare July 4, 2021 23:45

valentinewallace reviewed Jul 5, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-06-more-chan-data branch from 6d9cefd to 928c33f Compare July 5, 2021 18:34

valentinewallace approved these changes Jul 5, 2021

View reviewed changes

ariard reviewed Jul 5, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-06-more-chan-data branch from 928c33f to eae4386 Compare July 5, 2021 20:57

TheBlueMatt added 5 commits July 6, 2021 00:18

Drop Channel HTLC transaction building thin wrapper function

c2b0db0

Expose the current best chain tip from ChannelManager + Monitors

da298e4

Fixes lightningdevkit#979

Tweak documentation in BestBlock to be a bit more clear

46c3ba4

TheBlueMatt force-pushed the 2021-06-more-chan-data branch from eae4386 to 46c3ba4 Compare July 6, 2021 00:18

TheBlueMatt merged commit 431f807 into lightningdevkit:main Jul 6, 2021

TheBlueMatt mentioned this pull request Jul 6, 2021

Make the base fee configurable in ChannelConfig #975

Merged

TheBlueMatt mentioned this pull request Jul 6, 2021

Improve ChannelDetails readability significantly. #988

Merged

		(1, self.minimum_depth, option),
		(3, self.counterparty_selected_channel_reserve_satoshis, option),

		@@ -1785,8 +1785,22 @@ impl<Signer: Sign> Channel<Signer> {
		/// corner case properly.
		pub fn get_inbound_outbound_available_balance_msat(&self) -> (u64, u64) {

Expose More Information about Channels and structs #984

Expose More Information about Channels and structs #984

Uh oh!

Conversation

TheBlueMatt commented Jul 3, 2021

Uh oh!

codecov bot commented Jul 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov bot commented Jul 3, 2021 •

edited

Loading

TheBlueMatt Jul 6, 2021 •

edited

Loading