feat(zk): zksolc linking #800

Karrq · 2024-12-20T21:37:01Z

What 💻

Add functions to use zksolc --link
Process deploy-time linking (CREATE only)
Update default zksolc to 1.5.8

Why ✋

Allow tests & scripts to rely on contracts with libraries that have not been deployed yet

Notes 📝

There's a simple merge conflicts due to the lack of access to DualCompiledContracts during the MultiContractRunner instantiation.

This is a working yet incomplete implementation, as such there are a few quirks, in particular: unlinked factory deps are not supported (zksolc 1.5.9 should have some extra info that we can use), linking via CREATE2 is not supported either yet.

Currently there's an issue with trying to actually run the library, I believe due to a missing storage migration:

2024-12-20T21:22:57.842992Z ERROR suite{name=CounterTest}:test{kind=test name=testIncrement}:call:inspect: foundry_zksync_core::vm::tracers::cheatcode: call may fail or behave unexpectedly due to empty code target=0x0e93455c2327dc24ac46d3a2edade6dbb6260629 calldata="771602f70000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

chore: identify action to deploy libs in EraVM

crates/evm/evm/src/executors/strategy.rs

crates/forge/src/multi_runner.rs

crates/linking/src/lib.rs

crates/strategy/zksync/src/executor.rs

crates/zksync/compilers/src/link.rs

crates/zksync/core/src/lib.rs

crates/config/src/zksync.rs

elfedy · 2025-01-13T14:52:44Z

crates/forge/src/multi_runner.rs

+                        .find(|(id, _)| id.source == needle.source && id.name == needle.name)
+                        .map(|(_, evm)| (needle, zk, evm))
+                })
+                .filter(|(_, zk, evm)| zk.bytecode.is_some() && evm.bytecode.is_some())


Shouldn't this be more of an assertion than a filter? Is it ok to keep on executing if we are missing some of the bytecodes?

It would simply not be added to the dual_compiled_contracts like it was before if it was unlinked (read as invalid bytecode)

Ok, I am just worried we would filter something we should be aware it's missing (for example what happened when using zksolc previous to 1.5.7 and trying to just deserialize the eravm field to get the bytecode) and then this fails later when trying to get the bytecode and takes a bit to debug and track it is being filetred here. If this cannot happen then we are good.

There are also some contracts which aren't meant to have bytecode like interfaces etc... Not sure the implications here tbh

crates/forge/src/multi_runner.rs

crates/forge/tests/it/zk/linking.rs

crates/linking/src/lib.rs

crates/strategy/zksync/src/executor.rs

crates/zksync/compiler/src/lib.rs

crates/zksync/compilers/src/lib.rs

crates/zksync/compilers/src/dual_compiled_contracts.rs

fix(strategy): remove get_mut for `DualCompiledContracts`

fix(zk:link): use version from config refactor(zk:config): extract config -> zksolc compiler logic into function chore: lints

feat(script:zk): link with zksolc

test(zk:link): use 1.5.9 for deploy-time linking feat(zk:link): detect factory dependencies as link references for library lookup fix(zk:link): require 1.5.9 for deploy-time-linking

fix(link:zk): consistent artifact id prefixes chore: formatting refactor(link:zk): improve link targets filtering

fix(link:zk): don't preemptively strip file prefixes fix(link:zk): better factory dep detection chore: formatting

feat(strategy): return "broadcastable" tx feat(link:zk): populate newly linked factory deps feat(script): use `deploy_library`

fix(lib:zk): remove EVM lib deployments from broadcastable txs

refactor(artifact:zk): `all_factory_deps` method chore: formatting

feat(link:script): register target & deps

Jrigada

Great job as always

Jrigada · 2025-01-21T15:45:45Z

crates/evm/evm/src/executors/strategy.rs

@@ -68,9 +109,32 @@ pub trait ExecutorStrategyRunner: Debug + Send + Sync + ExecutorStrategyExt {
        amount: U256,
    ) -> BackendResult<()>;

+    fn get_balance(&self, executor: &mut Executor, address: Address) -> BackendResult<U256>;


Where do we use this?

during zksync library deployment, I made our strategy to snapshot the nonce&balance before the lib deployment, to then restore them for EraVM, so the "co-deployment" happens with the same input parameters

Jrigada · 2025-01-21T15:58:59Z

crates/evm/evm/src/executors/strategy.rs

+
+                // manually increase nonce when performing CALLs
+                executor
+                    .set_nonce(from, nonce + 1)


Let's keep in mind that when the nonce PR from @nbaztec is merged we have to change this don't we?

