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<HTLCSource>` 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.

Author: TheBlueMatt

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<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 	/// 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<Txid>)
-	-> Option<Balance> {
+	fn get_htlc_balance(&self, htlc: &HTLCOutputInCommitment, source: Option<&HTLCSource>,
+		holder_commitment: bool, counterparty_revoked_commitment: bool,
+		confirmed_txid: Option<Txid>
+	) -> Option<Balance> {
 		let htlc_commitment_tx_output_idx =
 			if let Some(v) = htlc.transaction_output_index { v } else { return None; };
 
@@ -2099,10 +2110,19 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					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<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 
 		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<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 					}
 				}
 				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<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 				}
 				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<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 				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<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 			}
 		} 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;

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()));