Skip to content

Conversation

@tnull
Copy link
Collaborator

@tnull tnull commented Feb 5, 2025

Previously, we'd skip handling any BumpTransactionEvent if the counterparty was trusted. However, this might result in funds getting stolen by the counterparty in the ContentiousClaimable case, as the counterparty is never expected to spend these back to us. Here, we therefore begin only skipping the bump events for the ChannelClose variant, and always try to claim funds requiring HTLCResolution.

@tnull tnull added this to the 0.5 milestone Feb 5, 2025
@tnull tnull requested review from TheBlueMatt and wpaulino February 5, 2025 14:21
Previously, we'd skip handling any `BumpTransactionEvent` if the
counterparty was trusted. However, this might result in funds getting
stolen by the counterparty in the `ContentiousClaimable` case, as the
counterparty is never expected to spend these back to us. Here, we
therefore begin only skipping the bump events for the `ChannelClose`
variant, and always try to claim funds requiring `HTLCResolution`.
@tnull tnull force-pushed the 2025-02-bump-on-htlcresolution-nonetheless branch from 5fd281c to 7a36e5b Compare February 5, 2025 14:22
@TheBlueMatt
Copy link

However, this might result in funds getting stolen by the counterparty in the ContentiousClaimable case

nit: its not strictly "stealing", in the common case I'd imagine the counterparty fails back to the sender so the payment simply never completed.

@tnull
Copy link
Collaborator Author

tnull commented Feb 5, 2025

nit: its not strictly "stealing", in the common case I'd imagine the counterparty fails back to the sender so the payment simply never completed.

Noted, will probably leave as is since the 'mistake' is only in the commit/PR description.

Copy link

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me.

self.config.anchor_channels_config.as_ref()
{
if anchor_channels_config
.trusted_peers_no_reserve
Copy link

@wpaulino wpaulino Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be renamed now? You will need a reserve to handle the HTLC events. I guess you could use the to_remote funds from the confirmed commitment, but I'm not sure if ldk-node is wired up to do so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be renamed now? You will need a reserve to handle the HTLC events.

Well, the no_reserve part refers to the fact that we don't account for these peers when calculating how much reserve we should keep. Essentially, the change in this PR makes the HTLCResolution case behave similar to setting the reserve to Some(0): we'd try to handle the events if we have funds, but don't keep a dedicated reserve for them.

I guess you could use the to_remote funds from the confirmed commitment, but I'm not sure if ldk-node is wired up to do so.

Yeah, I don't think there is a way to do this in LDK generally, as the respective events are entirely separate? I guess now, if we wouldn't have any reserve, we would still first sweep the commitment tx and then once the sweep confirms would be able to still reuse the funds to sweep the HTLC claim? FWIW, that might be yet another motivation for finally allowing to override payment_key to get rid of the additional sweep.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess now, if we wouldn't have any reserve, we would still first sweep the commitment tx and then once the sweep confirms would be able to still reuse the funds to sweep the HTLC claim?

Yeah, it would take a few blocks to happen though since we wait for 6 confs on the commitment to yield the spendable output descriptor, plus however long that spend takes to confirm. By then, we may have lost the HTLC to the counterparty already.

FWIW, that might be yet another motivation for finally allowing to override payment_key to get rid of the additional sweep.

It ties your current L1 wallet to the L2 state, which might not be ideal if you want to switch your L1 wallet (e.g., upgrading to a taproot-based wallet, compromised keys, etc.) while still keeping your L2 state intact. We would probably also want the ability to dynamically change payment_key at the protocol level, likely using quiescence.

@tnull tnull merged commit 9f8d30a into lightningdevkit:main Feb 6, 2025
10 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants