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

Expose onion messages in public API #1650

Conversation

valentinewallace
Copy link
Contributor

@valentinewallace valentinewallace commented Aug 4, 2022

This completes the final anti-DoS steps needed for us to be able to make the onion messages API public and integrate the OnionMessenger with the PeerManager. See commit messages for more info.

TODO: add tests for the last commit after it's conceptually ACK'd

Blocked on #1604 and #1648

@valentinewallace valentinewallace force-pushed the 2022-08-take-onionmsgs-public branch 2 times, most recently from 4e8e973 to 3209ac5 Compare August 9, 2022 19:23
@valentinewallace valentinewallace force-pushed the 2022-08-take-onionmsgs-public branch from 3209ac5 to 6b01547 Compare August 15, 2022 18:27
@jkczyz jkczyz self-requested a review August 19, 2022 19:47
@valentinewallace valentinewallace force-pushed the 2022-08-take-onionmsgs-public branch from 6b01547 to ee37e44 Compare August 26, 2022 15:16
@valentinewallace valentinewallace marked this pull request as ready for review August 26, 2022 15:17
@valentinewallace
Copy link
Contributor Author

Now based on #1604

@TheBlueMatt
Copy link
Collaborator

Needs rebase.

lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved
lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved
lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved
lightning/src/ln/features.rs Outdated Show resolved Hide resolved
@valentinewallace valentinewallace force-pushed the 2022-08-take-onionmsgs-public branch from ee37e44 to 664f139 Compare August 31, 2022 17:40
@codecov-commenter
Copy link

codecov-commenter commented Aug 31, 2022

Codecov Report

Merging #1650 (447ba29) into main (0624cf9) will increase coverage by 0.13%.
The diff coverage is 90.58%.

❗ Current head 447ba29 differs from pull request most recent head a8ea0bd. Consider uploading reports for the commit a8ea0bd to get more accurate results

@@            Coverage Diff             @@
##             main    #1650      +/-   ##
==========================================
+ Coverage   90.85%   90.98%   +0.13%     
==========================================
  Files          85       85              
  Lines       45948    46681     +733     
  Branches    45948    46681     +733     
==========================================
+ Hits        41746    42474     +728     
- Misses       4202     4207       +5     
Impacted Files Coverage Δ
lightning/src/lib.rs 100.00% <ø> (ø)
lightning/src/ln/features.rs 99.46% <ø> (ø)
lightning/src/ln/msgs.rs 86.24% <ø> (+0.49%) ⬆️
lightning/src/onion_message/packet.rs 72.52% <50.00%> (+7.69%) ⬆️
lightning/src/onion_message/messenger.rs 89.55% <87.27%> (+0.01%) ⬆️
lightning/src/ln/peer_handler.rs 57.02% <100.00%> (+0.25%) ⬆️
lightning/src/onion_message/functional_tests.rs 96.33% <100.00%> (+0.45%) ⬆️
lightning/src/routing/gossip.rs 91.43% <0.00%> (-0.24%) ⬇️
lightning/src/ln/functional_tests.rs 96.96% <0.00%> (-0.15%) ⬇️
lightning/src/routing/scoring.rs 96.14% <0.00%> (+0.01%) ⬆️
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@valentinewallace
Copy link
Contributor Author

@TheBlueMatt should we only be connecting peers to OnionMessenger if and when we have an open channel? Or does this new approach make that less important?

@TheBlueMatt
Copy link
Collaborator

We should consider some kind of only-channel-peers thing in the future, I think, but just as a way to give those peers larger buffers and give other peers almost no buffer space. That said, we need to limit the buffer in OnionMessager to some size. Given we're in OM-specific stuff, let's do it as some number of bytes, not a number of messages (ie we have to walk the pending messages map before deciding to insert). Should be super trivial but needs to happen here I think.

Picking a random number for the buffer limit, let's go with 256KiB for now. Default open files limit on my local machine in front of me is 1024, which means we'd max out at 256MiB in buffer size.

@valentinewallace valentinewallace force-pushed the 2022-08-take-onionmsgs-public branch from 366662e to 0ed9416 Compare September 1, 2022 19:28
@TheBlueMatt
Copy link
Collaborator

Looks like the fuzzing seeds need updating.

lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved
lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved
lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved
@valentinewallace valentinewallace force-pushed the 2022-08-take-onionmsgs-public branch 3 times, most recently from 5251618 to 3a4a6eb Compare September 2, 2022 19:24
@valentinewallace valentinewallace force-pushed the 2022-08-take-onionmsgs-public branch from 3a4a6eb to 447ba29 Compare September 2, 2022 19:31
TheBlueMatt
TheBlueMatt previously approved these changes Sep 2, 2022
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

No major concerns.

lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved
lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved
lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved
lightning/src/onion_message/functional_tests.rs Outdated Show resolved Hide resolved
let mut peer_buffered_bytes = 0;
for (pk, peer_buf) in buffer {
for om in peer_buf {
let om_len = om.serialized_length();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we optimize this by implmenting serialized_length on OnionMessage and Packet? Seems a bit wasteful to serialize all pending messages any time an onion message is sent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd put good money on it being free. In the past I've investigated this for a few things and LLVM is pretty good at dealing with the simple cases. As the comment for serialized_length says "Note that LLVM optimizes this away in most cases! Check that it isn't before you override!"

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right. Was just looking at the implementation and noticed that comment. Feel free to disregard!

We also refuse to connect to peers that don't advertise onion message
forwarding support.
TheBlueMatt
TheBlueMatt previously approved these changes Sep 2, 2022
Drop OMs if they push us over the max OnionMessenger outbound buffer size
@TheBlueMatt TheBlueMatt merged commit fc7b14b into lightningdevkit:main Sep 2, 2022
@jkczyz jkczyz mentioned this pull request May 10, 2023
60 tasks
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.

4 participants