test: add special transaction validation fuzz target#7169
test: add special transaction validation fuzz target#7169thepastaclaw wants to merge 6 commits intodashpay:developfrom
Conversation
|
WalkthroughAdds a new fuzz test target for special transaction validation. The Makefile test include registers test/fuzz/special_tx_validation.cpp. The fuzz file initializes a regtest context, constructs or deserializes CMutableTransaction candidates from fuzz input, iterates over selected SPECIAL_TYPES (optionally setting vExtraPayload), acquires cs_main, obtains the previous block index and coins view, calls the special_tx helper's CheckSpecialTx for each transaction, and compares the result with the fuzz State's IsValid(). Sequence Diagram(s)sequenceDiagram
participant Fuzzer
participant CandidateTx
participant SpecialTxHelper
participant cs_main_lock
participant Chainstate_CoinsView
Fuzzer->>CandidateTx: DeserializeCandidateTx(fuzz_data) / fallback build
Fuzzer->>Fuzzer: choose subset of SPECIAL_TYPES
Fuzzer->>CandidateTx: clone & set type, maybe set vExtraPayload
Fuzzer->>cs_main_lock: acquire
cs_main_lock->>Chainstate_CoinsView: get prev block index & coins view
Fuzzer->>SpecialTxHelper: CheckSpecialTx(tx, prev_block, coins_view, state)
SpecialTxHelper-->>Fuzzer: returns acceptance result
Fuzzer->>Fuzzer: compare acceptance result with state.IsValid()
cs_main_lock->>Fuzzer: release
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 (3)
src/Makefile.test.include (1)
365-366: Alphabetical ordering:special_tx_validation.cppshould followsocks5.cpp.
"so..."('o' = 111) sorts before"sp..."('p' = 112), so the entry belongs aftersocks5.cpp.🔧 Proposed fix
test/fuzz/signature_checker.cpp \ -test/fuzz/special_tx_validation.cpp \ test/fuzz/socks5.cpp \ test/fuzz/span.cpp \ test/fuzz/spanparsing.cpp \ +test/fuzz/special_tx_validation.cpp \ test/fuzz/string.cpp \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Makefile.test.include` around lines 365 - 366, The two entries in the test list are out of alphabetical order; move test/fuzz/special_tx_validation.cpp so it comes after test/fuzz/socks5.cpp in the file (swap their order in the list) to ensure the test filenames are sorted lexicographically (reference entries: test/fuzz/socks5.cpp and test/fuzz/special_tx_validation.cpp).src/test/fuzz/special_tx_validation.cpp (2)
50-56: Fallback:nVersionandnTypeassignments consume fuzzer entropy but are always overridden.In the fallback construction path,
nVersion(line 51, 1 byte) andnType(line 52, 2 bytes) are set, but theFUZZ_TARGETunconditionally overwrites both at lines 89–90 (mut_tx.nVersion = CTransaction::SPECIAL_VERSIONandmut_tx.nType = SPECIAL_TYPES[i]). OnlynLockTimeandvExtraPayloadfrom the fallback path actually survive into validation. Removing the two dead assignments preserves those 3 bytes of entropy for more useful downstream consumption.♻️ Proposed fix
if (!deserialized) { - tx.nVersion = fuzzed_data_provider.ConsumeBool() ? CTransaction::CURRENT_VERSION : CTransaction::SPECIAL_VERSION; - tx.nType = fuzzed_data_provider.ConsumeIntegral<uint16_t>(); tx.nLockTime = fuzzed_data_provider.ConsumeIntegral<uint32_t>(); tx.vExtraPayload = fuzzed_data_provider.ConsumeBytes<uint8_t>( fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 256)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/special_tx_validation.cpp` around lines 50 - 56, The fallback branch in special_tx_validation.cpp assigns tx.nVersion and tx.nType using fuzzed_data_provider but those fields are later unconditionally overwritten by FUZZ_TARGET (mut_tx.nVersion = CTransaction::SPECIAL_VERSION and mut_tx.nType = SPECIAL_TYPES[i]), wasting 3 bytes of entropy; remove the assignments to tx.nVersion and tx.nType in the if (!deserialized) block so only tx.nLockTime and tx.vExtraPayload are consumed there and the fuzzer entropy is preserved for other uses (leave the nLockTime and vExtraPayload consumption intact and do not change FUZZ_TARGET logic).
86-101: Sequential type selection skews coverage toward earlierSPECIAL_TYPESentries.
iterationsdrives both the loop count and the index, soTRANSACTION_ASSET_UNLOCK(index 8) is only exercised wheniterations == 9, whileTRANSACTION_PROVIDER_REGISTER(index 0) is always exercised. Beyond the index imbalance, the type tested at positioniis never randomized — the sameiterationsvalue always produces the same prefix of types with no opportunity for inter-type combinations.
FuzzedDataProvider::PickValueInArrayworks directly withstd::array, randomly selecting an element viaConsumeIntegralInRange<size_t>(0, size - 1). Using it per iteration gives every type equal reachability regardless ofiterations, and allows same-type repetitions that may expose state-dependent bugs.♻️ Proposed refactor
-const auto iterations = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, SPECIAL_TYPES.size()); +const auto iterations = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, SPECIAL_TYPES.size() * 2); for (size_t i = 0; i < iterations; ++i) { CMutableTransaction mut_tx{base_candidate}; mut_tx.nVersion = CTransaction::SPECIAL_VERSION; - mut_tx.nType = SPECIAL_TYPES[i]; + mut_tx.nType = fuzzed_data_provider.PickValueInArray(SPECIAL_TYPES);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/special_tx_validation.cpp` around lines 86 - 101, The loop currently selects types deterministically via SPECIAL_TYPES[i], biasing early entries; change the per-iteration selection to randomly pick from SPECIAL_TYPES using the fuzzed_data_provider's PickValueInArray (e.g., mut_tx.nType = fuzzed_data_provider.PickValueInArray(SPECIAL_TYPES)) so each iteration chooses an independent, uniformly random type (allowing repeats) while keeping iterations computed the same way; update the loop body where mut_tx.nType is set (and any related index usage) and keep special_tx->CheckSpecialTx call and state handling unchanged.
🤖 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/special_tx_validation.cpp`:
- Around line 28-29: The fuzz target initializes TestingSetup via
MakeNoLogFileContext with activation heights at 2 but never advances the chain,
so CheckSpecialTx receives pindex_prev at genesis and DeploymentActiveAfter
remains false; fix this by mining at least two blocks immediately after the
TestingSetup/MakeNoLogFileContext initialization (before entering the fuzz loop)
so pindex_prev points past the activation height and the special-tx validation
paths are exercised — add block-mining calls analogous to other fuzz targets
(e.g., process_messages.cpp / tx_pool.cpp) right after TestingSetup creation and
before calling CheckSpecialTx.
---
Nitpick comments:
In `@src/Makefile.test.include`:
- Around line 365-366: The two entries in the test list are out of alphabetical
order; move test/fuzz/special_tx_validation.cpp so it comes after
test/fuzz/socks5.cpp in the file (swap their order in the list) to ensure the
test filenames are sorted lexicographically (reference entries:
test/fuzz/socks5.cpp and test/fuzz/special_tx_validation.cpp).
In `@src/test/fuzz/special_tx_validation.cpp`:
- Around line 50-56: The fallback branch in special_tx_validation.cpp assigns
tx.nVersion and tx.nType using fuzzed_data_provider but those fields are later
unconditionally overwritten by FUZZ_TARGET (mut_tx.nVersion =
CTransaction::SPECIAL_VERSION and mut_tx.nType = SPECIAL_TYPES[i]), wasting 3
bytes of entropy; remove the assignments to tx.nVersion and tx.nType in the if
(!deserialized) block so only tx.nLockTime and tx.vExtraPayload are consumed
there and the fuzzer entropy is preserved for other uses (leave the nLockTime
and vExtraPayload consumption intact and do not change FUZZ_TARGET logic).
- Around line 86-101: The loop currently selects types deterministically via
SPECIAL_TYPES[i], biasing early entries; change the per-iteration selection to
randomly pick from SPECIAL_TYPES using the fuzzed_data_provider's
PickValueInArray (e.g., mut_tx.nType =
fuzzed_data_provider.PickValueInArray(SPECIAL_TYPES)) so each iteration chooses
an independent, uniformly random type (allowing repeats) while keeping
iterations computed the same way; update the loop body where mut_tx.nType is set
(and any related index usage) and keep special_tx->CheckSpecialTx call and state
handling unchanged.
thepastaclaw
changed the title
… target The fuzz target configured activations at height 2 but never advanced the chain past genesis, causing CheckSpecialTx to return early without exercising validation paths. Mine 3 blocks in init to ensure DIP3, v20, and mn_rr are active when the fuzz loop runs.
Required for clang-format CI enforcement on Dash-specific files.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/test/fuzz/special_tx_validation.cpp (2)
46-53: Optional: reconsider the leading protocol-version prefix inDeserializeCandidateTx.Reading a raw
nVersionint from the stream and using it as the stream's serialization version means the fuzzer's input format is[4-byte proto version][raw transaction bytes]. This wastes 4 bytes of entropy on the stream version and forces the fuzzer engine to learn the implicit two-part layout before it can generate valid transactions. A simpler approach — pinning the stream version toPROTOCOL_VERSIONand deserializingtxdirectly — would leave all fuzz bytes available for the transaction payload itself while still exercising the deserialization paths that matter.🔧 Suggested simplification
- try { - int nVersion; - ds >> nVersion; - ds.SetVersion(nVersion); - ds >> tx; - deserialized = true; - } catch (const std::ios_base::failure&) { - } + try { + ds >> tx; + deserialized = true; + } catch (const std::ios_base::failure&) { + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/special_tx_validation.cpp` around lines 46 - 53, The test currently consumes a 4-byte nVersion from the fuzz input and calls ds.SetVersion(nVersion) before deserializing tx, which wastes entropy; remove the ds >> nVersion and ds.SetVersion(nVersion) steps and instead construct CDataStream with SER_NETWORK and PROTOCOL_VERSION (pinning the stream version) and then deserialize tx directly (keep ds >> tx) so all fuzz bytes are used for the transaction payload while preserving DeserializeCandidateTx behavior.
94-98: Sequential type selection creates coverage bias acrossSPECIAL_TYPES.
SPECIAL_TYPES[i]is always accessed in order starting from index 0, soTRANSACTION_PROVIDER_REGISTER(index 0) is exercised in every fuzzing run, whileTRANSACTION_ASSET_UNLOCK(index 8) is only reached wheniterations == 9— roughly 1-in-9 of randomly sampled iteration counts. Types deeper in the array receive disproportionately less fuzzing pressure, especially early in a fuzzing campaign before the corpus matures.A per-iteration random type pick gives uniform coverage from the start:
♻️ Proposed refactor for uniform type coverage
- const auto iterations = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, SPECIAL_TYPES.size()); + const auto iterations = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, SPECIAL_TYPES.size() * 2); for (size_t i = 0; i < iterations; ++i) { CMutableTransaction mut_tx{base_candidate}; mut_tx.nVersion = CTransaction::SPECIAL_VERSION; - mut_tx.nType = SPECIAL_TYPES[i]; + mut_tx.nType = SPECIAL_TYPES[fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, SPECIAL_TYPES.size() - 1)];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/special_tx_validation.cpp` around lines 94 - 98, The loop currently selects types sequentially via SPECIAL_TYPES[i], causing index 0 to be exercised every run and biasing coverage; change the selection inside the loop to pick an index per-iteration from fuzzed_data_provider (e.g., call ConsumeIntegralInRange<size_t>(0, SPECIAL_TYPES.size()-1) each iteration) and use SPECIAL_TYPES[random_index] when assigning mut_tx.nType so each fuzz iteration uniformly samples all entries in SPECIAL_TYPES; keep existing variables (iterations, fuzzed_data_provider, mut_tx, SPECIAL_TYPES) and only replace the deterministic SPECIAL_TYPES[i] access with the per-iteration random index.
🤖 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/special_tx_validation.cpp`:
- Around line 29-39: The setup in initialize_special_tx_validation correctly
mines three blocks past the activation height and calls
SyncWithValidationInterfaceQueue to drain callbacks, so no code change is
needed; keep the MakeNoLogFileContext<const TestingSetup> usage, assignment to
g_setup, the MineBlock loop, and the SyncWithValidationInterfaceQueue() call
as-is to ensure DIP3/v20/mn_rr are active before fuzzing.
---
Nitpick comments:
In `@src/test/fuzz/special_tx_validation.cpp`:
- Around line 46-53: The test currently consumes a 4-byte nVersion from the fuzz
input and calls ds.SetVersion(nVersion) before deserializing tx, which wastes
entropy; remove the ds >> nVersion and ds.SetVersion(nVersion) steps and instead
construct CDataStream with SER_NETWORK and PROTOCOL_VERSION (pinning the stream
version) and then deserialize tx directly (keep ds >> tx) so all fuzz bytes are
used for the transaction payload while preserving DeserializeCandidateTx
behavior.
- Around line 94-98: The loop currently selects types sequentially via
SPECIAL_TYPES[i], causing index 0 to be exercised every run and biasing
coverage; change the selection inside the loop to pick an index per-iteration
from fuzzed_data_provider (e.g., call ConsumeIntegralInRange<size_t>(0,
SPECIAL_TYPES.size()-1) each iteration) and use SPECIAL_TYPES[random_index] when
assigning mut_tx.nType so each fuzz iteration uniformly samples all entries in
SPECIAL_TYPES; keep existing variables (iterations, fuzzed_data_provider,
mut_tx, SPECIAL_TYPES) and only replace the deterministic SPECIAL_TYPES[i]
access with the per-iteration random index.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/util/data/non-backported.txt`:
- Line 53: The non-backported list contains a malformed entry where two paths
are concatenated into a single token
("src/test/fuzz/special_tx_validation.cppsrc/test/bls_tests.cpp"); split that
token into two separate lines so each valid .cpp/.h path appears on its own line
(ensure "src/test/fuzz/special_tx_validation.cpp" and "src/test/bls_tests.cpp"
exist as distinct entries), remove any accidental whitespace or extra
characters, and re-run the clang-format/lint that reads non-backported.txt to
verify both files are recognized.
58d1047 to
61df133
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/test/fuzz/special_tx_validation.cpp (2)
93-97: Sequential type selection skews coverage toward low-index special types.The loop uses
idirectly as the index intoSPECIAL_TYPES, soTRANSACTION_MNHF_SIGNAL(index 6),TRANSACTION_ASSET_LOCK(index 7), andTRANSACTION_ASSET_UNLOCK(index 8) are only reachable wheniterations >= 7/8/9respectively. In practice, those iterations also get less fuzz input because earlier iterations consume bytes first. Replacing the sequential index with a random pick per iteration gives each type uniform, independent coverage.
PickValueInArrayis designed for exactly this pattern — selecting from a predefined set of values.♻️ Proposed fix — random type selection per iteration
- const auto iterations = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, SPECIAL_TYPES.size()); - for (size_t i = 0; i < iterations; ++i) { + const auto iterations = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, SPECIAL_TYPES.size()); + for (size_t i = 0; i < iterations; ++i) { CMutableTransaction mut_tx{base_candidate}; mut_tx.nVersion = CTransaction::SPECIAL_VERSION; - mut_tx.nType = SPECIAL_TYPES[i]; + mut_tx.nType = fuzzed_data_provider.PickValueInArray(SPECIAL_TYPES);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/special_tx_validation.cpp` around lines 93 - 97, The loop currently uses the loop index `i` to pick special types from `SPECIAL_TYPES`, skewing coverage toward low-index entries; change the selection inside the loop to pick a value randomly each iteration using the fuzzer helper (e.g., call `PickValueInArray(fuzzed_data_provider, SPECIAL_TYPES)` or equivalent) instead of `SPECIAL_TYPES[i]`, so each iteration creates `mut_tx.nType` from an independent random selection; keep `iterations` and the rest of the loop logic intact and only replace the index-based selection with the per-iteration random pick.
57-63: Fallback path consumes entropy fornVersionandnTypethat are always overridden.In the fallback branch,
ConsumeBool()fortx.nVersion(line 58) andConsumeIntegral<uint16_t>()fortx.nType(line 59) each consume fuzz bytes that are unconditionally clobbered by lines 96–97 of theFUZZ_TARGETbody. OnlynLockTimeandvExtraPayloadcarry through, so those two consume calls are dead entropy.♻️ Proposed fix — remove dead field assignments
if (!deserialized) { - tx.nVersion = fuzzed_data_provider.ConsumeBool() ? CTransaction::CURRENT_VERSION : CTransaction::SPECIAL_VERSION; - tx.nType = fuzzed_data_provider.ConsumeIntegral<uint16_t>(); tx.nLockTime = fuzzed_data_provider.ConsumeIntegral<uint32_t>(); tx.vExtraPayload = fuzzed_data_provider.ConsumeBytes<uint8_t>( fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 256)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/special_tx_validation.cpp` around lines 57 - 63, The fallback branch for deserialized in the fuzz setup consumes entropy into tx.nVersion and tx.nType that are later overwritten in the FUZZ_TARGET body; remove the dead consumes by deleting the tx.nVersion = fuzzed_data_provider.ConsumeBool() and tx.nType = fuzzed_data_provider.ConsumeIntegral<uint16_t>() lines (leave the tx.nLockTime and tx.vExtraPayload consumes intact) so only meaningful fields consume fuzz bytes and avoid wasting entropy.
🤖 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/special_tx_validation.cpp`:
- Around line 29-38: The block-mining change in initialize_special_tx_validation
is correct: keep the MakeNoLogFileContext<const
TestingSetup>(CBaseChainParams::REGTEST, {"-dip3params=2:2",
"-testactivationheight=v20@2", "-testactivationheight=mn_rr@2"}) setup, retain
the loop that calls MineBlock(g_setup->m_node, CScript() << OP_TRUE) three times
to advance past activation height 2, and keep SyncWithValidationInterfaceQueue()
after the loop; no further changes required to MineBlock, MakeNoLogFileContext,
or SyncWithValidationInterfaceQueue usage.
---
Nitpick comments:
In `@src/test/fuzz/special_tx_validation.cpp`:
- Around line 93-97: The loop currently uses the loop index `i` to pick special
types from `SPECIAL_TYPES`, skewing coverage toward low-index entries; change
the selection inside the loop to pick a value randomly each iteration using the
fuzzer helper (e.g., call `PickValueInArray(fuzzed_data_provider,
SPECIAL_TYPES)` or equivalent) instead of `SPECIAL_TYPES[i]`, so each iteration
creates `mut_tx.nType` from an independent random selection; keep `iterations`
and the rest of the loop logic intact and only replace the index-based selection
with the per-iteration random pick.
- Around line 57-63: The fallback branch for deserialized in the fuzz setup
consumes entropy into tx.nVersion and tx.nType that are later overwritten in the
FUZZ_TARGET body; remove the dead consumes by deleting the tx.nVersion =
fuzzed_data_provider.ConsumeBool() and tx.nType =
fuzzed_data_provider.ConsumeIntegral<uint16_t>() lines (leave the tx.nLockTime
and tx.vExtraPayload consumes intact) so only meaningful fields consume fuzz
bytes and avoid wasting entropy.
1 participant
Summary
Adds a new Dash-specific fuzz target,
special_tx_validation, that exercisesCSpecialTxProcessor::CheckSpecialTx()across all current Dash special transaction types:The target uses a
TestingSetupregtest context with early DIP3/V20/MN_RR activation heights so the special-tx validation logic is reached consistently, then feeds fuzzed candidate transactions into validation.Validation
What was tested
./configure --enable-fuzz-binary --disable-bench --without-gui --disable-wallet --disable-zmq --disable-stacktracesmake -C src -j8 test/fuzz/fuzzPRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 src/test/fuzz/fuzz | rg special_tx_validationFUZZ=special_tx_validation src/test/fuzz/fuzzResults
special_tx_validationlisted).elapsed_seconds=60runs=371Environment
~/Projects/dash/worktrees/fuzz-special-tx-validation.