Speed up shrinking in some cases #1192
Conversation
WalkthroughThe update to the Changes
Assessment against linked issues
Related issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
lib/Echidna/ABI.hs
Outdated
| shrinkAbiCall (name, vals) = do | ||
| let numShrinkable = length $ filter canShrinkAbiValue vals | ||
|
|
||
| halfwayVal <- getRandomR (0, numShrinkable) | ||
| -- This list was made arbitrarily. Feel free to change | ||
| let numToShrinkOptions = [1, 2, halfwayVal, numShrinkable] | ||
|
|
||
| numToShrink <- min numShrinkable <$> uniform numToShrinkOptions | ||
| shrunkVals <- shrinkVals (fromIntegral numShrinkable) (fromIntegral numToShrink) vals | ||
| pure (name, shrunkVals) | ||
| where | ||
| shrinkVals _ 0 l = pure l | ||
| shrinkVals _ _ [] = pure [] | ||
| shrinkVals numShrinkable numToShrink (h:t) | ||
| | not (canShrinkAbiValue h) = (h:) <$> shrinkVals numShrinkable numToShrink t | ||
| | otherwise = do | ||
| -- We want to pick which ones to shrink uniformly from the vals list. | ||
| -- Odds of shrinking one element is numToShrink/numShrinkable. | ||
| shouldShrink <- fromList [(True, numToShrink), (False, numShrinkable-numToShrink)] | ||
| h' <- if shouldShrink then shrinkAbiValue h else pure h | ||
| let | ||
| numShrinkable' = numShrinkable-1 | ||
| numToShrink' = if shouldShrink then numToShrink-1 else numToShrink | ||
| (h':) <$> shrinkVals numShrinkable' numToShrink' t |
There was a problem hiding this comment.
The refactoring of the shrinkAbiCall function introduces a more nuanced approach to shrinking contract call parameters by selecting a subset of parameters to shrink. This approach is expected to enhance the efficiency of the shrinking process, especially in scenarios where shrinking all parameters simultaneously leads to suboptimal outcomes. The implementation uses randomness and uniform probabilities in selecting the subset size and the parameters to shrink, which aligns with the PR objectives. However, there are a few points to consider:
-
Randomness and Reproducibility: The use of randomness in the shrinking process can introduce variability in the outcomes. It's important to ensure that this variability does not compromise the reproducibility of test results. Consider documenting the seed used for random number generation or providing a mechanism to set it, ensuring consistent results across runs when needed.
-
Efficiency and Performance: While the new approach aims to improve efficiency, it's crucial to assess its performance impact comprehensively. The introduction of randomness and the selection process for the subset of parameters to shrink could potentially introduce overhead. Conducting benchmark tests comparing the new and old strategies in various scenarios would be beneficial to quantify the improvements and identify any cases where the new approach might not perform as expected.
-
Edge Cases and Robustness: The logic for determining the subset of parameters to shrink (lines 280-284) and the subsequent shrinking process (lines 290-300) should be thoroughly tested to ensure robustness. Consider edge cases such as functions with a large number of parameters, parameters that cannot be further shrunk, and scenarios where the chosen subset size is larger than the number of shrinkable parameters.
-
Documentation and Comments: The comments in the code provide some insight into the logic and intentions behind the changes. However, expanding the documentation to include more details about the rationale, expected outcomes, and potential limitations of the new shrinking strategy would be helpful for future maintainers and users of the Echidna framework.
Overall, the changes to the shrinkAbiCall function represent a promising improvement to the Echidna testing framework's shrinking process. Addressing the points mentioned above can further enhance its effectiveness, reliability, and usability.
samalws-tob
force-pushed
the
slow-shrink
branch
from
f39dc29
to
f4cb053
Compare
|
This is quite a fancy solution however I feel it's a bit too complex. Wouldn't something like this work too: shrinkAbiCall (func, vals) = do
vals' <- forM vals $ \val -> do
chance :: Float <- getRandomR (0,1)
-- 50% chance of shrinking a value
if chance < 0.5 then shrinkAbiValue val
else pure val
pure (func, vals') |
Actually, we don't need floats here, let's keep everything in integers if possible. |
|
The reason I wanted to avoid doing that is if we have a function with a lot of arguments and we can shrink only one of them, the odds of that one (and no others) getting picked by the simplified algo is 1/2^n |
|
Ok, let's see how it performs and adjust if we feel the need |
This PR changes the process used to shrink a contract call. For a given contract call, instead of trying to shrink all the parameters at once, it picks a subset of the parameters and shrinks those. Subset size is chosen out of
[1,2,rng(0,n),n]; this means that sometimes we will try to shrink just one parameter, sometimes we will try to shrink all parameters, etc.I'm fairly sure this fixes #1028 , and it also definitely fixes the following proof-of-concept bug, which the current echidna code on the master branch is unable to shrink:
Summary by CodeRabbit