-
Notifications
You must be signed in to change notification settings - Fork 434
test(wip): basic harness for precision loss analysis #1462
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
base: main
Are you sure you want to change the base?
Conversation
4a71510
to
0b6f6d8
Compare
5e3bde6
to
685665b
Compare
685665b
to
622e86b
Compare
} | ||
|
||
// TODO: consider incremental manual fuzzing from 1 up to WAD - 1 | ||
function test_rounding_allMagsSlashed(uint16 slashes, uint64 initialWadToSlash, uint64 _initTokenBalance, uint24 r) public rand(r) { |
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.
IMO we should choose a lane for param generation:
- either use
rand(r)
and the_randUint
method to generate all parameters - or allow forge to generate all parameters and if we need additional randomness, use
cheats.randomUint
and similar
#1 means we should do sequential runs, supplying (1, 2, 3, 4, etc)
#2 means we just let forge decide and we only control the number of runs
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.
Or I guess there's a way to do both - let forge generate parameters via #2, but run tests using forge t --fuzz-seed <1, 2, 3, ...>
That may be most convenient?
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.
I prefer option #2 but open to either. My impression was we eventually want to give Foundry some more direction (e.g. slash from initialMaxMag 1 to WAD - 1), but I believe we discussed that as a parallel, second route which we would do in addition to letting Foundry fire whatever inputs it feels like
vm.pauseGasMetering(); | ||
// Bound slashes to a reasonable range, with at least 1 slash. | ||
// Note: Runs after 2500 hit OOG errors with default gas. | ||
slashes = uint16(bound(slashes, 1, 5000)); |
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.
IMO number of slashes doesn't really impact the "path" so much as it defines the depth you're testing to (and overall runtime). I think this should not be a random param, but something we define.
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.
I actually think it's valuable to randomize within a bound! I've just found a test where 500 slashes does not show precision loss by the end of the test, but 1 slash does.
I suggest we either do one of two things:
- Keep # of slashes as parameterized value (easy, more simple)
- Measure more granular precision loss after every operation (hard, more granular)
We can measure it how we like, but I suggest we keep slashing as a parametrized value until we have more sophisticated intermediate measurements for precision loss in the file. So we can do option 1 until option 2 is ready.
deal(address(token), address(attacker), initTokenBalance); | ||
|
||
_magnitudeManipulation(initialWadToSlash); // Manipulate operator magnitude for a given strategy. | ||
_deposit(initialWadToSlash); // Setup operator with new opSet as well as honest stake in same strategy. |
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.
Here's a point we may want to change - we're allocating all available magnitude before we use multiple slashes to "get funds out".
It kinda makes sense that this wouldn't result in any loss.
What if we don't allocate all available magnitude here, and instead allocate a portion of it (parameterized). We then use multiple slashes to get a portion of the funds out via redistribution, and then attempt to withdraw remaining funds via queue+completeWithdrawals?
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.
Maybe this is captured in partialMagsSlashed? not sure.
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.
partialMagsSlashed
somewhat does this. It allocates all magnitude but doesn't fully slash at the end, leaving some partial residual magnitude. We can def add another test case that partially allocates.
initTokenBalance = uint64(bound(_initTokenBalance, 1, type(uint64).max)); | ||
deal(address(token), address(attacker), initTokenBalance); | ||
|
||
_magnitudeManipulation(initialWadToSlash); // Manipulate operator magnitude for a given strategy. |
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.
I kinda want to refactor this a bit to more explicitly set the parameter we're trying to set here -- we're using this method to "set max magnitude", not "initialWadToSlash"
…a file for longer fuzzing campaigns.
[DO NOT MERGE]
WIP harness for precision loss analysis. Run with
forge t --mt test_rounding -vvvv