feat: add process_epoch benchmark and upgrade RISC Zero to v3.0.3 #25
Conversation
|
Great work btw! |
Co-authored-by: Unnawut Leepaisalsuwanna <[email protected]>
|
@syjn99 only if you'd like to review and have time! |
unnawut
left a comment
unnawut
left a comment
There was a problem hiding this comment.
Anyhow this looks good to me. Just fix the build failure and see if Jun would like to review.
If no updates feel free to merge by the end of today once the fmt is fixed.
syjn99
left a comment
syjn99
left a comment
There was a problem hiding this comment.
Two things I'd like to note. BTW good job!
1. Rust version issue
➜ host git:(pr/x-senpai-x/25) ✗ make run-block-attestation
##################################################
Running block benchmarks for attestation...
##################################################
error: rustc 1.85.0 is not supported by the following packages:
[email protected] requires rustc 1.86
[email protected] requires rustc 1.86
[email protected] requires rustc 1.86
[email protected] requires rustc 1.88.0
[email protected] requires rustc 1.88.0
[email protected] requires rustc 1.88.0
[email protected] requires rustc 1.88.0
[email protected] requires rustc 1.88.0
Either upgrade rustc or select compatible dependency versions with
`cargo update <name>@<current-ver> --precise <compatible-ver>`
where `<compatible-ver>` is the latest version supporting rustc 1.85.0
Execution complete for block attestation.
As the Rust version of Ream was bumped, this should also be applied to this project. make run-* command doesn't work on my machine with the current rust-toolchain.
[toolchain]
channel = "1.89.0"
components = ["llvm-tools", "rustc-dev"]
This worked for me.
2. Formatting issue
As CI says, all files are not well formatted. Could you check your default formatter also?
methods/guest/src/main.rs
Outdated
|
|
||
| let mut state: BeaconState = deserialize(&pre_state_ssz_bytes); | ||
|
|
||
There was a problem hiding this comment.
Yeah this file needs to be formatted.
methods/guest/Cargo.toml
Outdated
| ethereum_ssz = "0.9" | ||
| ream-consensus = { git = "https://github.com/ReamLabs/ream.git", package = "ream-consensus", features = ["zkvm"] } | ||
| ethereum_ssz = { workspace = true } | ||
| ream-consensus = { workspace=true } |
There was a problem hiding this comment.
| ream-consensus = { workspace=true } | |
| ream-consensus = { workspace = true } |
Sorry for nitpicking 🙏
host/Cargo.toml
Outdated
| hex = "0.4.3" | ||
| methods = { path = "../methods" } | ||
| risc0-zkvm = { version = "2.0.1" } | ||
| risc0-zkvm = { version = "3.0.1" } |
There was a problem hiding this comment.
How about updating this version with the latest one? (3.0.3)
| ethereum_ssz = "0.9" | ||
| ream-consensus = { git = "https://github.com/ReamLabs/ream.git", package = "ream-consensus", features = ["zkvm"] } | ||
| ream-consensus = { git = "https://github.com/ReamLabs/ream.git", package = "ream-consensus-beacon", features = ["zkvm"] } | ||
| sha2 = { git = "https://github.com/risc0/RustCrypto-hashes", tag = "sha2-v0.10.8-risczero.0" } |
There was a problem hiding this comment.
Do we use sha2 here? Deletting this dependency doesn't make any trouble in my local environment.
There was a problem hiding this comment.
It was introduced by @SuccinctPaul for benchmarks using this precompile. It significantly reduces the execution cycles and time. From cargo.lock, it seems sha2 is being used extensively by several packages.
If we delete this dependency it would switch to the original sha2 crate and the timings almost double.
There was a problem hiding this comment.
I guess these changes should be also applied to the README.md in the project root.
host/src/bin/cli/operation.rs
Outdated
| } | ||
|
|
||
| // Generic traits for operation handling | ||
| pub trait OperationHandler { |
There was a problem hiding this comment.
| pub trait OperationHandler { | |
| pub trait OperationHandler: std::fmt::Display { |
If you want this to implement Display trait, you can inherit the trait like this. This can also make run_tests more clean.
There was a problem hiding this comment.
aah got it, makes much more sense this way.
host/src/bin/cli/operation.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn to_block_operation_type(&self) -> BlockOperationType { |
There was a problem hiding this comment.
Maybe to_* functions can be implemented with From trait (https://doc.rust-lang.org/std/convert/trait.From.html)
There was a problem hiding this comment.
Thanks! I'll use the From trait instead
…test and formatted the codebase
|
@syjn99 thanks for the review, I have addressed all the changes could you please review the changes again? |
x-senpai-x
changed the title
|
Thanks @x-senpai-x! |
4 participants
Introduces benchmarking for
process_epochin the Beacon State transition function.Upgrades risczero dependency from previous version to v3.0.1.