Skip to content

Don't round the returned last seen timestamp #18

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

Closed
wants to merge 1 commit into from

Conversation

jdenken
Copy link

@jdenken jdenken commented Oct 4, 2022

Good morning,
This is an attempt to address issue #17.

I realize that this is a classical example of Chesterton’s Fence, but even after reading related server and client code I wasn't able to understand why the rounding to the nearest day was here in the first place, so please help me out :)

On the other hand, the motivation for not rounding the value is to avoid confusion for the rapid gossip sync client that uses this timestamp to derive the time for a batch of updates (e.g. in lightning-rapid-gossip-sync::update_network_graph_from_byte_stream function).
Here is an example of how things could go wrong:
0. Let D_SEC be the rounded timestamp for the current day

  1. On the Gossip Sync Server, channel update U1 arrives and is applied at time D_SEC + 1
  2. Full gossip sync is requested by the client
  3. Channel update U1 is returned with last_seen_timestamp rounded to D_SEC (because of the logic under question)
  4. Update is applied to the client's local state with D_SEC timestamp returned by the server (it actually is backdated to D_SEC - 7 * SECS_IN_DAY, but that doesn't matter for this argument)
  5. Channel update U2 arrived and is applied on the server at time D_SEC + 2
  6. Gossip sync at timestamp D_SEC (or D_SEC + 1) is requested
  7. Channel update U2 is returned with last_seen_timestamp rounded to D_SEC
  8. Client tries to apply U2 with timestamp D_SEC and fails with "Update had same timestamp as last processed update" as previous update had the same rounded timestamp
  9. As a result client ends up with the old channel state U1 and ignores U2 which is incorrect

To resolve this the client either needs to apply U2 even if the timestamp is the same as in the previous update or the update U2 should have timestamp > U1 timestamp, which is achieved with this change.

Additional issues arise when this timestamp is used to derive last_rapid_gossip_sync_timestamp as this leads to duplicate update messages that need to be ignored and causes lightningdevkit/rust-lightning#1746 (but arguably it shouldn't be used in this situation at all and instead we should use the actual current time as seen by the Rapid Gossip Server Postgres instance).

It's not entirely clear why the rounding to a nearest day was here in the first place,
but it could lead to confusion for the client that uses this timestamp
for provenance of time for a batch of updates (e.g. in lightning-rapid-gossip-sync::update_network_graph_from_byte_stream).
For example:
1. Channel update U1 is applied at time D1_SEC + 1
2. Full gossip sync is requested
3. Channel update U1 is returned with `last_seen_timestamp` rounded to D1_SEC
4. Update is applied to client local state with D1_SEC timestamp (it
   actually is backdated to D1_SEC - 7 * SECS_IN_DAY, but that doesn't
   matter)
5. Channel update U2 is applied at time D1_SEC + 2
6. Gossip sync at timestamp D1_SEC (or D1_SEC + 1) is requested
7. Channel update U2 is returned with `last_seen_timestamp` rounded to D1_SEC
8. Client tries to apply U2 with timestamp D1_SEC and fails with "Update had same timestamp as last processed update" as previous update has the same rounded timestamp, which is not expected as this is actually a new update.

Additional issues arise when this timestamp is used to derive
`last_rapid_gossip_sync_timestamp` as this leads to duplicate update
messages that need to be ignored, but arguably it shouldn't be used in
this situation at all and instead we should use the actual current time
as seen by the Rapid Gossip Server Postgres instance.
@TheBlueMatt
Copy link
Contributor

We can't skip the rounding - clients are expected to make requests to the server with the last-seen-timestamp of the last data they fetched. The server needs to only generate a finite number of files for clients to download, so it does so by using round-number indexes only, specifically daily updates. If we don't round the last-seen-timestamp then the server needs to remember every last-seen-timestamp it ever generated, writing files for each possible timestamp any client could have ever seen.

@TheBlueMatt TheBlueMatt closed this Jul 2, 2023
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.

2 participants