revamp fee updates (draft)

electrum/lnchannel.py

@@ -46,6 +46,7 @@ from .lnutil import (Outpoint, LocalConfig, RemoteConfig, Keypair, OnlyPubkeyKey
                     HTLC_TIMEOUT_WEIGHT, HTLC_SUCCESS_WEIGHT, extract_ctn_from_tx_and_chan, UpdateAddHtlc,
                     funding_output_script, SENT, RECEIVED, LOCAL, REMOTE, HTLCOwner, make_commitment_outputs,
                     ScriptHtlc, PaymentFailure, calc_onchain_fees, RemoteMisbehaving, make_htlc_output_witness_script)
+from .lnutil import FeeUpdate
 from .lnsweep import create_sweeptxs_for_our_ctx, create_sweeptxs_for_their_ctx
 from .lnsweep import create_sweeptx_for_their_revoked_htlc
 from .lnhtlc import HTLCManager
@@ -63,29 +64,7 @@ class ChannelJsonEncoder(json.JSONEncoder):
 
 RevokeAndAck = namedtuple("RevokeAndAck", ["per_commitment_secret", "next_per_commitment_point"])
 
-class FeeUpdateProgress(Enum):
-    FUNDEE_SIGNED = auto()
-    FUNDEE_ACKED =  auto()
-    FUNDER_SIGNED = auto()
 
-FUNDEE_SIGNED = FeeUpdateProgress.FUNDEE_SIGNED
-FUNDEE_ACKED = FeeUpdateProgress.FUNDEE_ACKED
-FUNDER_SIGNED = FeeUpdateProgress.FUNDER_SIGNED
-
-class FeeUpdate(defaultdict):
-    def __init__(self, chan, rate):
-        super().__init__(lambda: False)
-        self.rate = rate
-        self.chan = chan
-
-    def pending_feerate(self, subject):
-        if self[FUNDEE_ACKED]:
-            return self.rate
-        if subject == REMOTE and self.chan.constraints.is_initiator:
-            return self.rate
-        if subject == LOCAL and not self.chan.constraints.is_initiator:
-            return self.rate
-        # implicit return None
 
 def decodeAll(d, local):
     for k, v in d.items():
@@ -112,6 +91,12 @@ def str_bytes_dict_from_save(x):
 def str_bytes_dict_to_save(x):
     return {str(k): bh2u(v) for k, v in x.items()}
 
+def deserialize_feeupdate(x):
+    return FeeUpdate(rate=x['rate'], ctn={LOCAL:x['ctn'][str(int(LOCAL))], REMOTE:x['ctn'][str(int(REMOTE))]})
+
+def serialize_feeupdate(x):
+    return {'rate':x.rate, 'ctn': {int(LOCAL):x.ctn[LOCAL], int(REMOTE):x.ctn[REMOTE]}}
+
 class Channel(Logger):
     # note: try to avoid naming ctns/ctxs/etc as "current" and "pending".
     #       they are ambiguous. Use "oldest_unrevoked" or "latest" or "next".
@@ -126,6 +111,8 @@ class Channel(Logger):
             return super().diagnostic_name()
 
     def __init__(self, state, *, sweep_address=None, name=None, lnworker=None):
+        self.name = name
+        Logger.__init__(self)
         self.lnworker = lnworker
         self.sweep_address = sweep_address
         assert 'local_state' not in state
@@ -150,6 +137,7 @@ class Channel(Logger):
         self.short_channel_id_predicted = self.short_channel_id
         self.onion_keys = str_bytes_dict_from_save(state.get('onion_keys', {}))
         self.force_closed = state.get('force_closed')
+        self.fee_updates = [deserialize_feeupdate(x) if type(x) is not FeeUpdate else x for x in state.get('fee_updates')] # populated with initial fee
 
         # FIXME this is a tx serialised in the custom electrum partial tx format.
         # we should not persist txns in this format. we should persist htlcs, and be able to derive
@@ -162,10 +150,6 @@ class Channel(Logger):
                               remote_ctn=self.config[REMOTE].ctn,
                               log=log)
 
-        self.name = name
-        Logger.__init__(self)
-
-        self.pending_fee = None
 
         self._is_funding_txo_spent = None  # "don't know"
         self._state = None
@@ -174,6 +158,43 @@ class Channel(Logger):
         self.remote_commitment = None
         self.sweep_info = {}
 
+    def pending_fee(self):
+        """ return FeeUpdate that has not been commited on any side"""
+        for f in self.fee_updates:
+            if f.ctn[LOCAL] is None and f.ctn[REMOTE] is None:
+                return f
+
+    def locally_pending_fee(self):
+        """ return FeeUpdate that been commited remotely and is still pending locally (should used if we are initiator)"""
+        for f in self.fee_updates:
+            if f.ctn[LOCAL] is None and f.ctn[REMOTE] is not None:
+                return f
+
+    def remotely_pending_fee(self):
+        """ return FeeUpdate that been commited locally and is still pending remotely (should be used if we are not initiator)"""
+        for f in self.fee_updates:
+            if f.ctn[LOCAL] is not None and f.ctn[REMOTE] is None:
+                return f
+
+    def get_feerate(self, subject, target_ctn):
+        next_ctn = self.config[subject].ctn + 1
+        assert target_ctn <= next_ctn
+        result = self.fee_updates[0]
+        for f in self.fee_updates[1:]:
+            ctn = f.ctn[subject]
+            if ctn is None:
+                ctn = next_ctn
+            if ctn > result.ctn[subject] and ctn <= target_ctn:
+                best_ctn = ctn
+                result = f
+        return result.rate
+
+    def get_current_feerate(self, subject):
+        return self.get_feerate(subject, self.config[subject].ctn)
+
+    def get_next_feerate(self, subject):
+        return self.get_feerate(subject, self.config[subject].ctn + 1)
+
     def get_payments(self):
         out = {}
         for subject in LOCAL, REMOTE:
@@ -377,11 +398,9 @@ class Channel(Logger):
             current_htlc_signatures=htlc_sigs_string,
             got_sig_for_next=True)
 
-        if self.pending_fee is not None:
-            if not self.constraints.is_initiator:
-                self.pending_fee[FUNDEE_SIGNED] = True
-            if self.constraints.is_initiator and self.pending_fee[FUNDEE_ACKED]:
-                self.pending_fee[FUNDER_SIGNED] = True
+        # if a fee update was acked, then we add it locally
+        f = self.locally_pending_fee()
+        if f: f.ctn[LOCAL] = next_local_ctn
 
         self.set_local_commitment(pending_local_commitment)
 
@@ -418,14 +437,6 @@ class Channel(Logger):
         new_ctx = self.pending_commitment(LOCAL)
         assert self.signature_fits(new_ctx)
         self.set_local_commitment(new_ctx)
