Skip to content

On-chain resolution flow ignores htlc interceptor settles #6040

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

Closed
joostjager opened this issue Nov 30, 2021 · 6 comments · Fixed by #6219
Closed

On-chain resolution flow ignores htlc interceptor settles #6040

joostjager opened this issue Nov 30, 2021 · 6 comments · Fixed by #6219
Assignees
Labels
contracts HTLC htlcswitch invoices P1 MUST be fixed or reviewed possible-bug rpc Related to the RPC interface
Milestone

Comments

@joostjager
Copy link
Contributor

With the htlc interceptor API it is possible to settle intercepted htlcs with a preimage.

It seems possible though that the interceptor signals settle, but it turns out to be impossible to settle the htlc. One reason for this is that the peer went offline before the settle was fully locked in. The interceptor will not be aware of this and assume the htlc is indeed settled.

When the peer doesn't come online anymore, lnd will consider going to chain with the channel. This decision is made in the channel arbitrator and depends on the availability of the preimage. The channel arbitrator currently isn't aware of the settle signal given by the interceptor and will not go to chain even though it should.

When the htlc times out, the counterparty can claim back the htlc. The interceptor will never find out that this happened and keep assuming that the money is in the pocket.

Similarly the incoming onchain resolver needs the preimage in order to settle the htlc that the interceptor considers settled.

@Roasbeef
Copy link
Member

Roasbeef commented Dec 2, 2021

Related issue: #4910

@joostjager
Copy link
Contributor Author

Repro scenario:

  • Nodes A, B and C
  • Node A is patched with:
diff --git a/lnwallet/channel.go b/lnwallet/channel.go
index ac72852d5..2693c729c 100644
--- a/lnwallet/channel.go
+++ b/lnwallet/channel.go
@@ -5259,6 +5259,8 @@ func (lc *LightningChannel) SettleHTLC(preimage [32]byte,
 // log, and error is returned. Similarly if the preimage is invalid w.r.t to
 // the referenced of then a distinct error is returned.
 func (lc *LightningChannel) ReceiveHTLCSettle(preimage [32]byte, htlcIndex uint64) error {
+       return errors.New("NEVER SETTLE")
+
        lc.Lock()
        defer lc.Unlock()
  • Create invoice on node C with preimage P
  • Run htlc interceptor on node B and settle htlcs with hash H(P)
  • Pay invoice from node A

Then what happens is:

  • B intercepts and settles
  • A fails the channel and goes to chain because of the patch
  • Block is mined
  • B sees the force-close and spins up an htlcIncomingContestResolve.
  • More blocks are mined
  • B never queries the interceptor for the preimage and lets the htlc timeout, even though the interceptor settled it previously.

@joostjager
Copy link
Contributor Author

joostjager commented Jan 11, 2022

Another potential problem here is that after an lnd restart, the htlc interceptor may not be quick enough to register itself. When a link comes up and htlcs are replayed and the interceptor isn't there, lnd could fail back the htlc. Even though the interceptor settled it on an earlier run (but that settle never made it to the database for some reason).

@carlaKC carlaKC self-assigned this Jan 11, 2022
@champo
Copy link
Contributor

champo commented Jan 12, 2022

Another potential problem here is that after an lnd restart, the htlc interceptor may not be quick enough to register itself. When a link comes up and htlcs are replayed and the interceptor isn't there, lnd could fail back the htlc. Even though the interceptor settled it on an earlier run (but that settle never made it to the database for some reason).

We've had this happen to us. We have a changeset that adds a flag that registers the interceptor queue as soon as the node starts to avoid it. Whenever there's HTLCs that couldn't be properly resolved, on startup they get queued and when the interceptor propper is registered via RPC, it gets called with all the HTLCs in the queue. The changeset is rather small & simple, I can submit it's interesting enough to merge.

@joostjager
Copy link
Contributor Author

joostjager commented Jan 14, 2022

Created separate issue for startup race condition: #6161

@joostjager
Copy link
Contributor Author

I think the missing repro tag can be removed now

@joostjager joostjager linked a pull request Jan 31, 2022 that will close this issue
@Roasbeef Roasbeef added this to the v0.15.0 milestone Feb 2, 2022
@Neil-LL Neil-LL added the P1 MUST be fixed or reviewed label Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contracts HTLC htlcswitch invoices P1 MUST be fixed or reviewed possible-bug rpc Related to the RPC interface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants