feat: Remove active gateway #4427

m1sterc001guy · 2024-02-27T22:39:04Z

Removes the "active gateway" concept from the client. Makes our tests more deterministic since every lightning operation explicitly accepts a gateway as a parameter. If a gateway is not supplied, it is assumed to be an internal payment.

Adds a gateway cache
Higher level apps like the CLI select a gateway from the cache to use when paying or creating an invoice.
Cache can be updated using update_gateway_cache. Higher level apps can call this periodically to stay in sync with gateway registrations.
Adds UpdateGatewayCache CLI command
Changes devimint tests to be backwards compatible.
Adds DB migration to remove active gateway entry

Closes #3844

m1sterc001guy · 2024-03-01T04:32:36Z

Two problems make this a tad difficult:

Devimint tests need to be made backwards compatible.
Removing "active gateway" effectively removes caching of the Lightning Gateways, which regresses the latency tests. Perhaps a better approach is to change "active gateway" into a gateway cache, but still force the cli to pass a gateway id.

elsirion · 2024-03-01T10:02:59Z

2. gateway cache

Great idea!

Devimint tests need to be made backwards compatible

Could you encapsulate the pay command in a function that, depending on the client version, calls set active gateway pay or just pay with a specific gateway as argument?

m1sterc001guy · 2024-03-05T17:31:25Z

modules/fedimint-ln-client/src/lib.rs

-                    .into_iter()
-                    .filter(|g| g.vetted)
-                    .collect::<Vec<_>>();
-                if !vetted.is_empty() {


Another question that arises is how we want to handle the "vetted" field. Previously we were filtering non-vetted gateways when selecting the active gateway. Perhaps we should just leave it up to the integrating client.

I think vetted gateways should be prioritized if provided and online, if not we should try another one. That would mean the "ping the GW before making a payment" logic would move into the selection function so we can skip offline vetted gateways.

This might lead to the "gateway cache" becoming its own service in the background that autonomously fetches available gateways and tries to connect to them to figure out which one to use when asked

I can take this another direction if you like, but I opted to keep the API at the fedimint-client level as simple as possible. Lightning operations always expect a gateway to be passed in and callers can acquire this gateway either using select_gateway or implementing their own logic in combination with list_gateways (they can select vetted gateways this way).

The goal of this PR was to remove non-determinism from our tests and I think implementing a fallback gateway causes some issues (it was the source of #4350 (comment)), since the client might unexpectedly choose a different gateway.

I think higher level apps can implement this fallback or select vetted gateways if they want, but the way it is now allows our tests to be more straightforward

devimint/src/federation.rs

justinmoon · 2024-03-12T19:38:32Z

devimint/src/devfed.rs

+    std::env::set_var("GWID_CLN", gw_cln.gateway_id().await?);
+    std::env::set_var("GWID_LND", gw_lnd.gateway_id().await?);


Was going to suggest prefixing with FM_ but then realized that the only reason we did this in the past was to be able to copy all relevant envirment variables into the mprocs shell IIRC. But devimint fixes this for us now. So it would probably make more sense to remove the FM_ prefixes!

It's good to have project-specfic prefixes in long living env variables, as it makes chances of accidental conflicts lower. So we should keep LM_. @m1sterc001guy @justinmoon

fedimint-load-test-tool/src/common.rs

fedimint-load-test-tool/src/main.rs

fedimint-load-test-tool/src/common.rs

justinmoon

Reviewed pretty thoroughly and it looks good. Only question is if we should delete the active gateway db key from code since we removed from the db in a migration.

elsirion

Apparently I never submitted these comments 🙈

modules/fedimint-ln-client/src/db.rs

elsirion · 2024-03-08T05:51:24Z

modules/fedimint-ln-client/src/db.rs

+    // Lightning Gateway cache records are stored the same way as the server, using 0x45
+    // [`fedimint_ln_common::db::DbKeyPrefix::LightningGateway`].


Why?

Can't we define the variant here too? This seems to break the invariant that every DB key used is documented using this enum (the comment helps, but might be overlooked if e.g. looking at it on docs.rs).

It was mainly to re-use the existing code, since we already had a struct definition that we needed. I understand the concern though, we should probably move db.rs in fedimint-ln-common into the server and only use the db records from within the respective crates.

Fixed here: #4567

elsirion · 2024-03-08T06:18:27Z

modules/fedimint-ln-client/src/lib.rs

+    /// Returns all gateways that are currently in the gateway cache.
+    pub async fn list_gateways(&self) -> Vec<LightningGatewayAnnouncement> {
+        let mut dbtx = self.client_ctx.module_db().begin_transaction_nc().await;
+        dbtx.find_by_prefix(&LightningGatewayKeyPrefix)
+            .await
+            .map(|(_, gw)| gw.unanchor())
+            .collect::<Vec<_>>()
            .await


I'd expect some sort of cache invalidation here, e.g. re-fetch if any gateway timed out or if we don't have any gateways cached.

I wanted to keep the API as simple as possible, with no unexpected network activity. I think higher level apps can make this decision themselves if they see that the cache is empty, they can call update_gateway_cache.

m1sterc001guy force-pushed the remove_active_gateway branch 2 times, most recently from b4aea41 to f5137e1 Compare February 29, 2024 16:50

justinmoon mentioned this pull request Mar 1, 2024

pay_bolt11_invoice can make 2 calls to fetch_registered_gateways when paying invoice #3842

Closed

m1sterc001guy commented Mar 5, 2024

View reviewed changes

m1sterc001guy force-pushed the remove_active_gateway branch 9 times, most recently from 74f6c6f to 900598c Compare March 6, 2024 16:03

m1sterc001guy changed the title ~~[WIP]: Remove active gateway~~ Remove active gateway Mar 6, 2024

m1sterc001guy changed the title ~~Remove active gateway~~ feat: Remove active gateway Mar 6, 2024

m1sterc001guy marked this pull request as ready for review March 6, 2024 16:35

m1sterc001guy requested review from a team as code owners March 6, 2024 16:35

m1sterc001guy added 4 commits March 9, 2024 16:17

chore: remove active gateway

be3738c

feat: gateway cache

575a857

feat: only initialize cache if it is empty and add CLI command

2aad48d

fix: fix backwards compat tests

ea1f525

m1sterc001guy force-pushed the remove_active_gateway branch from 900598c to 99d296a Compare March 9, 2024 22:25

dpc reviewed Mar 10, 2024

View reviewed changes

devimint/src/federation.rs Show resolved Hide resolved

m1sterc001guy force-pushed the remove_active_gateway branch from 99d296a to 69a07d5 Compare March 10, 2024 17:06

justinmoon reviewed Mar 12, 2024

View reviewed changes

fedimint-load-test-tool/src/common.rs Outdated Show resolved Hide resolved

justinmoon reviewed Mar 12, 2024

View reviewed changes

fedimint-load-test-tool/src/main.rs Outdated Show resolved Hide resolved

justinmoon reviewed Mar 12, 2024

View reviewed changes

fedimint-load-test-tool/src/main.rs Outdated Show resolved Hide resolved

justinmoon reviewed Mar 12, 2024

View reviewed changes

fedimint-load-test-tool/src/common.rs Outdated Show resolved Hide resolved

justinmoon previously approved these changes Mar 12, 2024

View reviewed changes

chore: add database migration for removing active gateway

c66c4b1

m1sterc001guy dismissed justinmoon’s stale review via c66c4b1 March 12, 2024 20:04

m1sterc001guy force-pushed the remove_active_gateway branch from 69a07d5 to c66c4b1 Compare March 12, 2024 20:04

justinmoon approved these changes Mar 12, 2024

View reviewed changes

dpc approved these changes Mar 12, 2024

View reviewed changes

justinmoon added this pull request to the merge queue Mar 12, 2024

Merged via the queue into fedimint:master with commit 043f6c3 Mar 12, 2024
20 checks passed

elsirion reviewed Mar 15, 2024

View reviewed changes

m1sterc001guy mentioned this pull request May 13, 2024

Gateway selection should consider VETTED gateways #5250

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Remove active gateway #4427

feat: Remove active gateway #4427

m1sterc001guy commented Feb 27, 2024 •

edited

Loading

m1sterc001guy commented Mar 1, 2024

elsirion commented Mar 1, 2024 •

edited

Loading

m1sterc001guy Mar 5, 2024

elsirion Mar 6, 2024

m1sterc001guy Mar 6, 2024

justinmoon Mar 12, 2024

dpc Mar 12, 2024

justinmoon left a comment •

edited

Loading

elsirion left a comment

elsirion Mar 8, 2024

m1sterc001guy Mar 15, 2024

m1sterc001guy Mar 15, 2024

elsirion Mar 8, 2024

m1sterc001guy Mar 15, 2024

		std::env::set_var("GWID_CLN", gw_cln.gateway_id().await?);
		std::env::set_var("GWID_LND", gw_lnd.gateway_id().await?);

		// Lightning Gateway cache records are stored the same way as the server, using 0x45
		// [`fedimint_ln_common::db::DbKeyPrefix::LightningGateway`].

feat: Remove active gateway #4427

feat: Remove active gateway #4427

Conversation

m1sterc001guy commented Feb 27, 2024 • edited Loading

m1sterc001guy commented Mar 1, 2024

elsirion commented Mar 1, 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

justinmoon left a comment • edited Loading

Choose a reason for hiding this comment

elsirion 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1sterc001guy commented Feb 27, 2024 •

edited

Loading

elsirion commented Mar 1, 2024 •

edited

Loading

justinmoon left a comment •

edited

Loading