Part 2: connectd onionmessage improvements #7455
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustyrussell
force-pushed
the
part-2-connectd-onionmessage-improvements
branch
from
99699f1
to
966b15e
Compare
niftynei
approved these changes
Jul 8, 2024
lightningd/onion_message.c
Outdated
| if (sphinx_path_payloads_size(sphinx_path) <= ROUTING_INFO_SIZE) | ||
| onion_size = ROUTING_INFO_SIZE; | ||
| else | ||
| onion_size = 32768; |
There was a problem hiding this comment.
the spec snippet above says "32834" but here it's 32768?
There was a problem hiding this comment.
Good catch! Indeed, 66 bytes is framing overhead. Added comment!
| msec = time_to_msec(timemono_between(now, peer->onionmsg_last_incoming)); | ||
| peer->onionmsg_incoming_tokens += msec; | ||
| if (peer->onionmsg_incoming_tokens > ONION_MSG_TOKENS_MAX) | ||
| peer->onionmsg_incoming_tokens = ONION_MSG_TOKENS_MAX; |
There was a problem hiding this comment.
would it be worth flipping onionmsg_limit_warned false here? (so resetting after frequency has decreased to less than 4/sec)
There was a problem hiding this comment.
Well, I don't expect it to happen at all? If it does, we might start logging more?
rustyrussell
force-pushed
the
part-2-connectd-onionmessage-improvements
branch
from
966b15e
to
a94558c
Compare
|
Added comment about 66 byte difference, and rebased on master. |
rustyrussell
force-pushed
the
part-2-connectd-onionmessage-improvements
branch
from
a94558c
to
9f87e0c
Compare
Allows for caller to log, but more importantly, when we add a command to inject onion messages, allows for us to capture the error. Signed-off-by: Rusty Russell <[email protected]>
Unlike "sendonionmessage" which instructs us to send to a peer, this process it locally (presumably, it contains the next hop). This is useful because it allows us to process an onion message which starts with us (a legal case for a blinded path supplied by someone else!). It also opens the door to bolt12 self-pay. Signed-off-by: Rusty Russell <[email protected]>
This will let us fwd onion messages via scid, even if they're aliases. Signed-off-by: Rusty Russell <[email protected]>
When we set them (i.e. at lockin), when we fire up channeld (for aliases, which we create at channel init, but aren't really useful until we have finished channel opening), and at startup. Signed-off-by: Rusty Russell <[email protected]>
This is now permitted in the offers PR, so we should support it. But we can't just look up in the gossmap, since the "short_channel_id" could be an alias. So we get lightningd to tell us all scid->peer mappings, and look up in that. Changelog-Added: Protocol: onion messages can now be forwarded by short_channel_id. Signed-off-by: Rusty Russell <[email protected]>
However fast we can handle them, it's antisocial to allow others to make us spam the rest of the network. Changelog-Protocol: onion messages: we limit incoming to 4 per second, allowing a little burst. Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: Protocol: onion messages are now supported by default. Changelog-Deprecated: Config: the --experimental-onion-messages option is ignored (on by default).
rustyrussell
force-pushed
the
part-2-connectd-onionmessage-improvements
branch
from
9f87e0c
to
e8d244c
Compare
|
Rebased on master. |
Comment on lines
+79
to
81
| } else | ||
| return false; | ||
|
|
There was a problem hiding this comment.
This looks like useful information to log, no? like "ignoring message blabla .." but this can be a followup PR
vincenzopalazzo
merged commit 029034a
into
ElementsProject:master
34 of 35 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(Based on #7454)- Merged, rebased!This enhanced connectd's handling of onion messages: we ratelimit them, we handle forwarding them via scid (not just node_id) and we turn them on by default now they're in the spec.