Skip to content

Commit

Permalink
Include an outbound_payment flag in MaybeTimeoutClaimableHTLC
Browse files Browse the repository at this point in the history 
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`.
  • Loading branch information
@TheBlueMatt
TheBlueMatt committed Sep 28, 2023
1 parent 082a19b commit 6921d46
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 13 deletions.
51 changes: 38 additions & 13 deletions lightning/src/chain/channelmonitor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
}
}
}
Expand Down Expand Up @@ -1674,9 +1678,10 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
impl<Signer: WriteableEcdsaChannelSigner> 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; };

Expand Down Expand Up @@ -1801,10 +1806,19 @@ impl<Signer: WriteableEcdsaChannelSigner> 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) {
Expand Down Expand Up @@ -1878,10 +1892,12 @@ impl<Signer: WriteableEcdsaChannelSigner> 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);
}
}
Expand Down Expand Up @@ -1912,9 +1928,9 @@ impl<Signer: WriteableEcdsaChannelSigner> 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
Expand Down Expand Up @@ -1954,7 +1970,7 @@ impl<Signer: WriteableEcdsaChannelSigner> 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,
Expand All @@ -1964,7 +1980,7 @@ impl<Signer: WriteableEcdsaChannelSigner> 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,
Expand All @@ -1987,13 +2003,22 @@ impl<Signer: WriteableEcdsaChannelSigner> 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;
Expand Down
11 changes: 11 additions & 0 deletions lightning/src/ln/monitor_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,11 +286,13 @@ fn do_test_claim_value_force_close(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,
Expand Down Expand Up @@ -627,11 +629,13 @@ fn test_balances_on_local_commitment_htlcs() {
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,
};

assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations {
Expand Down Expand Up @@ -757,6 +761,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,
Expand All @@ -772,6 +777,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
Expand Down Expand Up @@ -1063,14 +1069,17 @@ fn do_test_revoked_counterparty_commitment_balances(confirm_htlc_spend_first: bo
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()));

Expand Down Expand Up @@ -1500,10 +1509,12 @@ fn test_revoked_counterparty_aggregated_claims() {
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()));

Expand Down

0 comments on commit 6921d46

Please sign in to comment.