Repository navigation
fix: bound governance object signatures and reset the governance cache - #7831
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. WalkthroughGovernance object signatures are limited to 96 bytes during network and disk serialization; hash serialization still excludes signatures. The governance store format advances to version 17 and no longer serializes the historical invalid-vote-cache field. Tests verify that version 16 caches are discarded and version 17 caches load and round-trip. The release note describes these changes. Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to The change bounds signature allocation on network and disk reads and causes legacy governance data to be resynced. The inspected behavior presents no concrete merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 5 files. (1 skipped: 1 unsupported.)
✨ 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 9e5ee16) · triage: low |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against exact head 1415fd7. The signature cap applies only to network reads and rejects oversized vectors before element allocation, while preserving disk representations, writes, hashes, and valid BLS signatures; no actionable in-scope issue remains. This was static verification only: no builds or tests were run, and the supplied CI snapshot shows formatting and metadata checks passing with container builds still queued.
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The production diff is a small, contained network-deserialization bound using an existing helper, with straightforward preservation of disk, write, and hashing behavior and extensive regression coverage. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 94% left, weekly 95% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
thepastaclaw
added
the
pastaclaw:approved
1415fd7 to
58cacfc
Compare
thepastaclaw
removed
the
pastaclaw:approved
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
Independently reviewed the complete diff at 58cacfc and confirmed that network deserialization rejects signature counts above 96 before allocating signature elements, while preserving disk reads, outbound serialization, hashing, and local validation. The boundary tests and build/lint registrations match the stated scope, and no in-scope defects were found. This was a static review: the supplied CI snapshot shows successful formatting and preliminary checks, but build-container jobs remain queued; the author's reported test and mutation results were not independently reproduced.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The change is a small, well-contained network signature-length bound in src/governance/common.h with focused compatibility tests, not a large or intricate change despite touching peer-facing deserialization. - Phase 1 reviewers: not run (skipped for throughput: 12 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
58cacfc to
4059f6c
Compare
PastaPastaPasta
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Independently reviewed the complete two-commit PR diff at head 4059f6c and the surrounding serialization, hashing, cache-loading, and network error-handling paths; no actionable in-scope defects were found. The 96-byte signature limit rejects oversized counts before signature-element allocation, preserves valid serialization and hashing, and is paired with a cache migration that skips legacy object decoding. Validation was static only: lint and formatting passed in the supplied CI snapshot, source builds remained queued or running, and the author's reported test results were not independently executed.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The diff makes small, well-contained changes using an existing bounded-vector serializer and cache-version invalidation mechanism, with focused tests, rather than a large or intricate deserialization or storage migration change. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 67% left, 5h 79% left - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
| @@ -0,0 +1,6 @@ | |||
| Updated settings | |||
There was a problem hiding this comment.
nit: wrong chapter; should not be updated settings, there's nothing says about settings.
There was a problem hiding this comment.
Agreed, fixed in 9e5ee16: the note is now under "P2P and network changes" and also mentions the 96-byte signature limit itself, not just the cache reset.
🤖 Posted autonomously by Claude on behalf of pasta.
| #include <ios> | ||
| #include <string> | ||
|
|
||
| BOOST_FIXTURE_TEST_SUITE(governance_object_wire_tests, BasicTestingSetup) |
There was a problem hiding this comment.
why new file to test trivial scenario?
There was a problem hiding this comment.
Fair. Dropped governance_object_wire_tests.cpp in 9e5ee16 and folded a single object_signature_size_limit case into governance_vote_wire_tests.cpp, next to the existing vote-signature cap tests from 8f0b813. It checks that network and disk reads accept 0/65/96-byte signatures and reject 97 bytes, and it fails if the bound is removed.
🤖 Posted autonomously by Claude on behalf of pasta.
| const CacheMap<uint256, CGovernanceVote> empty_invalid_votes{MAX_CACHE_SIZE}; | ||
| s << SERIALIZATION_VERSION_STRING | ||
| << mapErasedGovernanceObjects | ||
| << empty_invalid_votes |
There was a problem hiding this comment.
is the only reason to bump version is Remove the historical invalid-vote-cache field or there's any other reason?
Seems as unrelated for PR's purpose.
There was a problem hiding this comment.
The field removal is not the reason for the bump. The bump is the migration for the next commit, which bounds vchSig on disk reads as well as network reads.
Proposal signatures were never bounded or checked (only triggers verify a signature), so an existing governance.dat can hold a proposal with an oversized signature. If the disk-read bound landed without a version bump, such a cache would fail to deserialize and CFlatDB would drop it with an ERROR log line. With the bump, every node drops the old cache once, on purpose, and the release note documents it.
Removing the invalid-vote-cache field only rides along because the format changes anyway; the two TODOs from 0a8c4da asked for exactly that on the next disk-format bump. Leaving it in would mean another format bump later just to remove it. I reworded the first commit description in the PR body to make that ordering clearer.
🤖 Posted autonomously by Claude on behalf of pasta.
Bump the governance cache version so every node discards its existing governance.dat once and syncs governance objects and votes again from the network. Proposal signatures are never checked, so a cache can hold objects whose signature field is arbitrarily large; dropping the old cache lets the next commit bound that field on every read without failing to load an existing cache. The format change also removes the historical invalid-vote-cache field, as the TODOs asked for on the next disk-format version bump. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Valid governance object signatures are at most 96 bytes (BLS), but the signature field was read without a size limit, so a peer could send an object with a huge signature. Bound the field on every read, network and disk alike; the previous commit discards older caches that could still hold larger values. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4059f6c to
9e5ee16
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Independently reviewed the complete PR diff at head 9e5ee16 and verified the supplied reviewer conclusions; no actionable in-scope defects were found. The 96-byte signature bound rejects oversized counts before allocating vector elements, preserves writes and hashing, and is paired with a cache-version bump that discards legacy contents before decoding objects. Validation was static only: the supplied CI snapshot shows successful linting, multiple platform builds, and no-wallet tests, while other build and test jobs remain pending.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The diff is small and contained, adding a straightforward signature-length bound and governance cache version reset with focused regression tests, rather than a large or intricate change to deserialization or storage migration. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 15% left, 5h 51% left - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
…vernance cache 9e5ee16 fix: bound governance object signatures (pasta) f2f1dd4 fix: reset the governance cache when upgrading (pasta) Pull request description: ## Issue being fixed or feature implemented Governance proposal validation limits proposal data to 512 bytes but ignores its separate signature field. A paid, confirmed proposal can therefore carry megabytes of unused signature bytes that peers retain and persist. Each distinct signature-bearing object requires its own collateral payment, so this is economically constrained memory/disk amplification rather than a vote-weight or issuance defect. ## What was done? Two commits, migration first and then enforcement: 1. **fix: reset the governance cache when upgrading.** Bumps the governance cache version (`CGovernanceManager-Version-16` → `17`), so every node discards its existing `governance.dat` once and syncs governance objects and votes again from the network. This is the migration for the next commit: proposal signatures were never bounded or checked, so an existing cache may hold oversized signatures, and bounding disk reads without a version bump would make such a cache fail to load. Since the format version changes anyway, this also drops the historical invalid-vote-cache field, which two TODOs (from 0a8c4da) asked to remove on the next disk-format bump. 2. **fix: bound governance object signatures.** Bounds the signature vector to 96 bytes on every read (network and disk) before its elements are allocated. Empty, 65-byte (legacy ECDSA) and 96-byte (BLS) signatures are still accepted. Writes and object/signature hashing are unchanged, and the existing log-only decode error handling adds no peer penalty. Release note added. ## How Has This Been Tested? On macOS arm64 with the existing depends prefix: - `governance_vote_wire_tests/object_signature_size_limit` (next to the existing vote-signature cap tests): `SER_NETWORK` and `SER_DISK` reads accept 0/65/96-byte signatures and reject a 97-byte one. With the bound removed, this case fails. - `governance_vote_processing_tests/legacy_governance_cache_is_discarded` (replaces `legacy_invalid_vote_cache_is_discarded`): a version 16 cache holding a proposal and a legacy invalid vote loads empty; a version 17 cache loads and round-trips. With the version string left at 16, this test fails. - All governance unit suites pass (`governance_vote_wire_tests`, `governance_vote_processing_tests`, `governance_vote_sync_tests`, `governance_inv_tests`, `governance_superblock_tests`, `governance_validators_tests`). - `feature_governance.py`, `feature_governance_cl.py`, `feature_governance_objects.py` and `feature_governance_txindex_devnet.py` pass. ## Breaking Changes - Governance object signatures over 96 bytes are rejected on network and disk reads. - On the first start after upgrading, the existing governance cache is discarded and governance data is synced again from the network (see `doc/release-notes-7831.md`). ## Checklist: - [x] I have performed a self-review of my own code - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 78fb7a852f3d67163dc35cf6f6f7c7e3d650e2834cf69b97a3d5be92641be73121b16b52bd7317c44e079062396678b6e4f294c55d59c2ecb1eca111960f713b (cherry picked from commit 41cf1a7)
6c02b2b ci: let DEPENDS_ARCH override the machine arch for multiprocess and tsan (pasta) ce5dbf3 Merge #7888: fix(consensus): canonical ExtNetInfo encoding and strict netInfo decoding (pasta) 54cba5c Merge #7537: fix: data races and check-then-act races in the CoinJoin server (pasta) 08d8bca Merge #7909: feat(mining): build the masternode list incrementally during block assembly (pasta) 5633761 Merge #7895: test: cover retroactive provider tx refusal, fix wording (pasta) cbb9500 Merge #7846: fix: treat pending owner keys and Platform node IDs as one conflict namespace (pasta) 8efdf5e Merge #7857: fix: mine a pending ProUpServTx before a key change of its masternode (pasta) e56e6ae Merge #7842: fix(wallet): reject mnemonic passphrases that seed derivation truncates (pasta) 79480ff test: follow-up for #7821: check if wallet is locked after failed upgrade instead creating brand-new scenario (Konstantin Akimov) a56ed47 fix(net): announce sporks to new peers instead of requesting them during mnsync (pasta) cdaea5f Merge #7894: fix: due to logical conflicts feature_pruning.py had been broken while (pasta) 98d92f6 Merge #7817: backport: partial bitcoin#34156 restore cleanup ownership (pasta) 35e6d18 Merge #7858: fix: do not InstantSend-lock provider transactions (pasta) 904d73d Merge #7881: fix: refuse to sign shared registrar updates that move the funder's coins (pasta) 95982f5 Merge #7782: fix(wallet): persist pending-observation coin locks once the record becomes readable (pasta) cdf65d0 Merge #7836: backport: partial bitcoin#25812, bitcoin#29855 (pasta) cc5716f Merge #7824: fix: retain initialized DKG sessions during inventory reads (pasta) 588e458 Merge #7827: backport: partial bitcoin#35984 (pasta) 795c6f1 Merge #7839: fix: release InstantSend tracking after mempool eviction (pasta) dc1a6b2 Merge #7875: docs(rpc): clarify getchainlockbyheight availability (pasta) e4798ff Merge #7876: fix(consensus): capture operator key scheme changes with unchanged bytes in state diffs (pasta) fe68ecb Merge #7871: backport!: Merge bitcoin#27217: wallet: Replace use of purpose strings with an enum (pasta) 8348d22 Merge #7867: fix: refuse masternode list RPCs for blocks outside the active chain (pasta) 861a579 Merge #7866: fix: stop `masternode payments` at the genesis block (pasta) efcef71 Merge #7521: fix: update masternode rate limit on failed governance trigger path (pasta) 0623182 Merge #7831: fix: bound governance object signatures and reset the governance cache (pasta) b230657 Merge #7841: fix: authenticate cached InstantSend proof bytes (pasta) a1f37ea Merge #7820: fix: hold chainstate lock during CoinJoin input validation (pasta) 50a5617 Merge #7829: fix: preserve wallet permissions when changing passphrases (pasta) 2ff0103 Merge #7818: fix: return REST errors for unavailable deployment data (pasta) cb39369 Merge #7834: fix: validate Electrum import input boundaries (pasta) e7e3daa Merge #7821: fix: relock encrypted wallets after HD upgrade failures (pasta) 2fea45f Merge #7792: fix: treat missing snapshots during evodb repair as repair errors (pasta) 3c5c039 Merge #7838: fix(wallet): authenticate unlock deadline extensions (pasta) 0bf80f8 Merge #7810: fix(rpc): handle unavailable masternode payment inputs (pasta) bcefee4 Merge #7822: fix: reject malformed detached quorum share batches (pasta) 86a7737 Merge #7828: fix: omit recovery words from invalid mnemonic errors (pasta) 886d5c4 Merge #7816: fix: check credit pool amount accumulation (pasta) 00fc3b9 Merge #7851: fix(rpc): correct masternode status help for top-level service field (pasta) 0e75c22 Merge #7814: fix(qt): restore HTML escaping of send confirmation recipients (pasta) Pull request description: ## Issue being fixed or feature implemented Backports for v24.0.0-rc.4, on top of `v24.0.x` at the v24.0.0-rc.3 tag (0364ed6, #7808). ## What was done? Cherry-picked (`git cherry-pick -m1 -x`) every develop merge since rc.3 that carries `backport-candidate-24.0.x`, plus eight unlabeled fixes picked after an audit of every other develop merge since rc.3 (marked † below). They are in develop merge order: - #7814 fix(qt): restore HTML escaping of send confirmation recipients † - #7851 fix(rpc): correct masternode status help for top-level service field - #7816 fix: check credit pool amount accumulation - #7828 fix: omit recovery words from invalid mnemonic errors † - #7822 fix: reject malformed detached quorum share batches † - #7810 fix(rpc): handle unavailable masternode payment inputs † - #7838 fix(wallet): authenticate unlock deadline extensions - #7792 fix: treat missing snapshots during evodb repair as repair errors - #7821 fix: relock encrypted wallets after HD upgrade failures - #7834 fix: validate Electrum import input boundaries - #7818 fix: return REST errors for unavailable deployment data - #7829 fix: preserve wallet permissions when changing passphrases - #7820 fix: hold chainstate lock during CoinJoin input validation - #7841 fix: authenticate cached InstantSend proof bytes - #7831 fix: bound governance object signatures and reset the governance cache - #7521 fix: update masternode rate limit on failed governance trigger path - #7866 fix: stop `masternode payments` at the genesis block - #7867 fix: refuse masternode list RPCs for blocks outside the active chain † - #7871 backport!: bitcoin#27217: wallet: Replace use of purpose strings with an enum - #7876 fix(consensus): capture operator key scheme changes with unchanged bytes in state diffs - #7875 docs(rpc): clarify getchainlockbyheight availability † - #7839 fix: release InstantSend tracking after mempool eviction - #7827 backport: partial bitcoin#35984 - #7824 fix: retain initialized DKG sessions during inventory reads - #7836 backport: partial bitcoin#25812, bitcoin#29855 - #7782 fix(wallet): persist pending-observation coin locks once the record becomes readable - #7881 fix: refuse to sign shared registrar updates that move the funder's coins † - #7858 fix: do not InstantSend-lock provider transactions - #7817 backport: partial bitcoin#34156 restore cleanup ownership - #7894 test: fix feature_pruning.py restart arguments † (needed because #7792 makes the evodb repair fatal, which the old restart arguments trip) - #7893 fix(net): announce sporks to new peers instead of requesting them during mnsync - 4a19742 (from #7859) test: follow-up for #7821: check if wallet is locked after failed upgrade instead creating brand-new scenario. Test-only, picked as its commit. Without it, #7821's `test_failed_upgrade()` expects RPC error -1 for a 257-byte mnemonic passphrase, which #7842's new precheck rejects with -8 first, so `wallet_upgradetohd.py --legacy-wallet` fails. On develop, #7859 replaced that scenario before #7842 merged. The rest of #7859 is test cleanup and docs and is left out. - #7842 fix(wallet): reject mnemonic passphrases that seed derivation truncates - #7857 fix: mine a pending ProUpServTx before a key change of its masternode - #7846 fix: treat pending owner keys and Platform node IDs as one conflict namespace - #7895 test: cover retroactive provider tx refusal, fix wording - #7909 feat(mining): build the masternode list incrementally during block assembly - #7537 fix: data races and check-then-act races in the CoinJoin server - #7888 fix(consensus): canonical ExtNetInfo encoding and strict netInfo decoding 34 of the 39 picks change exactly the same lines as their develop merges. #7888 and #7782 needed test-only changes (see testing), and three others needed adaptation, described in their commit messages: - **#7893** was merged to develop twice: f99212b with an earlier revision of its commit, then 72a5997 with the final one, so develop now announces sporks twice in the VERACK handler (a develop fix is open separately). Here the PR's final commit 45acdf3 is picked once instead of either merge. In `src/node/sync_manager.cpp`, this branch still sent `GETSPORKS` from the mnsync tick on every network, because #7847, which limited it to test chains, is not included. The request is removed entirely, as on develop. Mainnet spork values are hardcoded in `CSporkManager::GetSporkValue`, so the mainnet request had no effect. - **#7857** and **#7909**: develop folded `CChainstateHelper` into `Chainstate` (d5c1bd7), which is not on this branch, so the new code goes through `ChainHelper().special_tx` / `m_chain_helper.special_tx` instead of `m_special_tx`. The neighbouring develop-only `CSpecialTxProcessor::GetDeterministicMNListHash` is left out. #7888 tightens consensus rules for ExtAddr provider transactions behind V24, so I checked it against testnet. V24 is not active there yet (the tip's CbTx is still version 3), so the chain has no ExtAddr ProTxs the new rules could reject. rc.3's own serializer always writes canonical, sorted purposes, so every ProTx it builds passes the new rules. Mixed rc.3/rc.4 networks could only split on a hand-crafted non-canonical ExtAddr ProTx mined after V24 activates. So testnet miners and masternodes should move to rc.4 before V24 is signalled. Unlabeled develop fixes since rc.3 that were audited and left out: - Worth considering before the final release: #7869 (startup block replay after an unclean shutdown asserts on a quorum commitment, so recovery needs `-reindex-chainstate`; not a v24 regression), #7833 (gate signing intake before quorum retrieval, DoS hardening), #7682 (intermittent `relic_conf.h` parallel-build failure). - Left for 24.1: #7812, #7826, #7844, #7847, #7850, #7868, plus the routine upstream backports (#7780, #7806, #7830, #7855, #7856, #7862, #7863, #7865, #7883, #7884, #7898, #7906), refactors (#7805, #7878, #7880), and CI, docs and test-only changes. One CI-only commit comes last. It takes only the `ci/test` hunks of 6f71fa5 (#7901), which make `ci/test/00_setup_env_native_{multiprocess,tsan}.sh` honor `DEPENDS_ARCH`. Pull requests into `v24.0.x` run develop's workflow files (`pull_request_target`), and since #7901 those check the depends cache on x86_64 runners and pass the aarch64 build arch in `DEPENDS_ARCH`. Without the hunks, `linux64_multiprocess / Build depends` fails on any depends cache miss (`Build runner computes HOST=aarch64-linux-gnu but check-cache used x86_64-pc-linux-gnu`). Without `DEPENDS_ARCH` the scripts still use `uname -m`, so this branch's own push CI is unchanged. No version change is needed. `configure.ac` is already 24.0.0 with `CLIENT_VERSION_IS_RELEASE` false, so the build takes its version string from the `v24.0.0-rc.4` tag. ## How Has This Been Tested? Built and tested at 6c02b2b on macOS arm64 against depends, with `--enable-debug --enable-werror --disable-hardening`. - `make check`: all unit test suites pass, including `test_dash-qt`. - Functional tests, all passing: `feature_evo_shares_activation.py`, `feature_governance_objects.py`, `feature_llmq_signing.py` (plain and `--spork21`), `feature_masternode_shares.py`, `feature_sporks.py`, `interface_rest.py`, `p2p_instantsend.py`, `p2p_quorum_data.py`, `rpc_masternode.py`, `rpc_psbt.py`, `rpc_quorum.py`, `wallet_backup.py`, `wallet_dump.py`, `wallet_encryption.py`, `wallet_labels.py` and `wallet_upgradetohd.py`, each with `--descriptors` and `--legacy-wallet` where it supports them. - Each pick's changed lines were compared with its develop merge (or, for #7893, its PR commit). They are identical except for the #7893, #7857 and #7909 adaptations above, and the #7888 test adaptation below. - A local macOS arm64 debug build (`--enable-debug --enable-werror`) compiled most of the tree and stopped on a single error in `src/test/evo_netinfo_tests.cpp`. #7888's new test used `CNetAddr::V2()`, the serialization-parameter API from bitcoin#25284, which this branch lacks. The pick now writes that entry through an `OverrideStream` with `ADDRV2_FORMAT`, the way `NetInfoEntry::Serialize` does on this branch (see its commit message). - #7782's new unit test reloads the wallet with develop's three-argument `CWallet` constructor. On this branch the constructor still takes the CoinJoin loader and an `ArgsManager`, so the test passes them the way its fixture already does (see the commit message). - With both test fixes, a full local `make` (`dashd`, `dash-qt`, `test_dash`, `test_dash-qt`) completes with no errors. - `feature_pruning.py` is an extended test that CI does not run. #7894 is included because #7792 makes its restart path fail. ## Breaking Changes Carried over from the picked PRs: - #7876: rc.1 to rc.3 nodes that connected an affected block stored a wrong masternode list diff for it. Those nodes need `-reindex`; `-reindex-chainstate` is not enough for nodes that already marked the following block invalid. - #7888: stricter ExtAddr `netInfo` decoding and validation behind V24 (see above). The RFC1918 check now applies even with `-allowprivatenet`. - #7858: provider transactions no longer get InstantSend locks. Software that waits for an islock on a ProTx should wait for a ChainLock or confirmations instead. - #7857: a `ProUpServTx` spending an output of a pending operator key change, asset lock, asset unlock or MNHF signal is rejected from the mempool until that transaction confirms. - #7831: governance object signatures over 96 bytes are rejected, and the governance cache is discarded and re-synced on first start (`doc/release-notes-7831.md`). - #7871: address purposes are limited to `send`, `receive` and `refund`. Wallets with other purpose strings load with warnings, and `listlabels` errors on an unknown purpose (`doc/release-notes-27217.md`). - #7842: creating an HD wallet with a mnemonic passphrase over 248 bytes fails unless `-allow-deprecated-long-mnemonics` is set, and over 256 bytes always fails (`doc/release-notes-7842.md`). - #7792: a node whose evodb is missing a masternode list snapshot now stops at startup and asks for `-reindex`, and `evodb repair` reports it in `repairErrors`. - #7867: `protx info <proTxHash> <blockHash>`, `protx listdiff` and `masternode payments <blockHash>` return an error for blocks outside the active chain. - #7881: `protx shared_combine` on a `ProUpSharedRegTx` is refused unless its inputs are unlocked coins of a single address of the calling wallet, change returns to that address, and the fee is within `-maxtxfee`. - #7836: PSBTs referencing nonexistent previous outputs fail to deserialize (RPC error -22). - #7834: negative Electrum-import rescan indices are rejected. - #7893: on this branch, mainnet nodes no longer send `GETSPORKS` during mnsync (no effect, mainnet spork values are hardcoded). ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 7de9535ca51620f4ddf5a46df2c3341a5e5fdd2263606f83f7db6b576c31ff7b82e1b285bf66988ac4b2729a9060513c45710135f61d7fd5c147b670d5f405dc
Issue being fixed or feature implemented
Governance proposal validation limits proposal data to 512 bytes but ignores its separate signature field. A paid, confirmed proposal can therefore carry megabytes of unused signature bytes that peers retain and persist. Each distinct signature-bearing object requires its own collateral payment, so this is economically constrained memory/disk amplification rather than a vote-weight or issuance defect.
What was done?
Two commits, migration first and then enforcement:
CGovernanceManager-Version-16→17), so every node discards its existinggovernance.datonce and syncs governance objects and votes again from the network. This is the migration for the next commit: proposal signatures were never bounded or checked, so an existing cache may hold oversized signatures, and bounding disk reads without a version bump would make such a cache fail to load. Since the format version changes anyway, this also drops the historical invalid-vote-cache field, which two TODOs (from 0a8c4da) asked to remove on the next disk-format bump.How Has This Been Tested?
On macOS arm64 with the existing depends prefix:
governance_vote_wire_tests/object_signature_size_limit(next to the existing vote-signature cap tests):SER_NETWORKandSER_DISKreads accept 0/65/96-byte signatures and reject a 97-byte one. With the bound removed, this case fails.governance_vote_processing_tests/legacy_governance_cache_is_discarded(replaceslegacy_invalid_vote_cache_is_discarded): a version 16 cache holding a proposal and a legacy invalid vote loads empty; a version 17 cache loads and round-trips. With the version string left at 16, this test fails.governance_vote_wire_tests,governance_vote_processing_tests,governance_vote_sync_tests,governance_inv_tests,governance_superblock_tests,governance_validators_tests).feature_governance.py,feature_governance_cl.py,feature_governance_objects.pyandfeature_governance_txindex_devnet.pypass.Breaking Changes
doc/release-notes-7831.md).Checklist:
🤖 Generated with Claude Code