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

Backfill gossip without buffering directly in LDK #1660

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

This is intended as a pre-req for #1604 with a few of the changes from #1604 pulled in. The commit description of the "main" commit follows, with a few other minor tweaks present as well.

Instead of backfilling gossip by buffering (up to) ten messages at
a time, only buffer one message at a time, as the peers' outbound
socket buffer drains. This moves the outbound backfill messages out
of PeerHandler and into the operating system buffer, where it
arguably belongs.

Not buffering causes us to walk the gossip B-Trees somewhat more
often, but avoids allocating vecs for the responses. While its
probably (without having benchmarked it) a net performance loss, it
simplifies buffer tracking and leaves us with more room to play
with the buffer sizing constants as we add onion message forwarding
which is an important win.

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

Looks good, basically :)

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved
lightning/src/ln/peer_handler.rs Outdated Show resolved Hide resolved
lightning/src/ln/peer_handler.rs Outdated Show resolved Hide resolved
@valentinewallace
Copy link
Contributor

valentinewallace commented Aug 10, 2022

Looks good! I'm happy with this after we get a second reviewer. Feel free to squash

valentinewallace and others added 2 commits August 10, 2022 19:29
This consolidates our various checks on peer buffer space into the
`Peer` impl itself, making the thresholds at which we stop taking
various actions on a peer more readable as a whole.

This commit was primarily authored by `Valentine Wallace
<vwallace@protonmail.com>` with some amendments by `Matt Corallo
<git@bluematt.me>`.
@TheBlueMatt
Copy link
Collaborator Author

Squashed without diff.

lightning/src/ln/peer_handler.rs Outdated Show resolved Hide resolved
lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved
lightning/src/routing/gossip.rs Outdated Show resolved Hide resolved
lightning/src/routing/gossip.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

ACK after squash

jkczyz
jkczyz previously approved these changes Aug 15, 2022
Instead of backfilling gossip by buffering (up to) ten messages at
a time, only buffer one message at a time, as the peers' outbound
socket buffer drains. This moves the outbound backfill messages out
of `PeerHandler` and into the operating system buffer, where it
arguably belongs.

Not buffering causes us to walk the gossip B-Trees somewhat more
often, but avoids allocating vecs for the responses. While its
probably (without having benchmarked it) a net performance loss, it
simplifies buffer tracking and leaves us with more room to play
with the buffer sizing constants as we add onion message forwarding
which is an important win.

Note that because we change how often we check if we're out of
messages to send before pinging, we slightly change how many
messages are exchanged at once, impacting the
`test_do_attempt_write_data` constants.
@TheBlueMatt
Copy link
Collaborator Author

Squashed without further changes.

@valentinewallace
Copy link
Contributor

Ah, I think a warning may have been introduced in stable actually?

warning: variable does not need to be mutable
   --> lightning/src/routing/gossip.rs:345:7
    |
345 |         let mut iter = if let Some(pubkey) = starting_point {
    |             ----^^^^
    |             |
    |             help: remove this `mut`
    |
    = note: `#[warn(unused_mut)]` on by default

jkczyz
jkczyz previously approved these changes Aug 15, 2022
@jkczyz
Copy link
Contributor

jkczyz commented Aug 15, 2022

Ah, I think a warning may have been introduced in stable actually?

warning: variable does not need to be mutable
   --> lightning/src/routing/gossip.rs:345:7
    |
345 |         let mut iter = if let Some(pubkey) = starting_point {
    |             ----^^^^
    |             |
    |             help: remove this `mut`
    |
    = note: `#[warn(unused_mut)]` on by default

Yeah, that's from addressing one of my comments. TIL you can avoid the explicit next call with this syntax:

diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs
index cb49ab81..e91d251f 100644
--- a/lightning/src/routing/gossip.rs
+++ b/lightning/src/routing/gossip.rs
@@ -38,6 +38,7 @@ use io_extras::{copy, sink};
 use prelude::*;
 use alloc::collections::{BTreeMap, btree_map::Entry as BtreeEntry};
 use core::{cmp, fmt};
+use core::ops::Bound;
 use sync::{RwLock, RwLockReadGuard};
 use core::sync::atomic::{AtomicUsize, Ordering};
 use sync::Mutex;
@@ -342,14 +343,9 @@ where C::Target: chain::Access, L::Target: Logger
 
        fn get_next_node_announcement(&self, starting_point: Option<&PublicKey>) -> Option<NodeAnnouncement> {
                let nodes = self.network_graph.nodes.read().unwrap();
-               let mut iter = if let Some(pubkey) = starting_point {
-                               let mut iter = nodes.range(NodeId::from_pubkey(pubkey)..);
-                               iter.next();
-                               iter
-                       } else {
-                               nodes.range::<NodeId, _>(..)
-                       };
-               for (_, ref node) in iter {
+               let starting_bound = starting_point.map_or(
+                       Bound::Unbounded, |pubkey| Bound::Excluded(NodeId::from_pubkey(pubkey)));
+               for (_, ref node) in nodes.range((starting_bound, Bound::Unbounded)) {
                        if let Some(node_info) = node.announcement_info.as_ref() {
                                if let Some(msg) = node_info.announcement_message.clone() {
                                        return Some(msg);

jkczyz
jkczyz previously approved these changes Aug 16, 2022
@TheBlueMatt
Copy link
Collaborator Author

Squashed without further changes.

@TheBlueMatt TheBlueMatt merged commit 12687d7 into lightningdevkit:main Aug 16, 2022
valentinewallace added a commit to valentinewallace/rust-lightning that referenced this pull request Aug 25, 2022
It's more accurate to name it as dropping gossip broadcasts, as it won't drop all gossip.
Also fix accidental flipped bool introduced in lightningdevkit#1660
valentinewallace added a commit to valentinewallace/rust-lightning that referenced this pull request Aug 25, 2022
Fixes a flipped bool that was introduced in lightningdevkit#1660
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.

None yet

4 participants