From 6a34baa270fe4fa846754b0f99a97562a9f53397 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 | 63 ++++++++++++++++++++------- lightning/src/ln/monitor_tests.rs | 11 +++++ 2 files changed, 58 insertions(+), 16 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 10aab8287..fb2f11458 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -664,6 +664,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 @@ -693,9 +697,15 @@ pub enum Balance { } impl Balance { - /// The amount claimable, in satoshis. This excludes balances that we are unsure if we are able - /// to claim, this is because we are waiting for a preimage or for a timeout to expire. For more - /// information on these balances see [`Balance::MaybeTimeoutClaimableHTLC`] and + /// The amount claimable, in satoshis. + /// + /// For outbound payments, this excludes the balance from the possible HTLC timeout. + /// + /// For forwarded payments, this includes the balance from the possible HTLC timeout as + /// (to be conservative) that balance does not include routing fees we'd earn if we'd claim + /// the balance from a preimage in a successful forward. + /// + /// For more information on these balances see [`Balance::MaybeTimeoutClaimableHTLC`] and /// [`Balance::MaybePreimageClaimableHTLC`]. /// /// On-chain fees required to claim the balance are not included in this amount. @@ -706,9 +716,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, } } } @@ -1960,9 +1970,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; }; @@ -2099,10 +2110,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) { @@ -2175,10 +2195,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); } } @@ -2209,9 +2231,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 @@ -2251,7 +2273,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, @@ -2261,7 +2283,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, @@ -2284,13 +2306,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 6f9a14577..13a7f21ae 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -402,11 +402,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, @@ -803,11 +805,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 * @@ -950,6 +954,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, @@ -965,6 +970,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 @@ -1261,14 +1267,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())); @@ -1811,10 +1820,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.39.5