refactor(utils): consolidate masternode test helpers - #7536
Merged
PastaPastaPasta merged 1 commit intoAug 3, 2026
Merged
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 (6)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughThis change adds shared masternode test utilities for UTXO selection, transaction funding and signing, provider-registration creation, and payout script generation. Existing tests use these shared helpers instead of local implementations. A test verifies that Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MasternodeTest
participant CreateProRegTx
participant FundTransaction
participant SignTransaction
MasternodeTest->>CreateProRegTx: create provider-registration transaction
CreateProRegTx->>FundTransaction: fund collateral
FundTransaction-->>CreateProRegTx: funded inputs and change
CreateProRegTx->>SignTransaction: sign transaction inputs
SignTransaction-->>CreateProRegTx: signed transaction
CreateProRegTx-->>MasternodeTest: provider-registration transaction
Possibly related PRs
🚥 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 |
This was referenced Aug 3, 2026
|
ℹ️ Review superseded (commit 5d52ff2) |
|
This pull request has conflicts, please rebase. |
PastaPastaPasta
force-pushed
the
codex/consolidate-masternode-test-utils
branch
2 times, most recently
from
August 3, 2026 17:49
bd1e5ca to
84c0a1e
Compare
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
|
This was referenced Aug 3, 2026
|
This pull request has conflicts, please rebase. |
PastaPastaPasta
force-pushed
the
codex/consolidate-masternode-test-utils
branch
from
August 3, 2026 18:29
84c0a1e to
5d52ff2
Compare
PastaPastaPasta
added a commit
that referenced
this pull request
Aug 8, 2026
…ain-backed fixture 6a31b03 test: cover the governance vote signature path with a chain-backed fixture (pasta) 5c001ef refactor(utils): let FundTransaction return change to a separate script (pasta) Pull request description: ## 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 on `TestingSetup` with no chain, so `m_dmnman.GetListAtChainTip()` returns an empty masternode list and every `CGovernanceVote::IsValid()` call short-circuits at `GetMNByCollateral()` before any signature is verified. Consequently: * nothing proves a correctly signed vote from a registered masternode is accepted at all; * nothing proves a forged signature is rejected; * nothing proves the orphan-vote path (a vote that arrives before its parent 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.py` does not deterministically 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 fixture plus three test cases. No production code is touched. The fixture mines a regtest chain, activates DIP3, registers one masternode with a real `ProRegTx` and keeps its voting (ECDSA) and operator (BLS) keys, so votes can be signed for real and the signature checks are genuinely exercised. It also enables the tx index, which `CGovernanceObject::IsCollateralValid()` reads the proposal fee transaction from — without it no proposal can ever be accepted. Test cases: * `orphan_vote_is_cached_and_applied_when_parent_arrives` — a properly signed vote 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_RETURN` committing to the object hash and buried 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, a vote 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-signed funding 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 `VALID` vote is accepted and counted, and a duplicate of an accepted vote is dropped without a penalty. The chain/ProRegTx plumbing comes from `src/test/util/masternode.h`, the shared module added in #7536, so nothing is duplicated here. One preparatory commit is needed for that: `FundTransaction()` always paid the change back to the payout script, which does not work for the proposal fee transaction — its payout is an `OP_RETURN` burn, and `IsCollateralValid()` 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_tests` and `--run_test=evo_dip3_activation_tests` — pass, covering the other users of the shared `FundTransaction()` helper. * Full `./src/test/test_dash` run — 793 cases, no errors. The pre-existing governance suites (`governance_inv_tests`, `governance_validators_tests`, `governance_superblock_tests`, `governance_vote_wire_tests`) and `evo_dip3_activation_tests` are unaffected. * `test/lint/all-lint.py` — clean apart from cppcheck warnings that already exist on develop in unrelated files; `clang-format` reports no differences on the new file. The tests were checked to actually bite, by mutation: * Adding a masternode/signature gate in front of the orphan cache in `CGovernanceManager::ProcessVote` with its condition inverted (so that legitimate votes are rejected) fails `orphan_vote_is_cached_and_applied_when_parent_arrives` on 7 assertions. * Making both `CGovernanceVote::CheckSignature` overloads return `true` unconditionally fails the other two cases on 15 assertions. * Forcing a fixture-constructor invariant to fail leaves the tx index torn down correctly and the failure contained to this suite: the three cases fail and `evo_dip3_activation_tests` / `txindex_tests` still pass in the same binary run. Environment: macOS (arm64), depends build, `--enable-debug`. ## Breaking Changes None. Test-only change. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone Top commit has no ACKs. Tree-SHA512: a71ff1ff8179c476df38eaec1eb32868f5ec7f30167a59ef9b924e50e846eb346f7a43178fab3240af52a796e4d6f15d8a03d2e9decb5636a7740e7011bfb674
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.
Issue being fixed or feature implemented
Masternode unit tests duplicated the same UTXO selection, transaction funding, signing, and ProRegTx construction helpers. Fixes to this plumbing had to be repeated in every copy, and unspendable coinbase outputs could enter the test UTXO map.
What was done?
test/util/masternode.{h,cpp}as the shared location for the common masternode transaction helpers.block_reward_reallocation_testsandevo_deterministicmns_teststo use the shared utility.BuildSimpleUtxoMapskip unspendable outputs such asOP_RETURN.How Has This Been Tested?
make -C src -j8 test/test_dash./src/test/test_dash --run_test=block_reward_reallocation_tests./src/test/test_dash --run_test=evo_dip3_activation_teststest/lint/lint-whitespace.pytest/lint/lint-includes.pytest/lint/lint-include-guards.pygit diff --checkBreaking Changes
None.
Checklist:
This pull request was created by Codex.