Expose in-flight claim balances #1034

TheBlueMatt · 2021-08-04T16:15:27Z

Based on #1025, this exposes the set of balances which are still pending for claim.

Fixes #995.

TheBlueMatt · 2021-08-14T16:48:40Z

Rebased on latest upstream so this has no dependencies, though the first two commits are also now in #1045, which we should consider landing for 0.0.100.

codecov · 2021-08-14T17:08:19Z

Codecov Report

Merging #1034 (3d53090) into main (2ced708) will increase coverage by 1.09%.
The diff coverage is 95.02%.

❗ Current head 3d53090 differs from pull request most recent head cae2123. Consider uploading reports for the commit cae2123 to get more accurate results

@@            Coverage Diff             @@
##             main    #1034      +/-   ##
==========================================
+ Coverage   90.82%   91.91%   +1.09%     
==========================================
  Files          65       65              
  Lines       32801    39899    +7098     
==========================================
+ Hits        29791    36673    +6882     
- Misses       3010     3226     +216

Impacted Files	Coverage Δ
lightning/src/chain/chainmonitor.rs	`95.91% <ø> (ø)`
lightning/src/util/ser_macros.rs	`87.90% <ø> (ø)`
lightning/src/chain/channelmonitor.rs	`91.26% <88.83%> (-0.71%)`	⬇️
lightning/src/chain/onchaintx.rs	`94.27% <100.00%> (+0.08%)`	⬆️
lightning/src/ln/monitor_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/util/macro_logger.rs	`87.87% <100.00%> (ø)`
lightning/src/util/enforcing_trait_impls.rs	`89.80% <0.00%> (-0.58%)`	⬇️
lightning/src/util/ser.rs	`91.68% <0.00%> (-0.52%)`	⬇️
lightning-invoice/src/de.rs	`80.62% <0.00%> (-0.12%)`	⬇️
... and 14 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 2ced708...cae2123. Read the comment docs.

ariard

Still need to get into the gritty details of get_claimable_balances but sounds good overall.

ariard · 2021-08-22T02:15:48Z

lightning/src/chain/channelmonitor.rs

