lnwatcher: remove rearly return that breaks accounting_addresses
What the developer wrote
Authored by ThomasV
lnwatcher: remove rearly return that breaks accounting_addresses
This reverts 6ffaa55.
Also, in regtests, add a check that fails if accounting_addresses are broken.
What changed, and why it matters
This commit fixes a bug in Electrum's Lightning watchtower code where an early exit was preventing the wallet from properly tracking certain closing transactions. The fix removes an unnecessary 'return early' check so that the wallet continues to process and account for these transactions. A new regression test checks that the closing transaction appears in the user's on-chain history even when it doesn't directly involve the user's wallet addresses. This is primarily an accounting/visibility bug, not a direct theft-of-funds vulnerability.
Treat as a routine bug-fix commit. Reviewers should verify that removing the early return does not reintroduce the original problem that 6ffaa55 was trying to solve, and that the new regression test reliably catches the accounting_addresses issue.
Security signals we found
Fixes a logic bug that breaks transaction accounting/visibility for Lightning channel closes
Adds regression test to prevent reintroduction of broken accounting_addresses behavior
Change is a revert of prior commit 6ffaa55
No cryptographic, network, or input-validation changes observed
Evidence from the diff
In electrum/lnwatcher.py, the commit removes a guard clause if not chan.need_to_subscribe(): return False inside sweep_commitment_transaction(). That early return, introduced in commit 6ffaa55, was causing accounting_addresses not to be set for some channel closing transactions, which in turn meant those transactions did not appear in the user’s onchain_history. The patch reverts that behavior and adds a regtest assertion that the closing transaction is visible in alice’s onchain_history even though its inputs/outputs do not touch alice’s on-chain wallet addresses.
Changed components
electrum/lnwatcher.pytests/regtest/regtest.shInspect captured patch +6 / −3
diff --git a/electrum/lnwatcher.py b/electrum/lnwatcher.py
index d683652..7927f65 100644
--- a/electrum/lnwatcher.py
+++ b/electrum/lnwatcher.py
@@ -157,9 +157,6 @@ class LNWatcher(Logger, EventListener):
chan = self.lnworker.channel_by_txo(funding_outpoint)
if not chan:
return False
- if not chan.need_to_subscribe():
- return False
- self.logger.info(f'sweep_commitment_transaction {funding_outpoint}')
# detect who closed and get information about how to claim outputs
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,
diff --git a/tests/regtest/regtest.sh b/tests/regtest/regtest.sh
index af3dc8d..718696a 100755
--- a/tests/regtest/regtest.sh
+++ b/tests/regtest/regtest.sh
@@ -344,6 +344,12 @@ if [[ $1 == "swapserver_forceclose" ]]; then
wait_until_spent $ctx_id $output_index
new_blocks 144
wait_for_balance bob 0.999
+ # check that the closing tx is in alice's onchain_history. Since this tx does not
+ # touch alice's wallet addresses, this test requires accounting_addresses to be set
+ if [[ ! $($alice onchain_history| jq --arg txid $ctx_id '.[]|select(.txid == $txid)') ]]; then
+ echo "accounting_address not set"
+ exit 1
+ fi
fi
Why this scored 29/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.