From b6a7baac1ca98f27755918a8a23869905f1b7b88 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 28 Sep 2023 23:02:26 +0000 Subject: [PATCH] Include an `outbound_payment` flag in `MaybeTimeoutClaimableHTLC` When the user is fetching their current balances after forwarding a payment (before it clears), they'll see a `MaybePreimageClaimableHTLC` and a `MaybeTimeoutClaimableHTLC` but if they sum up their balance using `Balance::claimable_amount_satoshis` neither will be included. Obviously, exactly one of the two balances should be included - one of the two resolutions should happen in our favor. This causes our visible balance to fluctuate up and down by the full value of any HTLCs we're in the middle of forwarding, which is incredibly confusing to see. If we want to stop the fluctuations, we need to pick one of the two balances to include. The obvious candidate is `MaybeTimeoutClaimableHTLC` as it is the lower of the two, and represents our balance without the fee we'd receive from the forward. Sadly, if we always include it, we'll end up also including any HTLCs which we've sent but which haven't yet been claimed by their recipient, which is the wrong behavior. Luckily, we have access to the `Option` while walking HTLCs, which allows us to add an `outbound_payment` flag to `MaybeTimeoutClaimableHTLC`. This allows us to only include forwarded payments in `claimable_amount_satoshis`. Sadly, even with this in place our balance still fluctuates by the changes in the commitment transaction fees we have to pay during forwarding, but addressing that is left for later. --- lightning/src/chain/channelmonitor.rs | 51 ++++++++++++++++++++------- lightning/src/ln/monitor_tests.rs | 11 ++++++ 2 files changed, 49 insertions(+), 13 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index af0f56d8..3845328c 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -620,6 +620,10 @@ pub enum Balance { claimable_height: u32, /// The payment hash whose preimage our counterparty needs to claim this HTLC. payment_hash: PaymentHash, + /// Whether this HTLC represents a payment which was sent outbound from us. Otherwise it + /// represents an HTLC which was forwarded (and should, thus, have a corresponding inbound + /// edge on another channel). + outbound_payment: bool, }, /// HTLCs which we received from our counterparty which are claimable with a preimage which we /// do not currently have. This will only be claimable if we receive the preimage from the node @@ -662,9 +666,9 @@ impl Balance { Balance::ContentiousClaimable { amount_satoshis, .. }| Balance::CounterpartyRevokedOutputClaimable { amount_satoshis, .. } => *amount_satoshis, - Balance::MaybeTimeoutClaimableHTLC { .. }| - Balance::MaybePreimageClaimableHTLC { .. } - => 0, + Balance::MaybeTimeoutClaimableHTLC { amount_satoshis, outbound_payment, .. } + => if *outbound_payment { 0 } else { *amount_satoshis }, + Balance::MaybePreimageClaimableHTLC { .. } => 0, } } } @@ -1719,9 +1723,10 @@ impl ChannelMonitor { impl ChannelMonitorImpl { /// Helper for get_claimable_balances which does the work for an individual HTLC, generating up /// to one `Balance` for the HTLC. - fn get_htlc_balance(&self, htlc: &HTLCOutputInCommitment, holder_commitment: bool, - counterparty_revoked_commitment: bool, confirmed_txid: Option) - -> Option { + fn get_htlc_balance(&self, htlc: &HTLCOutputInCommitment, source: Option<&HTLCSource>, + holder_commitment: bool, counterparty_revoked_commitment: bool, + confirmed_txid: Option + ) -> Option { let htlc_commitment_tx_output_idx = if let Some(v) = htlc.transaction_output_index { v } else { return None; }; @@ -1858,10 +1863,19 @@ impl ChannelMonitorImpl { confirmation_height: conf_thresh, }); } else { + let outbound_payment = match source { + None => { + debug_assert!(false, "Outbound HTLCs should have a source"); + true + }, + Some(&HTLCSource::PreviousHopData(_)) => false, + Some(&HTLCSource::OutboundRoute { .. }) => true, + }; return Some(Balance::MaybeTimeoutClaimableHTLC { amount_satoshis: htlc.amount_msat / 1000, claimable_height: htlc.cltv_expiry, payment_hash: htlc.payment_hash, + outbound_payment, }); } } else if let Some(payment_preimage) = self.payment_preimages.get(&htlc.payment_hash) { @@ -1934,10 +1948,12 @@ impl ChannelMonitor { macro_rules! walk_htlcs { ($holder_commitment: expr, $counterparty_revoked_commitment: expr, $htlc_iter: expr) => { - for htlc in $htlc_iter { + for (htlc, source) in $htlc_iter { if htlc.transaction_output_index.is_some() { - if let Some(bal) = us.get_htlc_balance(htlc, $holder_commitment, $counterparty_revoked_commitment, confirmed_txid) { + if let Some(bal) = us.get_htlc_balance( + htlc, source, $holder_commitment, $counterparty_revoked_commitment, confirmed_txid + ) { res.push(bal); } } @@ -1968,9 +1984,9 @@ impl ChannelMonitor { } } if Some(txid) == us.current_counterparty_commitment_txid || Some(txid) == us.prev_counterparty_commitment_txid { - walk_htlcs!(false, false, counterparty_tx_htlcs.iter().map(|(a, _)| a)); + walk_htlcs!(false, false, counterparty_tx_htlcs.iter().map(|(a, b)| (a, b.as_ref().map(|b| &**b)))); } else { - walk_htlcs!(false, true, counterparty_tx_htlcs.iter().map(|(a, _)| a)); + walk_htlcs!(false, true, counterparty_tx_htlcs.iter().map(|(a, b)| (a, b.as_ref().map(|b| &**b)))); // The counterparty broadcasted a revoked state! // Look for any StaticOutputs first, generating claimable balances for those. // If any match the confirmed counterparty revoked to_self output, skip @@ -2010,7 +2026,7 @@ impl ChannelMonitor { } found_commitment_tx = true; } else if txid == us.current_holder_commitment_tx.txid { - walk_htlcs!(true, false, us.current_holder_commitment_tx.htlc_outputs.iter().map(|(a, _, _)| a)); + walk_htlcs!(true, false, us.current_holder_commitment_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref()))); if let Some(conf_thresh) = pending_commitment_tx_conf_thresh { res.push(Balance::ClaimableAwaitingConfirmations { amount_satoshis: us.current_holder_commitment_tx.to_self_value_sat, @@ -2020,7 +2036,7 @@ impl ChannelMonitor { found_commitment_tx = true; } else if let Some(prev_commitment) = &us.prev_holder_signed_commitment_tx { if txid == prev_commitment.txid { - walk_htlcs!(true, false, prev_commitment.htlc_outputs.iter().map(|(a, _, _)| a)); + walk_htlcs!(true, false, prev_commitment.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref()))); if let Some(conf_thresh) = pending_commitment_tx_conf_thresh { res.push(Balance::ClaimableAwaitingConfirmations { amount_satoshis: prev_commitment.to_self_value_sat, @@ -2043,13 +2059,22 @@ impl ChannelMonitor { } } else { let mut claimable_inbound_htlc_value_sat = 0; - for (htlc, _, _) in us.current_holder_commitment_tx.htlc_outputs.iter() { + for (htlc, _, source) in us.current_holder_commitment_tx.htlc_outputs.iter() { if htlc.transaction_output_index.is_none() { continue; } if htlc.offered { + let outbound_payment = match source { + None => { + debug_assert!(false, "Outbound HTLCs should have a source"); + true + }, + Some(HTLCSource::PreviousHopData(_)) => false, + Some(HTLCSource::OutboundRoute { .. }) => true, + }; res.push(Balance::MaybeTimeoutClaimableHTLC { amount_satoshis: htlc.amount_msat / 1000, claimable_height: htlc.cltv_expiry, payment_hash: htlc.payment_hash, + outbound_payment, }); } else if us.payment_preimages.get(&htlc.payment_hash).is_some() { claimable_inbound_htlc_value_sat += htlc.amount_msat / 1000; diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 60caed4d..685ec75c 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -346,11 +346,13 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) { amount_satoshis: 3_000, claimable_height: htlc_cltv_timeout, payment_hash, + outbound_payment: true, }; let sent_htlc_timeout_balance = Balance::MaybeTimeoutClaimableHTLC { amount_satoshis: 4_000, claimable_height: htlc_cltv_timeout, payment_hash: timeout_payment_hash, + outbound_payment: true, }; let received_htlc_balance = Balance::MaybePreimageClaimableHTLC { amount_satoshis: 3_000, @@ -746,11 +748,13 @@ fn do_test_balances_on_local_commitment_htlcs(anchors: bool) { amount_satoshis: 10_000, claimable_height: htlc_cltv_timeout, payment_hash, + outbound_payment: true, }; let htlc_balance_unknown_preimage = Balance::MaybeTimeoutClaimableHTLC { amount_satoshis: 20_000, claimable_height: htlc_cltv_timeout, payment_hash: payment_hash_2, + outbound_payment: true, }; let commitment_tx_fee = chan_feerate * @@ -893,6 +897,7 @@ fn test_no_preimage_inbound_htlc_balances() { amount_satoshis: 10_000, claimable_height: htlc_cltv_timeout, payment_hash: to_b_failed_payment_hash, + outbound_payment: true, }; let a_received_htlc_balance = Balance::MaybePreimageClaimableHTLC { amount_satoshis: 20_000, @@ -908,6 +913,7 @@ fn test_no_preimage_inbound_htlc_balances() { amount_satoshis: 20_000, claimable_height: htlc_cltv_timeout, payment_hash: to_a_failed_payment_hash, + outbound_payment: true, }; // Both A and B will have an HTLC that's claimable on timeout and one that's claimable if they @@ -1204,14 +1210,17 @@ fn do_test_revoked_counterparty_commitment_balances(anchors: bool, confirm_htlc_ amount_satoshis: 2_000, claimable_height: missing_htlc_cltv_timeout, payment_hash: missing_htlc_payment_hash, + outbound_payment: true, }, Balance::MaybeTimeoutClaimableHTLC { amount_satoshis: 4_000, claimable_height: htlc_cltv_timeout, payment_hash: timeout_payment_hash, + outbound_payment: true, }, Balance::MaybeTimeoutClaimableHTLC { amount_satoshis: 5_000, claimable_height: live_htlc_cltv_timeout, payment_hash: live_payment_hash, + outbound_payment: true, }]), sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances())); @@ -1754,10 +1763,12 @@ fn do_test_revoked_counterparty_aggregated_claims(anchors: bool) { amount_satoshis: 4_000, claimable_height: htlc_cltv_timeout, payment_hash: revoked_payment_hash, + outbound_payment: true, }, Balance::MaybeTimeoutClaimableHTLC { amount_satoshis: 3_000, claimable_height: htlc_cltv_timeout, payment_hash: claimed_payment_hash, + outbound_payment: true, }]), sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances())); -- 2.30.2