test: add roundtrip serialize/deserialize fuzz targets for Dash types#7162
test: add roundtrip serialize/deserialize fuzz targets for Dash types#7162thepastaclaw wants to merge 4 commits intodashpay:developfrom
Conversation
…or Dash types Phase 2 of the Dash fuzzing initiative. Tests that for each Dash-specific type, deserialize(serialize(deserialize(input))) produces byte-identical output, catching asymmetric serialization bugs.
WalkthroughAdds a new fuzz test source and a fuzzing implementation that performs roundtrip serialization/deserialization for many Dash-specific types. Introduces a generic DashRoundtripFromFuzzingInput routine, a testing setup initialization, a custom dash_invalid_fuzzing_input_exception, and multiple FUZZ_TARGET_DASH_ROUNDTRIP registrations. Clears mnSkipList before unserializing in CQuorumSnapshot::Unserialize. Updates Makefile.test.include and test/util/data/non-backported.txt to include the new fuzz source. Sequence Diagram(s)sequenceDiagram
participant Fuzzer
participant Setup as BasicTestingSetup
participant Stream as FuzzingStream
participant Deser as Deserializer
participant Obj as Object(T)
participant Ser as Serializer
participant Comparator as Comparator
Fuzzer->>Setup: initialize_roundtrip_dash()
Fuzzer->>Stream: provide fuzz buffer
Stream->>Deser: read version
Deser->>Obj: unserialize from Stream
Deser-->>Fuzzer: throw dash_invalid_fuzzing_input_exception on failure
Obj->>Ser: serialize to bytes
Ser->>Deser: deserialize bytes into Obj'
Deser->>Comparator: compare original bytes vs reserialized bytes
Comparator-->>Fuzzer: assert equal / report mismatch
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/fuzz/roundtrip_dash.cpp (2)
76-76: Inconsistent span construction — preferMakeByteSpanuniformlyLine 76 uses
Span<const std::byte>{ds2}while line 86 usesMakeByteSpan(ds2). Both are equivalent here (sincenReadPos == 0on a freshly-written stream), but mixing idioms is inconsistent.🔄 Use `MakeByteSpan` consistently
- CDataStream ds3(Span<const std::byte>{ds2}, SER_NETWORK, ds.GetVersion()); + CDataStream ds3(MakeByteSpan(ds2), SER_NETWORK, ds.GetVersion());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/roundtrip_dash.cpp` at line 76, Replace the direct Span construction with the helper to keep idioms consistent: change the CDataStream construction that currently uses Span<const std::byte>{ds2} to use MakeByteSpan(ds2) instead so both CDataStream creations use MakeByteSpan and the code uniformly handles byte spans (refer to the CDataStream constructor call and the ds2 variable).
49-49:g_setupis set but never used — dead code
g_setupis assigned ininitialize_roundtrip_dash()but no fuzz target reads it. The static localtesting_setupinsideinitialize_roundtrip_dashstill performs the necessary global-context initialization, sog_setupitself is redundant.🧹 Remove the unused pointer
-const BasicTestingSetup* g_setup; - struct dash_invalid_fuzzing_input_exception : public std::exception {void initialize_roundtrip_dash() { static const auto testing_setup = MakeNoLogFileContext<>(); - g_setup = testing_setup.get(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/roundtrip_dash.cpp` at line 49, g_setup is an unused global pointer left over from initialization; remove the dead variable and its assignment to avoid unused-state confusion. Delete the declaration "const BasicTestingSetup* g_setup;" and remove any assignments to g_setup in initialize_roundtrip_dash(), leaving the static local testing_setup to perform the global initialization; ensure no other code references g_setup after removal and run a build to confirm no remaining uses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/fuzz/roundtrip_dash.cpp`:
- Around line 1-3: Add the missing entry for roundtrip_dash.cpp to the
non-backported list: open test/util/data/non-backported.txt and insert the line
"src/test/fuzz/roundtrip_dash.cpp" into the alphabetical ordering, placing it
after the existing src/test/evo*.cpp entries so the file is included for
clang-format enforcement.
---
Nitpick comments:
In `@src/test/fuzz/roundtrip_dash.cpp`:
- Line 76: Replace the direct Span construction with the helper to keep idioms
consistent: change the CDataStream construction that currently uses Span<const
std::byte>{ds2} to use MakeByteSpan(ds2) instead so both CDataStream creations
use MakeByteSpan and the code uniformly handles byte spans (refer to the
CDataStream constructor call and the ds2 variable).
- Line 49: g_setup is an unused global pointer left over from initialization;
remove the dead variable and its assignment to avoid unused-state confusion.
Delete the declaration "const BasicTestingSetup* g_setup;" and remove any
assignments to g_setup in initialize_roundtrip_dash(), leaving the static local
testing_setup to perform the global initialization; ensure no other code
references g_setup after removal and run a build to confirm no remaining uses.
| // Copyright (c) 2026 The Dash Core developers | ||
| // Distributed under the MIT software license, see the accompanying | ||
| // file COPYING or http://www.opensource.org/licenses/mit-license.php. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if roundtrip_dash.cpp exists
echo "=== Checking if roundtrip_dash.cpp exists ==="
find . -name "roundtrip_dash.cpp" -type f 2>/dev/null
# Find and check the non-backported.txt file
echo -e "\n=== Locating non-backported.txt ==="
find . -path "*/test/util/data/non-backported.txt" -type f 2>/dev/null
# Check if roundtrip_dash is in non-backported.txt
echo -e "\n=== Checking if roundtrip_dash is in non-backported.txt ==="
grep -n "roundtrip_dash" test/util/data/non-backported.txt 2>/dev/null || echo "Not found"
# Show entries around 'roundtrip' and 'fuzz' for context
echo -e "\n=== Entries containing 'roundtrip' or 'fuzz' in non-backported.txt ==="
grep -n -E "roundtrip|fuzz" test/util/data/non-backported.txt 2>/dev/null || echo "None found"
# Show a sample of the file to understand format
echo -e "\n=== Sample entries from non-backported.txt (first 20 lines) ==="
head -20 test/util/data/non-backported.txt 2>/dev/null || echo "File not accessible"Repository: dashpay/dash
Length of output: 782
🏁 Script executed:
#!/bin/bash
# Show entries related to src/test
echo "=== Entries starting with 'src/test' ==="
grep -n "^src/test" test/util/data/non-backported.txt | head -20
# Count total lines
echo -e "\n=== Total lines in non-backported.txt ==="
wc -l test/util/data/non-backported.txt
# Show the entire file to understand complete context
echo -e "\n=== Full content of non-backported.txt ==="
cat test/util/data/non-backported.txtRepository: dashpay/dash
Length of output: 1977
Add src/test/fuzz/roundtrip_dash.cpp to test/util/data/non-backported.txt
This Dash-specific .cpp file is not currently listed in test/util/data/non-backported.txt, which is required for clang-format enforcement. Insert src/test/fuzz/roundtrip_dash.cpp alphabetically after src/test/evo*.cpp (line 57).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/fuzz/roundtrip_dash.cpp` around lines 1 - 3, Add the missing entry
for roundtrip_dash.cpp to the non-backported list: open
test/util/data/non-backported.txt and insert the line
"src/test/fuzz/roundtrip_dash.cpp" into the alphabetical ordering, placing it
after the existing src/test/evo*.cpp entries so the file is included for
clang-format enforcement.
|
Note: This PR is a companion to #7161 (deserialization fuzz targets) but not dependent on it. Both add new fuzz target files independently. If #7161 merges first, this will need a trivial rebase of This is Phase 2 of the Dash fuzzing initiative (tracked in thepastaclaw/tracker#108). |
thepastaclaw
changed the title
|
Phase 2 validation update: 35/36 roundtrip targets pass, 1 crashes (). I marked this PR draft while I triage/fix the failure. Tracking fix in thepastaclaw/tracker#128. |
|
Correction: failed target is |
…ading CQuorumSnapshot::Unserialize used push_back to append entries to mnSkipList without clearing it first. This means deserializing into a non-empty CQuorumSnapshot object would corrupt mnSkipList by doubling its contents. While ReadFixedBitSet properly replaces activeQuorumMembers via resize(), mnSkipList had no such clearing, creating a serialization asymmetry: serialize(deserialize(data)) != data when the target object is pre-populated. Found by the dash_quorum_snapshot_roundtrip fuzz target.
|
Fuzz validation complete after CQuorumSnapshot fix: 36/36 roundtrip targets pass clean (60s each, ASan+UBSan+libFuzzer on aarch64 Linux). |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/llmq/snapshot.h (1)
187-206:⚠️ Potential issue | 🟠 Major
CQuorumRotationInfo::Unserializehas the same accumulation bug just fixed inCQuorumSnapshot.All three vectors are appended via
push_backwithout a precedingclear():
lastCommitmentPerIndex(line 191)quorumSnapshotList(line 198)mnListDiffList(line 205)Re-deserializing into a non-empty
CQuorumRotationInfo(exactly what the roundtrip fuzz harness does) will cause each vector to double, producing the sameserialize(deserialize(data)) != dataasymmetry that was just fixed inCQuorumSnapshot. The fuzz target forCQuorumRotationInforoundtrip will likely reproduce this.🐛 Proposed fix — clear all three vectors before reading
size_t cnt = ReadCompactSize(s); + lastCommitmentPerIndex.clear(); for ([[maybe_unused]] const auto _ : irange::range(cnt)) { CFinalCommitment qc; ::Unserialize(s, qc); lastCommitmentPerIndex.push_back(std::move(qc)); } cnt = ReadCompactSize(s); + quorumSnapshotList.clear(); for ([[maybe_unused]] const auto _ : irange::range(cnt)) { CQuorumSnapshot snap; ::Unserialize(s, snap); quorumSnapshotList.push_back(std::move(snap)); } cnt = ReadCompactSize(s); + mnListDiffList.clear(); for ([[maybe_unused]] const auto _ : irange::range(cnt)) { CSimplifiedMNListDiff mnlist; ::Unserialize(s, mnlist); mnListDiffList.push_back(std::move(mnlist)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llmq/snapshot.h` around lines 187 - 206, In CQuorumRotationInfo::Unserialize the vectors lastCommitmentPerIndex, quorumSnapshotList, and mnListDiffList are appended to during deserialization; before reading each compact-size loop in CQuorumRotationInfo::Unserialize call lastCommitmentPerIndex.clear(), quorumSnapshotList.clear(), and mnListDiffList.clear() respectively so each push_back populates fresh vectors and prevents doubling when deserializing into an existing object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/llmq/snapshot.h`:
- Around line 187-206: In CQuorumRotationInfo::Unserialize the vectors
lastCommitmentPerIndex, quorumSnapshotList, and mnListDiffList are appended to
during deserialization; before reading each compact-size loop in
CQuorumRotationInfo::Unserialize call lastCommitmentPerIndex.clear(),
quorumSnapshotList.clear(), and mnListDiffList.clear() respectively so each
push_back populates fresh vectors and prevents doubling when deserializing into
an existing object.
Required for clang-format CI enforcement on Dash-specific files.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/util/data/non-backported.txt (1)
53-53: LGTM — entry is correctly a.cppfile for a new non-backported Dash target.Minor nit: the
src/test/block in this file is roughly alphabetical, andfuzz/roundtrip_dash.cpp(f) lands between twob-prefixed entries. It would read more consistently if moved aftersrc/test/evo*.cpp(line 58), i.e. between theevoandgovernancegroups.🔧 Suggested re-ordering
-src/test/block_reward_reallocation_tests.cpp -src/test/fuzz/roundtrip_dash.cpp -src/test/bls_tests.cpp +src/test/block_reward_reallocation_tests.cpp +src/test/bls_tests.cpp…and insert later:
src/test/evo*.cpp +src/test/fuzz/roundtrip_dash.cpp src/test/llmq*.cppBased on learnings:
test/util/data/non-backported.txtmust include only.cpp/.hfiles (satisfied), and new non-backported files must be added to this list (satisfied).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/util/data/non-backported.txt` at line 53, The list entry for the new non-backported test file is out of alphabetical order: move the entry "roundtrip_dash.cpp" so it appears after the existing evo*.cpp entries and before the governance group in the src/test block of non-backported.txt to keep the section alphabetically consistent; ensure you only relocate the single line (don’t change file extension or other entries) so the file remains listed and still under the .cpp/.h-only rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/util/data/non-backported.txt`:
- Line 53: The list entry for the new non-backported test file is out of
alphabetical order: move the entry "roundtrip_dash.cpp" so it appears after the
existing evo*.cpp entries and before the governance group in the src/test block
of non-backported.txt to keep the section alphabetically consistent; ensure you
only relocate the single line (don’t change file extension or other entries) so
the file remains listed and still under the .cpp/.h-only rule.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/test/fuzz/roundtrip_dash.cpp (2)
75-75: Copy-constructingobj2fromobjweakens the roundtrip invariant.
T obj2 = objseedsobj2with the already-deserialized state beforeds3 >> obj2overwrites it. If anyUnserializeimplementation appends to (rather than resets) a container field, both serializations would still match becauseobj2started as a valid copy. A freshly default-constructedobj2would be a stricter probe of that invariant.This matters only for the explicit-
T&overload; the zero-argument overload (line 92) already starts from a default-constructed object.♻️ Proposed fix (default-constructible types only)
- T obj2 = obj; - CDataStream ds3(Span<const std::byte>{ds2}, SER_NETWORK, ds.GetVersion()); + T obj2{}; + CDataStream ds3(Span<const std::byte>{ds2}, SER_NETWORK, ds.GetVersion());For non-default-constructible types (e.g.
CDeterministicMN) the copy-construct path must be retained, so aif constexpr (std::is_default_constructible_v<T>)guard or a dedicated overload can be used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/roundtrip_dash.cpp` at line 75, The copy-construction T obj2 = obj weakens the roundtrip test because obj2 inherits deserialized state before ds3 >> obj2; change to start obj2 default-constructed when possible: in the function in roundtrip_dash.cpp, replace the copy-path with a conditional that does if constexpr (std::is_default_constructible_v<T>) { T obj2{}; ds3 >> obj2; } else { T obj2 = obj; ds3 >> obj2; } (or add a dedicated overload) so that default-constructible types get a fresh object and non-default-constructible types (e.g. CDeterministicMN) keep the copy-construct fallback; ensure references to ds3 >> obj2 and Unserialize remain unchanged.
49-50:g_setupis written but never read.The raw pointer set via
g_setup = testing_setup.get()(line 101) is never dereferenced anywhere in this file. Thestaticlocal ininitialize_roundtrip_dashis what drives initialization; storing the pointer ing_setupis redundant. Consider removing the global or, if kept for future use, annotating with a[[maybe_unused]]attribute to suppress potential warnings.♻️ Proposed cleanup
-const BasicTestingSetup* g_setup;void initialize_roundtrip_dash() { static const auto testing_setup = MakeNoLogFileContext<>(); - g_setup = testing_setup.get(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/roundtrip_dash.cpp` around lines 49 - 50, The global pointer g_setup is assigned from testing_setup.get() but never read; remove the redundant global declaration and the assignment in initialize_roundtrip_dash (references: g_setup and testing_setup.get()), or if you prefer to keep it for future use annotate the declaration with [[maybe_unused]] to suppress warnings; ensure any removed assignment/variable has no other uses and leave the static local in initialize_roundtrip_dash as the sole initializer driving setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/test/fuzz/roundtrip_dash.cpp`:
- Around line 1-3: The PR added src/test/fuzz/roundtrip_dash.cpp to
test/util/data/non-backported.txt but that entry is already present; locate the
non-backported.txt list (the entry referencing roundtrip_dash.cpp) and remove
the duplicate line so each file appears only once, ensuring the unique
identifier "src/test/fuzz/roundtrip_dash.cpp" is not listed twice.
---
Nitpick comments:
In `@src/test/fuzz/roundtrip_dash.cpp`:
- Line 75: The copy-construction T obj2 = obj weakens the roundtrip test because
obj2 inherits deserialized state before ds3 >> obj2; change to start obj2
default-constructed when possible: in the function in roundtrip_dash.cpp,
replace the copy-path with a conditional that does if constexpr
(std::is_default_constructible_v<T>) { T obj2{}; ds3 >> obj2; } else { T obj2 =
obj; ds3 >> obj2; } (or add a dedicated overload) so that default-constructible
types get a fresh object and non-default-constructible types (e.g.
CDeterministicMN) keep the copy-construct fallback; ensure references to ds3 >>
obj2 and Unserialize remain unchanged.
- Around line 49-50: The global pointer g_setup is assigned from
testing_setup.get() but never read; remove the redundant global declaration and
the assignment in initialize_roundtrip_dash (references: g_setup and
testing_setup.get()), or if you prefer to keep it for future use annotate the
declaration with [[maybe_unused]] to suppress warnings; ensure any removed
assignment/variable has no other uses and leave the static local in
initialize_roundtrip_dash as the sole initializer driving setup.
1 participant
Summary
Add 37 roundtrip serialize/deserialize fuzz targets for Dash-specific types. This is Phase 2 of the Dash Core Fuzzing Initiative.
Motivation
All 106 existing fuzz targets in Dash Core are inherited from Bitcoin Core. None cover Dash-specific code paths (~24,000 lines in evo/, llmq/, governance/, coinjoin/, bls/). Phase 1 (PR #7161) added deserialization-only targets. This PR adds roundtrip consistency targets that verify:
This catches serialization asymmetries, field truncation, and version-dependent encoding bugs.
What's included
37 new fuzz targets covering:
Special handling
CDeterministicMN(0)(non-default-constructible)Testing
make -C src/test/fuzz -j$(nproc)— all targets compilePart of
Validation
What was tested:
make -C src/test/fuzz -j$(nproc)— compiled all 37 new roundtrip fuzz targetsResults:
linux64-build / Build source— passlinux64-test / Test source— passlinux64_nowallet-build— pass;linux64_nowallet-test— passlinux64_sqlite-build— pass;linux64_sqlite-test— passlinux64_tsan-build— pass;linux64_tsan-test— passlinux64_ubsan-build— pass;linux64_ubsan-test— passmac-build / Build source— passwin64-build / Build source— passEnvironment: Local macOS arm64 (fuzz build); GitHub Actions CI (linux64_fuzz + full matrix)