@@ -386,6 +394,10 @@ enum OnchainEvent {
 	MaturingOutput {
 		descriptor: SpendableOutputDescriptor,
 	},
+	FundingSpendConfirmation {


"Funding output has been either by us or remote, when it's mature we return the balance availability to the caller". Though I wonder if we couldn't just add a field to MaturingOutput, they're both generated in transactions_confirmed ?

Hmm? MaturingOutput is not created in transactions_confirmed, and it could be skipped if we have no to-self output or on a cooperative close transaction.

Well it's created in is_paying_spendable_output which is called by transactions_confirmed. Though yes no guarantee it's generated for cooperative closing transactions, I wasn't sure if you intended to cover this case.

Should user also expect to get the txid/on_local_output on revoked commitment transaction ?

Yes, we should cover cooperative closing transactions so that we stop providing a balance after a channel is closed. I imagine users will basically ask for the current set of spendable balances and add it to the GUI-displayed balance so we should be careful to not expose too much balance and have it persist.

Indeed, tracking balances for revoked spend needs to happen, but is left largely implemented in this PR (I believe we'll always just say no balance to spend. I'd updated the docs to mention this a few days ago after you'd started review initially.

ariard · 2021-08-22T02:21:04Z

lightning/src/chain/channelmonitor.rs

+#[cfg_attr(test, derive(PartialOrd, Ord))]
+pub enum ClaimableBalance {
+	/// The channel is not yet closed (or the initial commitment or closing transaction has not yet
+	/// been confirmed). The given balance is claimable (less on-chain fees) if the channel is


Hmmm confusing, just say "the channel is not yet closed, either cooperatively or non-cooperatively" ? I don't see what the distinctions initial commitment/beyond-initial commitment/closing bring.

Is it better now? I think its very important we note that we will still use this before the commitment appears in a block, even if the channel is "closed" from the users' perspective.

ariard · 2021-08-22T02:25:57Z

lightning/src/chain/channelmonitor.rs

@@ -521,6 +521,56 @@ impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep,
 	},
 );

+/// Details about the balance available for claim once the channel appears on chain.


Precise by balance you mean to_local output + HTLC outputs and it negatively change until all HTLCs
are settled.

Tweaked it a bit but mostly just pushed for people to look at get_claimable_balances which has more details.

valentinewallace · 2021-08-24T16:24:31Z

Failing test?

This simplifies the tlv serialization read macro somewhat by allowing callsites to simply read into an `Option<Vec>` instead of needing to read into an `Option<VecReadWrapper>` when using `vec_type`.

lightning/src/chain/channelmonitor.rs

TheBlueMatt · 2021-08-24T17:46:14Z

Failing test?

Ah, it only fails when rebased on upstream git. Rebased and fixed the tests to match new upstream close negotiation changes.

lightning/src/chain/channelmonitor.rs

valentinewallace · 2021-08-24T18:33:36Z

lightning/src/chain/channelmonitor.rs

+			debug_assert!(us.funding_spend_confirmed.is_none(), "We have a pending funding spend awaiting confirmation, we can't have confirmed it already!");
+			confirmed_txid = Some(txid);
+			res.push(ClaimableBalance::ClaimableAwaitingConfirmations {
+				claimable_amount_satoshis: us.current_holder_commitment_tx.to_self_value_sat,


~~Is it possible that it was the prev_holder_commitment_tx that was spending the funding tx instead?~~

Is it possible that we want to get our to_self_value from the previous commitment tx instead?

Hmm, good point. I adapted it to hopefully fix this.

lightning/src/chain/channelmonitor.rs

valentinewallace · 2021-08-24T19:28:40Z

lightning/src/chain/channelmonitor.rs

+										// `payment_data.is_none()` implies that this is our
+										// payment, as we haven't learned anything to cause us to


I read "our payment" as "an local outbound HTLC". But then below it allows accepted_preimage_claim (which is an inbound HTLC, iiuc?). Is there some way this comment could be clarified?

Changed "our payment" to "a payment to us"

valentinewallace · 2021-08-24T20:27:41Z

lightning/src/chain/channelmonitor.rs

+										if input_idx == htlc_input_idx { Some(event.confirmation_threshold()) } else { None }
+									} else { None }
+								}) {
+									res.push(ClaimableBalance::ClaimableAwaitingConfirmations {


Checking my understanding -- Is this situation for any HTLC output claim that's been seen in a block, but we're awaiting irrevocable confirmation (timeout claim txs + success claim txs)?

This is an HTLC output claim transaction that has appeared in a block, but we're awaiting ANTI_REORG_DELAY confirmations until we pass the user the SpendableOutput information.

lightning/src/chain/channelmonitor.rs

lightning/src/util/macro_logger.rs

lightning/src/chain/channelmonitor.rs

lightning/src/ln/monitor_tests.rs

valentinewallace

I think I just want to understand the HTLCSpendConfirmation events a little more and then this'll look good to me.

lightning/src/chain/chainmonitor.rs

lightning/src/chain/channelmonitor.rs

valentinewallace · 2021-08-25T20:51:04Z

lightning/src/chain/channelmonitor.rs

+	/// Included for both claims and failures, to allow us to track when we should stop informing
+	/// users there is a contentious claim and stop informing users there is a pending claim after
+	/// we generate a `SpendableOutput`.
+	HTLCSpendConfirmation {


I think it'd be good to rename HTLCSpendConfirmation and HTLCUpdate to make them more distinguishable at a glance. Suggestion: HTLCUpdate -> HTLCTimeoutSpend and PendingSpendableHTLC or PendingClaimableHTLC

Do you still think so given the change docs that clarify exactly what everything is used for? The docs are slightly wrong in this version.

lightning/src/chain/channelmonitor.rs

valentinewallace · 2021-08-25T22:36:28Z

lightning/src/chain/chainmonitor.rs

+					continue 'monitor_iter;
+				}
+			}
+			ret.append(&mut monitor.get_claimable_balances());


Hmm, I'm fine with it if users are fine with it, but it seems odd to not relate ClaimableBalances to the channel they correspond to

Hmm, I guess if users want it they can get it manually. Lets expose it as-is and see if anyone complains and we can deal with it then.

Yeah, I tend to agree with @valentinewallace. Seems more intuitive / useful and would simplify the interface if a user asked for the balances of a particular channel rather than providing a filter. It would be easier for them to combine balances across all channels (if needed) than figure out which channel each balance belongs to.

What exactly was the use case that led to this request?

user asked for the balances of a particular channel rather than providing a filter

You should be able to with the current interface?

What exactly was the use case that led to this request?

This PR is targeted at clients who want to display a consistent user balance while a channel close is being confirmed on-chain. If you take the output of ChannelManager::list_channels and calculate live balances from those channels, and add them to the balances (of sensible type) in this PR, you can simply display that as the "user balance".

user asked for the balances of a particular channel rather than providing a filter

You should be able to with the current interface?

True, but (1) it isn't as intuitive since you need to provide all channel except the one you want and (2) it isn't efficient as you need to iterate through all ignored channels for each channel monitor. The alternative is to have the user provide a funding tx output for the channel of interest and perform a hash map lookup.

That said, given my comment below maybe this is moot.

What exactly was the use case that led to this request?

This PR is targeted at clients who want to display a consistent user balance while a channel close is being confirmed on-chain. If you take the output of ChannelManager::list_channels and calculate live balances from those channels, and add them to the balances (of sensible type) in this PR, you can simply display that as the "user balance".

Ah, I see. I think I may have been confused in that you'd typically use it for balances of closed channels. I guess ChannelManager won't have any ChannelDetails for those? Happy to keep this as is if that's the case.

To be a bit more clear, I don't think we should approach it as "user queries on a per-channel basis", as that requires the user first asking the ChainMonitor for the list of channels they want to query for. I do think there's an argument to be had for exposing which channel each event corresponds to, but I'm not sure the right way to go about it, and I'm not 100% sure if users will care about that anyway. Open to ideas, or we can leave it as a future work.

Ah, I see. I think I may have been confused in that you'd typically use it for balances of closed channels. I guess ChannelManager won't have any ChannelDetails for those? Happy to keep this as is if that's the case.

Right! The idea is "I want to know my full balance, plus or minus cause balances in lightning are a bit confusing". The answer is "well, I have some off-chain balances, for which I'll ask ChannelManager and I have some on-chain balances, for which I mostly track myself, but there's some 'hiding' in ChainMonitor, which I'll have to ask it about". You don't want to double-count the not-yet-closed channels, so you tell ChainMonitor the list of things that you've already calculated as off-chain balances and let it tell you the rest.

valentinewallace · 2021-08-26T17:16:22Z

lightning/src/chain/channelmonitor.rs

@@ -2388,6 +2682,15 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 					if !self.pending_monitor_events.iter().any(
 						|update| if let &MonitorEvent::HTLCEvent(ref upd) = update { upd.source == source } else { false }) {
 						payment_preimage.0.copy_from_slice(&input.witness[3]);
+						self.onchain_events_awaiting_threshold_conf.push(OnchainEventEntry {


Missing something here -- how do we know that this is an HTLC accepted by us and not accepted by the counterparty (since accepted_preimage_claim could apply to either iiuc)?

I've updated a ton of docs here, let me know if its clearer.

ariard

I've been through the whole changes, I think it's pretty mature though maybe one bug ?

lightning/src/chain/channelmonitor.rs

ariard · 2021-08-29T15:10:34Z

lightning/src/chain/channelmonitor.rs

+	/// state(s) may not be fully captured here.
+	// TODO, fix that ^
+	///
+	/// See [`ClaimableBalance`] for additional details on the types of claimable balances which


"Note, if the tracked chain endures a reorg superior to ANTI_REORG_DELAY the reported claimable balances are likely inaccurate" ?

Hmm, ANTI_REORG_DELAY is more of a library-wide security assumption. The balances being wrong is a very minor issue compared to losing funds, I expanded the comment on ANTI_REORG_DELAY, though.

lightning/src/chain/channelmonitor.rs

ariard · 2021-08-29T16:01:37Z

lightning/src/chain/channelmonitor.rs

+		} else {
+			let mut claimable_inbound_htlc_value_sat = 0;
+			for (htlc, _, _) in us.current_holder_commitment_tx.htlc_outputs.iter() {
+				if htlc.transaction_output_index.is_none() { continue; }


Maybe add a TODO "Provide burn-as-fees balance". A user might be interested to not go on-chain because the dust amount is high-enough to wait a bit for those HTLC to settle ?

Its not a balance, though, because it wont later come in. Providing accounting of fees is a much larger project, see #1014.

lightning/src/chain/channelmonitor.rs

ariard · 2021-08-29T17:25:47Z

lightning/src/chain/channelmonitor.rs

+											// claimable balance if the claim was an HTLC-Success or
+											// HTLC-Timeout transaction.
+											on_to_local_output_csv: if accepted_preimage_claim || offered_timeout_claim {
+												Some(self.on_holder_tx_csv) } else { Some(0) },


If the HTLC is inbound (!outbound_htlc), it means it's either a received HTLC ouput on holder commitment or an offered HTLC output on counterparty commitment. If it's an offered HTLC output on counterparty commitment, it can be claimed with an offered_preimage_claim and not a offered_timeout_claim ? And further, if it's a offered_preimage_claim, transaction isn't encumbered by a CSV ?

This is inside of an if block that checks !outbound_htlc || revocation_sig_claim, so it can't be an inbound HTLC that is claimed by anything other than a revoked claim (which we don't handle anyway).

valentinewallace

I'm ACK mod these comments

lightning/src/chain/channelmonitor.rs

valentinewallace

ACK after squash :)

valentinewallace · 2021-09-03T00:12:15Z

lightning/src/chain/channelmonitor.rs

+											// `payment_data.is_none()` implies that this is a payment
+											// to us, as we haven't learned anything to cause us to
+											// update another channel or our offchain state. Thus, wait
+											// for the CSV delay before dropping the HTLC from
+											// claimable balance if the claim was an HTLC-Success or
+											// HTLC-Timeout transaction.
+											on_to_local_output_csv: if accepted_preimage_claim || offered_timeout_claim {
+												Some(self.on_holder_tx_csv) } else { None },


Oh wait sorry... So if this is a payment to us, and it's an offered_timeout_claim (implying counterparty commit tx), why would we use on_holder_tx_csv? Wouldn't we just be able to claim without waiting for csv to pass?

Its helpful that the offered_timeout_claim case's Some value here is dead code :p. If its an inbound (ie from counterparty to us) HTLC, claimed via a timeout on a counterparty commitment transaction, it is not our HTLC/we won't have a preimage for it and we won't care about it in get_claimable_balance. All that said, yea, its wrong.

jkczyz

I took a pretty thorough look. Largely looks good modulo some comments. Will want to take a closer look after they're resolved.

lightning/src/chain/onchaintx.rs

lightning/src/chain/channelmonitor.rs

lightning/src/chain/chainmonitor.rs

jkczyz · 2021-09-13T21:11:14Z

lightning/src/chain/chainmonitor.rs

+					continue 'monitor_iter;
+				}
+			}
+			ret.append(&mut monitor.get_claimable_balances());


Yeah, I tend to agree with @valentinewallace. Seems more intuitive / useful and would simplify the interface if a user asked for the balances of a particular channel rather than providing a filter. It would be easier for them to combine balances across all channels (if needed) than figure out which channel each balance belongs to.

What exactly was the use case that led to this request?

TheBlueMatt · 2021-09-15T04:23:06Z

I believe I've addressed all outstanding comments.

jkczyz

ACK 249f3d8

This allows us to easily look up how our channel was closed and track which balances may be spendable on-chain.

This tracks how any HTLC outputs in broadcast commitment transactions are resolved on-chain, storing the result of the HTLC resolution persistently in the ChannelMonitor. This can be used to determine which outputs may still be available for claiming on-chain.

In general, we should always allow users to query for how much is currently in-flight being claimed on-chain at any time. This does so by examining the confirmed claims on-chain and breaking down what is left to be claimed into a new `ClaimableBalance` enum. Fixes lightningdevkit#995.

The common user desire is to get the set of claimable balances for all non-closed channels. In order to do so, they really want to just ask their `ChainMonitor` for the set of balances, which they can do here by passing the `ChannelManager::list_channels` output to `ChainMonitor::get_claimable_balances`.

TheBlueMatt · 2021-09-15T18:08:07Z

Squashed without changes, CI should pass now as no intermediary commits fail.

$ git diff-tree -U1 249f3d8bd cae212379
$

TheBlueMatt changed the title ~~2021 07 maturing claims~~ Expose in-flight claim balances Aug 4, 2021

TheBlueMatt mentioned this pull request Aug 4, 2021

Correctly detect missing HTLCs when a local commitment tx was broadcast #1025

Merged

TheBlueMatt force-pushed the 2021-07-maturing-claims branch 2 times, most recently from eeb9f36 to 2110eae Compare August 4, 2021 17:56

TheBlueMatt added this to the 0.0.101 milestone Aug 10, 2021

TheBlueMatt mentioned this pull request Aug 14, 2021

Make ChannelMonitor serialization slightly more upgradable #1045

Merged

TheBlueMatt force-pushed the 2021-07-maturing-claims branch from eab7407 to b15d74a Compare August 14, 2021 16:48

TheBlueMatt force-pushed the 2021-07-maturing-claims branch 3 times, most recently from 73327c0 to 5bbc1a3 Compare August 16, 2021 18:33

ariard reviewed Aug 22, 2021

View reviewed changes

jkczyz requested a review from valentinewallace August 23, 2021 19:16

TheBlueMatt force-pushed the 2021-07-maturing-claims branch from 5bbc1a3 to a9ae5f3 Compare August 24, 2021 04:35

Don't initialise Vecs being read with VecReadWrapper explicitly

efa579b

This simplifies the tlv serialization read macro somewhat by allowing callsites to simply read into an `Option<Vec>` instead of needing to read into an `Option<VecReadWrapper>` when using `vec_type`.

valentinewallace reviewed Aug 24, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-07-maturing-claims branch from a9ae5f3 to bc2a72a Compare August 24, 2021 17:45

TheBlueMatt force-pushed the 2021-07-maturing-claims branch 2 times, most recently from 2a20a82 to a9067be Compare August 24, 2021 18:59

valentinewallace reviewed Aug 24, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-07-maturing-claims branch from a9067be to 1a72b97 Compare August 25, 2021 03:05

valentinewallace reviewed Aug 25, 2021

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/util/macro_logger.rs Outdated Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Show resolved Hide resolved

valentinewallace reviewed Aug 25, 2021

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

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

TheBlueMatt force-pushed the 2021-07-maturing-claims branch from 1a72b97 to 21766be Compare August 25, 2021 19:47

valentinewallace reviewed Aug 26, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-07-maturing-claims branch 2 times, most recently from ddd79da to 8f793fd Compare August 27, 2021 00:46

TheBlueMatt force-pushed the 2021-07-maturing-claims branch 2 times, most recently from 3bba749 to 27ce70b Compare August 28, 2021 03:25

ariard reviewed Aug 29, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-07-maturing-claims branch 2 times, most recently from 903a8e4 to 5a8452e Compare August 30, 2021 18:40

valentinewallace reviewed Sep 2, 2021

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Show resolved Hide resolved

TheBlueMatt force-pushed the 2021-07-maturing-claims branch from 628c134 to cd7bdd0 Compare September 2, 2021 23:38

valentinewallace approved these changes Sep 2, 2021

View reviewed changes

valentinewallace reviewed Sep 3, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-07-maturing-claims branch from cd7bdd0 to 86bbfba Compare September 3, 2021 00:37

valentinewallace approved these changes Sep 3, 2021

View reviewed changes

jkczyz reviewed Sep 13, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-07-maturing-claims branch 3 times, most recently from 3d53090 to 249f3d8 Compare September 15, 2021 04:22

jkczyz approved these changes Sep 15, 2021

View reviewed changes

TheBlueMatt added 9 commits September 15, 2021 18:07

Store to-self value in the current commitment tx in ChannelMonitor

aaba0a2

Drop unused CounterpartyCommitmentTransaction::per_htlc HashMap

44e4867

Rename CounterpartyCommitmentTransaction to Params as it is static

cd578b5

Track the tx which spends our funding output in ChannelMonitor

73ee30d

This allows us to easily look up how our channel was closed and track which balances may be spendable on-chain.

Fix indentation in ChannelMonitor

f9febb0

Expand ANTI_REORG_DELAY docs to say its a library-wide assumption

cae2123

TheBlueMatt force-pushed the 2021-07-maturing-claims branch from 249f3d8 to cae2123 Compare September 15, 2021 18:07

valentinewallace approved these changes Sep 15, 2021

View reviewed changes

TheBlueMatt merged commit 35573bb into lightningdevkit:main Sep 15, 2021

		// `payment_data.is_none()` implies that this is our
		// payment, as we haven't learned anything to cause us to

Expose in-flight claim balances #1034

Expose in-flight claim balances #1034

Conversation

TheBlueMatt commented Aug 4, 2021

TheBlueMatt commented Aug 14, 2021 • edited Loading

codecov bot commented Aug 14, 2021 • edited Loading

Codecov Report

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

valentinewallace commented Aug 24, 2021

TheBlueMatt commented Aug 24, 2021

valentinewallace Aug 24, 2021 • 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

valentinewallace left a comment

Choose a reason for hiding this comment

valentinewallace Aug 25, 2021 • 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

ariard left a comment • 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

valentinewallace left a comment

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

valentinewallace Sep 3, 2021 • edited Loading

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

Choose a reason for hiding this comment

TheBlueMatt commented Sep 15, 2021

jkczyz left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Sep 15, 2021

TheBlueMatt commented Aug 14, 2021 •

edited

Loading

codecov bot commented Aug 14, 2021 •

edited

Loading

valentinewallace Aug 24, 2021 •

edited

Loading

valentinewallace Aug 25, 2021 •

edited

Loading

ariard left a comment •

edited

Loading

valentinewallace Sep 3, 2021 •

edited

Loading