consensus: add timeout to UTXO queries #1391
Conversation
The state service API says explicitly that AwaitUTXO requests should be coupled with a timeout layer. I didn't add this when I was testing and fixing the UTXO lookup code (#1348, #1358) because causing zebrad to hang on a failed dependency was useful for identifying cases where the code wasn't useful (and then inspecting execution traces). As a side effect, I believe this closes #1389, because far-future gossiped blocks will have their UTXO lookups time out, though we may wish to do other work as part of debugging the combined sync+gossip logic.
hdevalence
force-pushed
the
script-utxo-timeout
branch
from
9adcded to
a9063bb
Compare
I think this is true for most blocks, but not for blocks that:
The checkpoint queue blocks will wait on their checkpoint range. The coinbase-only and shielded-only blocks will pass the UTXO part of block verification, and end up waiting in the non-finalized state queue for their parent block. So I might keep #1389 open, with a note that we've solved part of the issue with UTXO timeouts. This fix might also fix the hangs in #1398, but we should double-check before closing that issue. |
| } | ||
|
|
||
| impl<ZS> Verifier<ZS> { | ||
| pub fn new(state: ZS) -> Self { | ||
| Self { state } | ||
| Self { | ||
| state: Timeout::new(state, UTXO_LOOKUP_TIMEOUT), |
There was a problem hiding this comment.
Note: Due to the mapping in the BlockVerifier, timeout errors should appear as VerifyBlockError::Transaction(Elapsed).
Ideally, we'd say that the UTXO request elapsed, but that doesn't really matter until we have a mempool. I've opened #1401 in the first stable release for follow-up.
The state service API says explicitly that AwaitUTXO requests should be coupled
with a timeout layer. I didn't add this when I was testing and fixing the UTXO
lookup code (#1348, #1358) because causing zebrad to hang on a failed
dependency was useful for identifying cases where the code wasn't useful (and
then inspecting execution traces).
As a side effect, I believe this fixes the UTXO part of #1389, because far-future gossiped
blocks will have their UTXO lookups time out, though we may wish to do other
work as part of debugging the combined sync+gossip logic (#1372)