From c9536180c50c230af43aa75bd22c7bdd97cc70b0 Mon Sep 17 00:00:00 2001 From: SomberNight Date: Mon, 8 May 2023 19:37:33 +0000 Subject: [PATCH] lnutil.LnFeatures: limit max feature bit to 10_000 closes https://github.com/spesmilo/electrum/issues/8403 > In Python 3.10 that worked fine, however in Python 3.11 large integer check https://github.com/python/cpython/issues/95778, so now this throws an error. Apparently this change was deemed a security fix and was backported to all supported branches of CPython (going back to 3.7). i.e. it affects ~all versions of python (if sufficiently updated with bugfix patches), not just 3.11 > Some offending node aliases: > ``` > ergvein-fiatchannels > test-mainnet > arakis > ``` The features bits set by some of these nodes: ``` (1, 7, 8, 11, 13, 14, 17, 19, 23, 27, 45, 32973, 52973) (1, 7, 8, 11, 13, 14, 17, 19, 23, 27, 39, 45, 55, 32973, 52973) ``` > P.S. I see there are a lot of nodes with 253 bytes in their feature vectors. Any idea why that could happen? Note that the valid [merged-into-spec features](https://github.com/lightning/bolts/blob/50b2df24a27879e8329712c275db78876fd022fe/09-features.md) currently only go as high as ~51. However the spec does not specify how to choose feature bits for experimental stuff, so I guess some people are using values in the 50k range. The only limit imposed by the spec on the length of the features bitvector is an implicit one due to the max message size: every msg must be smaller than 65KB, and the features bitvector needs to fit inside the init message, hence it can be up to ~524K bits. (note that the features are not stored in a sparse representation in the init message and in gossip messages, so if many nodes set such high feature bits, that would noticably impact the size of the gossip). ----- Anyway, our current implementation of LnFeatures is subclassing IntFlag, and it looks like it does not work well for such large integers. I've managed to make IntFlags reasonably in python 3.11 by overriding __str__ and __repr__ (note that in cpython it is apparently only the base2<->base10 conversions that are slow, power-of-2 conversions are fast, so we can e.g. use `hex()`). However in python 3.10 and older, enum.py itself seems really slow for bigints, e.g. enum._decompose in python 3.10. Try e.g. this script, which is instant in py3.11 but takes minutes in py3.10: ```py from enum import IntFlag class c(IntFlag): known_flag_1 = 1 << 0 known_flag_2 = 1 << 1 known_flag_3 = 1 << 2 if hasattr(IntFlag, "_numeric_repr_"): # python 3.11+ _numeric_repr_ = hex def __repr__(self): return f"<{self._name_}: {hex(self._value_)}>" def __str__(self): return hex(self._value_) a = c(2**70000-1) q1 = repr(a) q2 = str(a) ``` AFAICT we have two options: either we rewrite LnFeatures so that it does not use IntFlag (and enum.py), or, for the short term as workaround, we could just reject very large feature bits. For now, I've opted to the latter, rejecting feature bits over 10k. (note that another option is bumping the min required python to 3.11, in which case with the overrides added in this commit the performance looks perfectly fine) --- electrum/channel_db.py | 4 ++-- electrum/lnpeer.py | 14 +++++++------- electrum/lnutil.py | 28 +++++++++++++++++++++++++++- 3 files changed, 36 insertions(+), 10 deletions(-) diff --git a/electrum/channel_db.py b/electrum/channel_db.py index 4331a56e5..e7b6e5f9e 100644 --- a/electrum/channel_db.py +++ b/electrum/channel_db.py @@ -68,7 +68,7 @@ class ChannelInfo(NamedTuple): @staticmethod def from_msg(payload: dict) -> 'ChannelInfo': features = int.from_bytes(payload['features'], 'big') - validate_features(features) + features = validate_features(features) channel_id = payload['short_channel_id'] node_id_1 = payload['node_id_1'] node_id_2 = payload['node_id_2'] @@ -164,7 +164,7 @@ class NodeInfo(NamedTuple): def from_msg(payload) -> Tuple['NodeInfo', Sequence['LNPeerAddr']]: node_id = payload['node_id'] features = int.from_bytes(payload['features'], "big") - validate_features(features) + features = validate_features(features) addresses = NodeInfo.parse_addresses_field(payload['addresses']) peer_addrs = [] for host, port in addresses: diff --git a/electrum/lnpeer.py b/electrum/lnpeer.py index 2dd1092cb..d143476da 100644 --- a/electrum/lnpeer.py +++ b/electrum/lnpeer.py @@ -42,7 +42,7 @@ from .lnutil import (Outpoint, LocalConfig, RECEIVED, UpdateAddHtlc, ChannelConf LightningPeerConnectionClosed, HandshakeFailed, RemoteMisbehaving, ShortChannelID, IncompatibleLightningFeatures, derive_payment_secret_from_payment_preimage, - ChannelType, LNProtocolWarning) + ChannelType, LNProtocolWarning, validate_features, IncompatibleOrInsaneFeatures) from .lnutil import FeeUpdate, channel_id_from_funding_tx from .lntransport import LNTransport, LNTransportBase from .lnmsg import encode_msg, decode_msg, UnknownOptionalMsgType, FailedToParseMsg @@ -352,12 +352,12 @@ class Peer(Logger): if self._received_init: self.logger.info("ALREADY INITIALIZED BUT RECEIVED INIT") return - self.their_features = LnFeatures(int.from_bytes(payload['features'], byteorder="big")) - their_globalfeatures = int.from_bytes(payload['globalfeatures'], byteorder="big") - self.their_features |= their_globalfeatures - # check transitive dependencies for received features - if not self.their_features.validate_transitive_dependencies(): - raise GracefulDisconnect("remote did not set all dependencies for the features they sent") + _their_features = int.from_bytes(payload['features'], byteorder="big") + _their_features |= int.from_bytes(payload['globalfeatures'], byteorder="big") + try: + self.their_features = validate_features(_their_features) + except IncompatibleOrInsaneFeatures as e: + raise GracefulDisconnect(f"remote sent insane features: {repr(e)}") # check if features are compatible, and set self.features to what we negotiated try: self.features = ln_compare_features(self.features, self.their_features) diff --git a/electrum/lnutil.py b/electrum/lnutil.py index e348659af..4ab8e3949 100644 --- a/electrum/lnutil.py +++ b/electrum/lnutil.py @@ -8,6 +8,8 @@ import json from collections import namedtuple, defaultdict from typing import NamedTuple, List, Tuple, Mapping, Optional, TYPE_CHECKING, Union, Dict, Set, Sequence import re +import sys + import attr from aiorpcx import NetAddress @@ -1211,6 +1213,18 @@ class LnFeatures(IntFlag): r.append(feature_name or f"bit_{flag}") return r + if hasattr(IntFlag, "_numeric_repr_"): # python 3.11+ + # performance improvement (avoid base2<->base10), see #8403 + _numeric_repr_ = hex + + def __repr__(self): + # performance improvement (avoid base2<->base10), see #8403 + return f"<{self._name_}: {hex(self._value_)}>" + + def __str__(self): + # performance improvement (avoid base2<->base10), see #8403 + return hex(self._value_) + class ChannelType(IntFlag): OPTION_LEGACY_CHANNEL = 0 @@ -1332,11 +1346,22 @@ def ln_compare_features(our_features: 'LnFeatures', their_features: int) -> 'LnF return our_features -def validate_features(features: int) -> None: +if hasattr(sys, "get_int_max_str_digits"): + # check that the user or other library has not lowered the limit (from default) + assert sys.get_int_max_str_digits() >= 4300, f"sys.get_int_max_str_digits() too low: {sys.get_int_max_str_digits()}" + + +def validate_features(features: int) -> LnFeatures: """Raises IncompatibleOrInsaneFeatures if - a mandatory feature is listed that we don't recognize, or - the features are inconsistent + For convenience, returns the parsed features. """ + if features.bit_length() > 10_000: + # This is an implementation-specific limit for how high feature bits we allow. + # Needed as LnFeatures subclasses IntFlag, and uses ints internally. + # See https://docs.python.org/3/library/stdtypes.html#integer-string-conversion-length-limitation + raise IncompatibleOrInsaneFeatures(f"features bitvector too large: {features.bit_length()=} > 10_000") features = LnFeatures(features) enabled_features = list_enabled_bits(features) for fbit in enabled_features: @@ -1345,6 +1370,7 @@ def validate_features(features: int) -> None: if not features.validate_transitive_dependencies(): raise IncompatibleOrInsaneFeatures(f"not all transitive dependencies are set. " f"features={features}") + return features def derive_payment_secret_from_payment_preimage(payment_preimage: bytes) -> bytes: