Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transport layer proposal #23

Merged

Conversation

ZmnSCPxj-jr
Copy link
Contributor

No description provided.

@ZmnSCPxj-jr
Copy link
Contributor Author

TODO: What happens if the client disconnects before the LSP can respond to a request?

@ZmnSCPxj-jr
Copy link
Contributor Author

Added text on what happens if connection lost.

@ZmnSCPxj-jr
Copy link
Contributor Author

Added section on implementing LSPS0, also changed how disconnections / reconnections should be handled.

@ZmnSCPxj-jr
Copy link
Contributor Author

@johncantrell97 taught me how feature bits can be added for LDK-based LSP software. Anyone up for writing how to implement LSPS0 for LND?

@SeverinAlexB
Copy link
Collaborator

TODO: What happens if the client disconnects before the LSP can respond to a request?

Client or LSP can just ignore the message in this case IMO.

LSPS0/README.md Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
[BOLT7]: https://github.com/lightning/bolts/blob/f7dcc32694b8cd4f3a1768b904f58cb177168f29/07-routing-gossip.md
[BOLT1]: https://github.com/lightning/bolts/blob/f7dcc32694b8cd4f3a1768b904f58cb177168f29/01-messaging.md

LSPs MUST set the `features` bit numbered 729
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pinged LL on the implementation timeline for setting custom feature bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No response from LL; also current LSPS0 requires init feature bits, but it seems only custom node_announcement feature bits are planned.

Copy link
Collaborator

@SeverinAlexB SeverinAlexB Apr 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carlaKC responded, she is working on it including the init feature bits. So from a compatibility POV, this shouldn't be an issue. Thx Carla 🙏

LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Show resolved Hide resolved
@ZmnSCPxj-jr
Copy link
Contributor Author

@SeverinAlexB wrote:

Client or LSP can just ignore the message in this case IMO.

What I wanted to think on was the id field; if you are doing a naive counter-based id you could potentially reuse it on restart. I have since added a section requiring that clients use high-entropy id fields so that if it cannot remember the previous id it could just ignore it, since a high-entropy id field is unlikely to be reused.

@ZmnSCPxj-jr ZmnSCPxj-jr mentioned this pull request Apr 2, 2023
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
@ZmnSCPxj-jr ZmnSCPxj-jr force-pushed the transport-layer branch 2 times, most recently from f325cd3 to d51c3a4 Compare April 3, 2023 12:03
@ZmnSCPxj-jr
Copy link
Contributor Author

Recent changes: fixed typos reported by @tnull, added short section on implementation for LND.

@ZmnSCPxj-jr ZmnSCPxj-jr force-pushed the transport-layer branch 2 times, most recently from 97c4e56 to 635e34f Compare April 4, 2023 10:15
@SeverinAlexB
Copy link
Collaborator

Adding this for completion of the gzip discussion:

gzip/zip can cause so called zip bombs. A small zipped file can bloat up to terabytes of data. This was the issue LN devs run into previously according to Christian Decker. This is of course a problem. In the unlikely case we need compression in the future, we can prevent such bombs by adding an uncompressed payload_bytes field to the message that stops the decompression as soon as decompressed_data > payload_bytes.

Copy link
Collaborator

@SeverinAlexB SeverinAlexB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a second review. All in all great work @ZmnSCPxj-jr. I got some open questions about error messages but otherwise this is an ACK.

LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
@ZmnSCPxj-jr ZmnSCPxj-jr force-pushed the transport-layer branch 4 times, most recently from 9247a50 to 49544d1 Compare April 5, 2023 12:26
@SeverinAlexB
Copy link
Collaborator

Complete ACK from my side.

@ZmnSCPxj-jr
Copy link
Contributor Author

Added "Status" field, as well as specification of this "Status" field, as per @SeverinAlexB suggestion during the meeting.

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good to me!

Excuse the delay, finally came around to have a closer look. I have only a few comments/questions that would be nice to clarify, but nothing major really.

I also did some proofreading and added a ton of wording suggestions/nits, feel free to regard anything with the "nit:" prefix as non-blocking suggestions, i.e., feel free to ignore if you don't agree.

LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
@ZmnSCPxj-jr
Copy link
Contributor Author

Modified as per @tnull feedback.

@ZmnSCPxj-jr ZmnSCPxj-jr force-pushed the transport-layer branch 3 times, most recently from 1debaf9 to 7254e15 Compare April 9, 2023 22:27
@ZmnSCPxj-jr
Copy link
Contributor Author

Changes: Added rationale for n.lsps namespace, added short text on LSPS specification versioning.

@ZmnSCPxj-jr ZmnSCPxj-jr force-pushed the transport-layer branch 2 times, most recently from 92a00b8 to d575f93 Compare April 9, 2023 22:48
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM.

I think I am ACK on this, provided the larger group comes to alignment on using the JSON-RPC approach.

LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Show resolved Hide resolved
LSPS0/README.md Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Show resolved Hide resolved
LSPS0/README.md Show resolved Hide resolved
LSPS0/README.md Show resolved Hide resolved
@ZmnSCPxj-jr
Copy link
Contributor Author

Changes: Describe how other LSPS specifications should design their APIs so they are resilient against the client crashing and aborting the flow or process, explicitly state that feature bit 729 be set if LSP supports LSPS0 at minimum.

@ZmnSCPxj-jr ZmnSCPxj-jr force-pushed the transport-layer branch 2 times, most recently from 4892bf3 to 4d96f59 Compare April 14, 2023 05:10
@ZmnSCPxj-jr
Copy link
Contributor Author

Changes: remove n. prefix for notifications, give justification for the client not bothering with 37913 messages to the LSP if the LSP gives a bad message until it reconnection since the reconnection might mean the LSP has been bugfixed.

@ZmnSCPxj-jr
Copy link
Contributor Author

Change: remove comment param of listprotocols.

@rbndg rbndg merged commit d1416ab into BitcoinAndLightningLayerSpecs:main Apr 18, 2023
@ZmnSCPxj-jr ZmnSCPxj-jr deleted the transport-layer branch April 18, 2023 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants