From f2f8c4533bd817775adc0526684d2ea605979785 Mon Sep 17 00:00:00 2001 From: bitromortac Date: Fri, 22 Oct 2021 11:59:44 +0200 Subject: [PATCH 1/3] implement option_shutdown_anysegwit https://github.com/lightningnetwork/lightning-rfc/pull/672 We check the received shutdown script against higher segwit versions and accept closing to that script if option_shutdown_anysegwit has been negotiated. --- electrum/lnpeer.py | 22 +++++++++++++++++----- electrum/lnutil.py | 7 +++++++ electrum/lnworker.py | 3 ++- electrum/tests/test_transaction.py | 12 ++++++++++-- electrum/transaction.py | 19 +++++++++++++++++++ 5 files changed, 55 insertions(+), 8 deletions(-) diff --git a/electrum/lnpeer.py b/electrum/lnpeer.py index 316d10e004ba..8d8309937f24 100644 --- a/electrum/lnpeer.py +++ b/electrum/lnpeer.py @@ -495,6 +495,9 @@ def close_and_cleanup(self): self.lnworker.peer_closed(self) self.got_disconnected.set() + def is_shutdown_anysegwit(self): + return self.features.supports(LnFeatures.OPTION_SHUTDOWN_ANYSEGWIT_OPT) + def is_static_remotekey(self): return self.features.supports(LnFeatures.OPTION_STATIC_REMOTEKEY_OPT) @@ -1692,11 +1695,20 @@ async def on_shutdown(self, chan: Channel, payload): if their_upfront_scriptpubkey: if not (their_scriptpubkey == their_upfront_scriptpubkey): raise UpfrontShutdownScriptViolation("remote didn't use upfront shutdown script it commited to in channel opening") - # BOLT-02 restrict the scriptpubkey to some templates: - if not (match_script_against_template(their_scriptpubkey, transaction.SCRIPTPUBKEY_TEMPLATE_WITNESS_V0) - or match_script_against_template(their_scriptpubkey, transaction.SCRIPTPUBKEY_TEMPLATE_P2SH) - or match_script_against_template(their_scriptpubkey, transaction.SCRIPTPUBKEY_TEMPLATE_P2PKH)): - raise Exception(f'scriptpubkey in received shutdown message does not conform to any template: {their_scriptpubkey.hex()}') + else: + # BOLT-02 restrict the scriptpubkey to some templates: + # order by decreasing dust limit + if match_script_against_template(their_scriptpubkey, transaction.SCRIPTPUBKEY_TEMPLATE_P2PKH): + pass + elif match_script_against_template(their_scriptpubkey, transaction.SCRIPTPUBKEY_TEMPLATE_P2SH): + pass + elif self.is_shutdown_anysegwit() and match_script_against_template(their_scriptpubkey, transaction.SCRIPTPUBKEY_TEMPLATE_ANYSEGWIT): + pass + elif match_script_against_template(their_scriptpubkey, transaction.SCRIPTPUBKEY_TEMPLATE_WITNESS_V0): + pass + else: + raise Exception(f'scriptpubkey in received shutdown message does not conform to any template: {their_scriptpubkey.hex()}') + chan_id = chan.channel_id if chan_id in self.shutdown_received: self.shutdown_received[chan_id].set_result(payload) diff --git a/electrum/lnutil.py b/electrum/lnutil.py index 2e3441c9a5e1..6971de53d9e5 100644 --- a/electrum/lnutil.py +++ b/electrum/lnutil.py @@ -1026,6 +1026,12 @@ class LnFeatures(IntFlag): _ln_feature_contexts[OPTION_TRAMPOLINE_ROUTING_REQ] = (LNFC.INIT | LNFC.NODE_ANN | LNFC.INVOICE) _ln_feature_contexts[OPTION_TRAMPOLINE_ROUTING_OPT] = (LNFC.INIT | LNFC.NODE_ANN | LNFC.INVOICE) + OPTION_SHUTDOWN_ANYSEGWIT_REQ = 1 << 26 + OPTION_SHUTDOWN_ANYSEGWIT_OPT = 1 << 27 + + _ln_feature_contexts[OPTION_SHUTDOWN_ANYSEGWIT_REQ] = (LNFC.INIT | LNFC.NODE_ANN) + _ln_feature_contexts[OPTION_SHUTDOWN_ANYSEGWIT_OPT] = (LNFC.INIT | LNFC.NODE_ANN) + # temporary OPTION_TRAMPOLINE_ROUTING_REQ_ECLAIR = 1 << 50 OPTION_TRAMPOLINE_ROUTING_OPT_ECLAIR = 1 << 51 @@ -1114,6 +1120,7 @@ def supports(self, feature: 'LnFeatures') -> bool: | LnFeatures.PAYMENT_SECRET_OPT | LnFeatures.PAYMENT_SECRET_REQ | LnFeatures.BASIC_MPP_OPT | LnFeatures.BASIC_MPP_REQ | LnFeatures.OPTION_TRAMPOLINE_ROUTING_OPT | LnFeatures.OPTION_TRAMPOLINE_ROUTING_REQ + | LnFeatures.OPTION_SHUTDOWN_ANYSEGWIT_OPT | LnFeatures.OPTION_SHUTDOWN_ANYSEGWIT_REQ ) diff --git a/electrum/lnworker.py b/electrum/lnworker.py index 7a60bd885278..601261538089 100644 --- a/electrum/lnworker.py +++ b/electrum/lnworker.py @@ -175,7 +175,8 @@ class ErrorAddingPeer(Exception): pass | LnFeatures.OPTION_STATIC_REMOTEKEY_REQ\ | LnFeatures.GOSSIP_QUERIES_REQ\ | LnFeatures.BASIC_MPP_OPT\ - | LnFeatures.OPTION_TRAMPOLINE_ROUTING_OPT + | LnFeatures.OPTION_TRAMPOLINE_ROUTING_OPT\ + | LnFeatures.OPTION_SHUTDOWN_ANYSEGWIT_OPT\ LNGOSSIP_FEATURES = BASE_FEATURES\ | LnFeatures.GOSSIP_QUERIES_OPT\ diff --git a/electrum/tests/test_transaction.py b/electrum/tests/test_transaction.py index c14ef61d186d..3b33ac6e6c96 100644 --- a/electrum/tests/test_transaction.py +++ b/electrum/tests/test_transaction.py @@ -3,7 +3,8 @@ from electrum import transaction, bitcoin from electrum.transaction import (convert_raw_tx_to_hex, tx_from_any, Transaction, PartialTransaction, TxOutpoint, PartialTxInput, - PartialTxOutput, Sighash) + PartialTxOutput, Sighash, match_script_against_template, + SCRIPTPUBKEY_TEMPLATE_ANYSEGWIT) from electrum.util import bh2u, bfh from electrum.bitcoin import (deserialize_privkey, opcodes, construct_script, construct_witness) @@ -78,6 +79,13 @@ def test_bool(self): class TestTransaction(ElectrumTestCase): + def test_match_against_script_template(self): + script = bfh(construct_script([opcodes.OP_5, bytes(29)])) + self.assertTrue(match_script_against_template(script, SCRIPTPUBKEY_TEMPLATE_ANYSEGWIT)) + script = bfh(construct_script([opcodes.OP_NOP, bytes(30)])) + self.assertFalse(match_script_against_template(script, SCRIPTPUBKEY_TEMPLATE_ANYSEGWIT)) + script = bfh(construct_script([opcodes.OP_0, bytes(50)])) + self.assertFalse(match_script_against_template(script, SCRIPTPUBKEY_TEMPLATE_ANYSEGWIT)) def test_tx_update_signatures(self): tx = tx_from_any("cHNidP8BAFUBAAAAASpcmpT83pj1WBzQAWLGChOTbOt1OJ6mW/OGM7Qk60AxAAAAAAD/////AUBCDwAAAAAAGXapFCMKw3g0BzpCFG8R74QUrpKf6q/DiKwAAAAAAAAA") @@ -927,7 +935,7 @@ class TestSighashTypes(ElectrumTestCase): txin = PartialTxInput(prevout=prevout) txin.nsequence=0xffffffff txin.script_type='p2sh-p2wsh' - txin.witness_script = bfh('56210307b8ae49ac90a048e9b53357a2354b3334e9c8bee813ecb98e99a7e07e8c3ba32103b28f0c28bfab54554ae8c658ac5c3e0ce6e79ad336331f78c428dd43eea8449b21034b8113d703413d57761b8b9781957b8c0ac1dfe69f492580ca4195f50376ba4a21033400f6afecb833092a9a21cfdf1ed1376e58c5d1f47de74683123987e967a8f42103a6d48b1131e94ba04d9737d61acdaa1322008af9602b3b14862c07a1789aac162102d8b661b0b3302ee2f162b09e07a55ad5dfbe673a9f01d9f0c19617681024306b56ae') + txin.witness_script = bfh('56210307b8ae49ac90a048e9b53357a2354b3334e9c8bee813ecb98e99a7e07e8c3ba32103b28f0c28bfab54554ae8c658ac5c3e0ce6e79ad336331f78c428dd43eea8449b21034b8113d703413d57761b8b9781957b8c0ac1dfe69f492580ca4195f50376ba4a21033400f6afecb833092a9a21cfdf1ed1376e58c5d1f47de74683123987e967a8f42103a6d48b1131e94ba04d9737d61acdaa1322008af9602b3b14862c07a1789aac162102d8b661b0b3302ee2f162b09e07a55ad5dfbe673a9f01d9f0c19617681024306b56ae') txin.redeem_script = bfh('0020a16b5755f7f6f96dbd65f5f0d6ab9418b89af4b1f14a1bb8a09062c35f0dcb54') txin._trusted_value_sats = 987654321 #Output of Transaction diff --git a/electrum/transaction.py b/electrum/transaction.py index 28821195745c..bd65802404e0 100644 --- a/electrum/transaction.py +++ b/electrum/transaction.py @@ -431,7 +431,23 @@ def is_instance(cls, item): or (isinstance(item, type) and issubclass(item, cls)) +class OPGeneric: + def __init__(self, matcher: Callable=None): + if matcher is not None: + self.matcher = matcher + + def match(self, op) -> bool: + return self.matcher(op) + + @classmethod + def is_instance(cls, item): + # accept objects that are instances of this class + # or other classes that are subclasses + return isinstance(item, cls) \ + or (isinstance(item, type) and issubclass(item, cls)) + OPPushDataPubkey = OPPushDataGeneric(lambda x: x in (33, 65)) +OP_ANYSEGWIT_VERSION = OPGeneric(lambda x: x in list(range(opcodes.OP_1, opcodes.OP_16 + 1))) SCRIPTPUBKEY_TEMPLATE_P2PKH = [opcodes.OP_DUP, opcodes.OP_HASH160, OPPushDataGeneric(lambda x: x == 20), @@ -440,6 +456,7 @@ def is_instance(cls, item): SCRIPTPUBKEY_TEMPLATE_WITNESS_V0 = [opcodes.OP_0, OPPushDataGeneric(lambda x: x in (20, 32))] SCRIPTPUBKEY_TEMPLATE_P2WPKH = [opcodes.OP_0, OPPushDataGeneric(lambda x: x == 20)] SCRIPTPUBKEY_TEMPLATE_P2WSH = [opcodes.OP_0, OPPushDataGeneric(lambda x: x == 32)] +SCRIPTPUBKEY_TEMPLATE_ANYSEGWIT = [OP_ANYSEGWIT_VERSION, OPPushDataGeneric(lambda x: x in list(range(2, 40 + 1)))] def match_script_against_template(script, template) -> bool: @@ -459,6 +476,8 @@ def match_script_against_template(script, template) -> bool: script_item = script[i] if OPPushDataGeneric.is_instance(template_item) and template_item.check_data_len(script_item[0]): continue + if OPGeneric.is_instance(template_item) and template_item.match(script_item[0]): + continue if template_item != script_item[0]: return False return True From 947693c90d6579ca52281ea7228a91fd59a81c81 Mon Sep 17 00:00:00 2001 From: bitromortac Date: Fri, 22 Oct 2021 12:58:04 +0200 Subject: [PATCH 2/3] check dust limits * on channel opening we verify that the peer's dust limit is above 354 sat, the limit for unknown segwit versions * we constrain the allowed scriptpubkey types for channel closing * we check that the remote's output is above the relay dust limit for the collaborative close case --- electrum/bitcoin.py | 8 ++++++-- electrum/lnpeer.py | 26 +++++++++++++++----------- electrum/lnutil.py | 7 ++++--- electrum/transaction.py | 15 +++++++++++++++ 4 files changed, 40 insertions(+), 16 deletions(-) diff --git a/electrum/bitcoin.py b/electrum/bitcoin.py index 64682dd4a49d..8a79dbca9684 100644 --- a/electrum/bitcoin.py +++ b/electrum/bitcoin.py @@ -349,8 +349,12 @@ def relayfee(network: 'Network' = None) -> int: # see https://github.com/bitcoin/bitcoin/blob/a62f0ed64f8bbbdfe6467ac5ce92ef5b5222d1bd/src/policy/policy.cpp#L14 -DUST_LIMIT_DEFAULT_SAT_LEGACY = 546 -DUST_LIMIT_DEFAULT_SAT_SEGWIT = 294 +# and https://github.com/lightningnetwork/lightning-rfc/blob/7e3dce42cbe4fa4592320db6a4e06c26bb99122b/03-transactions.md#dust-limits +DUST_LIMIT_P2PKH = 546 +DUST_LIMIT_P2SH = 540 +DUST_LIMIT_UNKNOWN_SEGWIT = 354 +DUST_LIMIT_P2WSH = 330 +DUST_LIMIT_P2WPKH = 294 def dust_threshold(network: 'Network' = None) -> int: diff --git a/electrum/lnpeer.py b/electrum/lnpeer.py index 8d8309937f24..c42fc7a1ebc6 100644 --- a/electrum/lnpeer.py +++ b/electrum/lnpeer.py @@ -534,7 +534,7 @@ def make_local_config(self, funding_sat: int, push_msat: int, initiator: HTLCOwn static_remotekey = bfh(wallet.get_public_key(addr)) else: static_remotekey = None - dust_limit_sat = bitcoin.DUST_LIMIT_DEFAULT_SAT_LEGACY + dust_limit_sat = bitcoin.DUST_LIMIT_P2PKH reserve_sat = max(funding_sat // 100, dust_limit_sat) # for comparison of defaults, see # https://github.com/ACINQ/eclair/blob/afa378fbb73c265da44856b4ad0f2128a88ae6c6/eclair-core/src/main/resources/reference.conf#L66 @@ -1697,12 +1697,7 @@ async def on_shutdown(self, chan: Channel, payload): raise UpfrontShutdownScriptViolation("remote didn't use upfront shutdown script it commited to in channel opening") else: # BOLT-02 restrict the scriptpubkey to some templates: - # order by decreasing dust limit - if match_script_against_template(their_scriptpubkey, transaction.SCRIPTPUBKEY_TEMPLATE_P2PKH): - pass - elif match_script_against_template(their_scriptpubkey, transaction.SCRIPTPUBKEY_TEMPLATE_P2SH): - pass - elif self.is_shutdown_anysegwit() and match_script_against_template(their_scriptpubkey, transaction.SCRIPTPUBKEY_TEMPLATE_ANYSEGWIT): + if self.is_shutdown_anysegwit() and match_script_against_template(their_scriptpubkey, transaction.SCRIPTPUBKEY_TEMPLATE_ANYSEGWIT): pass elif match_script_against_template(their_scriptpubkey, transaction.SCRIPTPUBKEY_TEMPLATE_WITNESS_V0): pass @@ -1765,9 +1760,9 @@ async def _shutdown(self, chan: Channel, payload, *, is_local: bool): # 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_remote = False + drop_to_remote = False def send_closing_signed(): - our_sig, closing_tx = chan.make_closing_tx(our_scriptpubkey, their_scriptpubkey, fee_sat=our_fee, drop_remote=drop_remote) + 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) def verify_signature(tx, sig): their_pubkey = chan.config[REMOTE].multisig_key.pubkey @@ -1788,13 +1783,22 @@ def verify_signature(tx, sig): # 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) if verify_signature(closing_tx, their_sig): - drop_remote = False + drop_to_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_remote = True + drop_to_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 raise Exception('failed to verify their signature') + # at this point we know how the closing tx looks like + # check that their output is above their scriptpubkey's network dust limit + if not drop_to_remote: + to_remote_idx = closing_tx.get_output_idxs_from_scriptpubkey(their_scriptpubkey.hex()).pop() + 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: our_fee = their_fee diff --git a/electrum/lnutil.py b/electrum/lnutil.py index 6971de53d9e5..b6ef77cc0c2a 100644 --- a/electrum/lnutil.py +++ b/electrum/lnutil.py @@ -40,6 +40,7 @@ HTLC_OUTPUT_WEIGHT = 172 LN_MAX_FUNDING_SAT = pow(2, 24) - 1 +DUST_LIMIT_MAX = 1000 # dummy address for fee estimation of funding tx def ln_dummy_address(): @@ -103,10 +104,10 @@ def validate_params(self, *, funding_sat: int) -> None: raise Exception(f"{conf_name}. insane initial_msat={self.initial_msat}. (funding_sat={funding_sat})") if self.reserve_sat < self.dust_limit_sat: raise Exception(f"{conf_name}. MUST set channel_reserve_satoshis greater than or equal to dust_limit_satoshis") - # technically this could be using the lower DUST_LIMIT_DEFAULT_SAT_SEGWIT - # but other implementations are checking against this value too; also let's be conservative - if self.dust_limit_sat < bitcoin.DUST_LIMIT_DEFAULT_SAT_LEGACY: + if self.dust_limit_sat < bitcoin.DUST_LIMIT_UNKNOWN_SEGWIT: raise Exception(f"{conf_name}. dust limit too low: {self.dust_limit_sat} sat") + if self.dust_limit_sat > DUST_LIMIT_MAX: + raise Exception(f"{conf_name}. dust limit too high: {self.dust_limit_sat} sat") if self.reserve_sat > funding_sat // 100: raise Exception(f"{conf_name}. reserve too high: {self.reserve_sat}, funding_sat: {funding_sat}") if self.htlc_minimum_msat > 1_000: diff --git a/electrum/transaction.py b/electrum/transaction.py index bd65802404e0..ba78f80a21b3 100644 --- a/electrum/transaction.py +++ b/electrum/transaction.py @@ -459,6 +459,21 @@ def is_instance(cls, item): SCRIPTPUBKEY_TEMPLATE_ANYSEGWIT = [OP_ANYSEGWIT_VERSION, OPPushDataGeneric(lambda x: x in list(range(2, 40 + 1)))] +def check_scriptpubkey_template_and_dust(scriptpubkey, amount: Optional[int]): + if match_script_against_template(scriptpubkey, SCRIPTPUBKEY_TEMPLATE_P2PKH): + dust_limit = bitcoin.DUST_LIMIT_P2PKH + elif match_script_against_template(scriptpubkey, SCRIPTPUBKEY_TEMPLATE_P2SH): + dust_limit = bitcoin.DUST_LIMIT_P2SH + elif match_script_against_template(scriptpubkey, SCRIPTPUBKEY_TEMPLATE_P2WSH): + dust_limit = bitcoin.DUST_LIMIT_P2WSH + elif match_script_against_template(scriptpubkey, SCRIPTPUBKEY_TEMPLATE_P2WPKH): + dust_limit = bitcoin.DUST_LIMIT_P2WPKH + else: + raise Exception(f'scriptpubkey does not conform to any template: {scriptpubkey.hex()}') + if amount < dust_limit: + raise Exception(f'amount ({amount}) is below dust limit for scriptpubkey type ({dust_limit})') + + def match_script_against_template(script, template) -> bool: """Returns whether 'script' matches 'template'.""" if script is None: From e97f35059781b9bc087f672eb8f721b5712761dc Mon Sep 17 00:00:00 2001 From: bitromortac Date: Fri, 22 Oct 2021 13:02:59 +0200 Subject: [PATCH 3/3] add comment for safer forwarding --- electrum/lnpeer.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/electrum/lnpeer.py b/electrum/lnpeer.py index c42fc7a1ebc6..b91de407157b 100644 --- a/electrum/lnpeer.py +++ b/electrum/lnpeer.py @@ -1346,6 +1346,8 @@ def maybe_forward_htlc( # - for example; atm we forward first and then persist "forwarding_info", # so if we segfault in-between and restart, we might forward an HTLC twice... # (same for trampoline forwarding) + # - we could check for the exposure to dust HTLCs, see: + # https://github.com/ACINQ/eclair/pull/1985 forwarding_enabled = self.network.config.get('lightning_forward_payments', False) if not forwarding_enabled: self.logger.info(f"forwarding is disabled. failing htlc.")