From 8b92e087df58e7f2cb44cb1d949bc13b2e4e6f79 Mon Sep 17 00:00:00 2001 From: hhsel <26063868+hhsel@users.noreply.github.com> Date: Mon, 21 Aug 2023 19:25:22 +0900 Subject: [PATCH] QBFT: prevent maximum request timeout overflow (#1665) --- consensus/istanbul/qbft/core/core.go | 29 +++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/consensus/istanbul/qbft/core/core.go b/consensus/istanbul/qbft/core/core.go index 20bd1569c3..e078d4edd4 100644 --- a/consensus/istanbul/qbft/core/core.go +++ b/consensus/istanbul/qbft/core/core.go @@ -261,13 +261,32 @@ func (c *core) newRoundChangeTimer() { // set timeout based on the round number baseTimeout := time.Duration(c.config.GetConfig(c.current.Sequence()).RequestTimeout) * time.Millisecond round := c.current.Round().Uint64() - - timeout := baseTimeout * time.Duration(math.Pow(2, float64(round))) - maxRequestTimeout := time.Duration(c.config.GetConfig(c.current.Sequence()).MaxRequestTimeoutSeconds) * time.Second - if maxRequestTimeout > time.Duration(0) && timeout > maxRequestTimeout { - timeout = maxRequestTimeout + // If the upper limit of the request timeout is capped by small maxRequestTimeout, round can be a quite large number, + // which leads to float64 overflow, making its value negative or zero forever after some point. + // In this case we cannot simply use math.Pow and have to implement a safeguard on our own, at the cost of performance (which is not important in this case). + var timeout time.Duration + if maxRequestTimeout > time.Duration(0) { + timeout = baseTimeout + for i := uint64(0); i < round; i++ { + timeout = timeout * 2 + if timeout > maxRequestTimeout { + timeout = maxRequestTimeout + break + } + } + // prevent log storm when unexpected overflow happens + if timeout < baseTimeout { + c.currentLogger(true, nil).Error("QBFT: Possible request timeout overflow detected, setting timeout value to maxRequestTimeout", + "timeout", timeout.Seconds(), + "max_request_timeout", maxRequestTimeout.Seconds(), + ) + timeout = maxRequestTimeout + } + } else { + // effectively impossible to observe overflow happen when maxRequestTimeout is disabled + timeout = baseTimeout * time.Duration(math.Pow(2, float64(round))) } c.currentLogger(true, nil).Trace("QBFT: start new ROUND-CHANGE timer", "timeout", timeout.Seconds())