Fuzz Shrinking feature #351

MeditationDuck · 2024-10-10T02:33:59Z

Fuzz Shrinking

1. Fuzz Test

To generate a crash log when the fuzz test is failing:

wake test tests/fuzz_test.py

2. Shrinking

To shrink the fuzz using the latest failure in the fuzz test:

wake test -SH

you can specify the test path. It verifies the testing target is the same:

wake test tests/fuzz_test.py -SH

Or specify a crash log directly:

wake test -SH .wake/logs/crashes/20241010_035704.txt

3. Reproduce the Error by Shrunk File

To reproduce the shrunk test:

wake test -SR

You can also specify the test here as well:

wake test tests/test_fuzz.py -SR

Alternatively, specify a shrunk data file:

wake test -SR .wake/logs/shrank/20241010_042322.bin

Shrinking phase

Shrinking tries to remove flows using two algorithms.
First, remove multiple flows thus it is faster, we print the progress of the removed flow.

Second, try to remove flow one by one.

✅ remove flow by flow kind (takes O(n))

✅ remove flow by brute force (takes O(n^2)

✅ flow fail because of removed flow dependency and it is not the target fail flow, can be removed.(checked precondition and un-executed flow will be removed in brute force shrink)

I clicked on "Allow edits from maintainers"

jaczkal · 2024-10-11T14:03:03Z

Does it also produce a crash log for failed invariants on assertions?

MeditationDuck · 2024-10-11T14:09:49Z

yes

MeditationDuck · 2024-10-11T15:39:27Z

Now, shrinking work with this

wake test -SH

michprev · 2024-12-05T15:16:00Z

wake/cli/test.py

+    type=str,
+    help="Path to the shrink log file.",
+    is_flag=False,
+    flag_value=0,


yes, true. we uses flag as well, but it better be string I will change to "-1"

michprev · 2024-12-05T15:16:11Z

wake/cli/test.py

+    type=str,
+    help="Path of shrank file.",
+    is_flag=False,
+    flag_value=0,


same question, why 0?

same as above

michprev · 2024-12-05T15:20:08Z

wake/development/core.py

@@ -1462,6 +1462,9 @@ class Chain(ABC):

    tx_callback: Optional[Callable[[TransactionAbc], None]]

+    def __deepcopy__(self, memo):


this may be tricky

on one side I understand why it's needed (FuzzTest members will contain Chain transitively), on the other snapshot + revert won't completely restore the state, just a subset

for example, default accounts won't be restored

we should either "backup" more attributes or find a better way

Other than the data stored in the FuzzTest class would not stored,
we store random state already in data collecting phase.
chain and account information couls store in chain.snapshot().

Even tester defined outside of FuzztestClass, chain state would stored.
since those chains are added in wake.testing.core.connected_chains as global variable and Shrinking use this array.

And we can add missing member in Chain() in snapshot function.

yeah, I think there are a few attributes missing to be stored in snapshot, then it should be good

michprev · 2024-12-05T15:20:30Z

wake/development/json_rpc/communicator.py

@@ -43,6 +43,7 @@ class JsonRpcCommunicator:
    _protocol: ProtocolAbc
    _request_id: int
    _connected: bool
+    _interrupt_received: bool


can be removed, I think

michprev · 2024-12-05T15:24:53Z

wake/testing/fuzzing/fuzz_shrink.py

+    # from wake.development.transactions import Error
+    if type(e1) == Error and type(e2) == Error:
+        # If it was the Error(TransactionRevertedError), compare message content.
+        if e1.message != e2.message:
+            return False


why must there be an extra check for Error? What about other exceptions of the same type but different values/members?

This is a transaction error.

If the transaction reverts with Error().

like, revert("Switch Pushed");

In this case, it is reverted without a custom error. It is usually with string. We compare the message.

But if there is no message with just require() and also multiple places, it is unable to distinguish.

yes but what if we are trying to reproduce NotOwner(0x742d35Cc6634C0532925a3b844Bc454e4438f44e) but encounter NotOwner(0x19E7E376E7C213B7E7e7e46cc70A5dD086DAff2A)?

shouldn't we just compare with ==?

ah, I see the next comment

michprev · 2024-12-05T15:26:30Z

wake/testing/fuzzing/fuzz_shrink.py

+    frame1 = None
+    for frame1 in tb1:
+        if is_relative_to(
+            Path(frame1.filename), Path.cwd()
+        ) and not is_relative_to(
+            Path(frame1.filename), Path().cwd() / "pytypes"
+        ):
+            break
+    frame2 = None
+    for frame2 in tb2:
+        if is_relative_to(
+            Path(frame2.filename), Path.cwd()
+        ) and not is_relative_to(
+            Path(frame2.filename), Path().cwd() / "pytypes"
+        ):
+            break


we are searching here for a frame with an exception that happened in our cwd but not in pytypes, correct?

so the comparison does not only take into account the exception data but also the location where it happened?

Yes, exactly.
If it was a transaction error, rely on the Error type also if transaction. Error then relies on message. And does not care about another error argument.

If it was an error in Python, rely on the file and line number in Python. so we do not care about the actual value.

There was a lot of consideration.
Since it could depend on the definition of the "same error".

The purpose of shrinking is to create a minimum flow sequence to reproduce the same error.

The same error could be different.

the same error is emitted.

the same assertion in Python fails. (fails at the same line in the Python test)

or

the same error with the same argument is emitted. (and also in exactly the same flow)

the same assertion in Python fails with the same value. (and also in exactly the same flow)

I decided to implement 1. and 2. since these conditions could significantly shorten the test.
For example, If it was 3. 4. and Error was emitted with TransferError(nft_id=10), then at least the test required to emit 10 NFT and make an error. This would be redundant.

However, one possible issue is that when checking the balance for each account, the shrinken result shows an unbalance for different accounts.

@invariant(period=30) def invariant_erc20_balances(self): for contract in self.erc20_balances: for acc in self.erc20_balances[contract]: assert contract.balanceOf(acc) == self.erc20_balances[contract][acc] # <- error in same file and same line.

Got it. Then we should at least implement the same logic for Panic error as it behaves the same as Error.

Still I think we should implement strict shrinking feature where we compare the errors exactly with ==.

Yes, that's true.

I already have an exact match for the flow number. I can extend this also Error matching.

wake/wake/testing/fuzzing/fuzz_shrink.py

Line 36 in 6256be2

IGNORE_FLOW_INDEX = True # True if you accept it could reproduce same error earlier.

michprev · 2024-12-05T15:29:09Z

wake/testing/fuzzing/fuzz_shrink.py

+        set_sequence_initial_internal_state(
+                pickle.dumps(
+                random.getstate()
+            )
+        )


why is this needed when we're not in the shrinking mode?

This is for the crash log file as well. We accept not only seeds but also random states in the test argument.

Also, the crash log file stores only the flow number and random state at the beginning of the sequence. So, shrinking or re-fuzzing using a crash log file does not need to be repeated.

michprev · 2024-12-06T16:10:44Z

wake/testing/fuzzing/fuzz_shrink.py

+    def revert(self, python_instance: FuzzTest, chains: Tuple[Chain, ...]):
+        assert self.chain_states != [], "Chain snapshot is missing"
+        assert self._python_state is not None, "Python state snapshot is missing "
+        assert self.flow_number is not None, "Flow number is missing"
+
+        python_instance.__dict__.update(copy.deepcopy(self._python_state.__dict__))
+
+        self._python_state = None
+        for temp_chain, chain in zip(self.chain_states, chains):
+            chain.revert(temp_chain)
+        self.chain_states = []


As I understand it, we always do revert just to create another snapshot just after the revert.

In the case of chain snapshot I'm afraid it's necessary to call it again but I don't think we need to create deepcopy snapshot again. Could it be optimized? Also, I don't understand why is deepcopy used in this revert function.

As far as I remember, Anvil creates a snapshot and returns the ID, but once the snapshot is used, it is removed, and reverting to that ID will fail.

But, the Python instance seems to be working with direct assignment.

michprev · 2024-12-06T16:13:52Z

wake/testing/fuzzing/fuzz_shrink.py

+    with print_ignore():
+        test_instance._flow_num = 0
+        test_instance.pre_sequence()
+        exception_content = None
+        try:
+            with redirect_stdout(open(os.devnull, 'w')), redirect_stderr(open(os.devnull, 'w')):


isn't the redirection applied twice? once in print_ignore and for the second time directly here?

This was redundant. removed.

michprev · 2024-12-06T16:27:04Z

Based on my measurements, most of the time is spent in invariants - can be even 70%. Do you think it would be possible to skip the execution of invariants when trying to reproduce the exception when deciding whether to keep a flow or not?
Of course, the last invariant for the erroring flow wouldn't be skipped.

Of course, it brings up some problems to handle:

Python state (incl. random generator state) may be altered in invariants
- but we can handle it in the same way as skipped flows, I believe
chain state may be altered
- this can be a big issue as we would never reproduce the bug
- but it's not recommended to send txs / change chain state in invariants
- we might even implement auto snapshot & revert for invariants (with optional override) - in this case, we would know what invariants must not be skipped
we might miss the opportunity to encounter the bug in one of earlier invariant executions
- but I think the proposed optimization would be on average more efficient

What do you think?

MeditationDuck · 2024-12-06T17:12:40Z

It could depend on the project and situation.
I like having invariant and detecting the same error because if it is found there, the shrinking phase is significantly faster.
Also, if those invariants find another error, they will take the flow required and proceed to the next flow.
But also, I agree that the shrinking is used after a lot of fuzz was run. In this case, we know only one invariant would fail at specific flow.

MeditationDuck · 2024-12-06T17:30:32Z

Having invariant
pros

find shortcut error,
find early in the fuzz test that the error is unrelated to the target.
(but most of the time, it will succeed and overrun)

cons

slower around 70%

It can select specific invariants. (target error occurred in erc20 balance, then do only this test)

It can change check or not depending on the shrinking removal

Brute force removal usually does not find significant shortcuts and differs from removing by flow kinds.

I think a bit more thinking is required.

…o think about random state input

MeditationDuck requested a review from jaczkal October 10, 2024 02:33

jaczkal requested a review from michprev October 11, 2024 14:04

MeditationDuck and others added 6 commits December 5, 2024 09:07

✨ Implement fuzz test shrinking

5835207

✨ single process error fixed but this commit is to remove lator

5425636

🔥 Remove no-pytest testing mode

e43a44b

🚧 fix datatype and pytest arguments

08bd1d9

✏️ apply typing fix

097ac93

🚸 Improve error on abs. paths in wake open

6256be2

michprev force-pushed the feat/fuzz-crash-shrink branch 2 times, most recently from 81df7ae to 6256be2 Compare December 5, 2024 15:22

michprev reviewed Dec 5, 2024

View reviewed changes

michprev reviewed Dec 6, 2024

View reviewed changes

MeditationDuck added 11 commits December 15, 2024 18:26

🚧 fix datatype and pytest arguments

5c73d39

✏️ apply typing fix

76851ce

✏️ review fix

b8687dd

✏️ add exact exception match option and remove redundant print ignore

b4f3bad

🚧 add no run invariants option

dff92bd

✨ add flow step execution

a4a09c8

✏️ chain copy elements for snapshot

16974e1

✏️ blacked fuzz_shrink file

7874c50

✨ remove developping feature and update comments

1c049b3

✏️ Remove .vscode/settings.json from version control

ce2fd4f

🚧 remove pickle. other than --random-state option are removed. need t…

c309f7f

…o think about random state input

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fuzz Shrinking feature #351

Fuzz Shrinking feature #351

MeditationDuck commented Oct 10, 2024 •

edited

Loading

jaczkal commented Oct 11, 2024

MeditationDuck commented Oct 11, 2024

MeditationDuck commented Oct 11, 2024

michprev Dec 5, 2024

MeditationDuck Dec 6, 2024

michprev Dec 5, 2024

MeditationDuck Dec 6, 2024

michprev Dec 5, 2024

MeditationDuck Dec 6, 2024 •

edited

Loading

michprev Dec 6, 2024

michprev Dec 5, 2024

MeditationDuck Dec 6, 2024

michprev Dec 5, 2024

MeditationDuck Dec 6, 2024

michprev Dec 6, 2024

michprev Dec 6, 2024

michprev Dec 5, 2024

MeditationDuck Dec 6, 2024

michprev Dec 6, 2024

MeditationDuck Dec 6, 2024

michprev Dec 5, 2024

MeditationDuck Dec 6, 2024

michprev Dec 6, 2024

MeditationDuck Dec 6, 2024

michprev Dec 6, 2024

MeditationDuck Dec 6, 2024

michprev commented Dec 6, 2024

MeditationDuck commented Dec 6, 2024

MeditationDuck commented Dec 6, 2024 •

edited

Loading

		@@ -1462,6 +1462,9 @@ class Chain(ABC):

		tx_callback: Optional[Callable[[TransactionAbc], None]]

		def __deepcopy__(self, memo):

Fuzz Shrinking feature #351

Are you sure you want to change the base?

Fuzz Shrinking feature #351

Conversation

MeditationDuck commented Oct 10, 2024 • edited Loading

Fuzz Shrinking

1. Fuzz Test

2. Shrinking

3. Reproduce the Error by Shrunk File

Shrinking phase

jaczkal commented Oct 11, 2024

MeditationDuck commented Oct 11, 2024

MeditationDuck commented Oct 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MeditationDuck Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michprev commented Dec 6, 2024

MeditationDuck commented Dec 6, 2024

MeditationDuck commented Dec 6, 2024 • edited Loading

MeditationDuck commented Oct 10, 2024 •

edited

Loading

MeditationDuck Dec 6, 2024 •

edited

Loading

MeditationDuck commented Dec 6, 2024 •

edited

Loading