Node Reminders #84

arik-so · 2024-07-02T10:00:10Z

Send reminders for node announcements, as we have been doing with channel updates.

Given that we now have similar logic for channels and nodes, there are some preparatory steps we take in this PR prior to its focal commit.

We need to move the prune interval to a config variable, extract the logic for determining reminder inclusion into its own method, and rename some fields whose role will now subtly change.

The reminder logic itself has the following breakdown:

If the pre-sync update was more than 6 days ago, serialize in full.
Otherwise:
- If the last mutation occurred after the last sync, serialize the mutated properties.
- Otherwise:
  - If the last mutation occurred more than 6 days ago, serialize as a reminder.
  - Otherwise, don't serialize at all.

This PR replaces #83.

TheBlueMatt · 2024-07-08T13:14:09Z

src/config.rs

@@ -23,6 +23,9 @@ pub(crate) const MAX_SNAPSHOT_SCOPE: u32 = 3600 * 24 * 21; // three weeks
 /// updates in both directions.
 pub(crate) const CHANNEL_REMINDER_AGE: Duration = Duration::from_secs(6 * 24 * 60 * 60);

+/// The interval after which graph data gets pruned after it was first seen


Something about how this must match LDK's behavior would be nice.

TheBlueMatt · 2024-07-09T01:27:52Z

src/lib.rs

