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

eth,rpc: allow for flag configured timeouts for eth_call #23645

Merged
merged 3 commits into from Oct 12, 2021

Conversation

noam-alchemy
Copy link
Contributor

@noam-alchemy noam-alchemy commented Sep 26, 2021

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-L900

Tested locally.

cmd/utils/flags.go Outdated Show resolved Hide resolved
@@ -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())
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

ligi
ligi previously requested changes Sep 28, 2021
Copy link
Member

@ligi ligi left a 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

cmd/utils/flags.go Outdated Show resolved Hide resolved
@noam-alchemy
Copy link
Contributor Author

Screen Shot 2021-10-04 at 11 11 18 AM

@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.

@noam-alchemy
Copy link
Contributor Author

Thanks for the PR! Just a minor nit regarding naming.

Naming updated.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman added this to the 1.10.10 milestone Oct 12, 2021
@holiman holiman merged commit 633e7ef into ethereum:master Oct 12, 2021
@karalabe
Copy link
Member

Nitpick, wouldn't it make sense to call it TimeCap if we already have FeeCap and GasCap?

sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Oct 12, 2021
)

* eth,rpc: allow for flag configured timeouts for eth_call

* lint: account for package-local import order

* cr: rename `rpc.calltimeout` to `rpc.evmtimeout`
yongjun925 pushed a commit to DODOEX/go-ethereum that referenced this pull request Dec 3, 2022
)

* eth,rpc: allow for flag configured timeouts for eth_call

* lint: account for package-local import order

* cr: rename `rpc.calltimeout` to `rpc.evmtimeout`
@kjeom kjeom mentioned this pull request Dec 6, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants