-
Notifications
You must be signed in to change notification settings - Fork 69
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.
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
Build blocks using Reth 1.1.0 #115
base: main
Are you sure you want to change the base?
Conversation
@hashcashier @SchmErik iiuc v1.81 toolchain should now be ready to use! |
crates/core/src/db.rs
Outdated
@@ -95,6 +95,7 @@ impl<DB: DatabaseRef + Recoverable> DatabaseRef for Wrapper<DB> { | |||
|
|||
impl<DB: DatabaseCommit + Recoverable> DatabaseCommit for Wrapper<DB> { | |||
fn commit(&mut self, changes: HashMap<Address, Account>) { | |||
dbg!("COMMIT!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that should be removed.
crates/preflight/src/client.rs
Outdated
// resolve storage orphans | ||
if let Some((trie, _)) = storage_tries.get_mut(&account_proof.address) { | ||
for storage_proof in account_proof.storage_proof { | ||
// let proof_nodes = parse_proof(&storage_proof.proof)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is still quite a bit of commented code, this should be removed
@@ -151,6 +153,48 @@ impl<N: Network, R: CoreDriver, P: PreflightDriver<R, N>> PreflightDB<N, R, P> { | |||
apply_changeset(&mut self.inner, state_changeset) | |||
} | |||
|
|||
pub fn sanity_check(&mut self, state_changeset: StateChangeset) -> anyhow::Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this is vestigial but it's also public and comes in handy when debugging
crates/preflight/src/trie.rs
Outdated
mpt_from_proof(&proof_nodes).context("invalid storage_proof")?; | ||
let proof_nodes = parse_proof(&storage_proof.proof) | ||
.context("extend_proof_tries/parse storage proof")?; | ||
mpt_from_proof(&proof_nodes).context(format!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be a with_context
crates/preflight/src/provider/mod.rs
Outdated
Ok(vec.into_iter().collect()) | ||
} | ||
} | ||
|
||
pub mod opt_ordered_map { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is never used, ordered_map + default is used instead
|
||
fn chain_spec(chain: &NamedChain) -> Option<Arc<Self::ChainSpec>> { | ||
match chain { | ||
NamedChain::Mainnet => Some(MAINNET.clone()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not support all possible forks. I know for a fact that the pre Byzantine validation is not correct in Reth: Back then, the TxReceipt still contained intermediate state hashes which was left out in Reth for performance reasons https://github.com/paradigmxyz/reth/blob/e3702cfc87449294da061f02a571a23061b8ab50/crates/ethereum/consensus/src/validation.rs#L31
So we must exclude at least everything before Byzantine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about possible future forks that are (or will be with newer reth versions) only partially supported? Do they also need to be explicitly excluded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this seems underconstrained as well like with the PoW checks
testing/ef-tests/testdata
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the submodule was removed to prevent Cargo from always checking out the huge tests.
However, then the ef-tests should be adapted to include a script or documentation that checks out the EF tests in the correct version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually didn't migrate the ef tests because reth takes care of much of the logic we had previously put together ourselves. But I agree this would be great to bring back.
@@ -1,6 +1,10 @@ | |||
[workspace] | |||
resolver = "2" | |||
members = ["guests", "host", "lib", "primitives", "testing/ef-tests"] | |||
members = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ef-tests have been removed here. Thus, they have not been adapted to the new reth version and will not compile.
- uses: risc0/cargo-install@v1 | ||
with: | ||
crate: cargo-binstall | ||
- run: cargo binstall -y --force cargo-risczero@$RISC0_VERSION | ||
- run: cargo risczero install --version $RISC0_TOOLCHAIN_VERSION | ||
- run: cargo test --workspace --all-targets -F ef-tests,debug-guest-build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should absolutely not disable the ef-tests. Even when we are now using reth they contain crucial testcases that we must also pass.
} | ||
|
||
#[derive(Default)] | ||
pub struct Wrapper<T: Recoverable> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative to "abusing" the drop to get the Database back is to move the DB in and out similar to https://github.com/risc0/risc0-ethereum/blob/73eebd4efb283c7f99748d45fc730a53ee2edf5c/steel/src/contract.rs#L211
However, this should probably not also be in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code to do this would have be on the reth side so to keep the changes there minimal I went with rescuing the dropped value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're constrained by reth, then I'd say this is a pretty neat work around. It's "non-local" and thus maybe less intuitive than the move-in-move-out approach -- but I do appreciate that drop()
only gets called once, which simplifies the process of reasoning about the Mutex
.
Nice trick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved pending concurrence from Wolf
~~ Some additional comments
This is a really exciting refactor. It makes good on our long-held goal of deleting code and leveraging the growing ecosystem of libraries, and incorporates some architectural lessons learned. This was obviously a ton of work and I deeply appreciate you taking it so seriously.
It's also a lot of code so I can't be 100% sure I didn't miss some important things. So, while I've read the PR and left some general comments, I still have a few higher level questions:
- (Checklist item) Are there any portions of code that need additional attributions?
- (Checklist item) Have you recently walked the security-critical code paths to convince yourself you didn't accidentally leave any parts commented out/disabled? Does the logic still look good to you?
- What's your least favorite part of the code?
- Which parts do you think will require the most ongoing maintenance? Chainspec seems like an obvious answer. Any others? Are there any parts that are "temporary" that I may have missed?
- It would be awesome if 1 or 2 of the core modules had some top-level cargo doc that explained the role of the key traits/types. Stateless? Drivers? Strategies? I think I was able to piece it together from context but it'd be very nice to have them laid out somewhere, even if only tersely.
Thank you again brother
// let block_with_senders = BlockWithSenders { | ||
// block, | ||
// senders: vec![], // todo: recover signers with non-det hints | ||
// }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop
@@ -1,4 +1,4 @@ | |||
// Copyright 2023 RISC Zero, Inc. | |||
// Copyright 2024 RISC Zero, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the usual convention is to make a list of years, including the year of the most recent edit. Example:
"Copyright 2023, 2024 RISC Zero, Inc."
} | ||
|
||
#[derive(Default)] | ||
pub struct Wrapper<T: Recoverable> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're constrained by reth, then I'd say this is a pretty neat work around. It's "non-local" and thus maybe less intuitive than the move-in-move-out approach -- but I do appreciate that drop()
only gets called once, which simplifies the process of reasoning about the Mutex
.
Nice trick.
|
||
impl Recoverable for MemoryDB { | ||
fn rescue(&mut self) -> Option<Self> { | ||
Some(core::mem::take(self)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL
|
||
impl<DB: DatabaseCommit + Recoverable> DatabaseCommit for Wrapper<DB> { | ||
fn commit(&mut self, changes: HashMap<Address, Account>) { | ||
// dbg!("COMMIT!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop
Ok(()) | ||
} | ||
|
||
/// Copied from [BundleState::into_plane_state]. Modified to retain account code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[BundleState::into_plane_state
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add additional attribution here? Is this from reth?
use std::io::Read; | ||
use std::sync::{Arc, Mutex}; | ||
|
||
pub type RescueDestination<D> = Arc<Mutex<Option<D>>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for? Does it get used?
This PR migrates
zeth
from directly usingrevm
to usingreth
for building blocks. This allows zeth to support all forks supported byreth
. The changes are an overhaul of many of zeth's internal components and project structure. Notably, because of Cargo's feature unification, and reth's dependence on feature flags for enabling/disabling OP functionality, two separate zeth binaries are now necessary.The project has been restructured into the following crates:
zeth-core
Library for defining and using the sparse MPT to statelessly build a block using reth.zeth-core-ethereum
Specialization of the core library for Ethereum blocks.zeth-core-optimism
Specialization of the core library for Optimism blocks.zeth-preflight
Library for populating the data required to statelessly build a block usingzeth-core
.zeth-preflight-ethereum
Specialization of the preflight library for Ethereum RPCs.zeth-preflight-optimism
Specialization of the preflight library for Optimism RPCs.zeth-guests-reth-ethereum
A binary for running the stateless block building process for Ethereum blocks.zeth-guests-reth-optimism
A binary for running the stateless block building process for Optimism blocks.zeth-guests
Library for exporting the compiled rv32im bytecode ofzeth-guests-reth
.zeth
A library for running a CLI to use the RISC Zero zkVM to generate/verify proofs for the stateless block building process.zeth-ethereum
A binary using thezeth
crate for Ethereum blocks.zeth-optimism
A binary using thezeth
crate for Optimism blocks.zeth-benchmark
A binary for benchmarking proving performance for randomly sampled blocks.Functionality changes:
Note: v1.81 of the toolchain is required:
Note: Separate binaries must be built individually
Issues affected:
reth 1.0
matiching or compatible depenancies #110