-
-        if self.pending_fee is not None:
-            if (not self.constraints.is_initiator and self.pending_fee[FUNDEE_SIGNED])\
-               or (self.constraints.is_initiator and self.pending_fee[FUNDER_SIGNED]):
-                self.constraints = self.constraints._replace(feerate=self.pending_fee.rate)
-                self.pending_fee = None
-                self.logger.info(f"Feerate change complete (initiator: {self.constraints.is_initiator})")
-
         self.hm.send_rev()
         self.config[LOCAL]=self.config[LOCAL]._replace(
             ctn=new_ctn,
@@ -461,29 +472,19 @@ class Channel(Logger):
         # FIXME not sure this is correct... but it seems to work
         # if there are update_add_htlc msgs between commitment_signed and rev_ack,
         # this might break
+        next_ctn = self.config[REMOTE].ctn + 1
         prev_remote_commitment = self.pending_commitment(REMOTE)
         self.config[REMOTE].revocation_store.add_next_entry(revocation.per_commitment_secret)
         ##### start applying fee/htlc changes
-        if self.pending_fee is not None:
-            if not self.constraints.is_initiator:
-                self.pending_fee[FUNDEE_SIGNED] = True
-            if self.constraints.is_initiator and self.pending_fee[FUNDEE_ACKED]:
-                self.pending_fee[FUNDER_SIGNED] = True
-
+        f = self.pending_fee()
+        if f: f.ctn[REMOTE] = next_ctn
         next_point = self.config[REMOTE].next_per_commitment_point
-
         self.hm.recv_rev()
-
         self.config[REMOTE]=self.config[REMOTE]._replace(
-            ctn=self.config[REMOTE].ctn + 1,
+            ctn=next_ctn,
             current_per_commitment_point=next_point,
             next_per_commitment_point=revocation.next_per_commitment_point,
         )
-
-        if self.pending_fee is not None:
-            if self.constraints.is_initiator:
-                self.pending_fee[FUNDEE_ACKED] = True
-
         self.set_remote_commitment()
         self.remote_commitment_to_be_revoked = prev_remote_commitment
 
@@ -539,7 +540,7 @@ class Channel(Logger):
                 - calc_onchain_fees(
                       # TODO should we include a potential new htlc, when we are called from receive_htlc?
                       len(self.included_htlcs(subject, SENT) + self.included_htlcs(subject, RECEIVED)),
-                      self.pending_feerate(subject),
+                      self.get_current_feerate(subject),
                       self.constraints.is_initiator,
                   )[subject]
 
@@ -551,7 +552,7 @@ class Channel(Logger):
         assert type(direction) is Direction
         if ctn is None:
             ctn = self.config[subject].ctn
-        feerate = self.pending_feerate(subject)
+        feerate = self.get_feerate(subject, ctn)
         conf = self.config[subject]
         if (subject, direction) in [(REMOTE, RECEIVED), (LOCAL, SENT)]:
             weight = HTLC_SUCCESS_WEIGHT
@@ -561,27 +562,18 @@ class Channel(Logger):
         fee_for_htlc = lambda htlc: htlc.amount_msat // 1000 - (weight * feerate // 1000)
         return list(filter(lambda htlc: fee_for_htlc(htlc) >= conf.dust_limit_sat, htlcs))
 
-    def pending_feerate(self, subject):
-        assert type(subject) is HTLCOwner
-        candidate = self.constraints.feerate
-        if self.pending_fee is not None:
-            x = self.pending_fee.pending_feerate(subject)
-            if x is not None:
-                candidate = x
-        return candidate
-
     def pending_commitment(self, subject):
         assert type(subject) is HTLCOwner
         this_point = self.config[REMOTE].next_per_commitment_point if subject == REMOTE else self.local_points(offset=1)[1]
         ctn = self.config[subject].ctn + 1
-        feerate = self.pending_feerate(subject)
+        feerate = self.get_feerate(subject, ctn)
         return self.make_commitment(subject, this_point, ctn, feerate, True)
 
     def current_commitment(self, subject):
         assert type(subject) is HTLCOwner
         this_point = self.config[REMOTE].current_per_commitment_point if subject == REMOTE else self.local_points(offset=0)[1]
         ctn = self.config[subject].ctn
-        feerate = self.constraints.feerate
+        feerate = self.get_feerate(subject, ctn)
         return self.make_commitment(subject, this_point, ctn, feerate, False)
 
     def get_current_ctn(self, subject):
@@ -636,11 +628,12 @@ class Channel(Logger):
         return self.constraints.capacity - sum(x[2] for x in self.pending_commitment(LOCAL).outputs())
 
     def update_fee(self, feerate, initiator):
-        if self.constraints.is_initiator != initiator:
-            raise Exception("Cannot update_fee: wrong initiator", initiator)
-        if self.pending_fee is not None:
-            raise Exception("a fee update is already in progress")
-        self.pending_fee = FeeUpdate(self, rate=feerate)
+        f = self.pending_fee()
+        if f:
+            f.rate = feerate
+            return
+        f = FeeUpdate(rate=feerate, ctn={LOCAL:None, REMOTE:None})
+        self.fee_updates.append(f)
 
     def to_save(self):
         to_save = {
@@ -652,6 +645,7 @@ class Channel(Logger):
                 "funding_outpoint": self.funding_outpoint,
                 "node_id": self.node_id,
                 "remote_commitment_to_be_revoked": str(self.remote_commitment_to_be_revoked),
+                "fee_updates": self.fee_updates,
                 "log": self.hm.to_save(),
                 "onion_keys": str_bytes_dict_to_save(self.onion_keys),
                 "force_closed": self.force_closed,
@@ -665,6 +659,8 @@ class Channel(Logger):
         for k, v in to_save_ref.items():
             if isinstance(v, tuple):
                 serialized_channel[k] = namedtuples_to_dict(v)
+            elif k == 'fee_updates':
+                serialized_channel[k] = [serialize_feeupdate(x) for x in v]
             else:
                 serialized_channel[k] = v
         dumped = ChannelJsonEncoder().encode(serialized_channel)

electrum/lnpeer.py

@@ -40,6 +40,7 @@ from .lnutil import (Outpoint, LocalConfig, RECEIVED, UpdateAddHtlc,
                      LightningPeerConnectionClosed, HandshakeFailed, NotFoundChanAnnouncementForUpdate,
                      MINIMUM_MAX_HTLC_VALUE_IN_FLIGHT_ACCEPTED, MAXIMUM_HTLC_MINIMUM_MSAT_ACCEPTED,
                      MAXIMUM_REMOTE_TO_SELF_DELAY_ACCEPTED, RemoteMisbehaving, DEFAULT_TO_SELF_DELAY)
+from .lnutil import FeeUpdate
 from .lnsweep import create_sweeptxs_for_watchtower
 from .lntransport import LNTransport, LNTransportBase
 from .lnmsg import encode_msg, decode_msg
@@ -537,14 +538,15 @@ class Peer(Logger):
         # remote commitment transaction
         channel_id, funding_txid_bytes = channel_id_from_funding_tx(funding_txid, funding_index)
         chan_dict = {
-                "node_id": self.pubkey,
-                "channel_id": channel_id,
-                "short_channel_id": None,
-                "funding_outpoint": Outpoint(funding_txid, funding_index),
-                "remote_config": remote_config,
-                "local_config": local_config,
-                "constraints": ChannelConstraints(capacity=funding_sat, is_initiator=True, funding_txn_minimum_depth=funding_txn_minimum_depth, feerate=feerate),
-                "remote_commitment_to_be_revoked": None,
+            "node_id": self.pubkey,
+            "channel_id": channel_id,
+            "short_channel_id": None,
+            "funding_outpoint": Outpoint(funding_txid, funding_index),
+            "remote_config": remote_config,
+            "local_config": local_config,
+            "constraints": ChannelConstraints(capacity=funding_sat, is_initiator=True, funding_txn_minimum_depth=funding_txn_minimum_depth),
+            "fee_updates": [FeeUpdate(rate=feerate, ctn={LOCAL:0, REMOTE:0})],
+            "remote_commitment_to_be_revoked": None,
         }
         chan = Channel(chan_dict,
                        sweep_address=self.lnworker.sweep_address,
@@ -630,8 +632,9 @@ class Peer(Logger):
                     revocation_store=their_revocation_store,
                 ),
                 "local_config": local_config,
-                "constraints": ChannelConstraints(capacity=funding_sat, is_initiator=False, funding_txn_minimum_depth=min_depth, feerate=feerate),
+                "constraints": ChannelConstraints(capacity=funding_sat, is_initiator=False, funding_txn_minimum_depth=min_depth),
                 "remote_commitment_to_be_revoked": None,
+                "fee_updates": [FeeUpdate(feerate, ctn={LOCAL:0, REMOTE:0})],
         }
         chan = Channel(chan_dict,
                        sweep_address=self.lnworker.sweep_address,
@@ -1026,7 +1029,7 @@ class Peer(Logger):
         # if there are no changes, we will not (and must not) send a new commitment
         next_htlcs, latest_htlcs = chan.hm.get_htlcs_in_next_ctx(REMOTE), chan.hm.get_htlcs_in_latest_ctx(REMOTE)
         if (next_htlcs == latest_htlcs
-                and chan.pending_feerate(REMOTE) == chan.constraints.feerate) \
+                and chan.get_next_feerate(REMOTE) == chan.get_current_feerate(REMOTE)) \
                 or ctn_to_sign == self.sent_commitment_for_ctn_last[chan]:
             return
         self.logger.info(f'send_commitment. old number htlcs: {len(latest_htlcs)}, new number htlcs: {len(next_htlcs)}')
@@ -1088,7 +1091,7 @@ class Peer(Logger):
         chan = self.channels[channel_id]
         # make sure there were changes to the ctx, otherwise the remote peer is misbehaving
         if (chan.hm.get_htlcs_in_next_ctx(LOCAL) == chan.hm.get_htlcs_in_latest_ctx(LOCAL)
-                and chan.pending_feerate(LOCAL) == chan.constraints.feerate):
+                and chan.get_next_feerate(LOCAL) == chan.get_current_feerate(LOCAL)):
             raise RemoteMisbehaving('received commitment_signed without pending changes')
         # make sure ctn is new
         ctn_to_recv = chan.get_current_ctn(LOCAL) + 1
@@ -1289,7 +1292,7 @@ class Peer(Logger):
             # TODO force close if initiator does not update_fee enough
             return
         feerate_per_kw = self.lnworker.current_feerate_per_kw()
-        chan_fee = chan.pending_feerate(REMOTE)
+        chan_fee = chan.get_next_feerate(REMOTE)
         self.logger.info(f"current pending feerate {chan_fee}")
         self.logger.info(f"new feerate {feerate_per_kw}")
         if feerate_per_kw < chan_fee / 2:

electrum/lnutil.py

@@ -78,7 +78,9 @@ class RemoteConfig(NamedTuple):
     current_per_commitment_point: Optional[bytes]
 
 
-ChannelConstraints = namedtuple("ChannelConstraints", ["capacity", "is_initiator", "funding_txn_minimum_depth", "feerate"])
+FeeUpdate = namedtuple("FeeUpdate", ["rate", "ctn"])
+
+ChannelConstraints = namedtuple("ChannelConstraints", ["capacity", "is_initiator", "funding_txn_minimum_depth"])
 
 ScriptHtlc = namedtuple('ScriptHtlc', ['redeem_script', 'htlc'])
 
@@ -359,7 +361,7 @@ def make_htlc_tx_with_open_channel(chan: 'Channel', pcp: bytes, for_us: bool,
     is_htlc_success = for_us == we_receive
     script, htlc_tx_output = make_htlc_tx_output(
         amount_msat = amount_msat,
-        local_feerate = chan.pending_feerate(LOCAL if for_us else REMOTE), # uses pending feerate..
+        local_feerate = chan.get_next_feerate(LOCAL if for_us else REMOTE),
         revocationpubkey=other_revocation_pubkey,
         local_delayedpubkey=delayedpubkey,
         success = is_htlc_success,

electrum/tests/test_lnchannel.py

@@ -31,6 +31,7 @@ from electrum import lnchannel
 from electrum import lnutil
 from electrum import bip32 as bip32_utils
 from electrum.lnutil import SENT, LOCAL, REMOTE, RECEIVED
+from electrum.lnutil import FeeUpdate
 from electrum.ecc import sig_string_from_der_sig
 from electrum.logging import console_stderr_handler
 
@@ -92,8 +93,8 @@ def create_channel_state(funding_txid, funding_index, funding_sat, local_feerate
                 capacity=funding_sat,
                 is_initiator=is_initiator,
                 funding_txn_minimum_depth=3,
-                feerate=local_feerate,
             ),
+            "fee_updates": [FeeUpdate(rate=local_feerate, ctn={LOCAL:0, REMOTE:0})],
             "node_id":other_node_id,
             "remote_commitment_to_be_revoked": None,
             'onion_keys': {},
@@ -527,31 +528,34 @@ class TestChannel(unittest.TestCase):
         return fee
 
     def test_UpdateFeeSenderCommits(self):
-        old_feerate = self.alice_channel.pending_feerate(LOCAL)
-        fee = self.alice_to_bob_fee_update()
-
         alice_channel, bob_channel = self.alice_channel, self.bob_channel
 
-        self.assertEqual(self.alice_channel.pending_feerate(LOCAL), old_feerate)
+        old_feerate = alice_channel.get_next_feerate(LOCAL)
+
+        fee = self.alice_to_bob_fee_update()
+        self.assertEqual(alice_channel.get_next_feerate(LOCAL), old_feerate)
+
         alice_sig, alice_htlc_sigs = alice_channel.sign_next_commitment()
-        self.assertEqual(self.alice_channel.pending_feerate(LOCAL), old_feerate)
+        #self.assertEqual(alice_channel.get_next_feerate(LOCAL), old_feerate)
 
         bob_channel.receive_new_commitment(alice_sig, alice_htlc_sigs)
 
-        self.assertNotEqual(fee, bob_channel.constraints.feerate)
+        self.assertNotEqual(fee, bob_channel.get_current_feerate(LOCAL))
         rev, _ = bob_channel.revoke_current_commitment()
-        self.assertEqual(fee, bob_channel.constraints.feerate)
+        self.assertEqual(fee, bob_channel.get_current_feerate(LOCAL))
 
-        bob_sig, bob_htlc_sigs = bob_channel.sign_next_commitment()
         alice_channel.receive_revocation(rev)
+
+        
+        bob_sig, bob_htlc_sigs = bob_channel.sign_next_commitment()
         alice_channel.receive_new_commitment(bob_sig, bob_htlc_sigs)
 
-        self.assertNotEqual(fee, alice_channel.constraints.feerate)
+        self.assertNotEqual(fee, alice_channel.get_current_feerate(LOCAL))
         rev, _ = alice_channel.revoke_current_commitment()
-        self.assertEqual(fee, alice_channel.constraints.feerate)
+        self.assertEqual(fee, alice_channel.get_current_feerate(LOCAL))
 
         bob_channel.receive_revocation(rev)
-        self.assertEqual(fee, bob_channel.constraints.feerate)
+        self.assertEqual(fee, bob_channel.get_current_feerate(LOCAL))
 
 
     def test_UpdateFeeReceiverCommits(self):
@@ -567,20 +571,20 @@ class TestChannel(unittest.TestCase):
         alice_sig, alice_htlc_sigs = alice_channel.sign_next_commitment()
         bob_channel.receive_new_commitment(alice_sig, alice_htlc_sigs)
 
-        self.assertNotEqual(fee, bob_channel.constraints.feerate)
+        self.assertNotEqual(fee, bob_channel.get_current_feerate(LOCAL))
         bob_revocation, _ = bob_channel.revoke_current_commitment()
-        self.assertEqual(fee, bob_channel.constraints.feerate)
+        self.assertEqual(fee, bob_channel.get_current_feerate(LOCAL))
 
         bob_sig, bob_htlc_sigs = bob_channel.sign_next_commitment()
         alice_channel.receive_revocation(bob_revocation)
         alice_channel.receive_new_commitment(bob_sig, bob_htlc_sigs)
 
-        self.assertNotEqual(fee, alice_channel.constraints.feerate)
+        self.assertNotEqual(fee, alice_channel.get_current_feerate(LOCAL))
         alice_revocation, _ = alice_channel.revoke_current_commitment()
-        self.assertEqual(fee, alice_channel.constraints.feerate)
+        self.assertEqual(fee, alice_channel.get_current_feerate(LOCAL))
 
         bob_channel.receive_revocation(alice_revocation)
-        self.assertEqual(fee, bob_channel.constraints.feerate)
+        self.assertEqual(fee, bob_channel.get_current_feerate(LOCAL))
 
     @unittest.skip("broken probably because we havn't implemented detecting when we come out of a situation where we violate reserve")
     def test_AddHTLCNegativeBalance(self):
@@ -798,7 +802,7 @@ class TestDust(unittest.TestCase):
 
         paymentPreimage = b"\x01" * 32
         paymentHash = bitcoin.sha256(paymentPreimage)
-        fee_per_kw = alice_channel.constraints.feerate
+        fee_per_kw = alice_channel.get_current_feerate(LOCAL)
         self.assertEqual(fee_per_kw, 6000)
         htlcAmt = 500 + lnutil.HTLC_TIMEOUT_WEIGHT * (fee_per_kw // 1000)
         self.assertEqual(htlcAmt, 4478)