Skip to content

Conversation

@t-bast
Copy link
Member

@t-bast t-bast commented Mar 19, 2025

While auditing our force-close code, I discovered that we were missing tests for scenarios where we must fail HTLCs back upstream. There are also important properties of various components that are crucial to funds safety and weren't explicitly spelled out or tested (such as the fact that redundant HTLC settlement commands are simply ignored in favor of the first settlement command created).

I also took this opportunity to slightly refactor some of the closing code, improve comments and simplify code that tried being too clever, which could bite us in the future when introducing additional channel types.

Below is an overview of how eclair ensures that channels are safe, at a high-level. I would encourage reviewers to take their time going through this and do their own audit of our force-close code: this is important to get right and all contributors should master that part of the code to ensure they don't introduce security issues!

Watching funding transactions

Whenever we open a channel (single-funded or dual-funded):

  • if 0-conf, we set WatchPublished
  • if not 0-conf, we set WatchFundingConfirmed
  • if single-funded, we do this:
    • when receiving funding_created when we're not the opener
    • when receiving funding_signed when we're the opener
  • if dual-funded, we do this:
    • before sending our tx_signatures
  • when WatchPublished triggers, we set WatchFundingConfirmed
  • when WatchFundingConfirmed triggers, we set WatchFundingSpent
  • whenUnhandled has handlers for WatchPublishedTriggered and WatchFundingConfirmedTriggered
    • this ensures that this happens regardless of the channel state (offline, syncing, etc)

Whenever we make a splice transaction:

  • before sending our tx_signatures:
    • if 0-conf, we set WatchPublished
    • if not 0-conf, we set WatchFundingConfirmed
  • when WatchPublished triggers, we set WatchFundingConfirmed
  • when WatchFundingConfirmed triggers, we set WatchFundingSpent
  • using the same code paths as channel open

Whenever we restart:

  • we send INPUT_RESTORED, in which:
    • for each unconfirmed funding/splice transaction:
      • if 0-conf, we set WatchPublished
      • if not 0-conf, we set WatchFundingConfirmed
    • for each confirmed funding/splice transaction:
      • if the channel is closing with a confirmed commitment transaction:
        • we don't set WatchFundingSpent
        • we republish the 2nd-stage transactions
        • we watch unspent outputs of the commitment transaction
      • otherwise:
        • we set WatchFundingSpent

Handling closing transactions

We detect that a funding transaction (including splice transactions) is spent by receiving WatchFundingSpentTriggered:

  • in whenUnhandled to ensure that this happens regardless of the channel state
    • we identify if the spending transaction is:
      • the local, remote, next-remote or a revoked commitment spending the latest funding transaction
      • or a commitment for a previous funding transaction, in which case:
        • we watch confirmation of this transaction with WatchAlternativeCommitTxConfirmed
        • we try to publish our local commitment for the latest funding transaction
        • when receiving WatchAlternativeCommitTxConfirmedTriggered:
          • it invalidates every other splice transaction
          • we identify if it is the local, remote, next-remote or a revoked commitment
  • we spend every output of the commitment transaction that we can spend
    • if we receive a preimage from a downstream channel:
      • we publish the corresponding HTLC-success transactions
  • we watch all HTLC outputs of the commitment transaction
    • when we detect a transaction spending one of those outputs:
      • we try to extract a payment preimage from it
      • if we succeed, we relay it upstream
  • when the commitment transaction confirms:
    • for every HTLC that does not appear in that commitment transaction:
      • we send a command to fail it upstream
      • this will be a no-op if we previously received the preimage (and thus sent a command to fulfill)
  • we consider the channel closed (and stop watching it on-chain) when:
    • a commitment transaction has been confirmed
    • we've spent our main output (if any) and this has confirmed
    • all HTLC outputs have been spent and are confirmed
    • if this was a local commitment:
      • all HTLC transactions have been spent by a 3rd-stage transaction
      • those transactions are confirmed

While auditing our force-close code, I discovered that we were missing
tests for scenarios where we must fail HTLCs back upstream. There are
also important properties of various components that are crucial to
funds safety and weren't explicitly spelled out or tested (such as the
fact that redundant HTLC settlement commands are simply ignored in
favor of the first settlement command created).

I also took this opportunity to slightly refactor some of the closing
code, improve comments and simplify code that tried being too clever,
which could bite us in the future when introducing additional channel
types.
@t-bast t-bast force-pushed the additional-force-close-tests branch from 98ad619 to 81a1a07 Compare March 24, 2025 08:47
t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Mar 25, 2025
This commit is importing test updates and refactoring to the closing
helpers from ACINQ/eclair#3040

We don't relay HTLCs, so we don't have an upstream channel to relay
preimages to, but it's important to relay preimages to the payment
handler to correctly mark payments as succeeded (or failed) and store
the proof of payment.
t-bast added 3 commits March 25, 2025 16:04
- Add preimage helper to `Scripts.scala`
- Split `failedIncomingHtlcs` and `nonRelayedHtlcs`
- Improve comment about failing HTLCs when revoked commit confirms
When we detect a transaction spending an HTLC output of a revoked
commit, we now always try to generate the corresponding penalty txs.
If that fails, it means that the spending transaction was actually
one of our HTLC-penalty txs.
t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Mar 25, 2025
This commit is importing test updates and refactoring to the closing
helpers from ACINQ/eclair#3040

We don't relay HTLCs, so we don't have an upstream channel to relay
preimages to, but it's important to relay preimages to the payment
handler to correctly mark payments as succeeded (or failed) and store
the proof of payment.
@t-bast t-bast merged commit fbc7004 into master Mar 27, 2025
1 check passed
@t-bast t-bast deleted the additional-force-close-tests branch March 27, 2025 08:48
t-bast added a commit that referenced this pull request Mar 27, 2025
t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Mar 28, 2025
This commit is importing test updates and refactoring to the closing
helpers from ACINQ/eclair#3040

We don't relay HTLCs, so we don't have an upstream channel to relay
preimages to, but it's important to relay preimages to the payment
handler to correctly mark payments as succeeded (or failed) and store
the proof of payment.
docker-lordvf6ik added a commit to docker-lordvf6ik/lightning-kmp that referenced this pull request Sep 28, 2025
This commit is importing test updates and refactoring to the closing
helpers from ACINQ/eclair#3040

We don't relay HTLCs, so we don't have an upstream channel to relay
preimages to, but it's important to relay preimages to the payment
handler to correctly mark payments as succeeded (or failed) and store
the proof of payment.
ebonyschneider462359 added a commit to ebonyschneider462359/lightning-kmp that referenced this pull request Oct 10, 2025
This commit is importing test updates and refactoring to the closing
helpers from ACINQ/eclair#3040

We don't relay HTLCs, so we don't have an upstream channel to relay
preimages to, but it's important to relay preimages to the payment
handler to correctly mark payments as succeeded (or failed) and store
the proof of payment.
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.

5 participants