Repository navigation
Reconcile monitor tips before starting peers, sync Lightning wallets on startup - #1161
Conversation
Bitcoin Core listener initialization and continuous polling share one background task, making changes to their startup ordering harder to review independently. Extract initialization while keeping it in the same background task. Preserve retry, cancellation, and polling behavior so startup ordering can change separately. Co-Authored-By: HAL 9000
|
👋 Thanks for assigning @joostjager as a reviewer! |
9c82dc2 to
77bbd1d
Compare
Bitcoin Core synchronized restored chain listeners in a background loop after startup. Peers and event processing could run while those listeners were still catching up, and sync failures only retried. Wait for initial synchronization before starting peers and event processing. Return synchronization errors to the caller so a failed start can shut down cleanly and be retried. Co-Authored-By: HAL 9000
Bitcoin Core startup synchronized all restored channel monitors from the lowest saved tip. A monitor saved farther ahead on an orphaned fork could miss disconnects and retain stale confirmations. Reconcile each monitor from its own current locator while peers and event processing remain stopped. Cover divergent persisted tips and retries after partial synchronization with regression tests. Fixes lightningdevkit#1076 Reported-by: Project Loupe / instagibbs Co-Authored-By: HAL 9000
Esplora and Electrum started peers and event processing before their first Lightning synchronization. With background sync disabled, startup could leave restored Lightning state stale indefinitely. Synchronize the Lightning wallet during initialization for both chain sources. Leave BDK synchronization to background or manual syncing and cover initial startup and restart against both backends. Co-Authored-By: HAL 9000
77bbd1d to
6424fb5
Compare
| output_sweeper, | ||
| &self.tx_broadcaster, | ||
| ), | ||
| source.update_fee_rate_estimates(), |
There was a problem hiding this comment.
We have duplicate code for all the chain sources. Can it be simplified?
There was a problem hiding this comment.
Not sure what you have in mind, but I find it pretty readable?
| } | ||
| } | ||
|
|
||
| async fn synchronize_chain_listeners<B: BlockSource>( |
There was a problem hiding this comment.
Maybe it's better to make a change upstream, in rust-lightning?
There was a problem hiding this comment.
We could, but we want to fix this issue for the upcoming release (based on LDK 0.3) so an upstream change won't make it in time.
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
I’d leave the final fee-initialization commit out of this PR. It’s an independent optimization, and concurrent replay may construct claims before fee estimates are loaded.
The sync policy also feels inconsistent: we refuse to start when sync fails, but keep running when we lose sync later. Reconciling restored state is necessary; requiring fresh chain data before starting peers seems like a separate policy decision.
At the risk of sounding like a broken record, I still think independent monitor persistence is doing more harm than good. This PR adds more recovery complexity to accommodate it, with questionable benefit to users.
| Arc::clone(&self.channel_manager), | ||
| Arc::clone(&self.chain_monitor), | ||
| Arc::clone(&self.output_sweeper), | ||
| ))?; |
There was a problem hiding this comment.
Is it intentional that failure to sync here now exits the process whereas previously it would retry? That seems to be a direction different from adding more lifecycle to the node (which we discussed in the context of db leases)
There was a problem hiding this comment.
Yes, that's intentional and needed if we want to ensure nodes won't start unless synced to the latest chain state. Yes, if once we have node lifecycle management we can simply retry restarting the node indefinitely, but right now this seems the right choice if we now wan to address this issue (or rather reconsider our prior choice).
There was a problem hiding this comment.
Yes, I get that it is a practical choice.
| ChainSourceKind::Bitcoind(source) => { | ||
| source | ||
| .initialize_wallets( | ||
| tokio::try_join!( |
There was a problem hiding this comment.
[P2] Load fee estimates before replaying chain data (src/chain/mod.rs:323). The final commit runs synchronization concurrently with fee initialization. Monitor callbacks can construct and queue claim transactions during synchronization, using fallback fees if the fee request finishes later. Waiting for both operations before starting peers cannot correct those already constructed transactions.
There was a problem hiding this comment.
Hmpf, I intentionally tried to avoid the extra RTT here, but the LLM is right unfortunately, will revert.
Startup separately awaited fee estimates and backend capability checks before initializing wallets. These checks must complete before replay can construct claim transactions using the fee estimator. Move the checks into wallet initialization while preserving their ordering. Run fee refresh and required 0FC validation concurrently, then synchronize chain state only after both succeed. Co-Authored-By: HAL 9000
6424fb5 to
fc380fd
Compare
fc380fd to
f552c4b
Compare
Considered that, but I think doing the fee rate update in the wallet initialization step is preferable, for one it keeps related logic together, and it saves a
Yes, though without full lifecycle management there is little we can do beyond refusing on startup, and encountering stale chain state is somewhat expected if a node would have been stopped for a extended period of time.
Related but orthogonal. |
4 participants
Fixes #29.
Fixes #1076.
In this PR we fix a few bugs in our chain syncing logic. In particular we individually sync monitors (disconnecting stale blocks) before giving them all to the chain listener on startup. This now also has the node block until the initial reconciliation is completed.
Relatedly, but somewhat orthogonal we now also have the Esplora/Electrum chain sources block on the initial sync of the lightning wallet, to ensure we never start a node with stale chain information.