-
Notifications
You must be signed in to change notification settings - Fork 906
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
Add option to allow even custom messages to be sent #4963
Add option to allow even custom messages to be sent #4963
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly OK, suggest expanding the err message though.
3f1f0d6
to
8b1616f
Compare
Renamed from |
5bb4d51
to
6ad606a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from a code point of view, but I'm a little bit worried about the dropping connection in some cases.
How you can be sure that there is no problem with your communication?
BTW, I have no enough knowledge to judge this PR from a correctness point of view
Can you write a test to verify that even messages are indeed forwarded to the plugin if enabled and drop the connection otherwise? It'd also be helpful to get some rationale for the change. Are you working on a new sub-protocol or is this to build a plugin for compatibility with an existing sub-protocol? |
Yes
Yeah, I outlined in #4960 that DLCs use even numbered types for messaging and am planning on creating a plugin to send and receive the DLC messages. |
Excellent, just saw the original issue now, that makes sense to me. Sounds like a good experimental change, with no promise that it'll graduate, but the experimental deployment will inform us about the safety. |
6ad606a
to
63081db
Compare
Added a test but in places like here, the message is dropped. I am trying to use the |
63081db
to
b792577
Compare
Hmm, this will get much easier once my PRs drop which move this handling to connectd. That should be in the next week; can this wait? |
Yes, can you link them so I can rebase on top? |
First draft at #4985... |
b792577
to
a5b9fb6
Compare
Rebased on master, don't see how I have access to Is this something someone could help me with? |
Changelog-Experimental: config: new option --experimental-all-onion-messages, which behaves like --experimental-onion-messages but also allow even-numbered messages to be sent
a5b9fb6
to
a3e66c9
Compare
@@ -622,6 +622,7 @@ static bool handle_custommsg(struct daemon *daemon, | |||
const u8 *msg) | |||
{ | |||
enum peer_wire type = fromwire_peektype(msg); | |||
// fixme use allow_even_custom_messages config here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to allow everything through here, but be careful: we should change the semantics of custommsg
hook to allow it to reply "result": "accept" (and terminate traversal of hooks if it does). If nobody does this for an even message, we should tell connectd to send an error and close.
@benthecarman we're looking to get an RC out this week, any chance you'll get a rebase / changes in in the next few days? If not I'll mark it for the next release. |
Hey sorry I won't have time in the next week or two. Last I touched this I wasn't really sure what to do next |
Removing milestone target for now, as this requires some work. Feel free to undraft as soon as you'd like me to re-review and assign to a milestone. |
Closes #4960
Adds an option
--experimental-all-onion-messages
that will behave like--experimental-onion-messages
but also allow even numbered messages to be sent