-
Notifications
You must be signed in to change notification settings - Fork 20.1k
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
eth,rpc: allow for flag configured timeouts for eth_call #23645
Conversation
internal/ethapi/api.go
Outdated
@@ -972,7 +972,7 @@ 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()) | |||
result, err := DoCall(ctx, s.b, args, blockNrOrHash, overrides, s.b.RPCCallTimeout(), s.b.RPCGasCap()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also put this timeout in DoEstimateCall
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming you mean DoEstimateGas
? Wouldn't that be a regression for anyone using eth_estimateGas
before sending XL transactions? Currently estimateGas
would not time out while running the EVM call. Adding this change here would enforce a new default limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Github's making me resolve this conversation because of the requested changes, but the open question still stands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes right. The original timeout for Estimate is 0(no timeout), lgtm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Just a minor nit regarding naming
@ligi - Github is not allowing me to "Re-request review" and or "Dismiss changes" but the PR is ready for review again. There's an open question about adding the timeouts to the EstimateGas path. |
Naming updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Nitpick, wouldn't it make sense to call it |
Allows eth_call timeouts to be specified via a startup flag. The default timeout is equivalent to the current hardcoded timeout (5 seconds).
Setting the value to
0
will not apply timeouts to the RPC call. See this block for handling: https://github.com/ethereum/go-ethereum/blob/master/internal/ethapi/api.go#L896-L900Tested locally.