[move-fuzz] A DUG-guided fuzzer for Move-based smart contracts#19406
[move-fuzz] A DUG-guided fuzzer for Move-based smart contracts#19406meng-xu-cs wants to merge 4 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } else { | ||
| self.mutator.random_signer() | ||
| } | ||
| }, |
There was a problem hiding this comment.
Double pick_seed_index causes sender/mutation seed mismatch
Medium Severity
In run_one, seed_choice captures the result of pick_seed_index() at line 551, but then in the Some(_) arm at line 555, pick_seed_index() is called a second time to determine the sender. Later at line 584, the original seed_choice index is used for mutation. This means the sender is derived from one randomly-selected seed while the mutation base comes from a different randomly-selected seed, which is almost certainly unintentional — the fuzzer intends to mutate a single coherent seed, not mix two.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9b156adb3674d353011fdbb85ce5788e22aa7254. Configure here.
| @@ -86,7 +86,7 @@ impl CoverageMap { | |||
| } else { | |||
| // Don't count scripts (for now) | |||
| assert_eq!(context_segs.pop().unwrap(), "main",); | |||
| assert_eq!(context_segs.pop().unwrap(), "Script",); | |||
| assert_eq!(context_segs.pop().unwrap(), "script",); | |||
There was a problem hiding this comment.
Inconsistent "Script" vs "script" assertion across trace parsers
High Severity
The update_coverage_from_trace_file method in CoverageMap was changed to assert "script" (lowercase), but the parallel update_from_trace_file method in TraceMap (line 335 of the same file) still asserts "Script" (uppercase). Since both methods parse the same VM trace format, one of them will panic at runtime when encountering a script trace entry. This is a correctness regression — whichever casing the VM actually emits, the other parser will crash.
Reviewed by Cursor Bugbot for commit 9b156adb3674d353011fdbb85ce5788e22aa7254. Configure here.
There was a problem hiding this comment.
Pull request overview
This PR reintroduces the Move fuzzer into aptos-core and exposes it as aptos move fuzz, including project/package resolution, driver-script generation, local execution, and persistent fuzzing state (Phase 1 + DUG-guided Phase 2).
Changes:
- Adds the new
move-fuzzRust crate (executor, simulator, package handling, script generation/prep, mutation, persistence, CLI glue) plus documentation and dev runner. - Integrates the fuzzer into the Aptos CLI (
aptos move fuzz) and the workspace build. - Adjusts supporting infrastructure (Move package locking helper, coverage trace parsing, FakeExecutor cloning/state delta access) to support fuzzing workflows.
Reviewed changes
Copilot reviewed 42 out of 45 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| third_party/move/tools/move-package/src/resolution/resolution_graph.rs | Adds a helper to download/update git deps under a strict package lock. |
| third_party/move/tools/move-fuzz/tests/prep/sources/test_struct_with_copy_drop.move | Adds Move source used for prep/script-gen testing scenarios. |
| third_party/move/tools/move-fuzz/tests/prep/sources/test_only_simple_args.move | Adds Move source used for prep/script-gen testing scenarios. |
| third_party/move/tools/move-fuzz/tests/prep/Move.toml | Adds a test Move package manifest for prep tests. |
| third_party/move/tools/move-fuzz/tests/demo/sources/demo.move | Adds a small demo target module for fuzzing. |
| third_party/move/tools/move-fuzz/tests/demo/Move.toml | Adds a demo Move package manifest. |
| third_party/move/tools/move-fuzz/src/utils.rs | Adds a helper to temporarily disable logging (used during builds). |
| third_party/move/tools/move-fuzz/src/subexec.rs | Adds subprocess execution wrapper used by the simulator/CLI invocations. |
| third_party/move/tools/move-fuzz/src/simulator.rs | Adds localnet-based simulator wrapper for publishing/compiling/running Move. |
| third_party/move/tools/move-fuzz/src/prep/model.rs | Implements model building + script generation orchestration, dedup, and progress reporting. |
| third_party/move/tools/move-fuzz/src/prep/mod.rs | Exposes prep submodules. |
| third_party/move/tools/move-fuzz/src/prep/ident.rs | Adds typed identifiers for modules/datatypes/functions. |
| third_party/move/tools/move-fuzz/src/prep/function.rs | Adds public-function discovery and source-based filtering. |
| third_party/move/tools/move-fuzz/src/prep/datatype.rs | Adds datatype discovery/content analysis and type instantiation. |
| third_party/move/tools/move-fuzz/src/package.rs | Adds build/unit-test helpers and persistent package build cache support. |
| third_party/move/tools/move-fuzz/src/mutate/mod.rs | Adds mutation module entrypoint. |
| third_party/move/tools/move-fuzz/src/lib.rs | Registers the move-fuzz crate modules. |
| third_party/move/tools/move-fuzz/src/language.rs | Adds language/optimization setting parsing and CLI/compiler config derivation. |
| third_party/move/tools/move-fuzz/src/executor/tracing.rs | Adds tracing executor for resource read/write profiling and package provisioning. |
| third_party/move/tools/move-fuzz/src/executor/oneshot.rs | Adds Phase 1 oneshot fuzzer (coverage + seed pool management). |
| third_party/move/tools/move-fuzz/src/executor/mod.rs | Adds shared coverage map utilities for fuzzers. |
| third_party/move/tools/move-fuzz/src/deps.rs | Adds Move project discovery, dependency resolution, and named-address handling. |
| third_party/move/tools/move-fuzz/src/common.rs | Adds shared account + txn arg types and conversions. |
| third_party/move/tools/move-fuzz/src/bin/move-fuzz-dev.rs | Adds standalone dev runner mirroring the Aptos CLI surface. |
| third_party/move/tools/move-fuzz/src/account.rs | Adds address registry for named addresses/users within the executor. |
| third_party/move/tools/move-fuzz/README.md | Adds user-facing documentation for running aptos move fuzz / move-fuzz-dev. |
| third_party/move/tools/move-fuzz/Cargo.toml | Introduces the move-fuzz crate manifest and dependencies. |
| third_party/move/tools/move-fuzz/AGENT.md | Adds contributor/agent guide for the move-fuzz subtree. |
| third_party/move/tools/move-coverage/src/coverage_map.rs | Updates script trace parsing expectation (script::main casing). |
| Cargo.toml | Adds move-fuzz to workspace members and workspace dependencies. |
| Cargo.lock | Records new crate and dependency resolutions. |
| aptos-move/e2e-tests/src/executor.rs | Adds FakeExecutor duplication helper + state delta extraction for fuzzing. |
| aptos-move/cli/src/lib.rs | Wires in the new fuzz module. |
| aptos-move/cli/src/fuzz.rs | Adds the aptos move fuzz CLI command wrapper. |
| aptos-move/cli/src/commands.rs | Registers MoveTool::Fuzz in the move CLI command enum/dispatch. |
| aptos-move/cli/Cargo.toml | Adds the move-fuzz dependency to the CLI crate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match signal { | ||
| None => { | ||
| if let Err(e) = child.wait() { | ||
| error!("failed to wait for process: {e}"); | ||
| has_error = true; | ||
| } |
There was a problem hiding this comment.
SubExec::end() calls child.wait() in the signal == None branch and then calls child.wait() again unconditionally later. With a Child-like API the second wait will typically error (or panic via expect), and it also discards the exit status from the first wait. Consider waiting exactly once (capture the ExitStatus from the first wait, or only call wait() after sending a signal) and reuse that status for the return value.
| if status.is_some() { | ||
| while !self.thread_stdout.is_finished() { | ||
| // wait for stdout thread to finish | ||
| } | ||
| while !self.thread_stderr.is_finished() { | ||
| // wait for stderr thread to finish | ||
| } | ||
| } |
There was a problem hiding this comment.
SubExec::probe() uses tight busy-wait loops (while !is_finished() {}) when the child has exited. This can spin a core at 100% until the reader threads drain/finish. Add a small sleep/yield (e.g. std::thread::yield_now() / sleep(Duration::from_millis(...))) or restructure to join the threads / collect output without active polling.
| let lines = stderr.read().expect("stderr read lock"); | ||
| let count = lines.len(); | ||
| if count == next_line { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The localnet readiness loop busy-spins when no new stderr lines are available (if count == next_line { continue; }). This will consume CPU while waiting for the node to start. Consider adding a small backoff (sleep/yield) or blocking on process output / a channel from the stderr reader thread.
| let seed_choice = self | ||
| .mutator | ||
| .should_mutate(self.seedpool.len()) | ||
| .map(|_| self.pick_seed_index()); | ||
| let sender = match seed_choice { | ||
| None => self.mutator.random_signer(), | ||
| Some(_) => { | ||
| let index = self.pick_seed_index(); | ||
| if self.mutator.random_percent() < 70 { | ||
| self.seedpool[index].input.sender | ||
| } else { | ||
| self.mutator.random_signer() | ||
| } | ||
| }, | ||
| }; |
There was a problem hiding this comment.
seed_choice computes a seed index (self.pick_seed_index()) but the index is not reused: the code calls pick_seed_index() again inside the Some(_) branch. This can cause the chosen seed for mutation and the chosen seed for sender selection to diverge, and it also updates last_used_at on multiple records unexpectedly. Store the picked index once and reuse it throughout this execution step.
| [dependencies] | ||
| AptosFramework = { git = "https://github.com/aptos-labs/aptos-core.git", subdir = "aptos-move/framework/aptos-framework/", rev = "main" } | ||
|
|
There was a problem hiding this comment.
These test Move packages depend on AptosFramework via git ... rev = "main", which is non-deterministic and may require network access during tests. For reproducible/offline CI, prefer a pinned commit hash/tag or a local path dependency to the in-repo framework sources.
| [dependencies] | ||
| AptosFramework = { git = "https://github.com/aptos-labs/aptos-core.git", subdir = "aptos-move/framework/aptos-framework/", rev = "main" } | ||
|
|
There was a problem hiding this comment.
These test Move packages depend on AptosFramework via git ... rev = "main", which is non-deterministic and may require network access during tests. For reproducible/offline CI, prefer a pinned commit hash/tag or a local path dependency to the in-repo framework sources.
| if has_error { | ||
| match child.kill() { | ||
| Ok(()) => (), | ||
| Err(e) => panic!("unable to kill command: {e}"), | ||
| } | ||
| } |
There was a problem hiding this comment.
SubExec::end() uses panic! on kill failures (and later on thread join panics). Panicking here will abort the whole fuzz run and skip higher-level cleanup/reporting. Prefer returning an error (bail!/anyhow!) so callers can surface diagnostics and shut down gracefully.
| let version = LanguageVersion::from_str(rest)?; | ||
|
|
||
| // sanity check | ||
| if matches!(version, LanguageVersion::V1) && !matches!(optimization, OptLevel::Default) { |
There was a problem hiding this comment.
I don't think we need to support language version V1 anymore
| // Don't count scripts (for now) | ||
| assert_eq!(context_segs.pop().unwrap(), "main",); | ||
| assert_eq!(context_segs.pop().unwrap(), "Script",); | ||
| assert_eq!(context_segs.pop().unwrap(), "script",); |
There was a problem hiding this comment.
this is a little concerning. Why do we need this change?
| Phase 1 is single-transaction fuzzing with online profiling. | ||
| Phase 2 is multi-transaction fuzzing driven by the DUG and chain seed pools. | ||
|
|
||
| The design inspiration is in `docs/idea.pdf`, but the implementation intentionally goes beyond the slides. When changing DUG-related logic, compare against `docs/idea.pdf` and call out any semantic gap you introduce or close. |
There was a problem hiding this comment.
Where is docs/idea.pdf? Do we need this?
| pub mod deps; | ||
| pub mod package; | ||
|
|
||
| // pub mod executor; |
| path::{Path, PathBuf}, | ||
| }; | ||
|
|
||
| pub const AUTO_STATE_VERSION: u32 = 6; |
There was a problem hiding this comment.
could you explain what 6 means here?
|
|
||
| pub const AUTO_STATE_VERSION: u32 = 6; | ||
| pub const AUTO_STATE_FILENAME: &str = "auto_state.json"; | ||
| pub const ENTRYPOINT_CACHE_VERSION: u32 = 2; |
There was a problem hiding this comment.
same here, maybe add a comment on the version?
|
|
||
| /// Supported transaction argument types | ||
| #[derive(Clone)] | ||
| pub enum TxnArgType { |
There was a problem hiding this comment.
note that public structs/enums will be supported soon.
There was a problem hiding this comment.
also, support of signed integers are not here.
| executor | ||
| } | ||
|
|
||
| pub fn duplicate_with_assumption(&self) -> Self { |
There was a problem hiding this comment.
Are duplicate_with_assumption and get_state_delta being used anywhere in this PR?
|
|
||
| const CHAIN_REBUILD_INTERVAL_SECS: u64 = 60; | ||
| const SEQUENCE_MUTATION_INTERVAL_SECS: u64 = 30; | ||
| const SEED_SCORE_COVERAGE: u32 = 32; |
There was a problem hiding this comment.
maybe add comments for these constants or add them to the design doc?
|
|
||
| /// Types that can be argument of driver function | ||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub enum BasicInput { |
There was a problem hiding this comment.
Type is represented in several places in this PR such as TxnArgType and BasicInput. I am wondering whether it is possible to unify them?
|
|
||
| /// Supported transaction argument types | ||
| #[derive(Clone)] | ||
| pub enum TxnArgType { |
There was a problem hiding this comment.
also, support of signed integers are not here.
| hasher.update(sig.name.as_bytes()); | ||
| hasher.update(sig.ident.to_string().as_bytes()); | ||
| for generic in &sig.generics { | ||
| hasher.update(format!("{generic:?}").as_bytes()); |
There was a problem hiding this comment.
Claude mentioned that debug print here is not stable: Debug change in move-core-types silently invalidates every persisted cache. Worth double check the code here.
| found_new = true; | ||
| } | ||
| let entry = func_map.entry(pos).or_insert(0); | ||
| *entry += count; |
There was a problem hiding this comment.
for long-runs, maybe use saturating_add to avoid overflow?
| Some(v) => v, | ||
| }; | ||
| if addrs.is_empty() { | ||
| continue; |
There was a problem hiding this comment.
could you double check whether this loop can run forever?
- oneshot: reuse the already-picked seed index for sender selection instead of picking a second, unrelated seed (sender and mutation base now agree) - move-coverage: assert lowercase "script" in the TraceMap trace parser to match the VM output and the CoverageMap parser (was a latent panic) - subexec: reap the child with a single wait() in end(); yield instead of busy-spinning in probe(); bail! instead of panic! on kill/join failures - simulator: back off briefly instead of busy-spinning while waiting for the localnet to come up - language: drop language version V1 support (rejected at parse time) - fuzzer: compute persisted-cache fingerprints from stable representations (Display / raw ability bits) instead of Debug; document the seed-score constants - mutator: bound random_account_address() so it cannot loop forever when an address bucket is empty - executor: use saturating_add for per-position coverage counters - state / common: document the persisted-state schema version constants and the current TxnArgType limitations - tests: depend on AptosFramework via a local path in the prep/demo packages instead of a non-deterministic git rev = "main" - AGENT.md: drop the dangling docs/idea.pdf reference Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| .expect("valid synthetic struct identifier"), | ||
| type_args: vec![], | ||
| } | ||
| } |
There was a problem hiding this comment.
Synthetic struct tags use unstable Debug format and hasher
Medium Severity
synthetic_struct_tag hashes the Debug representation of StateKey via DefaultHasher to produce struct tag names used in the DUG and persisted ResourceTag values. DefaultHasher is explicitly documented as non-stable across Rust versions ("its hashes should not be relied upon over releases"), and Debug format of upstream StateKey can change when move-core-types is modified. Together these mean a Rust toolchain or dependency update silently changes the generated tag names, breaking DUG resource tracking and invalidating persisted campaign state. The reviewer flagged this concern ("Debug change in move-core-types silently invalidates every persisted cache"). Other fingerprinting in the crate already switched to SHA3-256 with stable serialization for exactly this reason.
Reviewed by Cursor Bugbot for commit 587c044. Configure here.
Upstream switched the source license header from the Apache-2.0 SPDX identifier to the Innovation-Enabling Source Code License. Update all new move-fuzz source files (and the CLI integration) so the insert-license pre-commit hook passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| let mut last_report = Instant::now(); | ||
| let mut last_report_coverage = 0usize; | ||
|
|
||
| loop { |
There was a problem hiding this comment.
Fuzzer main loop lacks unconditional exit mechanism
Medium Severity
The main loop at line 1274 has no unconditional timeout or maximum-iteration exit. Phase 1 only transitions to Phase 2 when all scripts go saturation_secs without new coverage. If even a single script sporadically finds coverage (e.g., due to non-determinism in gas or object addresses), the fuzzer never enters Phase 2 and runs indefinitely. Phase 2 is the only phase with an exit condition. A maximum wall-clock cap or iteration limit would prevent the loop from running forever during automated CI or batch runs.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ce81181. Configure here.
- reorder legacy-move-compiler so `cargo sort --grouped` passes - remove unused move-compiler-v2 and move-symbol-pool deps (cargo machete) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
Reviewed by Cursor Bugbot for commit 9114017. Configure here.
| let (vm_status, txn_status, resource_writes, resource_reads) = self | ||
| .executor | ||
| .run_payload_with_sender_tracking(executed_seed.sender, payload)?; | ||
| self.replay_log.push(executed_seed.clone()); |
There was a problem hiding this comment.
Unbounded replay log causes memory and restore problems
Medium Severity
Every call to run_one unconditionally pushes the executed seed into self.replay_log with no size cap. This log is fully serialized via replay_log_snapshot into PersistedOneshotFuzzer and restored by replay_checkpoint_log, which re-executes every entry. For long-running campaigns with millions of iterations across many scripts, the replay log grows without bound, causing unbounded memory consumption, massive persisted JSON state files, and restoration times proportional to total execution count rather than meaningful state.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9114017. Configure here.
Description
This PR merges the Move fuzzer back into
aptos-coreas a single rebased commit on top of currentmain.It adds
aptos move fuzz, a DUG-inspired fuzzer for Move smart contracts that:DUG in this context stands for
def-use graph, it is the data structure that models how one transaction can create on-chain state that a later transaction needs.In the current implementation, the DUG is a bipartite graph between:
A resource tag is not just a type name. It is
(storage account, StructTag), so the fuzzer distinguishes the same resource type under different accounts or objects. That is important for Move, because state identity is often “type + address/object”, not just “type”.Defedge means a script writes a resource.Useedge means a script reads a resource.The graph is built from actual execution observations, not only static signatures. move-fuzz also seeds it with resources already present in the initial state, and it records concrete execution seeds so itcan later build not only abstract chains, but also concrete transaction sequences.
The primary reason we use it for Move fuzzing is based on the observation that Move bugs are often stateful and multi-transactional. A lot of interesting contract logic is unreachable from a clean slate. A function may only do meaningful work after some earlier transaction has already created. Without that context, a one-shot fuzzer spends most of its time hitting early aborts and shallow checks.
The DUG fixes that by letting the fuzzer learn, from observed reads and writes:
That is exactly how move-fuzz moves from Phase 1 to Phase 2:
How Has This Been Tested?
Key Areas to Review
third_party/move/tools/move-fuzz/src/prep/third_party/move/tools/move-fuzz/src/deps.rsthird_party/move/tools/move-fuzz/src/package.rsthird_party/move/tools/move-fuzz/src/executor/sequence.rsthird_party/move/tools/move-fuzz/src/fuzzer.rsaptos-move/cli/src/fuzz.rsthird_party/move/tools/move-fuzz/README.mdthird_party/move/tools/move-fuzz/AGENT.mdType of Change
Which Components or Systems Does This Change Impact?
Checklist
Note
Medium Risk
Large new developer-only surface (VM-backed local fuzzing) with minor shared changes to e2e executor and coverage trace parsing; no validator or network path changes.
Overview
Introduces the
move-fuzzcrate and wires it into the monorepo asaptos move fuzz(plus standalonemove-fuzz-dev). Theautocommand runs a full pipeline: resolve/build Move packages (with persistent package and entrypoint caches under.move-fuzz), statically analyze entrypoints, generate and compile driver scripts, then fuzz on a local simulator with Phase 1 single-transaction, coverage-guided runs and Phase 2 multi-transaction chains driven by a runtime def-use graph over scripts and observed resource reads/writes.Supporting changes:
aptos-language-e2e-testsgainsduplicate_with_assumptionandget_state_deltaso fuzzing can fork executor state;move-coverageexpects script trace contexts labeledscriptinstead ofScript.Reviewed by Cursor Bugbot for commit 9114017. Bugbot is set up for automated code reviews on this repo. Configure here.