multi: use bitcoind uptime api for healthcheck #4755
Conversation
chainreg/chainregistry.go
Outdated
| return nil, err | ||
| } | ||
| cc.HealthCheck = func() error { | ||
| _, err := chainConn.RawRequest("uptime", nil) |
There was a problem hiding this comment.
Which version of bitcoind was this call added with? Perhaps we should do a version check on start up to warn the user about the feature, possibly falling back to the older way of doing things?
There was a problem hiding this comment.
First added in 0.15, will add a version check 👍
There was a problem hiding this comment.
Discussed offline. Actual segwit code in in 0.13, but without activation, 0.14 has no mention of segwit and "full support" added in 0.16, so I think it's safe to assume people have >=0.16 running because you couldn't signal segwit support without it? Can add a version check to be safe if we're worried about it.
carlaKC
requested review from
Roasbeef and
guggero
and removed request for
cfromknecht
The getblockchaininfo call in bitcoind uses a commonly used lock, csmain, in bitcoind. This made the endpoint unsuitable for a health check, because some nodes were seeing waits up to 5 minutes (!). This commit updates our health check function to use the uptime api, provided our bitcoind version is > 0.15, when the api was added. We do not need to switch our health check for btcd, because it has more granular locking.
carlaKC
force-pushed
the
4669-nonlockedcall
branch
from
9ea0597
to
ca3b36c
Compare
|
Updated this to have a version check to be safe, I don't think it costs us much. It's difficult to tell where the line is for segwit enabled because 0.13 has all the code, but then activation was more complex. All the 0.14 versions are also vulnerable to inflation bug, so I think most people should be running on 0.15 and above. |
|
Thanks, that was quick 👍 |
Previously we were using
GetBestBlockfor all of our healthchecks. This was running into some issues withbitcoind, because theGetBestBlockrpc call uses the csmain lock, which is widely used, and would block for extended periods of time (5 minutes sometimes!).btcdhas more granular locking, so it does not face these issues.This PR updates our chain backend health check to use the
uptimeapi because it has no locking. Another option would be to use something likeestimatesmartfeewhich uses a less common lock inbitcoindand does not need us to make an ad-hocrpcclient. Decided against this becauseestimatesmartfeecould crunch through mempool data unnecessarily.Fixes #4669.