refactor: remove subscribe htlcs and complete_htlc #2161

m1sterc001guy · 2023-04-06T22:04:01Z

Currently in the gateway, there are two RPCs needed for completing an HTLC: subscribe_htlcs and complete_htlc. Subscribe returns a stream which the actors use to determine if they need to take an action with the federation. If the gateway is able to get the preimage, it then calls complete_htlc to complete the payment.

However, it is not necessary to have two separate RPCs and this makes the code a bit more complicated. This refactor changes it so there is only one RPC called route_htlcs. This RPC takes a stream as input and returns a stream. This way, the actors can send messages using the input stream rather than needing to call a separate RPC.

This allows us to clean up the code a bit. In the LND implementation, we are able to remove the outcomes map.

Fixes: #1936

fedimint-testing/src/ln/fixtures.rs

gateway/ln-gateway/src/lnd.rs

codecov · 2023-04-07T16:23:25Z

Codecov Report

Patch coverage: 11.56% and project coverage change: -0.07 ⚠️

Comparison is base (7bad9c1) 60.09% compared to head (75b47e3) 60.02%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2161      +/-   ##
==========================================
- Coverage   60.09%   60.02%   -0.07%     
==========================================
  Files         152      152              
  Lines       31054    31263     +209     
==========================================
+ Hits        18662    18766     +104     
- Misses      12392    12497     +105

Impacted Files	Coverage Δ
gateway/ln-gateway/src/bin/cln_extension.rs	`0.22% <0.00%> (-0.03%)`	⬇️
gateway/ln-gateway/src/lib.rs	`46.30% <0.00%> (-0.34%)`	⬇️
gateway/ln-gateway/src/lnd.rs	`0.00% <0.00%> (ø)`
gateway/ln-gateway/src/lnrpc_client.rs	`0.00% <0.00%> (ø)`
gateway/ln-gateway/src/actor.rs	`44.83% <28.87%> (+0.77%)`	⬆️
fedimint-testing/src/ln/fixtures.rs	`93.82% <86.66%> (+20.37%)`	⬆️

... and 18 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

mxz42

started a review yesterday but didn't get quite through.

Submitting the comments now anyway hoping that they're not all out of date after the push

gateway/ln-gateway/src/bin/cln_extension.rs

gateway/ln-gateway/src/actor.rs

fedimint-testing/src/ln/fixtures.rs

gateway/ln-gateway/src/actor.rs

elsirion · 2023-04-09T14:05:59Z

gateway/ln-gateway/proto/gatewaylnrpc.proto

+  // The index of the incoming htlc in the incoming channel.
+  uint64 htlc_id = 2;


Does this abstract well over implementations?

It does not currently, it is specific to LND. Let me see if there's an easy refactor so that it is generalized.

I changed the gRPC spec to just include the incoming channel id and the htlc id in the SubscribeInterceptHtlcsResponse. This is generic across all implementations.

In LND we just pass these values onto LND to complete the HTLC. For our CLN implementation, we use it as the key into our outcomes map to lookup the channel sender needed to unblock the thread that will complete the HTLC.

gateway/ln-gateway/src/actor.rs

m1sterc001guy · 2023-04-10T14:47:22Z

(Adding @okjodom's comments that he sent offline while he didn't have much internet access)

gateway/ln-gateway/proto/gatewaylnrpc.proto

justinmoon

LGTM

justinmoon · 2023-04-11T16:14:54Z

gateway/ln-gateway/src/actor.rs

-                                continue;
+                            Some(route_htlc_response::Action::CompleteResponse(_complete_response)) => {
+                                // TODO: Might need to add some error handling here
+                                info!("Successfully handled HTLC");


Might be good to include some information about the HTLC that was handled

Gonna leave this one as a todo since we don't actually know much about this HTLC (CompleteResponse doesn't contain any data currently).

gateway/ln-gateway/src/actor.rs

mxz42

big change. left some comments and questions but these are better addressed in a new PR as this one is already rather big.

nice work! @m1sterc001guy

(also tested a bit in tmuxinator. seems to work so far 👍)

mxz42 · 2023-04-11T23:31:35Z

gateway/ln-gateway/src/actor.rs

+#[derive(Clone)]
+struct LightningSenderStream {
+    ln_sender: Sender<RouteHtlcRequest>,
+    lnrpc: Arc<RwLock<dyn ILnRpcClient>>,
+}


can we add a doc string here?

for my understanding: the decision to use a RwLock instead of a tokio Mutex is that the tokio::RwLock is write-preferring, right?

I'll add a docstring. My understanding is RwLock is actually read-optimizied since it won't block other readers

mxz42 · 2023-04-11T23:39:30Z

gateway/ln-gateway/src/actor.rs

+        // Create a channel that will be used to shutdown the HTLC thread
+        let (sender, receiver) = mpsc::channel::<Arc<AtomicBool>>(100);


can we use a one shot channel or do we need to be able to receive the shutdown signal more than once?

This is currently somewhat difficult because GatewayActor needs to be clonable. After this refactor though I think it should be possible

mxz42 · 2023-04-11T23:42:22Z

gateway/ln-gateway/src/bin/cln_extension.rs

 type HtlcOutcomeSender = oneshot::Sender<serde_json::Value>;

 /// Functional structure to filter intercepted HTLCs into subscription streams.
 /// Used as a CLN plugin
 #[derive(Clone)]
 pub struct ClnHtlcInterceptor {
    subscriptions: Arc<Mutex<HashMap<u64, HtlcSubscriptionSender>>>,
-    pub outcomes: Arc<Mutex<HashMap<sha256::Hash, HtlcOutcomeSender>>>,
+    pub outcomes: Arc<Mutex<BTreeMap<(u64, u64), HtlcOutcomeSender>>>,


(u64, u64) is what exactly? channel_id and ~~hash~~ "the index of the incoming htlc in the channel"?

Yes. Probably should make a new data structure to encapsulate

mxz42 · 2023-04-11T23:50:14Z

gateway/ln-gateway/src/lib.rs

        for actor in actors.values() {
-            actor.write().await.subscribe_htlcs().await?;
+            let (sender, receiver) = mpsc::channel::<Arc<AtomicBool>>(100);


could this be a oneshot channel, too?

mxz42 · 2023-04-12T00:01:26Z

CI audit:

fedimint-workspace-audit-audit> error: 1 vulnerability found!
error: builder for '/nix/store/p7qkfpa0mxn3nzz4y303g41d69vf895h-fedimint-workspace-audit-audit-0.0.1.drv' failed with exit code 1;
       last 10 log lines:
       > Dependency tree:
       > libsqlite3-sys 0.24.2
       > └── sqlx-core 0.6.2
       >     ├── sqlx-macros 0.6.2
       >     │   └── sqlx 0.6.2
       >     │       └── fedimint-sqlite 0.1.0
       >     │           └── fedimint-tests 0.1.0
       >     └── sqlx 0.6.2
       >
       > error: 1 vulnerability found!

we shouldn't forget about his, either.

justinmoon · 2023-04-12T18:51:28Z

CI audit:

fedimint-workspace-audit-audit> error: 1 vulnerability found!
error: builder for '/nix/store/p7qkfpa0mxn3nzz4y303g41d69vf895h-fedimint-workspace-audit-audit-0.0.1.drv' failed with exit code 1;
       last 10 log lines:
       > Dependency tree:
       > libsqlite3-sys 0.24.2
       > └── sqlx-core 0.6.2
       >     ├── sqlx-macros 0.6.2
       >     │   └── sqlx 0.6.2
       >     │       └── fedimint-sqlite 0.1.0
       >     │           └── fedimint-tests 0.1.0
       >     └── sqlx 0.6.2
       >
       > error: 1 vulnerability found!

we shouldn't forget about his, either.

This got fixed here #2204

justinmoon reviewed Apr 6, 2023

View reviewed changes

fedimint-testing/src/ln/fixtures.rs Outdated Show resolved Hide resolved

justinmoon reviewed Apr 6, 2023

View reviewed changes

gateway/ln-gateway/src/lnd.rs Outdated Show resolved Hide resolved

justinmoon reviewed Apr 6, 2023

View reviewed changes

gateway/ln-gateway/src/lnd.rs Outdated Show resolved Hide resolved

justinmoon reviewed Apr 6, 2023

View reviewed changes

gateway/ln-gateway/src/lnd.rs Outdated Show resolved Hide resolved

m1sterc001guy marked this pull request as draft April 7, 2023 13:07

m1sterc001guy force-pushed the remove_complete branch 2 times, most recently from c8be000 to f296b80 Compare April 7, 2023 16:17

mxz42 reviewed Apr 7, 2023

View reviewed changes

gateway/ln-gateway/src/bin/cln_extension.rs Outdated Show resolved Hide resolved

gateway/ln-gateway/src/actor.rs Outdated Show resolved Hide resolved

fedimint-testing/src/ln/fixtures.rs Outdated Show resolved Hide resolved

gateway/ln-gateway/src/actor.rs Outdated Show resolved Hide resolved

m1sterc001guy marked this pull request as ready for review April 7, 2023 16:34

m1sterc001guy requested a review from a team as a code owner April 7, 2023 16:34

m1sterc001guy force-pushed the remove_complete branch 2 times, most recently from 0c7f409 to d065757 Compare April 7, 2023 17:07

elsirion reviewed Apr 9, 2023

View reviewed changes

m1sterc001guy commented Apr 10, 2023

View reviewed changes

gateway/ln-gateway/proto/gatewaylnrpc.proto Outdated Show resolved Hide resolved

m1sterc001guy commented Apr 10, 2023

View reviewed changes

gateway/ln-gateway/proto/gatewaylnrpc.proto Outdated Show resolved Hide resolved

m1sterc001guy force-pushed the remove_complete branch from d065757 to 53f3c6b Compare April 10, 2023 19:27

m1sterc001guy requested review from a team as code owners April 10, 2023 19:27

m1sterc001guy force-pushed the remove_complete branch from 53f3c6b to 4e9c2ea Compare April 10, 2023 19:32

m1sterc001guy added 2 commits April 10, 2023 14:35

refactor: remove subscribe htlcs and complete_htlc

f23554d

refactor: add LND comments and additional functions

6eab701

m1sterc001guy force-pushed the remove_complete branch 3 times, most recently from 74a8d9e to 8591246 Compare April 10, 2023 20:23

m1sterc001guy added the waiting-on-review PR needs review label Apr 11, 2023

justinmoon previously approved these changes Apr 11, 2023

View reviewed changes

refactor: use incoming_chan_id and htlc_id to uniquely identify htlcs

75b47e3

m1sterc001guy dismissed justinmoon’s stale review via 75b47e3 April 11, 2023 17:25

m1sterc001guy force-pushed the remove_complete branch from 8591246 to 75b47e3 Compare April 11, 2023 17:25

m1sterc001guy mentioned this pull request Apr 11, 2023

Remove per-actor event loop #2192

Closed

justinmoon approved these changes Apr 11, 2023

View reviewed changes

mxz42 approved these changes Apr 11, 2023

View reviewed changes

mxz42 merged commit 60fbb75 into fedimint:master Apr 12, 2023
13 of 15 checks passed

m1sterc001guy mentioned this pull request Apr 12, 2023

Lightning gateway serves multiple federations #664

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remove subscribe htlcs and complete_htlc #2161

refactor: remove subscribe htlcs and complete_htlc #2161

m1sterc001guy commented Apr 6, 2023

codecov bot commented Apr 7, 2023 •

edited

mxz42 left a comment

elsirion Apr 9, 2023

m1sterc001guy Apr 10, 2023

m1sterc001guy Apr 10, 2023

m1sterc001guy commented Apr 10, 2023

justinmoon left a comment

justinmoon Apr 11, 2023

m1sterc001guy Apr 11, 2023

mxz42 left a comment •

edited

mxz42 Apr 11, 2023

m1sterc001guy Apr 12, 2023 •

edited

mxz42 Apr 11, 2023

m1sterc001guy Apr 12, 2023

mxz42 Apr 11, 2023

m1sterc001guy Apr 12, 2023

mxz42 Apr 11, 2023

mxz42 commented Apr 12, 2023

justinmoon commented Apr 12, 2023

		// The index of the incoming htlc in the incoming channel.
		uint64 htlc_id = 2;

		// Create a channel that will be used to shutdown the HTLC thread
		let (sender, receiver) = mpsc::channel::<Arc<AtomicBool>>(100);

refactor: remove subscribe htlcs and complete_htlc #2161

refactor: remove subscribe htlcs and complete_htlc #2161

Conversation

m1sterc001guy commented Apr 6, 2023

codecov bot commented Apr 7, 2023 • edited

Codecov Report

mxz42 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

m1sterc001guy commented Apr 10, 2023

justinmoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxz42 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1sterc001guy Apr 12, 2023 • edited

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

mxz42 commented Apr 12, 2023

justinmoon commented Apr 12, 2023

codecov bot commented Apr 7, 2023 •

edited

mxz42 left a comment •

edited

m1sterc001guy Apr 12, 2023 •

edited