Skip to content

Payment benchmark - #664

Merged
tnull merged 2 commits into
lightningdevkit:mainfrom
joostjager:payment-benchmark
Nov 3, 2025
Merged

Payment benchmark#664
tnull merged 2 commits into
lightningdevkit:mainfrom
joostjager:payment-benchmark

Conversation

@joostjager

@joostjagerjoostjager commented Oct 15, 2025

Copy link
Copy Markdown
Contributor

This PR adds a new async payment benchmark test to integration_tests_rust.rs. The test sets up two nodes with a channel and measures performance while sending 1,000 concurrent payments. It uses Tokio’s multi-threaded runtime to spawn payments as separate async tasks and tracks completion through LDK events. The benchmark provides a simple way to gauge throughput and identify performance bottlenecks in payment handling.

Related: lightningdevkit/rust-lightning#4161

@ldk-reviews-bot

ldk-reviews-bot commented Oct 15, 2025

Copy link
Copy Markdown

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@joostjager

Copy link
Copy Markdown
ContributorAuthor

Converted test to criterion benchmark and added it to CI. Also removed the payment throttling, now that it has been established that there is a bug in lightning-net-tokio that causes this. For the benchmark, the 1000 payments can now simply be launched at the same time.

Criterion reports outliers, so this should be able to detect to occassional 30 sec hangs in the payments test.

@joostjager
joostjager marked this pull request as ready for review October 21, 2025 13:04
@joostjager
joostjager requested a review from tnullOctober 21, 2025 13:04
Comment threadCargo.toml Outdated
Comment threadbenches/payments.rs
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 3rd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@joostjager
joostjager removed the request for review from tnullOctober 28, 2025 09:33
@joostjager

Copy link
Copy Markdown
ContributorAuthor

Rebased onto async test framework changes.

@tnull

Copy link
Copy Markdown
Collaborator

Mind rebasing once more to fix CI?

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! The test store changes don't make sense to me, I believe you intended something else there. Otherwise some comments.

Comment threadtests/common/mod.rs Outdated
let node = builder.build_with_store(test_sync_store).unwrap();
let kv_store: Arc<DynStore> = match config.store_type {
TestStoreType::InMemory => {
Arc::new(TestSyncStore::new(config.node_config.storage_dir_path.into()))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This doesn't do what you think it does. This is TestSyncStore, a store that writes to multiple backends to ensure they are kept in-sync, not the in-memory TestStore.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, I missed that indeed. Renamed InMemory to TestSyncStore. The intention of the commit is to allow a test/benchmark to run with sqlite (only).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment thread.github/workflows/rust.yml Outdated
Comment threadCargo.toml Outdated
Comment threadbenches/payments.rs
Comment threadbenches/payments.rs

// Spawn each payment as a separate async task
task::spawn(async move {
println!("{}: Starting payment", payment_hash.0.as_hex());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are we positive all the printing IO doesn't impact the performance? Should we omit this for 'production' benchmarking?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I did wonder the same. Maybe I should indeed remove it to be sure. Would that be a cfg flag?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could be, but do we really need them in the first place?

Comment threadbenches/payments.rs
Comment threadbenches/payments.rs
// Pre-check the HTLC slots to try to avoid the performance impact of a failed payment.
while node_a.list_channels()[0].next_outbound_htlc_limit_msat == 0 {
println!("{}: Waiting for HTLC slots to free up", payment_hash.0.as_hex());
tokio::time::sleep(std::time::Duration::from_millis(100)).await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How did you determine 100ms here? Do we have an intuition if this could starve any of the waiting payers?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It isn't great indeed. Basically something that looked 'reasonable' to me. I think it is a weakness in the API that a user even needs to do this.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment threadtests/common/mod.rs Outdated
Arc::new(TestSyncStore::new(config.node_config.storage_dir_path.into()))
},
TestStoreType::Sqlite => Arc::new(
SqliteStore::new(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's just use the default build() here, which defaults to SQLite.

If you prefer to have it explicit, feel free to rename build to build_with_sqlite_store and have a new build method on Builder call it. Then you can explicitly set build_with_sqlite_store here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No, it's alright to use build. Done.

Comment threadbenches/payments.rs
// Pre-check the HTLC slots to try to avoid the performance impact of a failed payment.
while node_a.list_channels()[0].next_outbound_htlc_limit_msat == 0 {
println!("{}: Waiting for HTLC slots to free up", payment_hash.0.as_hex());
tokio::time::sleep(std::time::Duration::from_millis(100)).await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment threadCargo.toml Outdated

vss-client = "0.3"
prost = { version = "0.11.6", default-features = false}
criterion = { version = "0.7.0", features = ["async_tokio"] }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this not a dev-dependency?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed

@joostjager
joostjagerforce-pushed the payment-benchmark branch 2 times, most recently from b9fc54d to c231f9dCompareOctober 30, 2025 14:02
@joostjager
joostjager requested a review from tnullOctober 30, 2025 14:03

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Feel free to squash fixups. CI is currently broken with warnings and errors:

warning: unused imports: `KV_TABLE_NAME` and `SQLITE_DB_FILE_NAME`
--> tests/common/mod.rs:32:47
|
32 | use ldk_node::io::sqlite_store::{SqliteStore, KV_TABLE_NAME, SQLITE_DB_FILE_NAME};
| ^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(unused_imports)]` on by default
warning: unused import: `DynStore`
--> tests/common/mod.rs:35:28
|
35 | Builder, CustomTlvRecord, DynStore, Event, LightningBalance, Node, NodeError,
| ^^^^^^^^
error[E0308]: arguments to this function are incorrect
--> tests/benchmarks.rs:185:15
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^^^^^^^^
|
note: expected `Arc<Node>`, found `Arc<Arc<Node>>`
--> tests/benchmarks.rs:185:29
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^
= note: expected struct `Arc<ldk_node::Node>`
found struct `Arc<Arc<ldk_node::Node>>`
note: expected `Arc<Node>`, found `Arc<Arc<Node>>`
--> tests/benchmarks.rs:185:37
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^
= note: expected struct `Arc<ldk_node::Node>`
found struct `Arc<Arc<ldk_node::Node>>`
note: function defined here
--> tests/benchmarks.rs:63:10
|
63 | async fn send_payments(node_a: Arc<Node>, node_b: Arc<Node>) -> std::time::Duration {
| ^^^^^^^^^^^^^ ----------------- -----------------

To enable more realistic testing with sqlite as a backend.
@joostjager

Copy link
Copy Markdown
ContributorAuthor

Had to move the code to the standard benches folder, which is better anyway.

@joostjager
joostjager requested a review from tnullNovember 3, 2025 08:43

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, please squash.

Introduces a criterion-based benchmark that sends 1000 concurrent
payments between two LDK nodes to measure total duration. Also adds a
CI job to automatically run the benchmark.
@tnull
tnull merged commit 22ae4d2 into lightningdevkit:mainNov 3, 2025
17 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@joostjager@ldk-reviews-bot@tnull
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Payment benchmark by joostjager · Pull Request #664 · lightningdevkit/ldk-node · GitHub
Skip to content

Payment benchmark - #664

Merged
tnull merged 2 commits into
lightningdevkit:mainfrom
joostjager:payment-benchmark
Nov 3, 2025
Merged

Payment benchmark#664
tnull merged 2 commits into
lightningdevkit:mainfrom
joostjager:payment-benchmark

Conversation

@joostjager

@joostjagerjoostjager commented Oct 15, 2025

Copy link
Copy Markdown
Contributor

This PR adds a new async payment benchmark test to integration_tests_rust.rs. The test sets up two nodes with a channel and measures performance while sending 1,000 concurrent payments. It uses Tokio’s multi-threaded runtime to spawn payments as separate async tasks and tracks completion through LDK events. The benchmark provides a simple way to gauge throughput and identify performance bottlenecks in payment handling.

Related: lightningdevkit/rust-lightning#4161

@ldk-reviews-bot

ldk-reviews-bot commented Oct 15, 2025

Copy link
Copy Markdown

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@joostjager

Copy link
Copy Markdown
ContributorAuthor

Converted test to criterion benchmark and added it to CI. Also removed the payment throttling, now that it has been established that there is a bug in lightning-net-tokio that causes this. For the benchmark, the 1000 payments can now simply be launched at the same time.

Criterion reports outliers, so this should be able to detect to occassional 30 sec hangs in the payments test.

@joostjager
joostjager marked this pull request as ready for review October 21, 2025 13:04
@joostjager
joostjager requested a review from tnullOctober 21, 2025 13:04
Comment threadCargo.toml Outdated
Comment threadbenches/payments.rs
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 3rd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@joostjager
joostjager removed the request for review from tnullOctober 28, 2025 09:33
@joostjager

Copy link
Copy Markdown
ContributorAuthor

Rebased onto async test framework changes.

@tnull

Copy link
Copy Markdown
Collaborator

Mind rebasing once more to fix CI?

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! The test store changes don't make sense to me, I believe you intended something else there. Otherwise some comments.

Comment threadtests/common/mod.rs Outdated
let node = builder.build_with_store(test_sync_store).unwrap();
let kv_store: Arc<DynStore> = match config.store_type {
TestStoreType::InMemory => {
Arc::new(TestSyncStore::new(config.node_config.storage_dir_path.into()))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This doesn't do what you think it does. This is TestSyncStore, a store that writes to multiple backends to ensure they are kept in-sync, not the in-memory TestStore.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, I missed that indeed. Renamed InMemory to TestSyncStore. The intention of the commit is to allow a test/benchmark to run with sqlite (only).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment thread.github/workflows/rust.yml Outdated
Comment threadCargo.toml Outdated
Comment threadbenches/payments.rs
Comment threadbenches/payments.rs

// Spawn each payment as a separate async task
task::spawn(async move {
println!("{}: Starting payment", payment_hash.0.as_hex());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are we positive all the printing IO doesn't impact the performance? Should we omit this for 'production' benchmarking?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I did wonder the same. Maybe I should indeed remove it to be sure. Would that be a cfg flag?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could be, but do we really need them in the first place?

Comment threadbenches/payments.rs
Comment threadbenches/payments.rs
// Pre-check the HTLC slots to try to avoid the performance impact of a failed payment.
while node_a.list_channels()[0].next_outbound_htlc_limit_msat == 0 {
println!("{}: Waiting for HTLC slots to free up", payment_hash.0.as_hex());
tokio::time::sleep(std::time::Duration::from_millis(100)).await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How did you determine 100ms here? Do we have an intuition if this could starve any of the waiting payers?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It isn't great indeed. Basically something that looked 'reasonable' to me. I think it is a weakness in the API that a user even needs to do this.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment threadtests/common/mod.rs Outdated
Arc::new(TestSyncStore::new(config.node_config.storage_dir_path.into()))
},
TestStoreType::Sqlite => Arc::new(
SqliteStore::new(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's just use the default build() here, which defaults to SQLite.

If you prefer to have it explicit, feel free to rename build to build_with_sqlite_store and have a new build method on Builder call it. Then you can explicitly set build_with_sqlite_store here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No, it's alright to use build. Done.

Comment threadbenches/payments.rs
// Pre-check the HTLC slots to try to avoid the performance impact of a failed payment.
while node_a.list_channels()[0].next_outbound_htlc_limit_msat == 0 {
println!("{}: Waiting for HTLC slots to free up", payment_hash.0.as_hex());
tokio::time::sleep(std::time::Duration::from_millis(100)).await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment threadCargo.toml Outdated

vss-client = "0.3"
prost = { version = "0.11.6", default-features = false}
criterion = { version = "0.7.0", features = ["async_tokio"] }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this not a dev-dependency?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed

@joostjager
joostjagerforce-pushed the payment-benchmark branch 2 times, most recently from b9fc54d to c231f9dCompareOctober 30, 2025 14:02
@joostjager
joostjager requested a review from tnullOctober 30, 2025 14:03

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Feel free to squash fixups. CI is currently broken with warnings and errors:

warning: unused imports: `KV_TABLE_NAME` and `SQLITE_DB_FILE_NAME`
--> tests/common/mod.rs:32:47
|
32 | use ldk_node::io::sqlite_store::{SqliteStore, KV_TABLE_NAME, SQLITE_DB_FILE_NAME};
| ^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(unused_imports)]` on by default
warning: unused import: `DynStore`
--> tests/common/mod.rs:35:28
|
35 | Builder, CustomTlvRecord, DynStore, Event, LightningBalance, Node, NodeError,
| ^^^^^^^^
error[E0308]: arguments to this function are incorrect
--> tests/benchmarks.rs:185:15
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^^^^^^^^
|
note: expected `Arc<Node>`, found `Arc<Arc<Node>>`
--> tests/benchmarks.rs:185:29
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^
= note: expected struct `Arc<ldk_node::Node>`
found struct `Arc<Arc<ldk_node::Node>>`
note: expected `Arc<Node>`, found `Arc<Arc<Node>>`
--> tests/benchmarks.rs:185:37
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^
= note: expected struct `Arc<ldk_node::Node>`
found struct `Arc<Arc<ldk_node::Node>>`
note: function defined here
--> tests/benchmarks.rs:63:10
|
63 | async fn send_payments(node_a: Arc<Node>, node_b: Arc<Node>) -> std::time::Duration {
| ^^^^^^^^^^^^^ ----------------- -----------------

To enable more realistic testing with sqlite as a backend.
@joostjager

Copy link
Copy Markdown
ContributorAuthor

Had to move the code to the standard benches folder, which is better anyway.

@joostjager
joostjager requested a review from tnullNovember 3, 2025 08:43

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, please squash.

Introduces a criterion-based benchmark that sends 1000 concurrent
payments between two LDK nodes to measure total duration. Also adds a
CI job to automatically run the benchmark.
@tnull
tnull merged commit 22ae4d2 into lightningdevkit:mainNov 3, 2025
17 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@joostjager@ldk-reviews-bot@tnull
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Payment benchmark by joostjager · Pull Request #664 · lightningdevkit/ldk-node · GitHub
Skip to content

Payment benchmark - #664

Merged
tnull merged 2 commits into
lightningdevkit:mainfrom
joostjager:payment-benchmark
Nov 3, 2025
Merged

Payment benchmark#664
tnull merged 2 commits into
lightningdevkit:mainfrom
joostjager:payment-benchmark

Conversation

@joostjager

@joostjagerjoostjager commented Oct 15, 2025

Copy link
Copy Markdown
Contributor

This PR adds a new async payment benchmark test to integration_tests_rust.rs. The test sets up two nodes with a channel and measures performance while sending 1,000 concurrent payments. It uses Tokio’s multi-threaded runtime to spawn payments as separate async tasks and tracks completion through LDK events. The benchmark provides a simple way to gauge throughput and identify performance bottlenecks in payment handling.

Related: lightningdevkit/rust-lightning#4161

@ldk-reviews-bot

ldk-reviews-bot commented Oct 15, 2025

Copy link
Copy Markdown

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@joostjager

Copy link
Copy Markdown
ContributorAuthor

Converted test to criterion benchmark and added it to CI. Also removed the payment throttling, now that it has been established that there is a bug in lightning-net-tokio that causes this. For the benchmark, the 1000 payments can now simply be launched at the same time.

Criterion reports outliers, so this should be able to detect to occassional 30 sec hangs in the payments test.

@joostjager
joostjager marked this pull request as ready for review October 21, 2025 13:04
@joostjager
joostjager requested a review from tnullOctober 21, 2025 13:04
Comment threadCargo.toml Outdated
Comment threadbenches/payments.rs
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 3rd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@joostjager
joostjager removed the request for review from tnullOctober 28, 2025 09:33
@joostjager

Copy link
Copy Markdown
ContributorAuthor

Rebased onto async test framework changes.

@tnull

Copy link
Copy Markdown
Collaborator

Mind rebasing once more to fix CI?

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! The test store changes don't make sense to me, I believe you intended something else there. Otherwise some comments.

Comment threadtests/common/mod.rs Outdated
let node = builder.build_with_store(test_sync_store).unwrap();
let kv_store: Arc<DynStore> = match config.store_type {
TestStoreType::InMemory => {
Arc::new(TestSyncStore::new(config.node_config.storage_dir_path.into()))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This doesn't do what you think it does. This is TestSyncStore, a store that writes to multiple backends to ensure they are kept in-sync, not the in-memory TestStore.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, I missed that indeed. Renamed InMemory to TestSyncStore. The intention of the commit is to allow a test/benchmark to run with sqlite (only).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment thread.github/workflows/rust.yml Outdated
Comment threadCargo.toml Outdated
Comment threadbenches/payments.rs
Comment threadbenches/payments.rs

// Spawn each payment as a separate async task
task::spawn(async move {
println!("{}: Starting payment", payment_hash.0.as_hex());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are we positive all the printing IO doesn't impact the performance? Should we omit this for 'production' benchmarking?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I did wonder the same. Maybe I should indeed remove it to be sure. Would that be a cfg flag?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could be, but do we really need them in the first place?

Comment threadbenches/payments.rs
Comment threadbenches/payments.rs
// Pre-check the HTLC slots to try to avoid the performance impact of a failed payment.
while node_a.list_channels()[0].next_outbound_htlc_limit_msat == 0 {
println!("{}: Waiting for HTLC slots to free up", payment_hash.0.as_hex());
tokio::time::sleep(std::time::Duration::from_millis(100)).await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How did you determine 100ms here? Do we have an intuition if this could starve any of the waiting payers?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It isn't great indeed. Basically something that looked 'reasonable' to me. I think it is a weakness in the API that a user even needs to do this.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment threadtests/common/mod.rs Outdated
Arc::new(TestSyncStore::new(config.node_config.storage_dir_path.into()))
},
TestStoreType::Sqlite => Arc::new(
SqliteStore::new(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's just use the default build() here, which defaults to SQLite.

If you prefer to have it explicit, feel free to rename build to build_with_sqlite_store and have a new build method on Builder call it. Then you can explicitly set build_with_sqlite_store here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No, it's alright to use build. Done.

Comment threadbenches/payments.rs
// Pre-check the HTLC slots to try to avoid the performance impact of a failed payment.
while node_a.list_channels()[0].next_outbound_htlc_limit_msat == 0 {
println!("{}: Waiting for HTLC slots to free up", payment_hash.0.as_hex());
tokio::time::sleep(std::time::Duration::from_millis(100)).await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment threadCargo.toml Outdated

vss-client = "0.3"
prost = { version = "0.11.6", default-features = false}
criterion = { version = "0.7.0", features = ["async_tokio"] }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this not a dev-dependency?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed

@joostjager
joostjagerforce-pushed the payment-benchmark branch 2 times, most recently from b9fc54d to c231f9dCompareOctober 30, 2025 14:02
@joostjager
joostjager requested a review from tnullOctober 30, 2025 14:03

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Feel free to squash fixups. CI is currently broken with warnings and errors:

warning: unused imports: `KV_TABLE_NAME` and `SQLITE_DB_FILE_NAME`
--> tests/common/mod.rs:32:47
|
32 | use ldk_node::io::sqlite_store::{SqliteStore, KV_TABLE_NAME, SQLITE_DB_FILE_NAME};
| ^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(unused_imports)]` on by default
warning: unused import: `DynStore`
--> tests/common/mod.rs:35:28
|
35 | Builder, CustomTlvRecord, DynStore, Event, LightningBalance, Node, NodeError,
| ^^^^^^^^
error[E0308]: arguments to this function are incorrect
--> tests/benchmarks.rs:185:15
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^^^^^^^^
|
note: expected `Arc<Node>`, found `Arc<Arc<Node>>`
--> tests/benchmarks.rs:185:29
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^
= note: expected struct `Arc<ldk_node::Node>`
found struct `Arc<Arc<ldk_node::Node>>`
note: expected `Arc<Node>`, found `Arc<Arc<Node>>`
--> tests/benchmarks.rs:185:37
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^
= note: expected struct `Arc<ldk_node::Node>`
found struct `Arc<Arc<ldk_node::Node>>`
note: function defined here
--> tests/benchmarks.rs:63:10
|
63 | async fn send_payments(node_a: Arc<Node>, node_b: Arc<Node>) -> std::time::Duration {
| ^^^^^^^^^^^^^ ----------------- -----------------

To enable more realistic testing with sqlite as a backend.
@joostjager

Copy link
Copy Markdown
ContributorAuthor

Had to move the code to the standard benches folder, which is better anyway.

@joostjager
joostjager requested a review from tnullNovember 3, 2025 08:43

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, please squash.

Introduces a criterion-based benchmark that sends 1000 concurrent
payments between two LDK nodes to measure total duration. Also adds a
CI job to automatically run the benchmark.
@tnull
tnull merged commit 22ae4d2 into lightningdevkit:mainNov 3, 2025
17 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@joostjager@ldk-reviews-bot@tnull
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Payment benchmark by joostjager · Pull Request #664 · lightningdevkit/ldk-node · GitHub
Skip to content

Payment benchmark - #664

Merged
tnull merged 2 commits into
lightningdevkit:mainfrom
joostjager:payment-benchmark
Nov 3, 2025
Merged

Payment benchmark#664
tnull merged 2 commits into
lightningdevkit:mainfrom
joostjager:payment-benchmark

Conversation

@joostjager

@joostjagerjoostjager commented Oct 15, 2025

Copy link
Copy Markdown
Contributor

This PR adds a new async payment benchmark test to integration_tests_rust.rs. The test sets up two nodes with a channel and measures performance while sending 1,000 concurrent payments. It uses Tokio’s multi-threaded runtime to spawn payments as separate async tasks and tracks completion through LDK events. The benchmark provides a simple way to gauge throughput and identify performance bottlenecks in payment handling.

Related: lightningdevkit/rust-lightning#4161

@ldk-reviews-bot

ldk-reviews-bot commented Oct 15, 2025

Copy link
Copy Markdown

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@joostjager

Copy link
Copy Markdown
ContributorAuthor

Converted test to criterion benchmark and added it to CI. Also removed the payment throttling, now that it has been established that there is a bug in lightning-net-tokio that causes this. For the benchmark, the 1000 payments can now simply be launched at the same time.

Criterion reports outliers, so this should be able to detect to occassional 30 sec hangs in the payments test.

@joostjager
joostjager marked this pull request as ready for review October 21, 2025 13:04
@joostjager
joostjager requested a review from tnullOctober 21, 2025 13:04
Comment threadCargo.toml Outdated
Comment threadbenches/payments.rs
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 3rd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@joostjager
joostjager removed the request for review from tnullOctober 28, 2025 09:33
@joostjager

Copy link
Copy Markdown
ContributorAuthor

Rebased onto async test framework changes.

@tnull

Copy link
Copy Markdown
Collaborator

Mind rebasing once more to fix CI?

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! The test store changes don't make sense to me, I believe you intended something else there. Otherwise some comments.

Comment threadtests/common/mod.rs Outdated
let node = builder.build_with_store(test_sync_store).unwrap();
let kv_store: Arc<DynStore> = match config.store_type {
TestStoreType::InMemory => {
Arc::new(TestSyncStore::new(config.node_config.storage_dir_path.into()))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This doesn't do what you think it does. This is TestSyncStore, a store that writes to multiple backends to ensure they are kept in-sync, not the in-memory TestStore.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, I missed that indeed. Renamed InMemory to TestSyncStore. The intention of the commit is to allow a test/benchmark to run with sqlite (only).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment thread.github/workflows/rust.yml Outdated
Comment threadCargo.toml Outdated
Comment threadbenches/payments.rs
Comment threadbenches/payments.rs

// Spawn each payment as a separate async task
task::spawn(async move {
println!("{}: Starting payment", payment_hash.0.as_hex());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are we positive all the printing IO doesn't impact the performance? Should we omit this for 'production' benchmarking?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I did wonder the same. Maybe I should indeed remove it to be sure. Would that be a cfg flag?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could be, but do we really need them in the first place?

Comment threadbenches/payments.rs
Comment threadbenches/payments.rs
// Pre-check the HTLC slots to try to avoid the performance impact of a failed payment.
while node_a.list_channels()[0].next_outbound_htlc_limit_msat == 0 {
println!("{}: Waiting for HTLC slots to free up", payment_hash.0.as_hex());
tokio::time::sleep(std::time::Duration::from_millis(100)).await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How did you determine 100ms here? Do we have an intuition if this could starve any of the waiting payers?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It isn't great indeed. Basically something that looked 'reasonable' to me. I think it is a weakness in the API that a user even needs to do this.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment threadtests/common/mod.rs Outdated
Arc::new(TestSyncStore::new(config.node_config.storage_dir_path.into()))
},
TestStoreType::Sqlite => Arc::new(
SqliteStore::new(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's just use the default build() here, which defaults to SQLite.

If you prefer to have it explicit, feel free to rename build to build_with_sqlite_store and have a new build method on Builder call it. Then you can explicitly set build_with_sqlite_store here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No, it's alright to use build. Done.

Comment threadbenches/payments.rs
// Pre-check the HTLC slots to try to avoid the performance impact of a failed payment.
while node_a.list_channels()[0].next_outbound_htlc_limit_msat == 0 {
println!("{}: Waiting for HTLC slots to free up", payment_hash.0.as_hex());
tokio::time::sleep(std::time::Duration::from_millis(100)).await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment threadCargo.toml Outdated

vss-client = "0.3"
prost = { version = "0.11.6", default-features = false}
criterion = { version = "0.7.0", features = ["async_tokio"] }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this not a dev-dependency?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed

@joostjager
joostjagerforce-pushed the payment-benchmark branch 2 times, most recently from b9fc54d to c231f9dCompareOctober 30, 2025 14:02
@joostjager
joostjager requested a review from tnullOctober 30, 2025 14:03

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Feel free to squash fixups. CI is currently broken with warnings and errors:

warning: unused imports: `KV_TABLE_NAME` and `SQLITE_DB_FILE_NAME`
--> tests/common/mod.rs:32:47
|
32 | use ldk_node::io::sqlite_store::{SqliteStore, KV_TABLE_NAME, SQLITE_DB_FILE_NAME};
| ^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(unused_imports)]` on by default
warning: unused import: `DynStore`
--> tests/common/mod.rs:35:28
|
35 | Builder, CustomTlvRecord, DynStore, Event, LightningBalance, Node, NodeError,
| ^^^^^^^^
error[E0308]: arguments to this function are incorrect
--> tests/benchmarks.rs:185:15
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^^^^^^^^
|
note: expected `Arc<Node>`, found `Arc<Arc<Node>>`
--> tests/benchmarks.rs:185:29
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^
= note: expected struct `Arc<ldk_node::Node>`
found struct `Arc<Arc<ldk_node::Node>>`
note: expected `Arc<Node>`, found `Arc<Arc<Node>>`
--> tests/benchmarks.rs:185:37
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^
= note: expected struct `Arc<ldk_node::Node>`
found struct `Arc<Arc<ldk_node::Node>>`
note: function defined here
--> tests/benchmarks.rs:63:10
|
63 | async fn send_payments(node_a: Arc<Node>, node_b: Arc<Node>) -> std::time::Duration {
| ^^^^^^^^^^^^^ ----------------- -----------------

To enable more realistic testing with sqlite as a backend.
@joostjager

Copy link
Copy Markdown
ContributorAuthor

Had to move the code to the standard benches folder, which is better anyway.

@joostjager
joostjager requested a review from tnullNovember 3, 2025 08:43

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, please squash.

Introduces a criterion-based benchmark that sends 1000 concurrent
payments between two LDK nodes to measure total duration. Also adds a
CI job to automatically run the benchmark.
@tnull
tnull merged commit 22ae4d2 into lightningdevkit:mainNov 3, 2025
17 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@joostjager@ldk-reviews-bot@tnull
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Payment benchmark by joostjager · Pull Request #664 · lightningdevkit/ldk-node · GitHub
Skip to content

Payment benchmark - #664

Merged
tnull merged 2 commits into
lightningdevkit:mainfrom
joostjager:payment-benchmark
Nov 3, 2025
Merged

Payment benchmark#664
tnull merged 2 commits into
lightningdevkit:mainfrom
joostjager:payment-benchmark

Conversation

@joostjager

@joostjagerjoostjager commented Oct 15, 2025

Copy link
Copy Markdown
Contributor

This PR adds a new async payment benchmark test to integration_tests_rust.rs. The test sets up two nodes with a channel and measures performance while sending 1,000 concurrent payments. It uses Tokio’s multi-threaded runtime to spawn payments as separate async tasks and tracks completion through LDK events. The benchmark provides a simple way to gauge throughput and identify performance bottlenecks in payment handling.

Related: lightningdevkit/rust-lightning#4161

@ldk-reviews-bot

ldk-reviews-bot commented Oct 15, 2025

Copy link
Copy Markdown

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@joostjager

Copy link
Copy Markdown
ContributorAuthor

Converted test to criterion benchmark and added it to CI. Also removed the payment throttling, now that it has been established that there is a bug in lightning-net-tokio that causes this. For the benchmark, the 1000 payments can now simply be launched at the same time.

Criterion reports outliers, so this should be able to detect to occassional 30 sec hangs in the payments test.

@joostjager
joostjager marked this pull request as ready for review October 21, 2025 13:04
@joostjager
joostjager requested a review from tnullOctober 21, 2025 13:04
Comment threadCargo.toml Outdated
Comment threadbenches/payments.rs
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 3rd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@joostjager
joostjager removed the request for review from tnullOctober 28, 2025 09:33
@joostjager

Copy link
Copy Markdown
ContributorAuthor

Rebased onto async test framework changes.

@tnull

Copy link
Copy Markdown
Collaborator

Mind rebasing once more to fix CI?

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! The test store changes don't make sense to me, I believe you intended something else there. Otherwise some comments.

Comment threadtests/common/mod.rs Outdated
let node = builder.build_with_store(test_sync_store).unwrap();
let kv_store: Arc<DynStore> = match config.store_type {
TestStoreType::InMemory => {
Arc::new(TestSyncStore::new(config.node_config.storage_dir_path.into()))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This doesn't do what you think it does. This is TestSyncStore, a store that writes to multiple backends to ensure they are kept in-sync, not the in-memory TestStore.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, I missed that indeed. Renamed InMemory to TestSyncStore. The intention of the commit is to allow a test/benchmark to run with sqlite (only).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment thread.github/workflows/rust.yml Outdated
Comment threadCargo.toml Outdated
Comment threadbenches/payments.rs
Comment threadbenches/payments.rs

// Spawn each payment as a separate async task
task::spawn(async move {
println!("{}: Starting payment", payment_hash.0.as_hex());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are we positive all the printing IO doesn't impact the performance? Should we omit this for 'production' benchmarking?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I did wonder the same. Maybe I should indeed remove it to be sure. Would that be a cfg flag?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could be, but do we really need them in the first place?

Comment threadbenches/payments.rs
Comment threadbenches/payments.rs
// Pre-check the HTLC slots to try to avoid the performance impact of a failed payment.
while node_a.list_channels()[0].next_outbound_htlc_limit_msat == 0 {
println!("{}: Waiting for HTLC slots to free up", payment_hash.0.as_hex());
tokio::time::sleep(std::time::Duration::from_millis(100)).await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How did you determine 100ms here? Do we have an intuition if this could starve any of the waiting payers?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It isn't great indeed. Basically something that looked 'reasonable' to me. I think it is a weakness in the API that a user even needs to do this.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment threadtests/common/mod.rs Outdated
Arc::new(TestSyncStore::new(config.node_config.storage_dir_path.into()))
},
TestStoreType::Sqlite => Arc::new(
SqliteStore::new(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's just use the default build() here, which defaults to SQLite.

If you prefer to have it explicit, feel free to rename build to build_with_sqlite_store and have a new build method on Builder call it. Then you can explicitly set build_with_sqlite_store here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No, it's alright to use build. Done.

Comment threadbenches/payments.rs
// Pre-check the HTLC slots to try to avoid the performance impact of a failed payment.
while node_a.list_channels()[0].next_outbound_htlc_limit_msat == 0 {
println!("{}: Waiting for HTLC slots to free up", payment_hash.0.as_hex());
tokio::time::sleep(std::time::Duration::from_millis(100)).await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment threadCargo.toml Outdated

vss-client = "0.3"
prost = { version = "0.11.6", default-features = false}
criterion = { version = "0.7.0", features = ["async_tokio"] }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this not a dev-dependency?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed

@joostjager
joostjagerforce-pushed the payment-benchmark branch 2 times, most recently from b9fc54d to c231f9dCompareOctober 30, 2025 14:02
@joostjager
joostjager requested a review from tnullOctober 30, 2025 14:03

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Feel free to squash fixups. CI is currently broken with warnings and errors:

warning: unused imports: `KV_TABLE_NAME` and `SQLITE_DB_FILE_NAME`
--> tests/common/mod.rs:32:47
|
32 | use ldk_node::io::sqlite_store::{SqliteStore, KV_TABLE_NAME, SQLITE_DB_FILE_NAME};
| ^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(unused_imports)]` on by default
warning: unused import: `DynStore`
--> tests/common/mod.rs:35:28
|
35 | Builder, CustomTlvRecord, DynStore, Event, LightningBalance, Node, NodeError,
| ^^^^^^^^
error[E0308]: arguments to this function are incorrect
--> tests/benchmarks.rs:185:15
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^^^^^^^^
|
note: expected `Arc<Node>`, found `Arc<Arc<Node>>`
--> tests/benchmarks.rs:185:29
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^
= note: expected struct `Arc<ldk_node::Node>`
found struct `Arc<Arc<ldk_node::Node>>`
note: expected `Arc<Node>`, found `Arc<Arc<Node>>`
--> tests/benchmarks.rs:185:37
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^
= note: expected struct `Arc<ldk_node::Node>`
found struct `Arc<Arc<ldk_node::Node>>`
note: function defined here
--> tests/benchmarks.rs:63:10
|
63 | async fn send_payments(node_a: Arc<Node>, node_b: Arc<Node>) -> std::time::Duration {
| ^^^^^^^^^^^^^ ----------------- -----------------

To enable more realistic testing with sqlite as a backend.
@joostjager

Copy link
Copy Markdown
ContributorAuthor

Had to move the code to the standard benches folder, which is better anyway.

@joostjager
joostjager requested a review from tnullNovember 3, 2025 08:43

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, please squash.

Introduces a criterion-based benchmark that sends 1000 concurrent
payments between two LDK nodes to measure total duration. Also adds a
CI job to automatically run the benchmark.
@tnull
tnull merged commit 22ae4d2 into lightningdevkit:mainNov 3, 2025
17 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@joostjager@ldk-reviews-bot@tnull
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Payment benchmark by joostjager · Pull Request #664 · lightningdevkit/ldk-node · GitHub
Skip to content

Payment benchmark - #664

Merged
tnull merged 2 commits into
lightningdevkit:mainfrom
joostjager:payment-benchmark
Nov 3, 2025
Merged

Payment benchmark#664
tnull merged 2 commits into
lightningdevkit:mainfrom
joostjager:payment-benchmark

Conversation

@joostjager

@joostjagerjoostjager commented Oct 15, 2025

Copy link
Copy Markdown
Contributor

This PR adds a new async payment benchmark test to integration_tests_rust.rs. The test sets up two nodes with a channel and measures performance while sending 1,000 concurrent payments. It uses Tokio’s multi-threaded runtime to spawn payments as separate async tasks and tracks completion through LDK events. The benchmark provides a simple way to gauge throughput and identify performance bottlenecks in payment handling.

Related: lightningdevkit/rust-lightning#4161

@ldk-reviews-bot

ldk-reviews-bot commented Oct 15, 2025

Copy link
Copy Markdown

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@joostjager

Copy link
Copy Markdown
ContributorAuthor

Converted test to criterion benchmark and added it to CI. Also removed the payment throttling, now that it has been established that there is a bug in lightning-net-tokio that causes this. For the benchmark, the 1000 payments can now simply be launched at the same time.

Criterion reports outliers, so this should be able to detect to occassional 30 sec hangs in the payments test.

@joostjager
joostjager marked this pull request as ready for review October 21, 2025 13:04
@joostjager
joostjager requested a review from tnullOctober 21, 2025 13:04
Comment threadCargo.toml Outdated
Comment threadbenches/payments.rs
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 3rd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@joostjager
joostjager removed the request for review from tnullOctober 28, 2025 09:33
@joostjager

Copy link
Copy Markdown
ContributorAuthor

Rebased onto async test framework changes.

@tnull

Copy link
Copy Markdown
Collaborator

Mind rebasing once more to fix CI?

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! The test store changes don't make sense to me, I believe you intended something else there. Otherwise some comments.

Comment threadtests/common/mod.rs Outdated
let node = builder.build_with_store(test_sync_store).unwrap();
let kv_store: Arc<DynStore> = match config.store_type {
TestStoreType::InMemory => {
Arc::new(TestSyncStore::new(config.node_config.storage_dir_path.into()))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This doesn't do what you think it does. This is TestSyncStore, a store that writes to multiple backends to ensure they are kept in-sync, not the in-memory TestStore.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, I missed that indeed. Renamed InMemory to TestSyncStore. The intention of the commit is to allow a test/benchmark to run with sqlite (only).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment thread.github/workflows/rust.yml Outdated
Comment threadCargo.toml Outdated
Comment threadbenches/payments.rs
Comment threadbenches/payments.rs

// Spawn each payment as a separate async task
task::spawn(async move {
println!("{}: Starting payment", payment_hash.0.as_hex());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are we positive all the printing IO doesn't impact the performance? Should we omit this for 'production' benchmarking?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I did wonder the same. Maybe I should indeed remove it to be sure. Would that be a cfg flag?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could be, but do we really need them in the first place?

Comment threadbenches/payments.rs
Comment threadbenches/payments.rs
// Pre-check the HTLC slots to try to avoid the performance impact of a failed payment.
while node_a.list_channels()[0].next_outbound_htlc_limit_msat == 0 {
println!("{}: Waiting for HTLC slots to free up", payment_hash.0.as_hex());
tokio::time::sleep(std::time::Duration::from_millis(100)).await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How did you determine 100ms here? Do we have an intuition if this could starve any of the waiting payers?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It isn't great indeed. Basically something that looked 'reasonable' to me. I think it is a weakness in the API that a user even needs to do this.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment threadtests/common/mod.rs Outdated
Arc::new(TestSyncStore::new(config.node_config.storage_dir_path.into()))
},
TestStoreType::Sqlite => Arc::new(
SqliteStore::new(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's just use the default build() here, which defaults to SQLite.

If you prefer to have it explicit, feel free to rename build to build_with_sqlite_store and have a new build method on Builder call it. Then you can explicitly set build_with_sqlite_store here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No, it's alright to use build. Done.

Comment threadbenches/payments.rs
// Pre-check the HTLC slots to try to avoid the performance impact of a failed payment.
while node_a.list_channels()[0].next_outbound_htlc_limit_msat == 0 {
println!("{}: Waiting for HTLC slots to free up", payment_hash.0.as_hex());
tokio::time::sleep(std::time::Duration::from_millis(100)).await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment threadCargo.toml Outdated

vss-client = "0.3"
prost = { version = "0.11.6", default-features = false}
criterion = { version = "0.7.0", features = ["async_tokio"] }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this not a dev-dependency?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed

@joostjager
joostjagerforce-pushed the payment-benchmark branch 2 times, most recently from b9fc54d to c231f9dCompareOctober 30, 2025 14:02
@joostjager
joostjager requested a review from tnullOctober 30, 2025 14:03

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Feel free to squash fixups. CI is currently broken with warnings and errors:

warning: unused imports: `KV_TABLE_NAME` and `SQLITE_DB_FILE_NAME`
--> tests/common/mod.rs:32:47
|
32 | use ldk_node::io::sqlite_store::{SqliteStore, KV_TABLE_NAME, SQLITE_DB_FILE_NAME};
| ^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(unused_imports)]` on by default
warning: unused import: `DynStore`
--> tests/common/mod.rs:35:28
|
35 | Builder, CustomTlvRecord, DynStore, Event, LightningBalance, Node, NodeError,
| ^^^^^^^^
error[E0308]: arguments to this function are incorrect
--> tests/benchmarks.rs:185:15
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^^^^^^^^
|
note: expected `Arc<Node>`, found `Arc<Arc<Node>>`
--> tests/benchmarks.rs:185:29
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^
= note: expected struct `Arc<ldk_node::Node>`
found struct `Arc<Arc<ldk_node::Node>>`
note: expected `Arc<Node>`, found `Arc<Arc<Node>>`
--> tests/benchmarks.rs:185:37
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^
= note: expected struct `Arc<ldk_node::Node>`
found struct `Arc<Arc<ldk_node::Node>>`
note: function defined here
--> tests/benchmarks.rs:63:10
|
63 | async fn send_payments(node_a: Arc<Node>, node_b: Arc<Node>) -> std::time::Duration {
| ^^^^^^^^^^^^^ ----------------- -----------------

To enable more realistic testing with sqlite as a backend.
@joostjager

Copy link
Copy Markdown
ContributorAuthor

Had to move the code to the standard benches folder, which is better anyway.

@joostjager
joostjager requested a review from tnullNovember 3, 2025 08:43

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, please squash.

Introduces a criterion-based benchmark that sends 1000 concurrent
payments between two LDK nodes to measure total duration. Also adds a
CI job to automatically run the benchmark.
@tnull
tnull merged commit 22ae4d2 into lightningdevkit:mainNov 3, 2025
17 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@joostjager@ldk-reviews-bot@tnull
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Payment benchmark by joostjager · Pull Request #664 · lightningdevkit/ldk-node · GitHub
Skip to content

Payment benchmark - #664

Merged
tnull merged 2 commits into
lightningdevkit:mainfrom
joostjager:payment-benchmark
Nov 3, 2025
Merged

Payment benchmark#664
tnull merged 2 commits into
lightningdevkit:mainfrom
joostjager:payment-benchmark

Conversation

@joostjager

@joostjagerjoostjager commented Oct 15, 2025

Copy link
Copy Markdown
Contributor

This PR adds a new async payment benchmark test to integration_tests_rust.rs. The test sets up two nodes with a channel and measures performance while sending 1,000 concurrent payments. It uses Tokio’s multi-threaded runtime to spawn payments as separate async tasks and tracks completion through LDK events. The benchmark provides a simple way to gauge throughput and identify performance bottlenecks in payment handling.

Related: lightningdevkit/rust-lightning#4161

@ldk-reviews-bot

ldk-reviews-bot commented Oct 15, 2025

Copy link
Copy Markdown

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@joostjager

Copy link
Copy Markdown
ContributorAuthor

Converted test to criterion benchmark and added it to CI. Also removed the payment throttling, now that it has been established that there is a bug in lightning-net-tokio that causes this. For the benchmark, the 1000 payments can now simply be launched at the same time.

Criterion reports outliers, so this should be able to detect to occassional 30 sec hangs in the payments test.

@joostjager
joostjager marked this pull request as ready for review October 21, 2025 13:04
@joostjager
joostjager requested a review from tnullOctober 21, 2025 13:04
Comment threadCargo.toml Outdated
Comment threadbenches/payments.rs
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 3rd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@joostjager
joostjager removed the request for review from tnullOctober 28, 2025 09:33
@joostjager

Copy link
Copy Markdown
ContributorAuthor

Rebased onto async test framework changes.

@tnull

Copy link
Copy Markdown
Collaborator

Mind rebasing once more to fix CI?

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! The test store changes don't make sense to me, I believe you intended something else there. Otherwise some comments.

Comment threadtests/common/mod.rs Outdated
let node = builder.build_with_store(test_sync_store).unwrap();
let kv_store: Arc<DynStore> = match config.store_type {
TestStoreType::InMemory => {
Arc::new(TestSyncStore::new(config.node_config.storage_dir_path.into()))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This doesn't do what you think it does. This is TestSyncStore, a store that writes to multiple backends to ensure they are kept in-sync, not the in-memory TestStore.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, I missed that indeed. Renamed InMemory to TestSyncStore. The intention of the commit is to allow a test/benchmark to run with sqlite (only).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment thread.github/workflows/rust.yml Outdated
Comment threadCargo.toml Outdated
Comment threadbenches/payments.rs
Comment threadbenches/payments.rs

// Spawn each payment as a separate async task
task::spawn(async move {
println!("{}: Starting payment", payment_hash.0.as_hex());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are we positive all the printing IO doesn't impact the performance? Should we omit this for 'production' benchmarking?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I did wonder the same. Maybe I should indeed remove it to be sure. Would that be a cfg flag?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could be, but do we really need them in the first place?

Comment threadbenches/payments.rs
Comment threadbenches/payments.rs
// Pre-check the HTLC slots to try to avoid the performance impact of a failed payment.
while node_a.list_channels()[0].next_outbound_htlc_limit_msat == 0 {
println!("{}: Waiting for HTLC slots to free up", payment_hash.0.as_hex());
tokio::time::sleep(std::time::Duration::from_millis(100)).await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How did you determine 100ms here? Do we have an intuition if this could starve any of the waiting payers?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It isn't great indeed. Basically something that looked 'reasonable' to me. I think it is a weakness in the API that a user even needs to do this.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment threadtests/common/mod.rs Outdated
Arc::new(TestSyncStore::new(config.node_config.storage_dir_path.into()))
},
TestStoreType::Sqlite => Arc::new(
SqliteStore::new(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's just use the default build() here, which defaults to SQLite.

If you prefer to have it explicit, feel free to rename build to build_with_sqlite_store and have a new build method on Builder call it. Then you can explicitly set build_with_sqlite_store here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No, it's alright to use build. Done.

Comment threadbenches/payments.rs
// Pre-check the HTLC slots to try to avoid the performance impact of a failed payment.
while node_a.list_channels()[0].next_outbound_htlc_limit_msat == 0 {
println!("{}: Waiting for HTLC slots to free up", payment_hash.0.as_hex());
tokio::time::sleep(std::time::Duration::from_millis(100)).await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment threadCargo.toml Outdated

vss-client = "0.3"
prost = { version = "0.11.6", default-features = false}
criterion = { version = "0.7.0", features = ["async_tokio"] }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this not a dev-dependency?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed

@joostjager
joostjagerforce-pushed the payment-benchmark branch 2 times, most recently from b9fc54d to c231f9dCompareOctober 30, 2025 14:02
@joostjager
joostjager requested a review from tnullOctober 30, 2025 14:03

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Feel free to squash fixups. CI is currently broken with warnings and errors:

warning: unused imports: `KV_TABLE_NAME` and `SQLITE_DB_FILE_NAME`
--> tests/common/mod.rs:32:47
|
32 | use ldk_node::io::sqlite_store::{SqliteStore, KV_TABLE_NAME, SQLITE_DB_FILE_NAME};
| ^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(unused_imports)]` on by default
warning: unused import: `DynStore`
--> tests/common/mod.rs:35:28
|
35 | Builder, CustomTlvRecord, DynStore, Event, LightningBalance, Node, NodeError,
| ^^^^^^^^
error[E0308]: arguments to this function are incorrect
--> tests/benchmarks.rs:185:15
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^^^^^^^^
|
note: expected `Arc<Node>`, found `Arc<Arc<Node>>`
--> tests/benchmarks.rs:185:29
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^
= note: expected struct `Arc<ldk_node::Node>`
found struct `Arc<Arc<ldk_node::Node>>`
note: expected `Arc<Node>`, found `Arc<Arc<Node>>`
--> tests/benchmarks.rs:185:37
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^
= note: expected struct `Arc<ldk_node::Node>`
found struct `Arc<Arc<ldk_node::Node>>`
note: function defined here
--> tests/benchmarks.rs:63:10
|
63 | async fn send_payments(node_a: Arc<Node>, node_b: Arc<Node>) -> std::time::Duration {
| ^^^^^^^^^^^^^ ----------------- -----------------

To enable more realistic testing with sqlite as a backend.
@joostjager

Copy link
Copy Markdown
ContributorAuthor

Had to move the code to the standard benches folder, which is better anyway.

@joostjager
joostjager requested a review from tnullNovember 3, 2025 08:43

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, please squash.

Introduces a criterion-based benchmark that sends 1000 concurrent
payments between two LDK nodes to measure total duration. Also adds a
CI job to automatically run the benchmark.
@tnull
tnull merged commit 22ae4d2 into lightningdevkit:mainNov 3, 2025
17 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@joostjager@ldk-reviews-bot@tnull
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); Payment benchmark by joostjager · Pull Request #664 · lightningdevkit/ldk-node · GitHub
Skip to content

Payment benchmark - #664

Merged
tnull merged 2 commits into
lightningdevkit:mainfrom
joostjager:payment-benchmark
Nov 3, 2025
Merged

Payment benchmark#664
tnull merged 2 commits into
lightningdevkit:mainfrom
joostjager:payment-benchmark

Conversation

@joostjager

@joostjagerjoostjager commented Oct 15, 2025

Copy link
Copy Markdown
Contributor

This PR adds a new async payment benchmark test to integration_tests_rust.rs. The test sets up two nodes with a channel and measures performance while sending 1,000 concurrent payments. It uses Tokio’s multi-threaded runtime to spawn payments as separate async tasks and tracks completion through LDK events. The benchmark provides a simple way to gauge throughput and identify performance bottlenecks in payment handling.

Related: lightningdevkit/rust-lightning#4161

@ldk-reviews-bot

ldk-reviews-bot commented Oct 15, 2025

Copy link
Copy Markdown

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@joostjager

Copy link
Copy Markdown
ContributorAuthor

Converted test to criterion benchmark and added it to CI. Also removed the payment throttling, now that it has been established that there is a bug in lightning-net-tokio that causes this. For the benchmark, the 1000 payments can now simply be launched at the same time.

Criterion reports outliers, so this should be able to detect to occassional 30 sec hangs in the payments test.

@joostjager
joostjager marked this pull request as ready for review October 21, 2025 13:04
@joostjager
joostjager requested a review from tnullOctober 21, 2025 13:04
Comment threadCargo.toml Outdated
Comment threadbenches/payments.rs
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 3rd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@joostjager
joostjager removed the request for review from tnullOctober 28, 2025 09:33
@joostjager

Copy link
Copy Markdown
ContributorAuthor

Rebased onto async test framework changes.

@tnull

Copy link
Copy Markdown
Collaborator

Mind rebasing once more to fix CI?

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! The test store changes don't make sense to me, I believe you intended something else there. Otherwise some comments.

Comment threadtests/common/mod.rs Outdated
let node = builder.build_with_store(test_sync_store).unwrap();
let kv_store: Arc<DynStore> = match config.store_type {
TestStoreType::InMemory => {
Arc::new(TestSyncStore::new(config.node_config.storage_dir_path.into()))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This doesn't do what you think it does. This is TestSyncStore, a store that writes to multiple backends to ensure they are kept in-sync, not the in-memory TestStore.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, I missed that indeed. Renamed InMemory to TestSyncStore. The intention of the commit is to allow a test/benchmark to run with sqlite (only).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment thread.github/workflows/rust.yml Outdated
Comment threadCargo.toml Outdated
Comment threadbenches/payments.rs
Comment threadbenches/payments.rs

// Spawn each payment as a separate async task
task::spawn(async move {
println!("{}: Starting payment", payment_hash.0.as_hex());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are we positive all the printing IO doesn't impact the performance? Should we omit this for 'production' benchmarking?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I did wonder the same. Maybe I should indeed remove it to be sure. Would that be a cfg flag?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could be, but do we really need them in the first place?

Comment threadbenches/payments.rs
Comment threadbenches/payments.rs
// Pre-check the HTLC slots to try to avoid the performance impact of a failed payment.
while node_a.list_channels()[0].next_outbound_htlc_limit_msat == 0 {
println!("{}: Waiting for HTLC slots to free up", payment_hash.0.as_hex());
tokio::time::sleep(std::time::Duration::from_millis(100)).await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How did you determine 100ms here? Do we have an intuition if this could starve any of the waiting payers?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It isn't great indeed. Basically something that looked 'reasonable' to me. I think it is a weakness in the API that a user even needs to do this.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment threadtests/common/mod.rs Outdated
Arc::new(TestSyncStore::new(config.node_config.storage_dir_path.into()))
},
TestStoreType::Sqlite => Arc::new(
SqliteStore::new(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's just use the default build() here, which defaults to SQLite.

If you prefer to have it explicit, feel free to rename build to build_with_sqlite_store and have a new build method on Builder call it. Then you can explicitly set build_with_sqlite_store here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No, it's alright to use build. Done.

Comment threadbenches/payments.rs
// Pre-check the HTLC slots to try to avoid the performance impact of a failed payment.
while node_a.list_channels()[0].next_outbound_htlc_limit_msat == 0 {
println!("{}: Waiting for HTLC slots to free up", payment_hash.0.as_hex());
tokio::time::sleep(std::time::Duration::from_millis(100)).await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay.

Comment threadCargo.toml Outdated

vss-client = "0.3"
prost = { version = "0.11.6", default-features = false}
criterion = { version = "0.7.0", features = ["async_tokio"] }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this not a dev-dependency?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed

@joostjager
joostjagerforce-pushed the payment-benchmark branch 2 times, most recently from b9fc54d to c231f9dCompareOctober 30, 2025 14:02
@joostjager
joostjager requested a review from tnullOctober 30, 2025 14:03

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Feel free to squash fixups. CI is currently broken with warnings and errors:

warning: unused imports: `KV_TABLE_NAME` and `SQLITE_DB_FILE_NAME`
--> tests/common/mod.rs:32:47
|
32 | use ldk_node::io::sqlite_store::{SqliteStore, KV_TABLE_NAME, SQLITE_DB_FILE_NAME};
| ^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(unused_imports)]` on by default
warning: unused import: `DynStore`
--> tests/common/mod.rs:35:28
|
35 | Builder, CustomTlvRecord, DynStore, Event, LightningBalance, Node, NodeError,
| ^^^^^^^^
error[E0308]: arguments to this function are incorrect
--> tests/benchmarks.rs:185:15
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^^^^^^^^
|
note: expected `Arc<Node>`, found `Arc<Arc<Node>>`
--> tests/benchmarks.rs:185:29
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^
= note: expected struct `Arc<ldk_node::Node>`
found struct `Arc<Arc<ldk_node::Node>>`
note: expected `Arc<Node>`, found `Arc<Arc<Node>>`
--> tests/benchmarks.rs:185:37
|
185 | total += send_payments(node_a, node_b).await;
| ^^^^^^
= note: expected struct `Arc<ldk_node::Node>`
found struct `Arc<Arc<ldk_node::Node>>`
note: function defined here
--> tests/benchmarks.rs:63:10
|
63 | async fn send_payments(node_a: Arc<Node>, node_b: Arc<Node>) -> std::time::Duration {
| ^^^^^^^^^^^^^ ----------------- -----------------

To enable more realistic testing with sqlite as a backend.
@joostjager

Copy link
Copy Markdown
ContributorAuthor

Had to move the code to the standard benches folder, which is better anyway.

@joostjager
joostjager requested a review from tnullNovember 3, 2025 08:43

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, please squash.

Introduces a criterion-based benchmark that sends 1000 concurrent
payments between two LDK nodes to measure total duration. Also adds a
CI job to automatically run the benchmark.
@tnull
tnull merged commit 22ae4d2 into lightningdevkit:mainNov 3, 2025
17 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@joostjager@ldk-reviews-bot@tnull