Skip to content

Add fuzz tests for funds adding up when squeezing - #364

Closed
BhariGowda wants to merge 1 commit into
drips-network:mainfrom
BhariGowda:test/squeeze-funds-add-up
Closed

BhariGowda wants to merge 1 commit into
drips-network:mainfrom
BhariGowda:test/squeeze-funds-add-up

Conversation

@BhariGowda

Copy link
Copy Markdown

The three conservation fuzz tests in Streams.t.sol (testFundsStreamedToReceiversAddUp, testFundsStreamedToReceiversAddUpAfterStreamsUpdate and testFundsStreamedFromSendersAddUp) all take funds out with receiveStreams. None of them squeeze, so squeezing was not covered by any of the funds adding up checks. It is the only path that writes negative amtDeltas, so it looked like the gap most worth closing.

This adds two tests asserting the same property those three assert, that squeezed plus withdrawn plus received equals the balance the sender started with.

testFundsSqueezedAndReceivedAddUp squeezes twice at independently fuzzed times, each up to three cycles long. The two squeezes can land in the same cycle or in different ones, and the second one can fall after the streams have already run out.

testFundsSqueezedAcrossStreamsUpdatesAddUp squeezes once, replaces the receivers list mid-stream, then squeezes both the old and the new list against a two entry history. This covers the currCycleConfigs slot mapping in _squeezeStreamsResult and stale nextSqueezed values being clamped up to the current cycle start.

Both pass at 50000 runs, the count CI uses on pushes.

The two tests cap the receivers list at 10 instead of the 100 the existing tests use. Squeezing every receiver in a list is quadratic, and at 100 these two cost more than the existing three combined. At 10 they finish in 81s at 50000 runs, with mean gas of 0.9M and 1.6M against 8.1M for testFundsStreamedToReceiversAddUpAfterStreamsUpdate. The squeezing logic does not depend on the list length, and longer lists are already covered by testStreamsWithManyReceiversCanBeSqueezed.

No source changes. forge test is green at 380 tests, forge fmt --check passes and Slither reports nothing.

The three existing conservation fuzz tests in Streams.t.sol all move funds
with receiveStreams only, so squeezing was never covered by them. Squeezing
is the one path that writes negative amtDeltas, which makes it the part of
the accounting most worth fuzzing.

The two new tests assert that squeezed plus withdrawn plus received equals
the balance the sender started with. The first repeats squeezing at two
fuzzed points that can fall in different cycles, the second squeezes both
before and after the receivers list is replaced mid-stream, so the history
has two entries and the currCycleConfigs slot mapping is exercised.

Both pass at 50000 runs, the count CI uses on pushes. They use at most 10
receivers because squeezing every receiver in a list is quadratic and the
squeezing logic does not depend on the list length.
@BhariGowda

Copy link
Copy Markdown
Author

Reopening with a fix to the tests themselves.

They passed, but they were barely exercising the path they exist to cover. Squeezing only reaches back to the start of the current cycle, so the squeezable window is skip mod cycleSecs, which with cycleSecs at 10 is at most 9 seconds and is often 1. sanitizeReceivers leaves amtPerSec as low as minAmtPerSec and start anywhere in the uint32 range, and over a window that short the amount owed rounds down to zero in _streamedAmt. I measured it: testFundsSqueezedAndReceivedAddUp squeezed a nonzero amount in 0 of 500 runs, testFundsSqueezedAcrossStreamsUpdatesAddUp in 3 of 5000. The conservation identity held, but only in the form squeezed == 0, so both tests would have kept passing if squeezing were broken.

The fix gives the squeeze tests their own sanitizer instead of the generic one. sanitizeSqueezedReceivers keeps the ordering and deduplication that sanitizeReceivers does, then bounds amtPerSec to between 1 and 16 tokens per second, enough that even a one second window is owed at least one unit, and bounds start into the test's own timespan. Over 50000 runs each, the count CI uses on pushes:

before after
testFundsSqueezedAndReceivedAddUp 0 / 500 34489 / 50000 (69.0%)
testFundsSqueezedAcrossStreamsUpdatesAddUp 3 / 5000 37569 / 50000 (75.1%)

The runs that still squeeze zero are empty receiver lists (10.4%), zero balances (1.6%) and squeeze times landing exactly on a cycle boundary, which gives a zero length window. I left those in rather than filtering them out.

I also corrected a factual error in the description. It justified the 10 receiver cap by claiming longer lists were already covered by testStreamsWithManyReceiversCanBeSqueezed. That test uses three hardcoded receivers, which is shorter than the cap, so the justification did not hold. The real reason is that these tests squeeze every receiver in the list, which is quadratic in its length, and _squeezedAmt sums each receiver's amount independently, so the list length is not a dimension of the squeezing accounting.

Verification: Slither reports 0 findings, forge test --deny warnings is 380 passed and 0 failed, forge fmt --check is clean.

One note on CI, so a red check here is not misread. The Tests run on this PR failed without executing a single job. This repo has had two pull_request workflow runs in total, this one and #363, both from forks, and both failed at startup with zero jobs while push runs on main with the same workflow file succeed. So the red X is not a test failure and it is not something I can turn green from a fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants