Skip to content
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

feat: manage ZkVM dependencies with trait and impl in each Guest #7

Open
wants to merge 27 commits into
base: unstable
Choose a base branch
from

Conversation

CeciliaZ030
Copy link

@CeciliaZ030 CeciliaZ030 commented Apr 3, 2024

Introduce a static operation list so that block builder can insert desired Zk Operation when initializing revm, and a static dyn trait object that impl the ZkvmOperator trait. For instance, if sp1 only has BN256, and Secp256k1, then the guest insert such two operations; then in the precompile hook, revm will check which operation is in the list and hook the correct ZkVm implementation.

pub static ZKVM_OPERATIONS: Lazy<OnceBox<Vec<Operation>>> = Lazy::new(OnceBox::<Vec<Operation>>::new);
pub static ZKVM_OPERATOR: OnceLock<Box<dyn ZkvmOperator>> = OnceLock::new();

#[derive(Debug, Clone, PartialEq)]
pub enum Operation {
    Bn128Add,
    // ...
}

The ZkvmOperator trait is implemented differently in standalone guest so that each guest can use their customized precompile, the revm DOES NOT need any additional dependencies.

pub trait ZkvmOperator: Send + Sync {
    fn bn128_run_add(&self, input: &[u8]) -> Result<[u8; 64], Error>;
    // ...
}

@CeciliaZ030 CeciliaZ030 changed the title feat: Use trait to manage ZkVM dependencies via impl in each Guest feat: manage ZkVM dependencies with trait and impl in each Guest Apr 3, 2024

#[cfg(feature = "zk-op")]
/// Zkvm errors
ZkvmOperatrion(String),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐱

Comment on lines 113 to 123
#[cfg(feature = "zk_op")]
let output = if crate::zk_op::contains_operation(&crate::zk_op::Operation::Modexp) {
crate::zk_op::ZKVM_OPERATOR
.get()
.expect("ZKVM_OPERATOR unset")
.modexp_run(base, exponent, modulus)?
.into()
} else {
modexp(base, exponent, modulus)
};
#[cfg(not(feature = "zk_op"))]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would drop the feature, seems cleaner not to have to support the base path twice like currently (one in the fallback, one in the path without the feature). Extra maintenance to do.

What do you think of actually implementing the base path also in a trait implementation like RustZkvmOperator. That way we always go through the trait (fewer code paths), the code in the implementation here would be very clean for example here just crate::zk_op::get().modexp(base, exponent, modulus). The get would either select the trait object that was set or if not would default to RustZkvmOperator implementation.

We could even just let the per functions selection to happen in the specific zkVM implementation directly, they could pass through the function calls to RustZkvmOperator themselves if there is not a more optimized implementation.

Copy link
Author

@CeciliaZ030 CeciliaZ030 Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point dropping the flag! some of ur POV is wild but I'll call it thinking out of the box 🐱 in a way that makes my life easier.
After dropping the flag i can easily make it pass through if there's no designated impl for certain operation in certain ZKVM. I'll just let the trait impl in the guest return an Error and upon the Error the revm will fall back to default.

Comment on lines +31 to +43
fn bn128_run_add(&self, input: &[u8]) -> Result<[u8; 64], Error>;
fn bn128_run_mul(&self, input: &[u8]) -> Result<[u8; 64], Error>;
fn bn128_run_pairing(&self, input: &[u8]) -> Result<bool, Error>;
fn blake2_run(&self, input: &[u8]) -> Result<[u8; 64], Error>;
fn sha256_run(&self, input: &[u8]) -> Result<[u8; 32], Error>;
fn ripemd160_run(&self, input: &[u8]) -> Result<[u8; 32], Error>;
fn modexp_run(&self, base: &[u8], exp: &[u8], modulus: &[u8]) -> Result<Vec<u8>, Error>;
fn secp256k1_ecrecover(
&self,
sig: &[u8; 64],
recid: u8,
msg: &[u8; 32],
) -> Result<[u8; 32], Error>;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best case scenario we can make these static. I would think they don't need a &self but perhaps in theory it could be useful if these could be stateful for some reason, though seems quite unlikely. I haven't worked much with rust for this type of case so not sure if there's any issues with making these fully static objects.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ur guess is right there is an issue! Without &self it says something like "cannot make XX into an object" cuz it's unsized. I think it has something to do with the vtable associated with the dynamic object that if these functions are not member functions of the type, the compiler can't determine wtf.
ChatGPT:

The vtable can only store information about methods that can be called on an instance (i.e., methods that have a self, &self, or &mut self parameter). This is because the vtable needs to know which implementation to call based on the actual type of the instance behind the trait object.

@CeciliaZ030 CeciliaZ030 marked this pull request as ready for review April 6, 2024 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants