-
Notifications
You must be signed in to change notification settings - Fork 71
Adding forcing of value for runAll #856
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Adding changelog Adding test that's wrong for the moment Add test
e7b87f1 to
2415e84
Compare
blishko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I just have a question about the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really using PleaseRunAll? Isn't there just one possible value?
I just want to make sure we are testing what we want to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there are at least two known address here: the deployed C and T contracts. The origin addresses are also valid ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point.
With current main, running: cabal run exe:hevm -- test --root tmp --match "only_deployed_force_addr" --only-deployed gives us:
[RUNNING] prove_only_deployed_force_addr(address)
[FAIL] prove_only_deployed_force_addr
Counterexample: [not reproducible]
calldata: prove_only_deployed_force_addr(0x0470C202ca30050800087234Ffee81fb7FCEaFc4)
result: Revert: 0x4e487b710000000000000000000000000000000000000000000000000000000000000001
This current branch gives:
[RUNNING] prove_only_deployed_force_addr(address)
[FAIL] prove_only_deployed_force_addr
Counterexample: [validated]
calldata: prove_only_deployed_force_addr(0xD95322745865822719164b1fC167930754c248DE)
result: Revert: 0x4e487b710000000000000000000000000000000000000000000000000000000000000001
The difference between the two is only this PR. The reason for this is that this runAll:
onlyDeployed addrExpr fallback continue = do
vm <- get
if not (?conf.onlyDeployed) then fallback addrExpr
else case eqT @t @Symbolic of
Just Refl -> do
let deployedAddrs = map forceEAddrToEWord $ mapMaybe (codeMustExist vm) $ Map.keys vm.env.contracts
runAll (?conf.maxDepth) vm.exploreDepth $ PleaseRunAll addrExpr deployedAddrs runAllPaths
_ -> internalError "Unknown address in Concrete mode"Will run for all --only-deployed as long as there are too many solutions. So it will run for this case, too, since the address can be anything here:
function prove_only_deployed_force_addr(address x) pure public {
uint256 y = C(x).f();
assert(y != 42);
}There is no restriction on x. So the SMT solver gets 100 different soltuions and then it gives up, and falls back to onlyDeployed:
forceAddr addrExpr fallback continue = case wordToAddr addrExpr of
Nothing -> manySolutions (?conf).maxDepth addrExpr 20 $ \case
Just sol -> continue $ LitAddr (truncateToAddr sol)
Nothing -> onlyDeployed addrExpr fallback continue
Just c -> continue cI hope this is a good explanation :) Took me a while to think through this but I hope it's a good description of what's going on!
|
Is there any blocker to merge? There is a test in red, but I don't know it is related 🤔 |
|
The failing test is unrelated. We'll investigate later. |
Description
As pointed out by Gustavo, we have actually missed forcing the value of the
runAll. This lead to incorrect address being generated for e.g. the small contract with--only-deployed:The address should have been that of
c, but instead, it was a random address. With this fix, the CEX validation system properly validates the CEX, i.e. the address is not random, but that ofc.This PR can only be merged on top of #827 -- currently failing because this PR is not yet merged
Checklist