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 engine timeout values #10645

Merged
merged 9 commits into from
May 9, 2022
Merged

Add engine timeout values #10645

merged 9 commits into from
May 9, 2022

Conversation

terencechain
Copy link
Member

Add engine call timeout values outlined in: ethereum/execution-apis#216
More of the rationale can be found here: ethereum/execution-apis#207

@terencechain terencechain requested a review from a team as a code owner May 5, 2022 21:55
@terencechain terencechain self-assigned this May 5, 2022
@terencechain terencechain requested review from potuz, rkapka and nisdas May 5, 2022 21:55
@terencechain terencechain added the Ready For Review A pull request ready for code review label May 9, 2022
@@ -33,6 +33,10 @@ const (
ExecutionBlockByHashMethod = "eth_getBlockByHash"
// ExecutionBlockByNumberMethod request string for JSON-RPC.
ExecutionBlockByNumberMethod = "eth_getBlockByNumber"
// Defines the seconds to wait before timing out engine endpoints with block execution semantics.
blockExecutionTimeOut = 8 * time.Second
Copy link
Contributor

Choose a reason for hiding this comment

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

newPayloadTimeout

Copy link
Member Author

Choose a reason for hiding this comment

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

It's actually used for fork choice updated too, I'll think of something

// Defines the seconds to wait before timing out engine endpoints with block execution semantics.
blockExecutionTimeOut = 8 * time.Second
// Defines the seconds before timing out engine endpoints with non-block execution semantics.
nonBlockExecutionTimeOut = time.Second
Copy link
Contributor

Choose a reason for hiding this comment

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

defaultEngineTimeout

Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

Requesting defer cancel()

terencechain and others added 3 commits May 9, 2022 10:14
Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>
Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>
Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>
@terencechain
Copy link
Member Author

@prestonvanloon I agree, thanks for the feedback!

// Defines the seconds to wait before timing out engine endpoints with block execution semantics (newPayload, forkchoiceUpdated).
payloadAndForkchoiceUpdatedTimeout = 8 * time.Second
// Defines the seconds before timing out engine endpoints with non-block execution semantics.
defaultTimeout = time.Second
Copy link
Contributor

Choose a reason for hiding this comment

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

can we call this defaultEngineTimeout? otherwise seems like a default for all things in the powchain service

@prestonvanloon prestonvanloon dismissed their stale review May 9, 2022 18:10

LGTM, waiting for Raul's feedback to approve

@prylabs-bulldozer prylabs-bulldozer bot merged commit 16273a2 into develop May 9, 2022
@delete-merged-branch delete-merged-branch bot deleted the use-timeouts branch May 9, 2022 20:02
rkapka pushed a commit that referenced this pull request May 11, 2022
* Add timeout values

* Update engine_client.go

* Update engine_client.go

* Update beacon-chain/powchain/engine_client.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

* Update beacon-chain/powchain/engine_client.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

* Update beacon-chain/powchain/engine_client.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

* Update engine_client.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>
prylabs-bulldozer bot added a commit that referenced this pull request May 16, 2022
* SubmitBlockSSZ grpc

* SubmitBlockSSZ middleware

* test fixes

* use VersionedUnmarshaller

* use VersionedUnmarshaller

(cherry picked from commit 7388eeb)

* tests

* fuzz: Add fuzz tests for sparse merkle trie (#10662)

* Add fuzz tests for sparse merkle trie and change HTR signature to return an error

* fix capitalization of error message

* Add engine timeout values (#10645)

* Add timeout values

* Update engine_client.go

* Update engine_client.go

* Update beacon-chain/powchain/engine_client.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

* Update beacon-chain/powchain/engine_client.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

* Update beacon-chain/powchain/engine_client.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

* Update engine_client.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

* Cleanup of `stategen` package (#10607)

* powchain and stategen

* revert powchain changes

* rename field to blockRootsOfSavedStates

* rename params to blockRoot

* review feedback

* fix loop

Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

* Process atts and update head before proposing (#10653)

* Process atts and updeate head

* Fix ctx

* New test and old tests

* Update validator_test.go

* Update validator_test.go

* Update service.go

* Rename to UpdateHead

* Update receive_attestation.go

* Update receive_attestation.go

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

* Add link to e2e docs in `README` (#10672)

* Improve `ReceiveBlock`'s comment (#10671)

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

* Call fcu on invalid payload (#10565)

* Starting

* remove finalized root

* Just call fcu

* Review feedbacks

* fix one test

* Fix conflicts

* Update execution_engine_test.go

* Add a test for invalid recursive call

* Add comprehensive recursive test

* dissallow override empty hash

Co-authored-by: Potuz <potuz@prysmaticlabs.com>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

* Cache and use justified and finalized payload block hash (#10657)

* Cache and use justified and finalized payload block hash

* Fix tests

* Use real byte

* Fix conflicts

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

* do not export slotFromBlock

* simplify tests

* grpc

* middleware

* extract package-level consts

* Simplify SSZ handling

* fix tests

* test fixes

* test hack

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>
Co-authored-by: terencechain <terence@prysmaticlabs.com>
Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Potuz <potuz@prysmaticlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review Spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants