test: add governance proposal validator fuzz target#7166
Closed
thepastaclaw wants to merge 5 commits intodashpay:developfrom
Closed
test: add governance proposal validator fuzz target#7166thepastaclaw wants to merge 5 commits intodashpay:developfrom
thepastaclaw wants to merge 5 commits intodashpay:developfrom
Conversation
|
WalkthroughAdds a new C++ fuzz test target, Sequence DiagramsequenceDiagram
participant FuzzEngine as Fuzz Engine
participant TestInit as Test Initialization
participant InputBuilder as Input Builder
participant Validator as Governance Proposal Validator
participant ErrorHandler as Error Handler
FuzzEngine->>TestInit: Start fuzz target
TestInit->>TestInit: initialize_governance_proposal_validator() (select mainnet params)
TestInit->>InputBuilder: Prepare presets (addresses, URLs, ranges)
loop For each fuzz iteration
InputBuilder->>InputBuilder: Generate fields (type,name,epochs,payment,address,URL)
InputBuilder->>InputBuilder: Create JSON variants (valid, malformed, oversized, random)
InputBuilder->>InputBuilder: Hex-encode payloads
InputBuilder->>FuzzEngine: Supply encoded payloads
loop For each flag combination (allow_script, check_expiration)
FuzzEngine->>Validator: Validate(payload, flags)
Validator->>Validator: Parse and validate JSON proposal
alt valid
Validator-->>FuzzEngine: Return success/result
else invalid/malformed
Validator-->>ErrorHandler: Throw/propagate error
ErrorHandler-->>FuzzEngine: Return error message
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
🤖 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/governance_proposal_validator.cpp`:
- Around line 90-91: Clamp or safely compute end_epoch to avoid signed overflow
when adding the offset from fuzzed_data_provider: read the offset via
ConsumeIntegralInRange<int64_t> into a variable, then compute end_epoch from
start_epoch using a saturated/checked addition (e.g., if start_epoch >
std::numeric_limits<int64_t>::max() - offset set end_epoch =
std::numeric_limits<int64_t>::max(); if start_epoch <
std::numeric_limits<int64_t>::min() - offset set end_epoch =
std::numeric_limits<int64_t>::min(); else end_epoch = start_epoch + offset).
Apply this logic around the existing use of fuzzed_data_provider, start_epoch
and end_epoch so the harness itself cannot trigger signed integer UB.
This was referenced Feb 22, 2026
Address CodeRabbit feedback: adding offset [-4, 1024] to an unconstrained int64_t start_epoch can overflow, which is UB under UBSan. Clamp to safe range and add explicit <limits> include.
Required for clang-format CI enforcement on Dash-specific files.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/fuzz/governance_proposal_validator.cpp (1)
19-29: Consider using the existingHexStrutility fromutil/strencodings.hinstead of the custom implementation.The Dash codebase provides
HexStr()which converts byte spans to hex strings. Other fuzz tests (e.g.,rpc.cpp,decode_tx.cpp) use this utility for consistency. To use it here, add#include <util/strencodings.h>and replace theHexEncodeStringfunction calls withHexStr().The current custom implementation is correct — the nibble extraction logic (
ch >> 4for high nibble,ch & 0x0ffor low nibble) is sound.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/governance_proposal_validator.cpp` around lines 19 - 29, Replace the custom HexEncodeString implementation with the project's standard HexStr utility: add `#include` <util/strencodings.h>, remove or stop using HexEncodeString, and replace calls to HexEncodeString(...) with HexStr(...) (ensuring the argument type matches the HexStr overload, e.g., pass a std::string or span/byte vector as required). This keeps behavior consistent with other fuzz tests like rpc.cpp and decode_tx.cpp and reuses the existing hex encoding utility.
🤖 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/governance_proposal_validator.cpp`:
- Around line 91-95: The clamping of start_epoch via
fuzzed_data_provider.ConsumeIntegralInRange ensures start_epoch + offset cannot
overflow; keep the change as shown (use the bounds
std::numeric_limits<int64_t>::min() + 1024 and
std::numeric_limits<int64_t>::max() - 1024) and leave the computation of
end_epoch (start_epoch +
fuzzed_data_provider.ConsumeIntegralInRange<int64_t>(-4, 1024)) intact; ensure
the explanatory comment ("Clamp start_epoch to avoid signed overflow UB when
adding offset") remains alongside the start_epoch and end_epoch uses to clarify
the intent for future readers and reviewers (references: start_epoch, end_epoch,
fuzzed_data_provider).
---
Nitpick comments:
In `@src/test/fuzz/governance_proposal_validator.cpp`:
- Around line 19-29: Replace the custom HexEncodeString implementation with the
project's standard HexStr utility: add `#include` <util/strencodings.h>, remove or
stop using HexEncodeString, and replace calls to HexEncodeString(...) with
HexStr(...) (ensuring the argument type matches the HexStr overload, e.g., pass
a std::string or span/byte vector as required). This keeps behavior consistent
with other fuzz tests like rpc.cpp and decode_tx.cpp and reuses the existing hex
encoding utility.
This was referenced Feb 23, 2026
test/util/data/non-backported.txt
Outdated
| src/stats/*.h | ||
| src/test/block_reward_reallocation_tests.cpp | ||
| src/test/bls_tests.cpp | ||
| src/test/fuzz/governance_proposal_validator.cppsrc/test/bls_tests.cpp |
Member
There was a problem hiding this comment.
seems you have some new lines missed here
thepastaclaw
force-pushed
the
fuzz/governance-proposal-validator
branch
from
b2e7273 to
3db377c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/fuzz/governance_proposal_validator.cpp (1)
19-29: Consider using the existingHexStrutility instead of a local re-implementation.
HexEncodeStringduplicates the functionality ofHexStrfromutil/strencodings.h. Using the canonical helper improves consistency and reduces local maintenance burden.HexStris already widely used throughout fuzz targets in the codebase.♻️ Proposed refactor
Add the include at the top:
+#include <util/strencodings.h>Then remove
HexEncodeStringand replace the call sites at lines 110–111 and 113:-const std::string json_hex = HexEncodeString( - MakeProposalJson(type, random_name, start_epoch, end_epoch, payment_amount, payment_address, url)); +const std::string json_hex = HexStr(MakeByteSpan( + MakeProposalJson(type, random_name, start_epoch, end_epoch, payment_amount, payment_address, url)));-const std::string malformed_json_hex = HexEncodeString("{" + fuzzed_data_provider.ConsumeRandomLengthString(128)); +const std::string malformed_json_hex = HexStr(MakeByteSpan("{" + fuzzed_data_provider.ConsumeRandomLengthString(128)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/governance_proposal_validator.cpp` around lines 19 - 29, Replace the local HexEncodeString implementation with the canonical HexStr utility: add `#include` "util/strencodings.h" at the top, remove the HexEncodeString function, and change all call sites that use HexEncodeString(some_string) to HexStr(some_string). Ensure the replacement uses the same input type (std::string or Span) as existing HexStr overloads and update any includes or using directives if necessary.
🤖 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 file non-backported.txt contains a concatenated entry
"src/test/fuzz/governance_proposal_validator.cppsrc/test/bls_tests.cpp" which
merges two distinct paths; split them into two separate lines so each path is on
its own line: "src/test/fuzz/governance_proposal_validator.cpp" and
"src/test/bls_tests.cpp". Update the single offending line by inserting a
newline between the two filenames so both entries are valid glob targets for
clang-format CI.
---
Duplicate comments:
In `@src/test/fuzz/governance_proposal_validator.cpp`:
- Around line 77-79: The change ensures start_epoch (via
fuzzed_data_provider.ConsumeIntegralInRange) is clamped to [INT64_MIN+1024,
INT64_MAX-1024] and end_epoch is computed as start_epoch + an offset from
ConsumeIntegralInRange(-4, 1024), preventing signed overflow; keep the existing
bounds and the use of start_epoch and end_epoch as-is (no further changes
required to fuzzed_data_provider usage or the addition).
---
Nitpick comments:
In `@src/test/fuzz/governance_proposal_validator.cpp`:
- Around line 19-29: Replace the local HexEncodeString implementation with the
canonical HexStr utility: add `#include` "util/strencodings.h" at the top, remove
the HexEncodeString function, and change all call sites that use
HexEncodeString(some_string) to HexStr(some_string). Ensure the replacement uses
the same input type (std::string or Span) as existing HexStr overloads and
update any includes or using directives if necessary.
test/util/data/non-backported.txt
Outdated
| src/stats/*.h | ||
| src/test/block_reward_reallocation_tests.cpp | ||
| src/test/bls_tests.cpp | ||
| src/test/fuzz/governance_proposal_validator.cppsrc/test/bls_tests.cpp |
There was a problem hiding this comment.
Fix the missing newline — both entries are concatenated into a single invalid glob.
Line 53 reads src/test/fuzz/governance_proposal_validator.cppsrc/test/bls_tests.cpp, merging two distinct entries without a line break. As a result, neither src/test/fuzz/governance_proposal_validator.cpp nor src/test/bls_tests.cpp is recognized, breaking clang-format CI enforcement for both files.
🐛 Proposed fix
-src/test/fuzz/governance_proposal_validator.cppsrc/test/bls_tests.cpp
+src/test/fuzz/governance_proposal_validator.cpp
+src/test/bls_tests.cpp📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| src/test/fuzz/governance_proposal_validator.cppsrc/test/bls_tests.cpp | |
| src/test/fuzz/governance_proposal_validator.cpp | |
| src/test/bls_tests.cpp |
🤖 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 file non-backported.txt
contains a concatenated entry
"src/test/fuzz/governance_proposal_validator.cppsrc/test/bls_tests.cpp" which
merges two distinct paths; split them into two separate lines so each path is on
its own line: "src/test/fuzz/governance_proposal_validator.cpp" and
"src/test/bls_tests.cpp". Update the single offending line by inserting a
newline between the two filenames so both entries are valid glob targets for
clang-format CI.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/test/fuzz/governance_proposal_validator.cpp (2)
116-116:random_hexis a misleading name — it contains raw bytes, not hex.Every other payload variable (
json_hex,malformed_json_hex,oversized_hex) is genuinely hex-encoded.random_hexis raw fuzzer bytes, which tests the hex-decode failure path of the validator. Rename torandom_bytes(orraw_payload) to make the intent clear and avoid confusion with the adjacent hex-encoded variants.♻️ Proposed rename
- const std::string random_hex = fuzzed_data_provider.ConsumeRandomLengthString(2048); + const std::string raw_payload = fuzzed_data_provider.ConsumeRandomLengthString(2048); for (const bool allow_script : {false, true}) { for (const bool check_expiration : {false, true}) { RunValidatorCase(json_hex, allow_script, check_expiration); RunValidatorCase(malformed_json_hex, allow_script, check_expiration); RunValidatorCase(oversized_hex, allow_script, check_expiration); - RunValidatorCase(random_hex, allow_script, check_expiration); + RunValidatorCase(raw_payload, allow_script, check_expiration); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/governance_proposal_validator.cpp` at line 116, The variable random_hex actually holds raw fuzzer bytes, not hex, so rename random_hex to random_bytes (or raw_payload) wherever it's declared and used in governance_proposal_validator.cpp to clarify intent and avoid confusion with json_hex/malformed_json_hex/oversized_hex; update the declaration (currently using fuzzed_data_provider.ConsumeRandomLengthString(2048)) and all subsequent references in the test helper or validation calls to use the new identifier (random_bytes or raw_payload) so the code clearly reflects that this value tests the hex-decode failure path.
19-29:HexEncodeStringduplicates the existingHexStrutility.
util/strencodings.hin Dash Core providesHexStr(), which converts a string to lowercase hexadecimal — identical to this custom helper. Replace the custom function with:`#include` <util/strencodings.h>Then use
HexStr(s)at lines 110 and 113 instead ofHexEncodeString(s).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/governance_proposal_validator.cpp` around lines 19 - 29, The file defines a redundant HexEncodeString function; remove that function, add `#include` <util/strencodings.h>, and replace uses of HexEncodeString(s) in governance_proposal_validator.cpp with HexStr(s) (which yields the same lowercase hex). Ensure you delete the HexEncodeString definition and update the two call sites where HexEncodeString is used to call HexStr instead.
🤖 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/governance_proposal_validator.cpp`:
- Around line 76-79: The clamp on start_epoch using
fuzzed_data_provider.ConsumeIntegralInRange<int64_t> to the range [INT64_MIN +
1024, INT64_MAX - 1024] properly prevents signed overflow when adding the offset
from ConsumeIntegralInRange<int64_t>(-4, 1024) to produce end_epoch, so no
further changes to the start_epoch/end_epoch logic in
governance_proposal_validator.cpp are required.
---
Nitpick comments:
In `@src/test/fuzz/governance_proposal_validator.cpp`:
- Line 116: The variable random_hex actually holds raw fuzzer bytes, not hex, so
rename random_hex to random_bytes (or raw_payload) wherever it's declared and
used in governance_proposal_validator.cpp to clarify intent and avoid confusion
with json_hex/malformed_json_hex/oversized_hex; update the declaration
(currently using fuzzed_data_provider.ConsumeRandomLengthString(2048)) and all
subsequent references in the test helper or validation calls to use the new
identifier (random_bytes or raw_payload) so the code clearly reflects that this
value tests the hex-decode failure path.
- Around line 19-29: The file defines a redundant HexEncodeString function;
remove that function, add `#include` <util/strencodings.h>, and replace uses of
HexEncodeString(s) in governance_proposal_validator.cpp with HexStr(s) (which
yields the same lowercase hex). Ensure you delete the HexEncodeString definition
and update the two call sites where HexEncodeString is used to call HexStr
instead.
Author
|
Re: newlines in non-backported.txt — fixed in f306b73, pushed right around when you commented. |
Author
|
Superseded by consolidated fuzz branch in #7173 to avoid merge conflicts. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
governance_proposal_validatorCProposalValidatorwith both structured and random proposal payloadsMAX_DATA_SIZEpath)fAllowScripttoggle)fCheckExpirationtrue/false)src/Makefile.test.includeTesting
./configure --enable-fuzz-binary --disable-tests --without-gui --disable-bench --disable-stacktracesmake -C src test/fuzz/fuzz -j8Tracker: thepastaclaw/tracker#129
Epic: thepastaclaw/tracker#108
Validation
What was tested:
./configure --enable-fuzz-binary --disable-tests --without-gui --disable-bench --disable-stacktraces+make -C src test/fuzz/fuzz -j8— built the fuzz binary including the newgovernance_proposal_validatortargetResults:
governance_proposal_validatorfuzz target compiled and linked successfullylinux64-build— pass;linux64-test— 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 + empty corpus run); GitHub Actions CI (linux64 + tsan + ubsan + mac + win64 matrix)