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 builder header timeout flag #5857

Merged
merged 7 commits into from
May 31, 2024

Conversation

eserilev
Copy link
Collaborator

@eserilev eserilev commented May 28, 2024

Issue Addressed

#4709

Proposed Changes

Add a new flag builder-header-timeout which allows for configuring timeouts for the get builder header api call. Ive added a max limit of 3 seconds to the timeout to prevent potential liveness issues.

@eserilev
Copy link
Collaborator Author

eserilev commented May 28, 2024

Do we want to force minimum and maximum values for this flag? I don't see why anyone would want this set to less than one second, or to more than 2-3 seconds

Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Do we want to force minimum and maximum values for this flag? I don't see why anyone would want this set to less than one second, or to more than 2-3 seconds

If you set a very low value you will prioritize local building, which you can do today already.
If you set a very high value you will never consider local building and potentially cause liveness issues if builders misbehave. In extreme cases the failover should kick in switching to local building if the failures are common.

I have a soft opinion towards not specifying neither a minimum nor a maximum

consensus/types/src/test_utils/test_random/bitfield.rs Outdated Show resolved Hide resolved
@chong-he chong-he added builder API ready-for-review The code is ready for review labels May 28, 2024
beacon_node/execution_layer/src/lib.rs Outdated Show resolved Hide resolved
beacon_node/builder_client/src/lib.rs Outdated Show resolved Hide resolved
@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels May 29, 2024
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Looks good - I don't have anything else to add!

@michaelsproul michaelsproul added the v5.2.0 Q2 2024 label May 30, 2024
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Looks good modulo capitalisation fix

beacon_node/src/cli.rs Outdated Show resolved Hide resolved
book/src/help_bn.md Outdated Show resolved Hide resolved
michaelsproul added a commit that referenced this pull request May 30, 2024
Squashed commit of the following:

commit 5950106
Author: Eitan Seri-Levi <eserilev@ucsc.edu>
Date:   Wed May 29 22:43:11 2024 +0200

    update cli

commit 773c4d4
Author: Eitan Seri-Levi <eserilev@ucsc.edu>
Date:   Wed May 29 20:04:02 2024 +0200

    add upper limit check, changes based on feedback

commit 8d1f865
Author: Eitan Seri-Levi <eserilev@ucsc.edu>
Date:   Tue May 28 18:43:13 2024 +0200

    make cli

commit 30f9dec
Author: Eitan Seri-Levi <eserilev@ucsc.edu>
Date:   Tue May 28 18:29:22 2024 +0200

    add a new flag that allows for configuring the timeout for get builder header api calls

commit e87cf1d
Author: Eitan Seri-Levi <eserilev@ucsc.edu>
Date:   Tue May 28 15:52:10 2024 +0200

    fix bitvector test random impl
@michaelsproul michaelsproul mentioned this pull request May 30, 2024
@eserilev eserilev added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels May 30, 2024
@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels May 30, 2024
@michaelsproul
Copy link
Member

@Mergifyio queue

Copy link

mergify bot commented May 31, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at fbc230e

mergify bot added a commit that referenced this pull request May 31, 2024
michaelsproul added a commit that referenced this pull request May 31, 2024
Squashed commit of the following:

commit 5950106
Author: Eitan Seri-Levi <eserilev@ucsc.edu>
Date:   Wed May 29 22:43:11 2024 +0200

    update cli

commit 773c4d4
Author: Eitan Seri-Levi <eserilev@ucsc.edu>
Date:   Wed May 29 20:04:02 2024 +0200

    add upper limit check, changes based on feedback

commit 8d1f865
Author: Eitan Seri-Levi <eserilev@ucsc.edu>
Date:   Tue May 28 18:43:13 2024 +0200

    make cli

commit 30f9dec
Author: Eitan Seri-Levi <eserilev@ucsc.edu>
Date:   Tue May 28 18:29:22 2024 +0200

    add a new flag that allows for configuring the timeout for get builder header api calls

commit e87cf1d
Author: Eitan Seri-Levi <eserilev@ucsc.edu>
Date:   Tue May 28 15:52:10 2024 +0200

    fix bitvector test random impl
@mergify mergify bot merged commit fbc230e into sigp:unstable May 31, 2024
28 of 29 checks passed
@eserilev eserilev mentioned this pull request May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builder API ready-for-merge This PR is ready to merge. v5.2.0 Q2 2024
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants