Repository navigation
validation: cap mainchain RPC timeout during peg-in validation (+ -mainchainrpctimeout=0 help text fix) - #1608
Open
GracedEternalKingCabbageMan wants to merge 2 commits into
Conversation
The help text claims a value of 0 disables the timeout. It does not: with the pinned libevent 2.1.12, a timeout of 0 is treated as unset and libevent substitutes its own internal defaults (45s connect, 50s read/write). There is no way to disable the timeout via this option.
Peg-in validation (-validatepegin, default on for chains with a federated peg) issues synchronous mainchain RPC calls while holding cs_main: during block connection (ConnectTip via CheckPeginRipeness; ConnectBlock via Consensus::CheckTxInputs), during mempool acceptance (PreChecks via CheckPeginSubsidyAndMinimum), and during mempool epoch ejection (removeForBlock). Each call uses the full -mainchainrpctimeout (default 900s) with a fresh blocking TCP connection and no caching, so a mainchain daemon that accepts connections but stops responding can stall block validation and mempool acceptance for up to 15 minutes per peg-in input. IsConfirmedBitcoinBlock treats RPC failure as 'not confirmed, retry later', and the getrawtransaction call in CheckPeginSubsidyAndMinimum fails the transaction into the same retryable mempool-rejection path, so blocking for the full timeout has no benefit on these paths. Give CallMainChainRPC an optional timeout parameter (negative preserves the existing behavior for all other callers), add GetValidationRPCTimeout() clamping -mainchainrpctimeout into [1, MAX_VALIDATION_RPC_TIMEOUT] (30s), and use it for the getblockheader call in IsConfirmedBitcoinBlock and the getrawtransaction call in CheckPeginSubsidyAndMinimum. This also bounds the advisory maturity check in createrawpegin. The startup MainchainRPCCheck keeps the full configured timeout. The help text now documents the cap.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
validation: cap mainchain RPC timeout during peg-in validation
This PR contains two commits:
rpc: correct the -mainchainrpctimeout=0 help text— a one-line documentation fix.validation: cap mainchain RPC timeout during peg-in validation— the behavior change.Problem
Peg-in validation (
-validatepegin, default-on for chains with a federated peg such as Liquid)issues synchronous JSON-RPC calls to the configured mainchain daemon, from code paths that
hold
cs_main(and, for mempool acceptance,mempool.csas well):Chainstate::ConnectTip→CheckPeginRipenessgetblockheader(per peg-in input)AssertLockHeld(cs_main)Consensus::CheckTxInputs, called from both block connection (ConnectBlock) and mempool acceptance (MemPoolAccept::PreChecks)getblockheader(per peg-in input)cs_main(+mempool.csat acceptance)MemPoolAccept::PreChecks→CheckPeginSubsidyAndMinimumgetrawtransaction(per peg-in input)EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs)CTxMemPool::removeForBlockgetblockheader(per peg-in input)All of these go through
CallMainChainRPC, which applies-mainchainrpctimeout(default900 seconds) to each call, with a fresh blocking TCP connection per call and no caching of
results.
If the local mainchain daemon accepts TCP connections but stops answering requests (it is itself
stalled, overloaded, or otherwise wedged), every one of these calls blocks for up to 15 minutes
per peg-in input while the locks are held — freezing block validation, mempool acceptance, and
most RPC for the duration.
ConnectTipeven logs that the chain "will not grow until this isremedied", so this coupling is by design; what is missing is a bound on how long a single
unresponsive-daemon episode can stall validation per attempt.
Notably, failing fast is already the safe semantic on every one of these paths:
IsConfirmedBitcoinBlockcatches the connection failure and returnsfalse— "not yetconfirmed, retry later" (at block connection this is the deliberate stall-and-retry machinery;
in the mempool the transaction simply stays).
getrawtransactioncall inCheckPeginSubsidyAndMinimumpropagates the sameCConnectionFailedit would hit after 15 minutes today, into the same retryablemempool-rejection path ("pegin-subsidy-mainchain-error").
So blocking for the full 900 s buys nothing on these paths: after 30 s the code lands in exactly
the same state it reaches after 900 s today.
Change
CallMainChainRPCgains an optionaltimeoutparameter. Negative (the default) preserves thecurrent behavior for every existing caller: read
-mainchainrpctimeout.GetValidationRPCTimeout()returns-mainchainrpctimeoutclamped into[1, MAX_VALIDATION_RPC_TIMEOUT](new constant, 30 seconds).IsConfirmedBitcoinBlock(getblockheader) and thegetrawtransactioncall inCheckPeginSubsidyAndMinimum.IsConfirmedBitcoinBlockalso serves the advisorymaturefield of
createrawpegin, which fails faster against a hung daemon — harmless and desirable.MainchainRPCCheckin init.cpp) and all otherCallMainChainRPCusers (wallet RPCs) are unchanged and keep the full
-mainchainrpctimeout.30 s is generous for the intended deployment (a co-located, trusted
bitcoindansweringgetblockheader/getrawtransaction) and bounds a stall to well under one Liquid block intervalper input instead of 15 minutes.
Help text fix (first commit)
The
-mainchainrpctimeouthelp text claims "0 for no timeout". That is not what happens: withthe pinned libevent 2.1.12, a timeout of 0 is treated as unset and libevent substitutes its own
internal defaults (45 s connect, 50 s read/write —
HTTP_CONNECT_TIMEOUT/HTTP_READ_TIMEOUT/HTTP_WRITE_TIMEOUTinhttp-internal.h, applied inhttp.c). There is no way to disable thetimeout through this option. The help text now states the actual behavior (and, in the second
commit, documents the validation cap).
Backward compatibility
-mainchainrpctimeoutto a value ≤ 30 see no change in validation behavior;values above 30 (including the 900 s default) are capped at 30 s for the two validation-path
call sites only.
false-retry and exception paths are taken, onlysooner.
Test
Adds
mainchainrpc_tests.cppcoveringGetValidationRPCTimeout: default clamped to the cap,values below the cap honored, values above the cap clamped, boundary value honored, and
0clamped to 1 (since 0 never meant "no timeout" anyway). A functional harness for a
hung-but-connected daemon was considered out of scope for this PR; happy to add one if reviewers
want end-to-end coverage of the stall path.
Notes for reviewers
bitcoind) while bounding a stall to well under one Liquid block interval; 60 s would workjust as well if preferred.
rejected to keep the option surface minimal; the patch is structured so that swapping the
constant for an argument is a two-line change if maintainers prefer that.
not apply there verbatim (autotools vs. CMake test registration, minor context drift) but is a
trivial manual re-application if wanted.