feat: Support airbender delegations in precompiles#209
Conversation
There was a problem hiding this comment.
Changes look ok, but left some comments.
But an important note: I don't think that we actually run these tests in CI. And we do not run anything in RISC-V context at all.
Additionally, imo keccak256 testing should be enhanced. It is true for all precompiles in general, but this is out of scope for this PR.
|
Looks fine, I'll take one more deeper manual review later, after we finalize other parts. I'll leave some AI-generated comments below |
0xVolosnikov
left a comment
There was a problem hiding this comment.
Deep review pass. Three coverage/observability concerns + two trait-shape suggestions. Left inline.
|
@0xVolosnikov -- my pushback is not because I disagree with the feedback; it's just I believe that LLM-induced "improvement loop" can never end in practice. LLMs are good at things that can be criticized, but LLMs are still bad at deciding whether the critique is justified given context. I have had several rounds of LLM-based self-review before publishing myself with different prompting techniques. I would classify provided LLM comments as "technically correct, practically low-impact" (again, given the purpose and original scope of the PR), so to me it sounds like "we've reached the point where LLM suggestions are not that useful, and a human review will provide much more value"; otherwise we might never merge this PR simply because the codebase is not perfect in the first place. |
|
@popzxc sure, agree with you, feel free to ignore if you don't see any value. |
|
@popzxc, flagging a potential issue for your review, please tell me if you think it's off base. The following 5 Wycheproof vectors (tcIds 169, 205, 208, 222, 223) produce divergent verdicts between the two
At the precompile output these map to different memory writes ([1, 0] vs [0, 0]) |
|
@ninolipartiia thank you! Pushed a fix and added provided tests in f7048c2. |
|
@popzxc, thanks, the fix looks good to me. Considering the tests from the wycheproof were added, shouldn't it be reflected in the license? I'd suggest adding more tests from here for |
# Conflicts: # Cargo.toml
This PR adds support for airbender delegations in precompiles, which is a prerequisite for making EraVM provable in airbender.
It does it through
airbender-cryptocrate fromairbender-platfromwhich is adapted from zksync-os dev branch with one exception: keccak delegation was taken from the rr/ethrpoofs branch.Dependency on
popzxc/airbender-platformwithmainbranch is temporary -- before we merge this PR, I expect that we'll move the repo to thematter-labsorg, have a release, and will pin via tag.