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.

Sign up for GitHub

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

Jump to bottom

7390 consensus block value #7574

Merged
merged 27 commits into from
Nov 1, 2023
Merged

7390 consensus block value #7574

merged 27 commits into from
Nov 1, 2023

Conversation

mehdi-aouadi
Copy link
Contributor

@mehdi-aouadi mehdi-aouadi commented Oct 4, 2023 •
edited
Loading

PR Description

Add the consensus block value to the Block V3 API:

Fixed Issue(s)

fixes #7390

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@mehdi-aouadi mehdi-aouadi self-assigned this Oct 4, 2023
@mehdi-aouadi mehdi-aouadi marked this pull request as draft October 5, 2023 07:45
@mehdi-aouadi mehdi-aouadi marked this pull request as ready for review October 5, 2023 15:11
@mehdi-aouadi mehdi-aouadi marked this pull request as draft October 6, 2023 08:12
@mehdi-aouadi mehdi-aouadi force-pushed the 7390-consensus-block-value branch 2 times, most recently from fbf04d2 to 357d56d Compare October 24, 2023 19:18
@mehdi-aouadi mehdi-aouadi marked this pull request as ready for review October 24, 2023 19:19
zilm13
zilm13 reviewed Oct 29, 2023
View reviewed changes
Copy link
Contributor

@zilm13 zilm13 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

You need to set consensus value somewhere in similar way you did it for local payload value, extend this method definition with consensusBlockValueResult https://github.com/mehdi-aouadi/teku/blob/fa84b508f6a978201d33e0628da3bc59dd0e8651/ethereum/spec/src/main/java/tech/pegasys/teku/spec/executionlayer/ExecutionLayerChannel.java#L134-L140 and set it in similar way you did local value above this line https://github.com/mehdi-aouadi/teku/blob/0b65bfac2ba63a4091e583889a90701607e926e3/ethereum/executionlayer/src/main/java/tech/pegasys/teku/ethereum/executionlayer/ExecutionBuilderModule.java#L209 with builderBidValue and filling it empty in all other cases (assuming it == localPayloadValue in all other cases). And extend ExecutionPayloadResult as well. RewardCalculator is designed to calculate CL side rewards from inclusion etc, it didn't count EL side rewards and vice versa.

@zilm13
Copy link
Contributor

zilm13 commented Oct 29, 2023

I'm stupid, I didn't read the spec carefully

zilm13
zilm13 approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

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

LGTM, just I think we need to add a minor fix to handle exception in RewardCalculator

data/provider/src/main/java/tech/pegasys/teku/api/ValidatorDataProvider.java Outdated Show resolved Hide resolved
@mehdi-aouadi mehdi-aouadi merged commit e7e856e into Consensys:master Nov 1, 2023
15 checks passed
@mehdi-aouadi mehdi-aouadi deleted the 7390-consensus-block-value branch November 1, 2023 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zilm13 zilm13 zilm13 approved these changes

Assignees

@mehdi-aouadi mehdi-aouadi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add block v3 endpoint
2 participants
@mehdi-aouadi @zilm13