@@ -187,11 +187,11 @@ async fn calculate_delta<L: Deref + Clone>(network_graph: Arc<NetworkGraph<L>>,
 	// for announcement-free incremental-only updates, chain hash can be skipped

 	let mut delta_set = DeltaSet::new();
-	lookup::fetch_channel_announcements(&mut delta_set, network_graph, &client, last_sync_timestamp, snapshot_reference_timestamp, logger.clone()).await;
+	lookup::fetch_channel_announcements(&mut delta_set, network_graph.clone(), &client, last_sync_timestamp, snapshot_reference_timestamp, logger.clone()).await;


nit: prefer Arc::clone(&network_graph) as its clear what's going on, whereas this reads as if we're actually clone'ing the network graph.

TheBlueMatt · 2024-07-09T01:32:22Z

src/lookup.rs

 		ORDER BY public_key ASC, seen DESC
-		", [last_sync_timestamp_float]).await.unwrap();
+		", params).await.unwrap();


Shouldn't the handling of this query's response now never insert into the map, cause we pre-populate it? Further, can't we entirely drop this query now cause we can just fill in the current values from the graph?

Unfortunately, the network graph doesn't give us the seen timestamps of the latest values, so we still need to look those up.

TheBlueMatt · 2024-07-09T01:34:26Z

src/lookup.rs

 					features: details.features().clone(),
 					addresses: details.addresses().into_iter().cloned().collect(),
 				}
 			} else {
+				println!("nope, here we go");


TheBlueMatt · 2024-07-09T01:42:37Z

src/lib.rs

-				if node_delta.has_address_set_changed {
-					node_address_update_count += 1;
-
+				if matches!(strategy, NodeSerializationStrategy::Mutated(MutatedNodeProperties { addresses: true, .. }) | NodeSerializationStrategy::Full) {


if matches! is super weird...this is what match is for (and it avoids forgetting cases in case the enum changes)

TheBlueMatt · 2024-07-09T01:44:33Z

src/lookup.rs

@@ -583,31 +589,50 @@ pub(super) async fn fetch_node_updates<L: Deref>(network_graph: Arc<NetworkGraph
 		let current_node_delta = delta_set.entry(node_id).or_insert(NodeDelta::default());
 		let address_set: HashSet<SocketAddress> = unsigned_node_announcement.addresses.into_iter().collect();

-		// determine mutations
+		if !is_previously_processed_node_id {


Honestly I find this kind of thing totally unreadable - why do we need a variable to store Some(node_id) == previous_node_id? Its just indirection I have to go look up when trying to review.

TheBlueMatt · 2024-07-09T01:48:04Z

src/lookup.rs

-				features: unsigned_node_announcement.features,
-				addresses: address_set,
-			});
+			if current_seen_timestamp >= last_sync_timestamp {


Shouldn't this be checked before setting has_address/feature_set_changed? Otherwise eg a node could update its features set last week, then update its address set every hour and once an hour we'll include its features set.

Don't we only serialize the mutations that have occurred? So if since the last value the client had seen, the features had changed, we would include it, but if they hadn't, we would only serialize the address mutation?

TheBlueMatt · 2024-07-09T01:50:13Z

src/lookup.rs

+			// this is the highest timestamp value, so set the seen timestamp accordingly
+			current_node_delta.latest_details.as_mut().map(|mut d| d.seen.replace(current_seen_timestamp));
+		}
+
 		if let Some(last_seen_update) = current_node_delta.last_details_before_seen.as_ref() {


All this logic is fine but honestly its pretty weird its done repeatedly for each update - we aren't really "deciding" what the serialization path is for a node in it, despite the way it reads. A comment could be helpful but you might also write it for the previous node in the above if block.

I don't think I quite follow what you mean. We need to check for each update pairwise whether it has diverged from the last value the client would have seen. We can only do so if we know what the last value saw a client had seen, so I'm not quite sure what it has to do with whether it's a new node ID iteration or not?

Create a config variable representing the time interval whereafter graph data gets pruned. This value should be used to limit lookup time frames.

If the latest node details have already been seen by a client, we still need to store them for correctly detecting reminder necessity in the future.

TheBlueMatt

Okay, I think this LGTM. I'm still quite annoyed at the level of code duplication (and difference!) across node and channel handling when they're logically doing basically the same thing, but I'm not sure its worth rewriting this again and again.

src/lookup.rs

TheBlueMatt · 2024-08-28T19:53:37Z

Cargo.toml

@@ -4,18 +4,18 @@ version = "0.1.0"
 edition = "2021"

 [dependencies]
-bitcoin = "0.30"
+bitcoin = "0.31.2"


Wanna update this?

TheBlueMatt · 2024-08-28T19:56:59Z

src/lookup.rs

 /// Fetch all the channel announcements that are presently in the network graph, regardless of
 /// whether they had been seen before.
 /// Also include all announcements for which the first update was announced
 /// after `last_sync_timestamp`
-pub(super) async fn fetch_channel_announcements<L: Deref>(delta_set: &mut DeltaSet, network_graph: Arc<NetworkGraph<L>>, client: &Client, last_sync_timestamp: u32, snapshot_reference_timestamp: Option<u64>, logger: L) where L::Target: Logger {
+pub(super) async fn fetch_channel_announcements<L: Deref + Clone>(delta_set: &mut DeltaSet, network_graph: Arc<NetworkGraph<L>>, client: &Client, last_sync_timestamp: u32, snapshot_reference_timestamp: Option<u64>, logger: L) where L::Target: Logger {


You really can pass references, I promise they won't bite.

TheBlueMatt · 2024-09-16T18:35:04Z

Looks like 1.63 CI is failing.

TheBlueMatt · 2024-09-16T18:36:56Z

src/lookup.rs

+			let details: NodeDetails = if let Some(details) = node_info.announcement_info.as_ref() {
+				NodeDetails {
+					seen: 0,
+					features: details.features().clone(),


This commit fails to compile because features is a field, not a method.

TheBlueMatt · 2024-09-16T18:40:05Z

src/tests/mod.rs

@@ -118,7 +118,8 @@ fn generate_update(scid: u64, direction: bool, timestamp: u32, expiry_delta: u16
 			chain_hash: genesis_hash(),
 			short_channel_id: scid,
 			timestamp,
-			flags: 0 | flag_mask,
+			message_flags: 0,
+			channel_flags: 0 | flag_mask,


lol

Suggested change

channel_flags: 0 | flag_mask,

channel_flags: flag_mask,

TheBlueMatt · 2024-09-16T19:43:07Z

Please make sure all individual commits compile on their own and the 1.63 build passes then we can land this!

TheBlueMatt · 2024-09-17T21:27:56Z

Looks like postgres-types still needs to be pinned back one.

We want to ignore any node announcements that have already been pruned. To do so, we extract all the node IDs from the network graph, and use those to filter our queries.

We will need to determine whether or not a snapshot should include reminders for both channel and node update messages. To prepare for that, we extract the decision logic into its own method.

With the addition of reminders, we may encounter scenarios where either a bit flip may suffice, instructing the client to look up its latest data, or we may need to serialize all announcement details a new if the client may have already purged the old data. To better distinguish between these scenarios, we introduce a serialization strategy enum that allows serializing either the full announcement, just the mutations, or serve solely as a reminder and serialize nothing at all.

This covers the following part of our serialization logic: If the pre-sync update was more than 6 days ago, serialize in full.

TheBlueMatt

Thanks!

TheBlueMatt reviewed Jul 9, 2024

View reviewed changes

arik-so force-pushed the node_reminders_v2 branch from 0c3942d to 92d9699 Compare July 10, 2024 14:10

arik-so mentioned this pull request Jul 10, 2024

Node reminders #83

Closed

arik-so marked this pull request as ready for review July 18, 2024 08:55

arik-so added 2 commits August 28, 2024 10:38

Parametrize prune interval.

71b7345

Create a config variable representing the time interval whereafter graph data gets pruned. This value should be used to limit lookup time frames.

Rename latest_details_after_seen to latest_details.

86104e5

If the latest node details have already been seen by a client, we still need to store them for correctly detecting reminder necessity in the future.

arik-so force-pushed the node_reminders_v2 branch from 73a56e3 to c552d06 Compare August 28, 2024 17:43

arik-so requested a review from TheBlueMatt August 28, 2024 17:59

TheBlueMatt reviewed Aug 28, 2024

View reviewed changes

arik-so force-pushed the node_reminders_v2 branch from c552d06 to a585a85 Compare September 12, 2024 05:00

arik-so force-pushed the node_reminders_v2 branch from 07bff67 to aba6deb Compare September 16, 2024 19:25

TheBlueMatt approved these changes Sep 16, 2024

View reviewed changes

arik-so force-pushed the node_reminders_v2 branch from aba6deb to 39f1746 Compare September 16, 2024 21:51

arik-so force-pushed the node_reminders_v2 branch from 39f1746 to dfb86d0 Compare September 18, 2024 17:02

arik-so added 7 commits September 18, 2024 10:07

Upgrade LDK to 0.0.124.

8219e5d

Only consider node announcements from current graph.

63aaf61

We want to ignore any node announcements that have already been pruned. To do so, we extract all the node IDs from the network graph, and use those to filter our queries.

Extract snapshot reminder inclusion check method.

f46b5ec

We will need to determine whether or not a snapshot should include reminders for both channel and node update messages. To prepare for that, we extract the decision logic into its own method.

Send full node announcements following old pre-sync updates.

1d78373

This covers the following part of our serialization logic: If the pre-sync update was more than 6 days ago, serialize in full.

Pin tokio-macros to 1.63.0.

a9674f7

Pin postgres-types to 0.2.6.

d7a9d62

arik-so force-pushed the node_reminders_v2 branch from dfb86d0 to d7a9d62 Compare September 18, 2024 18:40

TheBlueMatt approved these changes Sep 18, 2024

View reviewed changes

TheBlueMatt merged commit dcb1d49 into lightningdevkit:main Sep 18, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node Reminders #84

Node Reminders #84

arik-so commented Jul 2, 2024 •

edited

Loading

TheBlueMatt Jul 8, 2024

TheBlueMatt Jul 9, 2024

TheBlueMatt Jul 9, 2024

arik-so Jul 17, 2024

TheBlueMatt Jul 9, 2024

TheBlueMatt Jul 9, 2024

TheBlueMatt Jul 9, 2024

TheBlueMatt Jul 9, 2024

arik-so Jul 18, 2024

TheBlueMatt Jul 9, 2024

arik-so Jul 18, 2024

TheBlueMatt left a comment

TheBlueMatt Aug 28, 2024

arik-so Aug 28, 2024

TheBlueMatt Aug 28, 2024

TheBlueMatt commented Sep 16, 2024

TheBlueMatt Sep 16, 2024

TheBlueMatt Sep 16, 2024

TheBlueMatt commented Sep 16, 2024

TheBlueMatt commented Sep 17, 2024

TheBlueMatt left a comment

Node Reminders #84

Node Reminders #84

Conversation

arik-so commented Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Sep 16, 2024

TheBlueMatt commented Sep 17, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

arik-so commented Jul 2, 2024 •

edited

Loading