From fae5773da15efcbb3b47bf0e3161af457f45aa91 Mon Sep 17 00:00:00 2001 From: Gabriel de Quadros Ligneul Date: Fri, 1 Nov 2024 11:41:38 -0300 Subject: [PATCH 1/7] Add unit test for hash_files function --- main/src/project.rs | 64 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 53 insertions(+), 11 deletions(-) diff --git a/main/src/project.rs b/main/src/project.rs index 0f0fd4e..1aca759 100644 --- a/main/src/project.rs +++ b/main/src/project.rs @@ -405,10 +405,39 @@ fn strip_user_metadata(wasm_file_bytes: &[u8]) -> Result> { #[cfg(test)] mod test { use super::*; - use std::fs::{self, File}; - use std::io::Write; + use std::{ + env, + fs::{self, File}, + io::Write, + path::Path, + }; use tempfile::tempdir; + fn write_valid_toolchain_file(toolchain_file_path: &Path) -> Result<()> { + let toolchain_contents = r#" + [toolchain] + channel = "nightly-2020-07-10" + components = [ "rustfmt", "rustc-dev" ] + targets = [ "wasm32-unknown-unknown", "thumbv2-none-eabi" ] + profile = "minimal" + "#; + fs::write(&toolchain_file_path, toolchain_contents)?; + Ok(()) + } + + fn write_mock_rust_files(base_path: &Path, num_files: usize, num_lines: u64) -> Result<()> { + fs::create_dir(base_path.join("src"))?; + let mut contents = String::new(); + for _ in 0..num_lines { + contents.push_str("// foo"); + } + for i in 0..num_files { + let file_path = base_path.join(format!("src/f{i}.rs")); + fs::write(&file_path, &contents)?; + } + Ok(()) + } + #[test] fn test_extract_toolchain_channel() -> Result<()> { let dir = tempdir()?; @@ -438,15 +467,7 @@ mod test { }; assert!(err_details.to_string().contains("is not a string"),); - let toolchain_contents = r#" - [toolchain] - channel = "nightly-2020-07-10" - components = [ "rustfmt", "rustc-dev" ] - targets = [ "wasm32-unknown-unknown", "thumbv2-none-eabi" ] - profile = "minimal" - "#; - std::fs::write(&toolchain_file_path, toolchain_contents)?; - + write_valid_toolchain_file(&toolchain_file_path)?; let channel = extract_toolchain_channel(&toolchain_file_path)?; assert_eq!(channel, "nightly-2020-07-10"); Ok(()) @@ -496,4 +517,25 @@ mod test { Ok(()) } + + #[test] + pub fn test_hash_files() -> Result<()> { + let dir = tempdir()?; + env::set_current_dir(dir.path())?; + + let toolchain_file_path = dir.path().join(TOOLCHAIN_FILE_NAME); + write_valid_toolchain_file(&toolchain_file_path)?; + write_mock_rust_files(dir.path(), 10, 100)?; + fs::write(dir.path().join("Cargo.toml"), "")?; + fs::write(dir.path().join("Cargo.lock"), "")?; + + let cfg = BuildConfig::new(false); + let hash = hash_files(vec![], cfg)?; + + assert_eq!( + hex::encode(hash), + "06b50fcc53e0804f043eac3257c825226e59123018b73895cb946676148cb262" + ); + Ok(()) + } } From 4dfc1163771bedfc9692adca4664f9c0b9f5a1fe Mon Sep 17 00:00:00 2001 From: Gabriel de Quadros Ligneul Date: Fri, 1 Nov 2024 19:11:56 -0300 Subject: [PATCH 2/7] Add benchmark for hash_files --- main/Cargo.toml | 3 +++ main/src/main.rs | 3 +++ main/src/project.rs | 46 ++++++++++++++++++++++++++++----------------- 3 files changed, 35 insertions(+), 17 deletions(-) diff --git a/main/Cargo.toml b/main/Cargo.toml index 98215aa..b784754 100644 --- a/main/Cargo.toml +++ b/main/Cargo.toml @@ -10,6 +10,9 @@ license.workspace = true version.workspace = true repository.workspace = true +[features] +nightly = [] + [dependencies] alloy-primitives.workspace = true alloy-json-abi.workspace = true diff --git a/main/src/main.rs b/main/src/main.rs index 82e781a..c34e4dd 100644 --- a/main/src/main.rs +++ b/main/src/main.rs @@ -1,6 +1,9 @@ // Copyright 2023-2024, Offchain Labs, Inc. // For licensing, see https://github.com/OffchainLabs/cargo-stylus/blob/main/licenses/COPYRIGHT.md +// Enable unstable test feature for benchmarks when nightly is available +#![cfg_attr(feature = "nightly", feature(test))] + use alloy_primitives::TxHash; use clap::{ArgGroup, Args, CommandFactory, Parser, Subcommand}; use constants::DEFAULT_ENDPOINT; diff --git a/main/src/project.rs b/main/src/project.rs index 1aca759..71d4d09 100644 --- a/main/src/project.rs +++ b/main/src/project.rs @@ -411,7 +411,10 @@ mod test { io::Write, path::Path, }; - use tempfile::tempdir; + use tempfile::{tempdir, TempDir}; + + #[cfg(feature = "nightly")] + extern crate test; fn write_valid_toolchain_file(toolchain_file_path: &Path) -> Result<()> { let toolchain_contents = r#" @@ -425,17 +428,26 @@ mod test { Ok(()) } - fn write_mock_rust_files(base_path: &Path, num_files: usize, num_lines: u64) -> Result<()> { - fs::create_dir(base_path.join("src"))?; + fn write_hash_files(num_files: usize, num_lines: usize) -> Result { + let dir = tempdir()?; + env::set_current_dir(dir.path())?; + + let toolchain_file_path = dir.path().join(TOOLCHAIN_FILE_NAME); + write_valid_toolchain_file(&toolchain_file_path)?; + + fs::create_dir(dir.path().join("src"))?; let mut contents = String::new(); for _ in 0..num_lines { contents.push_str("// foo"); } for i in 0..num_files { - let file_path = base_path.join(format!("src/f{i}.rs")); + let file_path = dir.path().join(format!("src/f{i}.rs")); fs::write(&file_path, &contents)?; } - Ok(()) + fs::write(dir.path().join("Cargo.toml"), "")?; + fs::write(dir.path().join("Cargo.lock"), "")?; + + Ok(dir) } #[test] @@ -520,22 +532,22 @@ mod test { #[test] pub fn test_hash_files() -> Result<()> { - let dir = tempdir()?; - env::set_current_dir(dir.path())?; - - let toolchain_file_path = dir.path().join(TOOLCHAIN_FILE_NAME); - write_valid_toolchain_file(&toolchain_file_path)?; - write_mock_rust_files(dir.path(), 10, 100)?; - fs::write(dir.path().join("Cargo.toml"), "")?; - fs::write(dir.path().join("Cargo.lock"), "")?; - - let cfg = BuildConfig::new(false); - let hash = hash_files(vec![], cfg)?; - + let _dir = write_hash_files(10, 100)?; + let hash = hash_files(vec![], BuildConfig::new(false))?; assert_eq!( hex::encode(hash), "06b50fcc53e0804f043eac3257c825226e59123018b73895cb946676148cb262" ); Ok(()) } + + #[cfg(feature = "nightly")] + #[bench] + pub fn bench_hash_files(b: &mut test::Bencher) -> Result<()> { + let _dir = write_hash_files(1000, 10000)?; + b.iter(|| { + hash_files(vec![], BuildConfig::new(false)).expect("failed to hash files"); + }); + Ok(()) + } } From 3fce6759e64d60a263f9494527f176ad2be50dc7 Mon Sep 17 00:00:00 2001 From: Gabriel de Quadros Ligneul Date: Fri, 1 Nov 2024 17:40:11 -0300 Subject: [PATCH 3/7] Improve hash_files performance Read the files in a separate thread and then send the keccak preimage through a channel to the main thread. This improves the performance in ~13% without changing the hash output. --- main/src/project.rs | 57 ++++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 26 deletions(-) diff --git a/main/src/project.rs b/main/src/project.rs index 71d4d09..a5d6fa0 100644 --- a/main/src/project.rs +++ b/main/src/project.rs @@ -18,6 +18,8 @@ use std::{ io::Read, path::{Path, PathBuf}, process, + sync::mpsc, + thread, }; use std::{ops::Range, process::Command}; use tiny_keccak::{Hasher, Keccak}; @@ -228,6 +230,21 @@ pub fn extract_cargo_toml_version(cargo_toml_path: &PathBuf) -> Result { Ok(version.to_string()) } +pub fn read_file_preimage(filename: &Path) -> Result> { + let mut contents = Vec::with_capacity(1024); + { + let filename = filename.as_os_str(); + contents.extend_from_slice(&(filename.len() as u64).to_be_bytes()); + contents.extend_from_slice(filename.as_encoded_bytes()); + } + let mut file = std::fs::File::open(filename) + .map_err(|e| eyre!("failed to open file {}: {e}", filename.display()))?; + contents.extend_from_slice(&file.metadata().unwrap().len().to_be_bytes()); + file.read_to_end(&mut contents) + .map_err(|e| eyre!("Unable to read file {}: {e}", filename.display()))?; + Ok(contents) +} + pub fn hash_files(source_file_patterns: Vec, cfg: BuildConfig) -> Result<[u8; 32]> { let mut keccak = Keccak::v256(); let mut cmd = Command::new("cargo"); @@ -245,26 +262,6 @@ pub fn hash_files(source_file_patterns: Vec, cfg: BuildConfig) -> Result keccak.update(&[1]); } - let mut buf = vec![0u8; 0x100000]; - - let mut hash_file = |filename: &Path| -> Result<()> { - keccak.update(&(filename.as_os_str().len() as u64).to_be_bytes()); - keccak.update(filename.as_os_str().as_encoded_bytes()); - let mut file = std::fs::File::open(filename) - .map_err(|e| eyre!("failed to open file {}: {e}", filename.display()))?; - keccak.update(&file.metadata().unwrap().len().to_be_bytes()); - loop { - let bytes_read = file - .read(&mut buf) - .map_err(|e| eyre!("Unable to read file {}: {e}", filename.display()))?; - if bytes_read == 0 { - break; - } - keccak.update(&buf[..bytes_read]); - } - Ok(()) - }; - // Fetch the Rust toolchain toml file from the project root. Assert that it exists and add it to the // files in the directory to hash. let toolchain_file_path = PathBuf::from(".").as_path().join(TOOLCHAIN_FILE_NAME); @@ -277,12 +274,20 @@ pub fn hash_files(source_file_patterns: Vec, cfg: BuildConfig) -> Result paths.push(toolchain_file_path); paths.sort(); - for filename in paths.iter() { - greyln!( - "File used for deployment hash: {}", - filename.as_os_str().to_string_lossy() - ); - hash_file(filename)?; + // Read the file contents in another thread and process the keccak in the main thread. + let (tx, rx) = mpsc::channel(); + thread::spawn(move || { + for filename in paths.iter() { + greyln!( + "File used for deployment hash: {}", + filename.as_os_str().to_string_lossy() + ); + tx.send(read_file_preimage(&filename)) + .expect("failed to send preimage (impossible)"); + } + }); + for result in rx { + keccak.update(result?.as_slice()); } let mut hash = [0u8; 32]; From b57487f435891fe259c0de71e6d49eeae28fb848 Mon Sep 17 00:00:00 2001 From: Gabriel de Quadros Ligneul Date: Mon, 4 Nov 2024 10:15:15 -0300 Subject: [PATCH 4/7] Add bench target in makefile --- Makefile | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Makefile b/Makefile index b4fbed4..38f0a23 100644 --- a/Makefile +++ b/Makefile @@ -8,6 +8,10 @@ build: test: cargo test +.PHONY: bench +bench: + rustup run nightly cargo bench -F nightly + .PHONY: fmt fmt: cargo fmt From bcfb70386fabafdc923e8120f0b5617773ed0b7c Mon Sep 17 00:00:00 2001 From: Gabriel de Quadros Ligneul Date: Mon, 4 Nov 2024 10:58:51 -0300 Subject: [PATCH 5/7] Move cargo version out of hash_files This makes the unit tests more resilient to changes in the rust environment. --- main/src/check.rs | 2 +- main/src/project.rs | 28 ++++++++++++++++++++++------ main/src/verify.rs | 2 +- 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/main/src/check.rs b/main/src/check.rs index 5c31ea6..572eb50 100644 --- a/main/src/check.rs +++ b/main/src/check.rs @@ -128,7 +128,7 @@ impl CheckConfig { let cfg = BuildConfig::new(rust_stable); let wasm = project::build_dylib(cfg.clone())?; let project_hash = - project::hash_files(self.common_cfg.source_files_for_project_hash.clone(), cfg)?; + project::hash_project(self.common_cfg.source_files_for_project_hash.clone(), cfg)?; Ok((wasm, project_hash)) } } diff --git a/main/src/project.rs b/main/src/project.rs index a5d6fa0..45cfa65 100644 --- a/main/src/project.rs +++ b/main/src/project.rs @@ -245,8 +245,7 @@ pub fn read_file_preimage(filename: &Path) -> Result> { Ok(contents) } -pub fn hash_files(source_file_patterns: Vec, cfg: BuildConfig) -> Result<[u8; 32]> { - let mut keccak = Keccak::v256(); +pub fn hash_project(source_file_patterns: Vec, cfg: BuildConfig) -> Result<[u8; 32]> { let mut cmd = Command::new("cargo"); cmd.arg("--version"); let output = cmd @@ -255,7 +254,21 @@ pub fn hash_files(source_file_patterns: Vec, cfg: BuildConfig) -> Result if !output.status.success() { bail!("cargo version command failed"); } - keccak.update(&output.stdout); + + hash_files(&output.stdout, source_file_patterns, cfg) +} + +pub fn hash_files( + cargo_version_output: &[u8], + source_file_patterns: Vec, + cfg: BuildConfig, +) -> Result<[u8; 32]> { + let mut keccak = Keccak::v256(); + println!( + "> {}", + String::from_utf8(cargo_version_output.into()).unwrap() + ); + keccak.update(cargo_version_output); if cfg.opt_level == OptLevel::Z { keccak.update(&[0]); } else { @@ -282,7 +295,7 @@ pub fn hash_files(source_file_patterns: Vec, cfg: BuildConfig) -> Result "File used for deployment hash: {}", filename.as_os_str().to_string_lossy() ); - tx.send(read_file_preimage(&filename)) + tx.send(read_file_preimage(filename)) .expect("failed to send preimage (impossible)"); } }); @@ -538,7 +551,8 @@ mod test { #[test] pub fn test_hash_files() -> Result<()> { let _dir = write_hash_files(10, 100)?; - let hash = hash_files(vec![], BuildConfig::new(false))?; + let rust_version = "cargo 1.80.0 (376290515 2024-07-16)\n".as_bytes(); + let hash = hash_files(rust_version, vec![], BuildConfig::new(false))?; assert_eq!( hex::encode(hash), "06b50fcc53e0804f043eac3257c825226e59123018b73895cb946676148cb262" @@ -550,8 +564,10 @@ mod test { #[bench] pub fn bench_hash_files(b: &mut test::Bencher) -> Result<()> { let _dir = write_hash_files(1000, 10000)?; + let rust_version = "cargo 1.80.0 (376290515 2024-07-16)\n".as_bytes(); b.iter(|| { - hash_files(vec![], BuildConfig::new(false)).expect("failed to hash files"); + hash_files(rust_version, vec![], BuildConfig::new(false)) + .expect("failed to hash files"); }); Ok(()) } diff --git a/main/src/verify.rs b/main/src/verify.rs index c65223b..23fb9af 100644 --- a/main/src/verify.rs +++ b/main/src/verify.rs @@ -65,7 +65,7 @@ pub async fn verify(cfg: VerifyConfig) -> eyre::Result<()> { let wasm_file: PathBuf = project::build_dylib(build_cfg.clone()) .map_err(|e| eyre!("could not build project to WASM: {e}"))?; let project_hash = - project::hash_files(cfg.common_cfg.source_files_for_project_hash, build_cfg)?; + project::hash_project(cfg.common_cfg.source_files_for_project_hash, build_cfg)?; let (_, init_code) = project::compress_wasm(&wasm_file, project_hash)?; let deployment_data = deploy::contract_deployment_calldata(&init_code); if deployment_data == *result.input { From 122469436bf9331784a17a251d1b85bcc8dbac3e Mon Sep 17 00:00:00 2001 From: Gabriel de Quadros Ligneul Date: Mon, 4 Nov 2024 11:09:25 -0300 Subject: [PATCH 6/7] Run nightly CI with nightly toolchain --- .ci/build_and_test.sh | 2 +- .github/workflows/linux.yml | 2 +- .github/workflows/mac.yml | 2 +- Makefile | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.ci/build_and_test.sh b/.ci/build_and_test.sh index 8277240..656b7d3 100755 --- a/.ci/build_and_test.sh +++ b/.ci/build_and_test.sh @@ -15,4 +15,4 @@ if [ "$CFG_RELEASE_CHANNEL" == "nightly" ]; then else cargo build --locked fi -cargo test \ No newline at end of file +cargo test diff --git a/.github/workflows/linux.yml b/.github/workflows/linux.yml index 78b333d..0591fd3 100644 --- a/.github/workflows/linux.yml +++ b/.github/workflows/linux.yml @@ -26,7 +26,7 @@ jobs: - name: install rustup run: | curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs > rustup-init.sh - sh rustup-init.sh -y --default-toolchain none + sh rustup-init.sh -y --default-toolchain ${{ matrix.cfg_release_channel }} rustup target add ${{ matrix.target }} - name: Build and Test diff --git a/.github/workflows/mac.yml b/.github/workflows/mac.yml index 49918f7..5a3fb27 100644 --- a/.github/workflows/mac.yml +++ b/.github/workflows/mac.yml @@ -27,7 +27,7 @@ jobs: - name: install rustup run: | curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs > rustup-init.sh - sh rustup-init.sh -y --default-toolchain none + sh rustup-init.sh -y --default-toolchain ${{ matrix.cfg_release_channel }} rustup target add ${{ matrix.target }} - name: Build and Test diff --git a/Makefile b/Makefile index 38f0a23..6b94b6d 100644 --- a/Makefile +++ b/Makefile @@ -10,7 +10,7 @@ test: .PHONY: bench bench: - rustup run nightly cargo bench -F nightly + cargo +nightly bench -F nightly .PHONY: fmt fmt: From e574ac50b84722d3d4aee8748381cd9437d94c56 Mon Sep 17 00:00:00 2001 From: Gabriel de Quadros Ligneul Date: Mon, 4 Nov 2024 11:22:37 -0300 Subject: [PATCH 7/7] Remove debug println --- main/src/project.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/main/src/project.rs b/main/src/project.rs index 45cfa65..4df27d1 100644 --- a/main/src/project.rs +++ b/main/src/project.rs @@ -264,10 +264,6 @@ pub fn hash_files( cfg: BuildConfig, ) -> Result<[u8; 32]> { let mut keccak = Keccak::v256(); - println!( - "> {}", - String::from_utf8(cargo_version_output.into()).unwrap() - ); keccak.update(cargo_version_output); if cfg.opt_level == OptLevel::Z { keccak.update(&[0]);