From 95f42df0ac13ae820edba97ea88eb20765f06294 Mon Sep 17 00:00:00 2001 From: bitromortac Date: Thu, 25 Nov 2021 10:11:44 +0100 Subject: [PATCH 1/4] lnwire: add fee_range tlv --- electrum/lnwire/peer_wire.csv | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/electrum/lnwire/peer_wire.csv b/electrum/lnwire/peer_wire.csv index 17b8a103d557..7ab89e918136 100644 --- a/electrum/lnwire/peer_wire.csv +++ b/electrum/lnwire/peer_wire.csv @@ -94,6 +94,10 @@ msgtype,closing_signed,39 msgdata,closing_signed,channel_id,channel_id, msgdata,closing_signed,fee_satoshis,u64, msgdata,closing_signed,signature,signature, +msgdata,closing_signed,tlvs,closing_signed_tlvs, +tlvtype,closing_signed_tlvs,fee_range,1 +tlvdata,closing_signed_tlvs,fee_range,min_fee_satoshis,u64, +tlvdata,closing_signed_tlvs,fee_range,max_fee_satoshis,u64, msgtype,update_add_htlc,128 msgdata,update_add_htlc,channel_id,channel_id, msgdata,update_add_htlc,id,u64, From ec740d45f1c4dc52e8e0018dbc47e9ea04069e34 Mon Sep 17 00:00:00 2001 From: bitromortac Date: Fri, 26 Nov 2021 09:45:06 +0100 Subject: [PATCH 2/4] lnpeer: modern fee negotiation Updates the closing fee negotiation to comply with most recent spec changes, see https://github.com/lightning/bolts/pull/847 The closing negotiation is backwards compatible with the old negotiation. --- electrum/lnpeer.py | 154 ++++++++++++++++++++++++++---- electrum/tests/regtest.py | 3 + electrum/tests/regtest/regtest.sh | 12 +++ 3 files changed, 152 insertions(+), 17 deletions(-) diff --git a/electrum/lnpeer.py b/electrum/lnpeer.py index a35b72b35d3d..0d17b896ce3c 100644 --- a/electrum/lnpeer.py +++ b/electrum/lnpeer.py @@ -51,6 +51,7 @@ from .lnutil import ln_dummy_address from .json_db import StoredDict from .invoices import PR_PAID +from .simple_config import FEE_LN_ETA_TARGET if TYPE_CHECKING: from .lnworker import LNGossip, LNWallet @@ -1840,30 +1841,68 @@ async def _shutdown(self, chan: Channel, payload, *, is_local: bool): assert our_scriptpubkey # estimate fee of closing tx our_sig, closing_tx = chan.make_closing_tx(our_scriptpubkey, their_scriptpubkey, fee_sat=0) - fee_rate = self.network.config.fee_per_kb() - our_fee = fee_rate * closing_tx.estimated_size() // 1000 + fee_rate_per_kb = self.network.config.eta_target_to_fee(FEE_LN_ETA_TARGET) + if not fee_rate_per_kb: # fallback + fee_rate_per_kb = self.network.config.fee_per_kb() + our_fee = fee_rate_per_kb * closing_tx.estimated_size() // 1000 + # TODO: anchors: remove this, as commitment fee rate can be below chain head fee rate? # BOLT2: The sending node MUST set fee less than or equal to the base fee of the final ctx max_fee = chan.get_latest_fee(LOCAL if is_local else REMOTE) our_fee = min(our_fee, max_fee) - drop_to_remote = False + + drop_to_remote = False # does the peer drop its to_local output or not? def send_closing_signed(): + MODERN_FEE = True + if MODERN_FEE: + nonlocal fee_range_sent # we change fee_range_sent in outer scope + fee_range_sent = fee_range + closing_signed_tlvs = {'fee_range': fee_range} + else: + closing_signed_tlvs = {} + our_sig, closing_tx = chan.make_closing_tx(our_scriptpubkey, their_scriptpubkey, fee_sat=our_fee, drop_remote=drop_to_remote) - self.send_message('closing_signed', channel_id=chan.channel_id, fee_satoshis=our_fee, signature=our_sig) + self.logger.info(f"Sending fee range: {closing_signed_tlvs} and fee: {our_fee}") + self.send_message( + 'closing_signed', + channel_id=chan.channel_id, + fee_satoshis=our_fee, + signature=our_sig, + closing_signed_tlvs=closing_signed_tlvs, + ) def verify_signature(tx, sig): their_pubkey = chan.config[REMOTE].multisig_key.pubkey preimage_hex = tx.serialize_preimage(0) pre_hash = sha256d(bfh(preimage_hex)) return ecc.verify_signature(their_pubkey, sig, pre_hash) + + # this is the fee range we initially try to enforce + # we aim at a fee between next block inclusion and some lower value + fee_range = {'min_fee_satoshis': our_fee // 2, 'max_fee_satoshis': our_fee * 2} + their_fee = None + fee_range_sent = {} + is_initiator = chan.constraints.is_initiator # the funder sends the first 'closing_signed' message - if chan.constraints.is_initiator: + if is_initiator: send_closing_signed() + # negotiate fee while True: - # FIXME: the remote SHOULD send closing_signed, but some don't. - cs_payload = await self.wait_for_message('closing_signed', chan.channel_id) + try: + cs_payload = await self.wait_for_message('closing_signed', chan.channel_id) + except asyncio.exceptions.TimeoutError: + if not is_initiator and not their_fee: # we only force close if a peer doesn't reply + self.lnworker.schedule_force_closing(chan.channel_id) + raise Exception("Peer didn't reply with closing signed, force closed.") + else: + # situation when we as an initiator send a fee and the recipient + # already agrees with that fee, but doens't tell us + raise Exception("Peer didn't reply, probably already closed.") + + their_previous_fee = their_fee their_fee = cs_payload['fee_satoshis'] - if their_fee > max_fee: - raise Exception(f'the proposed fee exceeds the base fee of the latest commitment transaction {is_local, their_fee, max_fee}') + + # 0. integrity checks + # determine their closing transaction their_sig = cs_payload['signature'] # verify their sig: they might have dropped their output our_sig, closing_tx = chan.make_closing_tx(our_scriptpubkey, their_scriptpubkey, fee_sat=their_fee, drop_remote=False) @@ -1885,17 +1924,98 @@ def verify_signature(tx, sig): to_remote_amount = closing_tx.outputs()[to_remote_idx].value transaction.check_scriptpubkey_template_and_dust(their_scriptpubkey, to_remote_amount) - # Agree if difference is lower or equal to one (see below) - if abs(our_fee - their_fee) < 2: + # 1. check fees + # if fee_satoshis is equal to its previously sent fee_satoshis: + if our_fee == their_fee: + # SHOULD sign and broadcast the final closing transaction. + break # we publish + + # 2. at start, adapt our fee range if we are not the channel initiator + fee_range_received = cs_payload['closing_signed_tlvs'].get('fee_range') + self.logger.info(f"Received fee range: {fee_range_received} and fee: {their_fee}") + # The sending node: if it is not the funder: + if fee_range_received and not is_initiator and not fee_range_sent: + # SHOULD set max_fee_satoshis to at least the max_fee_satoshis received + fee_range['max_fee_satoshis'] = max(fee_range_received['max_fee_satoshis'], fee_range['max_fee_satoshis']) + # SHOULD set min_fee_satoshis to a fairly low value + # TODO: what's fairly low value? allows the initiator to go to low values + fee_range['min_fee_satoshis'] = fee_range['max_fee_satoshis'] // 2 + + # 3. if fee_satoshis matches its previously sent fee_range: + if fee_range_sent and (fee_range_sent['min_fee_satoshis'] <= their_fee <= fee_range_sent['max_fee_satoshis']): + # SHOULD reply with a closing_signed with the same fee_satoshis value if it is different from its previously sent fee_satoshis + if our_fee != their_fee: + our_fee = their_fee + send_closing_signed() # peer publishes + break + # SHOULD use `fee_satoshis` to sign and broadcast the final closing transaction + else: + our_fee = their_fee + break # we publish + + # 4. if the message contains a fee_range + if fee_range_received: + overlap_min = max(fee_range['min_fee_satoshis'], fee_range_received['min_fee_satoshis']) + overlap_max = min(fee_range['max_fee_satoshis'], fee_range_received['max_fee_satoshis']) + # if there is no overlap between that and its own fee_range + if overlap_min > overlap_max: + raise Exception("There is no overlap between between their and our fee range.") + # TODO: MUST fail the channel if it doesn't receive a satisfying fee_range after a reasonable amount of time + # otherwise: + else: + if is_initiator: + # if fee_satoshis is not in the overlap between the sent and received fee_range: + if not (overlap_min <= their_fee <= overlap_max): + # MUST fail the channel + self.lnworker.schedule_force_closing(chan.channel_id) + raise Exception("Their fee is not in the overlap region, we force closed.") + # otherwise: + else: + our_fee = their_fee + # MUST reply with the same fee_satoshis. + send_closing_signed() # peer publishes + break + # otherwise (it is not the funder): + else: + # if it has already sent a closing_signed: + if fee_range_sent: + # if fee_satoshis is not the same as the value it sent: + if their_fee != our_fee: + # MUST fail the channel + self.lnworker.schedule_force_closing(chan.channel_id) + raise Exception("Expected the same fee as ours, we force closed.") + # otherwise: + else: + # MUST propose a fee_satoshis in the overlap between received and (about-to-be) sent fee_range. + our_fee = (overlap_min + overlap_max) // 2 + send_closing_signed() + continue + # otherwise, if fee_satoshis is not strictly between its last-sent fee_satoshis + # and its previously-received fee_satoshis, UNLESS it has since reconnected: + elif their_previous_fee and not (min(our_fee, their_previous_fee) < their_fee < max(our_fee, their_previous_fee)): + # SHOULD fail the connection. + raise Exception('Their fee is not between our last sent and their last sent fee.') + # otherwise, if the receiver agrees with the fee: + elif abs(their_fee - our_fee) <= 1: # we cannot have another strictly in-between value + # SHOULD reply with a closing_signed with the same fee_satoshis value. our_fee = their_fee + send_closing_signed() # peer publishes break - # this will be "strictly between" (as in BOLT2) previous values because of the above - our_fee = (our_fee + their_fee) // 2 - # another round - send_closing_signed() - # the non-funder replies - if not chan.constraints.is_initiator: + # otherwise: + else: + # MUST propose a value "strictly between" the received fee_satoshis and its previously-sent fee_satoshis. + our_fee_proposed = (our_fee + their_fee) // 2 + if not (min(our_fee, their_fee) < our_fee_proposed < max(our_fee, their_fee)): + our_fee_proposed += (their_fee - our_fee) // 2 + else: + our_fee = our_fee_proposed + send_closing_signed() + + # reaching this part of the code means that we have reached agreement; to make + # sure the peer doesn't force close, send a last closing_signed + if not is_initiator: send_closing_signed() + # add signatures closing_tx.add_signature_to_txin( txin_idx=0, diff --git a/electrum/tests/regtest.py b/electrum/tests/regtest.py index b42e4b3315ac..53eaa3e11391 100644 --- a/electrum/tests/regtest.py +++ b/electrum/tests/regtest.py @@ -44,6 +44,9 @@ def test_unixsockets(self): class TestLightningAB(TestLightning): agents = ['alice', 'bob'] + def test_collaborative_close(self): + self.run_shell(['collaborative_close']) + def test_backup(self): self.run_shell(['backup']) diff --git a/electrum/tests/regtest/regtest.sh b/electrum/tests/regtest/regtest.sh index b3b366f5272d..bd3797573587 100755 --- a/electrum/tests/regtest/regtest.sh +++ b/electrum/tests/regtest/regtest.sh @@ -158,6 +158,18 @@ if [[ $1 == "backup" ]]; then fi +if [[ $1 == "collaborative_close" ]]; then + wait_for_balance alice 1 + echo "alice opens channel" + bob_node=$($bob nodeid) + channel=$($alice open_channel $bob_node 0.15) + new_blocks 3 + wait_until_channel_open alice + echo "alice closes channel" + request=$($bob close_channel $channel) +fi + + if [[ $1 == "extract_preimage" ]]; then # instead of settling bob will broadcast $bob enable_htlc_settle false From 0b203f0b94e9396d57b17b10a3ff53b65ef5db5f Mon Sep 17 00:00:00 2001 From: ThomasV Date: Tue, 22 Feb 2022 18:25:24 +0100 Subject: [PATCH 3/4] lnpeer: refactor fee negotiation in _shutdown - the fee negotiation is split into smaller functions, reducing the scope of variables. - the while loop logic is condensed in a few lines, so it is easier to understand termination conditions. - removed code that was never executed --- electrum/lnpeer.py | 203 +++++++++++++++------------------- electrum/tests/test_lnpeer.py | 13 ++- 2 files changed, 100 insertions(+), 116 deletions(-) diff --git a/electrum/lnpeer.py b/electrum/lnpeer.py index 0d17b896ce3c..0920a6f5896c 100644 --- a/electrum/lnpeer.py +++ b/electrum/lnpeer.py @@ -1850,17 +1850,22 @@ async def _shutdown(self, chan: Channel, payload, *, is_local: bool): max_fee = chan.get_latest_fee(LOCAL if is_local else REMOTE) our_fee = min(our_fee, max_fee) - drop_to_remote = False # does the peer drop its to_local output or not? - def send_closing_signed(): - MODERN_FEE = True - if MODERN_FEE: - nonlocal fee_range_sent # we change fee_range_sent in outer scope - fee_range_sent = fee_range - closing_signed_tlvs = {'fee_range': fee_range} + is_initiator = chan.constraints.is_initiator + # config modern_fee_negotiation can be set in tests + if self.network.config.get('modern_fee_negotiation', True): + # this is the fee range we initially try to enforce + # we aim at a fee between next block inclusion and some lower value + our_fee_range = {'min_fee_satoshis': our_fee // 2, 'max_fee_satoshis': our_fee * 2} + self.logger.info(f"Our fee range: {our_fee_range} and fee: {our_fee}") + else: + our_fee_range = None + + def send_closing_signed(our_fee, our_fee_range, drop_remote): + if our_fee_range: + closing_signed_tlvs = {'fee_range': our_fee_range} else: closing_signed_tlvs = {} - - our_sig, closing_tx = chan.make_closing_tx(our_scriptpubkey, their_scriptpubkey, fee_sat=our_fee, drop_remote=drop_to_remote) + our_sig, closing_tx = chan.make_closing_tx(our_scriptpubkey, their_scriptpubkey, fee_sat=our_fee, drop_remote=drop_remote) self.logger.info(f"Sending fee range: {closing_signed_tlvs} and fee: {our_fee}") self.send_message( 'closing_signed', @@ -1869,49 +1874,30 @@ def send_closing_signed(): signature=our_sig, closing_signed_tlvs=closing_signed_tlvs, ) + def verify_signature(tx, sig): their_pubkey = chan.config[REMOTE].multisig_key.pubkey preimage_hex = tx.serialize_preimage(0) pre_hash = sha256d(bfh(preimage_hex)) return ecc.verify_signature(their_pubkey, sig, pre_hash) - # this is the fee range we initially try to enforce - # we aim at a fee between next block inclusion and some lower value - fee_range = {'min_fee_satoshis': our_fee // 2, 'max_fee_satoshis': our_fee * 2} - their_fee = None - fee_range_sent = {} - is_initiator = chan.constraints.is_initiator - # the funder sends the first 'closing_signed' message - if is_initiator: - send_closing_signed() - - # negotiate fee - while True: + async def receive_closing_signed(): try: cs_payload = await self.wait_for_message('closing_signed', chan.channel_id) except asyncio.exceptions.TimeoutError: - if not is_initiator and not their_fee: # we only force close if a peer doesn't reply - self.lnworker.schedule_force_closing(chan.channel_id) - raise Exception("Peer didn't reply with closing signed, force closed.") - else: - # situation when we as an initiator send a fee and the recipient - # already agrees with that fee, but doens't tell us - raise Exception("Peer didn't reply, probably already closed.") - - their_previous_fee = their_fee + self.lnworker.schedule_force_closing(chan.channel_id) + raise Exception("closing_signed not received, force closing.") their_fee = cs_payload['fee_satoshis'] - - # 0. integrity checks - # determine their closing transaction + their_fee_range = cs_payload['closing_signed_tlvs'].get('fee_range') their_sig = cs_payload['signature'] - # verify their sig: they might have dropped their output + # perform checks our_sig, closing_tx = chan.make_closing_tx(our_scriptpubkey, their_scriptpubkey, fee_sat=their_fee, drop_remote=False) if verify_signature(closing_tx, their_sig): - drop_to_remote = False + drop_remote = False else: our_sig, closing_tx = chan.make_closing_tx(our_scriptpubkey, their_scriptpubkey, fee_sat=their_fee, drop_remote=True) if verify_signature(closing_tx, their_sig): - drop_to_remote = True + drop_remote = True else: # this can happen if we consider our output too valuable to drop, # but the remote drops it because it violates their dust limit @@ -1919,102 +1905,91 @@ def verify_signature(tx, sig): # at this point we know how the closing tx looks like # check that their output is above their scriptpubkey's network dust limit to_remote_set = closing_tx.get_output_idxs_from_scriptpubkey(their_scriptpubkey.hex()) - if not drop_to_remote and to_remote_set: + if not drop_remote and to_remote_set: to_remote_idx = to_remote_set.pop() to_remote_amount = closing_tx.outputs()[to_remote_idx].value transaction.check_scriptpubkey_template_and_dust(their_scriptpubkey, to_remote_amount) + return their_fee, their_fee_range, their_sig, drop_remote - # 1. check fees - # if fee_satoshis is equal to its previously sent fee_satoshis: - if our_fee == their_fee: - # SHOULD sign and broadcast the final closing transaction. - break # we publish - - # 2. at start, adapt our fee range if we are not the channel initiator - fee_range_received = cs_payload['closing_signed_tlvs'].get('fee_range') - self.logger.info(f"Received fee range: {fee_range_received} and fee: {their_fee}") - # The sending node: if it is not the funder: - if fee_range_received and not is_initiator and not fee_range_sent: + def choose_new_fee(our_fee, our_fee_range, their_fee, their_fee_range, their_previous_fee): + assert our_fee != their_fee + fee_range_sent = our_fee_range and (is_initiator or (their_previous_fee is not None)) + + # The sending node, if it is not the funder: + if our_fee_range and their_fee_range and not is_initiator: # SHOULD set max_fee_satoshis to at least the max_fee_satoshis received - fee_range['max_fee_satoshis'] = max(fee_range_received['max_fee_satoshis'], fee_range['max_fee_satoshis']) + our_fee_range['max_fee_satoshis'] = max(their_fee_range['max_fee_satoshis'], our_fee_range['max_fee_satoshis']) # SHOULD set min_fee_satoshis to a fairly low value # TODO: what's fairly low value? allows the initiator to go to low values - fee_range['min_fee_satoshis'] = fee_range['max_fee_satoshis'] // 2 + our_fee_range['min_fee_satoshis'] = our_fee_range['max_fee_satoshis'] // 2 - # 3. if fee_satoshis matches its previously sent fee_range: - if fee_range_sent and (fee_range_sent['min_fee_satoshis'] <= their_fee <= fee_range_sent['max_fee_satoshis']): + # the receiving node, if fee_satoshis matches its previously sent fee_range, + if fee_range_sent and (our_fee_range['min_fee_satoshis'] <= their_fee <= our_fee_range['max_fee_satoshis']): # SHOULD reply with a closing_signed with the same fee_satoshis value if it is different from its previously sent fee_satoshis - if our_fee != their_fee: - our_fee = their_fee - send_closing_signed() # peer publishes - break - # SHOULD use `fee_satoshis` to sign and broadcast the final closing transaction - else: - our_fee = their_fee - break # we publish + our_fee = their_fee - # 4. if the message contains a fee_range - if fee_range_received: - overlap_min = max(fee_range['min_fee_satoshis'], fee_range_received['min_fee_satoshis']) - overlap_max = min(fee_range['max_fee_satoshis'], fee_range_received['max_fee_satoshis']) + # the receiving node, if the message contains a fee_range + elif our_fee_range and their_fee_range: + overlap_min = max(our_fee_range['min_fee_satoshis'], their_fee_range['min_fee_satoshis']) + overlap_max = min(our_fee_range['max_fee_satoshis'], their_fee_range['max_fee_satoshis']) # if there is no overlap between that and its own fee_range if overlap_min > overlap_max: - raise Exception("There is no overlap between between their and our fee range.") # TODO: MUST fail the channel if it doesn't receive a satisfying fee_range after a reasonable amount of time - # otherwise: + raise Exception("There is no overlap between between their and our fee range.") + # otherwise, if it is the funder + if is_initiator: + # if fee_satoshis is not in the overlap between the sent and received fee_range: + if not (overlap_min <= their_fee <= overlap_max): + # MUST fail the channel + self.lnworker.schedule_force_closing(chan.channel_id) + raise Exception("Their fee is not in the overlap region, we force closed.") + # otherwise, MUST reply with the same fee_satoshis. + our_fee = their_fee + # otherwise (it is not the funder): else: - if is_initiator: - # if fee_satoshis is not in the overlap between the sent and received fee_range: - if not (overlap_min <= their_fee <= overlap_max): - # MUST fail the channel - self.lnworker.schedule_force_closing(chan.channel_id) - raise Exception("Their fee is not in the overlap region, we force closed.") - # otherwise: - else: - our_fee = their_fee - # MUST reply with the same fee_satoshis. - send_closing_signed() # peer publishes - break - # otherwise (it is not the funder): - else: - # if it has already sent a closing_signed: - if fee_range_sent: - # if fee_satoshis is not the same as the value it sent: - if their_fee != our_fee: - # MUST fail the channel - self.lnworker.schedule_force_closing(chan.channel_id) - raise Exception("Expected the same fee as ours, we force closed.") - # otherwise: - else: - # MUST propose a fee_satoshis in the overlap between received and (about-to-be) sent fee_range. - our_fee = (overlap_min + overlap_max) // 2 - send_closing_signed() - continue - # otherwise, if fee_satoshis is not strictly between its last-sent fee_satoshis - # and its previously-received fee_satoshis, UNLESS it has since reconnected: - elif their_previous_fee and not (min(our_fee, their_previous_fee) < their_fee < max(our_fee, their_previous_fee)): - # SHOULD fail the connection. - raise Exception('Their fee is not between our last sent and their last sent fee.') - # otherwise, if the receiver agrees with the fee: - elif abs(their_fee - our_fee) <= 1: # we cannot have another strictly in-between value - # SHOULD reply with a closing_signed with the same fee_satoshis value. - our_fee = their_fee - send_closing_signed() # peer publishes - break - # otherwise: + # if it has already sent a closing_signed: + if fee_range_sent: + # fee_satoshis is not the same as the value we sent, we MUST fail the channel + self.lnworker.schedule_force_closing(chan.channel_id) + raise Exception("Expected the same fee as ours, we force closed.") + # otherwise: + # MUST propose a fee_satoshis in the overlap between received and (about-to-be) sent fee_range. + our_fee = (overlap_min + overlap_max) // 2 else: - # MUST propose a value "strictly between" the received fee_satoshis and its previously-sent fee_satoshis. - our_fee_proposed = (our_fee + their_fee) // 2 - if not (min(our_fee, their_fee) < our_fee_proposed < max(our_fee, their_fee)): - our_fee_proposed += (their_fee - our_fee) // 2 + # otherwise, if fee_satoshis is not strictly between its last-sent fee_satoshis + # and its previously-received fee_satoshis, UNLESS it has since reconnected: + if their_previous_fee and not (min(our_fee, their_previous_fee) < their_fee < max(our_fee, their_previous_fee)): + # SHOULD fail the connection. + raise Exception('Their fee is not between our last sent and their last sent fee.') + # accept their fee if they are very close + if abs(their_fee - our_fee) < 2: + our_fee = their_fee else: - our_fee = our_fee_proposed - send_closing_signed() + # this will be "strictly between" (as in BOLT2) previous values because of the above + our_fee = (our_fee + their_fee) // 2 - # reaching this part of the code means that we have reached agreement; to make - # sure the peer doesn't force close, send a last closing_signed + return our_fee, our_fee_range + + # Fee negotiation: both parties exchange 'funding_signed' messages. + # The funder sends the first message, the non-funder sends the last message. + # In the 'modern' case, at most 3 messages are exchanged, because choose_new_fee of the funder either returns their_fee or fails + their_fee = None + drop_remote = False # does the peer drop its to_local output or not? + if is_initiator: + send_closing_signed(our_fee, our_fee_range, drop_remote) + while True: + their_previous_fee = their_fee + their_fee, their_fee_range, their_sig, drop_remote = await receive_closing_signed() + if our_fee == their_fee: + break + our_fee, our_fee_range = choose_new_fee(our_fee, our_fee_range, their_fee, their_fee_range, their_previous_fee) + if not is_initiator and our_fee == their_fee: + break + send_closing_signed(our_fee, our_fee_range, drop_remote) + if is_initiator and our_fee == their_fee: + break if not is_initiator: - send_closing_signed() + send_closing_signed(our_fee, our_fee_range, drop_remote) # add signatures closing_tx.add_signature_to_txin( diff --git a/electrum/tests/test_lnpeer.py b/electrum/tests/test_lnpeer.py index 2d5bbb33a94b..d964ad35008d 100644 --- a/electrum/tests/test_lnpeer.py +++ b/electrum/tests/test_lnpeer.py @@ -1063,13 +1063,22 @@ async def f(): run(f()) @needs_test_with_all_chacha20_implementations - def test_close(self): + def test_close_modern(self): + self._test_close(True, True) + + @needs_test_with_all_chacha20_implementations + def test_close_old_style(self): + self._test_close(False, False) + + def _test_close(self, modern_alice, modern_bob): alice_channel, bob_channel = create_test_channels() p1, p2, w1, w2, _q1, _q2 = self.prepare_peers(alice_channel, bob_channel) + w1.network.config.set_key('modern_fee_negotiation', modern_alice) + w2.network.config.set_key('modern_fee_negotiation', modern_bob) w1.network.config.set_key('dynamic_fees', False) w2.network.config.set_key('dynamic_fees', False) w1.network.config.set_key('fee_per_kb', 5000) - w2.network.config.set_key('fee_per_kb', 1000) + w2.network.config.set_key('fee_per_kb', 2000) w2.enable_htlc_settle = False lnaddr, pay_req = run(self.prepare_invoice(w2)) async def pay(): From 6667a79f10d9bd1edcb86fdab1a54e9913b2ccb2 Mon Sep 17 00:00:00 2001 From: ThomasV Date: Tue, 8 Mar 2022 11:34:57 +0100 Subject: [PATCH 4/4] modern shutdown: - clarify TODOs - add tests for shutdown with modern negotiation --- electrum/lnpeer.py | 54 ++++++++++++++++++++++------------- electrum/tests/test_lnpeer.py | 46 ++++++++++++++++++++++------- 2 files changed, 69 insertions(+), 31 deletions(-) diff --git a/electrum/lnpeer.py b/electrum/lnpeer.py index 0920a6f5896c..8d32372c5d8f 100644 --- a/electrum/lnpeer.py +++ b/electrum/lnpeer.py @@ -1825,6 +1825,31 @@ async def send_shutdown(self, chan: Channel): # can fullfill or fail htlcs. cannot add htlcs, because state != OPEN chan.set_can_send_ctx_updates(True) + def get_shutdown_fee_range(self, chan, closing_tx, is_local): + """ return the closing fee and fee range we initially try to enforce """ + config = self.network.config + if config.get('test_shutdown_fee'): + our_fee = config.get('test_shutdown_fee') + else: + fee_rate_per_kb = config.eta_target_to_fee(FEE_LN_ETA_TARGET) + if not fee_rate_per_kb: # fallback + fee_rate_per_kb = self.network.config.fee_per_kb() + our_fee = fee_rate_per_kb * closing_tx.estimated_size() // 1000 + # TODO: anchors: remove this, as commitment fee rate can be below chain head fee rate? + # BOLT2: The sending node MUST set fee less than or equal to the base fee of the final ctx + max_fee = chan.get_latest_fee(LOCAL if is_local else REMOTE) + our_fee = min(our_fee, max_fee) + # config modern_fee_negotiation can be set in tests + if config.get('test_shutdown_legacy'): + our_fee_range = None + elif config.get('test_shutdown_fee_range'): + our_fee_range = config.get('test_shutdown_fee_range') + else: + # we aim at a fee between next block inclusion and some lower value + our_fee_range = {'min_fee_satoshis': our_fee // 2, 'max_fee_satoshis': our_fee * 2} + self.logger.info(f"Our fee range: {our_fee_range} and fee: {our_fee}") + return our_fee, our_fee_range + @log_exceptions async def _shutdown(self, chan: Channel, payload, *, is_local: bool): # wait until no HTLCs remain in either commitment transaction @@ -1841,24 +1866,9 @@ async def _shutdown(self, chan: Channel, payload, *, is_local: bool): assert our_scriptpubkey # estimate fee of closing tx our_sig, closing_tx = chan.make_closing_tx(our_scriptpubkey, their_scriptpubkey, fee_sat=0) - fee_rate_per_kb = self.network.config.eta_target_to_fee(FEE_LN_ETA_TARGET) - if not fee_rate_per_kb: # fallback - fee_rate_per_kb = self.network.config.fee_per_kb() - our_fee = fee_rate_per_kb * closing_tx.estimated_size() // 1000 - # TODO: anchors: remove this, as commitment fee rate can be below chain head fee rate? - # BOLT2: The sending node MUST set fee less than or equal to the base fee of the final ctx - max_fee = chan.get_latest_fee(LOCAL if is_local else REMOTE) - our_fee = min(our_fee, max_fee) is_initiator = chan.constraints.is_initiator - # config modern_fee_negotiation can be set in tests - if self.network.config.get('modern_fee_negotiation', True): - # this is the fee range we initially try to enforce - # we aim at a fee between next block inclusion and some lower value - our_fee_range = {'min_fee_satoshis': our_fee // 2, 'max_fee_satoshis': our_fee * 2} - self.logger.info(f"Our fee range: {our_fee_range} and fee: {our_fee}") - else: - our_fee_range = None + our_fee, our_fee_range = self.get_shutdown_fee_range(chan, closing_tx, is_local) def send_closing_signed(our_fee, our_fee_range, drop_remote): if our_fee_range: @@ -1916,12 +1926,14 @@ def choose_new_fee(our_fee, our_fee_range, their_fee, their_fee_range, their_pre fee_range_sent = our_fee_range and (is_initiator or (their_previous_fee is not None)) # The sending node, if it is not the funder: - if our_fee_range and their_fee_range and not is_initiator: + if our_fee_range and their_fee_range and not is_initiator and not self.network.config.get('test_shutdown_fee_range'): # SHOULD set max_fee_satoshis to at least the max_fee_satoshis received our_fee_range['max_fee_satoshis'] = max(their_fee_range['max_fee_satoshis'], our_fee_range['max_fee_satoshis']) # SHOULD set min_fee_satoshis to a fairly low value - # TODO: what's fairly low value? allows the initiator to go to low values - our_fee_range['min_fee_satoshis'] = our_fee_range['max_fee_satoshis'] // 2 + our_fee_range['min_fee_satoshis'] = min(their_fee_range['min_fee_satoshis'], our_fee_range['min_fee_satoshis']) + # Note: the BOLT describes what the sending node SHOULD do. + # However, this assumes that we have decided to send 'funding_signed' in response to their fee_range. + # In practice, we might prefer to fail the channel in some cases (TODO) # the receiving node, if fee_satoshis matches its previously sent fee_range, if fee_range_sent and (our_fee_range['min_fee_satoshis'] <= their_fee <= our_fee_range['max_fee_satoshis']): @@ -1934,7 +1946,9 @@ def choose_new_fee(our_fee, our_fee_range, their_fee, their_fee_range, their_pre overlap_max = min(our_fee_range['max_fee_satoshis'], their_fee_range['max_fee_satoshis']) # if there is no overlap between that and its own fee_range if overlap_min > overlap_max: - # TODO: MUST fail the channel if it doesn't receive a satisfying fee_range after a reasonable amount of time + # TODO: the receiving node should first send a warning, and fail the channel + # only if it doesn't receive a satisfying fee_range after a reasonable amount of time + self.lnworker.schedule_force_closing(chan.channel_id) raise Exception("There is no overlap between between their and our fee range.") # otherwise, if it is the funder if is_initiator: diff --git a/electrum/tests/test_lnpeer.py b/electrum/tests/test_lnpeer.py index d964ad35008d..7cc0716d804c 100644 --- a/electrum/tests/test_lnpeer.py +++ b/electrum/tests/test_lnpeer.py @@ -1063,22 +1063,46 @@ async def f(): run(f()) @needs_test_with_all_chacha20_implementations - def test_close_modern(self): - self._test_close(True, True) + def test_legacy_shutdown_low(self): + self._test_shutdown(alice_fee=100, bob_fee=150) @needs_test_with_all_chacha20_implementations - def test_close_old_style(self): - self._test_close(False, False) + def test_legacy_shutdown_high(self): + self._test_shutdown(alice_fee=2000, bob_fee=100) - def _test_close(self, modern_alice, modern_bob): + @needs_test_with_all_chacha20_implementations + def test_modern_shutdown_with_overlap(self): + self._test_shutdown( + alice_fee=1, + bob_fee=200, + alice_fee_range={'min_fee_satoshis': 1, 'max_fee_satoshis': 10}, + bob_fee_range={'min_fee_satoshis': 10, 'max_fee_satoshis': 300}) + + ## This test works but it is too slow (LN_P2P_NETWORK_TIMEOUT) + ## because tests do not use a proper LNWorker object + #@needs_test_with_all_chacha20_implementations + #def test_modern_shutdown_no_overlap(self): + # self.assertRaises(Exception, lambda: asyncio.run( + # self._test_shutdown( + # alice_fee=1, + # bob_fee=200, + # alice_fee_range={'min_fee_satoshis': 1, 'max_fee_satoshis': 10}, + # bob_fee_range={'min_fee_satoshis': 50, 'max_fee_satoshis': 300}) + # )) + + def _test_shutdown(self, alice_fee, bob_fee, alice_fee_range=None, bob_fee_range=None): alice_channel, bob_channel = create_test_channels() p1, p2, w1, w2, _q1, _q2 = self.prepare_peers(alice_channel, bob_channel) - w1.network.config.set_key('modern_fee_negotiation', modern_alice) - w2.network.config.set_key('modern_fee_negotiation', modern_bob) - w1.network.config.set_key('dynamic_fees', False) - w2.network.config.set_key('dynamic_fees', False) - w1.network.config.set_key('fee_per_kb', 5000) - w2.network.config.set_key('fee_per_kb', 2000) + w1.network.config.set_key('test_shutdown_fee', alice_fee) + w2.network.config.set_key('test_shutdown_fee', bob_fee) + if alice_fee_range is not None: + w1.network.config.set_key('test_shutdown_fee_range', alice_fee_range) + else: + w1.network.config.set_key('test_shutdown_legacy', True) + if bob_fee_range is not None: + w2.network.config.set_key('test_shutdown_fee_range', bob_fee_range) + else: + w2.network.config.set_key('test_shutdown_legacy', True) w2.enable_htlc_settle = False lnaddr, pay_req = run(self.prepare_invoice(w2)) async def pay():