fix(traces): identify artifacts using both deployed and creation code #9050
Conversation
grandizzy
requested review from
DaniPopes,
klkvr,
mattsse,
yash-atreya and
zerosnacks
as code owners
|
Could you expand on how the bug raised in #8995 is connected to the suggested change? As in, how does it solve it? |
ah, yep, here's the explanation #8998 (comment) |
| let mut score = bytecode_diff_score(deployed_bytecode, runtime_code); | ||
|
|
||
| // Compute and use creation code score. | ||
| if let Some(bytecode) = contract.bytecode() { | ||
| score += bytecode_diff_score(bytecode, creation_code); | ||
| score /= 2.0; | ||
| } |
There was a problem hiding this comment.
do we actually need to check both here? are there cases when comparing just creation code is not enough?
There was a problem hiding this comment.
this is done quite a lot, so don't think we should make it twice slower without a strong motivation
There was a problem hiding this comment.
there seems to be regression introduced if not checking both, for example for
abstract contract ParentProxy {
Counter impl;
bytes data;
constructor(Counter _implementation, bytes memory _data) {
impl = _implementation;
data = _data;
}
}and inherited
contract Proxy is ParentProxy {
constructor(Counter _implementation, bytes memory _data)
ParentProxy(_implementation, _data)
{}
}following test
function test_proxy_trace() public {
Counter counter = new Counter();
Proxy proxy = new Proxy(counter, "");
}results in Unknown id (for both with and without --no-metadata runs)
Traces:
[130526] Issue8995Test::test_proxy_trace()
├─ [38693] → new Counter@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
│ └─ ← [Return] 193 bytes of code
├─ [27175] → new <unknown>@0x2e234DAe75C793f67A35089C9d99245E1C58470b
│ └─ ← [Return] 9 bytes of code
└─ ← [Stop]
hence opted to compute for both
There was a problem hiding this comment.
this looks like an edge case we've discussed earlier. constructor args are of length 192 while bytecode length is 1320. on >10% diff we consider bytecodes completely different.
what we could do is to only fallback to deployed bytecode comparison if we haven't found a match through comparing creation codes and/or target for exact match when comparing creation codes (i.e if we have an artifact with creation code that exactly matches the beginning of some creation bytecode in traces, then this is a 100% match)
There was a problem hiding this comment.
@klkvr sorry for the delay, fell out of the radar. Can you please recheck and see if that's inline (if creation bytecode score is > threshold of 0.85 then the deployed bytecode is checked and confirm is fixing scenario above). thanks!
There was a problem hiding this comment.
nice! yep, this is waht we want. could we refactor this a bit and still have a single check closure which would just accept a is_creation: bool? looks like current closures are identical
There was a problem hiding this comment.
updated in 781b892 I also added a test for scenario mentioned above, ptal (unrelated tests failing due to avax rpc url) Thanks!
…foundry-rs#9050) * Identify by creation code * Compute score for both creation and runtime code * Fallback to deployed bytecode only if min creation bytecode score is under threshold * reuse check closure, add basic test
Motivation
Closes #8995
see #8998 (comment)
Solution