Skip to content

Asyncify test suite and reintroduce VSS-internal runtime - #670

Merged
tnull merged 7 commits into
lightningdevkit:mainfrom
tnull:2025-10-asyncify-more-things
Oct 28, 2025
Merged

Asyncify test suite and reintroduce VSS-internal runtime#670
tnull merged 7 commits into
lightningdevkit:mainfrom
tnull:2025-10-asyncify-more-things

Conversation

@tnull

@tnulltnull commented Oct 17, 2025

Copy link
Copy Markdown
Collaborator

As we're looking to move to a more async-friendly API over time (which would allow us to drop the dependency on the KVStoreSync implementation entirely), we here make a few prefactors and minor improvements that move in that direction.

Most notably, we switch to have our tests run in async contexts, which also should make them more efficient as not every node will spawn its own runtime, but rather reuse the one runtime spawned for each test case.

Moreover, we reintroduce a secondary runtime internal to VssStore in order to fix the blocking issue we recently hit in our CI. Additionally, we found that not all tests cfg-gated under vss_test were being run, so we fix these test cases and have them run in CI.

.. as we're about to `cargo test` anyways.
.. some of the `ReadFailed` cases didn't log why they failed. Here we
fix that oversight.
@tnull
tnull requested a review from joostjagerOctober 17, 2025 11:08
@ldk-reviews-bot

ldk-reviews-bot commented Oct 17, 2025

Copy link
Copy Markdown

👋 Thanks for assigning @joostjager 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.

@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 3 times, most recently from f817f78 to 141f186CompareOctober 17, 2025 11:21
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @joostjager! 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.

Comment threadsrc/lib.rs
Comment threadsrc/event.rs
Comment threadsrc/event.rs
Comment threadtests/integration_tests_cln.rs
Comment threadtests/reorg_test.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from e6e2073 to 584fff6CompareOctober 21, 2025 15:11
@tnulltnull changed the title Asyncify test suite and event queueAsyncify test suite and reintroduce VSS-internal runtimeOct 21, 2025
@tnull
tnull requested a review from joostjagerOctober 21, 2025 15:17
@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Now added changes reintroducing a secondary runtime internal to VssStore in order to fix the blocking issue we recently hit in our CI. Additionally, we found that not all tests cfg-gated under vss_test were being run, so we fix these test cases and have them run in CI.

@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 7 times, most recently from 6d3a774 to eedfb12CompareOctober 21, 2025 21:31

@joostjagerjoostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR could have been split up in independent changes, but perhaps too late now.

The whole rust async situation is so painful. It really seems to get to the point - if it wasn't already there - of being a true velocity killer.

Comment threadsrc/lib.rs
Comment threadsrc/event.rs
Comment threadtests/integration_tests_cln.rs
Comment threadsrc/runtime.rs
Comment threadsrc/lib.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from eedfb12 to dba502dCompareOctober 27, 2025 12:52

@tnulltnull left a comment

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Addressed remaining comments.

Comment threadsrc/lib.rs
Comment threadtests/integration_tests_cln.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/runtime.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 2 times, most recently from 300e9bb to 4622fb1CompareOctober 27, 2025 13:08
@tnull
tnull requested a review from joostjagerOctober 27, 2025 15:02
@tnull

Copy link
Copy Markdown
CollaboratorAuthor

CI failure is unrelated: #679

@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Addressed pending feedback.

@joostjager let me know if I can squash.

Given we regularly run into issues arising from mixing sync and async
contexts, we here simplify our `EventQueue` implementation by avoiding
to use `Condvar::wait_while` (which parks the current thread) and rather
simply us `block_on` on our `next_event_async` method.
.. as LDK Node is moving towards a more `async` core, it starts to make
sense to switch our test suite over to be `async`.
This change should make our CI more efficient (as not every node will
spawn its independent runtime, but we just have one runtime per test
created) and also makes sure we won't run into any edge cases arising
from blocking test threads that are executing other async tasks.
Since it seems to make a difference to `tokio` (see
https://docs.rs/tokio/latest/tokio/time/fn.timeout.html#panics) we make
sure the futures are always put in an `async` closure.
In order to avoid the recently, discovered blocking-task-deadlock (in
which the task holding the runtime reactor got blocked and hence
stopped polling VSS write tasks), we where re-introduce an internal
runtime to the `VssStore`, on which we spawn the tasks, while still
using `block_on` of our regular runtime for async-sync conversions.
This also finally fixes our VSS CI.
.. we previously avoided running some tests which turned out to be
broken.
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from e4f3ba0 to 8cad63bCompareOctober 28, 2025 09:23

@joostjagerjoostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remaining questions answered offline and summarized in threads. LGTM

@tnull
tnull merged commit d984aca into lightningdevkit:mainOct 28, 2025
6 of 15 checks passed
@tnulltnull mentioned this pull request Oct 28, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tnull@ldk-reviews-bot@joostjager
, '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" + '
Asyncify test suite and reintroduce VSS-internal runtime by tnull · Pull Request #670 · lightningdevkit/ldk-node · GitHub
Skip to content

Asyncify test suite and reintroduce VSS-internal runtime - #670

Merged
tnull merged 7 commits into
lightningdevkit:mainfrom
tnull:2025-10-asyncify-more-things
Oct 28, 2025
Merged

Asyncify test suite and reintroduce VSS-internal runtime#670
tnull merged 7 commits into
lightningdevkit:mainfrom
tnull:2025-10-asyncify-more-things

Conversation

@tnull

@tnulltnull commented Oct 17, 2025

Copy link
Copy Markdown
Collaborator

As we're looking to move to a more async-friendly API over time (which would allow us to drop the dependency on the KVStoreSync implementation entirely), we here make a few prefactors and minor improvements that move in that direction.

Most notably, we switch to have our tests run in async contexts, which also should make them more efficient as not every node will spawn its own runtime, but rather reuse the one runtime spawned for each test case.

Moreover, we reintroduce a secondary runtime internal to VssStore in order to fix the blocking issue we recently hit in our CI. Additionally, we found that not all tests cfg-gated under vss_test were being run, so we fix these test cases and have them run in CI.

.. as we're about to `cargo test` anyways.
.. some of the `ReadFailed` cases didn't log why they failed. Here we
fix that oversight.
@tnull
tnull requested a review from joostjagerOctober 17, 2025 11:08
@ldk-reviews-bot

ldk-reviews-bot commented Oct 17, 2025

Copy link
Copy Markdown

👋 Thanks for assigning @joostjager 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.

@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 3 times, most recently from f817f78 to 141f186CompareOctober 17, 2025 11:21
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @joostjager! 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.

Comment threadsrc/lib.rs
Comment threadsrc/event.rs
Comment threadsrc/event.rs
Comment threadtests/integration_tests_cln.rs
Comment threadtests/reorg_test.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from e6e2073 to 584fff6CompareOctober 21, 2025 15:11
@tnulltnull changed the title Asyncify test suite and event queueAsyncify test suite and reintroduce VSS-internal runtimeOct 21, 2025
@tnull
tnull requested a review from joostjagerOctober 21, 2025 15:17
@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Now added changes reintroducing a secondary runtime internal to VssStore in order to fix the blocking issue we recently hit in our CI. Additionally, we found that not all tests cfg-gated under vss_test were being run, so we fix these test cases and have them run in CI.

@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 7 times, most recently from 6d3a774 to eedfb12CompareOctober 21, 2025 21:31

@joostjagerjoostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR could have been split up in independent changes, but perhaps too late now.

The whole rust async situation is so painful. It really seems to get to the point - if it wasn't already there - of being a true velocity killer.

Comment threadsrc/lib.rs
Comment threadsrc/event.rs
Comment threadtests/integration_tests_cln.rs
Comment threadsrc/runtime.rs
Comment threadsrc/lib.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from eedfb12 to dba502dCompareOctober 27, 2025 12:52

@tnulltnull left a comment

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Addressed remaining comments.

Comment threadsrc/lib.rs
Comment threadtests/integration_tests_cln.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/runtime.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 2 times, most recently from 300e9bb to 4622fb1CompareOctober 27, 2025 13:08
@tnull
tnull requested a review from joostjagerOctober 27, 2025 15:02
@tnull

Copy link
Copy Markdown
CollaboratorAuthor

CI failure is unrelated: #679

@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Addressed pending feedback.

@joostjager let me know if I can squash.

Given we regularly run into issues arising from mixing sync and async
contexts, we here simplify our `EventQueue` implementation by avoiding
to use `Condvar::wait_while` (which parks the current thread) and rather
simply us `block_on` on our `next_event_async` method.
.. as LDK Node is moving towards a more `async` core, it starts to make
sense to switch our test suite over to be `async`.
This change should make our CI more efficient (as not every node will
spawn its independent runtime, but we just have one runtime per test
created) and also makes sure we won't run into any edge cases arising
from blocking test threads that are executing other async tasks.
Since it seems to make a difference to `tokio` (see
https://docs.rs/tokio/latest/tokio/time/fn.timeout.html#panics) we make
sure the futures are always put in an `async` closure.
In order to avoid the recently, discovered blocking-task-deadlock (in
which the task holding the runtime reactor got blocked and hence
stopped polling VSS write tasks), we where re-introduce an internal
runtime to the `VssStore`, on which we spawn the tasks, while still
using `block_on` of our regular runtime for async-sync conversions.
This also finally fixes our VSS CI.
.. we previously avoided running some tests which turned out to be
broken.
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from e4f3ba0 to 8cad63bCompareOctober 28, 2025 09:23

@joostjagerjoostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remaining questions answered offline and summarized in threads. LGTM

@tnull
tnull merged commit d984aca into lightningdevkit:mainOct 28, 2025
6 of 15 checks passed
@tnulltnull mentioned this pull request Oct 28, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tnull@ldk-reviews-bot@joostjager
, '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('^' + ".*" + ' Asyncify test suite and reintroduce VSS-internal runtime by tnull · Pull Request #670 · lightningdevkit/ldk-node · GitHub
Skip to content

Asyncify test suite and reintroduce VSS-internal runtime - #670

Merged
tnull merged 7 commits into
lightningdevkit:mainfrom
tnull:2025-10-asyncify-more-things
Oct 28, 2025
Merged

Asyncify test suite and reintroduce VSS-internal runtime#670
tnull merged 7 commits into
lightningdevkit:mainfrom
tnull:2025-10-asyncify-more-things

Conversation

@tnull

@tnulltnull commented Oct 17, 2025

Copy link
Copy Markdown
Collaborator

As we're looking to move to a more async-friendly API over time (which would allow us to drop the dependency on the KVStoreSync implementation entirely), we here make a few prefactors and minor improvements that move in that direction.

Most notably, we switch to have our tests run in async contexts, which also should make them more efficient as not every node will spawn its own runtime, but rather reuse the one runtime spawned for each test case.

Moreover, we reintroduce a secondary runtime internal to VssStore in order to fix the blocking issue we recently hit in our CI. Additionally, we found that not all tests cfg-gated under vss_test were being run, so we fix these test cases and have them run in CI.

.. as we're about to `cargo test` anyways.
.. some of the `ReadFailed` cases didn't log why they failed. Here we
fix that oversight.
@tnull
tnull requested a review from joostjagerOctober 17, 2025 11:08
@ldk-reviews-bot

ldk-reviews-bot commented Oct 17, 2025

Copy link
Copy Markdown

👋 Thanks for assigning @joostjager 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.

@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 3 times, most recently from f817f78 to 141f186CompareOctober 17, 2025 11:21
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @joostjager! 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.

Comment threadsrc/lib.rs
Comment threadsrc/event.rs
Comment threadsrc/event.rs
Comment threadtests/integration_tests_cln.rs
Comment threadtests/reorg_test.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from e6e2073 to 584fff6CompareOctober 21, 2025 15:11
@tnulltnull changed the title Asyncify test suite and event queueAsyncify test suite and reintroduce VSS-internal runtimeOct 21, 2025
@tnull
tnull requested a review from joostjagerOctober 21, 2025 15:17
@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Now added changes reintroducing a secondary runtime internal to VssStore in order to fix the blocking issue we recently hit in our CI. Additionally, we found that not all tests cfg-gated under vss_test were being run, so we fix these test cases and have them run in CI.

@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 7 times, most recently from 6d3a774 to eedfb12CompareOctober 21, 2025 21:31

@joostjagerjoostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR could have been split up in independent changes, but perhaps too late now.

The whole rust async situation is so painful. It really seems to get to the point - if it wasn't already there - of being a true velocity killer.

Comment threadsrc/lib.rs
Comment threadsrc/event.rs
Comment threadtests/integration_tests_cln.rs
Comment threadsrc/runtime.rs
Comment threadsrc/lib.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from eedfb12 to dba502dCompareOctober 27, 2025 12:52

@tnulltnull left a comment

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Addressed remaining comments.

Comment threadsrc/lib.rs
Comment threadtests/integration_tests_cln.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/runtime.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 2 times, most recently from 300e9bb to 4622fb1CompareOctober 27, 2025 13:08
@tnull
tnull requested a review from joostjagerOctober 27, 2025 15:02
@tnull

Copy link
Copy Markdown
CollaboratorAuthor

CI failure is unrelated: #679

@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Addressed pending feedback.

@joostjager let me know if I can squash.

Given we regularly run into issues arising from mixing sync and async
contexts, we here simplify our `EventQueue` implementation by avoiding
to use `Condvar::wait_while` (which parks the current thread) and rather
simply us `block_on` on our `next_event_async` method.
.. as LDK Node is moving towards a more `async` core, it starts to make
sense to switch our test suite over to be `async`.
This change should make our CI more efficient (as not every node will
spawn its independent runtime, but we just have one runtime per test
created) and also makes sure we won't run into any edge cases arising
from blocking test threads that are executing other async tasks.
Since it seems to make a difference to `tokio` (see
https://docs.rs/tokio/latest/tokio/time/fn.timeout.html#panics) we make
sure the futures are always put in an `async` closure.
In order to avoid the recently, discovered blocking-task-deadlock (in
which the task holding the runtime reactor got blocked and hence
stopped polling VSS write tasks), we where re-introduce an internal
runtime to the `VssStore`, on which we spawn the tasks, while still
using `block_on` of our regular runtime for async-sync conversions.
This also finally fixes our VSS CI.
.. we previously avoided running some tests which turned out to be
broken.
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from e4f3ba0 to 8cad63bCompareOctober 28, 2025 09:23

@joostjagerjoostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remaining questions answered offline and summarized in threads. LGTM

@tnull
tnull merged commit d984aca into lightningdevkit:mainOct 28, 2025
6 of 15 checks passed
@tnulltnull mentioned this pull request Oct 28, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tnull@ldk-reviews-bot@joostjager
, '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('^' + ".*" + ' Asyncify test suite and reintroduce VSS-internal runtime by tnull · Pull Request #670 · lightningdevkit/ldk-node · GitHub
Skip to content

Asyncify test suite and reintroduce VSS-internal runtime - #670

Merged
tnull merged 7 commits into
lightningdevkit:mainfrom
tnull:2025-10-asyncify-more-things
Oct 28, 2025
Merged

Asyncify test suite and reintroduce VSS-internal runtime#670
tnull merged 7 commits into
lightningdevkit:mainfrom
tnull:2025-10-asyncify-more-things

Conversation

@tnull

@tnulltnull commented Oct 17, 2025

Copy link
Copy Markdown
Collaborator

As we're looking to move to a more async-friendly API over time (which would allow us to drop the dependency on the KVStoreSync implementation entirely), we here make a few prefactors and minor improvements that move in that direction.

Most notably, we switch to have our tests run in async contexts, which also should make them more efficient as not every node will spawn its own runtime, but rather reuse the one runtime spawned for each test case.

Moreover, we reintroduce a secondary runtime internal to VssStore in order to fix the blocking issue we recently hit in our CI. Additionally, we found that not all tests cfg-gated under vss_test were being run, so we fix these test cases and have them run in CI.

.. as we're about to `cargo test` anyways.
.. some of the `ReadFailed` cases didn't log why they failed. Here we
fix that oversight.
@tnull
tnull requested a review from joostjagerOctober 17, 2025 11:08
@ldk-reviews-bot

ldk-reviews-bot commented Oct 17, 2025

Copy link
Copy Markdown

👋 Thanks for assigning @joostjager 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.

@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 3 times, most recently from f817f78 to 141f186CompareOctober 17, 2025 11:21
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @joostjager! 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.

Comment threadsrc/lib.rs
Comment threadsrc/event.rs
Comment threadsrc/event.rs
Comment threadtests/integration_tests_cln.rs
Comment threadtests/reorg_test.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from e6e2073 to 584fff6CompareOctober 21, 2025 15:11
@tnulltnull changed the title Asyncify test suite and event queueAsyncify test suite and reintroduce VSS-internal runtimeOct 21, 2025
@tnull
tnull requested a review from joostjagerOctober 21, 2025 15:17
@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Now added changes reintroducing a secondary runtime internal to VssStore in order to fix the blocking issue we recently hit in our CI. Additionally, we found that not all tests cfg-gated under vss_test were being run, so we fix these test cases and have them run in CI.

@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 7 times, most recently from 6d3a774 to eedfb12CompareOctober 21, 2025 21:31

@joostjagerjoostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR could have been split up in independent changes, but perhaps too late now.

The whole rust async situation is so painful. It really seems to get to the point - if it wasn't already there - of being a true velocity killer.

Comment threadsrc/lib.rs
Comment threadsrc/event.rs
Comment threadtests/integration_tests_cln.rs
Comment threadsrc/runtime.rs
Comment threadsrc/lib.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from eedfb12 to dba502dCompareOctober 27, 2025 12:52

@tnulltnull left a comment

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Addressed remaining comments.

Comment threadsrc/lib.rs
Comment threadtests/integration_tests_cln.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/runtime.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 2 times, most recently from 300e9bb to 4622fb1CompareOctober 27, 2025 13:08
@tnull
tnull requested a review from joostjagerOctober 27, 2025 15:02
@tnull

Copy link
Copy Markdown
CollaboratorAuthor

CI failure is unrelated: #679

@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Addressed pending feedback.

@joostjager let me know if I can squash.

Given we regularly run into issues arising from mixing sync and async
contexts, we here simplify our `EventQueue` implementation by avoiding
to use `Condvar::wait_while` (which parks the current thread) and rather
simply us `block_on` on our `next_event_async` method.
.. as LDK Node is moving towards a more `async` core, it starts to make
sense to switch our test suite over to be `async`.
This change should make our CI more efficient (as not every node will
spawn its independent runtime, but we just have one runtime per test
created) and also makes sure we won't run into any edge cases arising
from blocking test threads that are executing other async tasks.
Since it seems to make a difference to `tokio` (see
https://docs.rs/tokio/latest/tokio/time/fn.timeout.html#panics) we make
sure the futures are always put in an `async` closure.
In order to avoid the recently, discovered blocking-task-deadlock (in
which the task holding the runtime reactor got blocked and hence
stopped polling VSS write tasks), we where re-introduce an internal
runtime to the `VssStore`, on which we spawn the tasks, while still
using `block_on` of our regular runtime for async-sync conversions.
This also finally fixes our VSS CI.
.. we previously avoided running some tests which turned out to be
broken.
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from e4f3ba0 to 8cad63bCompareOctober 28, 2025 09:23

@joostjagerjoostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remaining questions answered offline and summarized in threads. LGTM

@tnull
tnull merged commit d984aca into lightningdevkit:mainOct 28, 2025
6 of 15 checks passed
@tnulltnull mentioned this pull request Oct 28, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tnull@ldk-reviews-bot@joostjager
, '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" + ' Asyncify test suite and reintroduce VSS-internal runtime by tnull · Pull Request #670 · lightningdevkit/ldk-node · GitHub
Skip to content

Asyncify test suite and reintroduce VSS-internal runtime - #670

Merged
tnull merged 7 commits into
lightningdevkit:mainfrom
tnull:2025-10-asyncify-more-things
Oct 28, 2025
Merged

Asyncify test suite and reintroduce VSS-internal runtime#670
tnull merged 7 commits into
lightningdevkit:mainfrom
tnull:2025-10-asyncify-more-things

Conversation

@tnull

@tnulltnull commented Oct 17, 2025

Copy link
Copy Markdown
Collaborator

As we're looking to move to a more async-friendly API over time (which would allow us to drop the dependency on the KVStoreSync implementation entirely), we here make a few prefactors and minor improvements that move in that direction.

Most notably, we switch to have our tests run in async contexts, which also should make them more efficient as not every node will spawn its own runtime, but rather reuse the one runtime spawned for each test case.

Moreover, we reintroduce a secondary runtime internal to VssStore in order to fix the blocking issue we recently hit in our CI. Additionally, we found that not all tests cfg-gated under vss_test were being run, so we fix these test cases and have them run in CI.

.. as we're about to `cargo test` anyways.
.. some of the `ReadFailed` cases didn't log why they failed. Here we
fix that oversight.
@tnull
tnull requested a review from joostjagerOctober 17, 2025 11:08
@ldk-reviews-bot

ldk-reviews-bot commented Oct 17, 2025

Copy link
Copy Markdown

👋 Thanks for assigning @joostjager 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.

@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 3 times, most recently from f817f78 to 141f186CompareOctober 17, 2025 11:21
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @joostjager! 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.

Comment threadsrc/lib.rs
Comment threadsrc/event.rs
Comment threadsrc/event.rs
Comment threadtests/integration_tests_cln.rs
Comment threadtests/reorg_test.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from e6e2073 to 584fff6CompareOctober 21, 2025 15:11
@tnulltnull changed the title Asyncify test suite and event queueAsyncify test suite and reintroduce VSS-internal runtimeOct 21, 2025
@tnull
tnull requested a review from joostjagerOctober 21, 2025 15:17
@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Now added changes reintroducing a secondary runtime internal to VssStore in order to fix the blocking issue we recently hit in our CI. Additionally, we found that not all tests cfg-gated under vss_test were being run, so we fix these test cases and have them run in CI.

@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 7 times, most recently from 6d3a774 to eedfb12CompareOctober 21, 2025 21:31

@joostjagerjoostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR could have been split up in independent changes, but perhaps too late now.

The whole rust async situation is so painful. It really seems to get to the point - if it wasn't already there - of being a true velocity killer.

Comment threadsrc/lib.rs
Comment threadsrc/event.rs
Comment threadtests/integration_tests_cln.rs
Comment threadsrc/runtime.rs
Comment threadsrc/lib.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from eedfb12 to dba502dCompareOctober 27, 2025 12:52

@tnulltnull left a comment

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Addressed remaining comments.

Comment threadsrc/lib.rs
Comment threadtests/integration_tests_cln.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/runtime.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 2 times, most recently from 300e9bb to 4622fb1CompareOctober 27, 2025 13:08
@tnull
tnull requested a review from joostjagerOctober 27, 2025 15:02
@tnull

Copy link
Copy Markdown
CollaboratorAuthor

CI failure is unrelated: #679

@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Addressed pending feedback.

@joostjager let me know if I can squash.

Given we regularly run into issues arising from mixing sync and async
contexts, we here simplify our `EventQueue` implementation by avoiding
to use `Condvar::wait_while` (which parks the current thread) and rather
simply us `block_on` on our `next_event_async` method.
.. as LDK Node is moving towards a more `async` core, it starts to make
sense to switch our test suite over to be `async`.
This change should make our CI more efficient (as not every node will
spawn its independent runtime, but we just have one runtime per test
created) and also makes sure we won't run into any edge cases arising
from blocking test threads that are executing other async tasks.
Since it seems to make a difference to `tokio` (see
https://docs.rs/tokio/latest/tokio/time/fn.timeout.html#panics) we make
sure the futures are always put in an `async` closure.
In order to avoid the recently, discovered blocking-task-deadlock (in
which the task holding the runtime reactor got blocked and hence
stopped polling VSS write tasks), we where re-introduce an internal
runtime to the `VssStore`, on which we spawn the tasks, while still
using `block_on` of our regular runtime for async-sync conversions.
This also finally fixes our VSS CI.
.. we previously avoided running some tests which turned out to be
broken.
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from e4f3ba0 to 8cad63bCompareOctober 28, 2025 09:23

@joostjagerjoostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remaining questions answered offline and summarized in threads. LGTM

@tnull
tnull merged commit d984aca into lightningdevkit:mainOct 28, 2025
6 of 15 checks passed
@tnulltnull mentioned this pull request Oct 28, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tnull@ldk-reviews-bot@joostjager
, '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('^' + ".*" + ' Asyncify test suite and reintroduce VSS-internal runtime by tnull · Pull Request #670 · lightningdevkit/ldk-node · GitHub
Skip to content

Asyncify test suite and reintroduce VSS-internal runtime - #670

Merged
tnull merged 7 commits into
lightningdevkit:mainfrom
tnull:2025-10-asyncify-more-things
Oct 28, 2025
Merged

Asyncify test suite and reintroduce VSS-internal runtime#670
tnull merged 7 commits into
lightningdevkit:mainfrom
tnull:2025-10-asyncify-more-things

Conversation

@tnull

@tnulltnull commented Oct 17, 2025

Copy link
Copy Markdown
Collaborator

As we're looking to move to a more async-friendly API over time (which would allow us to drop the dependency on the KVStoreSync implementation entirely), we here make a few prefactors and minor improvements that move in that direction.

Most notably, we switch to have our tests run in async contexts, which also should make them more efficient as not every node will spawn its own runtime, but rather reuse the one runtime spawned for each test case.

Moreover, we reintroduce a secondary runtime internal to VssStore in order to fix the blocking issue we recently hit in our CI. Additionally, we found that not all tests cfg-gated under vss_test were being run, so we fix these test cases and have them run in CI.

.. as we're about to `cargo test` anyways.
.. some of the `ReadFailed` cases didn't log why they failed. Here we
fix that oversight.
@tnull
tnull requested a review from joostjagerOctober 17, 2025 11:08
@ldk-reviews-bot

ldk-reviews-bot commented Oct 17, 2025

Copy link
Copy Markdown

👋 Thanks for assigning @joostjager 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.

@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 3 times, most recently from f817f78 to 141f186CompareOctober 17, 2025 11:21
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @joostjager! 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.

Comment threadsrc/lib.rs
Comment threadsrc/event.rs
Comment threadsrc/event.rs
Comment threadtests/integration_tests_cln.rs
Comment threadtests/reorg_test.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from e6e2073 to 584fff6CompareOctober 21, 2025 15:11
@tnulltnull changed the title Asyncify test suite and event queueAsyncify test suite and reintroduce VSS-internal runtimeOct 21, 2025
@tnull
tnull requested a review from joostjagerOctober 21, 2025 15:17
@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Now added changes reintroducing a secondary runtime internal to VssStore in order to fix the blocking issue we recently hit in our CI. Additionally, we found that not all tests cfg-gated under vss_test were being run, so we fix these test cases and have them run in CI.

@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 7 times, most recently from 6d3a774 to eedfb12CompareOctober 21, 2025 21:31

@joostjagerjoostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR could have been split up in independent changes, but perhaps too late now.

The whole rust async situation is so painful. It really seems to get to the point - if it wasn't already there - of being a true velocity killer.

Comment threadsrc/lib.rs
Comment threadsrc/event.rs
Comment threadtests/integration_tests_cln.rs
Comment threadsrc/runtime.rs
Comment threadsrc/lib.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from eedfb12 to dba502dCompareOctober 27, 2025 12:52

@tnulltnull left a comment

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Addressed remaining comments.

Comment threadsrc/lib.rs
Comment threadtests/integration_tests_cln.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/runtime.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 2 times, most recently from 300e9bb to 4622fb1CompareOctober 27, 2025 13:08
@tnull
tnull requested a review from joostjagerOctober 27, 2025 15:02
@tnull

Copy link
Copy Markdown
CollaboratorAuthor

CI failure is unrelated: #679

@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Addressed pending feedback.

@joostjager let me know if I can squash.

Given we regularly run into issues arising from mixing sync and async
contexts, we here simplify our `EventQueue` implementation by avoiding
to use `Condvar::wait_while` (which parks the current thread) and rather
simply us `block_on` on our `next_event_async` method.
.. as LDK Node is moving towards a more `async` core, it starts to make
sense to switch our test suite over to be `async`.
This change should make our CI more efficient (as not every node will
spawn its independent runtime, but we just have one runtime per test
created) and also makes sure we won't run into any edge cases arising
from blocking test threads that are executing other async tasks.
Since it seems to make a difference to `tokio` (see
https://docs.rs/tokio/latest/tokio/time/fn.timeout.html#panics) we make
sure the futures are always put in an `async` closure.
In order to avoid the recently, discovered blocking-task-deadlock (in
which the task holding the runtime reactor got blocked and hence
stopped polling VSS write tasks), we where re-introduce an internal
runtime to the `VssStore`, on which we spawn the tasks, while still
using `block_on` of our regular runtime for async-sync conversions.
This also finally fixes our VSS CI.
.. we previously avoided running some tests which turned out to be
broken.
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from e4f3ba0 to 8cad63bCompareOctober 28, 2025 09:23

@joostjagerjoostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remaining questions answered offline and summarized in threads. LGTM

@tnull
tnull merged commit d984aca into lightningdevkit:mainOct 28, 2025
6 of 15 checks passed
@tnulltnull mentioned this pull request Oct 28, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tnull@ldk-reviews-bot@joostjager
, '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('^' + ".*" + ' Asyncify test suite and reintroduce VSS-internal runtime by tnull · Pull Request #670 · lightningdevkit/ldk-node · GitHub
Skip to content

Asyncify test suite and reintroduce VSS-internal runtime - #670

Merged
tnull merged 7 commits into
lightningdevkit:mainfrom
tnull:2025-10-asyncify-more-things
Oct 28, 2025
Merged

Asyncify test suite and reintroduce VSS-internal runtime#670
tnull merged 7 commits into
lightningdevkit:mainfrom
tnull:2025-10-asyncify-more-things

Conversation

@tnull

@tnulltnull commented Oct 17, 2025

Copy link
Copy Markdown
Collaborator

As we're looking to move to a more async-friendly API over time (which would allow us to drop the dependency on the KVStoreSync implementation entirely), we here make a few prefactors and minor improvements that move in that direction.

Most notably, we switch to have our tests run in async contexts, which also should make them more efficient as not every node will spawn its own runtime, but rather reuse the one runtime spawned for each test case.

Moreover, we reintroduce a secondary runtime internal to VssStore in order to fix the blocking issue we recently hit in our CI. Additionally, we found that not all tests cfg-gated under vss_test were being run, so we fix these test cases and have them run in CI.

.. as we're about to `cargo test` anyways.
.. some of the `ReadFailed` cases didn't log why they failed. Here we
fix that oversight.
@tnull
tnull requested a review from joostjagerOctober 17, 2025 11:08
@ldk-reviews-bot

ldk-reviews-bot commented Oct 17, 2025

Copy link
Copy Markdown

👋 Thanks for assigning @joostjager 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.

@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 3 times, most recently from f817f78 to 141f186CompareOctober 17, 2025 11:21
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @joostjager! 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.

Comment threadsrc/lib.rs
Comment threadsrc/event.rs
Comment threadsrc/event.rs
Comment threadtests/integration_tests_cln.rs
Comment threadtests/reorg_test.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from e6e2073 to 584fff6CompareOctober 21, 2025 15:11
@tnulltnull changed the title Asyncify test suite and event queueAsyncify test suite and reintroduce VSS-internal runtimeOct 21, 2025
@tnull
tnull requested a review from joostjagerOctober 21, 2025 15:17
@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Now added changes reintroducing a secondary runtime internal to VssStore in order to fix the blocking issue we recently hit in our CI. Additionally, we found that not all tests cfg-gated under vss_test were being run, so we fix these test cases and have them run in CI.

@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 7 times, most recently from 6d3a774 to eedfb12CompareOctober 21, 2025 21:31

@joostjagerjoostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR could have been split up in independent changes, but perhaps too late now.

The whole rust async situation is so painful. It really seems to get to the point - if it wasn't already there - of being a true velocity killer.

Comment threadsrc/lib.rs
Comment threadsrc/event.rs
Comment threadtests/integration_tests_cln.rs
Comment threadsrc/runtime.rs
Comment threadsrc/lib.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from eedfb12 to dba502dCompareOctober 27, 2025 12:52

@tnulltnull left a comment

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Addressed remaining comments.

Comment threadsrc/lib.rs
Comment threadtests/integration_tests_cln.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/runtime.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 2 times, most recently from 300e9bb to 4622fb1CompareOctober 27, 2025 13:08
@tnull
tnull requested a review from joostjagerOctober 27, 2025 15:02
@tnull

Copy link
Copy Markdown
CollaboratorAuthor

CI failure is unrelated: #679

@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Addressed pending feedback.

@joostjager let me know if I can squash.

Given we regularly run into issues arising from mixing sync and async
contexts, we here simplify our `EventQueue` implementation by avoiding
to use `Condvar::wait_while` (which parks the current thread) and rather
simply us `block_on` on our `next_event_async` method.
.. as LDK Node is moving towards a more `async` core, it starts to make
sense to switch our test suite over to be `async`.
This change should make our CI more efficient (as not every node will
spawn its independent runtime, but we just have one runtime per test
created) and also makes sure we won't run into any edge cases arising
from blocking test threads that are executing other async tasks.
Since it seems to make a difference to `tokio` (see
https://docs.rs/tokio/latest/tokio/time/fn.timeout.html#panics) we make
sure the futures are always put in an `async` closure.
In order to avoid the recently, discovered blocking-task-deadlock (in
which the task holding the runtime reactor got blocked and hence
stopped polling VSS write tasks), we where re-introduce an internal
runtime to the `VssStore`, on which we spawn the tasks, while still
using `block_on` of our regular runtime for async-sync conversions.
This also finally fixes our VSS CI.
.. we previously avoided running some tests which turned out to be
broken.
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from e4f3ba0 to 8cad63bCompareOctober 28, 2025 09:23

@joostjagerjoostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remaining questions answered offline and summarized in threads. LGTM

@tnull
tnull merged commit d984aca into lightningdevkit:mainOct 28, 2025
6 of 15 checks passed
@tnulltnull mentioned this pull request Oct 28, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tnull@ldk-reviews-bot@joostjager
, '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); } })(); })(); Asyncify test suite and reintroduce VSS-internal runtime by tnull · Pull Request #670 · lightningdevkit/ldk-node · GitHub
Skip to content

Asyncify test suite and reintroduce VSS-internal runtime - #670

Merged
tnull merged 7 commits into
lightningdevkit:mainfrom
tnull:2025-10-asyncify-more-things
Oct 28, 2025
Merged

Asyncify test suite and reintroduce VSS-internal runtime#670
tnull merged 7 commits into
lightningdevkit:mainfrom
tnull:2025-10-asyncify-more-things

Conversation

@tnull

@tnulltnull commented Oct 17, 2025

Copy link
Copy Markdown
Collaborator

As we're looking to move to a more async-friendly API over time (which would allow us to drop the dependency on the KVStoreSync implementation entirely), we here make a few prefactors and minor improvements that move in that direction.

Most notably, we switch to have our tests run in async contexts, which also should make them more efficient as not every node will spawn its own runtime, but rather reuse the one runtime spawned for each test case.

Moreover, we reintroduce a secondary runtime internal to VssStore in order to fix the blocking issue we recently hit in our CI. Additionally, we found that not all tests cfg-gated under vss_test were being run, so we fix these test cases and have them run in CI.

.. as we're about to `cargo test` anyways.
.. some of the `ReadFailed` cases didn't log why they failed. Here we
fix that oversight.
@tnull
tnull requested a review from joostjagerOctober 17, 2025 11:08
@ldk-reviews-bot

ldk-reviews-bot commented Oct 17, 2025

Copy link
Copy Markdown

👋 Thanks for assigning @joostjager 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.

@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 3 times, most recently from f817f78 to 141f186CompareOctober 17, 2025 11:21
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @joostjager! 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.

Comment threadsrc/lib.rs
Comment threadsrc/event.rs
Comment threadsrc/event.rs
Comment threadtests/integration_tests_cln.rs
Comment threadtests/reorg_test.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from e6e2073 to 584fff6CompareOctober 21, 2025 15:11
@tnulltnull changed the title Asyncify test suite and event queueAsyncify test suite and reintroduce VSS-internal runtimeOct 21, 2025
@tnull
tnull requested a review from joostjagerOctober 21, 2025 15:17
@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Now added changes reintroducing a secondary runtime internal to VssStore in order to fix the blocking issue we recently hit in our CI. Additionally, we found that not all tests cfg-gated under vss_test were being run, so we fix these test cases and have them run in CI.

@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 7 times, most recently from 6d3a774 to eedfb12CompareOctober 21, 2025 21:31

@joostjagerjoostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR could have been split up in independent changes, but perhaps too late now.

The whole rust async situation is so painful. It really seems to get to the point - if it wasn't already there - of being a true velocity killer.

Comment threadsrc/lib.rs
Comment threadsrc/event.rs
Comment threadtests/integration_tests_cln.rs
Comment threadsrc/runtime.rs
Comment threadsrc/lib.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from eedfb12 to dba502dCompareOctober 27, 2025 12:52

@tnulltnull left a comment

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Addressed remaining comments.

Comment threadsrc/lib.rs
Comment threadtests/integration_tests_cln.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/runtime.rs
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch 2 times, most recently from 300e9bb to 4622fb1CompareOctober 27, 2025 13:08
@tnull
tnull requested a review from joostjagerOctober 27, 2025 15:02
@tnull

Copy link
Copy Markdown
CollaboratorAuthor

CI failure is unrelated: #679

@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Addressed pending feedback.

@joostjager let me know if I can squash.

Given we regularly run into issues arising from mixing sync and async
contexts, we here simplify our `EventQueue` implementation by avoiding
to use `Condvar::wait_while` (which parks the current thread) and rather
simply us `block_on` on our `next_event_async` method.
.. as LDK Node is moving towards a more `async` core, it starts to make
sense to switch our test suite over to be `async`.
This change should make our CI more efficient (as not every node will
spawn its independent runtime, but we just have one runtime per test
created) and also makes sure we won't run into any edge cases arising
from blocking test threads that are executing other async tasks.
Since it seems to make a difference to `tokio` (see
https://docs.rs/tokio/latest/tokio/time/fn.timeout.html#panics) we make
sure the futures are always put in an `async` closure.
In order to avoid the recently, discovered blocking-task-deadlock (in
which the task holding the runtime reactor got blocked and hence
stopped polling VSS write tasks), we where re-introduce an internal
runtime to the `VssStore`, on which we spawn the tasks, while still
using `block_on` of our regular runtime for async-sync conversions.
This also finally fixes our VSS CI.
.. we previously avoided running some tests which turned out to be
broken.
@tnull
tnullforce-pushed the 2025-10-asyncify-more-things branch from e4f3ba0 to 8cad63bCompareOctober 28, 2025 09:23

@joostjagerjoostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remaining questions answered offline and summarized in threads. LGTM

@tnull
tnull merged commit d984aca into lightningdevkit:mainOct 28, 2025
6 of 15 checks passed
@tnulltnull mentioned this pull request Oct 28, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tnull@ldk-reviews-bot@joostjager