SRLabs: Introduce fuzzing harness for pallet-domains #3693

R9295 · 2025-09-01T12:30:13Z

This PR introduces a fuzzing harness for pallet-domains, encoding invariants and valid user-processes when fuzzing for maximum impact.

Please note that the changes seem large due to the use of the macro to make functions public conditionally.

Code contributor checklist:

I have read, understood and followed contributing guide

immunefi-magnus · 2025-09-01T12:30:25Z

🛡️ Immunefi PR Reviews

We noticed that your project isn't set up for automatic code reviews. If you'd like this PR reviewed by the Immunefi team, you can request it manually using the link below:

🔗 Send this PR in for review

Once submitted, we'll take care of assigning a reviewer and follow up here.

jfrank-summit · 2025-09-05T00:04:41Z

@R9295 thank you for this PR! It will likely not get fully reviewed until early next week. In the meantime could you please cleanup the commit history. See our contribution guide for what we are expecting. Thanks!

teor2345

Thank you for this PR, it's an important part of our security testing.

I've made a large number of comments, but most of them are code style nitpicks (starting with Nit:). Feel free to ignore those, one of the developers can do them later.

The most important revisions are:

adding comments to new functions and structs, so reviewers know what they're meant to do
reducing the size of the PR by doing re-exports in the standard Rust way

crates/pallet-domains/Cargo.toml

crates/pallet-domains/src/fuzz_utils.rs

teor2345 · 2025-09-08T09:02:00Z

crates/pallet-domains/src/fuzz_utils.rs

+use sp_core::H256;
+use sp_domains::{DomainId, OperatorId};
+use sp_runtime::traits::One;
+


Nit: Most of the crate uses a single import block. This might require a rustfmt run after this change.

Suggested change

fuzz/staking/src/main.rs

teor2345 · 2025-09-08T09:56:31Z

fuzz/staking/src/main.rs

