txbatcher: add TODOs re nLocktime block-height vs timestamp confusion
What the developer wrote
Authored by SomberNight
txbatcher: add TODOs re nLocktime block-height vs timestamp confusion
seems harmless atm but some rethinking is needed and checks should be added
What changed, and why it matters
This commit adds comments and TODOs warning that the wallet code may be mixing up two different kinds of Bitcoin time locks: block heights and wall-clock timestamps. It also fixes some helper functions to explicitly return None instead of implicitly returning nothing. The commit itself says the issue 'seems harmless atm' but needs rethinking. There is no actual exploit shown, only a note that future checks are needed.
Treat this as a maintenance note rather than an active vulnerability. Developers should follow up by adding type tags or assertions distinguishing block-height from timestamp locktimes, especially in TxBatch locktime selection and SweepInfo.cltv_abs handling. Users do not need to take immediate action.
Security signals we found
TODO comments warning about nLocktime/CLTV block-height vs timestamp confusion
FIXME marking an existing locktime comparison as potentially unsafe
Explicit None returns in locktime helpers to avoid ambiguous implicit returns
Evidence from the diff
The patch touches electrum/lnsweep.py, electrum/transaction.py, and electrum/txbatcher.py. It adds TODO comments noting that cltv_abs values and nLocktime comparisons should be checked to ensure they are block-height-based rather than timestamp-based. It also fixes get_time_based_relative_locktime and get_block_based_relative_locktime to return None explicitly when no locktime applies, and adds similar explicit None returns in Transaction-level helpers. The txbatcher.py change marks the existing locktime = sweep_info.cltv_abs comparison as FIXME height vs timestamp confusion. No runtime checks or type distinctions are actually added yet.
Changed components
electrum/txbatcher.pyelectrum/lnsweep.pyelectrum/transaction.pyInspect captured patch +20 / −10
diff --git a/electrum/lnsweep.py b/electrum/lnsweep.py
index 28f74f3..3e1c152 100644
--- a/electrum/lnsweep.py
+++ b/electrum/lnsweep.py
@@ -40,6 +40,7 @@ HTLCTX_INPUT_OUTPUT_INDEX = 0
class SweepInfo(NamedTuple):
name: str
cltv_abs: Optional[int] # set to None only if the script has no cltv
+ # TODO add asserts that cltv_abs is block-based (see NLOCKTIME_BLOCKHEIGHT_MAX)
txin: PartialTxInput
txout: Optional[PartialTxOutput] # only for first-stage htlc tx
can_be_batched: bool # todo: this could be more fine-grained
diff --git a/electrum/transaction.py b/electrum/transaction.py
index cca06ff..7f621af 100644
--- a/electrum/transaction.py
+++ b/electrum/transaction.py
@@ -349,16 +349,18 @@ class TxInput:
def get_time_based_relative_locktime(self) -> Optional[int]:
# see bip 68
- if self.nsequence & (1<<31):
- return
- if self.nsequence & (1<<22):
+ if self.nsequence & (1<<31): # "disable" flag
+ return None
+ if self.nsequence & (1<<22): # in units of 512 sec
return self.nsequence & 0xffff
+ return None
def get_block_based_relative_locktime(self) -> Optional[int]:
- if self.nsequence & (1<<31):
- return
- if not self.nsequence & (1<<22):
+ if self.nsequence & (1<<31): # "disable" flag
+ return None
+ if not self.nsequence & (1<<22): # in blocks
return self.nsequence & 0xffff
+ return None
@property
def short_id(self):
@@ -1328,13 +1330,13 @@ class Transaction:
def get_time_based_relative_locktime(self) -> Optional[int]:
if self.version < 2:
- return
+ return None
locktimes = list(filter(None, [txin.get_time_based_relative_locktime() for txin in self.inputs()]))
return max(locktimes) if locktimes else None
def get_block_based_relative_locktime(self) -> Optional[int]:
if self.version < 2:
- return
+ return None
locktimes = list(filter(None, [txin.get_block_based_relative_locktime() for txin in self.inputs()]))
return max(locktimes) if locktimes else None
diff --git a/electrum/txbatcher.py b/electrum/txbatcher.py
index 91b5332..ee1727e 100644
--- a/electrum/txbatcher.py
+++ b/electrum/txbatcher.py
@@ -45,7 +45,7 @@
#
# 1. CPFP:
# When a batch is forgotten but not mined (because the server returned an error), we no longer bump its fee.
-# However, the current code does not theat the next batch as a CPFP when computing the fee.
+# However, the current code does not treat the next batch as a CPFP when computing the fee.
#
# 2. Reorgs:
# This code does not guarantee that a payment or a sweep will happen.
@@ -56,6 +56,13 @@
# In the case of sweeps, lnwatcher ensures that SweepInfo is added again after a client restart.
# In order to generalize that logic to payments, callers would need to pass a unique ID along with
# the payment output, so that we can prevent paying twice.
+#
+# - nLocktime/CLTV values (bip-65) and nSequence/CSV values (bip-112) are either explicitly
+# or implicitly block-height-based everywhere in this file.
+# SCRIPT execution fails on height vs timestamp confusion, and
+# it is not safe to do naive integer comparison between these values without establishing type.
+# TODO review this is correct, and add checks.
+# nLocktime/CLTV usage in particular seems dangerously *implicit* for being block-heights
import asyncio
import threading
@@ -517,7 +524,7 @@ class TxBatch(Logger):
# sort inputs so that txin-txout pairs are first
for sweep_info in sorted(to_sweep, key=lambda x: not bool(x.txout)):
if sweep_info.cltv_abs is not None:
- if locktime is None or locktime < sweep_info.cltv_abs:
+ if locktime is None or locktime < sweep_info.cltv_abs: # FIXME height vs timestamp confusion
# nLockTime must be greater than or equal to the stack operand.
locktime = sweep_info.cltv_abs
inputs.append(copy.deepcopy(sweep_info.txin))
Why this scored 32/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.