test: make governance vote fixtures wire-valid - #7450
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe Estimated code review effort: 1 (Trivial) | ~2 minutes 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 |
|
✅ Review complete (commit e058152) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified the one-line test-only fixture fix at the exact requested head: the 65-byte placeholder satisfies the compact-signature wire-size check, the unknown-parent path returns before cryptographic signature verification, and the vote hash does not include the signature bytes. The single commit is focused and accurately described; no in-scope issue survived verification.
Source: reviewers gpt-5.6-sol (general, dash-core-commit-history) and claude-sonnet-5 (general, dash-core-commit-history); verifier claude-sonnet-5.
Note: GitHub does not allow me to approve my own PR, so the verified APPROVE verdict is posted as a COMMENT review.
e058152 test: make governance vote fixtures wire-valid (PastaClaw) Pull request description: ## Issue being fixed or feature implemented The governance inventory tests merged in dashpay#7442 construct unsigned synthetic votes and round-trip them through the network parser. Develop now requires governance vote signatures to use a structurally valid 65- or 96-byte encoding, so the fixture is rejected as malformed before the authorization behavior under test is reached. This causes six unit-test failures across CI configurations. ## What was done? Give the synthetic `VOTE_SIGNAL_FUNDING` vote a compact-signature-sized placeholder. The 65-byte encoding matches the real ECDSA voting-key form for funding votes. These tests deliberately use a missing parent object, so vote processing takes the orphan path before cryptographic signature verification; production validation is unchanged. ## How Has This Been Tested? - Built `src/test/test_dash` in an isolated macOS arm64 worktree using the depends toolchain - `./src/test/test_dash --run_test=governance_inv_tests` - `./src/test/test_dash --run_test=governance_vote_wire_tests` - `git diff --check` - Mandatory independent pre-PR review gate: `ship` with no findings ## Breaking Changes None. Test-only change. ## Checklist: - [x] I have performed a self-review of my own code - [ ] 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 ACKs for top commit: PastaPastaPasta: utACK e058152 Tree-SHA512: 5caca1e34ce563444ded42f669ff599f7658c6d1dbea1d541588211ac542ee45f3dd768e158899cb23150c5d45ec8dbcbf412a1472f73a3cc3e72a5ac37636e0
e058152 test: make governance vote fixtures wire-valid (PastaClaw) Pull request description: ## Issue being fixed or feature implemented The governance inventory tests merged in dashpay#7442 construct unsigned synthetic votes and round-trip them through the network parser. Develop now requires governance vote signatures to use a structurally valid 65- or 96-byte encoding, so the fixture is rejected as malformed before the authorization behavior under test is reached. This causes six unit-test failures across CI configurations. ## What was done? Give the synthetic `VOTE_SIGNAL_FUNDING` vote a compact-signature-sized placeholder. The 65-byte encoding matches the real ECDSA voting-key form for funding votes. These tests deliberately use a missing parent object, so vote processing takes the orphan path before cryptographic signature verification; production validation is unchanged. ## How Has This Been Tested? - Built `src/test/test_dash` in an isolated macOS arm64 worktree using the depends toolchain - `./src/test/test_dash --run_test=governance_inv_tests` - `./src/test/test_dash --run_test=governance_vote_wire_tests` - `git diff --check` - Mandatory independent pre-PR review gate: `ship` with no findings ## Breaking Changes None. Test-only change. ## Checklist: - [x] I have performed a self-review of my own code - [ ] 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 ACKs for top commit: PastaPastaPasta: utACK e058152 Tree-SHA512: 5caca1e34ce563444ded42f669ff599f7658c6d1dbea1d541588211ac542ee45f3dd768e158899cb23150c5d45ec8dbcbf412a1472f73a3cc3e72a5ac37636e0
…lization 8f0b813 fix(governance): bound vote signature deserialization (PastaClaw) Pull request description: Uses the shared bounded-vector deserialization primitive merged in dashpay#7439. ## Motivation Governance vote signatures were deserialized through the generic byte-vector path. A peer could declare a very large signature length, causing allocation before the stream reported truncation. The outer message-processing catch did not score or disconnect the peer, allowing repeated malformed messages. ## Changes - bound network governance-vote signature reads to 96 bytes before allocation - require one of the two structurally valid encodings: 65-byte compact ECDSA or 96-byte BLS - score malformed or truncated governance vote messages with 100 misbehavior points - preserve disk, hash, and outbound serialization behavior - add focused unit coverage ## Testing - `./src/test/test_dash --run_test=governance_vote_wire_tests` (4/4 tests) - `./src/test/test_dash --run_test=serialize_tests` (10/10 tests) - `test/lint/lint-python.py` Tree-SHA512: backported to v23.1.x by cherry-picking 8f0b813 (applies cleanly). Backport note for v23.1.8 ------------------------- This was missing from the original v23.1.8 branch while its test follow-up dashpay#7450 ("test: make governance vote fixtures wire-valid", 915566d) was already included. That ordering was inverted: dashpay#7450 exists solely to adapt the dashpay#7442 governance-inv fixtures to the bound that dashpay#7440 introduces. Verified by removing dashpay#7450's SetSignature() line and rebuilding: without dashpay#7440 present the fixtures pass regardless, and re-adding dashpay#7440 reproduces exactly the six governance_inv_tests failures dashpay#7450's description cites. So the branch was shipping the compensating test change for a hardening fix it did not have, leaving CGovernanceVote::vchSig unbounded on the network path. The prerequisite dashpay#7439 (LIMITED_VECTOR) is already present via 099b99d, as are the sibling bounding backports dashpay#7416/dashpay#7418/dashpay#7419/dashpay#7438/dashpay#7444, so this restores the intended set rather than widening release scope. Reported-by: UdjinM6 Co-Authored-By: Claude <noreply@anthropic.com>
Issue being fixed or feature implemented
The governance inventory tests merged in #7442 construct unsigned synthetic votes and round-trip them through the network parser. Develop now requires governance vote signatures to use a structurally valid 65- or 96-byte encoding, so the fixture is rejected as malformed before the authorization behavior under test is reached. This causes six unit-test failures across CI configurations.
What was done?
Give the synthetic
VOTE_SIGNAL_FUNDINGvote a compact-signature-sized placeholder. The 65-byte encoding matches the real ECDSA voting-key form for funding votes. These tests deliberately use a missing parent object, so vote processing takes the orphan path before cryptographic signature verification; production validation is unchanged.How Has This Been Tested?
src/test/test_dashin an isolated macOS arm64 worktree using the depends toolchain./src/test/test_dash --run_test=governance_inv_tests./src/test/test_dash --run_test=governance_vote_wire_testsgit diff --checkshipwith no findingsBreaking Changes
None. Test-only change.
Checklist: