cbf: make sync_wallets wait for applied tip - #32
Conversation
There was a problem hiding this comment.
I'm not sure that I understood correctly some parts of PR.
Pending blocks: why do we need them, we download and apply them sequentially.
(also if we do download them in parallel "in background", we'd need some way to apply them, so we don't end up in the situation like this:
- we queue block 101 (it downloads in background), try apply pending blocks, but nothing to apply
- we queue block 102 in the same way
- and at height of 102 we reach the tip, but we'll have no event to trigger the block application.
This we need to try apply blocks the moment we receive it (not the moment we queue them).
Also it seems we need to alter "failed sync" state. Bitcoind chain source indefinitely retries in background as far as I understood and wait_until_synced could for example fail if we hit some timeout. Maybe we don't need "failed" state at all.
Sorry for several reviews, I'm exploring github's UX...
| @@ -114,53 +135,130 @@ | |||
| match op { | |||
| ChainOp::ConnectFull { block_rx } => match block_rx.await { | |||
There was a problem hiding this comment.
We still wait for the block (make the actual network request) here. This means we cannot really have a race condition and blocks arrive in order.
| .expect("lock") | ||
| .insert(ib.height, (ib.block.block_hash(), fee_rate)); | ||
| } | ||
| self.queue_block(ib.height, PendingBlock::Full(ib)).await; |
There was a problem hiding this comment.
queue_block is added to remove code repeat present like height check in arms for filtered/full blocks, right?
| ); | ||
| } | ||
| self.pending.insert(height, block); | ||
| self.apply_pending_blocks().await; |
There was a problem hiding this comment.
I'd just paste body of apply_pending_blocks() here instead of having a distinct function. IMO additional functions decrease readability (if you're a person viewing the code for the first time).
| } | ||
| } | ||
| async fn mark_synced(&mut self, tip_height: u32) { |
There was a problem hiding this comment.
Same, I'd move the body to the only call of this method.
| } | ||
| } | ||
| async fn queue_block(&mut self, height: u32, block: PendingBlock) { |
There was a problem hiding this comment.
Blocks are not queued in this function. I'd change its name.
| //reorganized, and we retrieve indeed the same block header that we | ||
| //received `IndexedFilter` event of. right now this would block | ||
| //the further sync, as we cannot apply blocks in order. | ||
| //Future solution would use something like `get_header_by_hash`. |
There was a problem hiding this comment.
as per discussions here: 2140-dev/kyoto#585 we're okay with the current setup
| } | ||
| } | ||
| fn publish_sync_failed(&self, error: Error) { |
There was a problem hiding this comment.
I'd remove this one line function.
| }, | ||
| Err(_) => { | ||
| log_error!(self.logger, "block oneshot dropped"); | ||
| self.publish_sync_failed(Error::TxSyncFailed); |
There was a problem hiding this comment.
An error in block fetch breaks everything. We cannot process the next event, as height would differ.
It seems we could just retry several times. I think it's more than a common situation when we request a full block from a peer to not receive because of something. We could just silently retry several times.
| if let Ok(handle) = requester.request_block(block_hash) { | ||
| ChainOp::ConnectFull { block_rx: handle } | ||
| } else { | ||
| log_error!( |
There was a problem hiding this comment.
Actually it's not a result of fetch, it is a result of taking "handle to the block receiver" from kyoto. It can fail only if the node stopped (per kyoto's docs: https://docs.rs/bip157/latest/bip157/struct.Requester.html#method.request_block)
1e5e3fc to
f9ba605CompareTrack the highest CBF tip whose chain operations have been applied and make manual wallet sync wait for that application-level completion. Keep block application sequential, retry matched block downloads before failing sync, and treat request_block handle failures as a stopped CBF node. AI-Tool: OpenAI Codex
Track the highest CBF tip whose chain operations have been applied and make manual wallet sync wait for that application-level completion.