Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

contractcourt: force the sweeper to always resolve outgoing HTLCs #7726

Merged
merged 2 commits into from
May 29, 2023

Conversation

Roasbeef
Copy link
Member

In this commit, we attempt to fix an issue that may lead to force closes due to small value HTLCs. The sweeper has built in a "negative yield" heuristic where it won't sweep something that'll result in paying more fees than the HTLC amount. However for HTLCs, we want to always sweep them, as we don't cancel back the HTLCs before the outgoing contract is fully resolved.

In the future, we'll start to make more economical decisions about if we should go to chain at all for small value HTLCs, and also do things like cancel back early if the HTLC is small and we think we might be contested by chain fees.

@Roasbeef Roasbeef added this to the v0.16.3 milestone May 24, 2023
Copy link
Member

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

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

LGTM🙏

@ziggie1984
Copy link
Collaborator

Hmm, so there exists a workaround for canceling back incoming HTLCs without uneconomically sweeping the Timeout Transaction tho? Just a short reconnect in the period where the htlc runs in the 13 blocks range before timeout. #7701

I tend to not go in this direction but rather fail the incoming back always without the need to reconnect when it hits the 13 block range before timeout? Wdyt? Rather than paying a negative yielding outgoing HLTC, just fail back the incoming and you are good, because you don't care about the Outgoing HTLC amount anyways if you were ready to sweep in negative yielding?

@yyforyongyu
Copy link
Member

Rather than paying a negative yielding outgoing HLTC, just fail back the incoming and you are good, because you don't care about the Outgoing HTLC amount anyways if you were ready to sweep in negative yielding?

This would be the ideal case, that we just cancel back the HTLC when we are sure about it. To safely implement it we need to investigate the full settlement flow more.

@ziggie1984
Copy link
Collaborator

agree, definitely in favour of investigating the potential of failing back no matter what, before we merge this.

@saubyk saubyk linked an issue May 26, 2023 that may be closed by this pull request
In this commit, we attempt to fix an issue that may lead to force closes due
to small value HTLCs. The sweeper has built in a "negative yield" heuristic
where it won't sweep something that'll result in paying more fees than the
HTLC amount. However for HTLCs, we want to always sweep them, as we don't
cancel back the HTLCs before the outgoing contract is fully resolved.

In the future, we'll start to make more uneconomical decisions about if we
should go to chain at all for small value HTLCs, and also do things like
cancel back early if the HTLC is small and we think we might be contested by
chain fees.
@Roasbeef
Copy link
Member Author

Roasbeef commented May 26, 2023

@ziggie1984 I think that's a distinct heuristic. This is a smaller fix to ensure that if we did decide to go to chain, we clean up all the outputs. This is also a more fundamental issue in that it can lead to force close cascades.

See this for a PoC of not going to chain at all for them, and cancelling back early. There's also another variant where if things are taking too long, then we cancel back to avoid the cascade. We can also just cancel back as soon as we decide to broadcast as well. IMO all these are larger changes compared to this one, that we're actively testing, and follows from first principles (if we don't resolve rn fully, then we never cancel back).

The thing about cancelling back early is that:

  1. That outgoing HTLC is still there and brings down your available slots, so you can't do it forever.
  2. With each outgoing HTLC you hold onto, you increase your risk exposure for the worst case: the pre-image does arrive for all of them, so then you're down that amount.

So an implementation needs to weigh: the HTLC amt, the on chain closing cost, the amt we'd forfeit, the amt we'd gain in fees, the current fee rate, the number of HTLCs we've already forfeited, etc. I think it's tractable to create something that factors all that in, but this seems to resolve a clear issue that's causing chain issues today. So IMO we should move forward with this, and examine the PR I linked above as a staging ground to make better decisions bound in the empirical unit economics of the network/chain.

@ziggie1984
Copy link
Collaborator

I think my reason was a bit to vague sry for that, and consider my opinion with care I am still new to this topic just throwing in some ideas and totally fine with this PR to be merged.

So my idea was only referring to the situation where we already went onchain for the outgoing HTLC and now have an Incoming HTLC which is still on an active Channel waiting for the Outgoing to be resolved. (I think you read my initial comment as if the Channel of the Outgoing HTLC is still online, but I was really talking about where we already decided to go onchain because of a timeout).

Let's say the channel went onchain with 2 outgoing HTLCs, and both of them are negative yielding in the first-stage sweep, I would propose failing the corresponding incoming htlc back before the Incoming HTLC would cause a ForceClose on the Incoming Channel. Some broadcasting delta before (reuse DefaultFinalCltvRejectDelta)

Pros: We do not spend sats for a negative yielding outgoing HTLC sweep and also do not loose the Incoming Channel

Cons: We do fail back the Incoming HTLC and the Outgoing HTLC is swept by the preimage some time after. But is this really worse than sweeping in negative yielding that's the question which has to be answered, I think it is.

Apart from that, this behaviour can already be imitated by reconnecting the Peer in the DefaultFinalCltvRejectDelta Period. So I would propose instead of Forcing the Sweep, we keep as is, but we fail the Incoming HTLC when it hits the Grace Period, however we do not discard the Outgoing HTLC just in case fee-levels come down and the preimage was still not revealed, giving us the possibility to still get our Outgoing Funds back.

@Roasbeef
Copy link
Member Author

Let's say the channel went onchain with 2 outgoing HTLCs, and both of them are negative yielding in the first-stage sweep, I would propose failing the corresponding incoming htlc back before the Incoming HTLC would cause a ForceClose on the Incoming Channel.

Ah ok I understand your suggestion properly now, thanks for that clarification. I think that suggestion is currently being tracked in: #7683.

@Roasbeef Roasbeef merged commit 20c1af9 into lightningnetwork:master May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants