feat: Linking rewrite #7027

klkvr · 2024-02-06T21:35:56Z

Motivation

The way current linking implementation works is by linking each individual artifact in an isolated way. For each artifact post_link hook is getting called with the following data (PostLinkInput).

id of the linked artifact
contract containing linked contract artifact
known_contracts which seems to be just a mutable reference getting passed around
dependencies - vector of libraries that needs to be deployed for the given artifact

The dependencies vector contents is explained here:

foundry/crates/forge/src/link.rs

Lines 580 to 583 in 2cf84d9

    
           /// A library may show up more than once, but it should *always* have the same nonce and address 
        
           /// with respect to the single `assert_dependencies` call. There should be no gaps in the nonce 
        
           /// otherwise, i.e. whenever a new dependency is encountered, the nonce should be a single 
        
           /// increment larger than the previous largest nonce.

The thing which highlights the downside of this approach is that for different artifact dependencies may include different libraries with different nonces. This is expected behavior but it results in verifiaction issues (#6506).

Solution

Considering that current approach does not really make much sense, I've decided to implement new linling algorithm to be less complex and linking just one target artifact. If we need to link more artifacts, it can just be invoked several times as it's done in tests right now.

I believe that one of the most sensitive places of the linker is conversion between ArtifactId and solc library format (src/Lib.sol:Lib) as it's related to paths. Even though we strip paths everywhere I think there might be some edge cases here and would like to do some testing in that direction.

Closes #6506
Closes #5014

mattsse

new linling algorithm to be less complex and linking just one target artifact. If we need to link more artifacts, it can just be invoked several times as it's done in tests right now.

this approach is so much better
looks very promising already

mattsse · 2024-02-07T12:55:30Z

crates/forge/bin/cmd/script/build.rs

-        let (new_libraries, predeploy_libraries): (Vec<_>, Vec<_>) =
-            run_dependencies.into_iter().unzip();
+    pub fn link(
+        &self,


do we need &self here or can this also be a standalone function?

We don't really need &self here, however I believe that highlevel_known_contracts and Libraries logic is specific to scripts so we can make it a standalone function but still in scope of the scripts

wonder if we can give this a different name e.g link_script for clarity?

@Evalir you mean separate it from ScriptArgs and rename?

@klkvr personally don't mind either separating it or keeping it on ScriptArgs—just think the fn name can be more specific 😄

If we can make this a standalone fn outside ScriptArgs I think it would be nice, as it shows this is explictly a script-related helper.

mds1 · 2024-02-07T14:26:02Z

crates/forge/bin/cmd/script/build.rs

+        let mut new_libraries = Libraries { libs: BTreeMap::new() };
+        for (id, address) in libs_to_deploy.iter().chain(predeployed_libs.iter()) {
+            new_libraries
+                .libs
+                .entry(id.source.clone())
+                .or_default()
+                .insert(id.name.split('.').next().unwrap().to_owned(), address.to_string());


When we need to deploy libraries, it might be nice to do it with create2 (if we don't already). Using create2 when a hardcoded create2 deployer is available on the chain seems preferable because then libs can be automatically reused across projects without needing to redeploy (see also: #4810)

this is a nice idea! I was also thinking about it in scope of #6215 and #5375. I think it should be much easier to implement with less complex linker implementation, but imo it makes more sense as a separate PR as it'd be a new feature

Evalir

Looking great, some comments

crates/forge/src/link.rs

crates/forge/src/multi_runner.rs

crates/forge/src/link.rs

Evalir · 2024-02-07T14:29:53Z

crates/forge/bin/cmd/script/build.rs

-        let (new_libraries, predeploy_libraries): (Vec<_>, Vec<_>) =
-            run_dependencies.into_iter().unzip();
+    pub fn link(
+        &self,


wonder if we can give this a different name e.g link_script for clarity?

Adds helper method to strip path prefixes from `Libraries` object, I'll need this for foundry-rs/foundry#7027

klkvr · 2024-02-08T13:47:05Z

Linking is a bit more complex than it has to be because of the way we handle libraries.

Before passing sources into solc, we are stripping paths to start from project root (otherwise, keep global path) and once compiler finishes, we are joining all paths to start from the project root again.

The issue here is that linkReferences in contracts artifacts are never updated and still contain relative paths. Because of that, when matching libraries, we are operating on relative paths. Thus, any linking calling function should firstly ensure to strip paths for both user-provided libraries and contract artifacts

foundry/crates/forge/src/link.rs

Lines 98 to 109 in 243eeee

    
           /// Links given artifact with either given library addresses or address computed from sender and 
        
           /// nonce. 
        
           /// 
        
           /// Current linking implementation assumes that all link_references keys are matching the format in 
        
           /// which sources appear in [ArtifactId] keys. 
        
           /// 
        
           /// You should make sure to strip paths via `with_stripped_file_prefixes` for both 
        
           /// `libraries` and `contracts` *before* invoking linker. 
        
           /// 
        
           /// This function will always link target contract completely, however, in case of paths format 
        
           /// mismatch it may not recognize some of the predeployed libraries, leading to more deployments 
        
           /// than needed.

klkvr · 2024-02-08T14:31:48Z

UPD: I've updated linker logic to accept root path and strip all paths while matching link_references to artifacts, that way we now only assume that libraries provided into linker have correct paths without remappings

mattsse

some nits,

test coverage looks good.

ptal @Evalir @DaniPopes

crates/forge/src/link.rs

crates/forge/src/multi_runner.rs

Evalir

So the code is looking great to me bar mattsse's nits. I think what we want is probably manual testing with big projects (Maple, spark protocol) that historically have had problems with our linker. I can gather a few of these closed issues so we can test.

I also think that, for next steps, we should consider extracting the core linking code into a new crate, and maybe exposing it through a struct Linker, instead of having multiple loose fns, some public, some private?

Cargo.toml

crates/forge/bin/cmd/script/build.rs

klkvr · 2024-02-08T18:57:53Z

I also think that, for next steps, we should consider extracting the core linking code into a new crate, and maybe exposing it through a struct Linker, instead of having multiple loose fns, some public, some private?

Yep, it makes sense, basically Linker needs just Project and ProjectCompileOutput to be initialized and can be made much clearner than current functions

klkvr · 2024-02-09T01:51:29Z

@Evalir @mattsse so I've refactored the code to have linking logic in Linker struct, it also allowed to simplify linking logic in script a little bit.

I've tested it on Aave-like codebase with several external libraries and fould couple bugs with how we collect known_contracts for scripts and tests, all of those should now be fixed, but I'll test for some more time.

Regarding link_script_target fn: what is does is links script target via link_with_address_or_nonce and then collects highlevel_known_contracts - set of contracts with fully-linked non-empty bytecode which is used for verification and traces decoding. I think that it's pretty specific and it makes sense to leave it in script for now as it has become much shorter after refactoring and should be readable

mattsse

last suggestions

lgtm

pending @DaniPopes

any additional issues this might close?

mattsse · 2024-02-09T12:42:11Z

crates/forge/src/multi_runner.rs

+                .deployed_bytecode
+                .and_then(|d_bcode| d_bcode.bytecode)
+                .and_then(|bcode| bcode.object.into_bytes())


I think this can be simplified with .get_deployed_bytecode_bytes().cloned()

we could also clean this up on foundry-compilers so that this function no longer returns Cow, not useful anyway

mattsse · 2024-02-09T12:43:08Z

crates/forge/src/multi_runner.rs

+            let Some(bytecode) = linked_contract
+                .bytecode
+                .and_then(|b| b.object.into_bytes())


I think this is also linked_contract.get_bytecode_bytes()

klkvr · 2024-02-09T14:54:43Z

@mattsse this should close #6506, not sure if it's a duplicate of something

klkvr · 2024-02-09T15:18:32Z

Also closes #5014, added test for it

onbjerg

overall looks better, although I wonder if we should get rid of eyre in the linker. imo eyre is more for CLI stuff, this should be more strongly typed. wdyt?

crates/forge/src/link.rs

onbjerg · 2024-02-09T17:20:17Z

crates/forge/src/link.rs

+        // user-provided paths to be able to match them correctly.
+        let mut libraries = libraries.with_stripped_file_prefixes(self.root.as_path());
+
+        let mut needed_libraries = BTreeSet::new();


At first I was wondering if this was safe, because you can have a library that itself is unlinked, so it's not deployable until all those unlinked references are resolved.

However at second thought, this is safe, because we know the addresses in such a scenario ahead of time, as long as it is deployed in the correct order.

I feel like we should add a note about that, wdyt?

yep, it's safe because libraries doesn't use its dependencies during deployment

there is already a note about the deployment order, do you think it should be extended?

foundry/crates/forge/src/link.rs

Line 28 in 20aad6b

/// The order in which they appear in the vector is the order in which they should be deployed.

no it's prob fine

Co-authored-by: Bjerg <onbjerg@users.noreply.github.com>

klkvr · 2024-02-09T18:31:10Z

imo eyre is more for CLI stuff, this should be more strongly typed

currently linker errors are more like panics and getting a error there probably means that the code invoking it didn't sanitize inputs normally, so it's not something we would need to catch

but we might be adding more features to linker (like that one) and for them we might have broader errors spectrum, so yeah it probably makes sense to add errors enum

DaniPopes

lg tm

crates/forge/src/multi_runner.rs

mattsse · 2024-02-12T22:33:31Z

only rustfmt, missing then send it @klkvr 🙏

mattsse reviewed Feb 7, 2024

View reviewed changes

mds1 reviewed Feb 7, 2024

View reviewed changes

Evalir reviewed Feb 7, 2024

View reviewed changes

klkvr mentioned this pull request Feb 7, 2024

Helper method for Libraries foundry-rs/compilers#72

Merged

mattsse pushed a commit to foundry-rs/compilers that referenced this pull request Feb 8, 2024

Helper method for Libraries (#72)

757652e

Adds helper method to strip path prefixes from `Libraries` object, I'll need this for foundry-rs/foundry#7027

klkvr added 6 commits February 8, 2024 16:20

rewrite linking

7c60403

fix ci

a5b1933

fix ci 2

cc9eefa

fix ci 3

4b1b6fa

docs

ec48668

Refactor

f900581

klkvr force-pushed the klkvr/link-rewrite branch from 6cf6caa to f900581 Compare February 8, 2024 13:20

klkvr added 2 commits February 8, 2024 16:21

fix

243eeee

fix tests

68143ed

handle root path

89d1a9d

klkvr added 4 commits February 8, 2024 17:53

tests

9e0aa35

clippy

3d720c5

tests

a12e87e

Bump compilers

824e027

mattsse requested changes Feb 8, 2024

View reviewed changes

review fixes

f847543

klkvr marked this pull request as ready for review February 8, 2024 17:49

klkvr requested a review from DaniPopes as a code owner February 8, 2024 17:49

Evalir approved these changes Feb 8, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

crates/forge/bin/cmd/script/build.rs Show resolved Hide resolved

crates/forge/bin/cmd/script/build.rs Show resolved Hide resolved

klkvr added 2 commits February 8, 2024 21:50

fix Cargo.toml

8884dc4

docs

474b608

ok_or_eyre -> ok_or_else for dynamic errors

834676e

klkvr added 3 commits February 9, 2024 02:54

refactor

2a37680

filter empty bytecode in scripts

e7c59a2

fix known_contracts for tests

61bcf0c

mattsse approved these changes Feb 9, 2024

View reviewed changes

klkvr added 2 commits February 9, 2024 18:04

get_bytecode_bytes

4c6062b

cycle lib deps

e5d808e

klkvr added 2 commits February 9, 2024 18:36

add doc about cyclic dependencies

25e47d1

add missed test file

20aad6b

mattsse changed the title ~~[WIP] Linking rewrite~~ feat: Linking rewrite Feb 9, 2024

onbjerg reviewed Feb 9, 2024

View reviewed changes

Evalir self-requested a review February 9, 2024 17:24

Update crates/forge/src/link.rs

c1a4f5d

Co-authored-by: Bjerg <onbjerg@users.noreply.github.com>

klkvr added 3 commits February 9, 2024 22:36

LinkerError

8926dc5

Merge branch 'master' into klkvr/link-rewrite

343aeb5

clippy

66b9909

DaniPopes approved these changes Feb 12, 2024

View reviewed changes

crates/forge/src/multi_runner.rs Outdated Show resolved Hide resolved

small fix

1114126

fmt

fc0e37b

klkvr merged commit 446bba8 into foundry-rs:master Feb 12, 2024
19 checks passed

DaniPopes mentioned this pull request Feb 20, 2024

refactor: linker #5366

Closed

sam-goldman mentioned this pull request Feb 27, 2024

Add --contract-sender flag to forge script for correct nonce handling w/ linked libraries #7248

Open

klkvr mentioned this pull request Apr 18, 2024

feat: library deployments though CREATE2 #7711

Merged

This was referenced Apr 26, 2024

Reset to upstream phylaxsystems/phoundry#12

Merged

feat/forge bin as lib phylaxsystems/phoundry#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Linking rewrite #7027

feat: Linking rewrite #7027

klkvr commented Feb 6, 2024 •

edited

mattsse left a comment

mattsse Feb 7, 2024

klkvr Feb 7, 2024

Evalir Feb 7, 2024

klkvr Feb 7, 2024

Evalir Feb 7, 2024 •

edited

mds1 Feb 7, 2024

klkvr Feb 7, 2024 •

edited

Evalir left a comment

Evalir Feb 7, 2024

klkvr commented Feb 8, 2024

klkvr commented Feb 8, 2024

mattsse left a comment

Evalir left a comment •

edited

klkvr commented Feb 8, 2024

klkvr commented Feb 9, 2024

mattsse left a comment

mattsse Feb 9, 2024

mattsse Feb 9, 2024

klkvr commented Feb 9, 2024

klkvr commented Feb 9, 2024

onbjerg left a comment

onbjerg Feb 9, 2024

klkvr Feb 9, 2024

onbjerg Feb 9, 2024

klkvr commented Feb 9, 2024

DaniPopes left a comment

mattsse commented Feb 12, 2024

	/// A library may show up more than once, but it should always have the same nonce and address
	/// with respect to the single `assert_dependencies` call. There should be no gaps in the nonce
	/// otherwise, i.e. whenever a new dependency is encountered, the nonce should be a single
	/// increment larger than the previous largest nonce.

feat: Linking rewrite #7027

feat: Linking rewrite #7027

Conversation

klkvr commented Feb 6, 2024 • edited

Motivation

Solution

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Evalir Feb 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klkvr Feb 7, 2024 • edited

Choose a reason for hiding this comment

Evalir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klkvr commented Feb 8, 2024

klkvr commented Feb 8, 2024

mattsse left a comment

Choose a reason for hiding this comment

Evalir left a comment • edited

Choose a reason for hiding this comment

klkvr commented Feb 8, 2024

klkvr commented Feb 9, 2024

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klkvr commented Feb 9, 2024

klkvr commented Feb 9, 2024

onbjerg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klkvr commented Feb 9, 2024

DaniPopes left a comment

Choose a reason for hiding this comment

mattsse commented Feb 12, 2024

klkvr commented Feb 6, 2024 •

edited

Evalir Feb 7, 2024 •

edited

klkvr Feb 7, 2024 •

edited

Evalir left a comment •

edited