Skip to content

7390 consensus block value#7574

Merged
mehdi-aouadi merged 27 commits intoConsensys:masterfrom
mehdi-aouadi:7390-consensus-block-value
Nov 1, 2023
Merged

7390 consensus block value#7574
mehdi-aouadi merged 27 commits intoConsensys:masterfrom
mehdi-aouadi:7390-consensus-block-value

Conversation

@mehdi-aouadi
Copy link
Copy Markdown
Contributor

@mehdi-aouadi mehdi-aouadi commented Oct 4, 2023

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 force-pushed the 7390-consensus-block-value branch from 139e44c to 43828c8 Compare October 4, 2023 15:52
final Optional<Bytes32> graffiti,
final boolean isBlinded) {
checkBlockProducingParameters(slot, randao);
return validatorApiChannel.createUnsignedBlock(slot, randao, graffiti, isBlinded);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation

Invoking [ValidatorApiChannel.createUnsignedBlock](1) should be avoided because it has been deprecated.
DeserializableOneOfTypeDefinitionBuilder<TObject, TBuilder> object(
@SuppressWarnings("unused") final Class<TObject> type,
@SuppressWarnings("unused") final Class<TBuilder> builderType) {
@SuppressWarnings("unused") final Class<TObject> type) {

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'type' is never used.
@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
@mehdi-aouadi mehdi-aouadi force-pushed the 7390-consensus-block-value branch from 357d56d to a39b192 Compare October 26, 2023 08:50
@mehdi-aouadi mehdi-aouadi requested a review from zilm13 October 26, 2023 08:55
Copy link
Copy Markdown
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.

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
Copy Markdown
Contributor

zilm13 commented Oct 29, 2023

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

@mehdi-aouadi mehdi-aouadi requested a review from zilm13 October 31, 2023 17:31
Copy link
Copy Markdown
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

@mehdi-aouadi mehdi-aouadi force-pushed the 7390-consensus-block-value branch from 99d72a0 to 570efea Compare October 31, 2023 21:45
@mehdi-aouadi mehdi-aouadi force-pushed the 7390-consensus-block-value branch from 3aaf7f1 to 5791009 Compare October 31, 2023 22:04
@mehdi-aouadi mehdi-aouadi merged commit e7e856e into Consensys:master Nov 1, 2023
@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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add block v3 endpoint

2 participants