Fix encryption of broadcasted gossip messages #1715
Merged
Conversation
This file contains hidden or 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
TheBlueMatt
force-pushed
the
2022-09-fix-msg-send
branch
from
0097784 to
72e53cd
Compare
Codecov Report
@@ Coverage Diff @@
## main #1715 +/- ##
==========================================
- Coverage 90.84% 90.82% -0.03%
==========================================
Files 86 86
Lines 46448 46479 +31
Branches 46448 46479 +31
==========================================
+ Hits 42198 42213 +15
- Misses 4250 4266 +16
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
jkczyz
reviewed
Sep 12, 2022
TheBlueMatt
force-pushed
the
2022-09-fix-msg-send
branch
from
72e53cd to
5976793
Compare
jkczyz
reviewed
Sep 12, 2022
jkczyz
previously approved these changes
Sep 12, 2022
In 47e818f, forwarding broadcasted gossip messages was split into a separate per-peer message buffer. However, both it and the original regular-message queue are encrypted immediately when the messages are enqueued. Because the lightning P2P encryption algorithm is order-dependent, this causes messages to fail their MAC checks as the messages from the two queues may not be sent to peers in the order in which they were encrypted. The fix is to simply queue broadcast gossip messages unencrypted, encrypting them when we add them to the regular outbound buffer.
For non-gossip-broadcast messages, our current flow is to first serialize the message into a `Vec`, and then allocate a new `Vec` into which we write the encrypted+MAC'd message and header. This is somewhat wasteful, and its rather simple to instead allocate only one buffer and encrypt the message in-place.
|
Squashed without further changes. |
TheBlueMatt
force-pushed
the
2022-09-fix-msg-send
branch
from
cd45baf to
45ec1db
Compare
valentinewallace
approved these changes
Sep 12, 2022
jkczyz
approved these changes
Sep 12, 2022
| /// Encrypts the given message, returning the encrypted version. | ||
| /// panics if the length of `message`, once encoded, is greater than 65535 or if the Noise | ||
| /// handshake has not finished. | ||
| pub fn encrypt_message<M: wire::Type>(&mut self, message: &M) -> Vec<u8> { |
There was a problem hiding this comment.
Could we consolidate this with encrypt_buffer if we made gossip_broadcast_buffer contain messages instead of encoded messages?
There was a problem hiding this comment.
We could, but I'm not sure that's better - many messages have multiple heap-allocated structs, and we'd have to clone all of that. The nice thing about storing the encoded message is we just need one heap-allocated Vec per peer, rather than potentially many.
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.
In 47e818f, forwarding broadcasted
gossip messages was split into a separate per-peer message buffer.
However, both it and the original regular-message queue are
encrypted immediately when the messages are enqueued. Because the
lightning P2P encryption algorithm is order-dependent, this causes
messages to fail their MAC checks as the messages from the two
queues may not be sent to peers in the order in which they were
encrypted.
The fix is to simply queue broadcast gossip messages unencrypted,
encrypting them when we add them to the regular outbound buffer.
We also take this opportunity to optimize non-gossip message sending by avoiding allocating two buffers.