feat: Cache invalidation on zksolc version change #871

MBerguer · 2025-01-24T22:28:31Z

What 💻

Updating the can_use_cached in zksync, so it accounts for the selected version as well
Adding two tests;
- test_zk_cache_ok is a very basic test that asserts the cache is working correctly (must have)
- test_zk_cache_invalid_on_version_changed to demonstrate that changing the zksolc version now recompiles the project.

Why ✋

This was a bug, we need to recompile after changing this setting because this affects the artifacts directly.

Evidence 📷

Before this change, we had this situation:

user@local % forge build --zksync
[⠃] Using zksolc-1.5.6
[⠊] Compiling (zksync)
[⠑] Compiling 27 files with zksolc and solc 0.8.28
[⠃] zksolc and solc 0.8.28 finished in 2.72s
Compiler run successful!
user@local % nano foundry.toml        
> zksolc = "1.5.8"
user@local % forge build --zksync
[⠃] Using zksolc-1.5.8
[⠒] Compiling (zksync)
No files changed, compilation skipped

Now, it is recompiling after the change.

Documentation 📚

Please ensure the following before submitting your PR:

Check if these changes affect any documented features or workflows.
Update the book if these changes affect any documented features or workflows.

elfedy · 2025-01-27T10:49:00Z

crates/common/src/compile.rs

