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

core/scheduler: add support for duty builder proposer #827

Conversation

ciaranmcveigh5
Copy link
Contributor

  • scheduler implementation for builder proposer

category: feature
ticket: #809

@ciaranmcveigh5 ciaranmcveigh5 added the Let's discuss Discussion for general feedback and positive criticism :) label Jul 21, 2022
@ciaranmcveigh5
Copy link
Contributor Author

The proposer duty type originates in Scheduler (in Charon)

As a first pass on the builder api for me it makes sense to exclusively schedule normal blocks or builder blocks

We mentioned having a flag to enable the builder-api this flag would disable normal blocks from being scheduled

thoughts?

err = s.resolveProDuties(ctx, slot, vals)
if err != nil {
return err
// TODO: replace if false with check for builder api flag
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be some check on the builder api flag not sure how we want to handle it for now

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I think you can just add a builderAPI bool variable/field to New and Scheduler. We can add the flag itself in another PR. But it would basically be adding BuilderAPI bool field to app.Config and wiring it up in the cmd and app packages.

@@ -317,7 +325,12 @@ func (s *Scheduler) resolveProDuties(ctx context.Context, slot slot, vals valida
continue
}

// TODO: replace if false with check for builder api flag
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have 2 options for checking for the builder api flag

in the resolveDuties function and implement the resolveBuilderProDuties function in this PR or have one generic resolveProDuties and have the check for the builder api flag in there

In the future i think you'll probably want to schedule both and only execute 1

Some notes from flashbots

  • MEV Boost has a BUILDER_PROPOSAL_DELAY_TOLERANCE which is the deadline within the slot for a builder to have proposed a block (External Block Build)
  • After this deadline the “External Block Build” will be aborted in favour of a “Local Block Build”
  • From MEV Boost documentation, both “External Block Build” and “Local Block Build” should begin as soon as they are able to. The Charon client must ensure one or the other NOT both are submitted.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we schedule just 1 for now to simplify things. Once we know and understand more, we can look at scheduling both and executing one.

Can switch inside resolveProDuties, just flip the duty type basically based on the flag.

@codecov
Copy link

codecov bot commented Jul 21, 2022

Codecov Report

Merging #827 (48348ec) into main (76ed685) will increase coverage by 0.97%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##             main     #827      +/-   ##
==========================================
+ Coverage   53.92%   54.90%   +0.97%     
==========================================
  Files         109      111       +2     
  Lines       11183    11508     +325     
==========================================
+ Hits         6030     6318     +288     
- Misses       4253     4277      +24     
- Partials      900      913      +13     
Impacted Files Coverage Δ
core/scheduler/scheduler.go 71.27% <57.14%> (-0.43%) ⬇️
cmd/run.go 85.29% <0.00%> (-14.71%) ⬇️
core/deadline.go 64.86% <0.00%> (-3.14%) ⬇️
core/bcast/bcast.go 52.17% <0.00%> (-2.83%) ⬇️
core/dutydb/memory.go 71.91% <0.00%> (-1.90%) ⬇️
core/validatorapi/router.go 65.68% <0.00%> (-1.19%) ⬇️
cluster/confighash.go 100.00% <0.00%> (ø)
core/tracker/tracker.go 74.37% <0.00%> (ø)
core/tracker/component_string.go 40.00% <0.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76ed685...48348ec. Read the comment docs.

err = s.resolveProDuties(ctx, slot, vals)
if err != nil {
return err
// TODO: replace if false with check for builder api flag
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I think you can just add a builderAPI bool variable/field to New and Scheduler. We can add the flag itself in another PR. But it would basically be adding BuilderAPI bool field to app.Config and wiring it up in the cmd and app packages.

@@ -317,7 +325,12 @@ func (s *Scheduler) resolveProDuties(ctx context.Context, slot slot, vals valida
continue
}

// TODO: replace if false with check for builder api flag
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we schedule just 1 for now to simplify things. Once we know and understand more, we can look at scheduling both and executing one.

Can switch inside resolveProDuties, just flip the duty type basically based on the flag.

@corverroos corverroos changed the title core: scheduler implementation for builder proposer scheduler: add support for duty builder proposer Jul 21, 2022
@corverroos corverroos changed the title scheduler: add support for duty builder proposer core/scheduler: add support for duty builder proposer Jul 21, 2022
@ciaranmcveigh5 ciaranmcveigh5 added merge when ready Indicates bulldozer bot may merge when all checks pass and removed Let's discuss Discussion for general feedback and positive criticism :) labels Jul 21, 2022
@obol-bulldozer obol-bulldozer bot merged commit 0f996c2 into ObolNetwork:main Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants