Follow reorgs with bitcoind REST - #1022
Conversation
Bitcoin Core's REST headers endpoint cannot serve headers from a stale branch. This prevented REST-backed nodes from finding a common ancestor and following the replacement chain after a reorg. In lightningdevkit#1006 we enabled bitcoind REST chain source support in CI, which exposed this failure. Co-Authored-By: HAL 9000
|
👋 Thanks for assigning @joostjager as a reviewer! |
|
Hmm, that's somewhat surprising. Can you file an issue on https://github.com/bitcoin/bitcoin as well? |
Will do. |
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
I wonder why we need a chain backend to find a common ancestor. What happens if someone switches to a different backend that doesn't know about the stale branch at all?
| BitcoindClient::Rest { rest_client, rpc_client, .. } => { | ||
| match rest_client.get_header(header_hash, height_hint).await { | ||
| Err(e) if e.kind() == BlockSourceErrorKind::Persistent => { | ||
| rpc_client.get_header(header_hash, height_hint).await |
There was a problem hiding this comment.
It looks a bit weird that we have to fallback to the other interface. From history it seems we need rest because rpc is too slow for large sync ops?
There was a problem hiding this comment.
REST was added both for its unauthenticated read-only interface and for more efficient/cacheable binary block retrieval; it isn’t simply that RPC is unusably slow.
There was a problem hiding this comment.
Is the unauth interface an advantage if you also may need rpc?
There was a problem hiding this comment.
It is to enable easier caching, but see your point.
Good question. We mostly need the backend to resolve the stale branch here because we accidentally stopped retaining the header cache in #874 (though note after restart that cache is also empty, so the fallback still makes sense anyways). Before that change / lightningdevkit/rust-lightning#4266, I’ll see to restore cache reuse by retaining the Switching backends happens across a restart. In that case, initial synchronization uses the recent hashes stored in |
|
Now updated to preserve the |
| result => result, | ||
| } | ||
| BitcoindClient::Rest { rest_client, .. } => { | ||
| rest_client.get_header(header_hash, height_hint).await |
There was a problem hiding this comment.
In your comment, you said "though note after restart that cache is also empty, so the fallback still makes sense anyways", but you remove it anyway?
Or is the shared and initialized spv client all that is needed?
There was a problem hiding this comment.
I think the shared SPV client is the core fix, but there are edge cases where the fallback helps (belt-and-suspenders I guess):
There is a concrete race:
- REST chaininfo returns tip A.
- Bitcoin Core reorganizes to tip B.
- REST headers/A returns 404, classified as persistent.
- During initial synchronization, that causes a 300-second backoff
RPC can still resolve stale header A, allowing synchronization to complete; the retained SpvClient then handles B on the next poll. The fallback also helps beyond the 12-block restart locator window or when locator history is incomplete.
There was a problem hiding this comment.
Yes, but I mean your second commit removes the fallback again.
There was a problem hiding this comment.
Yes, but I mean your second commit removes the fallback again.
Ah, whoops, amended.
| }; | ||
| *spv_client_lock = Some(self.new_spv_client( | ||
| chain_tip, | ||
| HeaderCache::new(), |
There was a problem hiding this comment.
Is it possible to hit this? And how is a stale branch walked backwards in that case?
There was a problem hiding this comment.
It is possible to hit this in a narrow startup race: start spawns the initial-sync future, but sync_wallets may run before that future is first polled and registers the pending sync. A persistent REST miss falls back to RPC, so stale headers remain available.
Fixing that narrow scheduling race would require coordinating registration with task spawning. Given that background synchronization corrects the state and the fallback remains available, I’d prefer to handle that some time separately rather than expand this reorg fix.
Retain the cache populated during initial listener synchronization so subsequent polls can find headers for stale forks when REST no longer serves them. Use weak listener handles to avoid creating an ownership cycle through the chain source. Co-Authored-By: HAL 9000
357f6e6 to
2ab34e3
Compare
4 participants
Bitcoin Core's REST headers endpoint cannot serve headers from a stale branch. This prevented REST-backed nodes from finding a common ancestor and following the replacement chain after a reorg.
In #1006 we enabled bitcoind REST chain source support in CI, which exposed this failure.
Co-Authored-By: HAL 9000