this was migrated from upstream's code, but the meaning is that EVM won't increase the nonce for just calls (only create or full tx, but here it's not a full tx), so foundry increments the nonce manually to account for that, since here it's a call to the deployer

crates/linking/src/zksync.rs

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

Karrq

Left comments to perhaps clarify some logic or choices

Karrq · 2025-01-21T16:28:56Z

crates/evm/evm/src/executors/strategy.rs

@@ -48,6 +61,34 @@ pub struct ExecutorStrategy {
    pub context: Box<dyn ExecutorStrategyContext>,
 }

+pub struct LinkOutput {


I personally would like to move this stuff to a separate module, perhaps even a separate trait from the ExecutorStrategy?

Karrq · 2025-01-21T16:29:45Z

crates/evm/evm/src/executors/strategy.rs

    fn set_nonce(&self, executor: &mut Executor, address: Address, nonce: u64)
        -> BackendResult<()>;

+    fn get_nonce(&self, executor: &mut Executor, address: Address) -> BackendResult<u64>;
+
+    fn link(


This is used in MultiContractRunnerBuilder::build only, as scripts do not make use of a strategy

crates/evm/evm/src/executors/strategy.rs

Karrq · 2025-01-21T16:33:19Z

crates/evm/evm/src/executors/strategy.rs

+
+        let linked_contracts = linker.get_linked_artifacts(&libraries)?;
+
+        // Create a mapping of name => (abi, deployment code, Vec<library deployment code>)


migrated from existing code, but to me the comment looks outdated (no library deployment code)

Karrq · 2025-01-21T16:36:00Z

crates/forge/bin/cmd/create.rs

other than clarifying a comment, optimizations

Karrq · 2025-01-21T17:03:31Z

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

@@ -40,7 +40,7 @@ pub const ZKSOLC: &str = "zksolc";
 /// ZKsync solc release used for all ZKsync solc versions
 pub const ZKSYNC_SOLC_RELEASE: Version = Version::new(1, 0, 1);
 /// Default zksolc version
-pub const ZKSOLC_VERSION: Version = Version::new(1, 5, 7);
+pub const ZKSOLC_VERSION: Version = Version::new(1, 5, 8);


same, waiting for base to be >=1.5.9 as default

Karrq · 2025-01-21T17:04:40Z

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

@@ -54,13 +54,15 @@ pub enum ZkSolcError {
    /// `sendtransfer` error: Using `send()` or `transfer()` methods on `address payable` instead
    /// of `call()`.
    SendTransfer,
+    AssemblyCreate,


Added just to be able to use 1.5.9 in tests

Won't be needed once #840 is merged with base

Karrq · 2025-01-21T17:07:29Z

crates/zksync/compilers/src/dual_compiled_contracts.rs

@@ -69,7 +73,7 @@ impl<'a> FindBytecodeResult<'a> {
 /// A collection of `[DualCompiledContract]`s
 #[derive(Debug, Default, Clone)]
 pub struct DualCompiledContracts {
-    contracts: Vec<DualCompiledContract>,
+    contracts: HashMap<ContractInfo, DualCompiledContract>,


This was done to make searching easier, mostly for linking where we don't have full bytecodes but mostly paths & names
I also don't think it hurts to convert this to a map, helps to avoid duplicates (which were possible before)

Karrq · 2025-01-21T18:48:17Z

crates/zksync/core/src/lib.rs

+}
+
+/// Compute a CREATE2 address according to zksync
+pub fn compute_create2_address(


didn't find an equivalent for this unlinke create

Karrq · 2025-01-21T18:49:44Z

crates/zksync/core/src/vm/runner.rs

@@ -51,7 +51,10 @@ where
    let caller = env.tx.caller;
    let nonce = ZKVMData::new(&mut ecx).get_tx_nonce(caller);
    let (transact_to, is_create) = match env.tx.transact_to {
-        TransactTo::Call(to) => (to.to_h160(), false),
+        TransactTo::Call(to) => {


had to change this one since transact may get deployments of the libs now, and we need to have the outcome parsed appropriately
technically speaking this is also helpful for broadcastable txs, as those could also contain deployments in this form

fix(test:zk): default to zksolc 1.5.9

fix(script): avoid unpacking create2 result fix(script): avoid marking create2 result address as persistent

feat(zk): zksolc linking

d6489ae

Karrq self-assigned this Dec 20, 2024

Karrq requested a review from a team as a code owner December 20, 2024 21:37

Karrq added 7 commits January 7, 2025 17:53

fix(zk:libs): calculate addresses w/ proper nonce

e5b805d

chore: identify action to deploy libs in EraVM

Merge remote-tracking branch 'origin/main' into feat/zksolc-link

44aebad

Merge remote-tracking branch 'origin/main' into feat/zksolc-link

f5dcfa1

fix: don't always assume DCC is present

262d2a7

feat(executor): deploy_library in strategy

b79cada

fix(zk): create address computation

1941e33

chore: cleanup unused imports

93babb9

nbaztec reviewed Jan 10, 2025

View reviewed changes

Karrq added 2 commits January 10, 2025 19:02

test(zk): deploy-time linking (script/test)

5ed5154

chore: default zksolc to 1.5.8

018f3f0