Repository navigation
backport: v24.0.x backports for v24.0.0-rc.4 - #7917
Merged
PastaPastaPasta merged 40 commits intoOct 17, 2026
Merged
Conversation
…on recipients b0f8630 fix: restore HTML escaping of send confirmation recipients (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented A payment URI can supply a label containing HTML. The send confirmation inserts that label, along with the amount, wallet name and address, into rich text, so tags such as `<br>` change what the user sees while reviewing the recipient. Dash used to escape the label and wallet name here. [Bitcoin Core bitcoin#16826](bitcoin#16826) was already backported as d7281c8 (2021), but that merge did not take the `sendcoinsdialog.cpp` hunks, because Dash's older escaping was still in place at the time. The [bitcoin#15886](bitcoin#15886) backport, a081afd (2022), then rewrote the confirmation text and dropped both `GUIUtil::HtmlEscape(rcp.label)` and `GUIUtil::HtmlEscape(model->getWalletName())`. Since then neither has been escaped. ## What was done? This is a fix that restores the lost escaping. It is not a new backport of bitcoin#16826. Each recipient line is HTML-escaped just before it is joined into the rich-text confirmation. That covers the `sendcoinsdialog.cpp` hunks of both upstream commits from bitcoin#16826: the label escape from [2c530ea](bitcoin@2c530ea) and the wallet-name escape from [1770a97](bitcoin@1770a972d471). The plain-text "Show Details" view is built from the same list before escaping and stays unchanged, so labels with ampersands, angle brackets, quotes or entity text still show literally there. A regression was added to the existing funded-wallet GUI test. It drives labels, including the URI-supplied label `<br>Forged recipient`, through the real send dialog, checks the rendered confirmation text, and cancels the confirmation before the existing send and transaction-list checks run. ## How Has This Been Tested? On macOS arm64 with prebuilt depends and Qt 5.15: - Built `qt/dash-qt` and `qt/test/test_dash-qt`. - `QT_QPA_PLATFORM=cocoa src/qt/test/test_dash-qt` passed in full, exit status 0, including `WalletTests`. The `minimal` platform skips `WalletTests` on macOS. - With the new regression and the `sendcoinsdialog.cpp` change reverted, `WalletTests::walletTests()` failed. The rendered-text check fails for the markup label, and the test then hangs until the Qt test timeout. ## Breaking Changes None. Labels and wallet names now display literally in the rich-text confirmation, as before the bitcoin#15886 backport. Stored labels, transaction recipients, amounts and send behavior are unchanged. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation (not applicable to this display fix) - [ ] I have assigned this pull request to a milestone (maintainer decision) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: f1490fff7d2b2355135cb041f2c36fb3d54b439af64de82a9e6527b566d8a3654f55f76aca400936d48a463174a2588899d0f72b00fa0207311bce9584c02723 (cherry picked from commit e07b557)
…level service field 2fb1e39 fix(rpc): correct masternode status help for top-level service field (thephez) Pull request description: ## Issue being fixed or feature implemented The `masternode status` help says the top-level `service` field is deprecated, optional, and only returned when `-deprecatedrpc=service` is passed. In fact the RPC returns it every time. This key reports the host node's own address, so it was deliberately left out of the `service` deprecation (see `rpc_netinfo.py`). The help text was taken from the shared deprecated entry, which only applies to the nested `dmnState.service` key. ## What was done? - Replaced the shared `GetRpcResult("service", /*optional=*/true)` lookup in `masternode status` with a dedicated, non-optional result entry: "IP address and port of this masternode". - The nested `dmnState.service` key and the shared help entry used by other RPCs are unchanged and still gated by `-deprecatedrpc=service`. ## How Has This Been Tested? Help-text-only change. `rpc_netinfo.py` already asserts that the top-level `service` key is always present in `masternode status` output, which matches the field now being non-optional. ## Breaking Changes None. RPC output is unchanged; only the help text differs. ## Checklist: - [ ] 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 _(for repository code-owners and collaborators only)_ Top commit has no ACKs. Tree-SHA512: a18b253e6ccc67b54b77805396b53b4ced59c1ce7722c5fef9d96d2ee3b6a749173e205b42bd2bcb4dbee6b44f68d7c02fc322b91e82efecbdd00307e7fbfa71 (cherry picked from commit 2340e3a)
3ef0555 fix(evo): check credit pool amount accumulation (pasta) Pull request description: ## Issue being fixed or feature implemented Credit-pool deposit accounting uses unchecked signed addition. `ConnectBlock` runs `ProcessSpecialTxsInBlock` (and with it the credit-pool diff) before `CheckTxInputs`, so asset-lock amounts are accumulated before their funding is checked. A block that stacks enough unfunded `MAX_MONEY` asset locks overflows `sessionLocked`, which is signed-overflow undefined behaviour and terminates trapping builds. Checking representability before mutation turns this into a normal validation failure and keeps correct accounting for representable deposit/withdrawal flows. ## What was done? Use the existing `CheckedAdd` helper for session deposit and withdrawal amounts and the withdrawal output sum. Assign session amounts only after the candidate sum passes. Calculate the final balance with checked arithmetic, subtracting withdrawals before adding deposits and the Platform reward. An unrepresentable final balance throws through the existing validation/mining error paths. The `CAmount` getter API, withdrawal limits and atomic package rollback remain intact. No aggregate or final `MAX_MONEY` rule was added, and the final net balance is checked after processing rather than after each deposit. ## How Has This Been Tested? - Local arm64 macOS build using prebuilt depends: `make -C src dashd test/test_dash` passed. - `./src/test/test_dash --run_test=evo_assetlocks_tests` passed. The new `credit_pool_lock_arithmetic` case accumulates individually in-range deposits to the `CAmount` boundary through the public `ProcessLockUnlockTransaction` API and checks that the next deposit is rejected and leaves the diff unchanged. The existing suite still covers ordinary asset locks and package rollback. The fixture exercises the public bookkeeping interface; it is not a funded-block proof, and full-node exploitability was not reproduced. ## Breaking Changes Successful representable accounting retains its mathematical result. Unrepresentable sums now report an error. No monetary cap or activation rule was added. ## 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 ACKs for top commit: knst: utACK 3ef0555 Tree-SHA512: 461cbd9f11ea078159b1e8166fcb511920b937590dcc83c8b4ccc48db009a34cc975b3fd431920038561cc793242756f9d93dc20ab60c3be079e7f7ceb58947c (cherry picked from commit 21ea7ba)
…rors e5fe71d fix: omit recovery words from invalid mnemonic errors (pasta) Pull request description: ## Issue being fixed or feature implemented Invalid-mnemonic errors included the submitted recovery words. The legacy wallet startup path forwards those errors to stderr and debug.log, exposing recovery material in diagnostics. ## What was done? Remove the words from the two existing error messages. Mnemonic validation, seed derivation and error propagation are unchanged. ## How Has This Been Tested? `make -j1`, the existing `bip39_tests` and `wallet_crypto_tests`, and diff, whitespace and include checks pass. The production edits are identical to the previously validated version: a standalone before/after check with a synthetic public mnemonic confirmed redaction in legacy startup stderr/debug.log and descriptor RPC errors. No test is added by this PR. ## Breaking Changes Invalid-mnemonic errors no longer echo recovery words. ## Checklist: - [x] I have performed a self-review of my own code - [x] Documentation is not needed for this diagnostic change - [ ] I have assigned this pull request to a milestone This pull request was created by Codex. Top commit has no ACKs. Tree-SHA512: b4aa992abcf5d79f04da63cda5297160dac8fccc01bb4cb3b7fee9259158b43f8e66f24062e35cdaffe1a5bdb5ad70432eb791357ab3fc51a98fcb48f7cadecf (cherry picked from commit e075859)
7a9d1b0 fix: reject malformed detached quorum share batches (pasta) Pull request description: ## Issue being fixed or feature implemented When a detached sig share's lazy BLS signature fails to deserialize, `NetSigning::ProcessPendingSigShares` bans the source and stops collecting its shares, but never adds it to `batchVerifier.badSources`. The dispatch loop then passes that source's whole vector to `CSigSharesManager::ProcessPendingSigShares`, including the malformed share and every unverified share after it. That method assumes its input is already verified, so unverified shares can reach the local share manager and interfere with recovery. ## What was done? In the malformed-signature branch, replace `BanNode(nodeId)` with `batchVerifier.badSources.emplace(nodeId)`. The existing dispatch loop already bans every node in `badSources` once (which also re-requests its shares) and skips its vector, so the malformed source's shares are no longer dispatched. Valid sources go through the same verification path as before. dashpay#7840 changes the same `ProcessPendingSigShares` block (per-signature BLS authentication) and should be rebased over whichever of the two merges first. ## How Has This Been Tested? Verified by inspection and build. No regression test is added, because the one-line change is self-evident from the diff: `badSources` now drives the existing ban-and-skip dispatch loop, and `CBLSBatchVerifier::Verify()` only adds to `badSources` and never clears it. - macOS arm64: `make -C src dashd test/test_dash dash-cli` built cleanly. - `./src/test/test_dash --run_test=llmq_utils_tests` passed. - `test/functional/test_runner.py feature_llmq_signing.py "feature_llmq_signing.py --spork21"` passed. ## Breaking Changes None to consensus rules, historical commitment validation, wire formats, or valid signatures. Detached batches from a source with a malformed share are now dropped instead of dispatched. ## 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 No user-facing documentation change is needed for this internal validation repair. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 26cfc448c36e93973224b4cad31b24bfee9341e3632cb73ebda5270f3a55bc0591bd2e5e919869c6793029ed8d89ea713b6099b6a0e1bbf8d876dc44a770e79c (cherry picked from commit 063aee1)
…nputs 696c0c0 fix(rpc): handle unavailable masternode payment inputs (pasta) Pull request description: ## Issue being fixed or feature implemented Source inspection shows that `masternode payments` dereferences a nullable previous-transaction lookup while calculating historical block fees. Querying a spending block when historical input data is unavailable can terminate the node. Returning a normal RPC error before the dereference preserves node availability and successful fee calculations. ## What was done? Check the transaction lookup and output index before reading the input amount. Missing transaction data returns `RPC_MISC_ERROR`; an unavailable referenced output returns `RPC_INTERNAL_ERROR`. Queries that do not need historical inputs continue to work without the index, and indexed fee and payment calculations remain unchanged. ## How Has This Been Tested? - `test/functional/test_runner.py rpc_masternode.py` passed on arm64 macOS. It restarts a node with `-txindex=0` and queries `masternode payments` for a block that spends a non-coinbase input; the RPC returns error -1 (`previous transaction not found`) and the node stays responsive. - The pre-fix crash was not reproduced; the out-of-range output check is covered by review only. ## Breaking Changes No changes to successful RPC results or consensus behavior. Missing input data now returns a normal RPC error. ## 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 Top commit has no ACKs. Tree-SHA512: d85a94f78763e41d8be33db80e64045d50a01cd02d32da8fd000a9339d8b79708c92613ed714ffc4e48b5a1c6b69b8e126cad9535b05dc0f149337257fb6667c (cherry picked from commit a94bafc)
86ba039 fix(wallet): authenticate unlock deadline extensions (pasta) Pull request description: ## Issue being fixed or feature implemented `CWallet::Unlock(SecureString)` returned `true` without checking the passphrase whenever the encrypted wallet was already fully unlocked. `walletpassphrase` treats that result as authentication and replaces the relock deadline and timer. So while a wallet is fully unlocked, a caller allowed to use `walletpassphrase` could keep it unlocked past the owner's deadline with any nonempty wrong passphrase. Spending or exporting keys after the original deadline still needs those RPC permissions too. A locked or mixing-only wallet cannot be unlocked this way, and RPC authentication is not bypassed. The early return is Dash-specific (added in 78c5ca5, blocked in 0.12.1, reintroduced by ff5540b in 0.16.0.0). Bitcoin Core decrypts the master key on every `Unlock`. ## What was done? - Removed the early return. Every request on an encrypted wallet now derives the key from the passphrase and decrypts the master key, as Bitcoin Core does. - When a master key is already held (full or mixing-only unlock), the decrypted candidate must match it, compared with `TimingResistantEqual`. A mismatch moves on to the next master-key entry. - This matters because the already-unlocked path skips `CheckDecryptionKey`, and a blank wallet passes that check trivially. Without the comparison, a wrong passphrase that happens to produce valid padding would still be accepted. - An authenticated request on a fully unlocked wallet is a no-op success. The metadata and descriptor-cache upgrades are not repeated. - The mixing-only → full upgrade path is unchanged, and so are the RPC timer logic, full → mixing rejection and the unencrypted-wallet result. ## How Has This Been Tested? macOS arm64, depends build. - **Regression test.** `wallet_encryption.py` now ends with one deterministic check under the mocktime the test already sets: with the wallet fully unlocked for 100 seconds, `walletpassphrase` with a wrong passphrase and a 1000-second timeout fails with `-14`, and `unlocked_until` does not change. Against an unpatched `wallet.cpp` the check fails ("No exception raised"). With the fix it passes. - `wallet_encryption.py --legacy-wallet` and `wallet_encryption.py --descriptors` both pass. - `test_dash --run_test=bip39_tests,wallet_crypto_tests` passes, and `lint-python.py` is clean. - All `Unlock(SecureString)` callers were checked: Qt unlock dialog and wallet controller, createwallet, EncryptWallet, migration and upgradetohd. Limits: - The valid-padding mismatch guard was reviewed in the source, not fault-injected. A wrong passphrase gets past decrypt padding only about 1 in 256 times, so a functional test cannot exercise the guard deterministically. - A *locked* blank encrypted wallet still accepts about 1 in 256 wrong passphrases. That gap already existed and Bitcoin Core has it too. Closing it needs a stored key check, so it is out of scope here. ## Breaking Changes None. ## 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: 18f0978db2da05fb2b658b83c0796fafc758859f27cc777d9f7b85196339f0824f128e94ad552feca7b903a3f387de61957e2900ae2995dace349cea9536b190 (cherry picked from commit 2630930)
…as repair errors 581a219 fix(evo): treat missing snapshots during evodb repair as repair errors (pasta) Pull request description: ## Issue being fixed or feature implemented When the evodb is missing a masternode list snapshot record, the automatic evodb verify/repair at startup (added in dashpay#6999) does not fail. It logs a warning, reports success and marks the database as repaired, so the check never runs again. `RecalculateAndRepairDiffs` treats a missing snapshot as unrecoverable. It stops processing and emits `CRITICAL: Snapshot missing at height N. This cannot be repaired by this tool - full reindex required.` That message goes into `verification_errors`, not `repair_errors`. The startup caller only treats `repair_errors` as fatal, so for a missing snapshot it: - prints `WARNING: Verification errors:` and then `Successfully repaired 0 masternode list diffs`; - calls `CompleteRepair()`, which sets `DB_LIST_REPAIRED`, so every later start logs `Masternode list diffs are already repaired` and skips the check, unless the debug-only `-forceevodbrepair` is passed; - discards any diffs it already recalculated for earlier snapshot pairs, because of the early return, and never checks the pairs after the missing snapshot. The `evodb repair` RPC has the same problem. It returns an empty `repairErrors` array, although the help text defines a non-empty `repairErrors` as "full reindex required". This only affects nodes whose own evodb is already missing a snapshot record. ### Why this is a real problem Code path at c14104b: - A missing `from` snapshot (except the initial DIP0003 one) and a missing `to` snapshot are both pushed to `verification_errors`, followed by `return result;` before `WriteRepairedDiffs`: [deterministicmns.cpp#L1245-L1266](https://github.com/dashpay/dash/blob/c14104b172a2/src/evo/deterministicmns.cpp#L1245-L1266), [#L1290-L1293](https://github.com/dashpay/dash/blob/c14104b172a2/src/evo/deterministicmns.cpp#L1290-L1293) - Startup only logs `verification_errors`, shuts down only on `repair_errors`, and otherwise calls `CompleteRepair()`: [init.cpp#L2517-L2533](https://github.com/dashpay/dash/blob/c14104b172a2/src/init.cpp#L2517-L2533) - `CompleteRepair()` persists `DB_LIST_REPAIRED`, and once it is set startup skips the check: [deterministicmns.cpp#L1298-L1310](https://github.com/dashpay/dash/blob/c14104b172a2/src/evo/deterministicmns.cpp#L1298-L1310), [init.cpp#L2491](https://github.com/dashpay/dash/blob/c14104b172a2/src/init.cpp#L2491) - RPC contract for `repairErrors`: [rpc/evo.cpp#L2132](https://github.com/dashpay/dash/blob/c14104b172a2/src/rpc/evo.cpp#L2132) End-to-end reproduction on regtest. I mined 1200 blocks, so snapshots exist at heights 432, 576 and 1152. I stopped the node, deleted the `("dmn_S3", <hash of block 576>)` record from `evodb` with a small leveldb tool, then restarted twice. <details> <summary>Reproduction with a <code>dashd</code> built from c14104b (unfixed)</summary> ``` == [unfixed] mine 1200 regtest blocks (snapshots at DIP0003 height 432, 576 and 1152) tip height: 1200 block 576: 0aa9baa5c36d2f2f357c320bc33c7893677b60681fc2d3dae17faa0705bbc917 == [unfixed] remove the masternode list snapshot record at height 576 from evodb snapshot record existed=1 (41 bytes), delete=OK == [unfixed] restart #1 (startup evodb verify/repair runs) node state after restart #1: RUNNING Verifying and repairing masternode list diffs... WARNING: Verification errors: CRITICAL: Snapshot missing at height 576. This cannot be repaired by this tool - full reindex required. Successfully repaired 0 masternode list diffs, verified 0 snapshots in 0s == [unfixed] restart #2 node state after restart #2: RUNNING Masternode list diffs are already repaired ``` `evodb repair` on the same node: ``` $ dash-cli evodb repair { "startHeight": 432, "stopHeight": 1200, "diffsRecalculated": 0, "snapshotsVerified": 0, "verificationErrors": [ "CRITICAL: Snapshot missing at height 576. This cannot be repaired by this tool - full reindex required." ], "repairErrors": [ ] } ``` </details> The new unit test also fails on the unfixed code: ``` $ src/test/test_dash --run_test=evo_dip3_activation_tests/evodb_repair_missing_snapshot_is_repair_error Running 1 test case... test/evo_deterministicmns_tests.cpp:3559: error: in "evo_dip3_activation_tests/evodb_repair_missing_snapshot_is_repair_error": check result.verification_errors.empty() has failed test/evo_deterministicmns_tests.cpp:3560: fatal error: in "evo_dip3_activation_tests/evodb_repair_missing_snapshot_is_repair_error": critical check result.repair_errors.size() == 1U has failed [0 != 1] *** 2 failures are detected in the test module "Dash Core Test Suite" ``` ### Why it matters The automatic startup check exists to find and fix bad masternode list diffs. When it hits a missing snapshot, the operator is told the repair succeeded, the database is marked as repaired, and the check is turned off for later starts. Diffs before the missing snapshot that the repair already found to be bad stay bad on disk, and nothing after it is checked. The node keeps running on evodb data the repair itself rated "full reindex required". Other nodes are not affected. ## What was done? In `RecalculateAndRepairDiffs`, both missing-snapshot errors go into `repair_errors` when `repair` is true, and into `verification_errors` as before when `repair` is false. The message text is unchanged. The existing startup path then does what it already does for other unrecoverable repair failures. It logs `Failed to repair masternode list diffs. Database corruption detected. Please restart with -reindex to rebuild the database.` with the error, calls `StartShutdown()`, and returns without calling `CompleteRepair()`. `evodb repair` reports the error under `repairErrors`, and `evodb verify` still reports it under `verificationErrors`. Added a regression test, `evodb_repair_missing_snapshot_is_repair_error`, to `src/test/evo_deterministicmns_tests.cpp`. It mines past two snapshot periods and first checks that the intact range verifies without errors. It then erases the snapshot record at height 576 and checks that repair mode reports it in `repair_errors`, both when the missing snapshot ends a pair and when it starts one. Finally it checks that verify-only mode still reports it in `verification_errors`. ### Why this is the correct minimal fix The root cause is that this condition was put in the wrong error category. Everywhere else in this function, an error that means "cannot repair, full reindex required" goes into `repair_errors`, which is what both callers act on. Moving the error there reuses the existing startup handling and the documented RPC contract, so `init.cpp`, the RPC code and `RecalcDiffsResult` need no change. Alternatives considered: - Skipping `CompleteRepair()` while letting the node keep running would also need a separate signal, either a new result field or a check in init. It would also rerun a check that cannot succeed on every start without telling the operator what to do. - Writing the diffs already recalculated before returning would not help. The snapshot still can't be rebuilt, and the operator still has to reindex. Intentionally unchanged: - The other early returns (`Stop height ... below DIP0003`, `Could not find starting snapshot`, `Need at least 2 snapshots`) stay in `verification_errors`. They are normal on short chains. - A missing initial snapshot at the DIP0003 height is still replaced by an empty list, as before. ## How Has This Been Tested? macOS arm64, `--enable-debug --enable-werror`, depends build. - `src/test/test_dash --run_test=evo_dip3_activation_tests/evodb_repair_missing_snapshot_is_repair_error`: fails without the fix (output above) and passes with it. - `src/test/test_dash --run_test=evo_dip3_activation_tests`: all 41 test cases pass with the fix. - I re-ran the regtest reproduction above with a `dashd` built with this change. <details> <summary>Same reproduction with this change</summary> ``` == [fixed] restart #1 (startup evodb verify/repair runs) node state after restart #1: NOT RUNNING Verifying and repairing masternode list diffs... Failed to repair masternode list diffs. Database corruption detected. Please restart with -reindex to rebuild the database. CRITICAL: Snapshot missing at height 576. This cannot be repaired by this tool - full reindex required. Shutdown: done == [fixed] restart #2 node state after restart #2: NOT RUNNING Verifying and repairing masternode list diffs... Failed to repair masternode list diffs. Database corruption detected. Please restart with -reindex to rebuild the database. CRITICAL: Snapshot missing at height 576. This cannot be repaired by this tool - full reindex required. Shutdown: done == [fixed] restart with -reindex after the repair failure tip height after reindex: 1200 Skipping evodb repair during reindex $ dash-cli evodb verify { "startHeight": 432, "stopHeight": 1200, "diffsRecalculated": 0, "snapshotsVerified": 2, "verificationErrors": [ ] } ``` RPC output with this change on a node that is missing the snapshot: ``` $ dash-cli evodb verify { ... "snapshotsVerified": 0, "verificationErrors": [ "CRITICAL: Snapshot missing at height 576. This cannot be repaired by this tool - full reindex required." ] } $ dash-cli evodb repair { ... "snapshotsVerified": 0, "verificationErrors": [ ], "repairErrors": [ "CRITICAL: Snapshot missing at height 576. This cannot be repaired by this tool - full reindex required." ] } ``` </details> - `test/lint/lint-whitespace.py` and `test/lint/lint-includes.py` pass, and `clang-format-diff.py` is clean on the new test code. ## Breaking Changes A node whose evodb is missing a masternode list snapshot now shuts down at startup and asks for `-reindex` until it is reindexed. Previously it kept running and marked the evodb as repaired. `evodb repair` now reports this condition in `repairErrors` instead of `verificationErrors`. Healthy nodes are unaffected. ## 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 1cc63dc64c8e0ef18899150d39d40529dd630246853f983f080e91078d88c1940c04c243c9deddbbe21115932e0324e9d7133c5eb1235e494853675aadebe3d8 (cherry picked from commit a0950fd)
…ilures 973f59e fix: relock encrypted wallets after HD upgrade failures (pasta) Pull request description: ## Issue being fixed or feature implemented An encrypted wallet temporarily unlocked by `upgradetohd` remained fully unlocked when mnemonic validation or later HD setup threw. An already authenticated caller with signing/spending permissions could then use existing keys until another lock event. This does not bypass RPC authentication. ## What was done? Install the existing cleanup handler before the temporary unlock and relock on every subsequent exit, including success, while the wallet locks are still held. Keep early validation behavior and relock before any rescan. ## How Has This Been Tested? On macOS arm64 with prebuilt depends, built baseline and candidate with `make -j1`. The added regression fails on the baseline in both legacy and descriptor modes because signing succeeds after a rejected upgrade. Full `wallet_upgradetohd.py --legacy-wallet` and `--descriptors` pass with the fix, covering signing denial after invalid mnemonic, legacy overlength passphrase and successful upgrade, plus verified signatures after explicit unlock. Existing recovery/rescan tests remain covered by those full runs. Ran `test_dash --run_test=bip39_tests,wallet_crypto_tests`, changed-file Python lint using the repository's enabled rules, and `git diff --check`. An independent four-pass review approved the final source and inspected the reproduced baseline evidence; its remaining cache/test suggestions were resolved by the complete reruns. No fund movement or RPC authentication bypass was demonstrated. ## Breaking Changes None. ## 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 This pull request was created by Codex. Top commit has no ACKs. Tree-SHA512: 01898d5180b07590481c09119cdcc7fcdc0cbefd347d1a938348c42b3471041641fe68b3b065551ec2bf48dd1bed8f9cd9fc237261632245e679155c446520a9 (cherry picked from commit 66d9b6a)
9178483 fix: validate Electrum import input boundaries (pasta) Pull request description: ## Issue being fixed or feature implemented `importelectrumwallet` reads an exact-sized JSON file allocation through UniValue's pointer-only parser, which determines length using `strlen`. A full read supplies no terminator. It also accepts negative rescan indices, imports keys and then passes the negative height to an exact chain lookup that aborts the node. Both paths require an authorized RPC caller, a readable local export and an unlocked legacy wallet. Method-limited deployments make the negative-index abort relevant beyond an administrator's existing ability to stop a node. ## What was done? Use a binary, length-managed string for JSON input, validate file size/read errors, resize to the actual byte count and invoke the bounded parser overload. The string also supplies the parser's sentinel. Reject a negative rescan index before processing the export or installing keys; preserve the existing clamp for indices above the tip and document the non-negative argument. Extend `wallet_dump.py` to reject a valid JSON prefix followed by embedded NUL/trailing bytes, check negative indices for JSON and CSV without importing keys, and retain valid bare/p2pkh-prefixed WIF imports at zero, tip and above-tip indices. Verify the imported private keys through dump/sign/verify RPCs and unload the temporary wallets before the existing dumpwallet scenarios. ## How Has This Been Tested? On macOS arm64 with BDB and SQLite enabled: full `make -j1`, the complete `wallet_dump.py --legacy-wallet --portseed=1`, a separate negative-index node-liveness regression (both serially at TEST_RUNNER_PORT_MIN=12000), 31 wallet/walletdb unit cases, and both UniValue object/data test executables passed. Python lint with the repository selection, assertion/include/whitespace lints and `git diff --check` passed. The UniValue data executable requires `src` as its working directory; an initial root-directory invocation failed to find its fixtures and the corrected invocation passed. Independent four-pass review found no blocking source defects; the final transferred patch exactly matches the reviewed file/diff hashes. Baseline defensive RPC regressions accepted the embedded-NUL JSON prefix, and a separate disposable regtest negative-index RPC terminated the node at the production `getBlockHash` assertion. The earlier isolated actual-UniValue ASan proof demonstrates the allocation/parser primitive, not a full-daemon disclosure or code-execution outcome. No deployed restricted account or production crash probability was measured. Stream-error injection, concurrent file modification and exact rescan-start observation were not tested. ## Breaking Changes Negative rescan indices now return RPC_INVALID_PARAMETER before import. JSON bytes after an embedded NUL are parsed and rejected instead of silently ignored. Valid export formats and the above-tip clamp remain supported. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone This pull request was created by Codex. Top commit has no ACKs. Tree-SHA512: b5809feb6b963b43c9e7d567bb7991c1695b35b2415f33c19afe54811c4098be1d7d751181019476cbb3101383e265c2d4916797bacc9c05be47a2166ef9c924 (cherry picked from commit bda514a)
…t data 2226682 fix: return REST errors for unavailable deployment data (pasta) Pull request description: ## Issue being fixed or feature implemented REST deployment queries call the deployment RPC directly. When EHF reconstruction needs an unavailable block body, its exception can escape the HTTP worker. This requires REST to be enabled and the client to pass the HTTP address ACL; REST is disabled by default. ## What was done? Translate deployment RPC exceptions into a plain-text HTTP 500 response before writing successful response headers. Valid requests retain the existing JSON response. The change leaves deployment rules and reconstruction unchanged. ## How Has This Been Tested? On macOS arm64 with prebuilt depends: `make -j1`, `test_dash --run_test=rpc_tests`, `interface_rest.py` with an isolated port range, selected Python lint, whitespace lint, circular-dependency lint, and `git diff --check` passed. The functional test requests a known post-V20 header without its body, checks the error and continued RPC/REST responses, then supplies the body and checks the successful response. The first runner attempt failed during cache startup because an HTTP port was occupied; the isolated retry passed. Independent final four-pass review completed. No pre-fix daemon termination or external exposure demonstration was performed, and the test is not claimed as a before/after crash reproduction. ## Breaking Changes Unavailable deployment reconstruction returns HTTP 500 instead of allowing an exception to escape the callback. ## 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 This pull request was created by Codex. Top commit has no ACKs. Tree-SHA512: 79824586e1311ca3d23b110b5cff11c75f1afdd6f42103cd802dce7fa6b990934f37bc86d5ffd6d187d76a4d3812f5aa67dcc2cde96f75b6a18a2dae8c2062fe (cherry picked from commit b258eab)
…ssphrases 6861fa5 fix: preserve wallet permissions when changing passphrases (pasta) Pull request description: ## Issue being fixed or feature implemented A successful `walletpassphrasechange` during a CoinJoin mixing-only unlock silently left the wallet fully unlocked. An already authenticated caller with signing/spending/export permission could use keys beyond the owner's prior mixing restriction. This does not bypass RPC authentication, and the existing unlock timer still bounds the interval. ## What was done? Capture the original locked/mixing mode under the wallet's existing locks. Unlock a mixing-only wallet as mixing-only so it stays that way, and relock wallets that started locked on every exit path via a cleanup handler. Successful maintenance preserves full unlock where it was already allowed; incorrect-old-passphrase behavior, encryption format and unlock timers remain unchanged. ## How Has This Been Tested? Built on macOS arm64 with prebuilt depends. The added regression fails against baseline in both wallet backends because signing succeeds after changing a mixing-only wallet's passphrase. `wallet_encryption.py --legacy-wallet` and `--descriptors` pass with the fix. The regression runs a locked/mixing/full input table: after `walletpassphrasechange`, it checks `signmessage` (with signature verification) and `coinjoin start`/`stop` authorization for each mode. Existing encryption, passphrase-change, null-byte and timeout-bound coverage in `run_test` is unchanged. Ran `test_dash --run_test=bip39_tests,wallet_crypto_tests`, Python lint, whitespace lint and `git diff --check`. ## Breaking Changes None. Successful passphrase changes retain the existing wallet permissions. ## 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 Top commit has no ACKs. Tree-SHA512: 66b98c25a9fdb88366367ad91a64490c63ec4e8b4d89506c132f6baf833b0312bf4e321a0204045babb0d66e3e56f425469099fd7826ea480c3a67986dfe8e12 (cherry picked from commit 8cb2c95)
…alidation a819870 fix: hold chainstate lock during CoinJoin input validation (pasta) Pull request description: ## Issue being fixed or feature implemented CoinJoin input validation obtained a pointer to the shared chainstate coins cache under `cs_main`, then released that lock before reading the coins. Confirmed-coin lookups can populate the cache, so those reads could race block connection, disconnection or flushing. CoinJoin is opt-in for clients and requires an active mixing session on coordinators. The source lock-contract violation is established; no race, crash or state corruption has been reproduced. ## What was done? Hold `cs_main` for the complete `IsValidInOuts()` input-validation path, including every `CCoinsViewMemPool::GetCoin()` fallback to the shared cache. The RAII guard also covers early returns and releases before callers perform wallet or session cleanup. This follows the existing locking used by collateral validation. ## How Has This Been Tested? macOS arm64 with the prebuilt depends prefix, configured with `--without-gui --disable-bench --disable-fuzz-binary`: - `make -j1` passed. - `test_dash --run_test=coinjoin_inouts_tests,coinjoin_tests,coinjoin_dstxmanager_tests,coinjoin_queue_tests --log_level=message` passed (82 cases). - `test_runner.py -j1 --portseed=1 rpc_coinjoin.py` passed with the assigned `TEST_RUNNER_PORT_MIN=20000`, along with the runner's framework unit tests. - Include, whitespace and circular-dependency linters and `git diff --check` passed. Independent four-pass review checked the shared-cache access, caller lock ordering, mempool and InstantSend nesting, and error paths. No blocking issue was found. No timing-sensitive regression test was added; no TSAN, scheduled race or crash consequence is claimed. GUI and other build configurations were not validated locally. ## Breaking Changes None. No wire-format, wallet-format, fee-policy or consensus-rule change. ## 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 This pull request was created by Codex. Top commit has no ACKs. Tree-SHA512: bab0dbd4041ab10bd6387820b27c37b0d8a47d658213e84337c6a1d8925395b8e63680c79057d16d769e33e8d42ad1da4f9af1d700a39b2afc0e2c71cb0e072b (cherry picked from commit 3d18d42)
ecae783 fix: authenticate cached InstantSend proof bytes (pasta) Pull request description: ## Issue being fixed or feature implemented When an ISDLOCK arrives for a transaction whose recovered signature this node already has, the verifier skipped checking the lock's own signature and cycle. The cached recovered signature proves that the quorum signed this txid. It does not prove that the bytes in the received lock are that signature. The unchecked lock was then stored, relayed by its hash and returned by `getislocks`. A lock with the same txid that arrives later is rejected once one is stored. So a peer that sends an on-curve but invalid signature first, or a different known cycle hash, can keep the authentic proof from being stored and relayed. This does not authorize any new spend or cause a consensus split. It needs the recovered signature to be cached while no lock is stored, for example on a `-watchquorums` node. ## What was done? `BuildVerificationBatch` now resolves the quorum from the received lock's cycle before it consults the cache. It skips verification only when the cached recovered signature came from that same quorum and has the same signature bytes. Any other lock goes through normal batch verification. `missing_quorum_does_not_drop_batch` now uses a real mined non-rotating quorum, because a cache hit now needs a quorum that can be resolved. Rotating missing-quorum coverage stays in `received_genesis_cycle_has_no_quorum`. This is a Dash-specific fix. Bitcoin Core has nothing to backport. ## How Has This Been Tested? macOS arm64, existing depends build: - Full `make -j1` passed. - `test/functional/p2p_instantsend.py` passed. It uses non-rotating quorums. - `test/functional/rpc_verifyislock.py` passed. It uses rotating quorums, so masternodes that already hold the recovered signature receive the network locks. - `lint-includes.py`, `lint-circular-dependencies.py` and `git diff --check` passed. Linux x86_64 debug build, after the tests were simplified: - `./src/test/test_dash --run_test=evo_islock_tests` passed all 14 cases, on 3 runs in a row. New test `cached_recovered_sig_does_not_authenticate_other_lock`: - It adds mined commitments for two non-rotating quorums and caches a genuine recovered signature for a mempool transaction. It then passes a changed lock straight to `ProcessPendingInstantSendLocks`: once signed by another key, and once with the other cycle hash. It checks that the changed lock is not stored, and that the authentic lock processed afterwards is stored byte for byte. - With `net_instantsend.cpp` reverted to the base, both cases failed: the changed lock was stored and the authentic lock was not (4 failed checks, exit 201). An independent review approved the change. Beyond `rpc_verifyislock.py`, rotating-quorum cache hits are covered by reasoning, not by a dedicated test. No end-to-end P2P timing demonstration was done. ## Breaking Changes None. The wire format, quorum selection and signature validity rules are unchanged. ## 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: ce382fe64d1a7fd18a6199d4fbde73ca488857bdc08f855f927694cbab645ed20a6376df587b8ba7adea8a390ce103d07ff0b4e77f5f0a6d3ce40f1817994e05 (cherry picked from commit 524173e)
… the governance 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)
…nance trigger path a551177 fix: update masternode rate limit on failed governance trigger path (pasta) Pull request description: ## Issue being fixed or feature implemented In `AddGovernanceObjectInternal()`, a trigger object is emplaced into `mapObjects`, then `AddTrigger()` is attempted. On failure the function calls `PrepareDeletion()` and returns early, before `MasternodeRateUpdate()`. `MasternodeRateCheck()` allows any masternode that has no entry in `mapLastMasternodeObject`, and only `MasternodeRateUpdate()` creates that entry. A masternode that never lands a *successful* trigger therefore never gets a rate-buffer entry, and every malformed but signed trigger it sends passes the rate check. That defeats the limiter meant to stop exactly this. Each rejected trigger still costs a BLS verification, a `mapObjects` entry held for the deletion delay, and, after erasure, a `mapErasedGovernanceObjects` entry retained for about two superblock cycles (roughly 60 days on mainnet). Sending one needs a valid operator key for a masternode in the tip list, and the victim must have requested the hash after an INV. Rejected triggers are not relayed, so there is no fan-out; the attacker has to connect to each victim directly. ## What was done? - Call `MasternodeRateUpdate()` before the `AddTrigger` check, so failed attempts advance the masternode's rate buffer too. - `MasternodeRateUpdate()` also queued near-future triggers for the deferred re-announcement in `CheckPostponedObjects`. That part is split out into `ScheduleTriggerRelay()` and still runs only after `AddTrigger` succeeds. Otherwise moving the rate update would make the node re-announce, and serve on GETDATA, a trigger it had just undone and marked for deletion. ### Scope note: creation-time rate limiting on short-cycle networks The rate buffer stores the object's signer-chosen creation time, and `MasternodeRateCheck` accepts creation times from `now - 2 * cycle` to `now + 1h`, with a maximum rate of `2.2 / cycle` over five entries. On mainnet (cycle = 16616 blocks, about 28.8 days) the widest possible span is about `2 * cycle`, so a full buffer is always rejected (`5 / (2 * cycle)` = `2.5 / cycle` > `2.2 / cycle`). On testnet, devnet (1 h cycle) and regtest the accepted window is wider than `5 / 2.2` cycles, so an operator who alternates creation times between the window ends can stay under the rate. That weakness is in the existing limiter and applies equally to well-formed triggers today. This PR only brings failed triggers under the same limiter. Moving the limiter to receipt time would change how valid triggers are throttled and is left for a separate change. ## How Has This Been Tested? Two new cases in `src/test/governance_vote_processing_tests.cpp`, using its existing chain-backed fixture, which already registers a masternode with a known operator key. No new file or fixture, and `governance_inv_tests` is unchanged. - `failed_triggers_advance_the_masternode_rate_limit`: submits 12 signed triggers that fail `AddTrigger` and checks that exactly `RATE_BUFFER_SIZE - 1` (4) are stored. It fails with `12 != 4` on develop's `governance.cpp`/`governance.h`. - `failed_trigger_is_not_scheduled_for_deferred_relay`: a failed trigger created inside the deferred-relay window is never announced, even after it ages past `RELIABLE_PROPAGATION_TIME`. It fails when `ScheduleTriggerRelay()` is called before `AddTrigger`, which is the regression the split prevents. Run locally on macOS arm64 with `--enable-debug --enable-werror`: - `src/test/test_dash --run_test=governance_vote_processing_tests,governance_inv_tests,governance_superblock_tests,governance_validators_tests` passes. - `test/functional/test_runner.py feature_governance.py feature_governance_objects.py feature_governance_cl.py` passes. - `test/lint/lint-whitespace.py` passes. ## Breaking Changes None. ## 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: 2892edb480e2c3c63c1be7d231d745ecea2d2dcf6161f48ec0cd3f3e6484a120af77de540db06e3e7892840dc6a1fb234df98025686fb27a121731a7996338ac (cherry picked from commit 8db4b76)
2c12027 fix: stop `masternode payments` at the genesis block (pasta) Pull request description: ## Issue being fixed or feature implemented `masternode payments` terminates dashd when it reaches the genesis block. For every block it visits, it reads the masternode list of the previous block with `GetListForBlock(pindex->pprev)`. That function takes a `gsl::not_null<const CBlockIndex*>`, so the genesis block's null `pprev` calls `std::terminate()`. Any RPC user can reach it in three ways: - `masternode payments <genesis hash>` - a negative count that walks back past block 1, e.g. `masternode payments <block 1 hash> -2` - `masternode payments` with no arguments on a chain still at height 0 ## What was done? Payees for a block are chosen from the previous block's list, so the genesis block has no masternode payments. The loop now stops at the genesis block. Asking for the genesis block returns an empty array, and a backwards walk ends at block 1. ## How Has This Been Tested? - Reproduced the crash by hand on an unpatched regtest build (`std::terminate() called` in debug.log), both at height 0 and at height 1. - `rpc_masternode.py` gains two assertions: payments for the genesis block, and a backwards walk from block 1. Against the unpatched dashd the test fails at the first of them because the node dies. With the fix it passes. - macOS arm64, depends build. ## Breaking Changes None. Requests that used to crash the node now return results. ## 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) ACKs for top commit: knst: utACK 2c12027 Tree-SHA512: 5b0a7646cde26c0c6de1a3341847e0619718acacbf392fc15e4cee034ab0bd680dd263d69d6937b1db0b43b9c1ae776cd9a1b5c5eaaffecd6d812dfbc18b7647 (cherry picked from commit 1aed5c3)
…de the active chain ec93e69 fix: ProcessBlock now stores replaces existing items in cache of mn-lists (Konstantin Akimov) 0f5b8d6 fix: refuse masternode list RPCs for blocks outside the active chain (pasta) Pull request description: ## Issue being fixed or feature implemented If an RPC asks for the masternode list at a block whose header the node has accepted but which it has never connected, the node caches a wrong list. It then rejects the next valid block and stalls. `CDeterministicMNManager::GetListForBlockInternal()` walks back from the requested block looking for a list snapshot or diff in EvoDB. If it finds neither, it treats that block as the initial snapshot: it caches an empty list under the block's hash and points `m_initial_snapshot_index` at it. A block that was never connected has no diff, so it lands in that fallback. When the block is connected later, `ProcessBlock()` adds the real list with `emplace`, which keeps the existing entry, so the empty list stays cached. The next block is then built on an empty list and rejected with `bad-cbtx-mnmerkleroot`. Steps on regtest with registered masternodes: 1. `submitheader` block N 2. `protx info <proTxHash> <hash N>` (returns "not found" because the list it read is the fabricated empty one) 3. `submitblock` block N 4. A correct block N+1 from a peer is rejected with `bad-cbtx-mnmerkleroot` The rejection is saved as `BLOCK_FAILED_VALID` in the block index. A restart drops the bad cache entry, but the block is still marked invalid, so the node stays stuck until someone runs `reconsiderblock`. I confirmed this on an unpatched build: after the restart, `getchaintips` still lists the peer's block as `invalid`. Three stock RPCs can reach this, all of which need an authenticated RPC user: - `protx info` with a block hash - `protx listdiff` - `masternode payments`, when it is given a block that is stored but whose parent is header-only `protx diff` and the P2P `getmnlistd`/`getqrinfo` handlers are not affected. They go through `BuildSimplifiedMNListDiff()`, which already requires both blocks to be in the active chain. ## What was done? - `protx info`, `protx listdiff` and `masternode payments` now return `RPC_INVALID_PARAMETER` ("Block is not in main chain") for a block outside the active chain. This is the same rule `protx diff` already follows. - `CDeterministicMNManager::ProcessBlock()` now stores each connected block's list with `insert_or_assign` instead of `emplace`. Whatever an earlier lookup cached under that block hash, connecting the block replaces it with the correct list. This closes the cache poisoning for any caller, not only these three RPCs; the RPC checks stay so callers get an error instead of a made-up empty list. ## How Has This Been Tested? - The new section in `rpc_masternode.py` follows the steps above: `generateblock` with `submit=False`, `submitheader`, then calls to the three RPCs at that hash. It then submits the block and mines the next one. - Unpatched dashd: the test fails, because the RPCs still answer. - Patched dashd: the test passes. - A separate probe run on the unpatched dashd went further. The node rejected a peer-mined block N+1 with `bad-cbtx-mnmerkleroot`, and still showed it as `invalid` after a restart. - Also passed on the patched build: - `feature_dip3_deterministicmns.py --descriptors` (uses `protx info` and `protx listdiff` at historical active-chain blocks) - `feature_llmq_simplepose.py` and `feature_llmq_simplepose.py --disable-spork23` - `rpc_quorum.py` - `evo_dip3_activation_tests/dip3_activation_legacy` now submits a block's header, reads the list for it, then connects the block and checks the tip list contains the newly registered masternode. With `emplace` restored the check fails (the stale empty list is still cached); with `insert_or_assign` it passes. - macOS arm64, depends build. ## Breaking Changes `protx info <proTxHash> <blockHash>`, `protx listdiff` and `masternode payments <blockHash>` now return an error for a block outside the active chain, including stale-fork blocks that were connected at some point. Before, they answered for any known block. ## 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) ACKs for top commit: knst: self-ACK ec93e69 Tree-SHA512: aabc1c038cf43f6c601b93d81c4d2dac6b2d6f38796730b0d9fdd5ccdd38a687fa6d4d18df19473d1bff5c03ac5ba1d9938b705b6986b7969f140533e48487e2 (cherry picked from commit 1cc3a63)
…se of purpose strings with an enum 57bd6a1 Merge bitcoin#27217: wallet: Replace use of purpose strings with an enum (fanquake) Pull request description: ## Issue being fixed or feature implemented Instead of storing and passing around fixed strings for the purpose of an address, use an enum. This backport bitcoin#27217 is touching multiple wallet's files and method signatures and skipping it for v24 will block many other backports until v25. Let's merge it in v24.0 better. ## What was done? Address Purposes strings are now restricted to the currently known values of "send", "receive", and "refund". Wallets that have unrecognized purpose strings will have loading warnings, and the `listlabels` RPC will raise an error if an unrecognized purpose is requested. ## How Has This Been Tested? See changes in tests ## Breaking Changes Wallet ------ * Address Purposes strings are now restricted to the currently known values of "send", "receive", and "refund". Wallets that have unrecognized purpose strings will have loading warnings, and the `listlabels` RPC will raise an error if an unrecognized purpose is requested. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone Top commit has no ACKs. Tree-SHA512: e56c86181d4f72721621e2bc69c312c09e8f6a36f5d2205eb663f1884551f87bf365cbdace89a277bf03051c6da1cd6662debba5d47a21dda608011c8aee1bb5 (cherry picked from commit c8bfd7a)
…es with unchanged bytes in state diffs c9552ef test: cover operator key scheme change with unchanged bytes in state diffs (UdjinM6) 020ddd1 fix(evo): capture operator key scheme changes with unchanged bytes in state diffs (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Syncing mainnet from scratch on recent `develop` fails at block 2005288 (`000000000000004f95a6f3b181270e2644ab76abd14bb9402cca7a1f3f3eea93`) with `bad-cbtx-mnmerkleroot`. Block 2005287 has a ProUpRegTx that moves masternode `af40de...9024` from LegacyBLS to BasicBLS. The new basic-scheme operator key has the same bytes as the old legacy key. Those bytes decode to different keys under the two schemes. `CDeterministicMNStateDiff` decided whether `pubKeyOperator` changed by comparing `GetHash()`, which only covers the encoded bytes. So the stored diff carried the new `nVersion` but not the key. A list built online is correct. A list reconstructed from the stored diff keeps the legacy-scheme key under `nVersion=2`. Its SML entry then serializes the key as zeros, and the merkle root in the next block's CbTx doesn't match. Recent masternode list cache eviction changes exposed it during mainnet sync by forcing reconstruction from diffs. ## What was done? - `CDeterministicMNStateDiff` now also records `pubKeyOperator` when the BLS scheme implied by `nVersion` changes, not only when the encoded bytes change. The diff format and field layout are unchanged; the existing field is just filled in one more case. - The existing same-key re-encoding test is now table-driven, with the mainnet block 2005287 key added as a second case. Nodes that already stored the bad diff need `-reindex` to rebuild it with the fixed code. `-reindex-chainstate` also rebuilds evodb, but it keeps the block index, so nodes that already rejected block 2005288 keep it marked invalid. ## How Has This Been Tested? Unit tests, mutation check and lint ran on macOS 27 arm64, configured with `--enable-debug --enable-werror --enable-crash-hooks` against depends, at `c9552ef5f9`. - `./src/test/test_dash --run_test=evo_dip3_activation_tests` passes. Removing the new scheme check from the fix makes only the new "same bytes, different key" case fail. The existing case still passes. - Before the fix, instrumenting a mainnet sync confirmed the diagnosis: the list rebuilt from the diff at 2005287 had the masternode with a legacy-scheme key under `nVersion=2`, and the list built online did not. - A full mainnet `-reindex` with the fixed binary synced past 2005288 with no new issues. - `test/lint/all-lint.py`: the only failure is an existing cppcheck `knownConditionTrueFalse` warning at `src/governance/object.cpp:856`, which this PR doesn't touch. ## Breaking Changes None. Diffs written by the fixed code use the existing format. Nodes that already stored the bad diff need `-reindex` (see above). ## Checklist: - [x] I have performed a self-review of my own code - [ ] 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: 77d9d2e794fe3f784c0b82ae5acf3e78d6447d7cc6278b1876439a232a4ca8c3519e061964638c7075307c18ccfda6cd165d900b62c8e902d0ae079ba9048a33 (cherry picked from commit 1bc5197)
969c5a5 docs(rpc): qualify ChainLock lookup failures (PastaClaw) 5e1bfe7 docs: omit release note for RPC help clarification (PastaClaw) a088909 docs: note historical ChainLock help clarification (PastaClaw) d60b248 docs(rpc): clarify historical ChainLock lookup (PastaClaw) Pull request description: ## Issue being fixed or feature implemented Fixes dashpay#7872. The `getchainlockbyheight` help currently makes it easy to expect any ChainLocked height to be available, including the current best ChainLock and heights skipped by embedded ChainLocks. Requested by thephez via PastaClaw. Source: dashpay#7872 ## What was done? - Explain that only ChainLocks embedded in a later active-chain block's coinbase can be returned. - Document that the current best ChainLock is unavailable until embedded, and direct callers to `getbestchainlock` for the live result. - Explain that skipped heights cannot be returned even if they are ChainLocked, and document the exact `Chainlock not found for height` error for completed lookups. - Document that unavailable block data, such as pruned carrier blocks, can cause the lookup to fail instead. No release note is included, as requested in human review for this help-only clarification. Keep the coinbase-backed behavior unchanged. An in-memory best-ChainLock fallback would lack the carrier block required by the existing `cbtx_height` result field, so it is not a clearly better small behavior change for this help fix. ## How Has This Been Tested? On macOS/Apple Silicon: - Reviewed the help against `CoinbaseChainLockReader::Find`, `LoadCarrier`, the active-chain carrier check, and `getbestchainlock`. Also inspected the existing unavailable-carrier-data unit test (not run). - `git diff origin/develop..HEAD --check` and `COMMIT_RANGE=origin/develop..HEAD python3 test/lint/lint-whitespace.py` passed, including the pruning clarification. - `python3 test/lint/lint-format-strings.py` passed again for the pruning clarification. - `./autogen.sh` and minimal wallet/GUI-disabled configuration succeeded. - `make -C src -j2 rpc/libbitcoin_node_a-quorums.o` passed again, compiling the changed RPC translation unit including the pruning clarification. No new regression test was added for this help-only change; the documentation correction is directly reviewable in the diff. No daemon build or functional tests were run. ## Breaking Changes None. RPC lookup behavior, response fields, and error codes are unchanged. ## Checklist: - [x] I have performed a self-review of my own code - [x] 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) <!-- pastaclaw-job: cj-20261013-175557-050a --> Top commit has no ACKs. Tree-SHA512: 92d14b1a8904561a1bbb302802c844a1d3d7a877ef309d6192406b9c690e5fb979355cbc2b91bd36882834349c065ddad9f6c8b7fbb9883ae29c11be7784693e (cherry picked from commit 09174e0)
…viction a01acfd fix: keep another transaction's InstantSend outpoint entry on removal (pasta) d82e327 test: check InstantSend release through the kept transaction ref (pasta) 312fb0d fix: release InstantSend tracking after mempool eviction (pasta) Pull request description: ## Issue being fixed or feature implemented InstantSend tracks every unlocked transaction that enters the mempool. When an ordinary unlocked transaction left the mempool through expiry or size trimming, `TransactionIsRemoved` returned early: only Platform transfers were released. The confirmation cleanup only handles mined entries, so the evicted transaction stayed tracked for good. Its full object, parent links, spent-outpoint index and timing record were all kept outside the mempool size limit. Sustained churn of transactions that are never locked or mined grows this memory without bound. Normal lock delivery limits the exposure in practice. The achievable growth rate has not been measured. ## What was done? - `TransactionIsRemoved` now releases the tracking of any unlocked transaction through the existing `RemoveNonLockedTx` cleanup, which handles parent links, outpoint index and retry entries. It also drops the timing record. - `TransactionIsRemoved` is also called when a peer's transaction is rejected. On a node without txindex, a mined transaction that is not yet locked can be replayed and rejected after a reorg clears the recent-confirmed filter. That transaction's tracking must survive so that a later conflicting lock still finds its block. `RemoveNonLockedTx` therefore takes a `keepMined` flag, and the mined check and the erase happen together under `cs_nonLocked`. - `RemoveNonLockedTx` now erases an outpoint-index entry only if the removed transaction owns it. The index keeps the first tracked spender of each outpoint. Before this, removing an unmined transaction that spends the same input as a tracked mined one, for example one admitted after a reorg, dropped the mined transaction's entry. Eviction now reaches this cleanup for every unlocked transaction. - Platform transfers keep their existing unconditional removal. Locked-transaction handling is unchanged. Block removals never emit this callback. This is a Dash-specific fix. Bitcoin Core has nothing to backport. ## How Has This Been Tested? macOS arm64, existing depends build: - Full `make -j1` passed. - `./src/test/test_dash --run_test=evo_islock_tests` passed all 14 cases. - `test/functional/p2p_instantsend.py` and `test/functional/feature_llmq_is_retroactive.py` passed. - `lint-includes.py`, `lint-include-guards.py`, `lint-circular-dependencies.py` and `git diff --check` passed. New test `nonlocked_ordinary_tx_released_on_mempool_removal`: - It registers the real `NetInstantSend` validation listener and adds standard valid transactions to the mempool. It then expires the mempool and, in a second pass, trims it. The test keeps its own `CTransactionRef` and checks that its `use_count()` drops to 1, so the mempool and the InstantSend tracker have both released the transaction. - With the production code unchanged, both release checks fail (`2 != 1`). - A negative case tracks a transaction as mined and calls the public `TransactionIsRemoved` entry point. It checks that the tracker still holds the transaction and that `RetrieveISConflicts` still reports it at that block. An earlier version of this fix removed mined entries unconditionally, and this case fails against it. The same case then removes an unmined spend of the mined transaction's input and checks that the conflict is still reported. With the old unconditional outpoint erase, that check fails. An independent review approved the change. No sustained churn or memory-exhaustion run was done. The mined negative case uses the manager's public tracking calls, not a mined block or an end-to-end replay. ## Breaking Changes None. Tracking for mined transactions is kept, and locked-transaction and Platform-transfer handling are unchanged. ## 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) ACKs for top commit: knst: utACK a01acfd Tree-SHA512: fbc683cf3b30c3052962c5c803ac34c139c775a6d73e4fb2ab24e9d876da41f063a8a7e22c26bc75d1a111a0428093a9721e71e47f1e3ec98523ccabce369ef3 (cherry picked from commit 552d552)
ce5b786 partial Merge bitcoin#35984: sign: skip signing SIGHASH_SINGLE inputs with no corresponding output (pasta) Pull request description: ## Issue being fixed or feature implemented When a signer explicitly requests `SINGLE` or `SINGLE|ANYONECANPAY` for a PSBT input that has no output at the same index, the legacy signature hash is the constant one, so the signature does not bind the transaction as the signer expects. `SignTransaction()` already skipped such inputs but `SignPSBTInput()` did not. Default `ALL` signing and the wallet's requested-sighash agreement checks limit exposure to explicit nondefault signing. ## What was done? Partial backport of bitcoin#35984 (only test hunks omitted; see below). The missing-output check moves into `MutableTransactionSignatureCreator::CreateSig`, so PSBT and raw transaction signing share the same refusal, and the redundant raw-signing check is removed. Dash adaptations: - Uses `nHashType & 0x1f` with the legacy `SignatureHash`, since Dash lacks `SIGHASH_OUTPUT_MASK` and the upstream signing-options interface. - The regression test is ported for P2PKH only (Dash has no segwit or taproot) and extended with `SINGLE|ANYONECANPAY`, `ALL` and raw-signing controls. - Omitted, hence partial: upstream's `test_psbt_roundtrip`/`test_psbt_version` hunks, which replace `listunspent()[0]` with picking a mature 50-coin coinbase. Both tests arrive with PSBTv2 (bitcoin#21283), which Dash does not have, so there is nothing to apply them to. No ordering-dependent `listunspent()[0]` call runs after the new test in Dash's `rpc_psbt.py`, so no Dash equivalent is needed. ## How Has This Been Tested? On macOS arm64 with the existing depends prefix and `--enable-werror`: - Full `make -j1` passed before and after the change. - The new signing scenario failed on the baseline in both legacy and descriptor modes (input 1 finalized under `SINGLE`) and passes after the fix. - `rpc_psbt.py` passed in both descriptor and legacy variants, including the `SINGLE|ANYONECANPAY`, `ALL` and raw-signing controls. - `src/test/test_dash --run_test=sighash_tests,script_tests,psbt_wallet_tests` passed. - Targeted Python lint, whitespace lint and `git diff --check` passed. ## Breaking Changes The shared signer refuses to create `SINGLE` signatures when the matching output is missing. No consensus or RPC schema changes. ## Checklist: - [x] I have performed a self-review of my own code - [ ] 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: 4848c67cdbb8f53429b1c3f2f7a80aea652426a198947b175137c3a8fe2381ab284cf8086950d4434f6eeafa5caa78822f2b38da8e51b2ae1fadcfe1e969fd61 (cherry picked from commit 3dc3d05)
…tory reads 1f33db3 fix: retain initialized DKG sessions during inventory reads (pasta) Pull request description: ## Issue being fixed or feature implemented DKG round initialization replaces the current session on a phase thread while inventory getters read it from peer processing. The unsynchronized owner can be destroyed during a getter, and the replacement was visible before initialization completed. This fixes that object lifetime race; reliable crashes and stronger exploitation have not been demonstrated. ## What was done? Protect publication and acquisition with one handler mutex, initialize the next session privately, and return shared ownership to inventory and phase callers. Release the publication mutex before session work or retirement destruction. Failed initialization retires prior-round inventory. ## How Has This Been Tested? No new test is added: the race needs a getter running concurrently with a session replacement, which a deterministic unit test cannot reproduce. Verification was code review plus the existing tests: - On macOS arm64 with the configured depends build: `./src/test/test_dash --run_test=llmq_dkg_tests` passed, and `test/functional/test_runner.py -j1 feature_llmq_connections.py` passed, including normal DKG, quorum connections, probes and restarts. - The existing `feature_llmq_*` functional tests pass in CI under TSan (`linux64_tsan-test`) and ASan (`linux64_asan-test`), which exercise the session lifetime path. No baseline mutation or runtime race reproduction is claimed. ## Breaking Changes None. No wire, consensus, activation or persisted-format changes. ## 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 Top commit has no ACKs. Tree-SHA512: 7c1aef760dfb132d781de4c27c50c8f63770a57ed3ceb648061fefccc17ae939c728c88ba0594309b49418b456165f7ad74dfef8377afd4c4bcf99dac0fa3760 (cherry picked from commit c989ec6)
37258c0 Merge bitcoin#29855: psbt: Check non witness utxo outpoint early (pasta) cfdbb32 partial Merge bitcoin#25812: psbt: Avoid unsigned int overflow in PSBT_IN_TAP_BIP32_DERIVATION (pasta) Pull request description: ## Issue being fixed or feature implemented PSBT deserialization verified that a supplied previous transaction has the expected hash but accepted outpoint indices beyond its output vector, and `decodepsbt` then copied the referenced output without its own bounds check. Upstream rejects the invalid reference at the parser boundary, which prevents that out-of-bounds access. Triggering it requires RPC access or an operator decoding an externally supplied PSBT; no wallet unlock is needed. ## What was done? Backport of bitcoin#29855, preceded by a partial backport of bitcoin#25812. - **partial bitcoin#25812:** only the test harness, i.e. the `invalid_with_msg` section of `test/functional/data/rpc_psbt.json` and the `rpc_psbt.py` loop that checks each entry fails `decodepsbt` with its specific reason. The `PSBT_IN_TAP_BIP32_DERIVATION` length check and its Taproot test vector are omitted because Dash has no Taproot PSBT fields; they become applicable only if Dash adopts them. - **bitcoin#29855:** the existing hash check is retained and nonexistent previous-output indices are rejected during PSBT deserialization. Dash adaptations in bitcoin#29855: - The two new invalid fixtures were witness-stripped (Dash serialization has no witness framing or stacks) without changing txids, scripts, outputs, locktimes or outpoints. - Because the fixtures were edited, they are placed in `invalid_with_msg` with their expected reasons ("Non-witness UTXO does not match outpoint hash" and "Input specifies output index that does not exist") rather than in `invalid` as upstream does. This confirms they still fail for their intended reasons and not on an unrelated parse error. - The existing analyzer and wallet tests are updated to expect the earlier decode error. ## How Has This Been Tested? On macOS arm64 with the existing depends prefix: - Built `dashd`, `dash-cli` and `dash-wallet`. - `rpc_psbt.py` passed in both `--legacy-wallet` and `--descriptors` variants; both fixtures fail with their intended specific reasons. - Offline comparison against the exact upstream fixtures verified the normalization, txids and distinct rejection cases. ## Breaking Changes PSBTs containing nonexistent previous-output references now fail deserialization with RPC error `-22`, including `analyzepsbt` and `walletprocesspsbt`. Valid PSBT behavior and consensus rules are unchanged. ## Checklist: - [x] I have performed a self-review of my own code - [ ] 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: 9d213b13b1022fa45203c42fd22eb8b5196a510637163d21b8578f93d530a177fbf626753989ceb3c8bec3674dd65671f2194642b17cfd8fb17f2b091cb8bf30 (cherry picked from commit b2f6d4b)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Collaborator
|
✅ Final review complete — no blockers (commit 6c02b2b) · triage: critical |
PastaPastaPasta
force-pushed
the
backport/v24.0.0-rc.4
branch
from
October 16, 2026 20:06
e8d6880 to
e2da323
Compare
thepastaclaw
added
the
pastaclaw:changes-requested
PastaPastaPasta
force-pushed
the
backport/v24.0.0-rc.4
branch
from
October 16, 2026 20:47
e2da323 to
3336e73
Compare
thepastaclaw
removed
the
pastaclaw:changes-requested
thepastaclaw
approved these changes
Oct 16, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static review of the complete diff at 3336e73 found no actionable in-scope defects; the prior wallet-upgrade test conflict is fixed. The six upstream-test omission claims concern areas explicitly excluded by the documented partial backports, not unacknowledged prerequisite gaps. Current CI has passed container, cache-check, and merge-check jobs, but dependency builds and lint remain pending, so compilation and test success are not yet established.
🔴 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: backport-reviewer); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 7: 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:
criticalbygpt-6.1-sol(effort low) — This large, intricate backport changes consensus-sensitive provider transaction validation in src/evo/specialtxman.cpp, masternode state construction in src/node/miner.cpp, and peer-facing network encoding and decoding in src/evo/netinfo.cpp. - 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 71% left, 5h 100% left - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- 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 xhigh); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); 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.
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
CI failed after approval — commit 3336e73
The standing review approved this commit; this follow-up moves the verdict to COMMENT: checks that failed after the review look related to this change.
Failing checks:
linux64_sqlite-build / Build source(failure)linux64-build / Build source(failure)
Why they look related: Both builds fail compiling src/wallet/test/coinjoin_tests.cpp:513, a file changed by this PR, because it constructs CWallet with three arguments while the available constructor requires five. This is a concrete test-code compilation incompatibility, not an infrastructure failure.
Nothing was re-reviewed: the findings of the standing review are unchanged. The next push gets a full review; a re-run that passes does not restore the approval.
Review provenance
Source: CI-check lane gpt-6.1-sol (agent: triage); no reviewer or verifier lanes ran for this follow-up
- Verdict moved because CI failed on this commit after the review; failure logs read: 2 GitHub Actions job(s)
…ks once the record becomes readable 06ecb47 fix(coinjoin): persist pending-observation coin locks once the record becomes readable (pasta) Pull request description: ## Issue being fixed or feature implemented After a successful mixing session the client keeps the session's inputs locked until the wallet sees the finalized transaction spending them (`m_pending_obs`). The set is mirrored to the wallet database as the `cj_pending_obs` record, and each input also gets a persistent `lockedutxo` entry, so the protection survives a restart. Both are written in one database transaction. When the record exists but can't be read, or no transaction can be started, `AddPendingObservation()` deliberately persists nothing. It locks the new inputs in memory only and logs that they "will not survive a restart". `LoadPendingObservations()` keeps retrying the read. When a retry succeeds it merges the record into the in-memory set, but nothing ever writes the locks of the memory-only entries to disk: - `CheckPendingObservations()` only writes when an entry was released, and even then it writes only the record, not the locks. - The next `AddPendingObservation()` rewrites the whole record, including the earlier memory-only entries, but writes `lockedutxo` only for the inputs it is adding itself. After a restart the record therefore lists inputs that have no persistent lock. The first check reads the missing lock as a manual `lockunspent` release, drops the entry and makes the input selectable again, even though the finalized mixing transaction spending it may still be in flight. A transient `TxnBegin()` failure in `AddPendingObservation()` ends in the same state: the next add persists the record but not the earlier lock. ### Why this is a real problem The code path at c14104b: 1. With the record unreadable, `m_pending_obs_loaded` is unset, so `fTxn`/`fPersisted` are false and `LockCoin()` gets a null batch, which is a memory-only lock: [src/coinjoin/client.cpp#L707-L714](https://github.com/dashpay/dash/blob/c14104b172a2/src/coinjoin/client.cpp#L707-L714) 2. A later successful read merges the record and marks it loaded. Nothing records that the set now contains unpersisted entries: [src/coinjoin/client.cpp#L769-L771](https://github.com/dashpay/dash/blob/c14104b172a2/src/coinjoin/client.cpp#L769-L771) 3. The check writes only on `fChanged`, and only the record: [src/coinjoin/client.cpp#L865](https://github.com/dashpay/dash/blob/c14104b172a2/src/coinjoin/client.cpp#L865) 4. After a restart, an entry whose coin isn't locked is dropped as a manual unlock: [src/coinjoin/client.cpp#L806-L810](https://github.com/dashpay/dash/blob/c14104b172a2/src/coinjoin/client.cpp#L806-L810) The original version of this PR added a unit test that walks through this sequence (it is now folded into `coinjoin_pending_observation_unreadable_tests`): 1. Corrupt the record. 2. Add an observation, which is locked in memory only. 3. Make the record readable again. 4. Run a check. 5. Load a copy of the wallet database the way a restart would. Here it is against the unfixed code: <details><summary>Test run without the fix</summary> ``` $ git rev-parse HEAD; git diff --stat c14104b src/wallet/test/coinjoin_tests.cpp | 58 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) $ src/test/test_dash --run_test=coinjoin_tests/coinjoin_pending_observation_recovered_tests --color_output=no Running 1 test case... wallet/test/coinjoin_tests.cpp:517: error: in "coinjoin_tests/coinjoin_pending_observation_recovered_tests": check has_persistent_lock(reloaded->GetDatabase(), outpoint) has failed wallet/test/coinjoin_tests.cpp:518: error: in "coinjoin_tests/coinjoin_pending_observation_recovered_tests": check (MaybeCheckNotHeld(reloaded->cs_wallet), [&]() -> decltype(auto) { UniqueLock criticalblock54(MaybeCheckNotHeld(reloaded->cs_wallet), "reloaded->cs_wallet", "wallet/test/coinjoin_tests.cpp", 518); return reloaded->IsLockedCoin(outpoint); }()) has failed wallet/test/coinjoin_tests.cpp:522: error: in "coinjoin_tests/coinjoin_pending_observation_recovered_tests": check persisted.size() == 2 has failed [1 != 2] wallet/test/coinjoin_tests.cpp:523: error: in "coinjoin_tests/coinjoin_pending_observation_recovered_tests": check persisted.count(outpointInMemory) > 0 has failed *** 4 failures are detected in the test module "Dash Core Test Suite" exit=201 ``` </details> After the restart, the input added while the record was unreadable has no `lockedutxo` entry, so the reloaded wallet doesn't consider it locked. It isn't in the persisted record either. The input that was already persisted before is still locked, which is why each per-outpoint check fails exactly once. ### Why it matters The trigger is narrow. It needs a pending-observation record that fails to read and later reads fine, or a transient failure to start a database transaction, followed by a restart before the wallet sees the finalized mixing transaction. When it happens, though, the protection this code exists for is silently lost, despite the code having recovered the record. The input can be picked for another mixing session that conflicts with the in-flight transaction. That session then fails, and a collateral may be charged. No funds are lost. This code is in v24.0.0-rc.1/rc.2, so this should also go to the v24.0.x release branch. ## What was done? - Move the transactional write out of `AddPendingObservation()` into a `PersistPendingObservations()` helper. In one database transaction it writes the record plus `lockedutxo` for every entry whose coin is still locked, not only the entries just added. It aborts the whole transaction on any failure, exactly as before. Nothing is written while the record is unread. - `AddPendingObservation()` locks the new inputs in memory as before, then calls the helper. - Add `m_pending_obs_dirty` to `CCoinJoinClientManager`. It is set whenever the set could not be persisted: entries added while the record was unreadable, or a failed write on either path. Only a successful write clears it. - `CheckPendingObservations()` no longer returns early while the set is dirty. It persists record and locks through the same helper when the set is dirty or an entry was released, so a failed write is retried on every pass until it succeeds. The error is logged once when writing starts to fail, not on every retry. ### Why this is the correct minimal fix The root cause is that "this entry's lock isn't on disk yet" was never remembered once the record became readable. The dirty flag remembers it, including across a failed write. Every following check pass then retries writing the record and the locks together, in the same single transaction the code already uses to avoid partial states, until a write succeeds. Two smaller alternatives aren't enough: - Writing only the record on recovery is exactly the existing partial state: the record without its lock, which is what drops the entry on restart. - Persisting only in `AddPendingObservation()` would leave the gap open until the next mixing session completes, which may be after the restart. Rewriting `lockedutxo` for entries that are already persisted is idempotent, and the set only holds the inputs of recent sessions. Tracking which entries are memory-only would add state without benefit. Behavior intentionally left unchanged: - Coins whose lock was released manually in the meantime are not re-locked; the next check drops their entries as before. - Nothing is written while the record is unreadable. - The common check pass, where nothing is dirty and nothing is pending, still doesn't touch the wallet. ## How Has This Been Tested? macOS arm64, debug build (`--enable-debug --enable-werror`, depends prefix). - `coinjoin_tests/coinjoin_pending_observation_unreadable_tests` (extended) re-adds an input while the record is unreadable, makes the record readable again, and checks that the recovering check pass persists that input's lock. - New `coinjoin_tests/coinjoin_pending_observation_persist_retry_tests`: an open database transaction makes the add's write fail (SQLite only, compiled out otherwise because BDB starts an independent transaction); the test checks that neither the record nor the lock was written, and after the transaction is closed one check pass with nothing else changing must persist both. The test reloads a copy of the wallet database the way a restart would and checks the lock and the record entry. With `client.cpp`/`client.h` reverted to the version without the dirty-flag retry of failed writes it fails 3 checks after the reload (neither the lock nor the record entry survives); with the fix it passes. - `src/test/test_dash --run_test=coinjoin_tests,coinjoin_inouts_tests,walletdb_tests`: 70 test cases, no errors. - `git diff -U0 | contrib/devtools/clang-format-diff.py -p1`: no changes in the touched code apart from a pre-existing include-order nit in the test file. ## Breaking Changes None. ## 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: c8c7cb406f3c76d2cadf8077256d1c441332aead26f524ebe5b1a2f5dca69fe80a1b7d8c4c9d2621a7afc608a0d6c1c8886c2e1460891f27437e25f61727cf48 (cherry picked from commit 546ba18) Backport notes: develop's CWallet constructor no longer takes the CoinJoin loader or an ArgsManager. This branch's constructor still does, so the reload in coinjoin_pending_observation_persist_retry_tests passes m_node.coinjoin_loader.get() and m_args, as the fixture's own wallet does.
… move the funder's coins 520a7e3 fix(qt): always show the requested change of a key rotation (pasta) 6116d4d fix(evo): refuse to sign a shared registrar update that moves the funder's coins (pasta) Pull request description: ## Issue being fixed or feature implemented A shared masternode key rotation (`ProUpSharedRegTx`) is approved by every share owner, but those approvals sign only the payload: the new operator/voting keys plus `inputsHash`, which covers the input outpoints. Nothing covers the outputs. `protx shared_combine` then re-signs the fee inputs with the calling wallet and, with `submit=true`, broadcasts the result. This lets a co-owner steal from another owner. Coin outpoints are public, so the attacker builds a rotation request that spends one of the victim's coins as its "fee input" and sends almost all of the value to themselves. They add their own approval and pass the request on. The victim's wallet can sign every input, so the Qt dialog (`RotateSharedKeysDialog::preparedByThisWallet()`) concludes the victim prepared the request. That hides the decoded key-change summary and enables Send. Clicking Approve and then Send signs and broadcasts the theft. The same trick can also rotate keys without showing the user what changes. The feature has not shipped yet. ## What was done? - **`CombineShared` (registrar-update path):** before re-signing the fee inputs, require the shape that funding gives the transaction: - every input is an unlocked coin of this wallet, and all of them belong to one address (the fee source). Masternode collaterals are locked, and funding never selects a locked coin; this stops a request that "returns" a collateral to its own address from spending it; - every output that carries value pays back to that address (the zero-valued `OP_RETURN` marker that `fundTransaction` leaves when there is no change is allowed); - the fee (inputs minus outputs) is no more than the wallet's `-maxtxfee`. A registrar update is now always combined to completion on the funding wallet. The partial-signing path is gone for this type: it let a co-owner who paid themselves combine the request and leave the funder only an input to sign. The RPC help used to suggest finishing such hex with `signrawtransactionwithwallet`; it now describes the new rule instead. - **`RotateSharedKeysDialog::showRequestedChange()`:** show the decoded operator/voting change whenever a request is loaded. Whether this wallet prepared the request is only a guess, and it no longer decides what the user sees. It still controls which editing controls and send hints appear: a wrong guess there at worst produces one of the errors above. The Send failure hint is reworded to match the new error. ## How Has This Been Tested? `feature_masternode_shares.py`, in the existing "unanimous registrar signing works across wallets" scenario, with a fully approved request whose outputs were changed after approval: - the funding wallet refuses outputs rewritten to another wallet's address, or to a different address of its own; - the funding wallet refuses the genuine request while its fee input is locked; - a co-owner whose own address was put in the outputs cannot combine it with `submit=false`; - the funding wallet refuses a request whose change was reduced so the difference goes to fees; - a non-funding wallet is refused with both `submit=true` and `submit=false` (expected message updated); - a real no-change preparation keeps only the zero-valued marker output, combines, and is accepted by `testmempoolaccept`. Run locally on macOS arm64 (non-debug build). The test passes. With each new guard disabled in turn (input ownership, locked coins, single address, fee ceiling, marker exemption), the test fails at the matching assertion. With the whole check reverted, the funding wallet signs and broadcasts the redirected transaction. `test_dash-qt` passes; `rotationSenderComesFromTheInputs` now expects the summary to be visible for a locally prepared request too. The dialog was not exercised interactively. ## Breaking Changes `protx shared_combine` on a `ProUpSharedRegTx` is now refused unless its inputs are unlocked coins of a single address of the calling wallet, any change returns to that address, and the fee is within `-maxtxfee`. It no longer returns partially signed hex for another wallet to finish. ## 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: d5a9508b300dd3d471cb61203471b0fd24a5dbf6f1e5cfaa262b3b6394d7e4c02795a05a129dbb822835cf001f13e7303c3df656cb0a14014f498ee954f254b3 (cherry picked from commit 33670ce)
18484d1 fix(instantsend): forget the seen time of txs that are no longer tracked (pasta) 09703a4 doc: add release note for dashpay#7858 (pasta) 8fa5ae4 fix(instantsend): do not lock provider transactions (pasta) 2f33a23 test: sync mempools before burying a tx (pasta) Pull request description: ## Issue being fixed or feature implemented Provider transactions (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx and the new v24 provider types) can be removed from the mempool for conflicts that are not input conflicts: a duplicate collateral, owner/operator key, service address or proTxHash. An InstantSend lock on such a transaction therefore can't be relied on the way a lock on a normal transaction can. Masternodes should stop locking them. ## What was done? - `CheckCanLock(tx)` refuses all provider special transaction types, so masternodes no longer cast input-lock votes for them, and `TrySignInstantSendLock` won't sign an islock for one even when the input sigs were recovered by other signers. - A provider transaction that isn't locked is an unlocked parent like any other, so transactions spending its outputs become lockable once it is mined. A provider transaction that still gets a lock from signers without this change keeps it, and its children stay lockable as they are today. - This only changes what masternodes sign. Islocks for provider transactions received from other signers are still processed as before, so nodes that upgrade and nodes that don't keep the same view of locks during the transition. - Provider transactions are now confirmed the way unlocked transactions already are: miners include them after they have been in the mempool for 10 minutes, and the block is ChainLocked once its transactions are old enough. Mined provider transactions are not retroactively locked either, so if signers first see a provider transaction in a block, the ChainLock on that block waits for the same 10 minutes. This is a deliberate tradeoff. - With StatsD or `-debug=instantsend` active, the time a transaction was first seen is now forgotten once it stops being tracked. Before, a transaction that was mined without a lock and then ChainLocked kept its entry until restart, and every provider transaction now takes that path. dashpay#7839 already dropped the entry when a transaction leaves the mempool. - `bury_tx` in the test framework now syncs mempools before bumping mocktime. ChainLock signers measure a transaction's age from when they first saw it, so a provider transaction relayed after the bump would hold up ChainLocks in tests now that it isn't locked. ## How Has This Been Tested? - New `test_provider_tx_not_locked` case in `p2p_instantsend.py`: a ProUpServTx is not locked on any node while a later normal transaction is, and once it is mined after 10 minutes the block is ChainLocked. The case fails without the `signing.cpp` change (the ProTx gets locked) and passes with it. - Ran locally on macOS (arm64): p2p_instantsend.py, feature_llmq_chainlocks.py, feature_llmq_is_retroactive.py, feature_dip3_deterministicmns.py, feature_asset_locks.py, feature_masternode_payout_shares.py, feature_protx_version.py, feature_evo_shares_activation.py, feature_llmq_evo.py, feature_llmq_signing.py (with and without --spork21), feature_llmq_singlenode.py, feature_masternode_shares.py, p2p_platform_ban.py, rpc_netinfo.py, rpc_masternode.py, wallet_dash_rpcs.py, feature_llmq_simplepose.py (with and without --disable-spork23). feature_llmq_chainlocks.py failed before the `bury_tx` change and passes with it. - After rebasing onto develop with dashpay#7839, reran the full unit suite and p2p_instantsend.py, feature_llmq_is_retroactive.py, feature_asset_locks.py, feature_llmq_chainlocks.py, feature_llmq_is_cl_conflicts.py, interface_zmq_dash.py, feature_dip3_deterministicmns.py and mempool_limit.py; all pass. The seen-time map has no public accessor, so it has no unit test; functional tests run with `-debug`, so the code path is active in them. ## Breaking Changes Provider transactions no longer receive InstantSend locks from upgraded masternodes and take about 10 minutes to be mined and ChainLocked. Software that waits for an islock on a provider transaction should wait for a ChainLock or confirmations instead. There are no consensus changes. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have made corresponding changes to the documentation (release note) - [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) ACKs for top commit: knst: utACK 18484d1 Tree-SHA512: 9891883263ab67152d751765fd6270ea634c195a1757150c3ea162614706fbbddce78adf59d456572416965f67515084122af65de68da16cf26cea684913ac24 (cherry picked from commit a5e863a)
…wnership b3e27d9 backport: partial bitcoin#34156 restore only owned directories (pasta) Pull request description: ## Issue being fixed or feature implemented A filesystem exception while checking a restore backup can reach failure cleanup before the destination directory was created. An authorized `restorewallet` request can consequently delete an existing node-writable destination. Cleanup must only remove a directory owned by the current restore attempt. ## What was done? Partially adapts Bitcoin PR bitcoin#34156, specifically commit `4ed0693a3f2a427ef9e7ad016930ec29fa244995`: track successful destination creation and gate cleanup on that ownership. Set an explicit load-failure status in the exception handler so RPC callers do not inspect an uninitialized status. Dash still rejects existing destinations and always loads the restored wallet. The copied-file tracking, empty-directory assertion, and nonrecursive removal in upstream's commit are omitted: failed BDB verification can create `.walletlock`, `database/`, and `db.log` in a newly created directory, so Dash retains recursive cleanup exclusively for that owned directory. The remaining migration/restore-in-existing-directory changes in Bitcoin bitcoin#34156 are outside this partial backport. No upstream tests are claimed as backported; adapted functional coverage exercises Dash's restore behavior. ## How Has This Been Tested? On macOS arm64 with the existing depends-backed BDB/SQLite/Qt build: - `make -j1` passed. - `wallet_backup.py --legacy-wallet` and `wallet_backup.py --descriptors` passed, including restored balances, missing-backup/existing-destination controls, and new relative/absolute temporary-directory preservation checks. - The new preservation regression failed with the baseline binary because the nested sentinel was deleted after the early filesystem exception. The fixed code preserves it and returns a wallet error. - A truncated, recognized BDB backup fails verification and the newly created restore directory is removed, retaining Dash's auxiliary-file cleanup. - `test_dash --run_test=walletdb_tests`, whitespace lint, targeted repository Python lint, and `git diff --check` passed. The filesystem exception regression uses an overlong path component in temporary test data. It does not operate on an existing real wallet; filesystem-error details can vary by platform. ## Breaking Changes None. Existing restore destinations remain rejected. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation (no documentation change required) - [ ] I have assigned this pull request to a milestone This pull request was created by Codex. Top commit has no ACKs. Tree-SHA512: 8f96dbd792c42ddea6005c9198153c274bd72ef5983fc459cf899ee9c0c00945bd383a30f17dbcad420cf0ff279d1adf1bb0c436c60aec81a0ec5a05464341d3 (cherry picked from commit 66c7308)
…had been broken while 2e49361 fix: due to logical conflicts feature_pruning.py had been broken while (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Test feature_pruning.py is broken awhile. Discovered when testing dashpay#7861 ## What was done? Added missing args for restarted nodes. ## How Has This Been Tested? Run test/functional/feature_pruning.py directly ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] 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)_ Top commit has no ACKs. Tree-SHA512: 98b13451c4f99e3e810ffc91095c3b9e97eca5a5ac3d4ea7e3b638a2828b992ba853cbad03185b928d60a337ed8ab7616fe9f4348151b25b860ef83d69b77b77 (cherry picked from commit cf91169)
…ing mnsync On test networks a node only fetched sporks by sending GETSPORKS from the mnsync tick. That request was skipped once mnsync finished, skipped for any peer address with a still-valid "spork-sync" entry in netfulfilled.dat (which survives restarts), and skipped for inbound peers on an active masternode. A node that came up without sporks, e.g. after the sporks.dat format change in v24, could therefore finish mnsync without them and never ask again, leaving DKG and ChainLocks disabled until `mnsync reset`. Announce every active spork as an inv to each peer when the handshake completes, in both directions. Like spork relay, this includes masternode connections, which only become relay-capable after MNAUTH. Peers request the ones they lack with getdata, and a node that newly accepts a spork already relays it, so the spork set converges without any opt-in or sync state. The GETSPORKS handler is kept so v23 nodes can still ask us. Fixes dashpay#7891 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 45acdf3) Backport notes: develop merged dashpay#7893 twice. The first merge (f99212b) carried an earlier revision of this commit, and the second merge (72a5997) carried this one, so develop now announces sporks in two places in the VERACK handler. Only the PR's final commit is picked here. Conflict in src/node/sync_manager.cpp: on this branch the mnsync tick still sent GETSPORKS on every network, because dashpay#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.
…led upgrade instead creating brand-new scenario (cherry picked from commit 4a19742)
…d derivation truncates 6051735 fix(wallet): reject mnemonic passphrases that seed derivation truncates (pasta) Pull request description: ## Issue being fixed or feature implemented Seed derivation (`CMnemonic::ToSeed`) uses the BIP39 salt `"mnemonic"` followed by the passphrase. History: - **Up to 19.1.0** the full salt was used. A very long passphrase could crash the node. - **19.2.0** (backport of d53e657, "truncate mnemonic passphrase instead of crashing") started cutting the salt at 256 bytes with `strnlen`. Every passphrase byte after the 248th has been silently ignored since then, so two passphrases that differ only after that point derive the same seed. A user who picks a long passphrase believes all of it protects the seed, but it doesn't. - **Before 23.0.0** the `strnlen` cut also stopped at the first NUL byte. 7f53b9f fixed that for v23. Fixing `ToSeed` to use the full salt would bring back the pre-19.2.0 derivation. That breaks mnemonic recovery for every wallet created on 19.2.0 or later with a long passphrase. Keeping the cap preserves those wallets, but wallets created before 19.2.0 with a long passphrase already can't be recovered from their mnemonic on current software, with or without any option. Whichever derivation is chosen, someone's mnemonic stops reproducing their keys. This PR therefore leaves `ToSeed` unchanged and rejects passphrases that it would truncate when a new seed is created. ## What was done? - `upgradetohd` and `createwallet` (RPC and GUI, using `-mnemonicpassphrase`) reject a mnemonic passphrase longer than 248 bytes. Both checks run before the wallet is modified or its database is created. - A new startup option, `-allow-deprecated-long-mnemonics`, raises that limit to 256 bytes so a wallet created with a passphrase of 249 to 256 bytes can still be recovered with the same (truncated) derivation. Passphrases over 256 bytes are rejected by both checks even with the option, because `CHDChain::SetMnemonic` throws for them. - The `createwallet` check is unconditional, so a `-mnemonicpassphrase` longer than 248 bytes is also refused on a node that would ignore it (`-usehd=0` or `-hdseed`). That is deliberate: it keeps the guard simple, and such a value is a misconfiguration that would otherwise be silently ignored. - Existing wallets, `ToSeed`, loading, restore and migration are unchanged. ## How Has This Been Tested? `wallet_upgradetohd.py` (legacy and descriptor modes) gains a short case: a 248-byte passphrase is accepted by `upgradetohd` and kept, a 249-byte passphrase is rejected by `upgradetohd` and by `createwallet` with `-mnemonicpassphrase`, and after a restart with `-allow-deprecated-long-mnemonics` both `createwallet` (same wallet name, so the rejection left nothing behind) and `upgradetohd` accept it and store the full passphrase, while a 257-byte passphrase is still rejected by `upgradetohd` with the option set. Also ran `wallet_createwallet.py`, `wallet_mnemonicbits.py`, `rpc_help.py`, and `test_dash --run_test=bip39_tests`. ## Breaking Changes Creating an HD wallet with a mnemonic passphrase longer than 248 bytes now fails unless `-allow-deprecated-long-mnemonics` is set, and one longer than 256 bytes always fails. Existing wallets are not affected. ## Checklist: - [x] I have performed a self-review of my own code - [x] 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: da4b2cf7dbae37db67df7374814106621f5e53b813a5d8acbed4a5e909ae51a108d2d9a0bf71640710c0ebfceb1d27ce7500ddb8f403fe91b22eee9f04b7193c (cherry picked from commit 8e4cd3d)
…ge of its masternode 64b828d test: mine a pending service update before a shared masternode's key change (pasta) 7645a26 docs: add release note for mining service updates before key changes (UdjinM6) f2c84a8 test: block assembly mines a pending ProUpServTx before a key change of its masternode (UdjinM6) 0295a51 fix(mining): mine a pending ProUpServTx before a key change of its masternode (UdjinM6) 89c40ec fix: move the masternode list tip before a reorg re-adds transactions to the mempool (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Fixes dashpay#7582 at the block assembly level, as an alternative to dashpay#7728. `CheckProUpServTx()` verifies the operator signature against the masternode list of the previous block, and block connection applies provider transactions in block order without checking again who signed them. A `ProUpServTx` signed with the operator key that an earlier transaction in the same block changes (`ProUpRegTx` or `ProUpSharedRegTx` with a new operator key) or revokes (`ProUpRevTx`) therefore still applies: it writes the previous operator's service fields back and, after a key change, revives the masternode the change just PoSe-banned. The mempool accepts both transactions together, and block assembly orders them by feerate, so a displaced operator can broadcast a low-feerate service update and wait for the key change to be mined. The reverse order is harmless: the key change resets the operator fields the service update set. This PR makes honest block assembly always produce that order, instead of keeping the two transactions out of the mempool as dashpay#7728 does. Compared to dashpay#7728, no valid service update the operator already broadcast is evicted or refused for being paired with a key change, and mempool contents do not depend on the order in which a node received the two transactions. Consensus is unchanged: a block with the harmful order is still valid, so a miner ignoring this policy can still produce one, as with dashpay#7728. ## What was done? - **Block assembly** (`BlockAssembler::addPackageTxs`): when a package contains an operator key change or revocation, the masternode's pending `ProUpServTx`s, with their unconfirmed ancestors, are pulled into the package and placed before it. New helper `CTxMemPool::GetServiceUpdatesBeforeKeyChange()`. - A service update is left out if its own package fails the usual transaction checks, if it is one the mempool would refuse as unorderable (see below; the mempool rejects those on entry, but a reorg can return such ancestors below an update already in the pool), or if its package has a service update of a masternode whose key already changed in the block. - If the pulled-in updates do not fit, they are all left out and the key change goes in without them, so pending updates, however many, cannot hold it back. - Once a key change is in the template, no later `ProUpServTx` of that masternode is added, so an update left out, or one that only becomes eligible while the template is built, cannot follow it. - Whether a transaction is a key change is decided by `CTxMemPool::GetKeyChangeTarget()` against the current tip list, not when the transaction entered the mempool, so a registrar update that becomes a key change after a reorg is handled too. - **Mempool policy**: a `ProUpServTx` that block assembly could not reliably put ahead of a key change is rejected with `protx-dup` (`CTxMemPool::IsUnorderableServiceUpdate()`, called next to `existsProviderTxConflict()`): - **With a pending operator key change among its ancestors** (it spends one, directly or through other pending transactions). It can only be mined after that change. For its own masternode it would restore the previous operator's fields. For another masternode, pulling it in would require that other change. - **With a pending asset lock, asset unlock or MNHF signal among its ancestors.** Their own checks during block assembly could keep the pulled-in update, and the key change with it, out of the block. Such a transaction can only arrive after the ancestor it spends, so every node makes the same decision. Package acceptance checks package members against the pool before adding any of them, so a package carrying both transactions is not caught. Packages are regtest-only. - **Reorg**: `MaybeUpdateMempoolForReorg()` moves the deterministic masternode list tip to the new chain tip before re-adding disconnected transactions. Before, a key change returned to the mempool was compared with the list of the disconnected block, which already had the new key, and was not recognized as a key change. This change was first proposed in dashpay#7728 and is kept as its own commit. - Release note `doc/release-notes-7857.md`. This keeps pending provider transactions in the mempool and only changes their order in block templates, so it does not depend on how they were relayed. ## How Has This Been Tested? Built on macOS arm64 with `--enable-debug --enable-werror --enable-crash-hooks`. New unit tests in `src/test/evo_deterministicmns_tests.cpp`, using block templates unless noted: - `test_miner_service_update_before_key_change`: - a low-fee `ProUpServTx` is placed before a high-fee key change or revocation of its masternode; - a registrar update that keeps the operator key is ordered by fee as usual; - a `ProUpServTx` that cannot be mined (it takes another masternode's address) does not hold the key change back. - `test_mempool_rejects_unorderable_service_update`, through `ProcessTransaction`: a `ProUpServTx` is rejected with `protx-dup` when funded from a pending key change of its masternode, from a transaction spending one, from a pending key change of another masternode, or from a pending asset lock. An independently funded one is accepted. - `test_miner_service_update_before_reorged_key_change`: a key change returned to the mempool by `InvalidateBlock` still counts as the masternode's pending key change, so a second one is rejected with `protx-dup`, and a pending `ProUpServTx` is still placed before it. - `test_miner_service_update_before_key_change_surviving_reorg`: a registrar update that kept the operator key when it entered the mempool becomes a key change after a reorg, and a service update signed with the restored key still goes in before it. - `test_miner_service_update_never_after_key_change`: a `ProUpServTx` that follows a key change in the pool (it spends it) is never mined after it, while one following a registrar update that keeps the key still is. - `test_miner_key_change_not_held_back_after_reorg`: a reorg returns a zero-fee key change of another masternode below a pending `ProUpServTx`; the key change of the update's own masternode is mined and the update is left out. - `test_miner_service_update_with_parent_before_key_change`: a `ProUpServTx` funded by an ordinary pending transaction goes in as parent, service update, key change. - `test_miner_key_change_without_room_for_service_update`: with room for either the parent and the service update or the key change, but not all three, the key change goes in without the service update, and the template stays within its size limit. - `test_miner_key_change_not_held_back_by_service_updates`: two independently funded `ProUpServTx`s that fit ahead of a key change only one at a time; the key change goes in and neither update does. - `test_miner_key_change_not_held_back_by_other_key_change`: a key change of masternode A goes in without A's service updates; a service update of masternode B funded by one of them is left out instead of holding back B's key change. Reverting each piece in turn makes the corresponding case fail: - the pull-in in block assembly (both ordering tests); - the reorg tip move (only the reorg test); - limiting the rejection to the update's own masternode (the other-masternode case); - removing asset locks from the rejection (the asset-lock case); - leaving unorderable service updates in the pull-in (`test_miner_key_change_not_held_back_after_reorg`); - holding the key change back when the pulled-in updates do not fit (both block-space tests); - pulling in an update package with a service update of a masternode whose key already changed in the block (`test_miner_key_change_not_held_back_by_other_key_change`). Run at the head of this PR: - **Unit tests:** all pass (`test_dash`, 1071 cases). - **Functional tests:** all pass: `feature_asset_locks.py`, `feature_dip3_deterministicmns.py --descriptors`, `feature_llmq_chainlocks.py`, `feature_masternode_shares.py`, `feature_protx_version.py`, `mempool_packages.py`, `mempool_reorg.py`, `mining_basic.py`, `rpc_masternode.py`, `rpc_packages.py`. - **Lint:** `test/lint/all-lint.py` reports only an existing cppcheck warning in `src/governance/object.cpp`, which this PR does not touch. ## Breaking Changes None for consensus. Mempool policy: a `ProUpServTx` spending an output of a pending operator key change, asset lock, asset unlock or MNHF signal is rejected until that transaction confirms. It can then be sent again, signed with the operator key current at that point. Mining policy: pending service updates of a masternode that do not fit in the block ahead of its key change are left out of that block, and become invalid once the key change confirms. ## 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) ACKs for top commit: knst: utACK 64b828d Tree-SHA512: 59bd24403894100976ef6388b1efd95df53a83c8d45101a70d37083865884c1d8786c58d737518783deee8a403bba02ecef15db5c7706e9dbb0db68c6da83f6f (cherry picked from commit cd7520e) Backport notes: develop folded CChainstateHelper into Chainstate in d5c1bd7, which is not on this branch. Here MaybeUpdateMempoolForReorg reaches the special tx processor through ChainHelper().special_tx instead of m_special_tx. In src/evo/specialtxman.{h,cpp}, only UpdatedMNListTip is added. GetDeterministicMNListHash, which sits next to it on develop, belongs to unrelated develop-only work. In src/test/evo_deterministicmns_tests.cpp, the branch's evo/chainhelper.h include is kept next to the new evo/assetlocktx.h.
…Ds as one conflict namespace 43ae7ac fix: treat pending owner keys and Platform node IDs as one namespace (pasta) Pull request description: ## Issue being fixed or feature implemented The deterministic masternode list keeps owner key IDs and Platform node IDs in one uniqueness map. It is keyed by `SerializeHash`, and a `CKeyID` serializes exactly like a `uint160` with the same bytes. So once one masternode uses a 160-bit value as its owner key ID, no masternode can use it as a Platform node ID, and vice versa. The mempool tracks the two fields in separate maps (`mapProTxPubKeyIDs`, `mapProTxPlatformNodeIDs`) and checks each field only against its own map. Two pending provider transactions can therefore claim the same value, one as an owner key and one as a Platform node ID. Each is valid against the tip list and both are admitted. If block assembly selects both, `BuildNewListFromBlock` rejects the pair and `CreateNewBlock` / `getblocktemplate` fails until one of them leaves the mempool. The same applies when a block confirms one claim while the other is still pending: the pending one can no longer be mined, but nothing evicts it. Both transactions must be funded, collateral-backed provider transactions, so the cost is not trivial. A mempool that can stop template creation is still worth closing, and the fix is policy-only. ## What was done? `src/txmempool.cpp`. Consensus rules are unchanged. - `existsProviderTxConflict`: - A `ProRegTx` owner key, or each share owner key for a shared registration, conflicts with a pending owner key *or* a pending Platform node ID of the same value. - An EvoNode `ProRegTx` or `ProUpServTx` Platform node ID conflicts with a pending Platform node ID *or* a pending owner key of the same value. - A registration that uses one value for both its own owner key and its own Platform node ID is still accepted. The masternode list allows that within one masternode. - `removeProTxPubKeyConflicts(tx, CKeyID)` also evicts a pending transaction claiming the value as a Platform node ID. `removeProTxPlatformNodeIDConflicts` also evicts one claiming it as an owner key. A confirmed claim therefore removes the pending claim in the other field, together with its descendants. Not covered here: conflicts between provider transactions inside one submitted package. Package `PreChecks` run before any package transaction is in the mempool maps, so same-field conflicts inside a package aren't caught either. That gap predates this change, and `submitpackage` is a test-only RPC. ## How Has This Been Tested? macOS arm64, depends build. The coverage extends the existing `FuncTestMempoolDualProregtx` scenario (`evo_dip3_activation_tests/test_mempool_dual_proregtx_legacy` and `_basic`) in `src/test/evo_deterministicmns_tests.cpp`. It uses the public mempool API (`addUnchecked`, `existsProviderTxConflict`, `removeForBlock`) with unsigned provider payloads. It checks: - Non-shared registrations: a pending owner key blocks a `ProRegTx` and a `ProUpServTx` that claim the same value as their Platform node ID, and a pending Platform node ID blocks a `ProRegTx` owner key of the same value. An unrelated Platform node ID doesn't conflict. - Eviction: a confirmed owner key evicts a pending Platform node ID claim, and a confirmed Platform node ID evicts a pending owner key claim. - Shared registrations, in both directions: a share owner key colliding with a pending Platform node ID is rejected, and a Platform node ID colliding with a pending share owner key is rejected. Confirming either one evicts the other. The registrations use distinct operator keys, no addresses and internal collateral, so only the cross-namespace collision can conflict. - Negative case: an otherwise non-conflicting EvoNode registration that uses the same value for its own owner key and Platform node ID is accepted, with an unrelated registration pending. Mutation checks: - Restoring the owner-only lookup (`mapProTxPubKeyIDs.count`) in the shares branch of `existsProviderTxConflict` makes the shared-registration admission check fail in both test cases. - Making an EvoNode registration conflict with its own owner key equal to its Platform node ID makes the negative check fail in both test cases. All of `evo_dip3_activation_tests` (42 cases) and the other mempool unit tests pass. `git diff --check` and `lint-whitespace.py` are clean. Limits: no functional test submits two funded, collateral-backed provider transactions and then calls `getblocktemplate`. ## Breaking Changes None. Provider transactions whose owner key and Platform node ID collide across two pending transactions are now rejected from the mempool, or evicted, instead of breaking block assembly. ## 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) ACKs for top commit: knst: utACK 43ae7ac Tree-SHA512: a410e026a0790685fdd0a4ad55c76a9586577c037929d5010706848836bf967a78b4cd04b6611c7fe1bcc88655693c552bd86b61ac3c84ad89f21752caf65752 (cherry picked from commit 41fdf14)
…wording 52cf9ae test: cover retroactive refusal of provider tx InstantSend locks (UdjinM6) 95fed01 doc: fix when children of provider txs become lockable (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Follow-up to dashpay#7858, which stopped masternodes from InstantSend-locking provider transactions. `test_provider_tx_not_locked` had gaps: - The mempool case checked that the ProTx had no lock right after an unrelated later transaction was locked. That is a timing proxy: a regression where the ProTx lock arrives slightly later than the other lock would still pass. - A provider transaction that signers first see in a block (`NetInstantSend::BlockConnected` → `ProcessTx(tx, /*fRetroactive=*/true)`) was not covered. The existing test passed a mutation that disabled the provider checks only for retroactive processing. - The release note said transactions spending provider outputs can be locked once the provider transaction is mined. A mined parent whose block is not ChainLocked is only accepted after `nInstantSendConfirmationsRequired` confirmations, and its children are retried when its block is ChainLocked, so "once the block that includes the provider transaction is ChainLocked" is the accurate condition. Nothing tested this. ## What was done? `doc/release-notes-7858.md`: children of provider transactions can be locked once the block that includes the parent is ChainLocked, not once the parent is mined. Test changes are in `test_provider_tx_not_locked` in `p2p_instantsend.py`: - Mempool case: the ProTx is built with `submit=False` and broadcast inside `assert_debug_log` on the masternode, which waits for the txid-specific `CheckCanLock -- txid=…: provider txs are not locked` and `ProcessTx -- txid=…: CheckCanLock returned false` lines. These confirm that `ProcessTx` returned before input signing, rather than inferring it from another transaction's lock. A redundant `sync_mempools()` is removed. - New case: node 0 is isolated, a ProUpServTx and a normal transaction are mined there, and node 0 then reconnects, so the masternode first sees both in a block. The same log barrier, taken around the reconnect, proves the retroactive path refused the ProTx. The test then waits for the normal transaction's retroactive `instantlock_internal` as a positive control, and checks that the ProTx has no lock and the block is not yet ChainLocked. - Child check, in the same case: while that block is mined (one confirmation) but not ChainLocked, a child spending the ProTx output is broadcast inside a log barrier waiting for `CheckCanLock -- txid=<child>: outpoint <protx>-0 too new and not ChainLocked`, and has no `instantlock_internal` on any node. Mocktime is then advanced so the same block is ChainLocked without mining another one, and the child must get `instantlock_internal` on all nodes while the tip is unchanged. This covers both the refusal before the ChainLock and the retry of pending children when it arrives. The normal-transaction and child checks poll `instantlock_internal` to require an InstantSend lock; `instantlock` is also true when the transaction's block is ChainLocked. ## How Has This Been Tested? - `p2p_instantsend.py` passed repeatedly on macOS (arm64) with an `--enable-debug` build, on top of current develop. A full GitHub CI run passed on an earlier revision of this branch. - Mutation checks, each with a rebuilt `dashd`: - Skip the provider refusal only when `fRetroactive` is set and drop the `TrySignInstantSendLock` guard: the retroactive case fails at its log barrier. The original test passed under this mutation. - Drop the provider refusal in `CheckCanLock(tx)` and the `TrySignInstantSendLock` guard entirely: the mempool case fails at its log barrier. - Don't retry children when the parent's block is ChainLocked (`RemoveNonLockedTx(txid, false)` in `RemoveConfirmedInstantSendLocks`): the child never gets locked and the test fails. - Drop the "too new and not ChainLocked" refusal in `CheckCanLock(outpoint)`: the child check fails at its log barrier. - The child check would also pass before dashpay#7858. It guards against breaking the descendants of provider transactions, not against reverting the refusal. - These tests don't exercise the `TrySignInstantSendLock` provider guard on its own; `CheckCanLock(tx)` refuses the provider transaction before input signing. ## Breaking Changes None. Test and release-note changes only. ## 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: 056ab842e7163e42973e62237c248d65e51023496c75def7434536cc2579b2029d66d98e3f52053ec1aa8af1d05afb1be27ee6fb9b50d75ab4cdb8153c46e6b5 (cherry picked from commit 4461504)
…ally during block assembly d9d3704 test: block assembly leaves out a provider package that conflicts with an earlier one (UdjinM6) dae84b9 feat(mining): build the masternode list incrementally during block assembly (UdjinM6) cb07b73 refactor: split RebuildListFromBlock into per-block, per-tx and final phases (UdjinM6) Pull request description: ## Issue being fixed or feature implemented In-block conflicts between provider transactions can make the masternode-list build after transaction selection fail, causing `CreateNewBlock` and `getblocktemplate` to fail. Block assembly should skip packages that conflict with the list built so far and continue selecting other transactions. ## What was done? - Split `CSpecialTxProcessor::RebuildListFromBlock` into `BeginListForBlock`, `ApplyTxToList` and `FinishListForBlock`, preserving the existing list-transition logic and validation rules. Add `CMNListBuilder` to drive these phases incrementally; its list copies share persistent maps. - When DIP3 is active, `CreateNewBlock` starts the builder from the tip's list and applies the quorum commitments before selecting mempool transactions. For each eligible package containing a special transaction, `addPackageTxs` applies its transactions in block order to a copy of the builder. A failed application or `std::runtime_error` causes the package to be skipped. Existing ordering, EHF-signal and current-minability checks precede this trial; credit-pool checks follow it. The copy replaces the builder only after all package checks pass. - Use `Finish()` to scan the complete block for collateral spends and perform payee bookkeeping, producing the final list for the CbTx masternode-list merkle root. - Add `template_skips_conflicting_provider_package` to cover conflict handling and discarded package trials. No consensus rule changes. ## How Has This Been Tested? macOS arm64, depends build, `--enable-debug --enable-werror`. New unit test `evo_dip3_activation_tests/template_skips_conflicting_provider_package` checks that: - Template creation succeeds, keeping the first selected conflicting transaction and an unrelated transaction while excluding the other conflicting transaction and its descendant. - A transaction the list rejects by throwing (a duplicate `platformNodeID`, which `AddMN` reports with an exception) is left out the same way. - A rejected package does not prevent selection of a later transaction that would conflict only with that rejected trial. The rejected package's transactions remain excluded. The test checks the ancestor-feerate ranking needed to exercise this case before assembly. Local regression checks: without the `feat(mining)` commit, `CreateNewBlock` fails; without the `catch`, `CreateNewBlock` throws; committing a rejected builder trial makes the test fail. Also checked locally, with a check not included in this PR: a simulated later credit-pool rejection discards the trial's masternode-list changes. Also run: - Unit suites: `evo_*`, `miner_tests`, `mempool_tests`, `txvalidation_tests`, `validation_block_tests`, `block_reward_reallocation_tests`. - Functional tests: `feature_dip3_deterministicmns.py`, `feature_masternode_shares.py`, `rpc_netinfo.py`, `mining_*.py`, `feature_llmq_simplepose.py`, `feature_llmq_evo.py`, `feature_llmq_rotation.py`, `feature_asset_locks.py`, `feature_mnehf.py`, `rpc_masternode.py`, `p2p_instantsend.py`. - `test/lint/all-lint.py`: only a pre-existing cppcheck warning in `src/governance/object.cpp`, which this PR does not modify. ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone Top commit has no ACKs. Tree-SHA512: e7a7d894449ba5738d044ab70a853ece979d18fd24e44882a2b8017aa1767cd92ca4b7c032b38c2b6a928c8b6c582ea450fbbd3563a0db56dc10a02e145674fb (cherry picked from commit 8a17a55) Backport notes: develop folded CChainstateHelper into Chainstate in d5c1bd7, which is not on this branch, so BlockAssembler builds its CMNListBuilder from *m_chain_helper.special_tx instead of m_chainstate.m_special_tx. In src/evo/specialtxman.cpp, GetDeterministicMNListHash, which sits next to the new code on develop, belongs to unrelated develop-only work and is left out.
…oinJoin server a231f05 fix(coinjoin): apply a DSSIGNFINALTX message atomically (pasta) 873295f fix(coinjoin): bind completion tail to session snapshot (pasta) d1e23c7 fix(coinjoin): prioritize progress over timeout reset (pasta) 0ba94a6 fix(coinjoin): serialize timeout transitions (pasta) 97daa21 fix(coinjoin): guard the session collaterals with cs_coinjoin (pasta) 3fdd82d fix(coinjoin): decide server state transitions under cs_coinjoin (pasta) Pull request description: ## Issue being fixed or feature implemented `CCoinJoinServer` state is touched by two threads: `msghand` (single-threaded, via `ProcessDS*`) and the 1s scheduler tick (`CheckForCompleteQueue` / `CheckPool` / `CheckTimeout`), plus RPC threads in `GetJsonInfo()`. `CheckPool()` runs on **both**: the scheduler tick and `msghand` via `ProcessDSVIN` / `ProcessDSSIGNFINALTX`. Several pieces of the original version of this PR have since landed on develop separately (dashpay#7597 `nSessionDenom` atomic, dashpay#7599 entry admission bound to the session snapshot, plus the single-snapshot `CheckPool()` and the session-bound `CreateFinalTransaction()` from the promotion/demotion work). This PR is now rebased on top of those and contains what is still missing: 1. **`CheckPool()` was not single-flight.** Two concurrent calls (scheduler + `msghand`) could both act on the same snapshot. If both finalize, clients receive `DSFINALTX` twice and sign twice, and the duplicate signatures make `AddScriptSig()` fail, which relays `STATUS_REJECTED` and aborts the session for everyone. Both calls could also commit the same final transaction. 2. **`CommitFinalTransaction()` was not bound to a session.** It committed whatever `finalMutableTransaction` held, even when the session had timed out and been reset after the decision. That pushes a cleared transaction through ATMP and notifies participants of a failure that never happened. 3. **`CheckTimeout()` raced finalization and commit.** It tested `HasTimedOut()` without a lock and could reset a session halfway through `CheckPool()` finalizing or committing it. It also reset sessions that could still advance, because the final collateral, entry or signature can arrive after the scheduler's `CheckPool()` snapshot. In that case `msghand` skips its own `CheckPool()` while the scheduler holds the guard. 4. **Offender selection and charging were not atomic with closing admission.** `ChargeFees()` sampled `nState` three times, so it could select "didn't send" offenders and then charge and log them as "didn't sign". On the timeout-finalize path it ran before eligibility was rechecked, so an ineligible session was charged once there and again by the `CheckTimeout()` reset that followed. 5. **The session collaterals were not `GUARDED_BY`.** `vecSessionCollaterals` was unannotated while its companion prevout index was guarded. `CreateNewSession()` still read it after releasing the lock, and nothing prevented the next unlocked read. 6. **The completion tail mutated live state.** If every participant had disconnected, `RelayCompletedTransaction()` could reset the session. A replacement session could then open before random charging and the unconditional tail reset ran, and those would charge or clear the replacement. The invalid-transaction path also reset the pool *before* notifying anyone, so `ERR_INVALID_TX` reached nobody. 7. **A `DSSIGNFINALTX` message was not applied atomically.** Each input went through its own locked `AddScriptSig()`. A signing timeout landing between two inputs of one already-received message would select the sender as a non-signer, charge them, and reset the session. ## What was done? Six commits, each standalone: **`fix(coinjoin): decide server state transitions under cs_coinjoin`**: addresses 1, 2 and the triple sample in 4. A new `cs_check_pool`, acquired **only** via `TRY_LOCK`, makes `CheckPool()` single-shot without ever blocking `msghand`. Signature completeness joins the existing pool snapshot. `CommitFinalTransaction()` takes the session id the decision was made for and revalidates it. `SetState()` and `IsSignaturesComplete()` now require `cs_coinjoin`, and `ChargeFees()` samples `nState` once. **`fix(coinjoin): guard the session collaterals with cs_coinjoin`**: addresses 5. The collateral transactions and their prevout index become a single `SessionCollaterals` member marked `GUARDED_BY(cs_coinjoin)`, so they cannot drift apart or be annotated inconsistently. Reintroducing an unlocked read is now a `-Wthread-safety` compile error. **`fix(coinjoin): serialize timeout transitions`**: addresses 3 (first half) and the rest of 4. `CheckTimeout()` uses the same `cs_check_pool` guard. Offender selection moves into `SelectCollateralToCharge()` under `cs_coinjoin`, in the same lock scope that resets the session (`CheckTimeout()`) or moves it to `SIGNING` (`CreateFinalTransaction(session_id, charge_fees)`). The timeout-finalize path only charges once the live session is confirmed eligible. Collateral is consumed after `cs_coinjoin` is released, because `ConsumeCollateral()` takes `cs_main`. The session may be reset before that consumption runs, so the selected collateral stays reserved in `m_collaterals_being_charged` (which survives `SetNull()`) until `ConsumeCollateral()` has finished. During that time `CreateNewSession()` and `AddUserToExistingSession()` refuse a dsa whose collateral spends one of its inputs. The charged participant therefore cannot be readmitted into the next session with a collateral that is about to be spent, and the fee is still taken. `AddEntry()`'s `ConsumeCollateralIfCurrentSession()` takes the same reservation. **`fix(coinjoin): prioritize progress over timeout reset`**: addresses 3 (second half). Before resetting, `CheckTimeout()` rechecks under `cs_coinjoin` whether the session can still advance: queue ready, an entry for every collateral, enough entries covering both sides of the session denomination, or every signature in. If so it leaves the session to the next tick. The rules mirror the decisions `CheckPool()` and `CreateFinalTransaction()` make, so a deferred session is always acted on. The decision is the pure function `ShouldResetTimedOutSession()`, which `CheckTimeout()` calls with the session's progress read under `cs_coinjoin`. **`fix(coinjoin): bind completion tail to session snapshot`**: addresses 6. The committed transaction, participants and collaterals are captured under `cs_coinjoin`. Completion notification is side-effect-free, and only the matching signing session is reset. The invalid-transaction path notifies the captured participants. `CheckPool()`'s uncovered-session reset captures its participants and resets in one step, then tells them. **`fix(coinjoin): apply a DSSIGNFINALTX message atomically`**: addresses 7. The signing session the sender was checked against is revalidated after decoding, and the whole batch is applied in one `cs_coinjoin` critical section. The rejection relay for a failed input happens inside that same section. ### Lock discipline No new `cs_coinjoin`-across-`cs_main` edge is introduced: offender collateral is consumed only after `cs_coinjoin` is released, and signature verification already ran under `cs_coinjoin`. `cs_check_pool` is `TRY_LOCK`-only, taken only by `CheckPool()` and `CheckTimeout()` while holding nothing else, so `cs_check_pool` → `cs_coinjoin` / `cs_main` is unidirectional and no ABBA cycle is possible. ## How Has This Been Tested? macOS arm64, depends build, clang with `-Wthread-safety -Werror`, rebased on develop c8bfd7a. - Every one of the six commits builds `test_dash` and passes `coinjoin_inouts_tests` on its own. - Unit suites `coinjoin_inouts_tests`, `coinjoin_tests`, `coinjoin_queue_tests`, `coinjoin_dstxmanager_tests` pass. - Functional `rpc_coinjoin.py` and `p2p_dstx.py` pass. - `lint-whitespace.py`, `lint-includes.py`, `lint-circular-dependencies.py` pass. New/updated unit tests in `coinjoin_inouts_tests`: - `server_timeout_resets_only_sessions_that_cannot_advance` is one table over `ShouldResetTimedOutSession()`. Rows that must be left alone: a session that has not timed out; a ready queue; an entry for every collateral (even with a side uncovered, which `CheckPool()` resets itself and tells the participants); enough covered entries with one straggler; every input signed. Rows that must be reset: a queue that is not ready; too few entries; enough entries with a side uncovered and one straggler; a missing signature. Mutations checked: removing the deferral, the signing-advance clause, either accepting-advance clause, or the side-coverage condition each fails at least one row. - `server_finalization_rechecks_live_side_coverage` now exercises the charging finalize path. The `cs_check_pool` single-flight guard, the completion-tail snapshot and the `DSSIGNFINALTX` atomicity change close timing windows that cannot be reproduced deterministically in a unit test without driving private synchronization state, so they have no dedicated test. The completion tail is bound to the session id and participant list captured under `cs_coinjoin`, and the reset checks both the session id and `nState`, which is visible in the diff. They were verified by inspection and by the existing `server_signfinaltx_*` tests, which still pass. The charge reservation is in the same position: its window lies between two lock scopes inside one call. It was verified by checking that every reservation is released exactly once, after `ConsumeCollateral()` returns, on every path; that admission checks it under the same `cs_coinjoin` scope that admits; by `-Wthread-safety -Werror`; and by an independent code review. ## Breaking Changes None. No protocol, serialization or RPC change. Two debug log lines have `vecSessionCollaterals.size():` renamed to `participants:`. ## 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 _(for repository code-owners and collaborators only)_ Top commit has no ACKs. Tree-SHA512: 0938a87650a83d76c71121b9defd3e0c2548f7749eaf0eb049a7c61a37344ac20011eba50a246d3bcdbcf5870f74d60b6b0fbe55578291de64f15537df353b2e (cherry picked from commit 6d5ffe8)
… strict netInfo decoding 516913a fix: return error netinfo has neither service nor domain entries (Konstantin Akimov) e3e7808 fix(evo): keep -allowprivatenet out of masternode address validation (Konstantin Akimov) 4714075 refactor(evo): compute ExtNetInfo flattened entries on demand (Konstantin Akimov) 567d146 fix(evo): require ascending known purposes in ExtNetInfo deserialization (Konstantin Akimov) 51a09b7 refactor(evo): drop entry validity checks made unreachable by strict decoding (Konstantin Akimov) 2186380 fix(evo): reject repeated purpose in ExtNetInfo deserialization (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Fixes dashpay#7864. `ExtNetInfo` read its purpose map through the generic `std::map` deserializer, which keeps the first copy of a repeated key and silently drops the rest. `Validate()` only saw the surviving map, so a ProTx whose `netInfo` carried a second `CORE_P2P` pair with an unknown entry type or an unreadable service passed validation and could be mined, while the dropped bytes were still covered by the txid. Strict third-party parsers (rust-dashcore, DashSync, dashj, Insight) reject such a transaction and with it the whole block. Reviewing the surrounding code showed that the repeated key was one of several ways one masternode configuration could have more than one accepted encoding: purposes in any order, an unknown purpose rejected only later by `Validate()`, and `NetInfoEntry` turning an unknown type byte, an invalid service or a failed service read into a cleared entry and reading on from an arbitrary stream offset. The ProUpServTx operator signature covers the re-serialized form, so every accepted variant shared one valid signature. While looking at `ValidateService` it also turned out that `CNetAddr::IsRoutable()` consults the node-local `-allowprivatenet` option, so a mainnet or testnet node started with it accepts an RFC1918 masternode address that every other node rejects. ## What was done? - fix: return error netinfo has neither service nor domain entries - fix(evo): keep -allowprivatenet out of masternode address validation - fix: make output ExtNetInfo's purposes addresses cannonical - fix: reject repeated purpose in ExtNetInfo deserialization ## How Has This Been Tested? See updates in regression tests. ## Breaking Changes it's gated beside v24 activation which is not active yet, so, no breaking changes yet. But changes are tighten consensus rules. ## Checklist: - [x] I have performed a self-review of my own code - [ ] 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)_ Top commit has no ACKs. Tree-SHA512: 62199f95fbe43d5eab3df086bf96ad75c7275a27a54d135bb2b5192fe68ad8c48af15fc9f401cbb7f6515886ea0e27bc7ed509bff14e8a5fc02ebca441ce2f6f (cherry picked from commit fad547f) Backport notes: develop's src/test/evo_netinfo_tests.cpp writes the "unspecified address" entry with CNetAddr::V2(), which comes from the serialization-parameter API of bitcoin#25284 and is not on this branch. The test writes it through an OverrideStream with ADDRV2_FORMAT instead, the way NetInfoEntry::Serialize does on this branch.
Pull requests into v24.0.x run develop's workflow files (pull_request_target), but with this branch's ci/ scripts. Since dashpay#7901, develop checks the depends cache on x86_64 light runners and passes the build runner's arch in DEPENDS_ARCH, expecting ci/test/00_setup_env_native_{multiprocess,tsan}.sh to use it. Without that, check-cache computes HOST=x86_64-pc-linux-gnu, the aarch64 build runner computes aarch64-linux-gnu, and "Build depends" fails whenever the depends cache misses. This takes only the ci/test hunks of 6f71fa5 (dashpay#7901). Without DEPENDS_ARCH the scripts fall back to uname -m, so this branch's own workflows behave as before. (partially cherry picked from commit 6f71fa5) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
added
pastaclaw:commented
PastaPastaPasta
force-pushed
the
backport/v24.0.0-rc.4
branch
from
October 16, 2026 21:37
3336e73 to
6c02b2b
Compare
thepastaclaw
removed
the
pastaclaw:commented
thepastaclaw
approved these changes
Oct 16, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of the complete backport range found no actionable defects, and the prior wallet-upgrade error-code mismatch is fixed. The nine upstream-test suggestions concern three explicitly partial backports whose excluded behavior and tests are documented in the picked commits, not missing prerequisites for the selected fixes. Current CI has passing lint, dependency builds, and the fuzz source build, with nine other source-build jobs still pending; no local builds or tests were run.
🔴 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: backport-reviewer); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: backport-reviewer); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 7: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 10: 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:
criticalbygpt-6.1-sol(effort low) — This large, intricate diff changes consensus-sensitive provider transaction validation in src/evo/specialtxman.cpp, masternode state construction in src/node/miner.cpp, network encoding in src/evo/netinfo.cpp, and wallet key handling. - 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 71% left, 5h 100% left - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- 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 xhigh); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— backport-reviewer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Track the declared bitcoin#34156 restore and migration test omissions — INTENTIONAL_EXCLUSION: 98d92f6 explicitly selects restore-directory cleanup ownership, leaves migration and restore-in-existing-directory behavior outside the backport, and states that no upstream tests are claimed. Upstream 90d651a adds those broader restore and migration scenarios, whereas HEAD retains rejection of existing destinations and adds preservation and corrupt-BDB cleanup regressions for the selected behavior. Their omission is documented scope, not an uncovered prerequisite for the ownership guard.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
thepastaclaw
added
the
pastaclaw:approved
thepastaclaw
removed
the
pastaclaw:approved
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
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
Backports for v24.0.0-rc.4, on top of
v24.0.xat 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 carriesbackport-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:masternode paymentsat the genesis block #7866 fix: stopmasternode paymentsat the genesis blocktest_failed_upgrade()expects RPC error -1 for a 257-byte mnemonic passphrase, which fix(wallet): reject mnemonic passphrases that seed derivation truncates #7842's new precheck rejects with -8 first, sowallet_upgradetohd.py --legacy-walletfails. On develop, test: update guidance for agents for writing tests and follow-up fixes for lately added tests. #7859 replaced that scenario before fix(wallet): reject mnemonic passphrases that seed derivation truncates #7842 merged. The rest of test: update guidance for agents for writing tests and follow-up fixes for lately added tests. #7859 is test cleanup and docs and is left out.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:
src/node/sync_manager.cpp, this branch still sentGETSPORKSfrom the mnsync tick on every network, because fix: sign sporks over their serialized fields and stop using sporks on mainnet #7847, which limited it to test chains, is not included. The request is removed entirely, as on develop. Mainnet spork values are hardcoded inCSporkManager::GetSporkValue, so the mainnet request had no effect.CChainstateHelperintoChainstate(d5c1bd7), which is not on this branch, so the new code goes throughChainHelper().special_tx/m_chain_helper.special_txinstead ofm_special_tx. The neighbouring develop-onlyCSpecialTxProcessor::GetDeterministicMNListHashis 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:
-reindex-chainstate; not a v24 regression), fix: gate signing intake before quorum retrieval #7833 (gate signing intake before quorum retrieval, DoS hardening), build: finish relic_conf.h regeneration before compiling BLS consumers #7682 (intermittentrelic_conf.hparallel-build failure).One CI-only commit comes last. It takes only the
ci/testhunks of 6f71fa5 (#7901), which makeci/test/00_setup_env_native_{multiprocess,tsan}.shhonorDEPENDS_ARCH. Pull requests intov24.0.xrun 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 inDEPENDS_ARCH. Without the hunks,linux64_multiprocess / Build dependsfails on any depends cache miss (Build runner computes HOST=aarch64-linux-gnu but check-cache used x86_64-pc-linux-gnu). WithoutDEPENDS_ARCHthe scripts still useuname -m, so this branch's own push CI is unchanged.No version change is needed.
configure.acis already 24.0.0 withCLIENT_VERSION_IS_RELEASEfalse, so the build takes its version string from thev24.0.0-rc.4tag.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, includingtest_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.pyandwallet_upgradetohd.py, each with--descriptorsand--legacy-walletwhere it supports them.Each pick's changed lines were compared with its develop merge (or, for fix(net): announce sporks to new peers instead of requesting them during mnsync #7893, its PR commit). They are identical except for the fix(net): announce sporks to new peers instead of requesting them during mnsync #7893, fix: mine a pending ProUpServTx before a key change of its masternode #7857 and feat(mining): build the masternode list incrementally during block assembly #7909 adaptations above, and the fix(consensus): canonical ExtNetInfo encoding and strict netInfo decoding #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 insrc/test/evo_netinfo_tests.cpp. fix(consensus): canonical ExtNetInfo encoding and strict netInfo decoding #7888's new test usedCNetAddr::V2(), the serialization-parameter API from net: Use serialization parameters for CAddress serialization bitcoin/bitcoin#25284, which this branch lacks. The pick now writes that entry through anOverrideStreamwithADDRV2_FORMAT, the wayNetInfoEntry::Serializedoes on this branch (see its commit message).fix(wallet): persist pending-observation coin locks once the record becomes readable #7782's new unit test reloads the wallet with develop's three-argument
CWalletconstructor. On this branch the constructor still takes the CoinJoin loader and anArgsManager, 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.pyis an extended test that CI does not run. fix: due to logical conflicts feature_pruning.py had been broken while #7894 is included because fix: treat missing snapshots during evodb repair as repair errors #7792 makes its restart path fail.Breaking Changes
Carried over from the picked PRs:
-reindex;-reindex-chainstateis not enough for nodes that already marked the following block invalid.netInfodecoding and validation behind V24 (see above). The RFC1918 check now applies even with-allowprivatenet.ProUpServTxspending an output of a pending operator key change, asset lock, asset unlock or MNHF signal is rejected from the mempool until that transaction confirms.doc/release-notes-7831.md).send,receiveandrefund. Wallets with other purpose strings load with warnings, andlistlabelserrors on an unknown purpose (doc/release-notes-27217.md).-allow-deprecated-long-mnemonicsis set, and over 256 bytes always fails (doc/release-notes-7842.md).-reindex, andevodb repairreports it inrepairErrors.protx info <proTxHash> <blockHash>,protx listdiffandmasternode payments <blockHash>return an error for blocks outside the active chain.protx shared_combineon aProUpSharedRegTxis 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.GETSPORKSduring mnsync (no effect, mainnet spork values are hardcoded).Checklist:
🤖 Generated with Claude Code