@@ -192,7 +193,7 @@ impl ProjectCompiler {
        let quiet = self.quiet.unwrap_or(false);
        let bail = self.bail.unwrap_or(true);

-        let output = with_compilation_reporter(self.quiet.unwrap_or(false), || {
+        let output = with_compilation_reporter(quiet, || {


This is upstream code. We should not change upstream code if there's no reason to as it potentially creates merge conflicts. If using the quiet variable here makes sense submit a PR upstream with that and then when we do the upstream sync we incorporate the change.

elfedy · 2025-01-27T11:14:29Z

crates/config/src/zksync.rs

@@ -144,6 +144,7 @@ impl ZkSyncConfig {
            codegen: if self.force_evmla { Codegen::EVMLA } else { Codegen::Yul },
            suppressed_warnings: self.suppressed_warnings.clone(),
            suppressed_errors: self.suppressed_errors.clone(),
+            zksolc_version: self.zksolc.as_ref().and_then(|req| req.try_version().ok()),


This won't work if user specifies a path instead of a hardcoded version (like zksolc = /path/to/zksolc_binary) or if the user does not specify a version and it gets resolved by foundry for him (different foundry versions may yield different zksolc versions as we update the latest supported). You probably need to always specify a version. The logic to resolve what compiler to use is here when building the ZkSolcCompiler so you need to somehow figure out which version it is before building the settings and always pass it, maybe using ZkSolc::get_version_for_path.

elfedy · 2025-01-27T11:20:02Z

crates/forge/tests/cli/zk_build.rs

@@ -11,3 +12,67 @@ forgetest_init!(test_zk_build_sizes, |prj, cmd| {

    assert!(pattern.is_match(&stdout), "Unexpected size output:\n{stdout}");
 });
+
+// tests cache works as expected in zksync mode
+forgetest_init!(test_zk_cache_ok, |prj, cmd| {


Maybe these test fit better on the foundry-zksync-compilers level. There are some tests we have where we check caching is working as expected. See for example:

foundry-zksync/crates/zksync/compilers/tests/zksync_tests.rs

Line 279 in cf0a88d

let compiled = project.compile().unwrap();

elfedy · 2025-01-27T11:30:31Z

crates/zksync/compilers/src/compilers/zksolc/settings.rs

@@ -87,6 +87,9 @@ pub struct ZkSettings {
    /// Suppressed `zksolc` errors.
    #[serde(default, skip_serializing_if = "HashSet::is_empty")]
    pub suppressed_errors: HashSet<ErrorType>,
+    /// The version of the zksolc compiler to use.
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub zksolc_version: Option<Version>,


ZkSettings represents fields that will be sent to the compiler as standard json input. Adding this field here means zksolc_version is sent to the compiler in the input json which is not ideal. Maybe we can add a field to ZkSolcSettings instead which is meant to encapsulate the whole compiler settings and not just the input json?

mostly moving things around

elfedy · 2025-01-27T16:35:05Z

crates/zksync/compilers/tests/zksync_tests.rs

+
+forgetest_init!(test_zk_cache_ok, |prj, cmd| {
+    let zk_toml = r#"[profile.default]
+src = 'src'


Tests in here should not call forge. The test should:

Build a Project directly using a given zksolc version (see other tests in this file on how to do it)

Compile project twice, second time should be cached.

Change version on ZkSolcCompiler , then try to compile and fail (because version in settings and version in ZkSolcCompiler does not match.

Change version in settings, compile and check it was a fresh compilation.

elfedy · 2025-01-27T16:37:43Z

crates/zksync/compilers/src/compilers/zksolc/settings.rs

@@ -99,6 +99,9 @@ pub struct ZkSolcSettings {
    /// Additional CLI args configuration
    #[serde(flatten)]
    pub cli_settings: solc::CliSettings,
+    /// The version of the zksolc compiler to use.
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub zksolc_version: Option<Version>,


Don't think this should be an option ever, there should always be a version here. Also, when compiling, there should also be a check that the version of the compiler matches the version in the settings so that it is not possible to set a compiler with a different version than the settings being used.

elfedy · 2025-01-27T16:38:10Z

crates/config/src/zksync.rs

@@ -146,8 +146,18 @@ impl ZkSyncConfig {
            suppressed_errors: self.suppressed_errors.clone(),
        };

+        let zksolc_version = self.zksolc.as_ref().map(|req| match req {


This still does not account for the default compiler version is being used

MBerguer added 6 commits January 24, 2025 12:23

ok cache invaldation

2473ce9

Refactoring a little bit

ba1e537

Adding a test that proves this works now

d6401ec

Adding the right test

5fc3aab

fmt

82e9ca8

name

9bed21f

MBerguer requested a review from a team as a code owner January 24, 2025 22:28

MBerguer added 3 commits January 24, 2025 19:35

clippy

0384dcb

changing the logic a little big, to avoid stuttering

ee16176

comments

79eb414

elfedy reviewed Jan 27, 2025

View reviewed changes

Jrigada added this to the Release 0.0.5 milestone Jan 27, 2025

MBerguer added 3 commits January 27, 2025 12:44

PR feedback

af26a8c

mostly moving things around

Getting the zksolc_version for path as well

c177bfc

Merge branch 'main' into tincho/cache-invalidation-on-zksolc-version

ed2c1e0

elfedy reviewed Jan 27, 2025

View reviewed changes

MBerguer added 2 commits January 27, 2025 16:29

calling the logic that ensures a default

3db5b6a

new tests

2b7b321

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Cache invalidation on zksolc version change #871

feat: Cache invalidation on zksolc version change #871

MBerguer commented Jan 24, 2025

elfedy Jan 27, 2025

elfedy Jan 27, 2025

elfedy Jan 27, 2025

elfedy Jan 27, 2025 •

edited

Loading

elfedy Jan 27, 2025

elfedy Jan 27, 2025

elfedy Jan 27, 2025

feat: Cache invalidation on zksolc version change #871

Are you sure you want to change the base?

feat: Cache invalidation on zksolc version change #871

Conversation

MBerguer commented Jan 24, 2025

What 💻

Why ✋

Evidence 📷

Documentation 📚

elfedy Jan 27, 2025

Choose a reason for hiding this comment

elfedy Jan 27, 2025

Choose a reason for hiding this comment

elfedy Jan 27, 2025

Choose a reason for hiding this comment

elfedy Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

elfedy Jan 27, 2025

Choose a reason for hiding this comment

elfedy Jan 27, 2025

Choose a reason for hiding this comment

elfedy Jan 27, 2025

Choose a reason for hiding this comment

elfedy Jan 27, 2025 •

edited

Loading