lnsweep: simplify maybe_reveal_preimage_for_htlc by SomberNight · Pull Request #10442 · spesmilo/electrum

SomberNight · 2026-01-26T18:25:32Z

"When should we reveal preimages onchain?"
This commit tries to simplify the thinking to:

we can reveal preimages (actually in any context) if they are already public
a preimage is public if any other lightning node knows it besides us
- if we learn the preimage from another LN node, it is public
- if we send update_fulfill_htlc, it becomes public
- if we see a preimage onchain, it is public
we can't restrict revealing preimages to only already public ones both onchain and offchain: there needs to be a state transition out of non-public for our own invoices
- we allow the state transition when we send update_fulfill_htlc (offchain)
  - this logic is unchanged
- ~~then onchain we can simplify: enforce preimage already being public~~
  - EDIT: several of the regtest tests expect the code to make the other choice, as that's what we've been doing so far. It is simpler for now not to change this logic.
in lnsweep._maybe_reveal_preimage_for_htlc:
- partial mpp check is not relevant if preimage is already public
- let's just always do KeepWatchingTXO, for sanity/safety

SomberNight · 2026-01-26T18:43:35Z

electrum/lnsweep.py

+        # - if this is an incoming payment, where
+        #   - all the HTLCs have already been received (covering invoice amt) and are pending currently, and
+        #   - no one knows the preimage besides us, and
+        #   - this channel was just force-closed by either party
+        #   -> then we can freely decide whether to claim the HTLCs or let them time out.
+        #      Both choices are valid and safe. *Here* it is simpler to implement
+        #      letting the HTLC time out, so that's what we do.


note: several of the regtest tests expect the code to make the other choice, as that's what we've been doing so far

Besides the simplicity aspect you mention i can also think of these tradeoffs:
Claiming may consume a large amount of the htlcs value, esp during high mining fees, so it might be preferable to retry another lightning payment to pay less fees and let the sender pay the mining fees to time out the htlcs.
OTOH not claiming them would cause the payment to be stuck until the htlcs timed out, which is also quite bad experience for both sides of the payment.
Though in practice i guess it should not happen often, so simplicity might outweigh the cost of any potential UX improvement that gets triggered only rarely.

note: decided not to change this behaviour for now

SomberNight · 2026-01-27T11:16:52Z

electrum/lnsweep.py

+        # - if this is an incoming payment, where
+        #   - all the HTLCs have already been received (covering invoice amt) and are pending currently, and
+        #   - no one knows the preimage besides us, and
+        #   - this channel was just force-closed by either party
+        #   -> then we can freely decide whether to claim the HTLCs or let them time out.
+        #      Both choices are valid and safe. *Here* it is simpler to implement
+        #      letting the HTLC time out, so that's what we do.


if we are forwarding, and the outgoing HTLC has already been irrevocably committed, we need to keep watching this incoming HTLC until cltv_abs and be prepared to still fulfill it if we learn the preimage from downstream

note: this is currently not handled and we might lose money when forwarding.

electrum/electrum/lnwatcher.py

Lines 163 to 168 in 4f7b6e8

is_local_ctx, sweep_info_dict = chan.get_ctx_sweep_info(closing_tx)

# note: we need to keep watching *at least* until the closing tx is deeply mined,

# possibly longer if there are TXOs to sweep

keep_watching = not self.adb.is_deeply_mined(closing_tx.txid())

# create and broadcast transactions

for prevout, sweep_info in sweep_info_dict.items():

If chan.get_ctx_sweep_info does not include an item for the incoming HTLC, e.g. because we don't YET know the preimage for it, lnwatcher might stop watching the channel and never redeem the incoming HTLC onchain, even after the outgoing HTLC gets fulfilled.

This looks like another use case for KeepWatchingTXO, 7e1fd00

ecdsa · 2026-01-27T13:08:21Z

the concept looks good to me, I would prefer a db upgrade.
also, lnworker.save_preimage could take an additional parameter for this boolean.

electrum/lnchannel.py

electrum/wallet_db.py

electrum/lnsweep.py

SomberNight · 2026-02-24T17:04:35Z

I squashed and force-pushed. There is a copy of the old branch here (commit).

"When should we reveal preimages onchain?" This commit tries to simplify the thinking by making the observation: - we can reveal preimages (actually in any context) if they are already public - a preimage is public if any other lightning node knows it besides us - if we learn the preimage from another LN node, it is public - if we send update_fulfill_htlc, it becomes public - if we see a preimage onchain, it is public - in lnsweep._maybe_reveal_preimage_for_htlc: - partial mpp check is not relevant if preimage is already public - let's just always do KeepWatchingTXO, for sanity/safety Co-authored-by: ThomasV <thomasv@electrum.org>

SomberNight · 2026-02-24T17:51:48Z

electrum/lnworker.py

+    def save_preimage(
+        self,
+        payment_hash: bytes,
+        preimage: bytes,
+        *,
+        write_to_disk: bool = True,
+        mark_as_public: Optional[bool] = None,  # see is_preimage_public
+    ):
+        assert isinstance(payment_hash, bytes), f"expected bytes, but got {type(payment_hash)}"
+        assert isinstance(preimage, bytes), f"expected bytes, but got {type(preimage)}"
        if sha256(preimage) != payment_hash:
            raise Exception("tried to save incorrect preimage for payment_hash")
-        if self._preimages.get(payment_hash.hex()) is not None:
-            return  # we already have this preimage
-        self.logger.debug(f"saving preimage for {payment_hash.hex()}")
-        self._preimages[payment_hash.hex()] = preimage.hex()
+        old_tuple = _, old_is_public = self._preimages.get(payment_hash.hex(), (None, None))
+        if mark_as_public is None:  # if unset, keep current DB value
+            mark_as_public = old_is_public or False
+        if old_is_public and not mark_as_public:
+            raise Exception("preimage mark_as_public: True->False transition is forbidden")
+        # sanity checks and conversions done.
+        new_tuple = preimage.hex(), mark_as_public
+        if old_tuple == new_tuple:  # no change
+            return
+        self.logger.debug(f"saving preimage for {payment_hash.hex()} (public={mark_as_public})")
+        self._preimages[payment_hash.hex()] = new_tuple


also, lnworker.save_preimage could take an additional parameter for this boolean.

I changed to this, as the previous API was too error-prone - there was already a bug in this branch due to that. (ordering of calls to lnworker.save_preimage vs lnworker.mark_preimage_as_public mattered)

Unfortunately, to keep the API (for callers) simple, I had to put quite a bit of complexity inside the implementation of save_preimage. Still, I think it's ~okay.

regarding complexity, why does save_preimage require the payment_hash as parameter?
I guess we could kill that parameter, although the caller would probably want to check the hash in some cases.

SomberNight · 2026-02-24T18:03:06Z

electrum/lnsweep.py

 ) -> Tuple[Optional[bytes], Optional[KeepWatchingTXO]]:
-    """Given a Remote-added-HTLC, return the preimage if it's okay to reveal it on-chain."""
-    if not chan.lnworker.is_complete_mpp(htlc.payment_hash):
+    """Given a Remote-added-HTLC, return the preimage if it's okay to reveal it on-chain.


lnsweep desperately needs unit tests. I have a WIP branch to build the architecture for it but we can merge this before I get to finish that.

I have tested the current branch using regtest (using the 'timebomb' approach).
It behaves well.

SomberNight requested a review from ecdsa January 26, 2026 18:25

SomberNight commented Jan 26, 2026

View reviewed changes

SomberNight commented Jan 27, 2026

View reviewed changes

ecdsa reviewed Jan 27, 2026

View reviewed changes

electrum/lnchannel.py Outdated Show resolved Hide resolved

ecdsa force-pushed the 202601_lnworker_is_preimage_public branch from a771758 to b2064e6 Compare February 16, 2026 16:27

SomberNight commented Feb 16, 2026

View reviewed changes

electrum/wallet_db.py Show resolved Hide resolved

ecdsa force-pushed the 202601_lnworker_is_preimage_public branch from b2064e6 to a277589 Compare February 18, 2026 12:30

SomberNight commented Feb 23, 2026

View reviewed changes

electrum/lnsweep.py Show resolved Hide resolved

SomberNight force-pushed the 202601_lnworker_is_preimage_public branch 2 times, most recently from d0eb87d to 0a08169 Compare February 24, 2026 16:55

SomberNight marked this pull request as ready for review February 24, 2026 17:04

SomberNight force-pushed the 202601_lnworker_is_preimage_public branch from 0a08169 to 0b2c7a8 Compare February 24, 2026 17:49

SomberNight commented Feb 24, 2026

View reviewed changes

simplify prev

10274c1

SomberNight commented Feb 24, 2026

View reviewed changes

	is_local_ctx, sweep_info_dict = chan.get_ctx_sweep_info(closing_tx)
	# note: we need to keep watching at least until the closing tx is deeply mined,
	# possibly longer if there are TXOs to sweep
	keep_watching = not self.adb.is_deeply_mined(closing_tx.txid())
	# create and broadcast transactions
	for prevout, sweep_info in sweep_info_dict.items():

Comments

Conversation

SomberNight commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

f321x Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecdsa commented Jan 27, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SomberNight commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecdsa Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SomberNight commented Jan 26, 2026 •

edited

Loading

f321x Jan 27, 2026 •

edited

Loading

SomberNight commented Feb 24, 2026 •

edited

Loading

ecdsa Feb 25, 2026 •

edited

Loading