txbatcher: add TODOs re nLocktime block-height vs timestamp confusion

seems harmless atm but some rethinking is needed and checks should be added

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

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
 

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))