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

EIP 4844 (part 2) #14759

Merged
merged 1 commit into from
Jan 22, 2024
Merged

EIP 4844 (part 2) #14759

merged 1 commit into from
Jan 22, 2024

Conversation

r0qs
Copy link
Member

@r0qs r0qs commented Dec 27, 2023 •
edited
Loading

Depends on #14757 (merged)
Second part of #14740
Fixes #14740

@r0qs r0qs changed the title Eip 4844 add blobhash (part 2) EIP 4844 (part 2) Dec 27, 2023
@r0qs r0qs added has dependencies The PR depends on other PRs that must be merged first feature labels Dec 27, 2023
@r0qs r0qs added this to the 0.8.24 milestone Dec 27, 2023
@r0qs r0qs self-assigned this Dec 27, 2023
@r0qs r0qs force-pushed the eip-4844-add-blobhash branch from e65d8bc to 69122e0 Compare December 27, 2023 21:43
@r0qs r0qs force-pushed the eip-4844-add-blobhash-2 branch from cbec8b1 to e2f1fad Compare December 27, 2023 21:49
@r0qs r0qs force-pushed the eip-4844-add-blobhash-2 branch 3 times, most recently from 88e1c07 to d5ae8de Compare January 4, 2024 13:31
@r0qs r0qs marked this pull request as ready for review January 4, 2024 13:33
@r0qs r0qs force-pushed the eip-4844-add-blobhash branch from 69122e0 to 66e145f Compare January 5, 2024 12:28
@r0qs r0qs force-pushed the eip-4844-add-blobhash-2 branch 3 times, most recently from 3ecd0e1 to a6c2bce Compare January 5, 2024 14:21
@cameel cameel force-pushed the eip-4844-add-blobhash branch from 66e145f to 9eb631b Compare January 6, 2024 19:32
@cameel cameel force-pushed the eip-4844-add-blobhash-2 branch from a6c2bce to cd106b6 Compare January 6, 2024 20:24
@cameel cameel mentioned this pull request Jan 6, 2024
Merged
cameel
cameel reviewed Jan 8, 2024
View reviewed changes
Changelog.md Outdated Show resolved Hide resolved
@r0qs r0qs force-pushed the eip-4844-add-blobhash branch 2 times, most recently from c65588e to 08bbb84 Compare January 9, 2024 16:02
@r0qs r0qs force-pushed the eip-4844-add-blobhash-2 branch 2 times, most recently from 20201ee to 4b71758 Compare January 9, 2024 17:07
@r0qs r0qs force-pushed the eip-4844-add-blobhash branch from 08bbb84 to d01cd35 Compare January 9, 2024 20:32
@r0qs r0qs force-pushed the eip-4844-add-blobhash-2 branch from 4b71758 to 4c3e118 Compare January 9, 2024 20:36
@r0qs r0qs force-pushed the eip-4844-add-blobhash-2 branch 3 times, most recently from c7f1c0e to 652fd12 Compare January 18, 2024 17:30
@r0qs r0qs force-pushed the eip-4844-add-blobhash-2 branch 3 times, most recently from f84b810 to 652696a Compare January 18, 2024 19:46
@cameel
Copy link
Member

cameel commented Jan 18, 2024

Like the blobbasefee PR, this is missing the fuzzer support (to be dealt with by @bshastry) and SMTChecker support (please either add it to #14780 or create a separate issue).

cameel
cameel reviewed Jan 18, 2024
View reviewed changes
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

That's all I had. Once my remaining comments are addressed, we can merge this.

@r0qs r0qs force-pushed the eip-4844-add-blobhash-2 branch from 652696a to 684188f Compare January 18, 2024 20:47
@r0qs r0qs mentioned this pull request Jan 18, 2024
Open
@cameel cameel removed the has dependencies The PR depends on other PRs that must be merged first label Jan 19, 2024
@r0qs r0qs force-pushed the eip-4844-add-blobhash-2 branch from 684188f to 8536b57 Compare January 19, 2024 18:43
@nikola-matic nikola-matic force-pushed the eip-4844-add-blobhash-2 branch from 8536b57 to e39e8f1 Compare January 22, 2024 07:50
nikola-matic
nikola-matic reviewed Jan 22, 2024
View reviewed changes
docs/using-the-compiler.rst Show resolved Hide resolved
test/libsolidity/syntaxTests/viewPureChecker/builtin_blobhash_restrict_warning.sol Outdated Show resolved Hide resolved
libsolidity/codegen/ExpressionCompiler.cpp
Comment on lines +1038 to +1044
case FunctionType::Kind::BlobHash:
{
acceptAndConvert(*arguments[0], *function.parameterTypes()[0], true);
m_context << Instruction::BLOCKHASH;
if (function.kind() == FunctionType::Kind::BlockHash)
m_context << Instruction::BLOCKHASH;
else
m_context << Instruction::BLOBHASH;
Copy link
Collaborator

@nikola-matic nikola-matic Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure I really like this approach - these are separate cases for a reason - why would you combine them and then perform another check as to what the function kind is? Of course, it's easy to see what's going on once you read the code, but it's confusing at first glance - initially, I thought they may have the same opcode and use the difficulty/prevrandao mechanism based on the fork, which is obviously not the case.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this one is short enough that it would have been clearer as a separate handler.

Still, when the code is longer this approach makes sense. The handler in IRGeneratorForStatements.cpp was actually the opposite case - there merging it with the other cases helps reduce duplication.

Copy link
Member Author

@r0qs r0qs Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I initially implemented them separately, but as pointed out in #14759 (comment) there are instances where multiple built-ins are handled within the same case, both in IRGeneratorForStatements and ExpressionCompiler.

Copy link
Member

Choose a reason for hiding this comment

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

Which is why I didn't complain about it myself, but @nikola-matic does have a point here :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@r0qs we seem to have wormholed ourselves into a parallel universe where Kamil doesn't complain, and I do. :D

@r0qs r0qs force-pushed the eip-4844-add-blobhash-2 branch 2 times, most recently from f9d3a14 to 763869d Compare January 22, 2024 13:50
@r0qs r0qs changed the base branch from develop to testing-tweaks-before-mcopy January 22, 2024 13:50
Base automatically changed from testing-tweaks-before-mcopy to develop January 22, 2024 13:51
@r0qs r0qs mentioned this pull request Jan 22, 2024
Closed
@cameel cameel force-pushed the eip-4844-add-blobhash-2 branch from 763869d to 81268e3 Compare January 22, 2024 18:28
@cameel cameel enabled auto-merge January 22, 2024 18:28
@cameel cameel merged commit 01cb85f into develop Jan 22, 2024
64 of 65 checks passed
@cameel cameel deleted the eip-4844-add-blobhash-2 branch January 22, 2024 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stackenbotten stackenbotten stackenbotten left review comments

@nikola-matic nikola-matic nikola-matic left review comments

@ekpyron ekpyron ekpyron left review comments

@cameel cameel cameel approved these changes

Assignees

@r0qs r0qs

Labels
feature
Projects
None yet
Milestone
0.8.24
Development

Successfully merging this pull request may close these issues.

Support for BLOBHASH from EIP-4844
5 participants
@r0qs @cameel @stackenbotten @ekpyron @nikola-matic