test: reach the governance vote signature path with a chain-backed fixture - #7528
test: reach the governance vote signature path with a chain-backed fixture#7528PastaPastaPasta wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds comprehensive governance vote-processing tests for orphan replay, invalid signatures, unknown masternodes, future-dated votes, key authorization, penalties, duplicates, and vote counts. Extends Sequence Diagram(s)sequenceDiagram
participant GovernanceVoteTest
participant GovernanceManager
participant MasternodeState
GovernanceVoteTest->>GovernanceManager: submit governance vote
GovernanceManager->>MasternodeState: validate masternode and signing key
MasternodeState-->>GovernanceManager: authorization result
GovernanceManager-->>GovernanceVoteTest: cache, reject, or count vote
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
✅ Final review complete — no blockers (commit c4f671e) |
|
PR #7536 adds shared |
FundTransaction always paid the change back to the payout script, which only works when that script is spendable. A test funding a governance proposal fee has to burn the amount to an OP_RETURN, so the change needs its own destination.
…xture The existing governance unit tests run on a fixture with no chain, so the tip masternode list is empty and every CGovernanceVote::IsValid() call short-circuits at GetMNByCollateral before any signature is verified. Nothing exercised CheckSignature, and nothing proved that a legitimately signed vote is accepted at all. Add a fixture that mines a regtest chain, registers a masternode via a real ProRegTx and keeps its voting (ECDSA) and operator (BLS) keys, so votes can be signed for real. On top of it: an orphan vote (parent object unknown) from a registered masternode is cached, requested and replayed onto the object once its fee collateral confirms and the proposal arrives; forged, unknown-masternode and future-dated votes are rejected with a peer penalty; and funding votes on a proposal are accepted only from the voting key while other signals accept the operator key. Verified by mutation: inverting a masternode/signature gate in front of the orphan cache fails the orphan test, and making CheckSignature always succeed fails the rejection tests.
fe12cc9 to
b7f0cde
Compare
|
This pull request has conflicts, please rebase. |
…the strprintf lint-format-strings.py cannot parse raw string literals and miscounts the format specifiers, failing the Lint CI job.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR adds a chain-backed governance vote fixture (governance_vote_processing_tests.cpp) that finally exercises CGovernanceVote::CheckSignature end-to-end, plus a small backward-compatible FundTransaction() overload to support burning proposal-fee change to an OP_RETURN. The fixture, teardown ordering, and the three test cases were verified against the actual governance/vote/object logic and are correct; the remaining findings are test-coverage suggestions (the orphan-replay test doesn't confirm manager-level vote indexing, and there's no negative BLS-signature test) plus a minor commit-hygiene note about squashing the trailing lint fixup into the commit it corrects.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 3 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/test/governance_vote_processing_tests.cpp`:
- [SUGGESTION] src/test/governance_vote_processing_tests.cpp:240-245: Orphan-replay assertions don't confirm the vote is indexed by the manager
CGovernanceManager::CheckOrphanVotes() (governance.cpp) replays a cached orphan vote by calling govobj.ProcessVote() directly — unlike CGovernanceManager::ProcessVote(), it never inserts the vote into cmapVoteToObject. That means HaveVoteForHash() returns false for an orphan vote that was just successfully applied through this path, so NetGovernance::ProcessGetData() (which requires HaveVoteForHash() before calling SerializeVoteForHash()) can't serve the inv that RelayVote() announced during the same replay to a peer that asks for it. This test only checks object-local counters (stored->GetAbsoluteYesCount(...), govman.GetCurrentVotes(...).size()) after the replay, never govman.HaveVoteForHash(vote.GetHash()), so it can't distinguish 'fully recovered and indexed' from 'applied to the object but invisible to the manager.' Add a HaveVoteForHash() assertion after the replay so the test's own claim of full recovery is actually verified — the underlying manager-indexing gap in CheckOrphanVotes() itself is pre-existing production behavior, not something this test PR needs to fix.
- [SUGGESTION] src/test/governance_vote_processing_tests.cpp:349-356: No negative test for a forged BLS (operator-key) signature
CGovernanceVote::CheckSignature(const CBLSPublicKey&) is only ever exercised with the real, correct operator key (SignWithOperatorKey(valid_by_operator, mn_operator_key) at line 351). There's no case that signs a vote with an unrelated, freshly generated BLS secret key and asserts rejection — so a regression that makes BLS verification always succeed would slip through this suite while the equivalent ECDSA case is explicitly covered (the attacker_key path in unsigned_and_unknown_masternode_votes_are_rejected, lines 265-276). Add a case that signs a VOTE_SIGNAL_VALID vote with a fresh, unrelated CBLSSecretKey and asserts GOVERNANCE_EXCEPTION_PERMANENT_ERROR with penalty 20 and a zero vote count, mirroring the existing forged-ECDSA case, before submitting the real operator-signed vote.
In `<commit:c4f671e71a4>`:
- [SUGGESTION] <commit:c4f671e71a4>:1: Squash the lint-only fixup commit into the commit it corrects
Commit c4f671e71a4 changes a single line in src/test/governance_vote_processing_tests.cpp — swapping a raw string literal for an escaped one so lint-format-strings.py can parse the strprintf call and count its format specifiers correctly. That file was introduced two commits earlier in this same PR by b7f0cde6edd, so this commit only exists to correct code that has never independently existed in history; it has no standalone value for git blame or bisect. Per the 'one logical change per commit' convention, squash it into b7f0cde6edd (mark it `fixup!` and run `git rebase -i --autosquash`) so the test file lands in develop already lint-clean in a single commit.
| // The orphan vote is now applied to the object it was waiting for and is no longer pending. | ||
| auto stored = govman.FindConstGovernanceObject(parent_hash); | ||
| BOOST_REQUIRE(stored != nullptr); | ||
| BOOST_CHECK_EQUAL(stored->GetAbsoluteYesCount(tip_mn_list(), VOTE_SIGNAL_FUNDING), 1); | ||
| BOOST_CHECK_EQUAL(govman.GetCurrentVotes(parent_hash, mn_collateral).size(), 1U); | ||
| BOOST_CHECK(govman.GetOrphanVoteObjectHashes().empty()); |
There was a problem hiding this comment.
🟡 Suggestion: Orphan-replay assertions don't confirm the vote is indexed by the manager
CGovernanceManager::CheckOrphanVotes() (governance.cpp) replays a cached orphan vote by calling govobj.ProcessVote() directly — unlike CGovernanceManager::ProcessVote(), it never inserts the vote into cmapVoteToObject. That means HaveVoteForHash() returns false for an orphan vote that was just successfully applied through this path, so NetGovernance::ProcessGetData() (which requires HaveVoteForHash() before calling SerializeVoteForHash()) can't serve the inv that RelayVote() announced during the same replay to a peer that asks for it. This test only checks object-local counters (stored->GetAbsoluteYesCount(...), govman.GetCurrentVotes(...).size()) after the replay, never govman.HaveVoteForHash(vote.GetHash()), so it can't distinguish 'fully recovered and indexed' from 'applied to the object but invisible to the manager.' Add a HaveVoteForHash() assertion after the replay so the test's own claim of full recovery is actually verified — the underlying manager-indexing gap in CheckOrphanVotes() itself is pre-existing production behavior, not something this test PR needs to fix.
source: ['codex']
| // Operator key on a non-funding signal: accepted and counted. | ||
| CGovernanceVote valid_by_operator{MakeVote(parent_hash, VOTE_SIGNAL_VALID, VOTE_OUTCOME_YES)}; | ||
| SignWithOperatorKey(valid_by_operator, mn_operator_key); | ||
|
|
||
| CGovernanceException valid_exception; | ||
| BOOST_CHECK(govman.ProcessVote(valid_by_operator, valid_exception, hash_to_request)); | ||
| BOOST_CHECK(govman.HaveVoteForHash(valid_by_operator.GetHash())); | ||
| BOOST_CHECK_EQUAL(stored->GetAbsoluteYesCount(tip_mn_list(), VOTE_SIGNAL_VALID), 1); |
There was a problem hiding this comment.
🟡 Suggestion: No negative test for a forged BLS (operator-key) signature
CGovernanceVote::CheckSignature(const CBLSPublicKey&) is only ever exercised with the real, correct operator key (SignWithOperatorKey(valid_by_operator, mn_operator_key) at line 351). There's no case that signs a vote with an unrelated, freshly generated BLS secret key and asserts rejection — so a regression that makes BLS verification always succeed would slip through this suite while the equivalent ECDSA case is explicitly covered (the attacker_key path in unsigned_and_unknown_masternode_votes_are_rejected, lines 265-276). Add a case that signs a VOTE_SIGNAL_VALID vote with a fresh, unrelated CBLSSecretKey and asserts GOVERNANCE_EXCEPTION_PERMANENT_ERROR with penalty 20 and a zero vote count, mirroring the existing forged-ECDSA case, before submitting the real operator-signed vote.
source: ['codex']
2 participants
Issue being fixed or feature implemented
No governance unit test ever reaches
CGovernanceVote::CheckSignature.The existing governance fixtures (e.g.
governance_inv_tests.cpp) are built onTestingSetupwith no chain, som_dmnman.GetListAtChainTip()returns an emptymasternode list and every
CGovernanceVote::IsValid()call short-circuits atGetMNByCollateral()before any signature is verified. Consequently:accepted at all;
object) actually recovers once the object shows up.
That last one is the dangerous gap: a change that made vote acceptance stricter
in the wrong place would silently break governance orphan-vote recovery
entirely, and the unit tests would stay green.
feature_governance.pydoes notdeterministically produce a vote that arrives before its parent object, so it is
not reliable coverage for that path either.
What was done?
Added
src/test/governance_vote_processing_tests.cpp: a chain-backed fixtureplus three test cases. No production code is touched.
The fixture mines a regtest chain, activates DIP3, registers one masternode with
a real
ProRegTxand keeps its voting (ECDSA) and operator (BLS) keys, so votescan be signed for real and the signature checks are genuinely exercised. It also
enables the tx index, which
CGovernanceObject::IsCollateralValid()reads theproposal fee transaction from — without it no proposal can ever be accepted.
Test cases:
orphan_vote_is_cached_and_applied_when_parent_arrives— a properly signedvote whose parent object is unknown is cached as an orphan, reported back as
the object hash to request from the peer, and raises no peer penalty. The
proposal fee is then burned to an
OP_RETURNcommitting to the object hash andburied under the required confirmations, the proposal is accepted for real, and
the orphan vote is replayed onto it (yes-count 1, orphan list empty). A peer
re-sending the vote afterwards is not punished.
unsigned_and_unknown_masternode_votes_are_rejected— a forged signature, avote from an outpoint belonging to no masternode, and a vote dated too far in
the future are each rejected with a permanent error and a penalty of 20; a
repeat of a vote already known to be invalid is still penalised; the object's
vote count stays at zero.
proposal_funding_votes_require_the_voting_key— an operator-key-signedfunding vote on a proposal is rejected (valid BLS signature, wrong key for that
signal), a voting-key-signed funding vote is accepted and counted, an
operator-key-signed
VALIDvote is accepted and counted, and a duplicate of anaccepted vote is dropped without a penalty.
The chain/ProRegTx plumbing comes from
src/test/util/masternode.h, the sharedmodule added in #7536, so nothing is duplicated here.
One preparatory commit is needed for that:
FundTransaction()always paid thechange back to the payout script, which does not work for the proposal fee
transaction — its payout is an
OP_RETURNburn, andIsCollateralValid()rejects the transaction unless the change lands on a P2PKH output. An overload
taking a separate change script covers that; the existing five-argument form
keeps its behaviour and no existing call site changes.
How Has This Been Tested?
./src/test/test_dash --run_test=governance_vote_processing_tests— passes,roughly 0.6s for all three cases.
./src/test/test_dash --run_test=block_reward_reallocation_testsand--run_test=evo_dip3_activation_tests— pass, covering the other users of theshared
FundTransaction()helper../src/test/test_dashrun — 793 cases, no errors. The pre-existinggovernance suites (
governance_inv_tests,governance_validators_tests,governance_superblock_tests,governance_vote_wire_tests) andevo_dip3_activation_testsare unaffected.test/lint/all-lint.py— clean apart from cppcheck warnings that alreadyexist on develop in unrelated files;
clang-formatreports no differences onthe new file.
The tests were checked to actually bite, by mutation:
CGovernanceManager::ProcessVotewith its condition inverted (so thatlegitimate votes are rejected) fails
orphan_vote_is_cached_and_applied_when_parent_arriveson 7 assertions.CGovernanceVote::CheckSignatureoverloads returntrueunconditionally fails the other two cases on 15 assertions.
correctly and the failure contained to this suite: the three cases fail and
evo_dip3_activation_tests/txindex_testsstill pass in the same binary run.Environment: macOS (arm64), depends build,
--enable-debug.Breaking Changes
None. Test-only change.
Checklist: