lnchan: only sign force_close_tx when demanded, assure consistency, fix test

electrum/lnchan.py

@@ -147,7 +147,7 @@ class Channel(PrintError):
         except:
             return super().diagnostic_name()
 
-    def __init__(self, state, sweep_address = None, name = None, payment_completed : Optional[Callable[[HTLCOwner, UpdateAddHtlc, bytes], None]] = None):
+    def __init__(self, state, sweep_address = None, name = None, payment_completed : Optional[Callable[[HTLCOwner, UpdateAddHtlc, bytes], None]] = None, local_commitment = None):
         self.preimages = {}
         if not payment_completed:
             payment_completed = lambda this, x, y, z: None
@@ -205,7 +205,12 @@ class Channel(PrintError):
         for sub in (LOCAL, REMOTE):
             self.log[sub].locked_in.update(self.log[sub].adds.keys())
 
-        self.set_local_commitment(self.current_commitment(LOCAL))
+        if local_commitment:
+            local_commitment = Transaction(str(local_commitment))
+            local_commitment.deserialize(True)
+            self.set_local_commitment(local_commitment)
+        else:
+            self.set_local_commitment(self.current_commitment(LOCAL))
         self.set_remote_commitment(self.current_commitment(REMOTE))
 
     def set_local_commitment(self, ctx):
@@ -213,6 +218,8 @@ class Channel(PrintError):
         if self.sweep_address is not None:
             self.local_sweeptxs = create_sweeptxs_for_our_latest_ctx(self, self.local_commitment, self.sweep_address)
 
+        self.assert_signature_fits(ctx)
+
     def set_remote_commitment(self, ctx):
         self.remote_commitment = ctx
         if self.sweep_address is not None:
@@ -449,7 +456,9 @@ class Channel(PrintError):
             feerate=new_feerate
         )
 
-        self.set_local_commitment(self.pending_commitment(LOCAL))
+        # since we should not revoke our latest commitment tx,
+        # we do not update self.local_commitment here,
+        # it should instead be updated when we receive a new sig
 
         return RevokeAndAck(last_secret, next_point), "current htlcs"
 
@@ -541,7 +550,6 @@ class Channel(PrintError):
             if self.constraints.is_initiator:
                 self.pending_fee[FUNDEE_ACKED] = True
 
-        self.set_local_commitment(self.pending_commitment(LOCAL))
         self.set_remote_commitment(self.pending_commitment(REMOTE))
         self.remote_commitment_to_be_revoked = prev_remote_commitment
         return received_this_batch, sent_this_batch
@@ -773,7 +781,7 @@ class Channel(PrintError):
                 serialized_channel[k] = v
         dumped = ChannelJsonEncoder().encode(serialized_channel)
         roundtripped = json.loads(dumped)
-        reconstructed = Channel(roundtripped)
+        reconstructed = Channel(roundtripped, local_commitment=self.local_commitment)
         to_save_new = reconstructed.to_save()
         if to_save_new != to_save_ref:
             from pprint import PrettyPrinter
@@ -864,19 +872,26 @@ class Channel(PrintError):
         sig = ecc.sig_string_from_der_sig(der_sig[:-1])
         return sig, closing_tx
 
+    def assert_signature_fits(self, tx):
+        remote_sig = self.config[LOCAL].current_commitment_signature
+        if remote_sig: # only None in test
+            preimage_hex = tx.serialize_preimage(0)
+            pre_hash = sha256d(bfh(preimage_hex))
+            assert ecc.verify_signature(self.config[REMOTE].multisig_key.pubkey, remote_sig, pre_hash)
+
     def force_close_tx(self):
-        tx = self.current_commitment(LOCAL)
+        tx = self.local_commitment
+        tx = Transaction(str(tx))
+        tx.deserialize(True)
+        self.assert_signature_fits(tx)
         tx.sign({bh2u(self.config[LOCAL].multisig_key.pubkey): (self.config[LOCAL].multisig_key.privkey, True)})
         remote_sig = self.config[LOCAL].current_commitment_signature
-
+        if remote_sig: # only None in test
-        preimage_hex = tx.serialize_preimage(0)
+            remote_sig = ecc.der_sig_from_sig_string(remote_sig) + b"\x01"
-        pre_hash = sha256d(bfh(preimage_hex))
+            sigs = tx._inputs[0]["signatures"]
-        assert ecc.verify_signature(self.config[REMOTE].multisig_key.pubkey, remote_sig, pre_hash)
+            none_idx = sigs.index(None)
-
+            tx.add_signature_to_txin(0, none_idx, bh2u(remote_sig))
-        remote_sig = ecc.der_sig_from_sig_string(remote_sig) + b"\x01"
+            assert tx.is_complete()
-        none_idx = tx._inputs[0]["signatures"].index(None)
-        tx.add_signature_to_txin(0, none_idx, bh2u(remote_sig))
-        assert tx.is_complete()
         return tx
 
     def included_htlcs_in_their_latest_ctxs(self, htlc_initiator) -> Dict[int, List[UpdateAddHtlc]]:

electrum/tests/test_lnchan.py

@@ -83,7 +83,7 @@ def create_channel_state(funding_txid, funding_index, funding_sat, local_feerate
                 funding_locked_received=True,
                 was_announced=False,
                 # just a random signature
-                current_commitment_signature=sig_string_from_der_sig(bytes.fromhex('3046022100c66e112e22b91b96b795a6dd5f4b004f3acccd9a2a31bf104840f256855b7aa3022100e711b868b62d87c7edd95a2370e496b9cb6a38aff13c9f64f9ff2f3b2a0052dd')),
+                current_commitment_signature=None,
                 current_htlc_signatures=None,
             ),
             "constraints":lnbase.ChannelConstraints(
@@ -134,6 +134,20 @@ def create_test_channels(feerate=6000, local=None, remote=None):
 
     alice.set_state('OPEN')
     bob.set_state('OPEN')
+
+    #old_bob = bob.config[REMOTE]
+    #bob.config[REMOTE] = bob.config[REMOTE]._replace(ctn=bob.config[REMOTE].ctn)
+    #sig_from_bob = bob.sign_next_commitment()[0]
+    #bob.config[REMOTE] = old_bob
+
+    #old_alice = alice.config[REMOTE]
+    #alice.config[REMOTE] = alice.config[REMOTE]._replace(ctn=alice.config[REMOTE].ctn)
+    #sig_from_alice = alice.sign_next_commitment()[0]
+    #alice.config[REMOTE] = old_alice
+
+    #alice.config[LOCAL] = alice.config[LOCAL]._replace(current_commitment_signature=sig_from_bob)
+    #bob.config[LOCAL] = bob.config[LOCAL]._replace(current_commitment_signature=sig_from_alice)
+
     return alice, bob
 
 class TestFee(unittest.TestCase):
@@ -204,6 +218,9 @@ class TestChannel(unittest.TestCase):
         self.assertEqual({0: [], 1: []}, alice_channel.included_htlcs_in_their_latest_ctxs(REMOTE))
         self.assertEqual({0: [], 1: []}, bob_channel.included_htlcs_in_their_latest_ctxs(LOCAL))
 
+        # this wouldn't work since we put None in the remote_sig
+        # alice_channel.force_close_tx()
+
         # Next alice commits this change by sending a signature message. Since
         # we expect the messages to be ordered, Bob will receive the HTLC we
         # just sent before he receives this signature, so the signature will
@@ -237,8 +254,6 @@ class TestChannel(unittest.TestCase):
         # forward since she's sending an outgoing HTLC.
         alice_channel.receive_revocation(bobRevocation)
 
-        alice_channel.force_close_tx()
-
         # test serializing with locked_in htlc
         self.assertEqual(len(alice_channel.to_save()['local_log']), 1)
         alice_channel.serialize()
@@ -248,9 +263,15 @@ class TestChannel(unittest.TestCase):
         # the point where she sent her signature, including the HTLC.
         alice_channel.receive_new_commitment(bobSig, bobHtlcSigs)
 
+        tx1 = str(alice_channel.force_close_tx())
+
         # Alice then generates a revocation for bob.
         aliceRevocation, _ = alice_channel.revoke_current_commitment()
 
+        tx2 = str(alice_channel.force_close_tx())
+        # since alice already has the signature for the next one, it doesn't change her force close tx (it was already the newer one)
+        self.assertEqual(tx1, tx2)
+
         # Finally Bob processes Alice's revocation, at this point the new HTLC
         # is fully locked in within both commitment transactions. Bob should
         # also be able to forward an HTLC now that the HTLC has been locked
@@ -284,6 +305,10 @@ class TestChannel(unittest.TestCase):
 
         alice_channel.receive_htlc_settle(preimage, self.aliceHtlcIndex)
 
+        tx3 = str(alice_channel.force_close_tx())
+        # just settling a htlc does not change her force close tx
+        self.assertEqual(tx2, tx3)
+
         bobSig2, bobHtlcSigs2 = bob_channel.sign_next_commitment()
 
         self.assertEqual({1: [htlc], 2: []}, alice_channel.included_htlcs_in_their_latest_ctxs(LOCAL))
@@ -293,6 +318,9 @@ class TestChannel(unittest.TestCase):
 
         alice_channel.receive_new_commitment(bobSig2, bobHtlcSigs2)
 
+        tx4 = str(alice_channel.force_close_tx())
+        self.assertNotEqual(tx3, tx4)
+
         aliceRevocation2, _ = alice_channel.revoke_current_commitment()
         aliceSig2, aliceHtlcSigs2 = alice_channel.sign_next_commitment()
         self.assertEqual(aliceHtlcSigs2, [], "alice should generate no htlc signatures")
@@ -326,8 +354,16 @@ class TestChannel(unittest.TestCase):
 
         alice_channel.update_fee(100000, True)
         bob_channel.update_fee(100000, False)
+
+        tx5 = str(alice_channel.force_close_tx())
+        # sending a fee update does not change her force close tx
+        self.assertEqual(tx4, tx5)
+
         force_state_transition(alice_channel, bob_channel)
 
+        tx6 = str(alice_channel.force_close_tx())
+        self.assertNotEqual(tx5, tx6)
+
         self.htlc_dict['amount_msat'] *= 5
         bob_index = bob_channel.add_htlc(self.htlc_dict)
         alice_index = alice_channel.receive_htlc(self.htlc_dict)