Node reminders #83
Node reminders #83
Conversation
src/lookup.rs
Outdated
| @@ -547,35 +566,55 @@ pub(super) async fn fetch_node_updates<L: Deref>(client: &Client, last_sync_time | |||
| let node_id = unsigned_node_announcement.node_id; | |||
| let is_previously_processed_node_id = Some(node_id) == previous_node_id; | |||
|
|
|||
| let current_node_delta = delta_set.entry(node_id).or_insert(NodeDelta { | |||
| // if our reminder consideration scope exceeds the default lookup scope, set to true by default | |||
There was a problem hiding this comment.
Huh? So if someone regularly syncs once an hour they'll never get a reminder and thus lose node state?
There was a problem hiding this comment.
Nope, not at all. However, I will rewrite this logic and the comments to make the procedure clearer.
| @@ -523,12 +530,24 @@ pub(super) async fn fetch_node_updates<L: Deref>(client: &Client, last_sync_time | |||
| // get all the intermediate node updates | |||
| // (to calculate the set of mutated fields for snapshotting, where intermediate updates may | |||
| // have been omitted) | |||
|
|
|||
| let current_timestamp = snapshot_reference_timestamp.unwrap_or(SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs()); | |||
| let reminder_lookup_threshold_timestamp = current_timestamp.checked_sub(config::CHANNEL_REMINDER_AGE.as_secs() * 3).unwrap() as u32; | |||
There was a problem hiding this comment.
The * 3 here is partially wrong, I think? Its right to use * 3 when SELECT'ing so that we can get old entries that haven't been updated (but instead of using a config field * 3 can we use some other constant?) but then you also use it at current_seen_timestamp >= reminder_lookup_threshold_timestamp which is always true.
There was a problem hiding this comment.
yeah, I think that else can be simplified. I'll rewrite this logic a bit, there's too much going on and it's confusing
src/lookup.rs
Outdated
| addresses: address_set, | ||
| }); | ||
| } | ||
| } else if include_reminders && current_seen_timestamp >= reminder_lookup_threshold_timestamp && current_node_delta.requires_reminder { |
There was a problem hiding this comment.
Can you add a comment explaining what is going on here?
| // either something changed, or this node is new | ||
| delta.has_feature_set_changed || delta.has_address_set_changed || delta.last_details_before_seen.is_none() | ||
| if delta.latest_details_after_seen.is_none() { | ||
| // this entry is vestigial due to the optimized reminder necessity lookup |
There was a problem hiding this comment.
What is "optimized reminder necessity lookup"?
There was a problem hiding this comment.
for channel updates, we have one query for checking mutations, and a separate one for checking if we need to send reminders. With nodes, the queries are simpler because we need to deserialize everything anyway to check modifications, and so we just have one combined query that starts at min(last_sync, reminder_threshold). That simplified query is what I'm referring to here, and its usage resulting in hallucinated mutation entries where there are none, and which therefore lack a latest_detail field, is the effect I'm referring to here.
Would love better phrasing suggestions :)
|
Please squash the fixups down. |
| // this is the timestamp we need to fetch all relevant updates | ||
| let include_reminders = should_snapshot_include_reminders(last_sync_timestamp, current_timestamp, logger.clone()); | ||
| let effective_threshold_timestamp = if include_reminders { | ||
| std::cmp::min(last_sync_timestamp, reminder_lookup_threshold_timestamp) as f64 |
There was a problem hiding this comment.
This is wrong, right? We're looking up all updates that are at least now - 6 days but we need to be looking up all updates that are now - 14 days (the pruning threshold). Even if a node hasn't updated in 7 days we should still send a reminder.
src/lookup.rs
Outdated
| @@ -523,12 +530,24 @@ pub(super) async fn fetch_node_updates<L: Deref>(client: &Client, last_sync_time | |||
| // get all the intermediate node updates | |||
| // (to calculate the set of mutated fields for snapshotting, where intermediate updates may | |||
| // have been omitted) | |||
|
|
|||
| let current_timestamp = snapshot_reference_timestamp.unwrap_or(SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs()); | |||
| let reminder_lookup_threshold_timestamp = current_timestamp.checked_sub(config::CHANNEL_REMINDER_AGE.as_secs()).unwrap() as u32; | |||
There was a problem hiding this comment.
The docs for CHANNEL_REMINDER_AGE say that if an update is at least as old as REMINDER_AGE we'll send a refresher. But your implementation only sends refreshers for announcement updates that are newer than REMINDER_AGE.
arik-so
force-pushed
the
node_reminders
branch
4 times, most recently
from
6c89f8f
to
13c8db5
Compare
As we had already been doing with channel updates, we now also consider node announcements' seen timestamps to determinate whether a given node might necessitate a reminder. We reuse the same threshold that we've already been using for channels.
This is a bugfix for when we don't have any data _after_ last_sync_timestamp, in which case that announcement should not be serialized. However, for the better serialization strategy, we will then fetch the latest update we've seen, regardless of if it came prior or after last_sync_timestamp, so this commit is followed by a rename and that scenario should become inapplicable, it will be squashed immediately following a concept ACK.
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.
|
Closing in favor of #84 |
Start sending reminders for node announcements, as we have been doing with channel updates.
NOTE: This PR is superseded by #84.