Cbf fix block fetch#34
Conversation
Previously we did not track the `FiltersSynced` kyoto event, so we could not tell when we had applied all blocks up to the tip. For example, when we stop and restart the node, kyoto's tip is 0 at the instant of start (it does not persist its chain), so our applied height trivially matches kyoto's tip and we would falsely conclude we had reached it. That is only actually true once we have received `FiltersSynced`.
`block_connected` applied the block and then called `block_on(persist_async)` while still holding the wallet mutex (`self.inner`) — the old `persist_async` was a method on the locked wallet, so the lock was structurally required. `Runtime::block_on` parks the current worker via `block_in_place` while driving the persist future. Holding the blocking (std) wallet lock across that park means any other task touching the wallet blocks the thread for the whole persist I/O; on a single-worker runtime that can wedge the block-application pipeline, and it widens the window for the documented `block_on` I/O-starvation deadlock. Apply the block under the lock, `take_staged()` the changeset, drop the lock, then persist the owned changeset via `AsyncWalletPersister::persist`. We still block on the persist before returning (block N durable before N+1 is applied); a reader observing applied-but-unflushed state is safe because a crash reverts to the last persisted checkpoint. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Now the function is called `list_watched_scripts`.
Previously stalled fetch would hang indefinitely.
| } | ||
| self.chain_listener.block_connected(&ib.block, ib.height); | ||
| self.next_height += 1; | ||
| self.mark_syncing(); |
There was a problem hiding this comment.
Could we mark the state as syncing as soon as a new header/filter is observed, before awaiting the block fetch? As written, synced_to_tip remains true while a matched block is being fetched, so sync_wallets() can still return before that known block is applied.
There was a problem hiding this comment.
What if block fetch fails in the end? We would not apply it all, but sync_wallets() would have returned.
There was a problem hiding this comment.
I think we're concerned about the same case. My suggestion is to set synced_to_tip to false as soon as an IndexedFilter is received, before awaiting request_block(), rather than only after ConnectFull has been applied. Then sync_wallets() will wait while the fetch is pending. If the fetch ultimately fails, the existing ChainOp::Failed(Error::TxSyncFailed) path will return an error; if it succeeds, synced_to_tip will become true again only after the block has been applied and Synced is processed.
Please let me know if I am missing the point!
| // now-durable delta or has already been flushed and cleared by a concurrent writer — | ||
| // we never drop unpersisted changes. | ||
| Ok(_) => { | ||
| let _ = self.inner.lock().expect("lock").take_staged(); |
There was a problem hiding this comment.
Could we drop this workaround and depend on lightningdevkit#980 instead? Once the wallet lock is released, another operation can add to the staged changeset, and this unconditional take_staged() may clear changes that were not persisted by this call.
There was a problem hiding this comment.
yeah, i'll try to cherry-pick upstream solution
2 participants
This PR fixes several bugs:
Previously we did not understand whether we really reached the tip: for example on restart of the node the tip from kyoto would be 0, so
sync_walletswould incorrectly return without really waiting for the sync. Right now we only are synced when we have receivedFiltersSyncedkyoto event.Wallet persistence sometime deadlock which affected our setup. While it will be fixed in a more general way in future (Wallet mutex held across
Runtime::block_on(persist_async)can deadlock the entire node runtime lightningdevkit/ldk-node#978 and Avoid wallet persistence deadlocks lightningdevkit/ldk-node#980) I decided to leave it here in place to distinguish tests failing because of that and other failing ones.Added lookahead addresses to
list_revealed_scriptsin wallet and changed its name tolist_watched_scripts, as on a fresh run I encountered missed payments during sync because of that.Added timeout to block fetch (same for the fee fetch and just block fetch for block data).