Conversation
1759bcc to
1ecab29
Compare
1d23e79 to
fa307d2
Compare
EVM Opcode Benchmark DiffAggregated runs: base=1, pr=1 No significant regressions or improvements detected. |
src/Nethermind/Nethermind.Core/ExecutionRequest/ExecutionRequestExtensions.cs
Show resolved
Hide resolved
| <packageSource key="nugettest.org"> | ||
| <package pattern="Nethermind.ZiskBindings" /> | ||
| </packageSource> |
There was a problem hiding this comment.
Should we have stable package to merge to master?
There was a problem hiding this comment.
Not yet. Things are changing, and this package will likely be gone soon.
|
|
||
| namespace Nethermind.StatelessInputGen; | ||
|
|
||
| public class OwnedReadOnlyListConverter : JsonConverterFactory |
There was a problem hiding this comment.
Can we potentially have a separate Witness classes to avoid pooled list in this project?
Or maybe we should just make IOwnedReadOnlyList<> equivalent to normal List under ZK_EVM?
This pooling isn't really netting us anything here right?
There was a problem hiding this comment.
Right, that was my initial approach because of the lack of a serializer. Then I decided to have the same class everywhere, but I don't have strong preferences here.
| /// <summary> | ||
| /// Provides methods for zkVM input serialization. <see cref="Witness.Keys"/> are ignored. | ||
| /// </summary> | ||
| public static class InputSerializer |
There was a problem hiding this comment.
Was there any point of using this over standarized IRlpDecoders?
There was a problem hiding this comment.
I'm not sure what you mean. Please elaborate.
There was a problem hiding this comment.
we have a standard how to design RLP serialization/deserialization and interfaces for it
There was a problem hiding this comment.
Please point to it then
|
There are still places with |
|
Claude finished @LukaszRozmej's task in 6m 40s —— View job Code Review — Shared Code Correctness (Precompiles & Hash)
SummaryThe shared code changes are correct. No consensus-breaking bugs found. Two findings below.
|
@LukaszRozmej Don't be like Claude :) Please see my answer to it. I have no intention to replace every |
benaadams
left a comment
There was a problem hiding this comment.
Presumably you could also swap out (most of) UInt256 by just changing the opcode registration? 🤔
@benaadams Could you please elaborate? We might switch to native UInt256 in the near future. |
Generate/PrepareOpcodes decides implementation src/Nethermind/Nethermind.Evm/VirtualMachine.zkevm.cs For example Arbitrum overrides some in its GenerateOpCodes https://github.com/NethermindEth/nethermind-arbitrum/blob/a98faba4d5a064a3e7fc0c20b974e6e0e864eb82/src/Nethermind.Arbitrum/Evm/ArbitrumVirtualMachine.cs#L422-L434 |
Changes
Nethermind.Stateless.ZiskGuestas a guest for Zisk, and theNethermind.Stateless.Executorwith the core execution and serialization logic. The idea is to have a separate guest app for each supported zkVM that contains all host-specific functionality. The stateless executor itself is a regular library that can be compiled and run on any platform.stdfor standard implementations andzkevmfor zkEVM implementations. The.std.csfiles andstd/*.csfiles are excluded from build whenEnableZkEvm = true, while the.zkevm.csfiles andzkevm/*.csfiles are excluded from build whenEnableZkEvm != true. This vastly eliminates the need for#if ZK_EVMdirectives in the codebase.StatelessExecutionproject.See the README
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Requires manual testing