Periodical external pathfinding scores merge by joostjager · Pull Request #449 · lightningdevkit/ldk-node

joostjager · 2025-01-29T11:14:52Z

Depends on lightningdevkit/rust-lightning#3562

Export functionality in #458

tnull

Thanks! Did a first pass, already looks pretty good!

tnull · 2025-02-06T12:46:10Z

+async fn sync_external_scores(
+	logger: &Logger, scorer: &Mutex<CombinedScorer<Arc<Graph>, Arc<Logger>>>,
+	node_metrics: &RwLock<NodeMetrics>, kv_store: Arc<DynStore>, url: &String,
+) -> () {


Should this return a Result?

I don't think so - this function cannot fail?

tnull · 2025-02-06T12:47:25Z

+ match body {
+ Err(e) => {
+ log_error!(logger, "Failed to read external scores update: {}", e);
+ return;


Let's avoid these explicit returns if possible.

Indeed, not necessary. I wanted to exit the parent function with these returns, but just letting it run would have the same result.
In this current revision I now have a few real ones, after unwrapping the matches a bit.

joostjager · 2025-02-07T10:56:24Z

Comments addressed. Now need to decide in lightningdevkit/rust-lightning#3562 how to read the combined scorer from disk (with local and merged data), and apply that in this PR.

benthecarman · 2025-08-13T16:07:14Z

+) -> () {
+	let response = tokio::time::timeout(
+ Duration::from_secs(EXTERNAL_PATHFINDING_SCORES_SYNC_TIMEOUT_SECS),
+ reqwest::get(url),


should create a reqwest client instead of doing reqwest::get, otherwise it needs to redo TLS and everything each time

should create a reqwest client instead of doing reqwest::get, otherwise it needs to redo TLS and everything each time
Right, but establishing a new connection once an hour is reasonable? And probably preferable compared to keep a connection open if we only use it once an hour?

ah yeah true

joostjager · 2025-09-10T12:30:03Z

This has been parked, but I suppose with the new develop branch we can merge this after a rebase.

tnull · 2025-09-10T12:40:34Z

This has been parked, but I suppose with the new develop branch we can merge this after a rebase.

Yes, please rebase!

tnull · 2025-10-07T09:00:10Z

Gentle ping @joostjager

joostjager · 2025-10-16T12:30:33Z

Rebased

tnull

Two minor comments, otherwise should look good.

tnull · 2025-10-17T06:58:38Z

@joostjager Feel free to squash the fixup. More detailed/verbose commit messages would also be appreciated.

…nal http source

Save external pathfinding scores in a cache so that they will be available immediately after a node restart. Otherwise there might be a time window where new scores need to be downloaded still and the node operates on local data only.

joostjager · 2025-10-23T17:21:14Z

Squashed without changes.

joostjager · 2025-01-29T11:14:52Z

Fixes#313

Depends on lightningdevkit/rust-lightning#3562

Export functionality in #458

tnull

Thanks! Did a first pass, already looks pretty good!

tnull · 2025-02-06T12:46:10Z

+async fn sync_external_scores(
+	logger: &Logger, scorer: &Mutex<CombinedScorer<Arc<Graph>, Arc<Logger>>>,
+	node_metrics: &RwLock<NodeMetrics>, kv_store: Arc<DynStore>, url: &String,
+) -> () {


Should this return a Result?

I don't think so - this function cannot fail?

tnull · 2025-02-06T12:47:25Z

+ match body {
+ Err(e) => {
+ log_error!(logger, "Failed to read external scores update: {}", e);
+ return;


Let's avoid these explicit returns if possible.

Indeed, not necessary. I wanted to exit the parent function with these returns, but just letting it run would have the same result.
In this current revision I now have a few real ones, after unwrapping the matches a bit.

joostjager · 2025-02-07T10:56:24Z

Comments addressed. Now need to decide in lightningdevkit/rust-lightning#3562 how to read the combined scorer from disk (with local and merged data), and apply that in this PR.

benthecarman · 2025-08-13T16:07:14Z

+) -> () {
+	let response = tokio::time::timeout(
+ Duration::from_secs(EXTERNAL_PATHFINDING_SCORES_SYNC_TIMEOUT_SECS),
+ reqwest::get(url),


should create a reqwest client instead of doing reqwest::get, otherwise it needs to redo TLS and everything each time

should create a reqwest client instead of doing reqwest::get, otherwise it needs to redo TLS and everything each time
Right, but establishing a new connection once an hour is reasonable? And probably preferable compared to keep a connection open if we only use it once an hour?

ah yeah true

joostjager · 2025-09-10T12:30:03Z

This has been parked, but I suppose with the new develop branch we can merge this after a rebase.

tnull · 2025-09-10T12:40:34Z

This has been parked, but I suppose with the new develop branch we can merge this after a rebase.

Yes, please rebase!

tnull · 2025-10-07T09:00:10Z

Gentle ping @joostjager

joostjager · 2025-10-16T12:30:33Z

Rebased

tnull

Two minor comments, otherwise should look good.

tnull · 2025-10-17T06:58:38Z

@joostjager Feel free to squash the fixup. More detailed/verbose commit messages would also be appreciated.

…nal http source

Save external pathfinding scores in a cache so that they will be available immediately after a node restart. Otherwise there might be a time window where new scores need to be downloaded still and the node operates on local data only.

joostjager · 2025-10-23T17:21:14Z

Squashed without changes.

joostjager · 2025-01-29T11:14:52Z

Fixes#313

Depends on lightningdevkit/rust-lightning#3562

Export functionality in #458

tnull

Thanks! Did a first pass, already looks pretty good!

tnull · 2025-02-06T12:46:10Z

+async fn sync_external_scores(
+	logger: &Logger, scorer: &Mutex<CombinedScorer<Arc<Graph>, Arc<Logger>>>,
+	node_metrics: &RwLock<NodeMetrics>, kv_store: Arc<DynStore>, url: &String,
+) -> () {


Should this return a Result?

I don't think so - this function cannot fail?

tnull · 2025-02-06T12:47:25Z

+ match body {
+ Err(e) => {
+ log_error!(logger, "Failed to read external scores update: {}", e);
+ return;


Let's avoid these explicit returns if possible.

Indeed, not necessary. I wanted to exit the parent function with these returns, but just letting it run would have the same result.
In this current revision I now have a few real ones, after unwrapping the matches a bit.

joostjager · 2025-02-07T10:56:24Z

Comments addressed. Now need to decide in lightningdevkit/rust-lightning#3562 how to read the combined scorer from disk (with local and merged data), and apply that in this PR.

benthecarman · 2025-08-13T16:07:14Z

+) -> () {
+	let response = tokio::time::timeout(
+ Duration::from_secs(EXTERNAL_PATHFINDING_SCORES_SYNC_TIMEOUT_SECS),
+ reqwest::get(url),


should create a reqwest client instead of doing reqwest::get, otherwise it needs to redo TLS and everything each time

should create a reqwest client instead of doing reqwest::get, otherwise it needs to redo TLS and everything each time
Right, but establishing a new connection once an hour is reasonable? And probably preferable compared to keep a connection open if we only use it once an hour?

ah yeah true

joostjager · 2025-09-10T12:30:03Z

This has been parked, but I suppose with the new develop branch we can merge this after a rebase.

tnull · 2025-09-10T12:40:34Z

This has been parked, but I suppose with the new develop branch we can merge this after a rebase.

Yes, please rebase!

tnull · 2025-10-07T09:00:10Z

Gentle ping @joostjager

joostjager · 2025-10-16T12:30:33Z

Rebased

tnull

Two minor comments, otherwise should look good.

tnull · 2025-10-17T06:58:38Z

@joostjager Feel free to squash the fixup. More detailed/verbose commit messages would also be appreciated.

…nal http source

Save external pathfinding scores in a cache so that they will be available immediately after a node restart. Otherwise there might be a time window where new scores need to be downloaded still and the node operates on local data only.

joostjager · 2025-10-23T17:21:14Z

Squashed without changes.

joostjager · 2025-01-29T11:14:52Z

Fixes#313

Depends on lightningdevkit/rust-lightning#3562

Export functionality in #458

tnull

Thanks! Did a first pass, already looks pretty good!

tnull · 2025-02-06T12:46:10Z

+async fn sync_external_scores(
+	logger: &Logger, scorer: &Mutex<CombinedScorer<Arc<Graph>, Arc<Logger>>>,
+	node_metrics: &RwLock<NodeMetrics>, kv_store: Arc<DynStore>, url: &String,
+) -> () {


Should this return a Result?

I don't think so - this function cannot fail?

tnull · 2025-02-06T12:47:25Z

+ match body {
+ Err(e) => {
+ log_error!(logger, "Failed to read external scores update: {}", e);
+ return;


Let's avoid these explicit returns if possible.

Indeed, not necessary. I wanted to exit the parent function with these returns, but just letting it run would have the same result.
In this current revision I now have a few real ones, after unwrapping the matches a bit.

joostjager · 2025-02-07T10:56:24Z

Comments addressed. Now need to decide in lightningdevkit/rust-lightning#3562 how to read the combined scorer from disk (with local and merged data), and apply that in this PR.

benthecarman · 2025-08-13T16:07:14Z

+) -> () {
+	let response = tokio::time::timeout(
+ Duration::from_secs(EXTERNAL_PATHFINDING_SCORES_SYNC_TIMEOUT_SECS),
+ reqwest::get(url),


should create a reqwest client instead of doing reqwest::get, otherwise it needs to redo TLS and everything each time

should create a reqwest client instead of doing reqwest::get, otherwise it needs to redo TLS and everything each time
Right, but establishing a new connection once an hour is reasonable? And probably preferable compared to keep a connection open if we only use it once an hour?

ah yeah true

joostjager · 2025-09-10T12:30:03Z

This has been parked, but I suppose with the new develop branch we can merge this after a rebase.

tnull · 2025-09-10T12:40:34Z

This has been parked, but I suppose with the new develop branch we can merge this after a rebase.

Yes, please rebase!

tnull · 2025-10-07T09:00:10Z

Gentle ping @joostjager

joostjager · 2025-10-16T12:30:33Z

Rebased

tnull

Two minor comments, otherwise should look good.

tnull · 2025-10-17T06:58:38Z

@joostjager Feel free to squash the fixup. More detailed/verbose commit messages would also be appreciated.

…nal http source

Save external pathfinding scores in a cache so that they will be available immediately after a node restart. Otherwise there might be a time window where new scores need to be downloaded still and the node operates on local data only.

joostjager · 2025-10-23T17:21:14Z

Squashed without changes.

joostjager · 2025-01-29T11:14:52Z

Fixes#313

Depends on lightningdevkit/rust-lightning#3562

Export functionality in #458

tnull

Thanks! Did a first pass, already looks pretty good!

tnull · 2025-02-06T12:46:10Z

+async fn sync_external_scores(
+	logger: &Logger, scorer: &Mutex<CombinedScorer<Arc<Graph>, Arc<Logger>>>,
+	node_metrics: &RwLock<NodeMetrics>, kv_store: Arc<DynStore>, url: &String,
+) -> () {


Should this return a Result?

I don't think so - this function cannot fail?

tnull · 2025-02-06T12:47:25Z

+ match body {
+ Err(e) => {
+ log_error!(logger, "Failed to read external scores update: {}", e);
+ return;


Let's avoid these explicit returns if possible.

Indeed, not necessary. I wanted to exit the parent function with these returns, but just letting it run would have the same result.
In this current revision I now have a few real ones, after unwrapping the matches a bit.

joostjager · 2025-02-07T10:56:24Z

Comments addressed. Now need to decide in lightningdevkit/rust-lightning#3562 how to read the combined scorer from disk (with local and merged data), and apply that in this PR.

benthecarman · 2025-08-13T16:07:14Z

+) -> () {
+	let response = tokio::time::timeout(
+ Duration::from_secs(EXTERNAL_PATHFINDING_SCORES_SYNC_TIMEOUT_SECS),
+ reqwest::get(url),


should create a reqwest client instead of doing reqwest::get, otherwise it needs to redo TLS and everything each time

should create a reqwest client instead of doing reqwest::get, otherwise it needs to redo TLS and everything each time
Right, but establishing a new connection once an hour is reasonable? And probably preferable compared to keep a connection open if we only use it once an hour?

ah yeah true

joostjager · 2025-09-10T12:30:03Z

This has been parked, but I suppose with the new develop branch we can merge this after a rebase.

tnull · 2025-09-10T12:40:34Z

This has been parked, but I suppose with the new develop branch we can merge this after a rebase.

Yes, please rebase!

tnull · 2025-10-07T09:00:10Z

Gentle ping @joostjager

joostjager · 2025-10-16T12:30:33Z

Rebased

tnull

Two minor comments, otherwise should look good.

tnull · 2025-10-17T06:58:38Z

@joostjager Feel free to squash the fixup. More detailed/verbose commit messages would also be appreciated.

…nal http source

Save external pathfinding scores in a cache so that they will be available immediately after a node restart. Otherwise there might be a time window where new scores need to be downloaded still and the node operates on local data only.

joostjager · 2025-10-23T17:21:14Z

Squashed without changes.

joostjager · 2025-01-29T11:14:52Z

Fixes#313

Depends on lightningdevkit/rust-lightning#3562

Export functionality in #458

tnull

Thanks! Did a first pass, already looks pretty good!

tnull · 2025-02-06T12:46:10Z

+async fn sync_external_scores(
+	logger: &Logger, scorer: &Mutex<CombinedScorer<Arc<Graph>, Arc<Logger>>>,
+	node_metrics: &RwLock<NodeMetrics>, kv_store: Arc<DynStore>, url: &String,
+) -> () {


Should this return a Result?

I don't think so - this function cannot fail?

tnull · 2025-02-06T12:47:25Z

+ match body {
+ Err(e) => {
+ log_error!(logger, "Failed to read external scores update: {}", e);
+ return;


Let's avoid these explicit returns if possible.

Indeed, not necessary. I wanted to exit the parent function with these returns, but just letting it run would have the same result.
In this current revision I now have a few real ones, after unwrapping the matches a bit.

joostjager · 2025-02-07T10:56:24Z

Comments addressed. Now need to decide in lightningdevkit/rust-lightning#3562 how to read the combined scorer from disk (with local and merged data), and apply that in this PR.

benthecarman · 2025-08-13T16:07:14Z

+) -> () {
+	let response = tokio::time::timeout(
+ Duration::from_secs(EXTERNAL_PATHFINDING_SCORES_SYNC_TIMEOUT_SECS),
+ reqwest::get(url),


should create a reqwest client instead of doing reqwest::get, otherwise it needs to redo TLS and everything each time

should create a reqwest client instead of doing reqwest::get, otherwise it needs to redo TLS and everything each time
Right, but establishing a new connection once an hour is reasonable? And probably preferable compared to keep a connection open if we only use it once an hour?

ah yeah true

joostjager · 2025-09-10T12:30:03Z

This has been parked, but I suppose with the new develop branch we can merge this after a rebase.

tnull · 2025-09-10T12:40:34Z

This has been parked, but I suppose with the new develop branch we can merge this after a rebase.

Yes, please rebase!

tnull · 2025-10-07T09:00:10Z

Gentle ping @joostjager

joostjager · 2025-10-16T12:30:33Z

Rebased

tnull

Two minor comments, otherwise should look good.

tnull · 2025-10-17T06:58:38Z

@joostjager Feel free to squash the fixup. More detailed/verbose commit messages would also be appreciated.

…nal http source

Save external pathfinding scores in a cache so that they will be available immediately after a node restart. Otherwise there might be a time window where new scores need to be downloaded still and the node operates on local data only.

joostjager · 2025-10-23T17:21:14Z

Squashed without changes.

joostjager · 2025-01-29T11:14:52Z

Fixes#313

Depends on lightningdevkit/rust-lightning#3562

Export functionality in #458

tnull

Thanks! Did a first pass, already looks pretty good!

tnull · 2025-02-06T12:46:10Z

+async fn sync_external_scores(
+	logger: &Logger, scorer: &Mutex<CombinedScorer<Arc<Graph>, Arc<Logger>>>,
+	node_metrics: &RwLock<NodeMetrics>, kv_store: Arc<DynStore>, url: &String,
+) -> () {


Should this return a Result?

I don't think so - this function cannot fail?

tnull · 2025-02-06T12:47:25Z

+ match body {
+ Err(e) => {
+ log_error!(logger, "Failed to read external scores update: {}", e);
+ return;


Let's avoid these explicit returns if possible.

Indeed, not necessary. I wanted to exit the parent function with these returns, but just letting it run would have the same result.
In this current revision I now have a few real ones, after unwrapping the matches a bit.

joostjager · 2025-02-07T10:56:24Z

Comments addressed. Now need to decide in lightningdevkit/rust-lightning#3562 how to read the combined scorer from disk (with local and merged data), and apply that in this PR.

benthecarman · 2025-08-13T16:07:14Z

+) -> () {
+	let response = tokio::time::timeout(
+ Duration::from_secs(EXTERNAL_PATHFINDING_SCORES_SYNC_TIMEOUT_SECS),
+ reqwest::get(url),


should create a reqwest client instead of doing reqwest::get, otherwise it needs to redo TLS and everything each time

should create a reqwest client instead of doing reqwest::get, otherwise it needs to redo TLS and everything each time
Right, but establishing a new connection once an hour is reasonable? And probably preferable compared to keep a connection open if we only use it once an hour?

ah yeah true

joostjager · 2025-09-10T12:30:03Z

This has been parked, but I suppose with the new develop branch we can merge this after a rebase.

tnull · 2025-09-10T12:40:34Z

This has been parked, but I suppose with the new develop branch we can merge this after a rebase.

Yes, please rebase!

tnull · 2025-10-07T09:00:10Z

Gentle ping @joostjager

joostjager · 2025-10-16T12:30:33Z

Rebased

tnull

Two minor comments, otherwise should look good.

tnull · 2025-10-17T06:58:38Z

@joostjager Feel free to squash the fixup. More detailed/verbose commit messages would also be appreciated.

…nal http source

Save external pathfinding scores in a cache so that they will be available immediately after a node restart. Otherwise there might be a time window where new scores need to be downloaded still and the node operates on local data only.

joostjager · 2025-10-23T17:21:14Z

Squashed without changes.

joostjager mentioned this pull request Jan 30, 2025
Merge probabilistic scores from external source lightningdevkit/rust-lightning#3562
Merged

joostjager changed the title ~~Import scores~~Periodical external score mergeJan 30, 2025

joostjager force-pushed the import-scores branch 2 times, most recently from e0b7070 to 58acc12Compare January 30, 2025 13:31

joostjager commented Jan 30, 2025
View reviewed changes

Comment threadsrc/config.rs Outdated

joostjager force-pushed the import-scores branch 3 times, most recently from 5858404 to fdf2b5dCompare January 30, 2025 14:04

joostjager marked this pull request as ready for review January 31, 2025 08:53

joostjager commented Jan 31, 2025
View reviewed changes

Comment threadCargo.toml Outdated

tnull added this to the 0.6 milestone Jan 31, 2025

tnull reviewed Jan 31, 2025
View reviewed changes

joostjager force-pushed the import-scores branch 2 times, most recently from 5a42ff9 to 0cf5774Compare February 1, 2025 17:04

joostjager commented Feb 1, 2025
View reviewed changes

Comment threadsrc/lib.rs Outdated

joostjager commented Feb 1, 2025
View reviewed changes

Comment threadsrc/lib.rs Outdated

joostjager force-pushed the import-scores branch 3 times, most recently from 619c9c9 to 6ded8afCompare February 3, 2025 10:01

tnull reviewed Feb 3, 2025
View reviewed changes

Comment threadsrc/lib.rs Outdated

joostjager force-pushed the import-scores branch 5 times, most recently from 7bf68af to 14167ebCompare February 4, 2025 12:33

joostjager requested a review from tnull February 4, 2025 14:43

joostjager mentioned this pull request Feb 5, 2025
Add pathfinding scores export method #458
Merged

joostjager changed the title ~~Periodical external score merge~~Periodical external pathfinding scores mergeFeb 5, 2025

tnull reviewed Feb 6, 2025
View reviewed changes

joostjager force-pushed the import-scores branch from 14167eb to 1a4b051Compare February 7, 2025 10:53

tnull reviewed Feb 10, 2025
View reviewed changes

Comment threadsrc/builder.rs
Comment threadsrc/builder.rs Outdated
Comment threadsrc/scoring.rs Outdated

joostjager force-pushed the import-scores branch from 7c9309d to d6bb7cfCompare February 11, 2025 07:41

joostjager requested a review from tnull February 11, 2025 07:42

joostjager force-pushed the import-scores branch from d6bb7cf to 5ae6a35Compare February 11, 2025 07:42

tnull modified the milestones: 0.6, 0.7 Jun 9, 2025

benthecarman mentioned this pull request Aug 7, 2025
Alpha/MVP Release lightningdevkit/orange-sdk#3
Open
24 tasks

benthecarman reviewed Aug 13, 2025
View reviewed changes

remove unnecessary arc clone
94cc2dc

joostjager force-pushed the import-scores branch from 5ae6a35 to 19bf276Compare October 16, 2025 11:21

tnull reviewed Oct 16, 2025
View reviewed changes

Comment threadsrc/scoring.rs Outdated
Comment threadsrc/scoring.rs Outdated

joostjager force-pushed the import-scores branch from 0dfafb6 to 961e67fCompare October 16, 2025 12:56

joostjager requested a review from tnull October 16, 2025 12:57

joostjager added this to Weekly Goals Oct 16, 2025

joostjager self-assigned this Oct 16, 2025

tnull approved these changes Oct 17, 2025
View reviewed changes

joostjager added 2 commits October 23, 2025 19:20

add functionality to periodically update routing scores from an exter…
49502cf
…nal http source

cache external pathfinding scores
52705d3
Save external pathfinding scores in a cache so that they will be available immediately after a node restart. Otherwise there might be a time window where new scores need to be downloaded still and the node operates on local data only.

joostjager force-pushed the import-scores branch from 961e67f to 52705d3Compare October 23, 2025 17:21

joostjager merged commit 9d71d3a into lightningdevkit:mainOct 23, 2025
8 of 15 checks passed

github-project-automationBot moved this to Done in Weekly Goals Oct 23, 2025