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

fix: fix block value calculation in produceBlockV3 #6207

Merged
merged 1 commit into from
Dec 22, 2023

Conversation

ensi321
Copy link
Contributor

@ensi321 ensi321 commented Dec 19, 2023

Motivation

The previous PR #6136 it introduces consensusBlockValue and modifies the logic of determining using builder block vs execution block by comparing their values using the direct sum of executionPayloadValue and consensusBlockValue.

    const blockValueBuilder = builderPayloadValue + consensusBlockValueBuilder;
    const blockValueEngine = enginePayloadValue + consensusBlockValueEngine;
...
    if (blockValueEngine >= blockValueBuilder) {
      selectedSource = ProducedBlockSource.engine;
    } else {
      selectedSource = ProducedBlockSource.builder;
    }

This is incorrect because consensus block value is in Gwei while payload value is in Wei. Directly summing the two will result in payload value dominating the entire block value.

Description

  • To convert consensus block value to Wei when calculating the total block value.
  • Explicitly mention total block value is in Wei
  • When logging consensus block value alone, the value should be in Gwei.

Caveat
There is an ongoing discussion in beacon API spec questioning the inconsistency of having different unit (Wei vs Gwei) in payload and CL block value. Will follow up if there is any related change to the spec.

Copy link

codecov bot commented Dec 19, 2023

Codecov Report

Merging #6207 (aa98db2) into unstable (54e8c45) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff            @@
##           unstable    #6207   +/-   ##
=========================================
  Coverage     90.35%   90.35%           
=========================================
  Files            78       78           
  Lines          8087     8087           
  Branches        490      490           
=========================================
  Hits           7307     7307           
  Misses          772      772           
  Partials          8        8           

@ensi321 ensi321 marked this pull request as ready for review December 19, 2023 09:02
@ensi321 ensi321 requested a review from a team as a code owner December 19, 2023 09:02
@g11tech g11tech merged commit 267991a into ChainSafe:unstable Dec 22, 2023
17 of 18 checks passed
@wemeetagain
Copy link
Member

🎉 This PR is included in v1.14.0 🎉

ensi321 added a commit to ensi321/lodestar that referenced this pull request Jan 22, 2024
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.

3 participants