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.

Sign up for GitHub

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

Jump to bottom

Add BLOBHASH instruction #668

Merged
merged 2 commits into from
Sep 14, 2023
Merged

Add BLOBHASH instruction #668

merged 2 commits into from
Sep 14, 2023

Conversation

chfast
Copy link
Member

@chfast chfast commented Jun 26, 2023

Implement BLOBHASH instruction and add support for blob transactions in state tests.

chfast
chfast commented Jun 26, 2023
View reviewed changes
test/state/state.cpp Outdated Show resolved Hide resolved
test/state/state.hpp Outdated Show resolved Hide resolved
@chfast chfast force-pushed the tx_type branch 3 times, most recently from 4b95a75 to 968caea Compare July 14, 2023 19:14
Base automatically changed from tx_type to master July 14, 2023 19:26
@chfast chfast marked this pull request as ready for review August 23, 2023 10:41
gumb0
gumb0 reviewed Sep 1, 2023
View reviewed changes
test/unittests/evm_eip4844_blobhash_test.cpp

execute(blobhash(1) + ret_top());
EXPECT_OUTPUT_INT(0);
}
Copy link
Member

Choose a reason for hiding this comment

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

You could also test it with 0xff...ff, here and in other tests

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

Copy link
Member

Choose a reason for hiding this comment

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

What about other tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added to all.

@gumb0
Copy link
Member

gumb0 commented Sep 1, 2023

Looks good to me, but Silkworm EVMC mismatch error on CI

@chfast chfast force-pushed the blobhash branch 2 times, most recently from 5a1029a to 9908478 Compare September 1, 2023 15:14
@chfast chfast added the Cancun Changes for Cancun Ethereum spec revision label Sep 2, 2023
@chfast
Copy link
Member Author

chfast commented Sep 2, 2023

Looks good to me, but Silkworm EVMC mismatch error on CI

Fixed now. Starting from #669 we use Silkworm with EVMC 11. It does not support BLOBHASH yet but is ABI-compatible and we make sure the old tests have not been broken.

@codecov
Copy link

codecov bot commented Sep 2, 2023 •
edited
Loading

Codecov Report

Merging #668 (7023e70) into master (28dbb9d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #668      +/-   ##
==========================================
+ Coverage   97.56%   97.58%   +0.01%     
==========================================
  Files          92       93       +1     
  Lines        8547     8610      +63     
==========================================
+ Hits         8339     8402      +63     
  Misses        208      208
Flag Coverage Δ
blockchaintests 62.37% <0.00%> (-0.24%) ⬇️
statetests 73.31% <0.00%> (-0.16%) ⬇️
statetests-silkpre 22.50% <8.06%> (-0.13%) ⬇️
unittests 95.55% <100.00%> (+0.03%) ⬆️

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

Files Changed Coverage Δ
lib/evmone/instructions.hpp 100.00% <100.00%> (ø)
test/unittests/evm_eip4844_blobhash_test.cpp 100.00% <100.00%> (ø)
test/unittests/instructions_test.cpp 89.58% <100.00%> (ø)
test/utils/bytecode.hpp 96.03% <100.00%> (+0.03%) ⬆️

@rodiazet rodiazet requested a review from gumb0 September 14, 2023 09:23
@chfast chfast merged commit 1773501 into master Sep 14, 2023
3 checks passed
@chfast chfast deleted the blobhash branch September 14, 2023 11:51
@yperbasis yperbasis mentioned this pull request Sep 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gumb0 gumb0 gumb0 approved these changes

Assignees
No one assigned
Labels
Cancun Changes for Cancun Ethereum spec revision
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chfast @gumb0