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

pectra devnet4: implement pectra devnet4 spec #3706

Merged
merged 53 commits into from
Oct 31, 2024
Merged

Conversation

g11tech
Copy link
Contributor

@g11tech g11tech commented Sep 27, 2024

@jochem-brouwer
Copy link
Member

jochem-brouwer commented Sep 27, 2024

Can we use this PR as devnet-4 branch and not merge this until devnet-4 is specced out completely?

Ref for further additions: ethereum/execution-spec-tests#832

Devnet-4 spec https://notes.ethereum.org/@ethpandaops/pectra-devnet-4

@g11tech
Copy link
Contributor Author

g11tech commented Sep 27, 2024

yes apparently the address specified in the 7002 eip and in the genesis generator changes don't match
cc @parithosh

in EIP:
image

in genesis generator PR:
image

@parithosh
Copy link

ah there's probably an update missing. lemme me clear that up and get back to you!

@g11tech
Copy link
Contributor Author

g11tech commented Sep 27, 2024

are you sure @g11tech ?

EIP update was merged in: https://github.com/ethereum/EIPs/pull/8890/files#diff-70472fac1debb567783ce13873323261713f3ce488a26a5c3769a9193f4a7e27R521

which is the address we used in the genesis generator: https://github.com/ethpandaops/ethereum-genesis-generator/pull/143/files#diff-e563a01282cd9e2db47dceb99a9337f1f37466f6bfe5d10b376435d51f179f34R114

yes i am sure as of the current published EIP :)

image

as well as latest github file

image

@jochem-brouwer
Copy link
Member

I just manually checked the EIP-7002 data, I get this:

sender: 0xac6afb9d8491e8b397f65331ce41e338cbfe1048
contract address: '0x0511ce19514e1298fba96de582652a016e2caaaa'

@jochem-brouwer
Copy link
Member

@parithosh I just saw that the address got updated again in this commit: ethereum/EIPs@a23be81

In this PR:
ethereum/EIPs#8855

This changed code, so it also changed the address. (I assume this is included in devnet-4?)

@@ -78,7 +78,7 @@ export const paramsVM: ParamsDict = {
7002: {
// config
systemAddress: '0xfffffffffffffffffffffffffffffffffffffffe', // The system address to perform operations on the withdrawal requests predeploy address
withdrawalRequestPredeployAddress: '0x00A3ca265EBcb825B45F985A16CEFB49958cE017', // Address of the validator excess address
withdrawalRequestPredeployAddress: '0x05F27129610CB42103b665629CB5c8C00296AaAa', // Address of the validator excess address
Copy link
Member

Choose a reason for hiding this comment

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

This is the address of this commit: ethereum/EIPs@1335765

The latest commit (which shows up on eips.ethereum.org) is ethereum/EIPs@a23be81 pointing to 0x0511Ce19514e1298Fba96de582652A016E2CAaAa

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes @parithosh mentioned there are a few more updates coming through, so we gotta hold on

@jochem-brouwer
Copy link
Member

Fixed merge conflicts, if I messed up please rollback to 7cd91a2

@jochem-brouwer
Copy link
Member

Have updated the PR (have not fixed tests) to reflect these EIP PRs:

ethereum/EIPs#8924
ethereum/EIPs#8934

I have also added support for the devnet-4 t8ntool interface, can be used to fill ethereum/execution-spec-tests#832 (I got a few filled tests, so the interface works, until I ran in a bug at EEST side).

@jochem-brouwer
Copy link
Member

We can actually get rid of the whole DepositRequest, WithdrawalRequest, etc, since now the requests only have a type and whatever the internal bytes are have (for us) no structure other than "just" being bytes. Also see ethereum/execution-apis#591

@holgerd77
Copy link
Member

We can actually get rid of the whole DepositRequest, WithdrawalRequest, etc, since now the requests only have a type and whatever the internal bytes are have (for us) no structure other than "just" being bytes. Also see ethereum/execution-apis#591

Oh, that's a major one we should absolutely do before the releases, can you please (don't need to be long) open a dedicated issue on this so that we do not forget?

@jochem-brouwer
Copy link
Member

jochem-brouwer commented Oct 10, 2024

The idea was actually to directly do that here :)

EDIT: this is also the result of the EIP revamps for the requests, I think this will naturally follow once we integrate it.

@jochem-brouwer
Copy link
Member

jochem-brouwer commented Oct 10, 2024

Never mind, removing them actually does not sounds like a good idea. The classes are now handy to also directly decode the raw bytes, which is handy for the end-user.

index: bigIntToHex(this.index),
}
export class DepositRequest extends CLRequest<CLRequestType.Deposit> {
constructor(requestData: Uint8Array) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it useful to have these sub classes now since they are functionally identical? Couldn't we just have a CLRequest class and then use the type property to distinguish between them?

export function createCLRequest<T extends CLRequestType>(bytes: Uint8Array): CLRequest<T> {
switch (bytes[0]) {
case CLRequestType.Deposit:
return new DepositRequest(bytes.subarray(1)) as CLRequest<T>
Copy link
Contributor

Choose a reason for hiding this comment

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

As above. Do we really need this? Feels very duplicative.

acolytec3
acolytec3 previously approved these changes Oct 31, 2024
Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

Looks good from my perspective, though the CLRequest code looks needlessly duplicative to my untrained eye.

@jochem-brouwer jochem-brouwer merged commit b37a8d9 into master Oct 31, 2024
38 of 41 checks passed
@jochem-brouwer
Copy link
Member

Just merged, thanks a lot all! 😄 👍

@@ -4,7 +4,7 @@ export * from './consensus/index.js'
export { type BeaconPayloadJSON, executionPayloadFromBeaconPayload } from './from-beacon-payload.js'
export * from './header/index.js'
export {
genRequestsTrieRoot,
genRequestsRoot,
Copy link
Member

Choose a reason for hiding this comment

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

Should we call this genRequestsHash ?

Copy link
Member

Choose a reason for hiding this comment

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

Yep this is actually a better name since it is not a trie root anymore!

@holgerd77
Copy link
Member

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants