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

packages: add requests hash to genesis block #3771

Merged

Conversation

spencer-tb
Copy link
Contributor

Description

Adds the request hash to the genesis block to enable running tests on hive: consume engine and consume rlp.

Reproduce Hive Passes

Using the following hive branch spencer-tb@prague-devnet-4-ethjs run either of:

./hive --sim 'ethereum/eest/consume-block-rlp' --client-file configs/prague.yaml --docker.output --client ethereumjs --docker.nocache .
./hive --sim 'ethereum/eest/consume-engine' --client-file configs/prague.yaml --docker.output --client ethereumjs --docker.nocache .

Empty Requests Hash

Note the keccak256 of the empty requests should account for the type prefixing, hence why:

KECCAK256_RLP_RH == 0x6036c41849da9c076ed79654d434017387a88fb833c2856b32e18218b3341c5f

and not the default 0x56e8....

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

This looks great, thanks a lot for this (will have to test locally). One small point regarding correctness and clarity (for devnet-4).

Side-note, in devnet-5 this will get easier, since the empty request hash will be the empty sha256 hash, see:

ethereum/EIPs#8989

(We will nevertheless need this change to import genesis blocks which the requests hash, plus we also need the specific empty hash for devnet-4 :) )

packages/util/src/constants.ts Outdated Show resolved Hide resolved
packages/util/src/constants.ts Outdated Show resolved Hide resolved
@jochem-brouwer jochem-brouwer force-pushed the prague-devnet-4-genesis branch from f8d1bd9 to 11faeca Compare October 25, 2024 15:59
@jochem-brouwer jochem-brouwer changed the base branch from 7702-devnet-4-plus-t8ntool to devnet4-contracts October 25, 2024 16:00
@jochem-brouwer jochem-brouwer force-pushed the prague-devnet-4-genesis branch from 11faeca to d1db80d Compare October 25, 2024 16:01
@jochem-brouwer jochem-brouwer merged commit 7e1c382 into ethereumjs:devnet4-contracts Oct 25, 2024
3 of 4 checks passed
Copy link

codecov bot commented Oct 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 25.46%. Comparing base (ea935e7) to head (d1db80d).
Report is 1 commits behind head on devnet4-contracts.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
client 0.00% <ø> (ø)
common 89.86% <100.00%> (+0.01%) ⬆️
devp2p 0.00% <ø> (ø)
evm 65.21% <ø> (ø)
genesis 0.00% <ø> (ø)
mpt 52.09% <ø> (-0.44%) ⬇️
statemanager 63.85% <ø> (ø)
tx 76.70% <ø> (ø)
util 73.04% <ø> (ø)
vm 58.08% <ø> (ø)
wallet 0.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@jochem-brouwer
Copy link
Member

What does docker.nocache . do? Will it re-build all docker images each time? 🤔

/**
* SHA-256 hash of the RLP of an empty requests hash
*/
export const SHA256_EMPTY_RH = sha256(new Uint8Array([0, 1, 2]))
Copy link
Member

Choose a reason for hiding this comment

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

This here would become part of the official API of the libraries and it would be hard "to get rid of it" if things change, so let's please not do this yet as long as specs are still changing and rather hardcode in blockchain.

Copy link
Member

Choose a reason for hiding this comment

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

Hi Holger, good point, I did somewhat notice this but note that the "empty requests hash" will be gone starting of devnet-5 (see: ethereum/EIPs#8989), where it is the empty sha256 hash and not this thing with encoded constants (so we can get rid of this constant!). But I agree it should not have been put in the util constants (note: this PR is not merged into master, it is merged in our devnet4 branch, so for now this is fine! 😄 )

Copy link
Member

Choose a reason for hiding this comment

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

Should nevertheless not forget to remove this constant 😄 👍

Copy link
Member

Choose a reason for hiding this comment

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

These things DO have some tendency to get forgotten, so I would still pledge to just give this a quick update now and we are done with it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes you are right, will do right away 👍

Copy link
Member

Choose a reason for hiding this comment

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

Done in dc93f8f

jochem-brouwer added a commit that referenced this pull request Oct 31, 2024
* vm: update the system contract addresses for prague devnet4

* change the requests root from trie root to flat root and update examples and spcs

* convert requests to flat type across util,block and vm

* bundle execution requests separately from execution payload

* t8ntool: update to devnet-4 interface support

* update devnet-4 to EIP PRs 8924, 8394

* t8ntool hotfix to fix state tests

* refactor cl requests to the new simplified version

* remove requests from the block and modify associated code paths

* remove storing and retriving of requests from blockchain

* modify the deposit, withdrawal and consolidation requests accumulation for buildblock and runblock and corresponding requestsroot calcs

* modfiy the 7002 eip spec along with the new contract and debug and fix the test including fixing a logs bloom bug in the generate fields block generation

* modify code to correctly patch generated requests on getpayload/build/pending block

* fix the newpayload engine codeflow to validate the cl requests

* remove the requests from eth rpc and blockfetcher p2p

* modify debug and fix 6110 deposit spec test

* update the vm 7685 spec and add todos for later consideration

* fix t8ntool rq output

* vm: fix 6110 requests

* update request to just store bytes and expose getters for data and type and fix the 6110 and 7685 spec

* repo: rename requestsRoot -> requestsHash

* vm: fix import (fix docker build)

* client: correctly return request data (not including type)

* fix the ingress, generation and propagation of execution requests/requestsroot data from the engine api and debug and fix the newpayloadv4 spec

* Update 6110 example

* Use sha256 constant for default

* Fix asserts

* Add sha256 empty string constant

* Update block REAME examples

* Reuse already computed hash

* Fix buildBlock tests

* Fix vm api tests

* Fix client tests

* Fix tests

* packages: add requests hash to genesis block (#3771)

* packages: add requests hash to genesis block.

* Update packages/util/src/constants.ts

Co-authored-by: Jochem Brouwer <[email protected]>

* Update packages/util/src/constants.ts

Co-authored-by: Jochem Brouwer <[email protected]>

* packages: integrate suggestions.

* Update packages/util/src/constants.ts

---------

Co-authored-by: Jochem Brouwer <[email protected]>

* util: correctly report empty rq hash

* blockchain/util: remove sha256_empty_rh from exported util constants

* Remove requests from being passed in as blockData

* Remove old tests that do not conform to new devnet4 specs

* Remove old test that does not conform to new devnet4 specs

* make linter happy

* block: make tsc happy

* block: remove obsolete examples

* util: make tsc happy

* vm: make linter and tsc happy

* blockchain: make linter happy

* vm: fix example

* make cspell happy

* client/util/vm: simplify CLRequest

---------

Co-authored-by: Jochem Brouwer <[email protected]>
Co-authored-by: Amir <[email protected]>
Co-authored-by: spencer <[email protected]>
Co-authored-by: acolytec3 <[email protected]>
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.

3 participants