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>
…it tests Backport of dashpay#7387 (upstream merge da52293, cherry-picked with -m1). Included because dashpay#7414/dashpay#7442/dashpay#7450 modify src/test/governance_inv_tests.cpp, which this PR introduces; it also replaces the p2p_governance_invs.py functional test with equivalent unit-test coverage. v23.1.x adaptations: (1) governance.h on this branch has no 'namespace governance' block at the top - added one containing only the RELIABLE_PROPAGATION_TIME constant this PR introduces (develop's SuperblockManager forward declaration does not exist here and was not brought along). (2) ProcessVoteAndRelay keeps this branch's 'override' specifier. (3) Makefile.test.include entry added without develop-only governance_superblock_tests.cpp. All other hunks unchanged from upstream. (cherry picked from commit da52293e8d6ba51fca27a557f6efd83a439a4b70)
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 (cherry picked from commit f343d58)
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 (cherry picked from commit f343d58)
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 (cherry picked from commit f343d58)
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 (cherry picked from commit f343d58)
24920a0 chore: prepare v23.1.8 release (pasta) 2194248 Merge #7348: fix: penalize oversized notfound messages (pasta) f5c72c3 Merge #7347: fix: punish invalid dstx messages (pasta) 550caf7 Merge #7465: fix(qt): handle pixel-sized fonts when scaling widgets (pasta) e203710 Merge #7419: fix(net): bound CoinJoin message vector intake (pasta) 5f5b960 Merge #7418: fix(net): bound signing message vector intake (Pasta) 7cc2cca Merge #7450: test: make governance vote fixtures wire-valid (Pasta) f011c80 Merge #7440: fix(net): bound governance vote signature deserialization (Pasta) 4b4d96a Merge #7442: fix(net): authorize governance inv responses via the net-layer per-peer request tracker (Pasta) f855b13 Merge #7444: fix(net): bound bloom message vectors before allocation (Pasta) 5b5c6fb Merge #7415: fix: bound pending sig share queue (Pasta) 9bbe808 Merge #7416: fix(net): bound quorum data response vectors (Pasta) da42f50 Merge #7424: fix: bound ChainLock seen cache (Pasta) 5b310df Merge #7438: fix: bound SPORK signature deserialization (Pasta) e118d0c Merge #7259: fix: dangling point to cj client (Pasta) 9921621 Merge #7439: refactor: add bounded vector deserialization (Pasta) 89bdf7c Merge #7414: fix(net): throttle per-object governance vote sync requests (Pasta) 44c396d Merge #7402: fix: bound pending recovered sig queue to prevent remote OOM (Pasta) 05cfe27 Merge #7351: fix: limit signing share sessions per peer (pasta) 3ef3a5b Merge #7408: fix: bound DKG contribution blob intake (pasta) 0ea6532 Merge #7387: test: migrate governance inv cache coverage to unit tests (Pasta) 8ffdf7f Merge #7398: backport: compact block relay hardening (bitcoin#26898, bitcoin#27626, bitcoin#27743, bitcoin#26969, bitcoin#29412, bitcoin#32646, bitcoin#33296) (Pasta) 2915142 backport: bitcoin#27608 - p2p: Avoid prematurely clearing download state for other peers (PastaClaw) 90b5473 Merge #7396: fix: run of circular-dependencies with python3.15 (Pasta) b003cdc Merge #7395: ci: update GitHub Actions pins for Node 24 (pasta) 97c3dd1 Merge #7394: fix: stabilize par help text in manpages (pasta) 8f8616b Merge #7372: backport: bitcoin#32693: depends: fix cmake compatibility error for freetype (pasta) 48f72be Merge #7360: fix: empty platformP2PPort deprecated field in protx listdiff results (pasta) a8cccff Merge #7298: fix(qt): keep PoSe score visible when hiding banned masternodes (pasta) Pull request description: Release PR for Dash Core v23.1.8, a patch release on top of v23.1.7. Fast-forwards from `v23.1.x` (currently at `chore: prepare v23.1.7 release`), 29 commits, no merge commits, no conflicts. ## Contents Backports of PRs already reviewed and merged on `develop`: `#7259` `#7347` `#7348` `#7351` `#7298` `#7360` `#7372` `#7387` `#7394` `#7395` `#7396` `#7398` `#7402` `#7408` `#7414` `#7415` `#7416` `#7418` `#7419` `#7424` `#7438` `#7439` `#7440` `#7442` `#7444` `#7450` `#7465` Plus `backport: bitcoin#27608`, a single commit taken from Dash #7237 because #7398's compact-block hardening depends on it. The rest of that v0.26 batch is intentionally not included on v23.1.x. The commit is byte-identical to its reviewed counterpart inside #7237. And release preparation: version bump, regenerated man pages, release notes, archived 23.1.7 notes. ## Note for reviewers: this branch was rebuilt An earlier revision of this PR was discarded and the branch rebuilt from scratch. Review comments on the previous revision point at commits that no longer exist, though the feedback itself was carried over (see below). The reason: several commits titled `Merge #NNNN` in the earlier revision contained substantial code that exists nowhere upstream — apparently written from a description of each PR rather than ported from its diff. For example, `feature_llmq_simplepose.py` is byte-identical between v23.1.7 and `develop`, yet the earlier `Merge #7408` rewrote 66 lines of it; `test/functional/p2p_governance_invs.py` does not exist on `develop` at all, yet had grown from 62 to 148 lines. That mislabeling matters because a commit titled `Merge #NNNN` invites less scrutiny, not more. It also had consequences: the earlier revision was **missing #7440 entirely**, and contained eleven consecutive commits that did not compile (code written against newer upstream APIs this branch does not have — `Misbehaving(Peer&)`, and `PeerIsBanned` used five commits before it was declared). Every commit on this branch has now been diffed against its upstream merge commit. Where a backport differs, it is because v23.1.x predates an upstream refactor and the change had to be applied to the pre-refactor file — for example #7418 and #7438 patch `signing_shares.cpp` / `spork.cpp` where upstream patches `net_signing.cpp` / `net_processing.cpp`. ## Dropped from this branch - **#7350** (`net: don't lock cs_main while reading blocks`) — dropped on review feedback. It is a 110-line lock-structure refactor of `ProcessGetBlockData` with no measured benefit, and it would add avoidable churn to the eventual master→develop merge-back. Nothing on this branch depends on it: #7398's compact-block work precedes it, and the remaining 14 commits replay with zero conflicts once it is removed. Thanks @knst. ## Added after the initial review pass - **#7351** (`fix: limit signing share sessions per peer`) — cherry-picked as a single commit and placed before #7402, matching upstream's merge order. The include block additionally carries `<ranges>`: upstream's diff adds only `<algorithm>` because develop already had it, whereas v23.1.x did not and the backported `GetSessionCount()` / `GetAnnouncementSessionCount()` use `std::ranges::count_if`. - **#7465** (`fix(qt): handle pixel-sized fonts when scaling widgets`) — cherry-picked from the five upstream commits. `optiontests.cpp` additionally includes `qt/guiutil_font.h`, because `fontsLoaded()` and `updateFonts()` are declared there on v23.1.x while develop declares them in `qt/guiutil.h`, which is all the upstream test includes. Two further backports were added later and applied without any adaptation -- their diffs are byte-for-byte identical to upstream: - **#7347** (`fix: punish invalid dstx messages`) - **#7348** (`fix: penalize oversized notfound messages`) ## Adaptations worth flagging - **#7360** — upstream gates `platformP2PPort` / `platformHTTPPort` in `protx listdiff` behind `IsServiceDeprecatedRPCEnabled()`. On 23.x those deprecated fields are deliberately not enforced through gating (see `bbcd9d543e6`), so shipping the gate as-is would silently drop two fields that v23.1.7 always returned. Changed to `if (true)` with a comment, per review feedback, keeping the block aligned with `develop`. The substantive fix from #7360 — reading the live port from `netInfo` instead of the always-zero scalar — is retained. - **#7415** — the pending-map caps (`MAX_PENDING_SIG_SHARES_PER_NODE`, `MAX_PENDING_SIG_SHARES_TOTAL`) are backported. The additional bound upstream places on batches awaiting verification is not, because it guards a condition that does not exist here: upstream's dispatcher pushes one task per batch inside an inner loop, whereas v23.1.x pushes a single looping worker per 10 ms tick. There is no unbounded task queue to bound. - **Man pages** — regenerated without the `lock` debug category, which only exists under `DEBUG_LOCKCONTENTION` and so is absent from release binaries. Thanks @UdjinM6 for catching this. ## Known CI failure macOS jobs are expected to fail. `actions/upload-artifact@v6` rejects filenames containing `:`, and the Xcode SDK ships Perl man pages with `::` in the name. A release-branch-only workaround existed on the earlier revision but was dropped as it corresponds to no upstream PR. This is accepted for this release. ## Testing - Every commit through #7465 compiles individually (verified for 27 of the 29; the three additions below were verified at the tip) — verified individually, not just at the tip. - Full build clean; no new warnings. - Unit tests pass. - Functional tests pass: `feature_llmq_signing` (both variants), `feature_llmq_chainlocks`, `feature_llmq_dkgerrors`, `feature_llmq_is_cl_conflicts`, `p2p_instantsend`, `feature_dip3_deterministicmns` (both wallet types), `rpc_coinjoin`. - Qt unit tests pass (32 cases, run under the `cocoa` platform plugin so the pixel-sized font regression from #7465 actually executes rather than self-skipping). - Lint: one pre-existing `lint-cppcheck-dash` failure, identical on v23.1.7, in files this branch does not touch. Top commit has no ACKs. Tree-SHA512: 0fa469c9a33820aa85fbb8b90c5877409d09490f746f1300b05ceda470a600765f900bec42e5aad5d90a2d46b44e28c20e44dc4a8fe719553a072298069eaec4
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: