Conversation
There was a problem hiding this comment.
Looks good, but I guess we have to fix/circunvent some tests (at least for now)
test_antl_grammar.sh needs to skip slotnum_osaka_function.sol for chk_antlr_grammar to pass.
We can also add a test case in test/libsolidity/ViewPureChecker.cpp and syntaxTests/constants/initialization/block_tx_msg_property.sol (although I guess they depend on restricting the evm version).
|
I updated our guide to cover new opcodes with a high-level builtin based on our past PRs. I did not find anything missing, but please double check: https://notes.argot.org/@solidity-new-opcode-guide#Special-case-Opcodes-with-a-high-level-Solidity-builtin. Especially the Yul part and test coverage. |
Added some additional points to the testing part. |
07faaa5 to
9412786
Compare
cameel
left a comment
There was a problem hiding this comment.
Other than the two things below, it seems fine, but I only skimmed through it (and did not review the SMT part too carefully). Up to @matheusaaguiar and @blishko to approve it when they think it's ready.
| * EVM: Support `block.slotnum` to access the beacon chain slot number of the current block, available since the Amsterdam EVM version ([EIP-7843](https://eips.ethereum.org/EIPS/eip-7843)). | ||
|
|
||
| Compiler Features: | ||
| * Yul: Introduce builtin `slotnum()` for the `SLOTNUM` opcode, available since the Amsterdam EVM version ([EIP-7843](https://eips.ethereum.org/EIPS/eip-7843)). |
There was a problem hiding this comment.
This is a language feature.
There was a problem hiding this comment.
Needs a separate entry for SMT checker as well.
There was a problem hiding this comment.
Moved and change log added.
There was a problem hiding this comment.
Does not look moved yet.
Also, the SMTChecker change, if it was possible with any other member then it's bugfix. If it wasn't and the problem only appeared with the addition of slotnum I would not mention it at all - it's just a shim, not a proper feature. #16884 will add an entry for it.
| uint constant slotnumGlobal = block.slotnum; | ||
|
|
||
| contract A { | ||
| uint constant slotnum = block.slotnum; |
There was a problem hiding this comment.
We should have a syntax test showing that the builtin returns uint64 rather than uint. You cannot test that directly, but you can show that uint64 works and uint56 does not.
Also, it would be better if the existing tests were generally using uint64 rather than uint, especially the semantic ones.
There was a problem hiding this comment.
Added to semanticTests/conversion
|
The SMT part seems OK to me, but we should as an SMTChecker test to show that we are hitting the change. |
nikola-matic
left a comment
There was a problem hiding this comment.
I looked it over as well and aside from Kamil's comments (uint64 test) and changelog, we can merge. Please fix and I'll approve.
And SMTChecker test. |
Unfortunately SMTChecker tests hardcodes evm version to the current one. This change requires some small infra change, but we have a cmdLineTest which check this test/cmdlineTests/model_checker_unsupported_block_slotnum/err. |
blishko
left a comment
There was a problem hiding this comment.
Looks good from my side!
Description
This PR implements support for
SLOTNUMopcode in the EVM.Introduces:
slotnumin theblockglobal variable on solidity language levelslotnum()function in yul and inline assemblyslotnumidentifier in the yul and inline assembly starting fromamsterdamfork.Comment:
Adding support in
libsolidity/formalmakes some tests start failing because of lack of resources. Increasing available resources for Z3 solver makes other tests failing on CI, because of lack of resources on the building machine. I introduced additional check in theSMTEncoderto report theblock.slotnumis not supported by the solver yet. Other way it crashes. Additional PR is pushed which implements the support in the SMT solver but the resource issue has to be resolved first.AI Disclosure
Claude Code, Anthropic's CLI for Claude (running on the Sonnet 5 model).