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

Update announcement inclusion logic #15

Open
arik-so opened this issue Sep 14, 2022 · 3 comments
Open

Update announcement inclusion logic #15

arik-so opened this issue Sep 14, 2022 · 3 comments
Labels
good first issue Good for newcomers

Comments

@arik-so
Copy link
Contributor

arik-so commented Sep 14, 2022

Currently, the announcement inclusion logic lives here:

https://github.com/lightningdevkit/rapid-gossip-sync-server/blob/main/src/serialization.rs#L134

We need to update it such that if a channel at any point had an interval exceeding two weeks between updates in either direction (since the client's last seen timestamp), an announcement is also included.

@TheBlueMatt
Copy link
Contributor

its kinda awkward, tho, because you have to care even if time A was before the last sync time

so we now need to fetch all updates between (last sync - 2 weeks) and now

because if A was last announced 3 weeks ago before the one yesterday we have to include the announcement for clients that last synced 2 weeks ago
though we do not need to include the announcement for clients that last synced 4 weeks ago

we only need to care, I think, if the "timeout period" spanned the "last sync time"

@arik-so
Copy link
Contributor Author

arik-so commented Sep 14, 2022

Additionally, in this line

https://github.com/lightningdevkit/rapid-gossip-sync-server/blob/main/src/serialization.rs#L129

it's possible that first_update_seen may have happened before last_seen_timestamp, however, the first update seen in the opposite direction happened afterwards. For that case, we'd still need to make sure to also include the channel announcement.

Additionally, by that point, the intermediate update series is no longer available in the delta, so it would need to be calculated in lookup.rs, and a largest_update_interval field would need to be added to the directional update structs.

@arik-so arik-so added the good first issue Good for newcomers label Oct 16, 2023
@arik-so
Copy link
Contributor Author

arik-so commented Oct 16, 2023

This is not super high priority, but would be a great project to tackle once #63 lands, which will allow customizing the announcement seen timestamps in tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants