From f03583a59b5fdf93d1bf7dd8f96ade9011ce1ac8 Mon Sep 17 00:00:00 2001 From: Wolfgang Welz Date: Thu, 12 Sep 2024 22:42:27 +0200 Subject: [PATCH 1/6] document unwraps --- steel/src/beacon.rs | 1 + steel/src/block.rs | 3 ++- steel/src/contract.rs | 15 ++++++++++----- steel/src/ethereum.rs | 1 + steel/src/host/db/proof.rs | 13 ++++++++++--- steel/src/host/mod.rs | 2 +- steel/src/lib.rs | 1 + 7 files changed, 26 insertions(+), 10 deletions(-) diff --git a/steel/src/beacon.rs b/steel/src/beacon.rs index ca7f0d6e..59b73e1f 100644 --- a/steel/src/beacon.rs +++ b/steel/src/beacon.rs @@ -297,6 +297,7 @@ mod host { } } // return the last error, if all calls failed + // safe unwrap: there must have been at least one error when we reach this line let err = anyhow::Error::from(request_error.unwrap()); Err(err.context("no valid response received for the 32 consecutive slots")) } diff --git a/steel/src/block.rs b/steel/src/block.rs index 819ea61d..6d61fe85 100644 --- a/steel/src/block.rs +++ b/steel/src/block.rs @@ -28,7 +28,7 @@ pub struct BlockInput { } impl BlockInput { - /// Converts the input into a [EvmEnv] for a verifiable state access in the guest. + /// Converts the input into a [EvmEnv] for verifiable state access in the guest. pub fn into_env(self) -> GuestEvmEnv { // verify that the state root matches the state trie let state_root = self.state_trie.hash_slow(); @@ -95,6 +95,7 @@ pub mod host { H: EvmBlockHeader + TryFrom<::HeaderResponse>, ::HeaderResponse>>::Error: Display, { + // safe unwrap: env is never returned without a DB let mut db = env.db.unwrap(); assert_eq!( db.inner().block_hash(), diff --git a/steel/src/contract.rs b/steel/src/contract.rs index d366d0d7..db7211df 100644 --- a/steel/src/contract.rs +++ b/steel/src/contract.rs @@ -201,6 +201,7 @@ mod host { /// list. This does *not* set the access list as part of the transaction (as specified in /// EIP-2930), and thus can only be specified during preflight on the host. pub async fn prefetch_access_list(self, access_list: AccessList) -> Result { + // safe unwrap: env is never returned without a DB let db = self.env.db.as_mut().unwrap(); db.add_access_list(access_list).await?; @@ -219,11 +220,11 @@ mod host { self.tx.to ); - let cfg = self.env.cfg_env.clone(); - let header = self.env.header.inner().clone(); - // we cannot clone the database, so it gets moved in and out of the task + // as mutable references are not possible, the DB must be moved in and out of the task let db = self.env.db.take().unwrap(); + let cfg = self.env.cfg_env.clone(); + let header = self.env.header.inner().clone(); let (result, db) = tokio::task::spawn_blocking(move || { let mut evm = new_evm(db, cfg, header); let result = self.tx.transact(&mut evm); @@ -232,8 +233,9 @@ mod host { (result, db) }) .await - .context("EVM execution panicked")?; + .expect("EVM execution panicked"); + // restore the DB before handling errors, so that we never return an env without a DB self.env.db = Some(db); result.map_err(|err| anyhow!("call '{}' failed: {}", C::SIGNATURE, err)) @@ -249,6 +251,7 @@ mod host { /// [EvmEnv]: crate::EvmEnv pub async fn call_with_prefetch(self) -> Result { let access_list = { + // safe unwrap: env is never returned without a DB let db = self.env.db.as_mut().unwrap(); let tx = ::TransactionRequest::default() @@ -286,13 +289,15 @@ where /// /// [EvmEnv]: crate::EvmEnv pub fn call(self) -> C::Return { + // safe unwrap: env is never returned without a DB let state_db = self.env.db.as_ref().unwrap(); let mut evm = new_evm::<_, H>( WrapStateDb::new(state_db), self.env.cfg_env.clone(), self.env.header.inner(), ); - self.tx.transact(&mut evm).unwrap() + // typically, guest functions should panic instead of returning an error + self.tx.transact(&mut evm).expect("call failed") } } diff --git a/steel/src/ethereum.rs b/steel/src/ethereum.rs index 0dd9156d..6b74613e 100644 --- a/steel/src/ethereum.rs +++ b/steel/src/ethereum.rs @@ -94,6 +94,7 @@ impl EvmBlockHeader for EthBlockHeader { // technically, this is only valid after EIP-4399 but revm makes sure it is not used before blk_env.prevrandao = Some(header.mix_hash); if let Some(excess_blob_gas) = header.excess_blob_gas { + // safe conversion: according to the consensus spec, excess_blob_gas is always an uint64 blk_env.set_blob_excess_gas_and_price(excess_blob_gas.try_into().unwrap()) }; } diff --git a/steel/src/host/db/proof.rs b/steel/src/host/db/proof.rs index 241c7f18..b8e9bac9 100644 --- a/steel/src/host/db/proof.rs +++ b/steel/src/host/db/proof.rs @@ -157,14 +157,19 @@ impl> ProofDb> ProofDb, { - /// Converts the environment into a [EvmInput] committing to a Ethereum Beacon block root. + /// Converts the environment into a [EvmInput] committing to an Ethereum Beacon block root. /// /// This function assumes that the /// [mainnet](https://github.com/ethereum/consensus-specs/blob/v1.4.0/configs/mainnet.yaml) diff --git a/steel/src/lib.rs b/steel/src/lib.rs index 5e05f0cd..4dd80a01 100644 --- a/steel/src/lib.rs +++ b/steel/src/lib.rs @@ -90,6 +90,7 @@ impl EvmEnv { self.cfg_env.chain_id = chain_spec.chain_id(); self.cfg_env.handler_cfg.spec_id = chain_spec .active_fork(self.header.number(), self.header.timestamp()) + // TODO: return an error .unwrap(); self } From 5b33c739e14398c22cd8ed7c5b117788e2ffaeeb Mon Sep 17 00:00:00 2001 From: Wolfgang Welz Date: Thu, 12 Sep 2024 23:56:21 +0200 Subject: [PATCH 2/6] address todos --- steel/src/beacon.rs | 2 +- steel/src/block.rs | 1 - steel/src/host/db/alloy.rs | 17 +++++++++++++---- steel/src/lib.rs | 4 +++- 4 files changed, 17 insertions(+), 7 deletions(-) diff --git a/steel/src/beacon.rs b/steel/src/beacon.rs index 59b73e1f..bc336a0a 100644 --- a/steel/src/beacon.rs +++ b/steel/src/beacon.rs @@ -273,7 +273,7 @@ mod host { /// Returns the header, with `parent_root` equal to `parent.root`. /// /// It iteratively tries to fetch headers of successive slots until success. - /// TODO: use `eth/v1/beacon/headers?parent_root`, once all the nodes support it. + // TODO(#242): use `eth/v1/beacon/headers?parent_root` when more clients support it. async fn get_child_beacon_header( client: &BeaconClient, parent: GetBlockHeaderResponse, diff --git a/steel/src/block.rs b/steel/src/block.rs index 6d61fe85..78f95d5f 100644 --- a/steel/src/block.rs +++ b/steel/src/block.rs @@ -55,7 +55,6 @@ impl BlockInput { previous_header = ancestor; } - // TODO(victor): When do we check that the storage tries are ok? let db = StateDb::new( self.state_trie, self.storage_tries, diff --git a/steel/src/host/db/alloy.rs b/steel/src/host/db/alloy.rs index 124d068e..c6002eac 100644 --- a/steel/src/host/db/alloy.rs +++ b/steel/src/host/db/alloy.rs @@ -79,8 +79,17 @@ impl> ProviderDb fo } } +/// Errors returned by the [AlloyDb]. +#[derive(Debug, thiserror::Error)] +pub enum Error { + #[error("RPC error")] + RPC(#[from] TransportError), + #[error("block not found")] + BlockNotFound, +} + impl> Database for AlloyDb { - type Error = TransportError; + type Error = Error; fn basic(&mut self, address: Address) -> Result, Self::Error> { let f = async { @@ -143,11 +152,11 @@ impl> Database for AlloyDb Result { - let block = self + let block_response = self .handle .block_on(self.provider.get_block_by_number(number.into(), false))?; - // TODO: return proper error - let block = block.unwrap(); + let block = block_response.ok_or(Error::BlockNotFound)?; + Ok(block.header().hash()) } } diff --git a/steel/src/lib.rs b/steel/src/lib.rs index 4dd80a01..ea684601 100644 --- a/steel/src/lib.rs +++ b/steel/src/lib.rs @@ -72,6 +72,7 @@ pub struct EvmEnv { impl EvmEnv { /// Creates a new environment. + /// /// It uses the default configuration for the latest specification. pub(crate) fn new(db: D, header: Sealed) -> Self { let cfg_env = CfgEnvWithHandlerCfg::new_with_spec_id(Default::default(), SpecId::LATEST); @@ -86,11 +87,12 @@ impl EvmEnv { } /// Sets the chain ID and specification ID from the given chain spec. + /// + /// This will panic when there is no valid specification ID for the current block. pub fn with_chain_spec(mut self, chain_spec: &config::ChainSpec) -> Self { self.cfg_env.chain_id = chain_spec.chain_id(); self.cfg_env.handler_cfg.spec_id = chain_spec .active_fork(self.header.number(), self.header.timestamp()) - // TODO: return an error .unwrap(); self } From 7c7c0307749f1e9841cf362e1b2e6b9a0cafbf83 Mon Sep 17 00:00:00 2001 From: Wolfgang Welz Date: Fri, 13 Sep 2024 13:36:49 +0200 Subject: [PATCH 3/6] check block ID and Digest individually --- steel/tests/corruption.rs | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/steel/tests/corruption.rs b/steel/tests/corruption.rs index f4decb86..ba397ae8 100644 --- a/steel/tests/corruption.rs +++ b/steel/tests/corruption.rs @@ -319,7 +319,7 @@ async fn corrupt_ancestor() { #[should_panic(expected = "Invalid commitment")] async fn corrupt_header_block_commitment() { let input = anvil_input().await.unwrap(); - let exp_commitment = input.clone().into_env().into_commitment(); + let exp_commit = input.clone().into_env().into_commitment(); // get the JSON representation of the block header for the state let mut input_value = to_value(&input).unwrap(); @@ -331,8 +331,12 @@ async fn corrupt_header_block_commitment() { *header_value = to_value(header).unwrap(); // executing this should lead to an Invalid commitment - let commitment = mock_anvil_guest(from_value(input_value).unwrap()); - assert_eq!(commitment, exp_commitment, "Invalid commitment"); + let commit = mock_anvil_guest(from_value(input_value).unwrap()); + assert_eq!(commit.blockID, exp_commit.blockID, "Commitment changed"); + assert_eq!( + commit.blockDigest, exp_commit.blockDigest, + "Invalid commitment" + ); } #[test(tokio::test)] @@ -343,15 +347,15 @@ async fn corrupt_header_beacon_commitment() { }) .await .unwrap(); - let exp_commitment = input.clone().into_env().into_commitment(); + let exp_commit = input.clone().into_env().into_commitment(); // get the JSON representation of the block header for the state let mut input_value = to_value(&input).unwrap(); let header_value = &mut get_block_input_mut(&mut input_value)["header"]; - // corrupt the header by modifying its timestamp + // corrupt the header by modifying its number let mut header: EthBlockHeader = from_value(header_value.clone()).unwrap(); - header.inner_mut().timestamp = 0xdeadbeaf; + header.inner_mut().number = 0xdeadbeaf; *header_value = to_value(header).unwrap(); // executing this should lead to an Invalid commitment @@ -360,7 +364,12 @@ async fn corrupt_header_beacon_commitment() { Contract::new(USDT_ADDRESS, &env) .call_builder(&USDT_CALL) .call(); - assert_eq!(env.into_commitment(), exp_commitment, "Invalid commitment"); + let commit = env.into_commitment(); + assert_eq!(commit.blockID, exp_commit.blockID, "Changed commitment"); + assert_eq!( + commit.blockDigest, exp_commit.blockDigest, + "Invalid commitment" + ); } #[test(tokio::test)] @@ -371,7 +380,7 @@ async fn corrupt_beacon_proof() { }) .await .unwrap(); - let exp_commitment = input.clone().into_env().into_commitment(); + let exp_commit = input.clone().into_env().into_commitment(); // get the JSON representation of the block header for the state let mut input_value = to_value(&input).unwrap(); @@ -386,5 +395,10 @@ async fn corrupt_beacon_proof() { Contract::new(USDT_ADDRESS, &env) .call_builder(&USDT_CALL) .call(); - assert_eq!(env.into_commitment(), exp_commitment, "Invalid commitment"); + let commit = env.into_commitment(); + assert_eq!(commit.blockID, exp_commit.blockID, "Commitment changed"); + assert_eq!( + commit.blockDigest, exp_commit.blockDigest, + "Invalid commitment" + ); } From 17b4550b97a91037dbef2d1cadc171fd6b5f5594 Mon Sep 17 00:00:00 2001 From: Wolfgang Welz Date: Fri, 13 Sep 2024 22:11:15 +0200 Subject: [PATCH 4/6] make call() return an error --- .../methods/guest/src/bin/balance_of.rs | 2 +- examples/erc20/README.md | 2 +- examples/erc20/methods/guest/src/main.rs | 2 +- steel/CHANGELOG.md | 2 ++ steel/src/contract.rs | 7 +++---- steel/tests/common/mod.rs | 2 +- steel/tests/corruption.rs | 12 ++++++++---- 7 files changed, 17 insertions(+), 12 deletions(-) diff --git a/examples/erc20-counter/methods/guest/src/bin/balance_of.rs b/examples/erc20-counter/methods/guest/src/bin/balance_of.rs index 6f1ce2c7..1c453e4c 100644 --- a/examples/erc20-counter/methods/guest/src/bin/balance_of.rs +++ b/examples/erc20-counter/methods/guest/src/bin/balance_of.rs @@ -55,7 +55,7 @@ fn main() { // Execute the view call; it returns the result in the type generated by the `sol!` macro. let call = IERC20::balanceOfCall { account }; - let returns = Contract::new(contract, &env).call_builder(&call).call(); + let returns = Contract::new(contract, &env).call_builder(&call).call().unwrap(); // Check that the given account holds at least 1 token. assert!(returns._0 >= U256::from(1)); diff --git a/examples/erc20/README.md b/examples/erc20/README.md index 8b638b24..f8c485a2 100644 --- a/examples/erc20/README.md +++ b/examples/erc20/README.md @@ -70,7 +70,7 @@ fn main() { // Execute the view call; it returns the result in the type generated by the `sol!` macro. let contract = Contract::new(CONTRACT, &env); - let returns = contract.call_builder(&CALL).from(CALLER).call(); + let returns = contract.call_builder(&CALL).from(CALLER).call().unwrap(); println!("View call result: {}", returns._0); } ``` diff --git a/examples/erc20/methods/guest/src/main.rs b/examples/erc20/methods/guest/src/main.rs index 22c060f6..bfb30a73 100644 --- a/examples/erc20/methods/guest/src/main.rs +++ b/examples/erc20/methods/guest/src/main.rs @@ -57,6 +57,6 @@ fn main() { // Execute the view call; it returns the result in the type generated by the `sol!` macro. let contract = Contract::new(CONTRACT, &env); - let returns = contract.call_builder(&CALL).from(CALLER).call(); + let returns = contract.call_builder(&CALL).from(CALLER).call().unwrap(); println!("View call result: {}", returns._0); } diff --git a/steel/CHANGELOG.md b/steel/CHANGELOG.md index 828c567c..1f112665 100644 --- a/steel/CHANGELOG.md +++ b/steel/CHANGELOG.md @@ -12,6 +12,8 @@ All notable changes to this project will be documented in this file. ### 🚨 Breaking Changes +- `CallBuilder::call` in the guest now returns an error that needs to be handled, before it just panicked. + ## [0.13.0](https://github.com/risc0/risc0-ethereum/releases/tag/steel-v0.13.0) - 2024-09-10 ### ⚡️ Features diff --git a/steel/src/contract.rs b/steel/src/contract.rs index db7211df..e27d5752 100644 --- a/steel/src/contract.rs +++ b/steel/src/contract.rs @@ -67,7 +67,7 @@ use revm::{ /// // Guest: /// let evm_env = evm_input.into_env(); /// let contract = Contract::new(contract_address, &evm_env); -/// contract.call_builder(&get_balance).call(); +/// contract.call_builder(&get_balance).call().unwrap(); /// /// # Ok(()) /// # } @@ -288,7 +288,7 @@ where /// Executes the call with a [EvmEnv] constructed with [Contract::new]. /// /// [EvmEnv]: crate::EvmEnv - pub fn call(self) -> C::Return { + pub fn call(self) -> Result { // safe unwrap: env is never returned without a DB let state_db = self.env.db.as_ref().unwrap(); let mut evm = new_evm::<_, H>( @@ -296,8 +296,7 @@ where self.env.cfg_env.clone(), self.env.header.inner(), ); - // typically, guest functions should panic instead of returning an error - self.tx.transact(&mut evm).expect("call failed") + self.tx.transact(&mut evm) } } diff --git a/steel/tests/common/mod.rs b/steel/tests/common/mod.rs index 9c340b69..cc87650d 100644 --- a/steel/tests/common/mod.rs +++ b/steel/tests/common/mod.rs @@ -70,7 +70,7 @@ where let result = { let contract = Contract::new(address, &env); - options.apply(contract.call_builder(&call)).call() + options.apply(contract.call_builder(&call)).call().unwrap() }; assert_eq!( result, preflight_result, diff --git a/steel/tests/corruption.rs b/steel/tests/corruption.rs index ba397ae8..f3b31e01 100644 --- a/steel/tests/corruption.rs +++ b/steel/tests/corruption.rs @@ -114,10 +114,12 @@ fn mock_anvil_guest(input: EthEvmInput) -> Commitment { let env = input.into_env().with_chain_spec(&ANVIL_CHAIN_SPEC); Contract::new(ANVIL_CONTRACT_ADDRESS, &env) .call_builder(&sol::Pair::aCall {}) - .call(); + .call() + .unwrap(); Contract::new(ANVIL_CONTRACT_ADDRESS, &env) .call_builder(&sol::Pair::bCall {}) - .call(); + .call() + .unwrap(); env.into_commitment() } @@ -363,7 +365,8 @@ async fn corrupt_header_beacon_commitment() { let env = input.into_env().with_chain_spec(&ANVIL_CHAIN_SPEC); Contract::new(USDT_ADDRESS, &env) .call_builder(&USDT_CALL) - .call(); + .call() + .unwrap(); let commit = env.into_commitment(); assert_eq!(commit.blockID, exp_commit.blockID, "Changed commitment"); assert_eq!( @@ -394,7 +397,8 @@ async fn corrupt_beacon_proof() { let env = input.into_env().with_chain_spec(&ANVIL_CHAIN_SPEC); Contract::new(USDT_ADDRESS, &env) .call_builder(&USDT_CALL) - .call(); + .call() + .unwrap(); let commit = env.into_commitment(); assert_eq!(commit.blockID, exp_commit.blockID, "Commitment changed"); assert_eq!( From c2e3779f6f30d352d192bebccd9196438cdd7dc5 Mon Sep 17 00:00:00 2001 From: Wolfgang Welz Date: Fri, 13 Sep 2024 22:13:56 +0200 Subject: [PATCH 5/6] fix fmt --- examples/erc20-counter/methods/guest/src/bin/balance_of.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/examples/erc20-counter/methods/guest/src/bin/balance_of.rs b/examples/erc20-counter/methods/guest/src/bin/balance_of.rs index 1c453e4c..5bde0da1 100644 --- a/examples/erc20-counter/methods/guest/src/bin/balance_of.rs +++ b/examples/erc20-counter/methods/guest/src/bin/balance_of.rs @@ -55,7 +55,10 @@ fn main() { // Execute the view call; it returns the result in the type generated by the `sol!` macro. let call = IERC20::balanceOfCall { account }; - let returns = Contract::new(contract, &env).call_builder(&call).call().unwrap(); + let returns = Contract::new(contract, &env) + .call_builder(&call) + .call() + .unwrap(); // Check that the given account holds at least 1 token. assert!(returns._0 >= U256::from(1)); From 81fc07d3fdf31c5e3315605247c2ff4985a954ab Mon Sep 17 00:00:00 2001 From: Wolfgang Welz Date: Fri, 13 Sep 2024 22:23:14 +0200 Subject: [PATCH 6/6] add unwrap --- examples/token-stats/methods/guest/src/main.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/examples/token-stats/methods/guest/src/main.rs b/examples/token-stats/methods/guest/src/main.rs index 86403326..2c64a449 100644 --- a/examples/token-stats/methods/guest/src/main.rs +++ b/examples/token-stats/methods/guest/src/main.rs @@ -36,10 +36,12 @@ fn main() { let utilization = contract .call_builder(&CometMainInterface::getUtilizationCall {}) .call() + .unwrap() ._0; let supply_rate = contract .call_builder(&CometMainInterface::getSupplyRateCall { utilization }) .call() + .unwrap() ._0; // The formula for APR in percentage is the following: