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

Implement configurable GossipSource to allow RGS gossip updates #70

Merged
merged 2 commits into from
May 17, 2023

Conversation

tnull
Copy link
Collaborator

@tnull tnull commented Apr 25, 2023

Fixes #2, Based on #68.

Here we implement a GossipSource object that allows us to configure P2P or RGS gossip sources without the need to propagate and leak LDK type parameters upwards. To this end, GossipSource wraps the corresponding variants and implements a RoutingMessageHandler that is delegating or ignoring the incoming messages.

@tnull
Copy link
Collaborator Author

tnull commented Apr 25, 2023

Draft for now, as I have yet to figure out the approach to persisting the latest retrieved timestamp and the chosen gossip configuration. The approach taken should nicely integrate with the greater picture of config persistence (see #4).

@tnull tnull marked this pull request as draft April 25, 2023 13:31
@tnull tnull added this to the 0.1 milestone Apr 25, 2023
@tnull tnull mentioned this pull request Apr 25, 2023
47 tasks
@tnull tnull force-pushed the 2023-04-add-rgs-support branch 4 times, most recently from 0fbf95a to 6c93102 Compare April 26, 2023 17:24
@tnull
Copy link
Collaborator Author

tnull commented Apr 26, 2023

Rebased on main.

@tnull tnull force-pushed the 2023-04-add-rgs-support branch from d046e12 to 596c5e1 Compare May 9, 2023 08:10
@tnull
Copy link
Collaborator Author

tnull commented May 9, 2023

Rebased on main after #25 has been merged. Still have to expose RGS config in bindings though.

@jkczyz
Copy link
Contributor

jkczyz commented May 9, 2023

Fixes #2, Based on #68.

Here we implement a GossipSource object that allows us to configure P2P or RGS gossip sources without the need to propagate and leak LDK type parameters upwards. To this end, GossipSource wraps the corresponding variants and implements a RoutingMessageHandler that is delegating or ignoring the incoming messages.

Would dyn RoutingMessageHandler work? Then you wouldn't need the boilerplate of implementing RoutingMessageHandler for GossipSource.

@tnull tnull force-pushed the 2023-04-add-rgs-support branch from 596c5e1 to 64cdc07 Compare May 12, 2023 13:52
@tnull
Copy link
Collaborator Author

tnull commented May 12, 2023

Would dyn RoutingMessageHandler work? Then you wouldn't need the boilerplate of implementing RoutingMessageHandler for GossipSource.

Ah, good point, thank you. I think I tried this before, but at another point. Seems to work now fine and allows us to remove the clutter.

@tnull tnull force-pushed the 2023-04-add-rgs-support branch from ac32977 to 244fd42 Compare May 13, 2023 07:30
@tnull
Copy link
Collaborator Author

tnull commented May 13, 2023

Cleaned up the code a bit and now added persistence of RGS last_sync_timestamp. Undrafting.

@tnull tnull marked this pull request as ready for review May 13, 2023 07:31
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Feel free to sqush.

@tnull tnull force-pushed the 2023-04-add-rgs-support branch from 244fd42 to 2416808 Compare May 17, 2023 07:26
@tnull
Copy link
Collaborator Author

tnull commented May 17, 2023

Now squashed fixups and included the following changes:

iff --git a/src/gossip.rs b/src/gossip.rs
index b94a346..a3c5353 100644
--- a/src/gossip.rs
+++ b/src/gossip.rs
@@ -1,3 +1,3 @@
-use crate::logger::{log_error, log_info, FilesystemLogger, Logger};
+use crate::logger::{log_trace, FilesystemLogger, Logger};
 use crate::types::{GossipSync, NetworkGraph, P2PGossipSync, RapidGossipSync};
 use crate::Error;
@@ -62,11 +62,15 @@ impl GossipSource {
                                let query_timestamp = latest_sync_timestamp.load(Ordering::Acquire);
                                let query_url = format!("{}/{}", server_url, query_timestamp);
-                               let response =
-                                       reqwest::get(query_url).await.map_err(|_| Error::GossipUpdateFailed)?;
+                               let response = reqwest::get(query_url).await.map_err(|e| {
+                                       log_trace!(logger, "Failed to retrieve RGS gossip update: {}", e);
+                                       Error::GossipUpdateFailed
+                               })?;

                                match response.error_for_status() {
                                        Ok(res) => {
-                                               let update_data =
-                                                       res.bytes().await.map_err(|_| Error::GossipUpdateFailed)?;
+                                               let update_data = res.bytes().await.map_err(|e| {
+                                                       log_trace!(logger, "Failed to retrieve RGS gossip update: {}", e);
+                                                       Error::GossipUpdateFailed
+                                               })?;

                                                let new_latest_sync_timestamp = gossip_sync
@@ -74,9 +78,8 @@ impl GossipSource {
                                                        .map_err(|_| Error::GossipUpdateFailed)?;
                                                latest_sync_timestamp.store(new_latest_sync_timestamp, Ordering::Release);
-                                               log_info!(logger, "Successfully retrieved latest gossip update.");
                                                Ok(new_latest_sync_timestamp)
                                        }
                                        Err(e) => {
-                                               log_error!(logger, "Failed to retrieve RGS gossip update: {}", e);
+                                               log_trace!(logger, "Failed to retrieve RGS gossip update: {}", e);
                                                Err(Error::GossipUpdateFailed)
                                        }

Here we implement a `GossipSource` object that allows us to configure
P2P or RGS gossip sources without the need to propagate and leak LDK
type parameters upwards. To this end, `GossipSource` wraps the
corresponding variants and implements a `RoutingMessageHandler` that is
delegating or ignoring the incoming messages.
@tnull tnull force-pushed the 2023-04-add-rgs-support branch from 2416808 to 56262be Compare May 17, 2023 07:29
@tnull tnull force-pushed the 2023-04-add-rgs-support branch from 56262be to f3aeeee Compare May 17, 2023 09:22
@tnull
Copy link
Collaborator Author

tnull commented May 17, 2023

Ah, fixed and squashed another tiny nit:

diff --git a/src/io/utils.rs b/src/io/utils.rs
index fefeda1..95ad56b 100644
--- a/src/io/utils.rs
+++ b/src/io/utils.rs
@@ -211,5 +211,5 @@ where
                log_error!(
                        logger,
-                       "Writing peer data to key {}/{} failed due to: {}",
+                       "Writing data to key {}/{} failed due to: {}",
                        RGS_LATEST_SYNC_TIMESTAMP_NAMESPACE,
                        RGS_LATEST_SYNC_TIMESTAMP_KEY,
@@ -221,5 +221,5 @@ where
                log_error!(
                        logger,
-                       "Committing peer data to key {}/{} failed due to: {}",
+                       "Committing data to key {}/{} failed due to: {}",
                        RGS_LATEST_SYNC_TIMESTAMP_NAMESPACE,
                        RGS_LATEST_SYNC_TIMESTAMP_KEY,

@tnull tnull merged commit e104c21 into lightningdevkit:main May 17, 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.

Support RapidGossipSync
2 participants