+                        .1;
+                    let slash_reason = match slash_reason % 2 {
+                        0 => SlashedReason::InvalidBundle(0),
+                        _ => SlashedReason::BadExecutionReceipt(H256::from([0u8; 32])),


Nit: I try to avoid wildcard matches, so the compiler will tell us that we need to update them if the surrounding code changes. What we really want is a compiler error when there is a new SlashedReason.

The standard way of doing it would be:

add an enum without fields that matches each existing SlashedReason, and converts to a dummy SlashedReason

add a function that converts a u8 to that enum

convert from the u8 to the fieldless enum to the dummy SlashedReason here

fuzz/staking/src/main.rs

jfrank-summit · 2025-09-30T14:37:47Z

Any update on this PR @R9295?

R9295 · 2025-10-06T08:31:58Z

@jfrank-summit sorry, I was OOO. On it

teor2345

This is looking good, but I still have some questions about how it works

.github/workflows/rust.yml

teor2345 · 2025-10-06T15:04:47Z

crates/pallet-domains/Cargo.toml

+fuzz = [
+    "dep:domain-pallet-executive",
+    "dep:pallet-timestamp",
+    "dep:pallet-block-fees",
+    "dep:sp-externalities",
+    "dep:sp-keystore",
+]


If the fuzz feature requires std, then please depend on the std feature here. That way you can replace all(feature = "std", feature = "fuzz") with feature = "fuzz".

This makes the cfgs easier to maintain.

teor2345 · 2025-10-06T15:09:51Z

crates/pallet-domains/src/lib.rs

+#[cfg(all(not(test), feature = "std", feature = "fuzz"))]
+pub mod tests;
+
 pub mod block_tree;
 pub mod bundle_storage_fund;
 pub mod domain_registry;
 pub mod extensions;
+#[cfg(feature = "fuzz")]
+pub mod fuzz_utils;
 pub mod migrations;
 mod nominator_position;
 pub mod runtime_registry;
 pub mod staking;
+#[cfg(feature = "fuzz")]
+pub mod staking_epoch;
+#[cfg(not(feature = "fuzz"))]


Please replace these duplicate module declarations with:

#[cfg(feature = "fuzz")] pub use module;

Only declaring modules once makes the code easier to maintain, and provides clearer compiler errors when we rename a module.

teor2345 · 2025-10-06T15:16:03Z

crates/pallet-domains/src/tests.rs

-use crate::block_tree::{BlockTreeNode, verify_execution_receipt};
-use crate::domain_registry::{DomainConfig, DomainConfigParams, DomainObject};
-use crate::runtime_registry::ScheduledRuntimeUpgrade;
-use crate::staking_epoch::do_finalize_domain_current_epoch;
+#[cfg(test)]
+use crate::block_tree::verify_execution_receipt;
+#[cfg(test)]
+use crate::domain_registry::{DomainConfig, DomainObject};
+
+#[cfg(test)]
+use crate::Config;
+#[cfg(test)]
 use crate::tests::pallet_mock_version_store::MockPreviousBundleAndExecutionReceiptVersions;
+use crate::{self as pallet_domains, BlockSlot, FungibleHoldId};
+#[cfg(test)]
 use crate::{
-    self as pallet_domains, BalanceOf, BlockSlot, BlockTree, BlockTreeNodes, BundleError, Config,
-    ConsensusBlockHash, DomainBlockNumberFor, DomainHashingFor, DomainRegistry,
-    DomainRuntimeUpgradeRecords, DomainRuntimeUpgrades, ExecutionInbox, ExecutionReceiptOf,
-    FraudProofError, FungibleHoldId, HeadDomainNumber, HeadReceiptNumber, NextDomainId,
-    OperatorConfig, RawOrigin as DomainOrigin, RuntimeRegistry, ScheduledRuntimeUpgrades,
+    BalanceOf, BlockTree, BlockTreeNodes, BundleError, ConsensusBlockHash, DomainBlockNumberFor,
+    DomainHashingFor, DomainRegistry, DomainRuntimeUpgradeRecords, DomainRuntimeUpgrades,
+    ExecutionInbox, ExecutionReceiptOf, FraudProofError, HeadDomainNumber, HeadReceiptNumber,
+    NextDomainId, OperatorConfig, OperatorId, ProofOfElection, RawOrigin as DomainOrigin,
+    RuntimeId, RuntimeRegistry, ScheduledRuntimeUpgrades, block_tree::BlockTreeNode,
+    domain_registry::DomainConfigParams, runtime_registry::ScheduledRuntimeUpgrade,
+    staking_epoch::do_finalize_domain_current_epoch,
 };
 use core::mem;
+use domain_runtime_primitives::BlockNumber as DomainBlockNumber;


There are a large number of new #[cfg(test)] in this module, which makes it difficult to maintain.

It looks like it would be easier to have two module files, with two different compilation cfg conditions:

test_support: cfg(any(test, feature = "fuzz")): all the types and functions you want to use in fuzzing, that are currently used in tests

tests: cfg(test): the existing tests (marked with #[test]) and the code that isn't used for fuzzing

Creating a new module and moving shared code into it should be a separate commit, at the start of your PR.

teor2345 · 2025-10-06T15:17:05Z

fuzz/staking/README.md

+
+Command to install ziggy:
+```
+cargo install --force ziggy cargo-afl honggfuzz grcov


Is --force needed here?

teor2345 · 2025-10-06T15:18:52Z

scripts/run-fuzzer-ci.sh

@@ -0,0 +1,3 @@
+#!/usr/bin/env bash
+sudo apt install -y protobuf-compiler binutils-dev


This line only works on a Linux distribution that uses apt packages. Please delete it.

Install instructions and commands that use sudo belong in the README, because they are only run once, and they can be different for different OS distributions or package managers.

teor2345 · 2025-10-06T15:19:53Z

scripts/run-fuzzer-ci.sh

@@ -0,0 +1,3 @@
+#!/usr/bin/env bash


The script should include the standard checks that make sure every command runs correctly:

subspace/scripts/runtime-benchmark.sh

Lines 3 to 4 in b59b800

# http://redsymbol.net/articles/unofficial-bash-strict-mode/

set -euo pipefail

teor2345 · 2025-10-06T15:25:15Z

.gitignore

+./fuzz/staking/target
+./fuzz/staking/output


Is this still required now that the crate has been moved into the workspace?

fuzz/staking/src/main.rs

staking fuzzing harness

teor2345

Thanks for this documentation!

These notes are for whoever next picks up this PR. I've also resolved any completed changes, and un-resolved anything that's still needed.

teor2345 · 2025-10-10T11:38:22Z

Cargo.toml

    "domains/test/service",
    "domains/test/utils",
    "shared/*",
+    "fuzz/staking",


Nit: alphabetical order

.github/workflows/rust.yml

scripts/run-fuzzer-ci.sh

.github/workflows/rust.yml

teor2345 · 2025-10-10T12:08:57Z

bugbot run

cursor

Comment @cursor review or bugbot run to trigger another review on this PR

.github/workflows/rust.yml

teor2345

More future change notes

.github/workflows/rust.yml

scripts/run-fuzzer-ci.sh

teor2345 · 2025-10-10T12:31:47Z

.github/workflows/rust.yml

+        run: cargo install --force ziggy cargo-afl honggfuzz grcov
+
+      - name: test fuzzer
+        run: scripts/run-fuzzer-ci.sh


This job should fail if:

there are crashes or timeouts

there are zero instances, speed, execs, or coverage

the status is not "ziggy rocking"

That way we will check and analyse any fuzzing results or run failures.

teor2345 · 2025-10-10T12:37:35Z

scripts/run-fuzzer-ci.sh

+sudo apt install -y protobuf-compiler binutils-dev
+
+cd ./fuzz/staking && cargo ziggy build --no-honggfuzz
+cargo ziggy fuzz --timeout 600 --release


The timeout should be a parameter (with a default of 5 minutes?) for CI. Making it a parameter lets developers run the script outside CI for long fuzzing sessions.

We can drop the -ci from the script name when we make this change.

teor2345 · 2025-10-14T10:08:18Z

scripts/run-fuzzer-ci.sh

+
+cd ./fuzz/staking && cargo ziggy build --no-honggfuzz
+# cargo ziggy fuzz doesn't allow us to set a number of runs or a run time limit
+timeout 5m cargo ziggy fuzz --release


Suggested change

timeout 5m cargo ziggy fuzz --release

timeout --preserve-status 5m cargo ziggy fuzz --release

Currently this exits with status 143 due to the SIGTERM sent by timeout.

The script should succeed if the exit status is 143, but fail on any other status (zero indicates the fuzzer has changed behaviour or there is a bug that caused it to exit early, so we should fail on that).

Unless there are plans to add an iteration or time limit to the fuzzer itself?

R9295 requested review from nazar-pc and vedhavyas as code owners September 1, 2025 12:30

jfrank-summit requested a review from teor2345 September 4, 2025 13:51

vanhauser-thc assigned teor2345 Sep 6, 2025

teor2345 reviewed Sep 8, 2025

View reviewed changes

teor2345 changed the title ~~SRLabs: Intrdouce fuzzing harness for pallet-domains~~ SRLabs: Introduce fuzzing harness for pallet-domains Sep 8, 2025

This comment was marked as outdated.

Sign in to view

R9295 force-pushed the main branch 2 times, most recently from 01b42f5 to 20420d7 Compare October 6, 2025 13:13

SRLabs: introduce staking harness

96d7831

R9295 force-pushed the main branch from 20420d7 to 96d7831 Compare October 6, 2025 13:13

Merge branch 'main' into main

23065d3

teor2345 reviewed Oct 6, 2025

View reviewed changes

fuzz/staking/src/main.rs Show resolved Hide resolved

R9295 added 3 commits October 10, 2025 10:22

Add deactivate_operator and reactivate_operator extrinsics to the

361620f

staking fuzzing harness

add documentation for the staking fuzzing harness

8000bd6

run the fuzzer in the CI to make sure it actually work

4d5d7f5

R9295 force-pushed the main branch 2 times, most recently from 3393a26 to 4f25c3f Compare October 10, 2025 09:17

fix clippy issues in fuzz_utils

1b0a25c

R9295 force-pushed the main branch from 4f25c3f to 1b0a25c Compare October 10, 2025 10:38

fix fuzzer CI script to use AFL_SKIP_CPUFREQ and set pipefail

8e62bf9

teor2345 reviewed Oct 10, 2025

View reviewed changes

This comment was marked as off-topic.

Sign in to view

Update fuzz settings so it actually runs in CI

f494fad

teor2345 reviewed Oct 10, 2025

View reviewed changes

.github/workflows/rust.yml Outdated Show resolved Hide resolved

Revert runs-on server fuzzer change due to missing packages

a890b10

cursor bot reviewed Oct 10, 2025

View reviewed changes

.github/workflows/rust.yml Outdated Show resolved Hide resolved

teor2345 previously approved these changes Oct 10, 2025

View reviewed changes

Apply suggestions

dd50feb

teor2345 dismissed their stale review via dd50feb October 10, 2025 12:38

teor2345 reviewed Oct 14, 2025

View reviewed changes

Preserve fuzzer exit status in scripts/run-fuzzer-ci.sh

d0a0dc3

		@@ -0,0 +1,3 @@
		#!/usr/bin/env bash
		sudo apt install -y protobuf-compiler binutils-dev

	# http://redsymbol.net/articles/unofficial-bash-strict-mode/
	set -euo pipefail

	timeout 5m cargo ziggy fuzz --release
	timeout --preserve-status 5m cargo ziggy fuzz --release

		./fuzz/staking/target
		./fuzz/staking/output

SRLabs: Introduce fuzzing harness for pallet-domains #3693

Are you sure you want to change the base?

SRLabs: Introduce fuzzing harness for pallet-domains #3693

Uh oh!

Conversation

R9295 commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code contributor checklist:

Uh oh!

immunefi-magnus bot commented Sep 1, 2025

🛡️ Immunefi PR Reviews

Uh oh!

jfrank-summit commented Sep 5, 2025

Uh oh!

teor2345 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jfrank-summit commented Sep 30, 2025

Uh oh!

R9295 commented Oct 6, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

teor2345 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

teor2345 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as off-topic.

Uh oh!

Uh oh!

teor2345 commented Oct 10, 2025

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

teor2345 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

R9295 commented Sep 1, 2025 •

edited

Loading