Implements BOLT1 and BOLT9 #208

sr-gi · 2020-09-03T20:02:11Z

This PR takes a first stab to use LN as transport layer for the tower.

It implements BOLTs 1 and 9 for encoding and decoding of network messages and features.

For message decoding, known messages types create a child instance of Message (e.g: InitMessage) whereas unknown messages types are rejected (given they cannot be parsed).

For TLVs, known types create a children of TLVRecord, whereas unknown create an instance of TLVRecord. Unknown even type TLVs do not raise any error since that's a rather node behaviour instead of an encoder / decoder behaviour. Same goes for features.

For BOLT1, only NetworksTLV is currently implemented.

Finally, there is currently no TLVStream parser, that will be part of a followup.

Review materials:

BOLT#1
BOLT#9

bigsize includes tools for encoding, decoding, and parsing BisSize data utils includes functions for message sanity checking lnnet - Adds bigsize docs and some additional sanity checks lnnet - Adds utils file for sanity checks

Includes the general Message class along with classes for init, error, ping and pong messages

Adds features and feature vector

bigspider

Initial review. Many comments are just nits, and some are style suggestions, feel free to disregard them if you disagree.

Did not check the tests yet.

common/net/bigsize.py

common/net/bolt1.py

bigspider

Only some additional minor comments on the tests.

test/common/unit/net/test_bolt1.py

test/common/unit/net/test_bolt9.py

test/common/unit/net/test_tlv.py

Several optional values were defaulted to None, this mean that checking some properties of this values required checking if they were None first (e.g. checking the length of the for a ). This sets the default value to the same type as expected for non-defaults, so the type can be assumed in every case

Adds to_dict to the class so the content of a message can be easily checked in a human readable way

bigspider

Looks good to me.

cdecker

Very nice implementation, I added some suggestions inline, but most of them were already addressed in later commits.

The proposal to switch to an io-based interface is probably too late, but maybe it comes in handy in the future :-)

cdecker · 2020-09-28T09:11:46Z

common/net/bigsize.py

+
+    if prefix < 253:
+        # prefix is actually the value to be parsed
+        return decode(value[0:1]), 1


One problem with this take-what-you-need-and-return-its-length is that it is not well-suited for unbuffered streams, since we can't take 9 bytes, parse them, and then reset the stream back by the remainder of the bytes. I usually use io.RawBaseIO as argument, wrapping str and bytes if necessary, that removes the need for this type of bookkeeping.

cdecker · 2020-09-28T09:13:56Z

common/net/tlv.py

+        v (:obj:`bytes`): the message value.
+    """
+
+    def __init__(self, t=b"", l=b"", v=b""):


Using mypy annotations would remove the need for the isinstance checks below.

cdecker · 2020-09-28T09:16:39Z

common/net/tlv.py

+            if t.to_bytes(t_offset, "big") == tlv_types["networks"]:
+                return NetworksTLV.from_bytes(message)
+            else:
+                l, l_offset = bigsize.parse(message[t_offset:])
+                v = message[t_offset + l_offset :]
+                if l > len(v):
+                    # Value is not long enough
+                    raise ValueError()  # This message get overwritten so it does not matter


Might be helpful to define a dict for t -> TLV class, since you're likely to add a few going forward :-)

cdecker · 2020-09-28T09:19:15Z

common/net/bolt1.py

+        """
+
+        if not isinstance(message, bytes):
+            raise TypeError(f"message be must a bytearray")


bytearray is not bytes it's a separate class.

cdecker · 2020-09-28T09:19:48Z

common/net/bolt1.py

+        if message[:2] == message_types["init"]:
+            return InitMessage.from_bytes(message)
+        elif message[:2] == message_types["error"]:
+            return ErrorMessage.from_bytes(message)
+        elif message[:2] == message_types["ping"]:
+            return PingMessage.from_bytes(message)
+        elif message[:2] == message_types["pong"]:
+            return PongMessage.from_bytes(message)


Dict messages[:2] -> cls will avoid this boilerplate.

cdecker · 2020-09-28T09:26:57Z

common/net/bolt1.py

+        # Add extensions if needed (this follows TLV format)
+        # FIXME: Only networks for now
+        if networks:
+            super().__init__(mtype=message_types["init"], payload=payload, extension=networks.serialize())


Seems you don't distinguish between a single TLV and a TLV stream, that is ok for now, but once we get to streams with multiple TLVs that might put more work on the caller.

cdecker · 2020-09-28T09:28:26Z

common/net/bolt1.py

+            gflen = int.from_bytes(message[2:4], "big")
+            global_features = FeatureVector.from_bytes(message[4 : gflen + 4])
+            flen = int.from_bytes(message[gflen + 4 : gflen + 6], "big")
+            local_features = FeatureVector.from_bytes(message[gflen + 6 : gflen + flen + 6])


This bookkeeping would also be easier if you used an io instance 😉

cdecker · 2020-09-28T09:35:42Z

common/net/bolt9.py

+        is unknown.
+    """
+
+    if name in known_features:


You can turn the three known_features lookups into a single one with this:

f = known_features.get(name, None)

And then check if f is None to see if it was in the dict.

cdecker · 2020-09-28T09:38:56Z

common/net/bolt9.py

+            elif not check_feature_name_bit_pair(key, value.bit):
+                raise ValueError("Feature name and bit do not match")
+
+            vars(self)[key] = value


This is equivalent to self.__dict__[key] = value I guess? Just wondering why vars() is used here.

An alternative would be to have a custom __getattr__ that first looks up into self._features and then calls the superclass __getattr__. That way the self._features dict remains authoritative and there's no chance for the two to be out of sync.

cdecker · 2020-09-28T09:44:42Z

common/net/bolt1.py

-            super().__init__(mtype=message_types["init"], payload=payload, extension=networks.serialize())
+            super().__init__(mtype=message_types["init"], payload=payload, extension=[networks])


Very nice, this addresses my earlier TLV vs. TLVStream comment 👍

sr-gi added 10 commits September 3, 2020 21:47

lnnet - Adds bigsize and utils

73a74d4

bigsize includes tools for encoding, decoding, and parsing BisSize data utils includes functions for message sanity checking lnnet - Adds bigsize docs and some additional sanity checks lnnet - Adds utils file for sanity checks

lnnet - Adds TLVRecord and moves NetworksTLV

abda49d

lnnet - Implements BOLT1

06e680e

Includes the general Message class along with classes for init, error, ping and pong messages

lnnet - Implements BOLT9

613ee82

Adds features and feature vector

test - Adds lnnet bigsize unit tests

8b96771

test - Adds bolt9 unit tests

8ff6f5a

test: Adds utils unit test

4f09f50

test: Adds tlv unit tests

74fb734

lnnet - Improves BOLT1

89717fd

test - Adds BOLT1 unit tests

647962b

sr-gi requested a review from bigspider September 3, 2020 20:02

bigspider suggested changes Sep 4, 2020

View reviewed changes

lnnet - Fixes typos and improves docs

785502a

sr-gi force-pushed the ln-net branch 2 times, most recently from 88401b6 to 06f0001 Compare September 7, 2020 15:07

bigspider suggested changes Sep 8, 2020

View reviewed changes

lnnet - Some code improvements and fixes from @bigspider's review

b9f06f4

sr-gi force-pushed the ln-net branch from 2f8652d to fedfa2e Compare September 8, 2020 17:43

sr-gi force-pushed the ln-net branch from fedfa2e to 5868af8 Compare September 8, 2020 17:45

sr-gi added 3 commits September 8, 2020 20:01

lnnet - Adds to_dict to

2690660

Adds to_dict to the class so the content of a message can be easily checked in a human readable way

test - typos and improvements from PR review

dc4655b

test - Adds Message.to_dict test for lnnet

13b5e13

sr-gi force-pushed the ln-net branch from 5868af8 to 13b5e13 Compare September 8, 2020 18:01

sr-gi requested a review from bigspider September 8, 2020 20:50

bigspider approved these changes Sep 9, 2020

View reviewed changes

cdecker approved these changes Sep 28, 2020

View reviewed changes

sr-gi changed the base branch from split-cli-client to master November 13, 2020 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements BOLT1 and BOLT9 #208

Implements BOLT1 and BOLT9 #208

sr-gi commented Sep 3, 2020 •

edited

bigspider left a comment

bigspider left a comment

bigspider left a comment

cdecker left a comment

cdecker Sep 28, 2020

cdecker Sep 28, 2020

cdecker Sep 28, 2020

cdecker Sep 28, 2020

cdecker Sep 28, 2020

cdecker Sep 28, 2020

cdecker Sep 28, 2020

cdecker Sep 28, 2020

cdecker Sep 28, 2020

cdecker Sep 28, 2020

cdecker Sep 28, 2020

		super().__init__(mtype=message_types["init"], payload=payload, extension=networks.serialize())
		super().__init__(mtype=message_types["init"], payload=payload, extension=[networks])

Implements BOLT1 and BOLT9 #208

Are you sure you want to change the base?

Implements BOLT1 and BOLT9 #208

Conversation

sr-gi commented Sep 3, 2020 • edited

bigspider left a comment

Choose a reason for hiding this comment

bigspider left a comment

Choose a reason for hiding this comment

bigspider left a comment

Choose a reason for hiding this comment

cdecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sr-gi commented Sep 3, 2020 •

edited