Consistently return NotRunning from live-node APIs

[ajaysehwal]

Related to #1141.

Several public APIs that need a live node didn't check the running state. For example, OnchainPayment::bump_fee_rbf could reach wallet mutation and signing after Node::stop(), while the on-chain send methods returned Error::NotRunning.

NotRunning is now returned as the first step of the following API methods, before any validation or side effects:

• On-chain: OnchainPayment::bump_fee_rbf
• Channels: open_channel, open_announced_channel, open_0reserve_channel and their *_with_all variants (so the announcement-configuration validation no longer runs first), splice_in, splice_in_with_all, splice_out, bump_channel_funding_fee, close_channel, force_close_channel, update_channel_config
• BOLT11: claim_for_id, fail_for_id, and the receive_*via_jit_channel* methods
• BOLT12: initiate_refund, receive_async, and set_paths_to_static_invoice_server (both the Rust and the UniFFI variants). The latter two update state that LDK persists via the background processor, which is no longer running after stop().
• Liquidity: Liquidity::add_liquidity_source, and LSPS1 request_channel / check_order_status (the running state is now plumbed into both handlers)
• Unified: UnifiedPayment::send returns NotRunning before any URI parsing or HRN resolution, instead of falling back to the next payment method

The checks use the same inline pattern as the existing ones and are placed in the public methods rather than in shared helpers.

Methods that intentionally work offline are left as is: address generation, BOLT11/BOLT12 invoice and offer creation (these only derive data from keys, including the *_for_hash variants), listings, balances, signing and verification, remove_payment, forwarding analytics, and lnurl_auth.

The TOCTOU race and the broader start/stop lifecycle handling are out of scope here, as discussed.

Testing

• unstarted_node_rejects_live_node_apis (unit test): the newly checked methods return NotRunning on a node that isn't running, including the announced-channel, receive_async, set_paths_to_static_invoice_server and UnifiedPayment::send cases. It fails on the previous implementation (e.g., bump_fee_rbf returned InvalidPaymentId, open_announced_channel returned ChannelCreationFailed) and passes with this change, with and without the uniffi feature.
• live_node_apis_return_not_running_after_stop (integration test): calls the live-node APIs after stop(), restarts the node, and checks that it is fully functional again by syncing and sending on-chain.
• The full lib test suite passes, as do cargo +nightly fmt --check and cargo clippy --lib -- -D clippy::unwrap_used.

AI disclosure

This change was prepared with the assistance of Claude Code.

[ldk-reviews-bot]

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

[ajaysehwal]

Hi @tnull, this is ready for a look. I did a self-review pass after opening it and pushed a follow-up commit (Flush queued broadcasts on stop and test the stop race). Summary of where it stands:

What the PR does

• Adds the missing NotRunning checks before any side effects (RBF bump, splice in/out, channel close/force-close/config, BOLT11 claim/fail and JIT receive, BOLT12 refunds, LSPS1, add_liquidity_source). Offline-capable methods are untouched.
• Closes the TOCTOU race by having mutating calls hold a read guard on is_running, so stop() (which takes the write lock) can't complete while one is in flight.

What the follow-up adds

• The guard alone wasn't enough: a tx queued by a send just before stop() was still dropped, because the broadcast processor was a cancellable task that exited on the stop signal. It is now a regular background task that flushes everything queued before the stop signal (bounded by the existing background task shutdown timeout).
• A deterministic regression test: a store that blocks the payment write, asserting that stop() doesn't return while the call is in flight and that later calls return NotRunning. It fails without the guard.
• A CHANGELOG note and a doc note on Node::stop.

Where I'd appreciate your steer

1. stop() waiting for in-flight calls. A call stuck in a slow store write now delays stop() (and, while the writer is pending, other API calls block on the read lock too). I left it unbounded since a timeout would reopen the race for exactly the stuck calls. Are you fine with that, or would you prefer a bounded wait or a different mechanism (e.g., an in-flight counter)?
2. Async unified on-chain fallback. UnifiedPayment::send is async and can't hold a std read guard across .await, so its on-chain fallback is checked right before sending but not exclusive with stop(). Acceptable for this PR, or should I address it (needs a different primitive)?
3. Invoice/offer creation after stop. receive*_for_hash still work after stop(), but they create pending inbound payment state that won't be persisted. I read "preserve methods that intentionally support offline use" as covering invoice creation, but should the _for_hash variants require a running node?
4. Pre-start behaviour change. initiate_refund, claim_for_id/fail_for_id and the splice/close/config methods now return NotRunning before start() too. I believe that's the intent of the issue (and add_liquidity_source/JIT receive failed pre-start anyway), but please confirm, since it's user-visible.
5. Test coverage. The post-restart integration test needs bitcoind/electrs, so I've only compiled it locally and am relying on CI for it. The race and flush behaviour are covered by unit tests that run without a chain backend.

Happy to split the broadcast-queue flush into its own PR if you'd rather review it separately.

[tnull]

Thanks for taking a look!

No, I think for now we should really just leave it at what's described in #1141, i.e., return NotRunning for the API methods that are missing it.

We want eventually to do proper start/stop handling that also covers node restarts on persistence failures, etc. But this is out-of-scope for now.

Btw, note that the chosen approach in this PR currently introduces a lot of complexity and risks, e.g., it makes the Node prone to deadlocks on shutdown.

[ajaysehwal]

Thanks for taking a look!

No, I think for now we should really just leave it at what's described in #1141, i.e., return NotRunning for the API methods that are missing it.

We want eventually to do proper start/stop handling that also covers node restarts on persistence failures, etc. But this is out-of-scope for now.

Btw, note that the chosen approach in this PR currently introduces a lot of complexity and risks, e.g., it makes the Node prone to deadlocks on shutdown.

Thanks for the feedback, and fair point on the complexity and the shutdown deadlock risk. I've cut the PR back to what #1141 describes: it now only adds the missing NotRunning checks (using the same inline pattern as the existing ones), with no guards, no stop() changes and no changes to the broadcast queue. It's a single commit now (force-pushed).

What's left:

• NotRunning before any side effects for bump_fee_rbf, the splice/close/force-close/channel config methods, claim_for_id/fail_for_id, the JIT receive methods, initiate_refund, add_liquidity_source and the LSPS1 methods (the running state is plumbed into Liquidity/LSPS1Liquidity for that).
• UnifiedPayment::send returns NotRunning instead of falling back to the next payment method.
• A unit test for the not-running state and an integration test for calls after stop() and normal operation after a restart, plus a changelog note.

I've left the TOCTOU race and the start/stop lifecycle handling for the broader start/stop rework you mentioned.

[tnull]

CI is failing it seems.

[ajaysehwal]

CI is failing it seems.

Pushed a small update (still a single commit): the previous revision failed cargo check --all-features --tests, since a test built a BOLT12 offer in a way that doesn't compile with the uniffi types. I dropped that assertion, as the BOLT12 send check isn't part of this change anyway.

Note that onchain_wallet_full_scan_stop_gap_recovers_far_esplora_and_electrum_funds fails on the macOS and self-hosted jobs. It isn't touched by this PR and fails on main as well, and it's what #1154 fixes. The rest of the integration suite, including the new live_node_apis_return_not_running_after_stop, passed there.

Ready for another look whenever you have time.

[tnull]

It seems there are still gaps. Codex:

• Bolt12Payment::receive_async() has no guard and changes persistent state. bolt12.rs:646 (/home/tnull/workspace/ldk-node-review-pr-1160-F3A8wr/src/payment/bolt12.rs:646) calls LDK’s get_async_receive_offer(), which marks a fresh offer as used and signals persistence. After stop(), the
background processor has stopped, so that change will not be persisted. Dropping and rebuilding the node can lose the marker, allowing an offer already handed to the application to be rotated out later. This deserves a running check or explicit synchronous persistence for offline use.

• Bolt12Payment::set_paths_to_static_invoice_server() has no guard in either Rust or UniFFI. bolt12.rs:663 (/home/tnull/workspace/ldk-node-review-pr-1160-F3A8wr/src/payment/bolt12.rs:663), bolt12.rs:695 (/home/tnull/workspace/ldk-node-review-pr-1160-F3A8wr/src/payment/bolt12.rs:695). LDK
updates its cached server paths and signals persistence; it also attempts to initiate offer setup when peers are connected. After shutdown, valid paths can return Ok(()) while leaving the change unpersisted. Offline configuration needs an explicit persistence path if it is intended.

• Both announced-channel opening methods check too late. lib.rs:1587 (/home/tnull/workspace/ldk-node-review-pr-1160-F3A8wr/src/lib.rs:1587), lib.rs:1664 (/home/tnull/workspace/ldk-node-review-pr-1160-F3A8wr/src/lib.rs:1664). They validate announcement configuration before reaching the guard
in open_channel_inner(). An unstarted node without the required configuration therefore returns ChannelCreationFailed, bypassing NotRunning. This performs validation and logging, but no node-state mutation.

[ajaysehwal]

It seems there are still gaps. Codex:

• Bolt12Payment::receive_async() has no guard and changes persistent state. bolt12.rs:646 (/home/tnull/workspace/ldk-node-review-pr-1160-F3A8wr/src/payment/bolt12.rs:646) calls LDK’s get_async_receive_offer(), which marks a fresh offer as used and signals persistence. After stop(), the
background processor has stopped, so that change will not be persisted. Dropping and rebuilding the node can lose the marker, allowing an offer already handed to the application to be rotated out later. This deserves a running check or explicit synchronous persistence for offline use.
• Bolt12Payment::set_paths_to_static_invoice_server() has no guard in either Rust or UniFFI. bolt12.rs:663 (/home/tnull/workspace/ldk-node-review-pr-1160-F3A8wr/src/payment/bolt12.rs:663), bolt12.rs:695 (/home/tnull/workspace/ldk-node-review-pr-1160-F3A8wr/src/payment/bolt12.rs:695). LDK
updates its cached server paths and signals persistence; it also attempts to initiate offer setup when peers are connected. After shutdown, valid paths can return Ok(()) while leaving the change unpersisted. Offline configuration needs an explicit persistence path if it is intended.
• Both announced-channel opening methods check too late. lib.rs:1587 (/home/tnull/workspace/ldk-node-review-pr-1160-F3A8wr/src/lib.rs:1587), lib.rs:1664 (/home/tnull/workspace/ldk-node-review-pr-1160-F3A8wr/src/lib.rs:1664). They validate announcement configuration before reaching the guard
in open_channel_inner(). An unstarted node without the required configuration therefore returns ChannelCreationFailed, bypassing NotRunning. This performs validation and logging, but no node-state mutation.

@tnull Thanks, good catches. Added the running check as the first step in receive_async, both variants of set_paths_to_static_invoice_server, and all six open_*channel* methods (so the announced-channel validation no longer runs first). While re-auditing, I also covered the two BOLT11 _for_hash receive methods, as they register a pending inbound payment in the same way. The unit test now asserts NotRunning for all of these on a stopped node.