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

Custom Feature Bits #2131

Closed
TheBlueMatt opened this issue Mar 26, 2023 · 11 comments · Fixed by #2204
Closed

Custom Feature Bits #2131

TheBlueMatt opened this issue Mar 26, 2023 · 11 comments · Fixed by #2204
Assignees
Milestone

Comments

@TheBlueMatt
Copy link
Collaborator

Now that we set feature bits based on a message handler call rather than a hard-coded thing, we need to support setting custom feature bits explicitly.

@TheBlueMatt TheBlueMatt added this to the 0.0.116 milestone Mar 26, 2023
@jkczyz jkczyz self-assigned this Mar 27, 2023
@TheBlueMatt
Copy link
Collaborator Author

Also when we do this we need to allow deserializing a BOLT11 invoice with unknown required bits, as long as the user tells us they support it.

@jkczyz
Copy link
Contributor

jkczyz commented Apr 13, 2023

Looks like we'll need to expose Features::or as it would be needed in lightning-custom-message crate. Thoughts on exposing it via implementing core::ops::BitOr?

@TheBlueMatt
Copy link
Collaborator Author

As you know I despise compiler magic, but I admit here it would read nice. I don't have a strong opinion, do what you think makes sense.

@jkczyz
Copy link
Contributor

jkczyz commented Apr 13, 2023

Hmm... so it occurs to me that in PeerManager::handle_message we check InitFeatures::requires_unknown_bits and error if true. Seems we'll instead need to check against those the features required by the message handlers rather than the ones LDK is aware of. Does that sound right?

Related: currently we can only set an unknown feature via Features::from_le_bytes. I assume we don't want to expose our sealed traits. Maybe a method for setting arbitrary feature bits is sufficient?

@TheBlueMatt
Copy link
Collaborator Author

TheBlueMatt commented Apr 14, 2023

Right, we'll have to call all the handlers and get all their features for that check. And, yes, we'll want a custom features setter, with docs clearly stating you should only set it to values in the experimental range (and probably fail if it's actually a known bit).

@jkczyz
Copy link
Contributor

jkczyz commented Apr 19, 2023

So there is an experimental range for message types, but not for features, AFAICT.

Also, I suppose it would be possible to set an unknown feature bit in one context even though it may be known in another, unless we check across all context. Not sure how particular we want to get about this, though.

@TheBlueMatt
Copy link
Collaborator Author

@TheBlueMatt
Copy link
Collaborator Author

I think we just need to pass feature bits in all the places we check them - mostly invoice deserialization and init/peer connection?

@jkczyz
Copy link
Contributor

jkczyz commented Apr 19, 2023

I think we just need to pass feature bits in all the places we check them - mostly invoice deserialization and init/peer connection?

I meant when setting the bit.

@carlaKC
Copy link
Contributor

carlaKC commented Apr 25, 2023

Ran into this lightning/blips#24 working on custom features for LND, might be useful here if you're allowing folks to set custom invoice feature bits.

@TheBlueMatt
Copy link
Collaborator Author

Yep, the above PR should do that for the low-level API, though we may want to do a followup which adds the same for the higher level invoice construction utilities.

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 a pull request may close this issue.

3 participants