From b8a14213d92fff07dacda993804e7edb7946ca58 Mon Sep 17 00:00:00 2001 From: Wolfgang Welz Date: Thu, 3 Oct 2024 13:44:42 +0200 Subject: [PATCH] backport: Fix incorrect encoding used by prove cheatcode (#254) (#276) With the change from `ethers` to `alloy`, we made an error in assuming they'd handle ABI encoding of `Vec<_>` the same. This broke the `prove` cheatcode that is used in the Foundry template tests. We did not catch this error prior to releasing, because we had moved all our tests in this repo over to using `RiscZeroMockVerifier.mockProve` instead and forgot to add test coverage specifically for this cheatcode. This PR fixes the encoding issue and additionally adds a simple test for using `risc0-forge-ffi` in dev mode. Additionally, this PR solves a problem where the FFI cheatcode would fail if `RUST_LOG` is set. --------- Co-authored-by: Victor Graf --- .github/workflows/main.yml | 2 + Cargo.lock | 9 +++ Cargo.toml | 2 +- contracts/src/test/RiscZeroCheats.sol | 10 +++- ffi/Cargo.toml | 4 ++ ffi/guests/Cargo.toml | 10 ++++ ffi/guests/build.rs | 17 ++++++ ffi/guests/echo/Cargo.toml | 13 ++++ ffi/guests/echo/src/main.rs | 26 ++++++++ ffi/guests/src/lib.rs | 15 +++++ ffi/src/main.rs | 59 ++++++++++++++----- ffi/tests/basic.rs | 85 +++++++++++++++++++++++++++ 12 files changed, 234 insertions(+), 18 deletions(-) create mode 100644 ffi/guests/Cargo.toml create mode 100644 ffi/guests/build.rs create mode 100644 ffi/guests/echo/Cargo.toml create mode 100644 ffi/guests/echo/src/main.rs create mode 100644 ffi/guests/src/lib.rs create mode 100644 ffi/tests/basic.rs diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index a7ea5cca..d25220d4 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -58,6 +58,8 @@ jobs: - run: cargo fmt --all --check - run: cargo sort --workspace --check - run: cargo clippy --workspace + env: + RISC0_SKIP_BUILD: true - run: forge fmt --check working-directory: contracts - uses: actions/setup-python@v4 diff --git a/Cargo.lock b/Cargo.lock index 2014f92f..1c1d0ccb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1956,6 +1956,13 @@ dependencies = [ "subtle", ] +[[package]] +name = "ffi-guests" +version = "0.1.0" +dependencies = [ + "risc0-build", +] + [[package]] name = "fixed-hash" version = "0.8.0" @@ -3978,7 +3985,9 @@ dependencies = [ "alloy", "anyhow", "clap", + "ffi-guests", "hex", + "libc", "risc0-ethereum-contracts", "risc0-zkvm", ] diff --git a/Cargo.toml b/Cargo.toml index 3a1edc7f..c4c18784 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [workspace] resolver = "2" -members = ["build", "contracts", "ffi", "steel"] +members = ["build", "contracts", "ffi", "ffi/guests", "steel"] [workspace.package] version = "1.1.2" diff --git a/contracts/src/test/RiscZeroCheats.sol b/contracts/src/test/RiscZeroCheats.sol index 1b9ddced..b08fdbc4 100644 --- a/contracts/src/test/RiscZeroCheats.sol +++ b/contracts/src/test/RiscZeroCheats.sol @@ -37,7 +37,7 @@ abstract contract RiscZeroCheats is CommonBase { return vm.envOr("RISC0_DEV_MODE", false); } - /// @notice Returns the journal, and Groth16 seal, resulting from running the + /// @notice Returns the journal, and seal, resulting from running the /// guest with elf_path using input on the RISC Zero zkVM. /// @dev Based on whether `devMode()` is `true`, will take one of two actions: /// * When `devMode()` is `true` @@ -47,11 +47,15 @@ abstract contract RiscZeroCheats is CommonBase { /// Uses the local prover or the Bonsai proving service to run the guest and produce an on-chain verifiable /// SNARK attesting to the correctness of the journal output. URL and API key for Bonsai /// should be specified using the BONSAI_API_URL and BONSAI_API_KEY environment variables. - function prove(string memory elf_path, bytes memory input) internal returns (bytes memory, bytes memory) { - string[] memory imageRunnerInput = new string[](10); + function prove(string memory elf_path, bytes memory input) + internal + returns (bytes memory journal, bytes memory seal) + { + string[] memory imageRunnerInput = new string[](11); uint256 i = 0; imageRunnerInput[i++] = "cargo"; imageRunnerInput[i++] = "run"; + imageRunnerInput[i++] = "--locked"; imageRunnerInput[i++] = "--manifest-path"; imageRunnerInput[i++] = "lib/risc0-ethereum/ffi/Cargo.toml"; imageRunnerInput[i++] = "--bin"; diff --git a/ffi/Cargo.toml b/ffi/Cargo.toml index 3813399b..51c58a9e 100644 --- a/ffi/Cargo.toml +++ b/ffi/Cargo.toml @@ -12,5 +12,9 @@ alloy = { workspace = true } anyhow = { workspace = true } clap = { version = "4.5", features = ["derive", "env"] } hex = { version = "0.4" } +libc = "0.2.159" risc0-ethereum-contracts = { workspace = true } risc0-zkvm = { workspace = true, features = ["client"] } + +[dev-dependencies] +ffi-guests = { path = "./guests" } diff --git a/ffi/guests/Cargo.toml b/ffi/guests/Cargo.toml new file mode 100644 index 00000000..b6e37a18 --- /dev/null +++ b/ffi/guests/Cargo.toml @@ -0,0 +1,10 @@ +[package] +name = "ffi-guests" +version = "0.1.0" +edition = "2021" + +[build-dependencies] +risc0-build = { workspace = true } + +[package.metadata.risc0] +methods = ["echo"] diff --git a/ffi/guests/build.rs b/ffi/guests/build.rs new file mode 100644 index 00000000..a4aa2563 --- /dev/null +++ b/ffi/guests/build.rs @@ -0,0 +1,17 @@ +// Copyright 2024 RISC Zero, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +fn main() { + risc0_build::embed_methods(); +} diff --git a/ffi/guests/echo/Cargo.toml b/ffi/guests/echo/Cargo.toml new file mode 100644 index 00000000..bc99e666 --- /dev/null +++ b/ffi/guests/echo/Cargo.toml @@ -0,0 +1,13 @@ +[package] +name = "echo" +version = "0.1.0" +edition = "2021" + +[workspace] + +[dependencies] +risc0-zkvm = { git = "https://github.com/risc0/risc0", branch = "main", default-features = false, features = ["std"] } + +[profile.release] +debug = 1 +lto = "thin" diff --git a/ffi/guests/echo/src/main.rs b/ffi/guests/echo/src/main.rs new file mode 100644 index 00000000..3d017228 --- /dev/null +++ b/ffi/guests/echo/src/main.rs @@ -0,0 +1,26 @@ +// Copyright 2024 RISC Zero, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::io::Read; + +use risc0_zkvm::guest::env; + +pub fn main() { + // Read the entire input stream as raw bytes. + let mut message = Vec::::new(); + env::stdin().read_to_end(&mut message).unwrap(); + + // Commit exactly what the host provided to the journal. + env::commit_slice(message.as_slice()); +} diff --git a/ffi/guests/src/lib.rs b/ffi/guests/src/lib.rs new file mode 100644 index 00000000..ae9d61e6 --- /dev/null +++ b/ffi/guests/src/lib.rs @@ -0,0 +1,15 @@ +// Copyright 2024 RISC Zero, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +include!(concat!(env!("OUT_DIR"), "/methods.rs")); diff --git a/ffi/src/main.rs b/ffi/src/main.rs index 86b5802f..04903a25 100644 --- a/ffi/src/main.rs +++ b/ffi/src/main.rs @@ -12,10 +12,15 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::io::Write; +use std::{ + fs::{File, OpenOptions}, + io, + io::Write, + os::unix::io::{AsRawFd, FromRawFd}, +}; use alloy::{primitives::Bytes, sol_types::SolValue}; -use anyhow::{Context, Result}; +use anyhow::{ensure, Context, Result}; use clap::Parser; use risc0_ethereum_contracts::encode_seal; use risc0_zkvm::{default_prover, ExecutorEnv, ProverOpts, VerifierContext}; @@ -35,7 +40,10 @@ enum Command { /// Run the CLI. pub fn main() -> Result<()> { - match Command::parse() { + // Take stdout is ensure no extra data is written to it. + let mut stdout = take_stdout()?; + + let output = match Command::parse() { Command::Prove { guest_binary_path, input, @@ -45,22 +53,19 @@ pub fn main() -> Result<()> { )?, }; + // Forge test FFI calls expect hex encoded bytes sent to stdout + write!(&mut stdout, "{}", hex::encode(output)).context("failed to write to stdout")?; + stdout.flush().context("failed to flush stdout")?; + Ok(()) } /// Prints on stdio the Ethereum ABI and hex encoded proof. -fn prove_ffi(elf_path: String, input: Vec) -> Result<()> { - let elf = std::fs::read(elf_path).unwrap(); +fn prove_ffi(elf_path: String, input: Vec) -> Result> { + let elf = std::fs::read(elf_path).expect("failed to read guest ELF"); let (journal, seal) = prove(&elf, &input)?; - let calldata = vec![Bytes(journal.into()), Bytes(seal.into())]; - let output = hex::encode(calldata.abi_encode()); - - // Forge test FFI calls expect hex encoded bytes sent to stdout - print!("{output}"); - std::io::stdout() - .flush() - .context("failed to flush stdout buffer")?; - Ok(()) + let calldata = (Bytes(journal.into()), Bytes(seal.into())); + Ok(calldata.abi_encode()) } /// Generates journal and snark seal as a pair (`Vec`, `Vec) @@ -78,3 +83,29 @@ fn prove(elf: &[u8], input: &[u8]) -> Result<(Vec, Vec)> { let seal = encode_seal(&receipt)?; Ok((journal, seal)) } + +/// "Takes" stdout, returning a handle and ensuring no other code in this process can write to it. +/// This is used to ensure that no additional data (e.g. log lines) is written to stdout, as any +/// extra will cause a decoding failure in the Forge FFI cheatcode. +fn take_stdout() -> Result { + let stdout = io::stdout(); + let mut handle = stdout.lock(); + // Ensure all buffered data is written before redirection + handle.flush()?; + + let devnull = OpenOptions::new().write(true).open("/dev/null")?; + + unsafe { + // Create a copy of stdout to use for our output. + let dup_fd = libc::dup(handle.as_raw_fd()); + ensure!(dup_fd >= 0, "call to libc::dup failed: {}", dup_fd); + // Redirect stdout to the fd we opened for /dev/null + let dup2_result = libc::dup2(devnull.as_raw_fd(), libc::STDOUT_FILENO); + ensure!( + dup2_result >= 0, + "call to libc::dup2 failed: {}", + dup2_result + ); + Ok(File::from_raw_fd(dup_fd)) + } +} diff --git a/ffi/tests/basic.rs b/ffi/tests/basic.rs new file mode 100644 index 00000000..9c39a6b3 --- /dev/null +++ b/ffi/tests/basic.rs @@ -0,0 +1,85 @@ +// Copyright 2024 RISC Zero, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::process::Command; + +use alloy::{primitives::Bytes, sol_types::SolValue}; +use ffi_guests::{ECHO_ID, ECHO_PATH}; +use risc0_ethereum_contracts::encode_seal; +use risc0_zkvm::{FakeReceipt, InnerReceipt, Receipt, ReceiptClaim}; + +#[test] +fn basic_usage() { + let exe_path = env!("CARGO_BIN_EXE_risc0-forge-ffi"); + let args = ["prove", ECHO_PATH, "0xdeadbeef"]; + println!("{} {:?}", exe_path, args); + let output = Command::new(exe_path) + .env_clear() + // PATH is required so r0vm can be found. + .env("PATH", std::env::var("PATH").unwrap()) + .env("RISC0_DEV_MODE", "1") + .args(args) + .output() + .unwrap(); + + println!("{:#?}", &output); + + let output_bytes = hex::decode(output.stdout).unwrap(); + let (journal, seal) = <(Bytes, Bytes)>::abi_decode(&output_bytes, true).unwrap(); + + assert_eq!(journal, hex::decode("deadbeef").unwrap()); + let expected_receipt = Receipt::new( + InnerReceipt::Fake(FakeReceipt::new(ReceiptClaim::ok( + ECHO_ID, + journal.to_vec(), + ))), + journal.into(), + ); + let expect_seal = encode_seal(&expected_receipt).unwrap(); + assert_eq!(expect_seal, seal.to_vec()); +} + +// It's important that `risc0-forge-ffi` only send to stdout the output to be consumed by forge +// with the FFI cheatcode. If any extra output is sent, ABI decoding will fail. +#[test] +fn basic_usage_with_rust_log() { + let exe_path = env!("CARGO_BIN_EXE_risc0-forge-ffi"); + let args = ["prove", ECHO_PATH, "0xdeadbeef"]; + println!("{} {:?}", exe_path, args); + let output = Command::new(exe_path) + .env_clear() + // PATH is required so r0vm can be found. + .env("PATH", std::env::var("PATH").unwrap()) + .env("RISC0_DEV_MODE", "1") + .env("RUST_LOG", "debug") + .args(args) + .output() + .unwrap(); + + println!("{:#?}", &output); + + let output_bytes = hex::decode(output.stdout).unwrap(); + let (journal, seal) = <(Bytes, Bytes)>::abi_decode(&output_bytes, true).unwrap(); + + assert_eq!(journal, hex::decode("deadbeef").unwrap()); + let expected_receipt = Receipt::new( + InnerReceipt::Fake(FakeReceipt::new(ReceiptClaim::ok( + ECHO_ID, + journal.to_vec(), + ))), + journal.into(), + ); + let expect_seal = encode_seal(&expected_receipt).unwrap(); + assert_eq!(expect_seal, seal.to_vec()); +}