Move `LockableScore` requirement away from `Router` trait by jurvis · Pull Request #1694 · lightningdevkit/rust-lightning

jurvis · 2022-09-05T04:50:02Z

Purpose

A scorer is not necessary for users who may be using a different server from their payer node for routing. This PR will allow them to implement a Router without it.

Implementation

Move Score type parameterization from InvoicePayer and Router to
DefaultRouter
Adds a new field, scorer, to DefaultRouter
Move AccountsForInflightHtlcs to DefaultRouter, which we
will use to wrap the new scorer field, so scoring only happens in
DefaultRouter explicitly.
Add scoring related functions to Router trait that we used to call
directly from InvoicePayer.
Instead of parameterizing scorer in find_route, we replace it with
inflight_map so InvoicePayer can pass on information about inflight
HTLCs to the router.

jurvis · 2022-09-05T04:51:06Z

I haven't updated the test suite yet, but I wanted to seek concept ACKs on my approach. I'm not 100% sure we want to parameterize inflight_htlcs_map in find_route, in particular.

TheBlueMatt · 2022-09-06T19:36:32Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

jkczyz · 2022-09-06T22:17:06Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

My thinking had been we would expand the Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler. Someone may have a RemoteRouter implementation that should still be notified and handle accordingly (e.g., by sending an RPC to the remote server to update the remote scorer).

jurvis · 2022-09-07T02:02:33Z

Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler

ah, got it. makes sense!

jurvis · 2022-09-10T19:35:14Z

Used a different approach in 2032f46 -- instead of implementing EventHandler in DefaultRouter, have payer call payment_path_successful, payment_path_failed, probe_success, and probe_failure as newly defined function in the Router trait.

jurvis · 2022-09-10T19:38:04Z

Docs need updating -- I'll look into that in a future commit

codecov-commenter · 2022-09-10T19:51:31Z

Codecov Report

Base: 90.92% // Head: 92.54% // Increases project coverage by +1.62% 🎉

Coverage data is based on head (df9ea09) compared to base (f99301d).
Patch coverage: 74.21% of modified lines in pull request are covered.

❗ Current head df9ea09 differs from pull request most recent head 82b46ed. Consider uploading reports for the commit 82b46ed to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1694 +/- ##
==========================================
+ Coverage 90.92% 92.54% +1.62% 
==========================================
Files 85 86 +1 Lines 46268 59531 +13263 Branches 46268 59531 +13263 ==========================================
+ Hits 42067 55091 +13024 - Misses 4201 4440 +239

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`95.47% <44.82%> (-1.30%)`	⬇️
lightning-invoice/src/payment.rs	`92.34% <80.46%> (+1.47%)`	⬆️
lightning-background-processor/src/lib.rs	`96.89% <100.00%> (+1.69%)`	⬆️
lightning/src/util/events.rs	`36.73% <0.00%> (-3.04%)`	⬇️
lightning-block-sync/src/init.rs	`93.03% <0.00%> (-0.54%)`	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.47% <0.00%> (-0.25%)`	⬇️
lightning/src/chain/onchaintx.rs	`94.71% <0.00%> (-0.23%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`96.36% <0.00%> (-0.16%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.80% <0.00%> (-0.09%)`	⬇️
lightning/src/ln/script.rs	`92.13% <0.00%> (-0.06%)`	⬇️
... and 39 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Generally LGTM. Would like to make progress quickly here as we may end up needing to backport this for the 0.0.111 bindings.

jkczyz · 2022-09-12T17:29:02Z

 // Since the path is reversed, the last element in our iteration is the first
 // hop.
+ let mut locked_scorer = self.scorer.lock();
+ let scorer = AccountForInFlightHtlcs::new(locked_scorer.deref_mut(), inflight_htlc_map.clone());


It's a little strange that we are testing AccountForInFlightHtlcs this way rather through DefaultRouter. Then again, seems doing the latter would require using the actual find_route code, which wouldn't lend to a simple test setup. Fine with keeping this as is, but wanted to raise this in case others have any better ideas.

jkczyz · 2022-09-12T17:46:23Z

+	fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+ self.scorer.payment_path_failed(path, short_channel_id)
+	}
+
+	fn payment_path_successful(&mut self, path: &[&RouteHop]) {
+ self.scorer.payment_path_successful(path)
+	}
+
+	fn probe_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+ self.scorer.probe_failed(path, short_channel_id)
+	}
+
+	fn probe_successful(&mut self, path: &[&RouteHop]) {
+ self.scorer.probe_successful(path)
+	}


These are never actually called since AccountForInFlightHtlcs is only used for channel_penalty_msat, so they can be implemented as unreachable!(). I wonder if we should split Score into two different traits or if we are fine with duplicating part of the interface in Router. I guess either way, any Score implementation used by DefaultRouter would also need to implement this half of the trait. Not sure what would be prefered. @tnull @TheBlueMatt Any thoughts?

Addressed in 7d95196

Hmm, yea, I think this is fine for now, we're probably gonna end up revisinting all of it in #1668 anyway, so...

tnull

Generally LGTM.

jkczyz

LGTM. Will need a squash once other reviewers are ready.

TheBlueMatt

LGTM, basically.

tnull

Generally LGTM

jkczyz · 2022-09-15T21:40:10Z

Feel free to squash.

jurvis · 2022-09-15T21:58:36Z

squashed to 82b46ed without changes

tnull

LGTM, one nit.

tnull · 2022-09-16T07:44:08Z

 //! # use std::ops::Deref;
 //! #
-//! # #[cfg(not(feature = "std"))]
-//! # use core2::io;


