Expose async monitor persistence for tests

[tnull]

Downstream tests need to exercise the same async monitor persistence path used by ChainMonitor::new_async_beta without reimplementing Persist. Add test-only constructors that let TestChainMonitor and AsyncPersister share the wake notifier so async completions drive the monitor update future.

Co-Authored-By: HAL 9000

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

[ldk-claude-review-bot]

I've re-examined the full PR including the surrounding code that the diff touches: the AsyncPersister struct and its Persist impl, new_async_beta (which the test-only path mirrors), and the Notifier wiring through with_event_notifier / with_deferred_and_event_notifier.

The notifier plumbing in the test-only constructors faithfully matches the production new_async_beta wiring (event_notifier: Arc::clone(...) shared between the ChainMonitor and the AsyncPersister). The cfg gating is consistent: new_with_event_notifier on both ChainMonitor and TestChainMonitor are gated on _test_utils, and the private with_deferred_and_event_notifier helper handles both compilation paths so no Some(notifier) can reach a non-test build. The Arc::strong_count assertions in the test (2 after new_test, 3 after new_with_event_notifier) are correct given the wiring.

No issues found.

[TheBlueMatt]

Hmm, its not entirely clear to me why this is needed? Can you spell out what kind of test you want to write and why it needs this? In general downstream tests (and, really, any tests) should just block the KVStore write they want to block, IMO.

[tnull]

Hmm, its not entirely clear to me why this is needed? Can you spell out what kind of test you want to write and why it needs this? In general downstream tests (and, really, any tests) should just block the KVStore write they want to block, IMO.

Yes see here: lightningdevkit/ldk-node#919 (comment)

This will allow us to do:

typeTestMonitorPersister<K> = MonitorUpdatingPersisterAsync<TestStoreRef<K>,TestFutureSpawner,&'static test_utils::TestLogger,&'static test_utils::TestKeysInterface,&'static test_utils::TestKeysInterface,&'static test_utils::TestBroadcaster,&'static test_utils::TestFeeEstimator,>;typeTestAsyncPersister<K> = lightning::chain::chainmonitor::AsyncPersister<TestStoreRef<K>,TestFutureSpawner,&'static test_utils::TestLogger,&'static test_utils::TestKeysInterface,&'static test_utils::TestKeysInterface,&'static test_utils::TestBroadcaster,&'static test_utils::TestFeeEstimator,>;(..)let monitor_persister = TestMonitorPersister::new(
store,TestFutureSpawner::new(Arc::clone(&runtime)),&chanmon_cfg.logger,
max_pending_updates,&chanmon_cfg.keys_manager,&chanmon_cfg.keys_manager,&chanmon_cfg.tx_broadcaster,&chanmon_cfg.fee_estimator,);let event_notifier = Arc::new(Notifier::new());let persister = lightning::chain::chainmonitor::AsyncPersister::new_test(
monitor_persister,Arc::clone(&event_notifier),);(...)test_utils::TestChainMonitor::new_with_event_notifier(Some(&chanmon_cfg.chain_source),&chanmon_cfg.tx_broadcaster,&chanmon_cfg.logger,&chanmon_cfg.fee_estimator,&persister.persister,&chanmon_cfg.keys_manager,Arc::clone(&persister.event_notifier),)

.. rather than duplicating code via our custom TestMonitorUpdatePersister we have in that PR branch currently (which doesn't actually exercise the MonitorUpdatingPersister we have in prod):

https://github.com/tnull/ldk-node/blob/9e095d154babc6a37ecbe8a967eceadf4bf4a3a6/src/io/test_utils.rs#L41-L158

[TheBlueMatt]

Are there some specific tests in LDK Node that need that struct vs just using the MonitorUpdatingPersister you use in prod and hooking any writes to pause/resume them at the KVStore layer?

[tnull]

Updated to line-wrap commit messages.

[TheBlueMatt]

Still left with the above question. I don't love exposing this if there's an alternative way to do it downstream.

[tnull]

Still left with the above question. I don't love exposing this if there's an alternative way to do it downstream.

Excuse the delay! We use this in our SqliteStore/PostgresStore tests, see https://github.com/tnull/ldk-node/blob/a3e7653bb41a43e4bb2e5b1c5f55bae0ee8d1880/src/io/test_utils.rs#L276, which is basically an async version of https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-persister/src/test_utils.rs#L162.

AFAIU, having a test-only path to allow constructing async-persisted variants of TestChainMonitors would also be required if we'd ever update our test coverage of the async-monitor path in general, but in particular also if we'd want to add coverage for the async path for the FilesystemStore tests, which seems like good end-to-end coverage to have (and of course eventually we'll want to upstream all of these stores to lightning-persister, so it would be good to reuse the same test utils / not have too much LDK Node custom code that we'll need to drop then anyways).

[TheBlueMatt]

I'm still confused. I can't try to play with this cause not sure what TestStoreRef and TestFutureSpawner are, but I don't see why you can't use new_async_beta and just build a normal ChainMonitor and use that. Why do you want the TestChainMonitor at all? And if you do, we could expose a way to build an async monitor in it (TestChainMonitor isn't really set up to support an underlying async monitor, though its certainly possible it mostly works).

[ldk-reviews-bot]

Hi @tnull,

Thanks for your contributions to rust-lightning!

After too many struggles with bugs, outages, contributor bans, and, finally, a multi-week CI ban, the rust-lightning project is moving off of GitHub for day-to-day development.

You can still file issues and access the git tree here, but PRs will now take place exclusively at https://git.rust-bitcoin.org/. As such, this PR has been migrated to https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4665

If you log in using GitHub (or otherwise link your GitHub account from https://git.rust-bitcoin.org/user/settings/security), ownership of your PRs, issues, and comments will automatically transfer. To push updates to this PR, you'll need to use git push git@gitea-ssh.bitcoin.ninja:lightningdevkit/rust-lightning YOUR_LOCAL_COMMIT_OR_BRANCH:tnull/2026-06-async-persister-test-notifier. This may require a permissions change - if it doesn't work initially just leave a comment and we'll get you access.

[tnull]

Downstream tests need to exercise the same async monitor persistence path used by ChainMonitor::new_async_beta without reimplementing Persist. Add test-only constructors that let TestChainMonitor and AsyncPersister share the wake notifier so async completions drive the monitor update future.

Co-Authored-By: HAL 9000

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

[ldk-claude-review-bot]

I've re-examined the full PR including the surrounding code that the diff touches: the AsyncPersister struct and its Persist impl, new_async_beta (which the test-only path mirrors), and the Notifier wiring through with_event_notifier / with_deferred_and_event_notifier.

The notifier plumbing in the test-only constructors faithfully matches the production new_async_beta wiring (event_notifier: Arc::clone(...) shared between the ChainMonitor and the AsyncPersister). The cfg gating is consistent: new_with_event_notifier on both ChainMonitor and TestChainMonitor are gated on _test_utils, and the private with_deferred_and_event_notifier helper handles both compilation paths so no Some(notifier) can reach a non-test build. The Arc::strong_count assertions in the test (2 after new_test, 3 after new_with_event_notifier) are correct given the wiring.

No issues found.

[TheBlueMatt]

Hmm, its not entirely clear to me why this is needed? Can you spell out what kind of test you want to write and why it needs this? In general downstream tests (and, really, any tests) should just block the KVStore write they want to block, IMO.

[tnull]

Hmm, its not entirely clear to me why this is needed? Can you spell out what kind of test you want to write and why it needs this? In general downstream tests (and, really, any tests) should just block the KVStore write they want to block, IMO.

Yes see here: lightningdevkit/ldk-node#919 (comment)

This will allow us to do:

typeTestMonitorPersister<K> = MonitorUpdatingPersisterAsync<TestStoreRef<K>,TestFutureSpawner,&'static test_utils::TestLogger,&'static test_utils::TestKeysInterface,&'static test_utils::TestKeysInterface,&'static test_utils::TestBroadcaster,&'static test_utils::TestFeeEstimator,>;typeTestAsyncPersister<K> = lightning::chain::chainmonitor::AsyncPersister<TestStoreRef<K>,TestFutureSpawner,&'static test_utils::TestLogger,&'static test_utils::TestKeysInterface,&'static test_utils::TestKeysInterface,&'static test_utils::TestBroadcaster,&'static test_utils::TestFeeEstimator,>;(..)let monitor_persister = TestMonitorPersister::new(
store,TestFutureSpawner::new(Arc::clone(&runtime)),&chanmon_cfg.logger,
max_pending_updates,&chanmon_cfg.keys_manager,&chanmon_cfg.keys_manager,&chanmon_cfg.tx_broadcaster,&chanmon_cfg.fee_estimator,);let event_notifier = Arc::new(Notifier::new());let persister = lightning::chain::chainmonitor::AsyncPersister::new_test(
monitor_persister,Arc::clone(&event_notifier),);(...)test_utils::TestChainMonitor::new_with_event_notifier(Some(&chanmon_cfg.chain_source),&chanmon_cfg.tx_broadcaster,&chanmon_cfg.logger,&chanmon_cfg.fee_estimator,&persister.persister,&chanmon_cfg.keys_manager,Arc::clone(&persister.event_notifier),)

.. rather than duplicating code via our custom TestMonitorUpdatePersister we have in that PR branch currently (which doesn't actually exercise the MonitorUpdatingPersister we have in prod):

https://github.com/tnull/ldk-node/blob/9e095d154babc6a37ecbe8a967eceadf4bf4a3a6/src/io/test_utils.rs#L41-L158

[TheBlueMatt]

Are there some specific tests in LDK Node that need that struct vs just using the MonitorUpdatingPersister you use in prod and hooking any writes to pause/resume them at the KVStore layer?

[tnull]

Updated to line-wrap commit messages.

[TheBlueMatt]

Still left with the above question. I don't love exposing this if there's an alternative way to do it downstream.

[tnull]

Still left with the above question. I don't love exposing this if there's an alternative way to do it downstream.

Excuse the delay! We use this in our SqliteStore/PostgresStore tests, see https://github.com/tnull/ldk-node/blob/a3e7653bb41a43e4bb2e5b1c5f55bae0ee8d1880/src/io/test_utils.rs#L276, which is basically an async version of https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-persister/src/test_utils.rs#L162.

AFAIU, having a test-only path to allow constructing async-persisted variants of TestChainMonitors would also be required if we'd ever update our test coverage of the async-monitor path in general, but in particular also if we'd want to add coverage for the async path for the FilesystemStore tests, which seems like good end-to-end coverage to have (and of course eventually we'll want to upstream all of these stores to lightning-persister, so it would be good to reuse the same test utils / not have too much LDK Node custom code that we'll need to drop then anyways).

[TheBlueMatt]

I'm still confused. I can't try to play with this cause not sure what TestStoreRef and TestFutureSpawner are, but I don't see why you can't use new_async_beta and just build a normal ChainMonitor and use that. Why do you want the TestChainMonitor at all? And if you do, we could expose a way to build an async monitor in it (TestChainMonitor isn't really set up to support an underlying async monitor, though its certainly possible it mostly works).

[ldk-reviews-bot]

Hi @tnull,

Thanks for your contributions to rust-lightning!

After too many struggles with bugs, outages, contributor bans, and, finally, a multi-week CI ban, the rust-lightning project is moving off of GitHub for day-to-day development.

You can still file issues and access the git tree here, but PRs will now take place exclusively at https://git.rust-bitcoin.org/. As such, this PR has been migrated to https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4665

If you log in using GitHub (or otherwise link your GitHub account from https://git.rust-bitcoin.org/user/settings/security), ownership of your PRs, issues, and comments will automatically transfer. To push updates to this PR, you'll need to use git push git@gitea-ssh.bitcoin.ninja:lightningdevkit/rust-lightning YOUR_LOCAL_COMMIT_OR_BRANCH:tnull/2026-06-async-persister-test-notifier. This may require a permissions change - if it doesn't work initially just leave a comment and we'll get you access.

[tnull]

Downstream tests need to exercise the same async monitor persistence path used by ChainMonitor::new_async_beta without reimplementing Persist. Add test-only constructors that let TestChainMonitor and AsyncPersister share the wake notifier so async completions drive the monitor update future.

Co-Authored-By: HAL 9000

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

[ldk-claude-review-bot]

I've re-examined the full PR including the surrounding code that the diff touches: the AsyncPersister struct and its Persist impl, new_async_beta (which the test-only path mirrors), and the Notifier wiring through with_event_notifier / with_deferred_and_event_notifier.

The notifier plumbing in the test-only constructors faithfully matches the production new_async_beta wiring (event_notifier: Arc::clone(...) shared between the ChainMonitor and the AsyncPersister). The cfg gating is consistent: new_with_event_notifier on both ChainMonitor and TestChainMonitor are gated on _test_utils, and the private with_deferred_and_event_notifier helper handles both compilation paths so no Some(notifier) can reach a non-test build. The Arc::strong_count assertions in the test (2 after new_test, 3 after new_with_event_notifier) are correct given the wiring.

No issues found.

[TheBlueMatt]

Hmm, its not entirely clear to me why this is needed? Can you spell out what kind of test you want to write and why it needs this? In general downstream tests (and, really, any tests) should just block the KVStore write they want to block, IMO.

[tnull]

Hmm, its not entirely clear to me why this is needed? Can you spell out what kind of test you want to write and why it needs this? In general downstream tests (and, really, any tests) should just block the KVStore write they want to block, IMO.

Yes see here: lightningdevkit/ldk-node#919 (comment)

This will allow us to do:

typeTestMonitorPersister<K> = MonitorUpdatingPersisterAsync<TestStoreRef<K>,TestFutureSpawner,&'static test_utils::TestLogger,&'static test_utils::TestKeysInterface,&'static test_utils::TestKeysInterface,&'static test_utils::TestBroadcaster,&'static test_utils::TestFeeEstimator,>;typeTestAsyncPersister<K> = lightning::chain::chainmonitor::AsyncPersister<TestStoreRef<K>,TestFutureSpawner,&'static test_utils::TestLogger,&'static test_utils::TestKeysInterface,&'static test_utils::TestKeysInterface,&'static test_utils::TestBroadcaster,&'static test_utils::TestFeeEstimator,>;(..)let monitor_persister = TestMonitorPersister::new(
store,TestFutureSpawner::new(Arc::clone(&runtime)),&chanmon_cfg.logger,
max_pending_updates,&chanmon_cfg.keys_manager,&chanmon_cfg.keys_manager,&chanmon_cfg.tx_broadcaster,&chanmon_cfg.fee_estimator,);let event_notifier = Arc::new(Notifier::new());let persister = lightning::chain::chainmonitor::AsyncPersister::new_test(
monitor_persister,Arc::clone(&event_notifier),);(...)test_utils::TestChainMonitor::new_with_event_notifier(Some(&chanmon_cfg.chain_source),&chanmon_cfg.tx_broadcaster,&chanmon_cfg.logger,&chanmon_cfg.fee_estimator,&persister.persister,&chanmon_cfg.keys_manager,Arc::clone(&persister.event_notifier),)

.. rather than duplicating code via our custom TestMonitorUpdatePersister we have in that PR branch currently (which doesn't actually exercise the MonitorUpdatingPersister we have in prod):

https://github.com/tnull/ldk-node/blob/9e095d154babc6a37ecbe8a967eceadf4bf4a3a6/src/io/test_utils.rs#L41-L158

[TheBlueMatt]

Are there some specific tests in LDK Node that need that struct vs just using the MonitorUpdatingPersister you use in prod and hooking any writes to pause/resume them at the KVStore layer?

[tnull]

Updated to line-wrap commit messages.

[TheBlueMatt]

Still left with the above question. I don't love exposing this if there's an alternative way to do it downstream.

[tnull]

Still left with the above question. I don't love exposing this if there's an alternative way to do it downstream.

Excuse the delay! We use this in our SqliteStore/PostgresStore tests, see https://github.com/tnull/ldk-node/blob/a3e7653bb41a43e4bb2e5b1c5f55bae0ee8d1880/src/io/test_utils.rs#L276, which is basically an async version of https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-persister/src/test_utils.rs#L162.

AFAIU, having a test-only path to allow constructing async-persisted variants of TestChainMonitors would also be required if we'd ever update our test coverage of the async-monitor path in general, but in particular also if we'd want to add coverage for the async path for the FilesystemStore tests, which seems like good end-to-end coverage to have (and of course eventually we'll want to upstream all of these stores to lightning-persister, so it would be good to reuse the same test utils / not have too much LDK Node custom code that we'll need to drop then anyways).

[TheBlueMatt]

I'm still confused. I can't try to play with this cause not sure what TestStoreRef and TestFutureSpawner are, but I don't see why you can't use new_async_beta and just build a normal ChainMonitor and use that. Why do you want the TestChainMonitor at all? And if you do, we could expose a way to build an async monitor in it (TestChainMonitor isn't really set up to support an underlying async monitor, though its certainly possible it mostly works).

[ldk-reviews-bot]

Hi @tnull,

Thanks for your contributions to rust-lightning!

After too many struggles with bugs, outages, contributor bans, and, finally, a multi-week CI ban, the rust-lightning project is moving off of GitHub for day-to-day development.

You can still file issues and access the git tree here, but PRs will now take place exclusively at https://git.rust-bitcoin.org/. As such, this PR has been migrated to https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4665

If you log in using GitHub (or otherwise link your GitHub account from https://git.rust-bitcoin.org/user/settings/security), ownership of your PRs, issues, and comments will automatically transfer. To push updates to this PR, you'll need to use git push git@gitea-ssh.bitcoin.ninja:lightningdevkit/rust-lightning YOUR_LOCAL_COMMIT_OR_BRANCH:tnull/2026-06-async-persister-test-notifier. This may require a permissions change - if it doesn't work initially just leave a comment and we'll get you access.

[tnull]

Downstream tests need to exercise the same async monitor persistence path used by ChainMonitor::new_async_beta without reimplementing Persist. Add test-only constructors that let TestChainMonitor and AsyncPersister share the wake notifier so async completions drive the monitor update future.

Co-Authored-By: HAL 9000

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

[ldk-claude-review-bot]

I've re-examined the full PR including the surrounding code that the diff touches: the AsyncPersister struct and its Persist impl, new_async_beta (which the test-only path mirrors), and the Notifier wiring through with_event_notifier / with_deferred_and_event_notifier.

The notifier plumbing in the test-only constructors faithfully matches the production new_async_beta wiring (event_notifier: Arc::clone(...) shared between the ChainMonitor and the AsyncPersister). The cfg gating is consistent: new_with_event_notifier on both ChainMonitor and TestChainMonitor are gated on _test_utils, and the private with_deferred_and_event_notifier helper handles both compilation paths so no Some(notifier) can reach a non-test build. The Arc::strong_count assertions in the test (2 after new_test, 3 after new_with_event_notifier) are correct given the wiring.

No issues found.

[TheBlueMatt]

Hmm, its not entirely clear to me why this is needed? Can you spell out what kind of test you want to write and why it needs this? In general downstream tests (and, really, any tests) should just block the KVStore write they want to block, IMO.

[tnull]

Hmm, its not entirely clear to me why this is needed? Can you spell out what kind of test you want to write and why it needs this? In general downstream tests (and, really, any tests) should just block the KVStore write they want to block, IMO.

Yes see here: lightningdevkit/ldk-node#919 (comment)

This will allow us to do:

typeTestMonitorPersister<K> = MonitorUpdatingPersisterAsync<TestStoreRef<K>,TestFutureSpawner,&'static test_utils::TestLogger,&'static test_utils::TestKeysInterface,&'static test_utils::TestKeysInterface,&'static test_utils::TestBroadcaster,&'static test_utils::TestFeeEstimator,>;typeTestAsyncPersister<K> = lightning::chain::chainmonitor::AsyncPersister<TestStoreRef<K>,TestFutureSpawner,&'static test_utils::TestLogger,&'static test_utils::TestKeysInterface,&'static test_utils::TestKeysInterface,&'static test_utils::TestBroadcaster,&'static test_utils::TestFeeEstimator,>;(..)let monitor_persister = TestMonitorPersister::new(
store,TestFutureSpawner::new(Arc::clone(&runtime)),&chanmon_cfg.logger,
max_pending_updates,&chanmon_cfg.keys_manager,&chanmon_cfg.keys_manager,&chanmon_cfg.tx_broadcaster,&chanmon_cfg.fee_estimator,);let event_notifier = Arc::new(Notifier::new());let persister = lightning::chain::chainmonitor::AsyncPersister::new_test(
monitor_persister,Arc::clone(&event_notifier),);(...)test_utils::TestChainMonitor::new_with_event_notifier(Some(&chanmon_cfg.chain_source),&chanmon_cfg.tx_broadcaster,&chanmon_cfg.logger,&chanmon_cfg.fee_estimator,&persister.persister,&chanmon_cfg.keys_manager,Arc::clone(&persister.event_notifier),)

.. rather than duplicating code via our custom TestMonitorUpdatePersister we have in that PR branch currently (which doesn't actually exercise the MonitorUpdatingPersister we have in prod):

https://github.com/tnull/ldk-node/blob/9e095d154babc6a37ecbe8a967eceadf4bf4a3a6/src/io/test_utils.rs#L41-L158

[TheBlueMatt]

Are there some specific tests in LDK Node that need that struct vs just using the MonitorUpdatingPersister you use in prod and hooking any writes to pause/resume them at the KVStore layer?

[tnull]

Updated to line-wrap commit messages.

[TheBlueMatt]

Still left with the above question. I don't love exposing this if there's an alternative way to do it downstream.

[tnull]

Still left with the above question. I don't love exposing this if there's an alternative way to do it downstream.

Excuse the delay! We use this in our SqliteStore/PostgresStore tests, see https://github.com/tnull/ldk-node/blob/a3e7653bb41a43e4bb2e5b1c5f55bae0ee8d1880/src/io/test_utils.rs#L276, which is basically an async version of https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-persister/src/test_utils.rs#L162.

AFAIU, having a test-only path to allow constructing async-persisted variants of TestChainMonitors would also be required if we'd ever update our test coverage of the async-monitor path in general, but in particular also if we'd want to add coverage for the async path for the FilesystemStore tests, which seems like good end-to-end coverage to have (and of course eventually we'll want to upstream all of these stores to lightning-persister, so it would be good to reuse the same test utils / not have too much LDK Node custom code that we'll need to drop then anyways).

[TheBlueMatt]

I'm still confused. I can't try to play with this cause not sure what TestStoreRef and TestFutureSpawner are, but I don't see why you can't use new_async_beta and just build a normal ChainMonitor and use that. Why do you want the TestChainMonitor at all? And if you do, we could expose a way to build an async monitor in it (TestChainMonitor isn't really set up to support an underlying async monitor, though its certainly possible it mostly works).

[ldk-reviews-bot]

Hi @tnull,

Thanks for your contributions to rust-lightning!

After too many struggles with bugs, outages, contributor bans, and, finally, a multi-week CI ban, the rust-lightning project is moving off of GitHub for day-to-day development.

You can still file issues and access the git tree here, but PRs will now take place exclusively at https://git.rust-bitcoin.org/. As such, this PR has been migrated to https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4665

If you log in using GitHub (or otherwise link your GitHub account from https://git.rust-bitcoin.org/user/settings/security), ownership of your PRs, issues, and comments will automatically transfer. To push updates to this PR, you'll need to use git push git@gitea-ssh.bitcoin.ninja:lightningdevkit/rust-lightning YOUR_LOCAL_COMMIT_OR_BRANCH:tnull/2026-06-async-persister-test-notifier. This may require a permissions change - if it doesn't work initially just leave a comment and we'll get you access.

[tnull]

Downstream tests need to exercise the same async monitor persistence path used by ChainMonitor::new_async_beta without reimplementing Persist. Add test-only constructors that let TestChainMonitor and AsyncPersister share the wake notifier so async completions drive the monitor update future.

Co-Authored-By: HAL 9000

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

[ldk-claude-review-bot]

I've re-examined the full PR including the surrounding code that the diff touches: the AsyncPersister struct and its Persist impl, new_async_beta (which the test-only path mirrors), and the Notifier wiring through with_event_notifier / with_deferred_and_event_notifier.

The notifier plumbing in the test-only constructors faithfully matches the production new_async_beta wiring (event_notifier: Arc::clone(...) shared between the ChainMonitor and the AsyncPersister). The cfg gating is consistent: new_with_event_notifier on both ChainMonitor and TestChainMonitor are gated on _test_utils, and the private with_deferred_and_event_notifier helper handles both compilation paths so no Some(notifier) can reach a non-test build. The Arc::strong_count assertions in the test (2 after new_test, 3 after new_with_event_notifier) are correct given the wiring.

No issues found.

[TheBlueMatt]

Hmm, its not entirely clear to me why this is needed? Can you spell out what kind of test you want to write and why it needs this? In general downstream tests (and, really, any tests) should just block the KVStore write they want to block, IMO.

[tnull]

Hmm, its not entirely clear to me why this is needed? Can you spell out what kind of test you want to write and why it needs this? In general downstream tests (and, really, any tests) should just block the KVStore write they want to block, IMO.

Yes see here: lightningdevkit/ldk-node#919 (comment)

This will allow us to do:

typeTestMonitorPersister<K> = MonitorUpdatingPersisterAsync<TestStoreRef<K>,TestFutureSpawner,&'static test_utils::TestLogger,&'static test_utils::TestKeysInterface,&'static test_utils::TestKeysInterface,&'static test_utils::TestBroadcaster,&'static test_utils::TestFeeEstimator,>;typeTestAsyncPersister<K> = lightning::chain::chainmonitor::AsyncPersister<TestStoreRef<K>,TestFutureSpawner,&'static test_utils::TestLogger,&'static test_utils::TestKeysInterface,&'static test_utils::TestKeysInterface,&'static test_utils::TestBroadcaster,&'static test_utils::TestFeeEstimator,>;(..)let monitor_persister = TestMonitorPersister::new(
store,TestFutureSpawner::new(Arc::clone(&runtime)),&chanmon_cfg.logger,
max_pending_updates,&chanmon_cfg.keys_manager,&chanmon_cfg.keys_manager,&chanmon_cfg.tx_broadcaster,&chanmon_cfg.fee_estimator,);let event_notifier = Arc::new(Notifier::new());let persister = lightning::chain::chainmonitor::AsyncPersister::new_test(
monitor_persister,Arc::clone(&event_notifier),);(...)test_utils::TestChainMonitor::new_with_event_notifier(Some(&chanmon_cfg.chain_source),&chanmon_cfg.tx_broadcaster,&chanmon_cfg.logger,&chanmon_cfg.fee_estimator,&persister.persister,&chanmon_cfg.keys_manager,Arc::clone(&persister.event_notifier),)

.. rather than duplicating code via our custom TestMonitorUpdatePersister we have in that PR branch currently (which doesn't actually exercise the MonitorUpdatingPersister we have in prod):

https://github.com/tnull/ldk-node/blob/9e095d154babc6a37ecbe8a967eceadf4bf4a3a6/src/io/test_utils.rs#L41-L158

[TheBlueMatt]

Are there some specific tests in LDK Node that need that struct vs just using the MonitorUpdatingPersister you use in prod and hooking any writes to pause/resume them at the KVStore layer?

[tnull]

Updated to line-wrap commit messages.

[TheBlueMatt]

Still left with the above question. I don't love exposing this if there's an alternative way to do it downstream.

[tnull]

Still left with the above question. I don't love exposing this if there's an alternative way to do it downstream.

Excuse the delay! We use this in our SqliteStore/PostgresStore tests, see https://github.com/tnull/ldk-node/blob/a3e7653bb41a43e4bb2e5b1c5f55bae0ee8d1880/src/io/test_utils.rs#L276, which is basically an async version of https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-persister/src/test_utils.rs#L162.

AFAIU, having a test-only path to allow constructing async-persisted variants of TestChainMonitors would also be required if we'd ever update our test coverage of the async-monitor path in general, but in particular also if we'd want to add coverage for the async path for the FilesystemStore tests, which seems like good end-to-end coverage to have (and of course eventually we'll want to upstream all of these stores to lightning-persister, so it would be good to reuse the same test utils / not have too much LDK Node custom code that we'll need to drop then anyways).

[TheBlueMatt]

I'm still confused. I can't try to play with this cause not sure what TestStoreRef and TestFutureSpawner are, but I don't see why you can't use new_async_beta and just build a normal ChainMonitor and use that. Why do you want the TestChainMonitor at all? And if you do, we could expose a way to build an async monitor in it (TestChainMonitor isn't really set up to support an underlying async monitor, though its certainly possible it mostly works).

[ldk-reviews-bot]

Hi @tnull,

Thanks for your contributions to rust-lightning!

After too many struggles with bugs, outages, contributor bans, and, finally, a multi-week CI ban, the rust-lightning project is moving off of GitHub for day-to-day development.

You can still file issues and access the git tree here, but PRs will now take place exclusively at https://git.rust-bitcoin.org/. As such, this PR has been migrated to https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4665

If you log in using GitHub (or otherwise link your GitHub account from https://git.rust-bitcoin.org/user/settings/security), ownership of your PRs, issues, and comments will automatically transfer. To push updates to this PR, you'll need to use git push git@gitea-ssh.bitcoin.ninja:lightningdevkit/rust-lightning YOUR_LOCAL_COMMIT_OR_BRANCH:tnull/2026-06-async-persister-test-notifier. This may require a permissions change - if it doesn't work initially just leave a comment and we'll get you access.

[tnull]

Downstream tests need to exercise the same async monitor persistence path used by ChainMonitor::new_async_beta without reimplementing Persist. Add test-only constructors that let TestChainMonitor and AsyncPersister share the wake notifier so async completions drive the monitor update future.

Co-Authored-By: HAL 9000

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

[ldk-claude-review-bot]

I've re-examined the full PR including the surrounding code that the diff touches: the AsyncPersister struct and its Persist impl, new_async_beta (which the test-only path mirrors), and the Notifier wiring through with_event_notifier / with_deferred_and_event_notifier.

The notifier plumbing in the test-only constructors faithfully matches the production new_async_beta wiring (event_notifier: Arc::clone(...) shared between the ChainMonitor and the AsyncPersister). The cfg gating is consistent: new_with_event_notifier on both ChainMonitor and TestChainMonitor are gated on _test_utils, and the private with_deferred_and_event_notifier helper handles both compilation paths so no Some(notifier) can reach a non-test build. The Arc::strong_count assertions in the test (2 after new_test, 3 after new_with_event_notifier) are correct given the wiring.

No issues found.

[TheBlueMatt]

Hmm, its not entirely clear to me why this is needed? Can you spell out what kind of test you want to write and why it needs this? In general downstream tests (and, really, any tests) should just block the KVStore write they want to block, IMO.

[tnull]

Hmm, its not entirely clear to me why this is needed? Can you spell out what kind of test you want to write and why it needs this? In general downstream tests (and, really, any tests) should just block the KVStore write they want to block, IMO.

Yes see here: lightningdevkit/ldk-node#919 (comment)

This will allow us to do:

typeTestMonitorPersister<K> = MonitorUpdatingPersisterAsync<TestStoreRef<K>,TestFutureSpawner,&'static test_utils::TestLogger,&'static test_utils::TestKeysInterface,&'static test_utils::TestKeysInterface,&'static test_utils::TestBroadcaster,&'static test_utils::TestFeeEstimator,>;typeTestAsyncPersister<K> = lightning::chain::chainmonitor::AsyncPersister<TestStoreRef<K>,TestFutureSpawner,&'static test_utils::TestLogger,&'static test_utils::TestKeysInterface,&'static test_utils::TestKeysInterface,&'static test_utils::TestBroadcaster,&'static test_utils::TestFeeEstimator,>;(..)let monitor_persister = TestMonitorPersister::new(
store,TestFutureSpawner::new(Arc::clone(&runtime)),&chanmon_cfg.logger,
max_pending_updates,&chanmon_cfg.keys_manager,&chanmon_cfg.keys_manager,&chanmon_cfg.tx_broadcaster,&chanmon_cfg.fee_estimator,);let event_notifier = Arc::new(Notifier::new());let persister = lightning::chain::chainmonitor::AsyncPersister::new_test(
monitor_persister,Arc::clone(&event_notifier),);(...)test_utils::TestChainMonitor::new_with_event_notifier(Some(&chanmon_cfg.chain_source),&chanmon_cfg.tx_broadcaster,&chanmon_cfg.logger,&chanmon_cfg.fee_estimator,&persister.persister,&chanmon_cfg.keys_manager,Arc::clone(&persister.event_notifier),)

.. rather than duplicating code via our custom TestMonitorUpdatePersister we have in that PR branch currently (which doesn't actually exercise the MonitorUpdatingPersister we have in prod):

https://github.com/tnull/ldk-node/blob/9e095d154babc6a37ecbe8a967eceadf4bf4a3a6/src/io/test_utils.rs#L41-L158

[TheBlueMatt]

Are there some specific tests in LDK Node that need that struct vs just using the MonitorUpdatingPersister you use in prod and hooking any writes to pause/resume them at the KVStore layer?

[tnull]

Updated to line-wrap commit messages.

[TheBlueMatt]

Still left with the above question. I don't love exposing this if there's an alternative way to do it downstream.

[tnull]

Still left with the above question. I don't love exposing this if there's an alternative way to do it downstream.

Excuse the delay! We use this in our SqliteStore/PostgresStore tests, see https://github.com/tnull/ldk-node/blob/a3e7653bb41a43e4bb2e5b1c5f55bae0ee8d1880/src/io/test_utils.rs#L276, which is basically an async version of https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-persister/src/test_utils.rs#L162.

AFAIU, having a test-only path to allow constructing async-persisted variants of TestChainMonitors would also be required if we'd ever update our test coverage of the async-monitor path in general, but in particular also if we'd want to add coverage for the async path for the FilesystemStore tests, which seems like good end-to-end coverage to have (and of course eventually we'll want to upstream all of these stores to lightning-persister, so it would be good to reuse the same test utils / not have too much LDK Node custom code that we'll need to drop then anyways).

[TheBlueMatt]

I'm still confused. I can't try to play with this cause not sure what TestStoreRef and TestFutureSpawner are, but I don't see why you can't use new_async_beta and just build a normal ChainMonitor and use that. Why do you want the TestChainMonitor at all? And if you do, we could expose a way to build an async monitor in it (TestChainMonitor isn't really set up to support an underlying async monitor, though its certainly possible it mostly works).

[ldk-reviews-bot]

Hi @tnull,

Thanks for your contributions to rust-lightning!

After too many struggles with bugs, outages, contributor bans, and, finally, a multi-week CI ban, the rust-lightning project is moving off of GitHub for day-to-day development.

You can still file issues and access the git tree here, but PRs will now take place exclusively at https://git.rust-bitcoin.org/. As such, this PR has been migrated to https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4665

If you log in using GitHub (or otherwise link your GitHub account from https://git.rust-bitcoin.org/user/settings/security), ownership of your PRs, issues, and comments will automatically transfer. To push updates to this PR, you'll need to use git push git@gitea-ssh.bitcoin.ninja:lightningdevkit/rust-lightning YOUR_LOCAL_COMMIT_OR_BRANCH:tnull/2026-06-async-persister-test-notifier. This may require a permissions change - if it doesn't work initially just leave a comment and we'll get you access.

[tnull]

Downstream tests need to exercise the same async monitor persistence path used by ChainMonitor::new_async_beta without reimplementing Persist. Add test-only constructors that let TestChainMonitor and AsyncPersister share the wake notifier so async completions drive the monitor update future.

Co-Authored-By: HAL 9000

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

[ldk-claude-review-bot]

I've re-examined the full PR including the surrounding code that the diff touches: the AsyncPersister struct and its Persist impl, new_async_beta (which the test-only path mirrors), and the Notifier wiring through with_event_notifier / with_deferred_and_event_notifier.

The notifier plumbing in the test-only constructors faithfully matches the production new_async_beta wiring (event_notifier: Arc::clone(...) shared between the ChainMonitor and the AsyncPersister). The cfg gating is consistent: new_with_event_notifier on both ChainMonitor and TestChainMonitor are gated on _test_utils, and the private with_deferred_and_event_notifier helper handles both compilation paths so no Some(notifier) can reach a non-test build. The Arc::strong_count assertions in the test (2 after new_test, 3 after new_with_event_notifier) are correct given the wiring.

No issues found.

[TheBlueMatt]

Hmm, its not entirely clear to me why this is needed? Can you spell out what kind of test you want to write and why it needs this? In general downstream tests (and, really, any tests) should just block the KVStore write they want to block, IMO.

[tnull]

Hmm, its not entirely clear to me why this is needed? Can you spell out what kind of test you want to write and why it needs this? In general downstream tests (and, really, any tests) should just block the KVStore write they want to block, IMO.

Yes see here: lightningdevkit/ldk-node#919 (comment)

This will allow us to do:

typeTestMonitorPersister<K> = MonitorUpdatingPersisterAsync<TestStoreRef<K>,TestFutureSpawner,&'static test_utils::TestLogger,&'static test_utils::TestKeysInterface,&'static test_utils::TestKeysInterface,&'static test_utils::TestBroadcaster,&'static test_utils::TestFeeEstimator,>;typeTestAsyncPersister<K> = lightning::chain::chainmonitor::AsyncPersister<TestStoreRef<K>,TestFutureSpawner,&'static test_utils::TestLogger,&'static test_utils::TestKeysInterface,&'static test_utils::TestKeysInterface,&'static test_utils::TestBroadcaster,&'static test_utils::TestFeeEstimator,>;(..)let monitor_persister = TestMonitorPersister::new(
store,TestFutureSpawner::new(Arc::clone(&runtime)),&chanmon_cfg.logger,
max_pending_updates,&chanmon_cfg.keys_manager,&chanmon_cfg.keys_manager,&chanmon_cfg.tx_broadcaster,&chanmon_cfg.fee_estimator,);let event_notifier = Arc::new(Notifier::new());let persister = lightning::chain::chainmonitor::AsyncPersister::new_test(
monitor_persister,Arc::clone(&event_notifier),);(...)test_utils::TestChainMonitor::new_with_event_notifier(Some(&chanmon_cfg.chain_source),&chanmon_cfg.tx_broadcaster,&chanmon_cfg.logger,&chanmon_cfg.fee_estimator,&persister.persister,&chanmon_cfg.keys_manager,Arc::clone(&persister.event_notifier),)

.. rather than duplicating code via our custom TestMonitorUpdatePersister we have in that PR branch currently (which doesn't actually exercise the MonitorUpdatingPersister we have in prod):

https://github.com/tnull/ldk-node/blob/9e095d154babc6a37ecbe8a967eceadf4bf4a3a6/src/io/test_utils.rs#L41-L158

[TheBlueMatt]

Are there some specific tests in LDK Node that need that struct vs just using the MonitorUpdatingPersister you use in prod and hooking any writes to pause/resume them at the KVStore layer?

[tnull]

Updated to line-wrap commit messages.

[TheBlueMatt]

Still left with the above question. I don't love exposing this if there's an alternative way to do it downstream.

[tnull]

Still left with the above question. I don't love exposing this if there's an alternative way to do it downstream.

Excuse the delay! We use this in our SqliteStore/PostgresStore tests, see https://github.com/tnull/ldk-node/blob/a3e7653bb41a43e4bb2e5b1c5f55bae0ee8d1880/src/io/test_utils.rs#L276, which is basically an async version of https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-persister/src/test_utils.rs#L162.

AFAIU, having a test-only path to allow constructing async-persisted variants of TestChainMonitors would also be required if we'd ever update our test coverage of the async-monitor path in general, but in particular also if we'd want to add coverage for the async path for the FilesystemStore tests, which seems like good end-to-end coverage to have (and of course eventually we'll want to upstream all of these stores to lightning-persister, so it would be good to reuse the same test utils / not have too much LDK Node custom code that we'll need to drop then anyways).

[TheBlueMatt]

I'm still confused. I can't try to play with this cause not sure what TestStoreRef and TestFutureSpawner are, but I don't see why you can't use new_async_beta and just build a normal ChainMonitor and use that. Why do you want the TestChainMonitor at all? And if you do, we could expose a way to build an async monitor in it (TestChainMonitor isn't really set up to support an underlying async monitor, though its certainly possible it mostly works).

[ldk-reviews-bot]

Hi @tnull,

Thanks for your contributions to rust-lightning!

After too many struggles with bugs, outages, contributor bans, and, finally, a multi-week CI ban, the rust-lightning project is moving off of GitHub for day-to-day development.

You can still file issues and access the git tree here, but PRs will now take place exclusively at https://git.rust-bitcoin.org/. As such, this PR has been migrated to https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4665

If you log in using GitHub (or otherwise link your GitHub account from https://git.rust-bitcoin.org/user/settings/security), ownership of your PRs, issues, and comments will automatically transfer. To push updates to this PR, you'll need to use git push git@gitea-ssh.bitcoin.ninja:lightningdevkit/rust-lightning YOUR_LOCAL_COMMIT_OR_BRANCH:tnull/2026-06-async-persister-test-notifier. This may require a permissions change - if it doesn't work initially just leave a comment and we'll get you access.

[tnull]

Downstream tests need to exercise the same async monitor persistence path used by ChainMonitor::new_async_beta without reimplementing Persist. Add test-only constructors that let TestChainMonitor and AsyncPersister share the wake notifier so async completions drive the monitor update future.

Co-Authored-By: HAL 9000

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

[ldk-claude-review-bot]

I've re-examined the full PR including the surrounding code that the diff touches: the AsyncPersister struct and its Persist impl, new_async_beta (which the test-only path mirrors), and the Notifier wiring through with_event_notifier / with_deferred_and_event_notifier.

The notifier plumbing in the test-only constructors faithfully matches the production new_async_beta wiring (event_notifier: Arc::clone(...) shared between the ChainMonitor and the AsyncPersister). The cfg gating is consistent: new_with_event_notifier on both ChainMonitor and TestChainMonitor are gated on _test_utils, and the private with_deferred_and_event_notifier helper handles both compilation paths so no Some(notifier) can reach a non-test build. The Arc::strong_count assertions in the test (2 after new_test, 3 after new_with_event_notifier) are correct given the wiring.

No issues found.

[TheBlueMatt]

Hmm, its not entirely clear to me why this is needed? Can you spell out what kind of test you want to write and why it needs this? In general downstream tests (and, really, any tests) should just block the KVStore write they want to block, IMO.

[tnull]

Hmm, its not entirely clear to me why this is needed? Can you spell out what kind of test you want to write and why it needs this? In general downstream tests (and, really, any tests) should just block the KVStore write they want to block, IMO.

Yes see here: lightningdevkit/ldk-node#919 (comment)

This will allow us to do:

typeTestMonitorPersister<K> = MonitorUpdatingPersisterAsync<TestStoreRef<K>,TestFutureSpawner,&'static test_utils::TestLogger,&'static test_utils::TestKeysInterface,&'static test_utils::TestKeysInterface,&'static test_utils::TestBroadcaster,&'static test_utils::TestFeeEstimator,>;typeTestAsyncPersister<K> = lightning::chain::chainmonitor::AsyncPersister<TestStoreRef<K>,TestFutureSpawner,&'static test_utils::TestLogger,&'static test_utils::TestKeysInterface,&'static test_utils::TestKeysInterface,&'static test_utils::TestBroadcaster,&'static test_utils::TestFeeEstimator,>;(..)let monitor_persister = TestMonitorPersister::new(
store,TestFutureSpawner::new(Arc::clone(&runtime)),&chanmon_cfg.logger,
max_pending_updates,&chanmon_cfg.keys_manager,&chanmon_cfg.keys_manager,&chanmon_cfg.tx_broadcaster,&chanmon_cfg.fee_estimator,);let event_notifier = Arc::new(Notifier::new());let persister = lightning::chain::chainmonitor::AsyncPersister::new_test(
monitor_persister,Arc::clone(&event_notifier),);(...)test_utils::TestChainMonitor::new_with_event_notifier(Some(&chanmon_cfg.chain_source),&chanmon_cfg.tx_broadcaster,&chanmon_cfg.logger,&chanmon_cfg.fee_estimator,&persister.persister,&chanmon_cfg.keys_manager,Arc::clone(&persister.event_notifier),)

.. rather than duplicating code via our custom TestMonitorUpdatePersister we have in that PR branch currently (which doesn't actually exercise the MonitorUpdatingPersister we have in prod):

https://github.com/tnull/ldk-node/blob/9e095d154babc6a37ecbe8a967eceadf4bf4a3a6/src/io/test_utils.rs#L41-L158

[TheBlueMatt]

Are there some specific tests in LDK Node that need that struct vs just using the MonitorUpdatingPersister you use in prod and hooking any writes to pause/resume them at the KVStore layer?

[tnull]

Updated to line-wrap commit messages.

[TheBlueMatt]

Still left with the above question. I don't love exposing this if there's an alternative way to do it downstream.

[tnull]

Still left with the above question. I don't love exposing this if there's an alternative way to do it downstream.

Excuse the delay! We use this in our SqliteStore/PostgresStore tests, see https://github.com/tnull/ldk-node/blob/a3e7653bb41a43e4bb2e5b1c5f55bae0ee8d1880/src/io/test_utils.rs#L276, which is basically an async version of https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-persister/src/test_utils.rs#L162.

AFAIU, having a test-only path to allow constructing async-persisted variants of TestChainMonitors would also be required if we'd ever update our test coverage of the async-monitor path in general, but in particular also if we'd want to add coverage for the async path for the FilesystemStore tests, which seems like good end-to-end coverage to have (and of course eventually we'll want to upstream all of these stores to lightning-persister, so it would be good to reuse the same test utils / not have too much LDK Node custom code that we'll need to drop then anyways).

[TheBlueMatt]

I'm still confused. I can't try to play with this cause not sure what TestStoreRef and TestFutureSpawner are, but I don't see why you can't use new_async_beta and just build a normal ChainMonitor and use that. Why do you want the TestChainMonitor at all? And if you do, we could expose a way to build an async monitor in it (TestChainMonitor isn't really set up to support an underlying async monitor, though its certainly possible it mostly works).

[ldk-reviews-bot]

Hi @tnull,

Thanks for your contributions to rust-lightning!

After too many struggles with bugs, outages, contributor bans, and, finally, a multi-week CI ban, the rust-lightning project is moving off of GitHub for day-to-day development.

You can still file issues and access the git tree here, but PRs will now take place exclusively at https://git.rust-bitcoin.org/. As such, this PR has been migrated to https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4665

If you log in using GitHub (or otherwise link your GitHub account from https://git.rust-bitcoin.org/user/settings/security), ownership of your PRs, issues, and comments will automatically transfer. To push updates to this PR, you'll need to use git push git@gitea-ssh.bitcoin.ninja:lightningdevkit/rust-lightning YOUR_LOCAL_COMMIT_OR_BRANCH:tnull/2026-06-async-persister-test-notifier. This may require a permissions change - if it doesn't work initially just leave a comment and we'll get you access.