Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add http rpc timeout option as per (#23416) #23483

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions cmd/geth/config_test.go
@@ -0,0 +1,16 @@
package main

import (
"testing"
)

func TestTomlHTTPRpcTimeout(t *testing.T) {
var cfg gethConfig
err := loadConfig("testdata/config.toml", &cfg)
if err != nil {
t.Fatal(err)
}
if cfg.Eth.HTTPRpcTimeout.Seconds() != 20 {
t.Errorf("HTTPRpcTimeout should be 20 seconds")
}
}
1 change: 1 addition & 0 deletions cmd/geth/main.go
Expand Up @@ -154,6 +154,7 @@ var (

rpcFlags = []cli.Flag{
utils.HTTPEnabledFlag,
utils.HTTPRpcTimeoutFlag,
utils.HTTPListenAddrFlag,
utils.HTTPPortFlag,
utils.HTTPCORSDomainFlag,
Expand Down
95 changes: 95 additions & 0 deletions cmd/geth/testdata/config.toml
@@ -0,0 +1,95 @@
[Eth]
NetworkId = 1
SyncMode = "snap"
EthDiscoveryURLs = ["enrtree://AKA3AM6LPBYEUDMVNU3BSVQJ5AD45Y7YPOHJLEF6W26QOE4VTUDPE@all.mainnet.ethdisco.net"]
SnapDiscoveryURLs = ["enrtree://AKA3AM6LPBYEUDMVNU3BSVQJ5AD45Y7YPOHJLEF6W26QOE4VTUDPE@all.mainnet.ethdisco.net"]
NoPruning = false
NoPrefetch = false
TxLookupLimit = 2350000
LightPeers = 100
UltraLightFraction = 75
DatabaseCache = 512
DatabaseFreezer = ""
TrieCleanCache = 154
TrieCleanCacheJournal = "triecache"
TrieCleanCacheRejournal = 3600000000000
TrieDirtyCache = 256
TrieTimeout = 3600000000000
SnapshotCache = 102
Preimages = false
EnablePreimageRecording = false
RPCGasCap = 50000000
RPCTxFeeCap = 1e+00
HTTPRpcTimeout = 20000000000

[Eth.Miner]
GasFloor = 8000000
GasCeil = 8000000
GasPrice = 1000000000
Recommit = 3000000000
Noverify = false

[Eth.Ethash]
CacheDir = "ethash"
CachesInMem = 2
CachesOnDisk = 3
CachesLockMmap = false
DatasetDir = "/home/mr/.ethash"
DatasetsInMem = 1
DatasetsOnDisk = 2
DatasetsLockMmap = false
PowMode = 0
NotifyFull = false

[Eth.TxPool]
Locals = []
NoLocals = false
Journal = "transactions.rlp"
Rejournal = 3600000000000
PriceLimit = 1
PriceBump = 10
AccountSlots = 16
GlobalSlots = 5120
AccountQueue = 64
GlobalQueue = 1024
Lifetime = 10800000000000

[Eth.GPO]
Blocks = 20
Percentile = 60
MaxPrice = 500000000000
IgnorePrice = 2

[Node]
DataDir = ".dev"
IPCPath = "geth.ipc"
HTTPHost = ""
HTTPPort = 8545
HTTPVirtualHosts = ["localhost"]
HTTPModules = ["net", "web3", "eth"]
WSHost = ""
WSPort = 8546
WSModules = ["net", "web3", "eth"]
GraphQLVirtualHosts = ["localhost"]

[Node.P2P]
MaxPeers = 50
NoDiscovery = false
StaticNodes = []
TrustedNodes = []
ListenAddr = ":30303"
EnableMsgEvents = false

[Node.HTTPTimeouts]
ReadTimeout = 30000000000
WriteTimeout = 30000000000
IdleTimeout = 120000000000

[Metrics]
HTTP = "127.0.0.1"
Port = 6060
InfluxDBEndpoint = "http://localhost:8086"
InfluxDBDatabase = "geth"
InfluxDBUsername = "test"
InfluxDBPassword = "test"
InfluxDBTags = "host=localhost"
1 change: 1 addition & 0 deletions cmd/geth/usage.go
Expand Up @@ -134,6 +134,7 @@ var AppHelpFlagGroups = []flags.FlagGroup{
utils.IPCDisabledFlag,
utils.IPCPathFlag,
utils.HTTPEnabledFlag,
utils.HTTPRpcTimeoutFlag,
utils.HTTPListenAddrFlag,
utils.HTTPPortFlag,
utils.HTTPApiFlag,
Expand Down
20 changes: 20 additions & 0 deletions cmd/utils/flags.go
Expand Up @@ -775,6 +775,13 @@ var (
Name: "catalyst",
Usage: "Catalyst mode (eth2 integration testing)",
}

HTTPRpcTimeoutFlag = cli.StringFlag{
Name: "http.timeout",
Usage: "Sets http rpc request timeout (use \"s\" suffix for seconds, eg: --http.timeout 10s)",
// uses time.ParseDuration() which requires a unit suffix, "h", "m", "s", "ms" etc.
Value: fmt.Sprint(ethconfig.Defaults.HTTPRpcTimeout.Nanoseconds(), "ns"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have (or time lib) have some nicer default printer? It seems a bit silly to print the default in nano-seconds, nicer to have it in seconds or milliseconds.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even better -- IIRC there's some DurationFlag type, isn't there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, just printing it out does a better job. 5s instead of 5...ns.

There is a DurationFlag type, nice.

}
)

// MakeDataDir retrieves the currently requested data directory, terminating
Expand Down Expand Up @@ -1600,6 +1607,19 @@ func SetEthConfig(ctx *cli.Context, stack *node.Node, cfg *ethconfig.Config) {
cfg.EthDiscoveryURLs = SplitAndTrim(urls)
}
}
if ctx.GlobalIsSet(HTTPRpcTimeoutFlag.Name) {
arg := ctx.GlobalString(HTTPRpcTimeoutFlag.Name)
_, err := strconv.Atoi(arg)
if err == nil {
arg = arg + "ns"
}
Comment on lines +1610 to +1614
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why parse, it's a Duration type already ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I cleaned this up and added some tests.

timeout, err := time.ParseDuration(arg)
if err != nil {
log.Warn("Bad http.timeout setting", arg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our log facilities are a bit peculiar: essentially it's on the form log.Level( <message>, <key-name>, <value>, <key-name>, <value>). So it's always 1 + 2N arguments. In this case, it would be appropriate with e.g. log.Warn("Failed to parse http timeout duration", "err", err)

timeout = ethconfig.Defaults.HTTPRpcTimeout
}
cfg.HTTPRpcTimeout = timeout
}
// Override any default configs for hard coded networks.
switch {
case ctx.GlobalBool(MainnetFlag.Name):
Expand Down
5 changes: 5 additions & 0 deletions eth/api_backend.go
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"errors"
"math/big"
"time"

"github.com/ethereum/go-ethereum/accounts"
"github.com/ethereum/go-ethereum/common"
Expand Down Expand Up @@ -353,3 +354,7 @@ func (b *EthAPIBackend) StateAtBlock(ctx context.Context, block *types.Block, re
func (b *EthAPIBackend) StateAtTransaction(ctx context.Context, block *types.Block, txIndex int, reexec uint64) (core.Message, vm.BlockContext, *state.StateDB, error) {
return b.eth.stateAtTransaction(block, txIndex, reexec)
}

func (b *EthAPIBackend) HTTPRpcTimeout() time.Duration {
return b.eth.config.HTTPRpcTimeout
}
12 changes: 8 additions & 4 deletions eth/ethconfig/config.go
Expand Up @@ -87,10 +87,11 @@ var Defaults = Config{
GasPrice: big.NewInt(params.GWei),
Recommit: 3 * time.Second,
},
TxPool: core.DefaultTxPoolConfig,
RPCGasCap: 50000000,
GPO: FullNodeGPO,
RPCTxFeeCap: 1, // 1 ether
TxPool: core.DefaultTxPoolConfig,
RPCGasCap: 50000000,
GPO: FullNodeGPO,
RPCTxFeeCap: 1, // 1 ether
HTTPRpcTimeout: time.Second * 5,
}

func init() {
Expand Down Expand Up @@ -200,6 +201,9 @@ type Config struct {

// Berlin block override (TODO: remove after the fork)
OverrideLondon *big.Int `toml:",omitempty"`

// Http Rpc request timeout.
HTTPRpcTimeout time.Duration
}

// CreateConsensusEngine creates a consensus engine for the given chain configuration.
Expand Down
6 changes: 6 additions & 0 deletions eth/ethconfig/gen_config.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 6 additions & 2 deletions internal/ethapi/api.go
Expand Up @@ -40,6 +40,7 @@ import (
"github.com/ethereum/go-ethereum/core/types"
"github.com/ethereum/go-ethereum/core/vm"
"github.com/ethereum/go-ethereum/crypto"
"github.com/ethereum/go-ethereum/eth/ethconfig"
"github.com/ethereum/go-ethereum/log"
"github.com/ethereum/go-ethereum/p2p"
"github.com/ethereum/go-ethereum/params"
Expand Down Expand Up @@ -882,7 +883,6 @@ func (diff *StateOverride) Apply(state *state.StateDB) error {

func DoCall(ctx context.Context, b Backend, args TransactionArgs, blockNrOrHash rpc.BlockNumberOrHash, overrides *StateOverride, timeout time.Duration, globalGasCap uint64) (*core.ExecutionResult, error) {
defer func(start time.Time) { log.Debug("Executing EVM call finished", "runtime", time.Since(start)) }(time.Now())

state, header, err := b.StateAndHeaderByNumberOrHash(ctx, blockNrOrHash)
if state == nil || err != nil {
return nil, err
Expand Down Expand Up @@ -972,7 +972,11 @@ func (e *revertError) ErrorData() interface{} {
// Note, this function doesn't make and changes in the state/blockchain and is
// useful to execute and retrieve values.
func (s *PublicBlockChainAPI) Call(ctx context.Context, args TransactionArgs, blockNrOrHash rpc.BlockNumberOrHash, overrides *StateOverride) (hexutil.Bytes, error) {
result, err := DoCall(ctx, s.b, args, blockNrOrHash, overrides, 5*time.Second, s.b.RPCGasCap())
timeout := s.b.HTTPRpcTimeout()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only place where we use this? Seems odd, if we have a 'global' rpctimeout flag, to only ever apply it on the Call method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going through internal/ethapi/api.go it seems to be the only rpc with that hard coded timeout. I'll dig in some more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There also this:

Service: filters.NewPublicFilterAPI(s.APIBackend, false, 5*time.Minute),

A hard coded 5 minute timeout for filters api, eth_getLogs, eth_getFilterChanges etc. Seems incompatible with the 5 second timeout for eth_call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not the same kind of timeout -- filters generally live on a ws subscription, not a one-off http request/response pair.

if timeout == 0 {
timeout = ethconfig.Defaults.HTTPRpcTimeout
}
result, err := DoCall(ctx, s.b, args, blockNrOrHash, overrides, timeout, s.b.RPCGasCap())
if err != nil {
return nil, err
}
Expand Down
3 changes: 3 additions & 0 deletions internal/ethapi/backend.go
Expand Up @@ -20,6 +20,7 @@ package ethapi
import (
"context"
"math/big"
"time"

"github.com/ethereum/go-ethereum/accounts"
"github.com/ethereum/go-ethereum/common"
Expand Down Expand Up @@ -90,6 +91,8 @@ type Backend interface {

ChainConfig() *params.ChainConfig
Engine() consensus.Engine

HTTPRpcTimeout() time.Duration
}

func GetAPIs(apiBackend Backend) []rpc.API {
Expand Down
5 changes: 5 additions & 0 deletions les/api_backend.go
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"errors"
"math/big"
"time"

"github.com/ethereum/go-ethereum/accounts"
"github.com/ethereum/go-ethereum/common"
Expand Down Expand Up @@ -326,3 +327,7 @@ func (b *LesApiBackend) StateAtBlock(ctx context.Context, block *types.Block, re
func (b *LesApiBackend) StateAtTransaction(ctx context.Context, block *types.Block, txIndex int, reexec uint64) (core.Message, vm.BlockContext, *state.StateDB, error) {
return b.eth.stateAtTransaction(ctx, block, txIndex, reexec)
}

func (b *LesApiBackend) HTTPRpcTimeout() time.Duration {
return b.eth.config.HTTPRpcTimeout
}