fix(linking): deep-merge link_references from creation and deployed bytecode #11759
Conversation
VolodymyrBg
requested review from
DaniPopes,
mattsse,
grandizzy,
yash-atreya,
zerosnacks,
onbjerg and
0xrusowsky
as code owners
|
Is there a test case we could add for this that would fail before? @klkvr thoughts? |
Yes, added a test case |
|
|
||
| #[test] | ||
| fn link_samefile_union() { | ||
| link_test("../../testdata/default/linking/samefile_union", |linker| { |
There was a problem hiding this comment.
looks like you didn't commit the linking/samefile_union but the test is still passing somehow 🤔
crates/linking/src/lib.rs
Outdated
| let mut references: BTreeMap<String, BTreeMap<String, Vec<Offsets>>> = BTreeMap::new(); | ||
| if let Some(bytecode) = &contract.bytecode { | ||
| references.extend(bytecode.link_references.clone()); | ||
| for (file, libs) in &bytecode.link_references { | ||
| let entry = references.entry(file.clone()).or_default(); | ||
| for (name, offsets) in libs { | ||
| entry.entry(name.clone()).or_default().extend(offsets.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
I see, the issue is that we were not supposed to use extend here bc it would make second cycle's values overwrite the file keys
this does make sense
it seems that we accumulate offsets in this map for no reason, do you mind also getting rid of this and changing references to BTreeMap<String, BTreeSet<String>> so that it's a bit easier to reason about?
There was a problem hiding this comment.
I see, the issue is that we were not supposed to use
extendhere bc it would make second cycle's values overwrite the file keysthis does make sense
it seems that we accumulate
offsetsin this map for no reason, do you mind also getting rid of this and changingreferencestoBTreeMap<String, BTreeSet<String>>so that it's a bit easier to reason about?
done, also added a test if artifacts are missing and committed samefile_union
VolodymyrBg
force-pushed
the
fix/linking-deep-merge-link-references
branch
from
554024c to
50d62f7
Compare
test / nextest / test unit (x86_64-pc-windows-msvc) (pull_request) I'm sorry but i can't fix this, i can't just test it on my computer, it's not that powerful, and I think making noise here is not the best idea.. |
crates/linking/src/lib.rs
Outdated
| ), | ||
| ( | ||
| "default/linking/duplicate/Duplicate.t.sol:C", | ||
| "default/linking/duplicate/Duplicate.t.sol:C".to_string(), |
There was a problem hiding this comment.
please undo these changes, you accepted the wrong side when merging master
VolodymyrBg
force-pushed
the
fix/linking-deep-merge-link-references
branch
from
7625f36 to
514142f
Compare
Before: Dependencies were built by shallow-merging link_references maps via extend. When both creation and deployed bytecode contained references under the same file key but with different library names, the latter overwrote the former, dropping dependencies.
After: We deep-merge the inner maps per file, unioning library names and offsets from both sources. This matches how we link and validate both bytecode objects and aligns with forge inspect ... libraries (union semantics). Prevents missing libs when a file has libraries used exclusively in constructor vs runtime.