EIP-7685: General purpose execution layer requests #1083

gumb0 · 2024-12-09T16:48:35Z

https://eips.ethereum.org/EIPS/eip-7685

Introduce Requests type.
Calculate hash of the list of requests after transactions execution.
Read expected hash from blockchain test files and compare with result of calculation.
Use requests hash calculation function in t8n.

(EIP-7685 tests are not enabled yet, because they require all of EIP-6110, 7002 and 7251 to be implemented.)

gumb0 · 2024-12-09T17:34:14Z

test/state/requests.cpp

+        //        if (requests.data.empty())
+        //            continue;
+
+        const bytes requests_bytes = requests.type + requests.data;


This can be optimized to get rid of copying, if we represent Requests as just struct Requests { bytes data; } and put type as the first byte of data right away when collecting requests.

Let me know if it's worth it. I think not really, as it only affects blockchain test runner performance.

I think this is worth it when all EIPs are implemented. So let's make an issue to track it.

This can be also handled by init-update-final API for sha256.

BTW, is there rationale for this double-hashing?

I think this is worth it when all EIPs are implemented. So let's make an issue to track it.

#1086

BTW, is there rationale for this double-hashing?

Good question, I couldn't find any in the EIP, other than previously it was a root of MPT and now is hash of flat list of hashes.
Asked on magicians https://ethereum-magicians.org/t/eip-7685-general-purpose-execution-layer-requests/19668/20

codecov · 2024-12-09T20:25:35Z

Codecov Report

Attention: Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.29%. Comparing base (94582ff) to head (b72a12a).

Files with missing lines	Patch %	Lines
test/state/requests.hpp	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1083   +/-   ##
=======================================
  Coverage   94.29%   94.29%           
=======================================
  Files         157      159    +2     
  Lines       17080    17102   +22     
=======================================
+ Hits        16105    16127   +22     
  Misses        975      975

Flag	Coverage Δ
eof_execution_spec_tests	`23.65% <100.00%> (+0.12%)`	⬆️
ethereum_tests	`26.49% <26.08%> (-0.01%)`	⬇️
ethereum_tests_silkpre	`19.08% <0.00%> (-0.02%)`	⬇️
execution_spec_tests	`20.35% <100.00%> (+0.12%)`	⬆️
unittests	`89.24% <4.34%> (-0.11%)`	⬇️

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

Files with missing lines	Coverage Δ
test/blockchaintest/blockchaintest.hpp	`0.00% <ø> (ø)`
test/blockchaintest/blockchaintest_loader.cpp	`98.73% <100.00%> (+0.01%)`	⬆️
test/blockchaintest/blockchaintest_runner.cpp	`79.20% <100.00%> (+2.12%)`	⬆️
test/state/requests.cpp	`100.00% <100.00%> (ø)`
test/state/requests.hpp	`0.00% <0.00%> (ø)`

pdobacz

LGTM overall, I left some minor comments. Also, maybe we're ready to undo the workarounds from https://github.com/ethereum/evmone/pull/1064/files ?

pdobacz · 2024-12-10T09:55:25Z

test/state/requests.hpp

+struct Requests
+{
+    /// Request type.
+    uint8_t type = 0;


is having a default good here?

also, maybe introduce a Request::Type enum like for tx?

I was meaning to just introduce constants for types in respective later PRs, but enum is a better idea, I'll add it in the next PR.

Having a default in general is good, otherwise it may be left uninitialized.

Having a default in general is good, otherwise it may be left uninitialized.

ah right, that would not error out :(. OK then

added enum here

gumb0 · 2024-12-10T10:40:09Z

Also, maybe we're ready to undo the workarounds from https://github.com/ethereum/evmone/pull/1064/files ?

Ah I forgot about t8n, thanks for the reminder. I'll look into it, it probably will go to a separate PR, too.

pdobacz · 2024-12-10T11:41:11Z

Ah I forgot about t8n, thanks for the reminder. I'll look into it, it probably will go to a separate PR, too.

OK, but also maybe this in particular is better off here to make the change more atomic? Also, t8n has an integration test, so your code would also get automatically covered by that for extra assurance.

test/state/requests.hpp

chfast · 2024-12-10T11:44:02Z

test/state/requests.hpp

+using RequestsList = std::vector<Requests>;
+
+/// Calculate commitment value of block requests list
+hash256 calculate_requests_hash(std::span<const Requests> block_requests_list);


Suggested change

hash256 calculate_requests_hash(std::span<const Requests> block_requests_list);

hash256 calculate_requests_hash(std::span<const Requests> requests_list);

chfast · 2024-12-10T11:45:01Z

test/state/requests.hpp

+};
+
+/// Block requests list - container of `requests` objects, ordered by request type.
+using RequestsList = std::vector<Requests>;


I'd skip this alias because we also use std::span<Requests>.

chfast · 2024-12-10T11:46:02Z

test/state/requests.cpp

+hash256 calculate_requests_hash(std::span<const Requests> block_requests_list)
+{
+    bytes requests_hash_list;
+    requests_hash_list.reserve(sizeof(bytes32) * block_requests_list.size());


Suggested change

requests_hash_list.reserve(sizeof(bytes32) * block_requests_list.size());

requests_hash_list.reserve(sizeof(hash256) * block_requests_list.size());

chfast · 2024-12-10T11:47:33Z

test/state/requests.cpp

+        //        if (requests.data.empty())
+        //            continue;
+
+        const bytes requests_bytes = requests.type + requests.data;


I think this is worth it when all EIPs are implemented. So let's make an issue to track it.

Introduce Requests and RequestsList types. Calculate hash of the list of requests after transactions execution. Read expected hash from blockchain test files and compare with result of calculation.

test/t8n/t8n.cpp

Copilot

Copilot reviewed 1 out of 9 changed files in this pull request and generated no suggestions.

Files not reviewed (8)

test/blockchaintest/blockchaintest.hpp: Language not supported
test/blockchaintest/blockchaintest_loader.cpp: Language not supported
test/blockchaintest/blockchaintest_runner.cpp: Language not supported
test/state/CMakeLists.txt: Language not supported
test/state/requests.cpp: Language not supported
test/state/requests.hpp: Language not supported
test/t8n/CMakeLists.txt: Language not supported
test/t8n/t8n.cpp: Language not supported

gumb0 force-pushed the eip-7685 branch from 84baf92 to 5a329be Compare December 9, 2024 16:49

gumb0 changed the title ~~Implement EIP-7685: General purpose execution layer requests~~ EIP-7685: General purpose execution layer requests Dec 9, 2024

gumb0 force-pushed the eip-7685 branch 3 times, most recently from c696b91 to 2a1cfaf Compare December 9, 2024 17:27

gumb0 commented Dec 9, 2024

View reviewed changes

gumb0 force-pushed the eip-7685 branch from 2a1cfaf to 0338f29 Compare December 9, 2024 17:36

gumb0 force-pushed the eip-7685 branch from 2287cae to b72a12a Compare December 9, 2024 20:28

gumb0 marked this pull request as ready for review December 9, 2024 20:43

gumb0 requested review from chfast and pdobacz December 9, 2024 20:43

gumb0 self-assigned this Dec 9, 2024

pdobacz reviewed Dec 10, 2024

View reviewed changes

chfast added the Prague Changes for Prague upgrade label Dec 10, 2024

pdobacz approved these changes Dec 10, 2024

View reviewed changes

chfast reviewed Dec 10, 2024

View reviewed changes

gumb0 added 2 commits December 10, 2024 19:09

Implement EIP-7685: General purpose execution layer requests

f4650d7

Introduce Requests and RequestsList types. Calculate hash of the list of requests after transactions execution. Read expected hash from blockchain test files and compare with result of calculation.

Update EEST version on CI

cb9ffe4

gumb0 force-pushed the eip-7685 branch 2 times, most recently from 229cdf7 to b991667 Compare December 10, 2024 18:29

gumb0 requested review from chfast and pdobacz December 10, 2024 18:43

pdobacz approved these changes Dec 10, 2024

View reviewed changes

test/t8n/t8n.cpp Show resolved Hide resolved

t8n: Use common request hash calculation function

eeb44f4

gumb0 force-pushed the eip-7685 branch from b991667 to eeb44f4 Compare December 11, 2024 10:28

chfast requested a review from Copilot December 11, 2024 14:11

Copilot AI reviewed Dec 11, 2024

View reviewed changes

chfast approved these changes Dec 11, 2024

View reviewed changes

gumb0 merged commit 2939b9f into master Dec 11, 2024
23 checks passed

gumb0 deleted the eip-7685 branch December 11, 2024 16:18

	hash256 calculate_requests_hash(std::span<const Requests> block_requests_list);
	hash256 calculate_requests_hash(std::span<const Requests> requests_list);

	requests_hash_list.reserve(sizeof(bytes32) * block_requests_list.size());
	requests_hash_list.reserve(sizeof(hash256) * block_requests_list.size());

EIP-7685: General purpose execution layer requests #1083

EIP-7685: General purpose execution layer requests #1083

Uh oh!

Conversation

gumb0 commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pdobacz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gumb0 commented Dec 10, 2024

Uh oh!

pdobacz commented Dec 10, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gumb0 commented Dec 9, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading