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 |
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
|
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?
| }; | ||
| *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?
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