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

CONTRIBUTING.md: modern feature bit assignment. #1108

Merged

Conversation

rustyrussell
Copy link
Collaborator

  1. Put it in the PR title so everyone can see.
  2. Deploy with +100 while it's still unratified, in case it changes.

1. Put it in the PR title so everyone can see.
2. Deploy with +100 while it's still unratified, in case it changes.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

ACK

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.

ACK 58473d0

@t-bast
Copy link
Collaborator

t-bast commented Sep 12, 2023

Should we extend that recommendation to messages and tlv fields introduced by experimental features?

This isn't strictly necessary: an implementation that wants to support both the experimental version and the official version could encode/decode messages and tlvs differently depending on what feature was negotiated (the experimental feature bit or the official one). But it's a bit cumbersome (at least in eclair we'd rather use separate messages for the experimental version of the feature). Maybe at least worth mentioning that this is a concern that implementers should worry about?

Copy link
Contributor

@ProofOfKeags ProofOfKeags left a comment

Choose a reason for hiding this comment

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

ACK

@rustyrussell rustyrussell merged commit 6649f51 into lightning:master Sep 25, 2023
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.

5 participants