Ah, you could then also remove the dependency, since it's not used anymore:
diff --git lightning-invoice/Cargo.toml lightning-invoice/Cargo.toml index cd67bcd3..852f62f4 100644 --- lightning-invoice/Cargo.toml +++ lightning-invoice/Cargo.toml @@ -15,7 +15,7 @@ rustdoc-args = ["--cfg", "docsrs"] [features] default = ["std"] -no-std = ["hashbrown", "lightning/no-std", "core2/alloc"] +no-std = ["hashbrown", "lightning/no-std"] std = ["bitcoin_hashes/std", "num-traits/std", "lightning/std", "bech32/std"] [dependencies] @@ -25,7 +25,6 @@ secp256k1 = { version = "0.24.0", default-features = false, features = ["recover num-traits = { version = "0.2.8", default-features = false } bitcoin_hashes = { version = "0.11", default-features = false } hashbrown = { version = "0.11", optional = true } -core2 = { version = "0.3.0", default-features = false, optional = true } serde = { version = "1.0.118", optional = true } [dev-dependencies]

👍 addressed and squashed into c353c3e

We do this to enable users to create routers that do not need a scorer. This can be useful if they are running a node the delegates pathfinding. * Move `Score` type parameterization from `InvoicePayer` and `Router` to `DefaultRouter` * Adds a new field, `scorer`, to `DefaultRouter` * Move `AccountsForInFlightHtlcs` to `DefaultRouter`, which we will use to wrap the new `scorer` field, so scoring only happens in `DefaultRouter` explicitly. * Add scoring related functions to `Router` trait that we used to call directly from `InvoicePayer`. * Instead of parameterizing `scorer` in `find_route`, we replace it with inflight_map so `InvoicePayer` can pass on information about inflight HTLCs to the router. * Introduced a new tuple struct, InFlightHtlcs, that wraps functionality for querying used liquidity.

jurvis · 2022-09-05T04:50:02Z

Purpose

A scorer is not necessary for users who may be using a different server from their payer node for routing. This PR will allow them to implement a Router without it.

Implementation

Move Score type parameterization from InvoicePayer and Router to
DefaultRouter
Adds a new field, scorer, to DefaultRouter
Move AccountsForInflightHtlcs to DefaultRouter, which we
will use to wrap the new scorer field, so scoring only happens in
DefaultRouter explicitly.
Add scoring related functions to Router trait that we used to call
directly from InvoicePayer.
Instead of parameterizing scorer in find_route, we replace it with
inflight_map so InvoicePayer can pass on information about inflight
HTLCs to the router.

jurvis · 2022-09-05T04:51:06Z

I haven't updated the test suite yet, but I wanted to seek concept ACKs on my approach. I'm not 100% sure we want to parameterize inflight_htlcs_map in find_route, in particular.

TheBlueMatt · 2022-09-06T19:36:32Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

jkczyz · 2022-09-06T22:17:06Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

My thinking had been we would expand the Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler. Someone may have a RemoteRouter implementation that should still be notified and handle accordingly (e.g., by sending an RPC to the remote server to update the remote scorer).

jurvis · 2022-09-07T02:02:33Z

Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler

ah, got it. makes sense!

jurvis · 2022-09-10T19:35:14Z

Used a different approach in 2032f46 -- instead of implementing EventHandler in DefaultRouter, have payer call payment_path_successful, payment_path_failed, probe_success, and probe_failure as newly defined function in the Router trait.

jurvis · 2022-09-10T19:38:04Z

Docs need updating -- I'll look into that in a future commit

codecov-commenter · 2022-09-10T19:51:31Z

Codecov Report

Base: 90.92% // Head: 92.54% // Increases project coverage by +1.62% 🎉

Coverage data is based on head (df9ea09) compared to base (f99301d).
Patch coverage: 74.21% of modified lines in pull request are covered.

❗ Current head df9ea09 differs from pull request most recent head 82b46ed. Consider uploading reports for the commit 82b46ed to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1694 +/- ##
==========================================
+ Coverage 90.92% 92.54% +1.62% 
==========================================
Files 85 86 +1 Lines 46268 59531 +13263 Branches 46268 59531 +13263 ==========================================
+ Hits 42067 55091 +13024 - Misses 4201 4440 +239

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`95.47% <44.82%> (-1.30%)`	⬇️
lightning-invoice/src/payment.rs	`92.34% <80.46%> (+1.47%)`	⬆️
lightning-background-processor/src/lib.rs	`96.89% <100.00%> (+1.69%)`	⬆️
lightning/src/util/events.rs	`36.73% <0.00%> (-3.04%)`	⬇️
lightning-block-sync/src/init.rs	`93.03% <0.00%> (-0.54%)`	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.47% <0.00%> (-0.25%)`	⬇️
lightning/src/chain/onchaintx.rs	`94.71% <0.00%> (-0.23%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`96.36% <0.00%> (-0.16%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.80% <0.00%> (-0.09%)`	⬇️
lightning/src/ln/script.rs	`92.13% <0.00%> (-0.06%)`	⬇️
... and 39 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Generally LGTM. Would like to make progress quickly here as we may end up needing to backport this for the 0.0.111 bindings.

jkczyz · 2022-09-12T17:29:02Z

 // Since the path is reversed, the last element in our iteration is the first
 // hop.
+ let mut locked_scorer = self.scorer.lock();
+ let scorer = AccountForInFlightHtlcs::new(locked_scorer.deref_mut(), inflight_htlc_map.clone());


It's a little strange that we are testing AccountForInFlightHtlcs this way rather through DefaultRouter. Then again, seems doing the latter would require using the actual find_route code, which wouldn't lend to a simple test setup. Fine with keeping this as is, but wanted to raise this in case others have any better ideas.

jkczyz · 2022-09-12T17:46:23Z

+	fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+ self.scorer.payment_path_failed(path, short_channel_id)
+	}
+
+	fn payment_path_successful(&mut self, path: &[&RouteHop]) {
+ self.scorer.payment_path_successful(path)
+	}
+
+	fn probe_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+ self.scorer.probe_failed(path, short_channel_id)
+	}
+
+	fn probe_successful(&mut self, path: &[&RouteHop]) {
+ self.scorer.probe_successful(path)
+	}


These are never actually called since AccountForInFlightHtlcs is only used for channel_penalty_msat, so they can be implemented as unreachable!(). I wonder if we should split Score into two different traits or if we are fine with duplicating part of the interface in Router. I guess either way, any Score implementation used by DefaultRouter would also need to implement this half of the trait. Not sure what would be prefered. @tnull @TheBlueMatt Any thoughts?

Addressed in 7d95196

Hmm, yea, I think this is fine for now, we're probably gonna end up revisinting all of it in #1668 anyway, so...

tnull

Generally LGTM.

jkczyz

LGTM. Will need a squash once other reviewers are ready.

TheBlueMatt

LGTM, basically.

tnull

Generally LGTM

jkczyz · 2022-09-15T21:40:10Z

Feel free to squash.

jurvis · 2022-09-15T21:58:36Z

squashed to 82b46ed without changes

tnull

LGTM, one nit.

tnull · 2022-09-16T07:44:08Z

 //! # use std::ops::Deref;
 //! #
-//! # #[cfg(not(feature = "std"))]
-//! # use core2::io;


Ah, you could then also remove the dependency, since it's not used anymore:
diff --git lightning-invoice/Cargo.toml lightning-invoice/Cargo.toml index cd67bcd3..852f62f4 100644 --- lightning-invoice/Cargo.toml +++ lightning-invoice/Cargo.toml @@ -15,7 +15,7 @@ rustdoc-args = ["--cfg", "docsrs"] [features] default = ["std"] -no-std = ["hashbrown", "lightning/no-std", "core2/alloc"] +no-std = ["hashbrown", "lightning/no-std"] std = ["bitcoin_hashes/std", "num-traits/std", "lightning/std", "bech32/std"] [dependencies] @@ -25,7 +25,6 @@ secp256k1 = { version = "0.24.0", default-features = false, features = ["recover num-traits = { version = "0.2.8", default-features = false } bitcoin_hashes = { version = "0.11", default-features = false } hashbrown = { version = "0.11", optional = true } -core2 = { version = "0.3.0", default-features = false, optional = true } serde = { version = "1.0.118", optional = true } [dev-dependencies]

👍 addressed and squashed into c353c3e

We do this to enable users to create routers that do not need a scorer. This can be useful if they are running a node the delegates pathfinding. * Move `Score` type parameterization from `InvoicePayer` and `Router` to `DefaultRouter` * Adds a new field, `scorer`, to `DefaultRouter` * Move `AccountsForInFlightHtlcs` to `DefaultRouter`, which we will use to wrap the new `scorer` field, so scoring only happens in `DefaultRouter` explicitly. * Add scoring related functions to `Router` trait that we used to call directly from `InvoicePayer`. * Instead of parameterizing `scorer` in `find_route`, we replace it with inflight_map so `InvoicePayer` can pass on information about inflight HTLCs to the router. * Introduced a new tuple struct, InFlightHtlcs, that wraps functionality for querying used liquidity.

jurvis · 2022-09-05T04:50:02Z

Purpose

A scorer is not necessary for users who may be using a different server from their payer node for routing. This PR will allow them to implement a Router without it.

Implementation

Move Score type parameterization from InvoicePayer and Router to
DefaultRouter
Adds a new field, scorer, to DefaultRouter
Move AccountsForInflightHtlcs to DefaultRouter, which we
will use to wrap the new scorer field, so scoring only happens in
DefaultRouter explicitly.
Add scoring related functions to Router trait that we used to call
directly from InvoicePayer.
Instead of parameterizing scorer in find_route, we replace it with
inflight_map so InvoicePayer can pass on information about inflight
HTLCs to the router.

jurvis · 2022-09-05T04:51:06Z

I haven't updated the test suite yet, but I wanted to seek concept ACKs on my approach. I'm not 100% sure we want to parameterize inflight_htlcs_map in find_route, in particular.

TheBlueMatt · 2022-09-06T19:36:32Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

jkczyz · 2022-09-06T22:17:06Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

My thinking had been we would expand the Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler. Someone may have a RemoteRouter implementation that should still be notified and handle accordingly (e.g., by sending an RPC to the remote server to update the remote scorer).

jurvis · 2022-09-07T02:02:33Z

Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler

ah, got it. makes sense!

jurvis · 2022-09-10T19:35:14Z

Used a different approach in 2032f46 -- instead of implementing EventHandler in DefaultRouter, have payer call payment_path_successful, payment_path_failed, probe_success, and probe_failure as newly defined function in the Router trait.

jurvis · 2022-09-10T19:38:04Z

Docs need updating -- I'll look into that in a future commit

codecov-commenter · 2022-09-10T19:51:31Z

Codecov Report

Base: 90.92% // Head: 92.54% // Increases project coverage by +1.62% 🎉

Coverage data is based on head (df9ea09) compared to base (f99301d).
Patch coverage: 74.21% of modified lines in pull request are covered.

❗ Current head df9ea09 differs from pull request most recent head 82b46ed. Consider uploading reports for the commit 82b46ed to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1694 +/- ##
==========================================
+ Coverage 90.92% 92.54% +1.62% 
==========================================
Files 85 86 +1 Lines 46268 59531 +13263 Branches 46268 59531 +13263 ==========================================
+ Hits 42067 55091 +13024 - Misses 4201 4440 +239

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`95.47% <44.82%> (-1.30%)`	⬇️
lightning-invoice/src/payment.rs	`92.34% <80.46%> (+1.47%)`	⬆️
lightning-background-processor/src/lib.rs	`96.89% <100.00%> (+1.69%)`	⬆️
lightning/src/util/events.rs	`36.73% <0.00%> (-3.04%)`	⬇️
lightning-block-sync/src/init.rs	`93.03% <0.00%> (-0.54%)`	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.47% <0.00%> (-0.25%)`	⬇️
lightning/src/chain/onchaintx.rs	`94.71% <0.00%> (-0.23%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`96.36% <0.00%> (-0.16%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.80% <0.00%> (-0.09%)`	⬇️
lightning/src/ln/script.rs	`92.13% <0.00%> (-0.06%)`	⬇️
... and 39 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Generally LGTM. Would like to make progress quickly here as we may end up needing to backport this for the 0.0.111 bindings.

jkczyz · 2022-09-12T17:29:02Z

 // Since the path is reversed, the last element in our iteration is the first
 // hop.
+ let mut locked_scorer = self.scorer.lock();
+ let scorer = AccountForInFlightHtlcs::new(locked_scorer.deref_mut(), inflight_htlc_map.clone());


It's a little strange that we are testing AccountForInFlightHtlcs this way rather through DefaultRouter. Then again, seems doing the latter would require using the actual find_route code, which wouldn't lend to a simple test setup. Fine with keeping this as is, but wanted to raise this in case others have any better ideas.

jkczyz · 2022-09-12T17:46:23Z

+	fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+ self.scorer.payment_path_failed(path, short_channel_id)
+	}
+
+	fn payment_path_successful(&mut self, path: &[&RouteHop]) {
+ self.scorer.payment_path_successful(path)
+	}
+
+	fn probe_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+ self.scorer.probe_failed(path, short_channel_id)
+	}
+
+	fn probe_successful(&mut self, path: &[&RouteHop]) {
+ self.scorer.probe_successful(path)
+	}


These are never actually called since AccountForInFlightHtlcs is only used for channel_penalty_msat, so they can be implemented as unreachable!(). I wonder if we should split Score into two different traits or if we are fine with duplicating part of the interface in Router. I guess either way, any Score implementation used by DefaultRouter would also need to implement this half of the trait. Not sure what would be prefered. @tnull @TheBlueMatt Any thoughts?

Addressed in 7d95196

Hmm, yea, I think this is fine for now, we're probably gonna end up revisinting all of it in #1668 anyway, so...

tnull

Generally LGTM.

jkczyz

LGTM. Will need a squash once other reviewers are ready.

TheBlueMatt

LGTM, basically.

tnull

Generally LGTM

jkczyz · 2022-09-15T21:40:10Z

Feel free to squash.

jurvis · 2022-09-15T21:58:36Z

squashed to 82b46ed without changes

tnull

LGTM, one nit.

tnull · 2022-09-16T07:44:08Z

 //! # use std::ops::Deref;
 //! #
-//! # #[cfg(not(feature = "std"))]
-//! # use core2::io;


Ah, you could then also remove the dependency, since it's not used anymore:
diff --git lightning-invoice/Cargo.toml lightning-invoice/Cargo.toml index cd67bcd3..852f62f4 100644 --- lightning-invoice/Cargo.toml +++ lightning-invoice/Cargo.toml @@ -15,7 +15,7 @@ rustdoc-args = ["--cfg", "docsrs"] [features] default = ["std"] -no-std = ["hashbrown", "lightning/no-std", "core2/alloc"] +no-std = ["hashbrown", "lightning/no-std"] std = ["bitcoin_hashes/std", "num-traits/std", "lightning/std", "bech32/std"] [dependencies] @@ -25,7 +25,6 @@ secp256k1 = { version = "0.24.0", default-features = false, features = ["recover num-traits = { version = "0.2.8", default-features = false } bitcoin_hashes = { version = "0.11", default-features = false } hashbrown = { version = "0.11", optional = true } -core2 = { version = "0.3.0", default-features = false, optional = true } serde = { version = "1.0.118", optional = true } [dev-dependencies]

👍 addressed and squashed into c353c3e

We do this to enable users to create routers that do not need a scorer. This can be useful if they are running a node the delegates pathfinding. * Move `Score` type parameterization from `InvoicePayer` and `Router` to `DefaultRouter` * Adds a new field, `scorer`, to `DefaultRouter` * Move `AccountsForInFlightHtlcs` to `DefaultRouter`, which we will use to wrap the new `scorer` field, so scoring only happens in `DefaultRouter` explicitly. * Add scoring related functions to `Router` trait that we used to call directly from `InvoicePayer`. * Instead of parameterizing `scorer` in `find_route`, we replace it with inflight_map so `InvoicePayer` can pass on information about inflight HTLCs to the router. * Introduced a new tuple struct, InFlightHtlcs, that wraps functionality for querying used liquidity.

jurvis · 2022-09-05T04:50:02Z

Purpose

A scorer is not necessary for users who may be using a different server from their payer node for routing. This PR will allow them to implement a Router without it.

Implementation

Move Score type parameterization from InvoicePayer and Router to
DefaultRouter
Adds a new field, scorer, to DefaultRouter
Move AccountsForInflightHtlcs to DefaultRouter, which we
will use to wrap the new scorer field, so scoring only happens in
DefaultRouter explicitly.
Add scoring related functions to Router trait that we used to call
directly from InvoicePayer.
Instead of parameterizing scorer in find_route, we replace it with
inflight_map so InvoicePayer can pass on information about inflight
HTLCs to the router.

jurvis · 2022-09-05T04:51:06Z

I haven't updated the test suite yet, but I wanted to seek concept ACKs on my approach. I'm not 100% sure we want to parameterize inflight_htlcs_map in find_route, in particular.

TheBlueMatt · 2022-09-06T19:36:32Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

jkczyz · 2022-09-06T22:17:06Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

My thinking had been we would expand the Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler. Someone may have a RemoteRouter implementation that should still be notified and handle accordingly (e.g., by sending an RPC to the remote server to update the remote scorer).

jurvis · 2022-09-07T02:02:33Z

Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler

ah, got it. makes sense!

jurvis · 2022-09-10T19:35:14Z

Used a different approach in 2032f46 -- instead of implementing EventHandler in DefaultRouter, have payer call payment_path_successful, payment_path_failed, probe_success, and probe_failure as newly defined function in the Router trait.

jurvis · 2022-09-10T19:38:04Z

Docs need updating -- I'll look into that in a future commit

codecov-commenter · 2022-09-10T19:51:31Z

Codecov Report

Base: 90.92% // Head: 92.54% // Increases project coverage by +1.62% 🎉

Coverage data is based on head (df9ea09) compared to base (f99301d).
Patch coverage: 74.21% of modified lines in pull request are covered.

❗ Current head df9ea09 differs from pull request most recent head 82b46ed. Consider uploading reports for the commit 82b46ed to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1694 +/- ##
==========================================
+ Coverage 90.92% 92.54% +1.62% 
==========================================
Files 85 86 +1 Lines 46268 59531 +13263 Branches 46268 59531 +13263 ==========================================
+ Hits 42067 55091 +13024 - Misses 4201 4440 +239

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`95.47% <44.82%> (-1.30%)`	⬇️
lightning-invoice/src/payment.rs	`92.34% <80.46%> (+1.47%)`	⬆️
lightning-background-processor/src/lib.rs	`96.89% <100.00%> (+1.69%)`	⬆️
lightning/src/util/events.rs	`36.73% <0.00%> (-3.04%)`	⬇️
lightning-block-sync/src/init.rs	`93.03% <0.00%> (-0.54%)`	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.47% <0.00%> (-0.25%)`	⬇️
lightning/src/chain/onchaintx.rs	`94.71% <0.00%> (-0.23%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`96.36% <0.00%> (-0.16%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.80% <0.00%> (-0.09%)`	⬇️
lightning/src/ln/script.rs	`92.13% <0.00%> (-0.06%)`	⬇️
... and 39 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Generally LGTM. Would like to make progress quickly here as we may end up needing to backport this for the 0.0.111 bindings.

jkczyz · 2022-09-12T17:29:02Z

 // Since the path is reversed, the last element in our iteration is the first
 // hop.
+ let mut locked_scorer = self.scorer.lock();
+ let scorer = AccountForInFlightHtlcs::new(locked_scorer.deref_mut(), inflight_htlc_map.clone());


It's a little strange that we are testing AccountForInFlightHtlcs this way rather through DefaultRouter. Then again, seems doing the latter would require using the actual find_route code, which wouldn't lend to a simple test setup. Fine with keeping this as is, but wanted to raise this in case others have any better ideas.

jkczyz · 2022-09-12T17:46:23Z

+	fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+ self.scorer.payment_path_failed(path, short_channel_id)
+	}
+
+	fn payment_path_successful(&mut self, path: &[&RouteHop]) {
+ self.scorer.payment_path_successful(path)
+	}
+
+	fn probe_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+ self.scorer.probe_failed(path, short_channel_id)
+	}
+
+	fn probe_successful(&mut self, path: &[&RouteHop]) {
+ self.scorer.probe_successful(path)
+	}


These are never actually called since AccountForInFlightHtlcs is only used for channel_penalty_msat, so they can be implemented as unreachable!(). I wonder if we should split Score into two different traits or if we are fine with duplicating part of the interface in Router. I guess either way, any Score implementation used by DefaultRouter would also need to implement this half of the trait. Not sure what would be prefered. @tnull @TheBlueMatt Any thoughts?

Addressed in 7d95196

Hmm, yea, I think this is fine for now, we're probably gonna end up revisinting all of it in #1668 anyway, so...

tnull

Generally LGTM.

jkczyz

LGTM. Will need a squash once other reviewers are ready.

TheBlueMatt

LGTM, basically.

tnull

Generally LGTM

jkczyz · 2022-09-15T21:40:10Z

Feel free to squash.

jurvis · 2022-09-15T21:58:36Z

squashed to 82b46ed without changes

tnull

LGTM, one nit.

tnull · 2022-09-16T07:44:08Z

 //! # use std::ops::Deref;
 //! #
-//! # #[cfg(not(feature = "std"))]
-//! # use core2::io;


Ah, you could then also remove the dependency, since it's not used anymore:
diff --git lightning-invoice/Cargo.toml lightning-invoice/Cargo.toml index cd67bcd3..852f62f4 100644 --- lightning-invoice/Cargo.toml +++ lightning-invoice/Cargo.toml @@ -15,7 +15,7 @@ rustdoc-args = ["--cfg", "docsrs"] [features] default = ["std"] -no-std = ["hashbrown", "lightning/no-std", "core2/alloc"] +no-std = ["hashbrown", "lightning/no-std"] std = ["bitcoin_hashes/std", "num-traits/std", "lightning/std", "bech32/std"] [dependencies] @@ -25,7 +25,6 @@ secp256k1 = { version = "0.24.0", default-features = false, features = ["recover num-traits = { version = "0.2.8", default-features = false } bitcoin_hashes = { version = "0.11", default-features = false } hashbrown = { version = "0.11", optional = true } -core2 = { version = "0.3.0", default-features = false, optional = true } serde = { version = "1.0.118", optional = true } [dev-dependencies]

👍 addressed and squashed into c353c3e

We do this to enable users to create routers that do not need a scorer. This can be useful if they are running a node the delegates pathfinding. * Move `Score` type parameterization from `InvoicePayer` and `Router` to `DefaultRouter` * Adds a new field, `scorer`, to `DefaultRouter` * Move `AccountsForInFlightHtlcs` to `DefaultRouter`, which we will use to wrap the new `scorer` field, so scoring only happens in `DefaultRouter` explicitly. * Add scoring related functions to `Router` trait that we used to call directly from `InvoicePayer`. * Instead of parameterizing `scorer` in `find_route`, we replace it with inflight_map so `InvoicePayer` can pass on information about inflight HTLCs to the router. * Introduced a new tuple struct, InFlightHtlcs, that wraps functionality for querying used liquidity.

jurvis · 2022-09-05T04:50:02Z

Purpose

A scorer is not necessary for users who may be using a different server from their payer node for routing. This PR will allow them to implement a Router without it.

Implementation

Move Score type parameterization from InvoicePayer and Router to
DefaultRouter
Adds a new field, scorer, to DefaultRouter
Move AccountsForInflightHtlcs to DefaultRouter, which we
will use to wrap the new scorer field, so scoring only happens in
DefaultRouter explicitly.
Add scoring related functions to Router trait that we used to call
directly from InvoicePayer.
Instead of parameterizing scorer in find_route, we replace it with
inflight_map so InvoicePayer can pass on information about inflight
HTLCs to the router.

jurvis · 2022-09-05T04:51:06Z

I haven't updated the test suite yet, but I wanted to seek concept ACKs on my approach. I'm not 100% sure we want to parameterize inflight_htlcs_map in find_route, in particular.

TheBlueMatt · 2022-09-06T19:36:32Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

jkczyz · 2022-09-06T22:17:06Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

My thinking had been we would expand the Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler. Someone may have a RemoteRouter implementation that should still be notified and handle accordingly (e.g., by sending an RPC to the remote server to update the remote scorer).

jurvis · 2022-09-07T02:02:33Z

Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler

ah, got it. makes sense!

jurvis · 2022-09-10T19:35:14Z

Used a different approach in 2032f46 -- instead of implementing EventHandler in DefaultRouter, have payer call payment_path_successful, payment_path_failed, probe_success, and probe_failure as newly defined function in the Router trait.

jurvis · 2022-09-10T19:38:04Z

Docs need updating -- I'll look into that in a future commit

codecov-commenter · 2022-09-10T19:51:31Z

Codecov Report

Base: 90.92% // Head: 92.54% // Increases project coverage by +1.62% 🎉

Coverage data is based on head (df9ea09) compared to base (f99301d).
Patch coverage: 74.21% of modified lines in pull request are covered.

❗ Current head df9ea09 differs from pull request most recent head 82b46ed. Consider uploading reports for the commit 82b46ed to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1694 +/- ##
==========================================
+ Coverage 90.92% 92.54% +1.62% 
==========================================
Files 85 86 +1 Lines 46268 59531 +13263 Branches 46268 59531 +13263 ==========================================
+ Hits 42067 55091 +13024 - Misses 4201 4440 +239

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`95.47% <44.82%> (-1.30%)`	⬇️
lightning-invoice/src/payment.rs	`92.34% <80.46%> (+1.47%)`	⬆️
lightning-background-processor/src/lib.rs	`96.89% <100.00%> (+1.69%)`	⬆️
lightning/src/util/events.rs	`36.73% <0.00%> (-3.04%)`	⬇️
lightning-block-sync/src/init.rs	`93.03% <0.00%> (-0.54%)`	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.47% <0.00%> (-0.25%)`	⬇️
lightning/src/chain/onchaintx.rs	`94.71% <0.00%> (-0.23%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`96.36% <0.00%> (-0.16%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.80% <0.00%> (-0.09%)`	⬇️
lightning/src/ln/script.rs	`92.13% <0.00%> (-0.06%)`	⬇️
... and 39 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Generally LGTM. Would like to make progress quickly here as we may end up needing to backport this for the 0.0.111 bindings.

jkczyz · 2022-09-12T17:29:02Z

 // Since the path is reversed, the last element in our iteration is the first
 // hop.
+ let mut locked_scorer = self.scorer.lock();
+ let scorer = AccountForInFlightHtlcs::new(locked_scorer.deref_mut(), inflight_htlc_map.clone());


It's a little strange that we are testing AccountForInFlightHtlcs this way rather through DefaultRouter. Then again, seems doing the latter would require using the actual find_route code, which wouldn't lend to a simple test setup. Fine with keeping this as is, but wanted to raise this in case others have any better ideas.

jkczyz · 2022-09-12T17:46:23Z

+	fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+ self.scorer.payment_path_failed(path, short_channel_id)
+	}
+
+	fn payment_path_successful(&mut self, path: &[&RouteHop]) {
+ self.scorer.payment_path_successful(path)
+	}
+
+	fn probe_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+ self.scorer.probe_failed(path, short_channel_id)
+	}
+
+	fn probe_successful(&mut self, path: &[&RouteHop]) {
+ self.scorer.probe_successful(path)
+	}


These are never actually called since AccountForInFlightHtlcs is only used for channel_penalty_msat, so they can be implemented as unreachable!(). I wonder if we should split Score into two different traits or if we are fine with duplicating part of the interface in Router. I guess either way, any Score implementation used by DefaultRouter would also need to implement this half of the trait. Not sure what would be prefered. @tnull @TheBlueMatt Any thoughts?

Addressed in 7d95196

Hmm, yea, I think this is fine for now, we're probably gonna end up revisinting all of it in #1668 anyway, so...

tnull

Generally LGTM.

jkczyz

LGTM. Will need a squash once other reviewers are ready.

TheBlueMatt

LGTM, basically.

tnull

Generally LGTM

jkczyz · 2022-09-15T21:40:10Z

Feel free to squash.

jurvis · 2022-09-15T21:58:36Z

squashed to 82b46ed without changes

tnull

LGTM, one nit.

tnull · 2022-09-16T07:44:08Z

 //! # use std::ops::Deref;
 //! #
-//! # #[cfg(not(feature = "std"))]
-//! # use core2::io;


Ah, you could then also remove the dependency, since it's not used anymore:
diff --git lightning-invoice/Cargo.toml lightning-invoice/Cargo.toml index cd67bcd3..852f62f4 100644 --- lightning-invoice/Cargo.toml +++ lightning-invoice/Cargo.toml @@ -15,7 +15,7 @@ rustdoc-args = ["--cfg", "docsrs"] [features] default = ["std"] -no-std = ["hashbrown", "lightning/no-std", "core2/alloc"] +no-std = ["hashbrown", "lightning/no-std"] std = ["bitcoin_hashes/std", "num-traits/std", "lightning/std", "bech32/std"] [dependencies] @@ -25,7 +25,6 @@ secp256k1 = { version = "0.24.0", default-features = false, features = ["recover num-traits = { version = "0.2.8", default-features = false } bitcoin_hashes = { version = "0.11", default-features = false } hashbrown = { version = "0.11", optional = true } -core2 = { version = "0.3.0", default-features = false, optional = true } serde = { version = "1.0.118", optional = true } [dev-dependencies]

👍 addressed and squashed into c353c3e

We do this to enable users to create routers that do not need a scorer. This can be useful if they are running a node the delegates pathfinding. * Move `Score` type parameterization from `InvoicePayer` and `Router` to `DefaultRouter` * Adds a new field, `scorer`, to `DefaultRouter` * Move `AccountsForInFlightHtlcs` to `DefaultRouter`, which we will use to wrap the new `scorer` field, so scoring only happens in `DefaultRouter` explicitly. * Add scoring related functions to `Router` trait that we used to call directly from `InvoicePayer`. * Instead of parameterizing `scorer` in `find_route`, we replace it with inflight_map so `InvoicePayer` can pass on information about inflight HTLCs to the router. * Introduced a new tuple struct, InFlightHtlcs, that wraps functionality for querying used liquidity.

jurvis · 2022-09-05T04:50:02Z

Purpose

A scorer is not necessary for users who may be using a different server from their payer node for routing. This PR will allow them to implement a Router without it.

Implementation

Move Score type parameterization from InvoicePayer and Router to
DefaultRouter
Adds a new field, scorer, to DefaultRouter
Move AccountsForInflightHtlcs to DefaultRouter, which we
will use to wrap the new scorer field, so scoring only happens in
DefaultRouter explicitly.
Add scoring related functions to Router trait that we used to call
directly from InvoicePayer.
Instead of parameterizing scorer in find_route, we replace it with
inflight_map so InvoicePayer can pass on information about inflight
HTLCs to the router.

jurvis · 2022-09-05T04:51:06Z

I haven't updated the test suite yet, but I wanted to seek concept ACKs on my approach. I'm not 100% sure we want to parameterize inflight_htlcs_map in find_route, in particular.

TheBlueMatt · 2022-09-06T19:36:32Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

jkczyz · 2022-09-06T22:17:06Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

My thinking had been we would expand the Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler. Someone may have a RemoteRouter implementation that should still be notified and handle accordingly (e.g., by sending an RPC to the remote server to update the remote scorer).

jurvis · 2022-09-07T02:02:33Z

Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler

ah, got it. makes sense!

jurvis · 2022-09-10T19:35:14Z

Used a different approach in 2032f46 -- instead of implementing EventHandler in DefaultRouter, have payer call payment_path_successful, payment_path_failed, probe_success, and probe_failure as newly defined function in the Router trait.

jurvis · 2022-09-10T19:38:04Z

Docs need updating -- I'll look into that in a future commit

codecov-commenter · 2022-09-10T19:51:31Z

Codecov Report

Base: 90.92% // Head: 92.54% // Increases project coverage by +1.62% 🎉

Coverage data is based on head (df9ea09) compared to base (f99301d).
Patch coverage: 74.21% of modified lines in pull request are covered.

❗ Current head df9ea09 differs from pull request most recent head 82b46ed. Consider uploading reports for the commit 82b46ed to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1694 +/- ##
==========================================
+ Coverage 90.92% 92.54% +1.62% 
==========================================
Files 85 86 +1 Lines 46268 59531 +13263 Branches 46268 59531 +13263 ==========================================
+ Hits 42067 55091 +13024 - Misses 4201 4440 +239

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`95.47% <44.82%> (-1.30%)`	⬇️
lightning-invoice/src/payment.rs	`92.34% <80.46%> (+1.47%)`	⬆️
lightning-background-processor/src/lib.rs	`96.89% <100.00%> (+1.69%)`	⬆️
lightning/src/util/events.rs	`36.73% <0.00%> (-3.04%)`	⬇️
lightning-block-sync/src/init.rs	`93.03% <0.00%> (-0.54%)`	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.47% <0.00%> (-0.25%)`	⬇️
lightning/src/chain/onchaintx.rs	`94.71% <0.00%> (-0.23%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`96.36% <0.00%> (-0.16%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.80% <0.00%> (-0.09%)`	⬇️
lightning/src/ln/script.rs	`92.13% <0.00%> (-0.06%)`	⬇️
... and 39 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Generally LGTM. Would like to make progress quickly here as we may end up needing to backport this for the 0.0.111 bindings.

jkczyz · 2022-09-12T17:29:02Z

 // Since the path is reversed, the last element in our iteration is the first
 // hop.
+ let mut locked_scorer = self.scorer.lock();
+ let scorer = AccountForInFlightHtlcs::new(locked_scorer.deref_mut(), inflight_htlc_map.clone());


It's a little strange that we are testing AccountForInFlightHtlcs this way rather through DefaultRouter. Then again, seems doing the latter would require using the actual find_route code, which wouldn't lend to a simple test setup. Fine with keeping this as is, but wanted to raise this in case others have any better ideas.

jkczyz · 2022-09-12T17:46:23Z

+	fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+ self.scorer.payment_path_failed(path, short_channel_id)
+	}
+
+	fn payment_path_successful(&mut self, path: &[&RouteHop]) {
+ self.scorer.payment_path_successful(path)
+	}
+
+	fn probe_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+ self.scorer.probe_failed(path, short_channel_id)
+	}
+
+	fn probe_successful(&mut self, path: &[&RouteHop]) {
+ self.scorer.probe_successful(path)
+	}


These are never actually called since AccountForInFlightHtlcs is only used for channel_penalty_msat, so they can be implemented as unreachable!(). I wonder if we should split Score into two different traits or if we are fine with duplicating part of the interface in Router. I guess either way, any Score implementation used by DefaultRouter would also need to implement this half of the trait. Not sure what would be prefered. @tnull @TheBlueMatt Any thoughts?

Addressed in 7d95196

Hmm, yea, I think this is fine for now, we're probably gonna end up revisinting all of it in #1668 anyway, so...

tnull

Generally LGTM.

jkczyz

LGTM. Will need a squash once other reviewers are ready.

TheBlueMatt

LGTM, basically.

tnull

Generally LGTM

jkczyz · 2022-09-15T21:40:10Z

Feel free to squash.

jurvis · 2022-09-15T21:58:36Z

squashed to 82b46ed without changes

tnull

LGTM, one nit.

tnull · 2022-09-16T07:44:08Z

 //! # use std::ops::Deref;
 //! #
-//! # #[cfg(not(feature = "std"))]
-//! # use core2::io;


Ah, you could then also remove the dependency, since it's not used anymore:
diff --git lightning-invoice/Cargo.toml lightning-invoice/Cargo.toml index cd67bcd3..852f62f4 100644 --- lightning-invoice/Cargo.toml +++ lightning-invoice/Cargo.toml @@ -15,7 +15,7 @@ rustdoc-args = ["--cfg", "docsrs"] [features] default = ["std"] -no-std = ["hashbrown", "lightning/no-std", "core2/alloc"] +no-std = ["hashbrown", "lightning/no-std"] std = ["bitcoin_hashes/std", "num-traits/std", "lightning/std", "bech32/std"] [dependencies] @@ -25,7 +25,6 @@ secp256k1 = { version = "0.24.0", default-features = false, features = ["recover num-traits = { version = "0.2.8", default-features = false } bitcoin_hashes = { version = "0.11", default-features = false } hashbrown = { version = "0.11", optional = true } -core2 = { version = "0.3.0", default-features = false, optional = true } serde = { version = "1.0.118", optional = true } [dev-dependencies]

👍 addressed and squashed into c353c3e

We do this to enable users to create routers that do not need a scorer. This can be useful if they are running a node the delegates pathfinding. * Move `Score` type parameterization from `InvoicePayer` and `Router` to `DefaultRouter` * Adds a new field, `scorer`, to `DefaultRouter` * Move `AccountsForInFlightHtlcs` to `DefaultRouter`, which we will use to wrap the new `scorer` field, so scoring only happens in `DefaultRouter` explicitly. * Add scoring related functions to `Router` trait that we used to call directly from `InvoicePayer`. * Instead of parameterizing `scorer` in `find_route`, we replace it with inflight_map so `InvoicePayer` can pass on information about inflight HTLCs to the router. * Introduced a new tuple struct, InFlightHtlcs, that wraps functionality for querying used liquidity.

jurvis · 2022-09-05T04:50:02Z

Purpose

A scorer is not necessary for users who may be using a different server from their payer node for routing. This PR will allow them to implement a Router without it.

Implementation

Move Score type parameterization from InvoicePayer and Router to
DefaultRouter
Adds a new field, scorer, to DefaultRouter
Move AccountsForInflightHtlcs to DefaultRouter, which we
will use to wrap the new scorer field, so scoring only happens in
DefaultRouter explicitly.
Add scoring related functions to Router trait that we used to call
directly from InvoicePayer.
Instead of parameterizing scorer in find_route, we replace it with
inflight_map so InvoicePayer can pass on information about inflight
HTLCs to the router.

jurvis · 2022-09-05T04:51:06Z

I haven't updated the test suite yet, but I wanted to seek concept ACKs on my approach. I'm not 100% sure we want to parameterize inflight_htlcs_map in find_route, in particular.

TheBlueMatt · 2022-09-06T19:36:32Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

jkczyz · 2022-09-06T22:17:06Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

My thinking had been we would expand the Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler. Someone may have a RemoteRouter implementation that should still be notified and handle accordingly (e.g., by sending an RPC to the remote server to update the remote scorer).

jurvis · 2022-09-07T02:02:33Z

Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler

ah, got it. makes sense!

jurvis · 2022-09-10T19:35:14Z

Used a different approach in 2032f46 -- instead of implementing EventHandler in DefaultRouter, have payer call payment_path_successful, payment_path_failed, probe_success, and probe_failure as newly defined function in the Router trait.

jurvis · 2022-09-10T19:38:04Z

Docs need updating -- I'll look into that in a future commit

codecov-commenter · 2022-09-10T19:51:31Z

Codecov Report

Base: 90.92% // Head: 92.54% // Increases project coverage by +1.62% 🎉

Coverage data is based on head (df9ea09) compared to base (f99301d).
Patch coverage: 74.21% of modified lines in pull request are covered.

❗ Current head df9ea09 differs from pull request most recent head 82b46ed. Consider uploading reports for the commit 82b46ed to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1694 +/- ##
==========================================
+ Coverage 90.92% 92.54% +1.62% 
==========================================
Files 85 86 +1 Lines 46268 59531 +13263 Branches 46268 59531 +13263 ==========================================
+ Hits 42067 55091 +13024 - Misses 4201 4440 +239

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`95.47% <44.82%> (-1.30%)`	⬇️
lightning-invoice/src/payment.rs	`92.34% <80.46%> (+1.47%)`	⬆️
lightning-background-processor/src/lib.rs	`96.89% <100.00%> (+1.69%)`	⬆️
lightning/src/util/events.rs	`36.73% <0.00%> (-3.04%)`	⬇️
lightning-block-sync/src/init.rs	`93.03% <0.00%> (-0.54%)`	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.47% <0.00%> (-0.25%)`	⬇️
lightning/src/chain/onchaintx.rs	`94.71% <0.00%> (-0.23%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`96.36% <0.00%> (-0.16%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.80% <0.00%> (-0.09%)`	⬇️
lightning/src/ln/script.rs	`92.13% <0.00%> (-0.06%)`	⬇️
... and 39 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Generally LGTM. Would like to make progress quickly here as we may end up needing to backport this for the 0.0.111 bindings.

jkczyz · 2022-09-12T17:29:02Z

 // Since the path is reversed, the last element in our iteration is the first
 // hop.
+ let mut locked_scorer = self.scorer.lock();
+ let scorer = AccountForInFlightHtlcs::new(locked_scorer.deref_mut(), inflight_htlc_map.clone());


It's a little strange that we are testing AccountForInFlightHtlcs this way rather through DefaultRouter. Then again, seems doing the latter would require using the actual find_route code, which wouldn't lend to a simple test setup. Fine with keeping this as is, but wanted to raise this in case others have any better ideas.

jkczyz · 2022-09-12T17:46:23Z

+	fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+ self.scorer.payment_path_failed(path, short_channel_id)
+	}
+
+	fn payment_path_successful(&mut self, path: &[&RouteHop]) {
+ self.scorer.payment_path_successful(path)
+	}
+
+	fn probe_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+ self.scorer.probe_failed(path, short_channel_id)
+	}
+
+	fn probe_successful(&mut self, path: &[&RouteHop]) {
+ self.scorer.probe_successful(path)
+	}


These are never actually called since AccountForInFlightHtlcs is only used for channel_penalty_msat, so they can be implemented as unreachable!(). I wonder if we should split Score into two different traits or if we are fine with duplicating part of the interface in Router. I guess either way, any Score implementation used by DefaultRouter would also need to implement this half of the trait. Not sure what would be prefered. @tnull @TheBlueMatt Any thoughts?

Addressed in 7d95196

Hmm, yea, I think this is fine for now, we're probably gonna end up revisinting all of it in #1668 anyway, so...

tnull

Generally LGTM.

jkczyz

LGTM. Will need a squash once other reviewers are ready.

TheBlueMatt

LGTM, basically.

tnull

Generally LGTM

jkczyz · 2022-09-15T21:40:10Z

Feel free to squash.

jurvis · 2022-09-15T21:58:36Z

squashed to 82b46ed without changes

tnull

LGTM, one nit.

tnull · 2022-09-16T07:44:08Z

 //! # use std::ops::Deref;
 //! #
-//! # #[cfg(not(feature = "std"))]
-//! # use core2::io;


Ah, you could then also remove the dependency, since it's not used anymore:
diff --git lightning-invoice/Cargo.toml lightning-invoice/Cargo.toml index cd67bcd3..852f62f4 100644 --- lightning-invoice/Cargo.toml +++ lightning-invoice/Cargo.toml @@ -15,7 +15,7 @@ rustdoc-args = ["--cfg", "docsrs"] [features] default = ["std"] -no-std = ["hashbrown", "lightning/no-std", "core2/alloc"] +no-std = ["hashbrown", "lightning/no-std"] std = ["bitcoin_hashes/std", "num-traits/std", "lightning/std", "bech32/std"] [dependencies] @@ -25,7 +25,6 @@ secp256k1 = { version = "0.24.0", default-features = false, features = ["recover num-traits = { version = "0.2.8", default-features = false } bitcoin_hashes = { version = "0.11", default-features = false } hashbrown = { version = "0.11", optional = true } -core2 = { version = "0.3.0", default-features = false, optional = true } serde = { version = "1.0.118", optional = true } [dev-dependencies]

👍 addressed and squashed into c353c3e

We do this to enable users to create routers that do not need a scorer. This can be useful if they are running a node the delegates pathfinding. * Move `Score` type parameterization from `InvoicePayer` and `Router` to `DefaultRouter` * Adds a new field, `scorer`, to `DefaultRouter` * Move `AccountsForInFlightHtlcs` to `DefaultRouter`, which we will use to wrap the new `scorer` field, so scoring only happens in `DefaultRouter` explicitly. * Add scoring related functions to `Router` trait that we used to call directly from `InvoicePayer`. * Instead of parameterizing `scorer` in `find_route`, we replace it with inflight_map so `InvoicePayer` can pass on information about inflight HTLCs to the router. * Introduced a new tuple struct, InFlightHtlcs, that wraps functionality for querying used liquidity.

jurvis · 2022-09-05T04:50:02Z

Purpose

A scorer is not necessary for users who may be using a different server from their payer node for routing. This PR will allow them to implement a Router without it.

Implementation

Move Score type parameterization from InvoicePayer and Router to
DefaultRouter
Adds a new field, scorer, to DefaultRouter
Move AccountsForInflightHtlcs to DefaultRouter, which we
will use to wrap the new scorer field, so scoring only happens in
DefaultRouter explicitly.
Add scoring related functions to Router trait that we used to call
directly from InvoicePayer.
Instead of parameterizing scorer in find_route, we replace it with
inflight_map so InvoicePayer can pass on information about inflight
HTLCs to the router.

jurvis · 2022-09-05T04:51:06Z

I haven't updated the test suite yet, but I wanted to seek concept ACKs on my approach. I'm not 100% sure we want to parameterize inflight_htlcs_map in find_route, in particular.

TheBlueMatt · 2022-09-06T19:36:32Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

jkczyz · 2022-09-06T22:17:06Z

Generally concept ack. Its kinda annoying to make DefaultRouter also an EventHandler but only optionally, though - it means we have to pass both the router and the InvoicePayer to the BackgroundProcessor to pipe events through. As an alternative, we could make the Router trait also require EventHandler, which means we could then call through to the Router's event handling directly in InvoicePayer. Its a bit awkward but at least less code for the user, dunno how @jkczyz would feel about that, though.

My thinking had been we would expand the Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler. Someone may have a RemoteRouter implementation that should still be notified and handle accordingly (e.g., by sending an RPC to the remote server to update the remote scorer).

jurvis · 2022-09-07T02:02:33Z

Router trait with methods for handling success / failure rather than having DefaultRouter implement EventHandler

ah, got it. makes sense!

jurvis · 2022-09-10T19:35:14Z

Used a different approach in 2032f46 -- instead of implementing EventHandler in DefaultRouter, have payer call payment_path_successful, payment_path_failed, probe_success, and probe_failure as newly defined function in the Router trait.

jurvis · 2022-09-10T19:38:04Z

Docs need updating -- I'll look into that in a future commit

codecov-commenter · 2022-09-10T19:51:31Z

Codecov Report

Base: 90.92% // Head: 92.54% // Increases project coverage by +1.62% 🎉

Coverage data is based on head (df9ea09) compared to base (f99301d).
Patch coverage: 74.21% of modified lines in pull request are covered.

❗ Current head df9ea09 differs from pull request most recent head 82b46ed. Consider uploading reports for the commit 82b46ed to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1694 +/- ##
==========================================
+ Coverage 90.92% 92.54% +1.62% 
==========================================
Files 85 86 +1 Lines 46268 59531 +13263 Branches 46268 59531 +13263 ==========================================
+ Hits 42067 55091 +13024 - Misses 4201 4440 +239

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`95.47% <44.82%> (-1.30%)`	⬇️
lightning-invoice/src/payment.rs	`92.34% <80.46%> (+1.47%)`	⬆️
lightning-background-processor/src/lib.rs	`96.89% <100.00%> (+1.69%)`	⬆️
lightning/src/util/events.rs	`36.73% <0.00%> (-3.04%)`	⬇️
lightning-block-sync/src/init.rs	`93.03% <0.00%> (-0.54%)`	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.47% <0.00%> (-0.25%)`	⬇️
lightning/src/chain/onchaintx.rs	`94.71% <0.00%> (-0.23%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`96.36% <0.00%> (-0.16%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.80% <0.00%> (-0.09%)`	⬇️
lightning/src/ln/script.rs	`92.13% <0.00%> (-0.06%)`	⬇️
... and 39 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Generally LGTM. Would like to make progress quickly here as we may end up needing to backport this for the 0.0.111 bindings.

jkczyz · 2022-09-12T17:29:02Z

 // Since the path is reversed, the last element in our iteration is the first
 // hop.
+ let mut locked_scorer = self.scorer.lock();
+ let scorer = AccountForInFlightHtlcs::new(locked_scorer.deref_mut(), inflight_htlc_map.clone());


It's a little strange that we are testing AccountForInFlightHtlcs this way rather through DefaultRouter. Then again, seems doing the latter would require using the actual find_route code, which wouldn't lend to a simple test setup. Fine with keeping this as is, but wanted to raise this in case others have any better ideas.

jkczyz · 2022-09-12T17:46:23Z

+	fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+ self.scorer.payment_path_failed(path, short_channel_id)
+	}
+
+	fn payment_path_successful(&mut self, path: &[&RouteHop]) {
+ self.scorer.payment_path_successful(path)
+	}
+
+	fn probe_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+ self.scorer.probe_failed(path, short_channel_id)
+	}
+
+	fn probe_successful(&mut self, path: &[&RouteHop]) {
+ self.scorer.probe_successful(path)
+	}


These are never actually called since AccountForInFlightHtlcs is only used for channel_penalty_msat, so they can be implemented as unreachable!(). I wonder if we should split Score into two different traits or if we are fine with duplicating part of the interface in Router. I guess either way, any Score implementation used by DefaultRouter would also need to implement this half of the trait. Not sure what would be prefered. @tnull @TheBlueMatt Any thoughts?

Addressed in 7d95196

Hmm, yea, I think this is fine for now, we're probably gonna end up revisinting all of it in #1668 anyway, so...

tnull

Generally LGTM.

jkczyz

LGTM. Will need a squash once other reviewers are ready.

TheBlueMatt

LGTM, basically.

tnull

Generally LGTM

jkczyz · 2022-09-15T21:40:10Z

Feel free to squash.

jurvis · 2022-09-15T21:58:36Z

squashed to 82b46ed without changes

tnull

LGTM, one nit.

tnull · 2022-09-16T07:44:08Z

 //! # use std::ops::Deref;
 //! #
-//! # #[cfg(not(feature = "std"))]
-//! # use core2::io;


Ah, you could then also remove the dependency, since it's not used anymore:
diff --git lightning-invoice/Cargo.toml lightning-invoice/Cargo.toml index cd67bcd3..852f62f4 100644 --- lightning-invoice/Cargo.toml +++ lightning-invoice/Cargo.toml @@ -15,7 +15,7 @@ rustdoc-args = ["--cfg", "docsrs"] [features] default = ["std"] -no-std = ["hashbrown", "lightning/no-std", "core2/alloc"] +no-std = ["hashbrown", "lightning/no-std"] std = ["bitcoin_hashes/std", "num-traits/std", "lightning/std", "bech32/std"] [dependencies] @@ -25,7 +25,6 @@ secp256k1 = { version = "0.24.0", default-features = false, features = ["recover num-traits = { version = "0.2.8", default-features = false } bitcoin_hashes = { version = "0.11", default-features = false } hashbrown = { version = "0.11", optional = true } -core2 = { version = "0.3.0", default-features = false, optional = true } serde = { version = "1.0.118", optional = true } [dev-dependencies]

👍 addressed and squashed into c353c3e

We do this to enable users to create routers that do not need a scorer. This can be useful if they are running a node the delegates pathfinding. * Move `Score` type parameterization from `InvoicePayer` and `Router` to `DefaultRouter` * Adds a new field, `scorer`, to `DefaultRouter` * Move `AccountsForInFlightHtlcs` to `DefaultRouter`, which we will use to wrap the new `scorer` field, so scoring only happens in `DefaultRouter` explicitly. * Add scoring related functions to `Router` trait that we used to call directly from `InvoicePayer`. * Instead of parameterizing `scorer` in `find_route`, we replace it with inflight_map so `InvoicePayer` can pass on information about inflight HTLCs to the router. * Introduced a new tuple struct, InFlightHtlcs, that wraps functionality for querying used liquidity.

tnull self-requested a review September 5, 2022 08:32

tnull added the Seeking Conceptual Review label Sep 5, 2022

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 431d157 to 2032f46Compare September 10, 2022 19:32

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 7bd046a to 90132ceCompare September 11, 2022 22:01

tnull reviewed Sep 12, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated
Comment threadlightning-invoice/src/payment.rs Outdated

TheBlueMatt reviewed Sep 12, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 12, 2022
View reviewed changes

jurvis marked this pull request as ready for review September 13, 2022 03:12

tnull reviewed Sep 13, 2022
View reviewed changes

Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning-invoice/src/utils.rs Outdated

jkczyz reviewed Sep 13, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz previously approved these changes Sep 14, 2022
View reviewed changes

TheBlueMatt reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated
Comment threadlightning-invoice/src/payment.rs

jurvis dismissed jkczyz’s stale review via b194c1dSeptember 15, 2022 02:40

tnull reviewed Sep 15, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from df9ea09 to 82b46edCompare September 15, 2022 21:58

jkczyz previously approved these changes Sep 15, 2022
View reviewed changes

tnull reviewed Sep 16, 2022
View reviewed changes

jurvis dismissed jkczyz’s stale review via c353c3eSeptember 16, 2022 15:39

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 82b46ed to c353c3eCompare September 16, 2022 15:39

tnull approved these changes Sep 16, 2022
View reviewed changes

jkczyz approved these changes Sep 16, 2022
View reviewed changes

jkczyz merged commit ca76d06 into lightningdevkit:mainSep 16, 2022

TheBlueMatt mentioned this pull request Sep 19, 2022
[0.0.111-bindings] Bindings Branch Rebase + Backports #1730
Merged

tnull self-requested a review September 5, 2022 08:32

tnull added the Seeking Conceptual Review label Sep 5, 2022

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 431d157 to 2032f46Compare September 10, 2022 19:32

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 7bd046a to 90132ceCompare September 11, 2022 22:01

tnull reviewed Sep 12, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated
Comment threadlightning-invoice/src/payment.rs Outdated

TheBlueMatt reviewed Sep 12, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 12, 2022
View reviewed changes

jurvis marked this pull request as ready for review September 13, 2022 03:12

tnull reviewed Sep 13, 2022
View reviewed changes

Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning-invoice/src/utils.rs Outdated

jkczyz reviewed Sep 13, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz previously approved these changes Sep 14, 2022
View reviewed changes

TheBlueMatt reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated
Comment threadlightning-invoice/src/payment.rs

jurvis dismissed jkczyz’s stale review via b194c1dSeptember 15, 2022 02:40

tnull reviewed Sep 15, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from df9ea09 to 82b46edCompare September 15, 2022 21:58

jkczyz previously approved these changes Sep 15, 2022
View reviewed changes

tnull reviewed Sep 16, 2022
View reviewed changes

jurvis dismissed jkczyz’s stale review via c353c3eSeptember 16, 2022 15:39

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 82b46ed to c353c3eCompare September 16, 2022 15:39

tnull approved these changes Sep 16, 2022
View reviewed changes

jkczyz approved these changes Sep 16, 2022
View reviewed changes

jkczyz merged commit ca76d06 into lightningdevkit:mainSep 16, 2022

TheBlueMatt mentioned this pull request Sep 19, 2022
[0.0.111-bindings] Bindings Branch Rebase + Backports #1730
Merged

tnull self-requested a review September 5, 2022 08:32

tnull added the Seeking Conceptual Review label Sep 5, 2022

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 431d157 to 2032f46Compare September 10, 2022 19:32

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 7bd046a to 90132ceCompare September 11, 2022 22:01

tnull reviewed Sep 12, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated
Comment threadlightning-invoice/src/payment.rs Outdated

TheBlueMatt reviewed Sep 12, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 12, 2022
View reviewed changes

jurvis marked this pull request as ready for review September 13, 2022 03:12

tnull reviewed Sep 13, 2022
View reviewed changes

Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning-invoice/src/utils.rs Outdated

jkczyz reviewed Sep 13, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz previously approved these changes Sep 14, 2022
View reviewed changes

TheBlueMatt reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated
Comment threadlightning-invoice/src/payment.rs

jurvis dismissed jkczyz’s stale review via b194c1dSeptember 15, 2022 02:40

tnull reviewed Sep 15, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from df9ea09 to 82b46edCompare September 15, 2022 21:58

jkczyz previously approved these changes Sep 15, 2022
View reviewed changes

tnull reviewed Sep 16, 2022
View reviewed changes

jurvis dismissed jkczyz’s stale review via c353c3eSeptember 16, 2022 15:39

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 82b46ed to c353c3eCompare September 16, 2022 15:39

tnull approved these changes Sep 16, 2022
View reviewed changes

jkczyz approved these changes Sep 16, 2022
View reviewed changes

jkczyz merged commit ca76d06 into lightningdevkit:mainSep 16, 2022

TheBlueMatt mentioned this pull request Sep 19, 2022
[0.0.111-bindings] Bindings Branch Rebase + Backports #1730
Merged

tnull self-requested a review September 5, 2022 08:32

tnull added the Seeking Conceptual Review label Sep 5, 2022

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 431d157 to 2032f46Compare September 10, 2022 19:32

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 7bd046a to 90132ceCompare September 11, 2022 22:01

tnull reviewed Sep 12, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated
Comment threadlightning-invoice/src/payment.rs Outdated

TheBlueMatt reviewed Sep 12, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 12, 2022
View reviewed changes

jurvis marked this pull request as ready for review September 13, 2022 03:12

tnull reviewed Sep 13, 2022
View reviewed changes

Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning-invoice/src/utils.rs Outdated

jkczyz reviewed Sep 13, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz previously approved these changes Sep 14, 2022
View reviewed changes

TheBlueMatt reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated
Comment threadlightning-invoice/src/payment.rs

jurvis dismissed jkczyz’s stale review via b194c1dSeptember 15, 2022 02:40

tnull reviewed Sep 15, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from df9ea09 to 82b46edCompare September 15, 2022 21:58

jkczyz previously approved these changes Sep 15, 2022
View reviewed changes

tnull reviewed Sep 16, 2022
View reviewed changes

jurvis dismissed jkczyz’s stale review via c353c3eSeptember 16, 2022 15:39

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 82b46ed to c353c3eCompare September 16, 2022 15:39

tnull approved these changes Sep 16, 2022
View reviewed changes

jkczyz approved these changes Sep 16, 2022
View reviewed changes

jkczyz merged commit ca76d06 into lightningdevkit:mainSep 16, 2022

TheBlueMatt mentioned this pull request Sep 19, 2022
[0.0.111-bindings] Bindings Branch Rebase + Backports #1730
Merged

tnull self-requested a review September 5, 2022 08:32

tnull added the Seeking Conceptual Review label Sep 5, 2022

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 431d157 to 2032f46Compare September 10, 2022 19:32

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 7bd046a to 90132ceCompare September 11, 2022 22:01

tnull reviewed Sep 12, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated
Comment threadlightning-invoice/src/payment.rs Outdated

TheBlueMatt reviewed Sep 12, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 12, 2022
View reviewed changes

jurvis marked this pull request as ready for review September 13, 2022 03:12

tnull reviewed Sep 13, 2022
View reviewed changes

Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning-invoice/src/utils.rs Outdated

jkczyz reviewed Sep 13, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz previously approved these changes Sep 14, 2022
View reviewed changes

TheBlueMatt reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated
Comment threadlightning-invoice/src/payment.rs

jurvis dismissed jkczyz’s stale review via b194c1dSeptember 15, 2022 02:40

tnull reviewed Sep 15, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from df9ea09 to 82b46edCompare September 15, 2022 21:58

jkczyz previously approved these changes Sep 15, 2022
View reviewed changes

tnull reviewed Sep 16, 2022
View reviewed changes

jurvis dismissed jkczyz’s stale review via c353c3eSeptember 16, 2022 15:39

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 82b46ed to c353c3eCompare September 16, 2022 15:39

tnull approved these changes Sep 16, 2022
View reviewed changes

jkczyz approved these changes Sep 16, 2022
View reviewed changes

jkczyz merged commit ca76d06 into lightningdevkit:mainSep 16, 2022

TheBlueMatt mentioned this pull request Sep 19, 2022
[0.0.111-bindings] Bindings Branch Rebase + Backports #1730
Merged

tnull self-requested a review September 5, 2022 08:32

tnull added the Seeking Conceptual Review label Sep 5, 2022

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 431d157 to 2032f46Compare September 10, 2022 19:32

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 7bd046a to 90132ceCompare September 11, 2022 22:01

tnull reviewed Sep 12, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated
Comment threadlightning-invoice/src/payment.rs Outdated

TheBlueMatt reviewed Sep 12, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 12, 2022
View reviewed changes

jurvis marked this pull request as ready for review September 13, 2022 03:12

tnull reviewed Sep 13, 2022
View reviewed changes

Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning-invoice/src/utils.rs Outdated

jkczyz reviewed Sep 13, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz previously approved these changes Sep 14, 2022
View reviewed changes

TheBlueMatt reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated
Comment threadlightning-invoice/src/payment.rs

jurvis dismissed jkczyz’s stale review via b194c1dSeptember 15, 2022 02:40

tnull reviewed Sep 15, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from df9ea09 to 82b46edCompare September 15, 2022 21:58

jkczyz previously approved these changes Sep 15, 2022
View reviewed changes

tnull reviewed Sep 16, 2022
View reviewed changes

jurvis dismissed jkczyz’s stale review via c353c3eSeptember 16, 2022 15:39

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 82b46ed to c353c3eCompare September 16, 2022 15:39

tnull approved these changes Sep 16, 2022
View reviewed changes

jkczyz approved these changes Sep 16, 2022
View reviewed changes

jkczyz merged commit ca76d06 into lightningdevkit:mainSep 16, 2022

TheBlueMatt mentioned this pull request Sep 19, 2022
[0.0.111-bindings] Bindings Branch Rebase + Backports #1730
Merged

tnull self-requested a review September 5, 2022 08:32

tnull added the Seeking Conceptual Review label Sep 5, 2022

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 431d157 to 2032f46Compare September 10, 2022 19:32

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 7bd046a to 90132ceCompare September 11, 2022 22:01

tnull reviewed Sep 12, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated
Comment threadlightning-invoice/src/payment.rs Outdated

TheBlueMatt reviewed Sep 12, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 12, 2022
View reviewed changes

jurvis marked this pull request as ready for review September 13, 2022 03:12

tnull reviewed Sep 13, 2022
View reviewed changes

Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning-invoice/src/utils.rs Outdated

jkczyz reviewed Sep 13, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz previously approved these changes Sep 14, 2022
View reviewed changes

TheBlueMatt reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated
Comment threadlightning-invoice/src/payment.rs

jurvis dismissed jkczyz’s stale review via b194c1dSeptember 15, 2022 02:40

tnull reviewed Sep 15, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from df9ea09 to 82b46edCompare September 15, 2022 21:58

jkczyz previously approved these changes Sep 15, 2022
View reviewed changes

tnull reviewed Sep 16, 2022
View reviewed changes

jurvis dismissed jkczyz’s stale review via c353c3eSeptember 16, 2022 15:39

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 82b46ed to c353c3eCompare September 16, 2022 15:39

tnull approved these changes Sep 16, 2022
View reviewed changes

jkczyz approved these changes Sep 16, 2022
View reviewed changes

jkczyz merged commit ca76d06 into lightningdevkit:mainSep 16, 2022

TheBlueMatt mentioned this pull request Sep 19, 2022
[0.0.111-bindings] Bindings Branch Rebase + Backports #1730
Merged

tnull self-requested a review September 5, 2022 08:32

tnull added the Seeking Conceptual Review label Sep 5, 2022

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 431d157 to 2032f46Compare September 10, 2022 19:32

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 7bd046a to 90132ceCompare September 11, 2022 22:01

tnull reviewed Sep 12, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated
Comment threadlightning-invoice/src/payment.rs Outdated

TheBlueMatt reviewed Sep 12, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 12, 2022
View reviewed changes

jurvis marked this pull request as ready for review September 13, 2022 03:12

tnull reviewed Sep 13, 2022
View reviewed changes

Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning-invoice/src/utils.rs Outdated

jkczyz reviewed Sep 13, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jkczyz previously approved these changes Sep 14, 2022
View reviewed changes

TheBlueMatt reviewed Sep 14, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated
Comment threadlightning-invoice/src/payment.rs

jurvis dismissed jkczyz’s stale review via b194c1dSeptember 15, 2022 02:40

tnull reviewed Sep 15, 2022
View reviewed changes

Comment threadlightning-invoice/src/payment.rs Outdated

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from df9ea09 to 82b46edCompare September 15, 2022 21:58

jkczyz previously approved these changes Sep 15, 2022
View reviewed changes

tnull reviewed Sep 16, 2022
View reviewed changes

jurvis dismissed jkczyz’s stale review via c353c3eSeptember 16, 2022 15:39

jurvis force-pushed the jurvis/2022-08-move-scorer-from-router branch from 82b46ed to c353c3eCompare September 16, 2022 15:39

tnull approved these changes Sep 16, 2022
View reviewed changes

jkczyz approved these changes Sep 16, 2022
View reviewed changes

jkczyz merged commit ca76d06 into lightningdevkit:mainSep 16, 2022

TheBlueMatt mentioned this pull request Sep 19, 2022
[0.0.111-bindings] Bindings Branch Rebase + Backports #1730
Merged

Uh oh!

Conversation

jurvis commented Sep 5, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Implementation

Uh oh!

jurvis commented Sep 5, 2022

Uh oh!

TheBlueMatt commented Sep 6, 2022

Uh oh!

jkczyz commented Sep 6, 2022

Uh oh!

jurvis commented Sep 7, 2022

Uh oh!

jurvis commented Sep 10, 2022

Uh oh!

jurvis commented Sep 10, 2022

Uh oh!

codecov-commenter commented Sep 10, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkczyz commented Sep 15, 2022

Uh oh!

jurvis commented Sep 15, 2022

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jurvis commented Sep 5, 2022•
edited
Loading

codecov-commenter commented Sep 10, 2022•
edited
Loading

jurvis commented Sep 5, 2022•
edited
Loading

codecov-commenter commented Sep 10, 2022•
edited
Loading