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

changes required for EIP-7742 #574

Closed

Conversation

ralexstokes
Copy link
Member

@ralexstokes ralexstokes commented Aug 21, 2024

see motivation etc here: ethereum/consensus-specs#3800

src/engine/prague.md Outdated Show resolved Hide resolved
Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm

@g11tech
Copy link
Contributor

g11tech commented Oct 19, 2024

actually i think we might need to add baseFeePerBlobGas in beacon's execution header and hence the api field, because the blob gas price can't just be now : minFee* e^(ecessBlobGasg/blobGasPriceUpdateFraction) since blobGasPriceUpdateFraction (for limiting change 1+-1.125 factor) could change block to block.

so we should return to the normal pricing mechanism like basefee, so blobBaseFee = parent.blobBaseFee * e^((blobGasUsed - targetBlobGas)/blockAdjustedTargetFraction))

where blockAdjustedTargetFraction makes sure fee doesn't "change" by 1.125 either direction and is calculated block to block.

so in EL block header, we now remove excessBlobGas and add three fields: baseFeePerBlobGas, targetBlobGas and maxBlobGas (since we don't want this to be a hardfork based setting anymore as well, and don't want to keep a static fraction between target and max for better control over the blob throughput and bandwidth params), and beacon execution header we just add baseFeePerBlobGas

if it makes sense i could update the EIP 7742 with the gas calc changes and introducing the new fields

@siladu
Copy link
Contributor

siladu commented Oct 28, 2024

Updated eth Block schema as well: #601

src/engine/prague.md Outdated Show resolved Hide resolved
src/engine/prague.md Outdated Show resolved Hide resolved
src/engine/prague.md Outdated Show resolved Hide resolved
src/engine/prague.md Outdated Show resolved Hide resolved
src/engine/prague.md Outdated Show resolved Hide resolved
@klkvr klkvr mentioned this pull request Oct 31, 2024
3 tasks
@ensi321
Copy link
Contributor

ensi321 commented Nov 19, 2024

Would probably want to rebase this so that targetBlobsPerBlock(targetBlobCount?) is placed after executionRequests in the param of engine_newPayloadV4

@siladu
Copy link
Contributor

siladu commented Dec 4, 2024

Updated eth_ Block schema with targetBlobsPerBlock as well #601

@@ -9,19 +9,62 @@ This specification is based on and extends [Engine API - Cancun](./cancun.md) sp
<!-- START doctoc generated TOC please keep comment here to allow auto update -->
<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->

- [Structures](#structures)
Copy link
Member

Choose a reason for hiding this comment

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

gotta add execution payload v4 here I think

Copy link
Member Author

Choose a reason for hiding this comment

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

ty, fixed

- [Update the methods of previous forks](#update-the-methods-of-previous-forks)

<!-- END doctoc generated TOC please keep comment here to allow auto update -->

## Structures

### ExecutionPayloadV4
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t think we need a new ExecutionPayload definition, since we have targetBlobsPerBlock passed as a param to newPayloadV4

Copy link
Member Author

Choose a reason for hiding this comment

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

that's the value coming from the CL, the EL needs a way to capture what is on-chain

the EL's job here is to verify the value in the EL header matches what the CL is sending

Copy link
Collaborator

Choose a reason for hiding this comment

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

right, but the treatment of this value looks similar to parentBeaconBlockRoot, i don’t get why the targetBlobsPerBlock should be a part of the payload

Copy link
Member Author

Choose a reason for hiding this comment

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

if we don't do it this way, we would need to have the CL validate this field in the ExecutionPayload

and I just took a look at how we handle the parent beacon block root, and kind of agree we should do this check there and can skip passing it via the ExecutionPayloadV4 here

let's see if we decide to ship EIP-7742 in pectra and we can make these updates in that case

@ralexstokes
Copy link
Member Author

ralexstokes commented Dec 12, 2024

closing as we decided against Pectra inclusion on ACDC #147

@ralexstokes ralexstokes deleted the eip-7742-blob-uncoupling branch December 12, 2024 22:35
@siladu
Copy link
Contributor

siladu commented Dec 16, 2024

Also reverting the eth_ block schema changes #610

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.

8 participants