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

prepare builder API, modify electra BuilderBid #6872

Open
wants to merge 2 commits into
base: unstable
Choose a base branch
from
Open

prepare builder API, modify electra BuilderBid #6872

wants to merge 2 commits into from

Conversation

agnxsh
Copy link
Contributor

@agnxsh agnxsh commented Jan 25, 2025

No description provided.

Copy link

github-actions bot commented Jan 25, 2025

Unit Test Results

       15 files  ±0    2 285 suites  ±0   1h 7m 51s ⏱️ + 2m 39s
  5 368 tests ±0    5 021 ✔️ ±0  347 💤 ±0  0 ±0 
37 069 runs  ±0  36 579 ✔️ ±0  490 💤 ±0  0 ±0 

Results for commit 4d89d95. ± Comparison against base commit 5547d2a.

♻️ This comment has been updated with latest results.

@tersec
Copy link
Contributor

tersec commented Jan 26, 2025

Run excluded_files="config.yaml|config.nims|beacon_chain.nimble"
The following files do not have an up-to-date copyright year:
- beacon_chain/spec/mev/electra_mev.nim
- beacon_chain/spec/mev/fulu_mev.nim
Error: Process completed with exit code 2.

@tersec
Copy link
Contributor

tersec commented Jan 26, 2025

The comment at

debugComment "verify (again) that this is what builder API needs"

is addressed by this PR and should be removed.

executionRequests: Opt.none(ExecutionRequests),
executionPayloadValue: builderBid.value))
elif EPH is electra_mev.BlindedExecutionPayloadAndBlobsBundle:
return ok(BuilderBid[EPH](
Copy link
Contributor

Choose a reason for hiding this comment

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

The electra_mev.BlindedExecutionPayloadAndBlobsBundle and fulu_mev.BlindedExecutionPayloadAndBlobsBundle cases are identical and can be combined.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, in general whenever nimbus-eth2 has these

  when cond1:
    ...
  elif cond2:
    ...
  elif cond3:
    ...

conditions, it adds on an

  else:
    static: doAssert false

or

  else:
    static: raiseAssert "..."

e.g.

when consensusFork <= ConsensusFork.Deneb:
let penalty_numerator = validator.effective_balance div increment *
adjusted_total_slashing_balance
penalty_numerator div total_balance * increment
elif consensusFork == ConsensusFork.Electra:
let
effective_balance_increments = validator.effective_balance div increment
penalty_per_effective_balance_increment =
adjusted_total_slashing_balance div (total_balance div increment)
# [Modified in Electra:EIP7251]
penalty_per_effective_balance_increment * effective_balance_increments
elif consensusFork == ConsensusFork.Fulu:
let
effective_balance_increments = validator.effective_balance div increment
penalty_per_effective_balance_increment =
adjusted_total_slashing_balance div (total_balance div increment)
# [Modified in Electra:EIP7251]
penalty_per_effective_balance_increment * effective_balance_increments
else:
static: doAssert false

That it's static: ... is important; the exact way it causes the compilation to fail is less important. This way, if/when the conditions fail to hold -- e.g., here, a new fork is added after Fulu -- it won't silently stop working, but rather loudly fail to compile.

executionRequests: Opt.none(ExecutionRequests),
executionPayloadValue: bidValue,
consensusBlockValue: consensusValue))
elif SBBB is electra_mev.SignedBlindedBeaconBlock:
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

2 participants