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.

Sign up for GitHub

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

Jump to bottom

Clarification how messages are referred to #941

Closed
wants to merge 9 commits into from
Closed

Clarification how messages are referred to #941

wants to merge 9 commits into from

Conversation

lightning-developer
Copy link
Contributor

I added a clarification to BOLT 1 that explains that messages are usually referred to with their message_name and not as message_name message.

I was confused while reviewing #932 where @pm47 wrote:

  • SHOULD send an error to request the peer to fail the channel.

I was looking where errors in BOLT 2 where defined and realized that error meant error message of type 17 in BOLT1.

I checked in the text and realized that "message" is often dropped.

However there are also 111 occurrences in the text where "message" is appended after message_name as can be found with:

grep "\` message" *md -c
00-introduction.md:0
01-messaging.md:17
02-peer-protocol.md:45
03-transactions.md:3
04-onion-routing.md:1
05-onchain.md:1
07-routing-gossip.md:37
08-transport.md:0
09-features.md:6
10-dns-bootstrap.md:0
11-payment-encoding.md:1
CONTRIBUTING.md:0

Since there are 111 occurrences of the opposite behavior I am not sure if this should be defined / unified.

@lightning-developer
Clarification how messages are referred to
2ae5c63 
I added a clarification to BOLT 1 that explains that messages are usually referred to with their `message_name` and not as `message_name` message.
@rustyrussell
Copy link
Collaborator

OK, so meta things (how to write the spec, vs how to read the spec) don't belong in the spec itself, but in .copy-edit-stylesheet-checklist.md or CONTRIBUTING.md (the latter could use some love!).

But this is more about reading the spec, in which case it's valid, however brevity is wit, so I suggest shortening this significantly?

@lightning-developer
Copy link
Contributor Author

Do you just want me to shorten this here in BOLT1 now or move it to one of the files that you named?

Also should I go through the 111 occurrences where message is being appended to the message_name and see if we can unify this with a bit of clean up? I guess Where message names are being introduced like headings it makes sense to keep it like update_add_htlc message but in other cases the word message could be removed.

@rustyrussell
Copy link
Collaborator

I think just say:

Note: we refer to messages by their message_name, instead of their type (e.g. "The receiving node sends error" instead of "... sends error message" or "... sends message 17").

And yes, a correctional commit following this (or series of commits) would be great!

@rustyrussell rustyrussell added the spelling These changes may be merged without additional sign off from the weekly meeting label Dec 6, 2021
@lightning-developer
Shorter formulation as suggested by @rustyrussell
af7fbb0 
Changed the formulation to the suggestion by @rustyrussel. for more context see: lightning#941 (comment)
@lightning-developer
removed message_name message usage from BOLT1
5d078f3
@lightning-developer
Removed message_name message usage from BOLT 2
cd6e38e 
Kept one instance where it it was written ...`init` message exchange and the situation where it was talked about `update_` messages
@lightning-developer
Removed message_name message usage from BOLT 03
4a3e4cb
@lightning-developer
Removed message_name message usage from BOLT 04
9534c63
@lightning-developer
Removed message_name message from BOLT 07
36c4681 
there were situation where there was written `message_name` messages which I have replaced with `message_name`s.
@lightning-developer
Removed message_name message usage in BOLT 09
e67f058
@lightning-developer
Copy link
Contributor Author

I have just removed the usage of message_name message usage from most bolts. Sometimes I had to fix the sentence a bit. There were a few cases where it made sense to leave the word message explicit behind the message_name for example once there was the talk about "init message exchange" where I felt that message semantically belonged to the word exchange.

I made them in separate commits for each BOLT which should make merging or rebasing simpler if that should become necessary

This should be ready for review now.

@lightning-developer lightning-developer mentioned this pull request Dec 17, 2021
Closed
@lightning-developer lightning-developer mentioned this pull request Jan 17, 2022
Closed
18 tasks
vincenzopalazzo
vincenzopalazzo reviewed Feb 3, 2022
View reviewed changes
01-messaging.md
@@ -325,11 +325,11 @@ The 2-byte `len` field indicates the number of bytes in the immediately followin
The channel is referred to by `channel_id`, unless `channel_id` is 0 (i.e. all bytes are 0), in which case it refers to all channels.

The funding node:
- for all error messages sent before (and including) the `funding_created` message:
Copy link
Contributor

Choose a reason for hiding this comment

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

nack

I don't think this introduces some improvement on spec, and as usual, the free text gives us the power like humans to interpret the free text in a different way. IMO, are two ways to say the same things, and this change can cause another change in the future for other people who preferer the first style.

vincenzopalazzo
vincenzopalazzo reviewed Feb 3, 2022
View reviewed changes
01-messaging.md
- MUST use `temporary_channel_id` in lieu of `channel_id`.

The fundee node:
- for all error messages sent before (and not including) the `funding_signed` message:
Copy link
Contributor

Choose a reason for hiding this comment

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

nack

vincenzopalazzo
vincenzopalazzo reviewed Feb 3, 2022
View reviewed changes
01-messaging.md
@@ -339,7 +339,7 @@ A sending node:
- SHOULD send `error` with the unknown `channel_id` in reply to messages of type `32`-`255` related to unknown channels.
- MAY send an empty `data` field.
- when failure was caused by an invalid signature check:
- SHOULD include the raw, hex-encoded transaction in reply to a `funding_created`, `funding_signed`, `closing_signed`, or `commitment_signed` message.
- SHOULD include the raw, hex-encoded transaction in reply to a `funding_created`, `funding_signed`, `closing_signed`, or `commitment_signed`.
Copy link
Contributor

Choose a reason for hiding this comment

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

nack

vincenzopalazzo
vincenzopalazzo reviewed Feb 3, 2022
View reviewed changes
01-messaging.md
Comment on lines -381 to +385
The `pong` message is to be sent whenever a `ping` message is received. It
`pong` is to be sent whenever a `ping` is received. It
serves as a reply and also serves to keep the connection alive, while
explicitly notifying the other end that the receiver is still active. Within
the received `ping` message, the sender will specify the number of bytes to be
included within the data payload of the `pong` message.
the received `ping`, the sender will specify the number of bytes to be
included within the data payload of the `pong`.
Copy link
Contributor

Choose a reason for hiding this comment

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

nack

vincenzopalazzo
vincenzopalazzo reviewed Feb 3, 2022
View reviewed changes
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

nack

IMO these are two ways to say the same things, I save only the clarification made in the first commit.

@t-bast
Copy link
Collaborator

t-bast commented Sep 18, 2024

This has been inactive and I'm not sure there's actionable follow-up, so I'm closing this. Can be re-worked an re-opened if deemed useful.

@t-bast t-bast closed this Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincenzopalazzo vincenzopalazzo vincenzopalazzo left review comments

Assignees
No one assigned
Labels
spelling These changes may be merged without additional sign off from the weekly meeting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lightning-developer @rustyrussell @t-bast @vincenzopalazzo