Document duplicate HTLC handling on revoked commitments
What the developer wrote
Authored by auto-pr-bot
Document duplicate HTLC handling on revoked commitments
@matt said:
> Bitcoin red team flagged that if there's two HTLCs with identical
> `(payment_hash, amount)`s on a channel and the counterparty broadcasts a
> revoked commitment tx and then claims both we will only propagate the
> claim backwards on one of the two. This is, of course, not a
> "vulnerability" because we'll still get our money by taking our
> counterparty's funds via the revocation path, but it might be nice to
> claim both HTLCs cause, why not, free money.
>
> @auto-pr-bot can you do this? try to keep the code change small and
> contained, if its too big I'll respond in review and we'll switch to
> just leaving a comment instead.
@matt said:
> @auto-pr-bot try again.
@matt asked:
> Turned out to be way too much code. @auto-pr-bot can you instead just
> add a comment?
HTLC outputs on a revoked counterparty commitment transaction don't come
with an `HTLCSource`, so when one is spent we search the counterparty's
non-revoked commitment transactions for an HTLC with a matching
direction, payment hash and value. If a channel had two HTLCs which
share both a payment hash and a value, spends of either output match the
same first source.
Thus, if the counterparty claims both outputs with the preimage, we
propagate only one fulfillment upstream and fail the second upstream
HTLC, even though each matching upstream HTLC is independently claimable
once we have the preimage. Because the counterparty broadcasted a
revoked commitment we still recover the channel balance via the
revocation path, so this doesn't leave us out of pocket, but it does
give up an upstream claim we were entitled to.
Resolving the outputs against distinct sources means resolving every
matching source when a spend reveals a preimage and deterministically
matching duplicate outputs to sources otherwise, which is more than the
lookup here can do. Describe the behavior in a comment instead so the
next reader doesn't have to rediscover it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fixes #4904
Requested-by: @matt
Link: https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/issues/4904#issuecomment-442538
What changed, and why it matters
This commit only adds a code comment explaining an existing quirk: if a revoked old channel transaction contains two identical-looking payment forwards and the other side claims both, the software may only claim one upstream while letting the other fail. The commit itself does not change behavior, and the comment notes the user still recovers funds through the revocation penalty path, so no money is lost. It is a documentation-only change for a known, low-consequence edge case.
No immediate action required; this is a documentation-only commit. If the project later wants to maximize upstream claims, it should implement deterministic duplicate-HTLC-to-source mapping and resolve all matching sources when a preimage is revealed, as described in the comment.
Security signals we found
Behavioral quirk in revoked-commitment HTLC resolution documented
Duplicate (payment_hash, amount) HTLCs can map to the same upstream source
Potential missed upstream preimage claim on second identical HTLC
Funds still recovered via revocation path, so no direct loss
No functional code change; comment-only commit
Evidence from the diff
The patch adds a 19-line comment in ChannelMonitorImpl’s revoked-commitment HTLC handling. It documents that HTLCSource data is dropped when a counterparty commitment is revoked, so spent HTLC outputs are matched by direction, payment_hash, and amount against non-revoked commitments. If two HTLCs share payment_hash and amount, the first matching source is used for both spends. When the counterparty claims both with the preimage, only one fulfillment is propagated upstream; the second upstream HTLC is failed. The comment states this is acceptable because the revoked commitment triggers the revocation path, recovering the channel balance. No code logic is changed.
Changed components
lightning/src/chain/channelmonitor.rsChannelMonitorImpl revoked commitment HTLC handlingInspect captured patch +19 / −0
### lightning/src/chain/channelmonitor.rs
@@ -6251,6 +6251,25 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}
}
+ // HTLC outputs on a revoked counterparty commitment transaction don't come with an
+ // `HTLCSource` (we drop those when the commitment is revoked), so when one is spent we
+ // have to search the counterparty's non-revoked commitment transactions for an HTLC
+ // with a matching direction, payment hash and value to figure out which HTLC upstream
+ // the spend resolves.
+ //
+ // Note that if the channel had two HTLCs which share both a payment hash and a value,
+ // spends of either output will find the same first matching source here. Thus, if our
+ // counterparty claims both outputs with the preimage, we only propagate one
+ // fulfillment upstream and the second upstream HTLC is failed rather than claimed,
+ // even though each matching upstream HTLC is independently claimable once we have the
+ // preimage.
+ //
+ // Because the counterparty broadcasted a revoked commitment transaction we still
+ // expect to recover the channel's balance via the revocation path, so dropping one
+ // fulfillment doesn't leave us out of pocket, but it does give up an upstream claim we
+ // were entitled to. Doing better means resolving every matching source when the spend
+ // reveals a preimage and matching duplicate outputs to sources deterministically
+ // otherwise, which is more machinery than the lookup here can offer.
macro_rules! check_htlc_valid_counterparty {
($htlc_output: expr, $per_commitment_data: expr) => {
for &(ref pending_htlc, ref pending_source) in $per_commitment_data {Why this scored 28/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.