backport!: bitcoin#23123, #23147 (wallet rescan related, breaking changes)#7037
backport!: bitcoin#23123, #23147 (wallet rescan related, breaking changes)#7037knst wants to merge 2 commits into
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
This pull request has conflicts, please rebase. |
knst
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e29ed01e62
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| return DBErrors::LOAD_OK; | ||
| return nLoadWalletRet; |
There was a problem hiding this comment.
Because the early return for non-LOAD_OK results was removed, any wallet load error now falls through to the CoinJoin salt initialization before this return. In the scenario where WalletBatch::LoadWallet() returns CORRUPT, TOO_NEW, or NEED_REWRITE and the wallet has no salt, SetCoinJoinSalt(GetRandHash()) writes a new record to the same database even though the wallet is about to be rejected, so merely attempting to open a damaged or unsupported wallet can mutate it. Please only fall through for statuses that are meant to continue loading, or return before side effects for hard failures.
Useful? React with 👍 / 👎.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (11)
WalkthroughThis PR removes the Sequence Diagram(s)sequenceDiagram
participant WalletBatch
participant CWalletCreate
participant CWalletAttach
participant Chain
WalletBatch->>CWalletCreate: return DBErrors::NEED_RESCAN
CWalletCreate->>CWalletAttach: AttachChain(rescan_required=true)
CWalletAttach->>Chain: set rescan_height = 0 (full rescan from genesis)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/wallet/rpc/backup.cppsrc/wallet/rpc/backup.cpp:6:10: fatal error: 'chain.h' file not found ... [truncated 2200 characters] ... langFrontend__CTrans.CTrans_funct.instruction_insert_cxx_temporary_markers in file "src/clang/cTrans.ml", line 4866, characters 22-60 src/wallet/rpc/wallet.cppsrc/wallet/rpc/wallet.cpp:7:10: fatal error: 'chainparams.h' file not found ... [truncated 1061 characters] ... g/install/lib/clang/18/include" src/wallet/salvage.cppsrc/wallet/salvage.cpp:6:10: fatal error: 'fs.h' file not found ... [truncated 1029 characters] ... lang/install/lib/clang/18/include"
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wallet/wallet.cpp (1)
3504-3516:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
rescan_requiredwhen choosing the scan start.Line 3507 still gates the birthday optimization on
-rescan, but this PR removes that option. As written, the condition is effectively always true, so a wallet loaded withDBErrors::NEED_RESCANcan still skip forward totime_first_keyinstead of doing the intended full rescan from genesis.Suggested fix
- // No need to read and scan block if block was created before - // our wallet birthday (as adjusted for block time variability) - // unless a full rescan was requested - if (gArgs.GetIntArg("-rescan", 0) != 2) { + // No need to read and scan blocks created before the wallet birthday + // (as adjusted for block time variability), unless a full rescan was + // requested due to load-time corruption. + if (!rescan_required) { std::optional<int64_t> time_first_key; for (auto spk_man : walletInstance->GetAllScriptPubKeyMans()) { int64_t time = spk_man->GetTimeFirstKey(); if (!time_first_key || time < *time_first_key) time_first_key = time; } if (time_first_key) { chain.findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, FoundBlock().height(rescan_height)); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/wallet/wallet.cpp` around lines 3504 - 3516, The birthday-based scan-start optimization currently checks gArgs.GetIntArg("-rescan", 0) instead of honoring the wallet's rescan_required state, so a wallet flagged with NEED_RESCAN can still use time_first_key; change the condition to consult walletInstance->IsRescanRequired() (or the rescan_required flag) and skip the birthday shortcut when rescan is required: compute time_first_key via GetTimeFirstKey on each spk_man, and only call chain.findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, FoundBlock().height(rescan_height)) when rescan_required is false (or IsRescanRequired() returns false), otherwise preserve full rescan-from-genesis behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/wallet/wallettool.cpp`:
- Around line 95-98: The branch that handles DBErrors::NEED_RESCAN logs an error
but does not stop wallet creation; update the MakeWallet() handling so that when
load_wallet_ret == DBErrors::NEED_RESCAN it returns/aborts immediately like
other fatal load states (e.g., return nullptr or propagate the failure) instead
of continuing. Locate the branch in wallettool.cpp where load_wallet_ret is
compared to DBErrors::NEED_RESCAN and change control flow to exit the function
early after logging (refer to the existing non-recoverable error branches in
MakeWallet() for the exact return behavior to mirror).
---
Outside diff comments:
In `@src/wallet/wallet.cpp`:
- Around line 3504-3516: The birthday-based scan-start optimization currently
checks gArgs.GetIntArg("-rescan", 0) instead of honoring the wallet's
rescan_required state, so a wallet flagged with NEED_RESCAN can still use
time_first_key; change the condition to consult
walletInstance->IsRescanRequired() (or the rescan_required flag) and skip the
birthday shortcut when rescan is required: compute time_first_key via
GetTimeFirstKey on each spk_man, and only call
chain.findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW,
rescan_height, FoundBlock().height(rescan_height)) when rescan_required is false
(or IsRescanRequired() returns false), otherwise preserve full
rescan-from-genesis behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3a24a618-4273-4ee7-b018-2551e111a833
📒 Files selected for processing (17)
doc/release-notes-remove-rescan.mdsrc/dummywallet.cppsrc/init.cppsrc/wallet/init.cppsrc/wallet/rpc/backup.cppsrc/wallet/rpc/wallet.cppsrc/wallet/salvage.cppsrc/wallet/test/wallet_tests.cppsrc/wallet/wallet.cppsrc/wallet/wallet.hsrc/wallet/walletdb.cppsrc/wallet/walletdb.hsrc/wallet/wallettool.cpptest/functional/feature_notifications.pytest/functional/wallet_basic.pytest/functional/wallet_hd.pytest/functional/wallet_upgradetohd.py
💤 Files with no reviewable changes (2)
- src/wallet/init.cpp
- src/dummywallet.cpp
|
✅ Review complete (commit 2970303) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Backport of bitcoin#23123 (remove -rescan startup parameter) and bitcoin#23147 (rename DBErrors::RESCAN_REQUIRED to NEED_RESCAN) is structurally sound. Upstream API changes are correctly carried through (DBErrors::NEED_RESCAN, AttachChain(rescan_required, ...), gArgs removal). No blocking issues. Several Dash-specific adaptation cleanups remain: two leftover code blocks that still read the now-removed -rescan arg (always dead), a behavior-broadening fall-through in CWallet::LoadWallet, stale manpage text, and minor test-adaptation hygiene.
🟡 4 suggestion(s) | 💬 2 nitpick(s)
3 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/init.cpp`:
- [SUGGESTION] src/wallet/init.cpp:155-160: Dead `-rescan` mode validation after argument removal
This Dash-specific block validates `-rescan` as an integer mode (0/1/2), but this PR removes the `-rescan` argument registration in `src/wallet/init.cpp` and `src/dummywallet.cpp`. With the arg no longer registered, `gArgs.GetIntArg("-rescan", 0)` returns the default 0, the `rescan_mode < 0 || rescan_mode > 2` condition is always false, and the warning plus `ForceRemoveArg` are unreachable. Delete the block to avoid misleading readers into thinking `-rescan=1|2` is still supported.
In `src/wallet/wallet.cpp`:
- [SUGGESTION] src/wallet/wallet.cpp:3507-3516: Dead `-rescan=2` (full-genesis) branch after argument removal
This Dash-specific guard `if (gArgs.GetIntArg("-rescan", 0) != 2)` previously gave users `-rescan=2` to force a rescan from genesis (bypassing the wallet-birthday optimization). With `-rescan` removed by this PR, `GetIntArg("-rescan", 0)` is always 0, the condition is always true, and the implicit `else` (full-genesis) branch is dead. Functionally fine — `NEED_RESCAN`-driven rescans correctly use the birthday optimization — but the conditional misleads readers into thinking a removed CLI mode is still honored. Drop the `if`.
- [SUGGESTION] src/wallet/wallet.cpp:2439-2447: LoadWallet: removing the early-return broadens fall-through to all non-LOAD_OK results
The merge removes the pre-existing Dash guard `if (nLoadWalletRet != DBErrors::LOAD_OK) return nLoadWalletRet;` so the new `NEED_RESCAN` result can propagate to `CWallet::Create`. That intent is correct, but the change also makes every other non-OK result (`CORRUPT`, `TOO_NEW`, `NEED_REWRITE`, `EXTERNAL_SIGNER_SUPPORT_REQUIRED`, `NONCRITICAL_ERROR`, ...) fall through into the Dash-only CoinJoin salt write. If `SetCoinJoinSalt` fails on a wallet that is already corrupt/unusable, `LoadWallet` returns `DBErrors::LOAD_FAIL`, shadowing the more specific error code that `CWallet::Create` keys its user-facing message off (e.g. "Wallet corrupted" becomes generic "Error loading..."). Narrow the gate so only `LOAD_OK` and `NEED_RESCAN` reach the salt block.
In `doc/man/dashd.1`:
- [SUGGESTION] doc/man/dashd.1:152: Shipped manpages still advertise the removed `-rescan` option
After this PR `dashd` rejects `-rescan` on the command line and `rescan=` in the config as an unknown option. The packaged manpages still mention `-rescan` here and document `-rescan=<mode>` at `doc/man/dashd.1:554-557`; the same stale text exists in `doc/man/dash-qt.1`. Operators with `rescan=1` or `rescan=2` in `dash.conf` will hit a startup failure and the docs will mislead them. Note: this repo normally regenerates manpages at release-prep time (see `chore: regenerate manpages for v23.1.2`), so deferring to the release commit is acceptable — flagging so the regeneration isn't forgotten.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wallet/walletdb.cpp (1)
880-890:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRoute duplicate-
TXcorruption intoNEED_RESCAN.
ReadKeyValue()markswss.tx_corruptfor duplicate wallet transaction records and documents that the wallet can be repaired by rescanning, but this branch still setsDBErrors::CORRUPT. That prevents the newNEED_RESCANpath from ever reachingCWallet::Create()for this recoverable corruption case, so startup aborts instead of performing the deferred rescan this PR is adding.Suggested fix
- } else if (wss.tx_corrupt) { + } else if (wss.tx_corrupt) { pwallet->WalletLogPrintf("Error: Corrupt transaction found. This can be fixed by removing transactions from wallet and rescanning.\n"); // Set tx_corrupt back to false so that the error is only printed once (per corrupt tx) wss.tx_corrupt = false; - result = DBErrors::CORRUPT; + fNoncriticalErrors = true; + rescan_required = true; } else {Also applies to: 954-958
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/wallet/walletdb.cpp` around lines 880 - 890, ReadKeyValue() currently treats wss.tx_corrupt (duplicate TX records) as a hard CORRUPT error which prevents the new deferred rescan path; change the branch that checks wss.tx_corrupt in walletdb.cpp so that for DBKeys::TX it sets the result to DBErrors::NEED_RESCAN (and sets rescan_required / leave fNoncriticalErrors behavior) instead of DBErrors::CORRUPT, ensuring duplicate-TX corruption flows into the rescan path in CWallet::Create(); update the identical handling at the other occurrence (lines noted in review) to match.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/wallet/walletdb.cpp`:
- Around line 880-890: ReadKeyValue() currently treats wss.tx_corrupt (duplicate
TX records) as a hard CORRUPT error which prevents the new deferred rescan path;
change the branch that checks wss.tx_corrupt in walletdb.cpp so that for
DBKeys::TX it sets the result to DBErrors::NEED_RESCAN (and sets rescan_required
/ leave fNoncriticalErrors behavior) instead of DBErrors::CORRUPT, ensuring
duplicate-TX corruption flows into the rescan path in CWallet::Create(); update
the identical handling at the other occurrence (lines noted in review) to match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b4867704-53d6-47b5-9d2f-758af312daf2
📒 Files selected for processing (17)
doc/release-notes-remove-rescan.mdsrc/dummywallet.cppsrc/init.cppsrc/wallet/init.cppsrc/wallet/rpc/backup.cppsrc/wallet/rpc/wallet.cppsrc/wallet/salvage.cppsrc/wallet/test/wallet_tests.cppsrc/wallet/wallet.cppsrc/wallet/wallet.hsrc/wallet/walletdb.cppsrc/wallet/walletdb.hsrc/wallet/wallettool.cpptest/functional/feature_notifications.pytest/functional/wallet_basic.pytest/functional/wallet_hd.pytest/functional/wallet_upgradetohd.py
💤 Files with no reviewable changes (2)
- src/dummywallet.cpp
- src/wallet/init.cpp
✅ Files skipped from review due to trivial changes (5)
- src/wallet/salvage.cpp
- src/init.cpp
- src/wallet/test/wallet_tests.cpp
- doc/release-notes-remove-rescan.md
- src/wallet/rpc/backup.cpp
🚧 Files skipped from review as they are similar to previous changes (7)
- src/wallet/rpc/wallet.cpp
- test/functional/wallet_hd.py
- src/wallet/wallet.h
- src/wallet/wallettool.cpp
- test/functional/wallet_upgradetohd.py
- test/functional/wallet_basic.py
- test/functional/feature_notifications.py
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review at fdaf1e4. The latest delta (e29ed01..fdaf1e4) addresses two prior findings: prior-3 (CoinJoin salt write now gated on LOAD_OK at wallet.cpp:2443) and prior-6 (release-notes-remove-rescan.md now ends with a trailing newline). Four prior findings remain valid against the current head and are carried forward: two dead -rescan code paths (init.cpp validator, wallet.cpp -rescan=2 birthday-skip), shipped manpages still advertising the removed option, and a stale connect_nodes(0, 1) in feature_notifications.py. No new findings introduced in this delta.
🟡 3 suggestion(s)
3 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/init.cpp`:
- [SUGGESTION] src/wallet/init.cpp:155-160: Dead `-rescan` mode validation after argument removal (carried forward)
Carried forward from prior-1. This PR removes the `-rescan` argument registration (verified: no `AddArg("-rescan", ...)` remains anywhere in `src/`). Consequently `gArgs.GetIntArg("-rescan", 0)` here always returns the default 0, so `rescan_mode < 0 || rescan_mode > 2` can never trigger and the `InitWarning`/`ForceRemoveArg` paths are unreachable. Worse, if a user actually passes `-rescan=...`, argument parsing now rejects it as unknown before this validator ever runs, so the warning would be misleading even hypothetically. Removing the block keeps the PR's argument-removal story internally consistent.
In `src/wallet/wallet.cpp`:
- [SUGGESTION] src/wallet/wallet.cpp:3504-3516: Dead `-rescan=2` (full-genesis) branch after argument removal (carried forward)
Carried forward from prior-2. With `-rescan` removed, `gArgs.GetIntArg("-rescan", 0) != 2` is now always true, so the birthday-shortcut always runs — including on the new corruption-driven `NEED_RESCAN` path introduced by this PR. The release notes promise that wallets requiring rescanning due to corruption will still be rescanned on startup; previously a user could force a from-genesis rescan via `-rescan=2`, but now even corruption recovery silently advances `rescan_height` to the wallet birthday. Either gate the birthday optimization on `!rescan_required` so `NEED_RESCAN` truly scans from genesis, or otherwise preserve a real full-genesis path for the corruption case. The removed startup arg cannot remain the gate.
In `doc/man/dashd.1`:
- [SUGGESTION] doc/man/dashd.1:152-557: Shipped manpages still advertise the removed `-rescan` option (carried forward)
Carried forward from prior-4. After this PR, `dashd` rejects `-rescan` on the command line and `rescan=` in config as unknown, but `doc/man/dashd.1` still references `-rescan` at line 152 (in the `-prune` incompatibility note) and fully documents `-rescan=<mode>` at lines 554-557. The mirror entries in `doc/man/dash-qt.1` are similarly stale. Manpages are autogenerated, so the release-time regeneration step will pick this up, but flagging here so the release engineer doesn't miss it — operators consulting installed manpages will otherwise be directed to a flag that now causes startup to fail.
|
This pull request has conflicts, please rebase. |
dc3ec74 Add rescan removal release note (Samuel Dobson) bccd1d9 Remove -rescan startup parameter (Samuel Dobson) f963b0f Corrupt wallet tx shouldn't trigger rescan of all wallets (Samuel Dobson) 6c00649 Remove outdated dummy wallet -salvagewallet arg (Samuel Dobson) Pull request description: Remove the `-rescan` startup parameter. Rescans can be run with the `rescanblockchain` RPC. Rescans are still done on wallet-load if needed due to corruption, for example. ACKs for top commit: achow101: ACK dc3ec74 laanwj: re-ACK dc3ec74 Tree-SHA512: 608360d0e7d73737fd3ef408b01b33d97a75eebccd70c6d1b47a32fecb99b9105b520b111b225beb10611c09aa840a2b6d2b6e6e54be5d0362829e757289de5c Co-authored-by: W. J. van der Laan <laanwj@protonmail.com>
…ED_RESCAN 8615507 scripted-diff: rename DBErrors::RESCAN_REQUIRED to NEED_RESCAN (Samuel Dobson) Pull request description: Suggested here as a trivial follow-up: bitcoin#23123 (comment) Makes RESCAN_REQUIRED consistent with NEED_REWRITE ACKs for top commit: achow101: ACK 8615507 jonatack: ACK 8615507 Tree-SHA512: 82d057c45e192cd6dd8a47675b52699e6cbc82272609a971e6e5d6796aad14a941a70e40d3913dbb611f79c8eadff8030c60ea6f203f2edc3720c0e78c166b97 Co-authored-by: fanquake <fanquake@gmail.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR #7037 backports bitcoin#23123 (remove -rescan startup parameter) and bitcoin#23147 (rename DBErrors::RESCAN_REQUIRED to NEED_RESCAN). The latest delta (fdaf1e4..2970303) adds nothing new beyond those two merge commits. All four prior findings remain valid on the current head and are carried forward; nothing was fixed. No new in-scope blocking issues were introduced. Codex-general's two new findings about src/txdb.cpp (legacy index migration) and src/masternode/payments.cpp (old-budget validation) target files modified by earlier unrelated develop merges (#7101 and masternode reward share commits), not by this PR — they are dropped as out-of-scope.
🟡 3 suggestion(s)
3 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/init.cpp`:
- [SUGGESTION] src/wallet/init.cpp:155-160: Dead -rescan mode validation after argument removal (carried forward from prior-1)
Carried forward from prior-1 — still valid at 2970303a. This PR removes the `-rescan` argument registration from `WalletInit::AddWalletOptions`, and no `AddArg("-rescan", ...)` remains anywhere under `src/`. Because `-rescan` is no longer recognized, `gArgs.GetIntArg("-rescan", 0)` always returns the default 0 here, so `rescan_mode < 0 || rescan_mode > 2` can never be true and the warning, init-warning, and `ForceRemoveArg("rescan")` block is unreachable. This is Dash-specific residue from the older numeric `-rescan` mode that upstream bitcoin#23123 did not have. Removing it also retires the orphaned translated string `"Incorrect -rescan mode, falling back to default value"` in `src/qt/dashstrings.cpp`.
In `src/wallet/wallet.cpp`:
- [SUGGESTION] src/wallet/wallet.cpp:3468-3489: Dead `-rescan != 2` gate around the birthday shortcut (carried forward from prior-2)
Carried forward from prior-2 — still valid at 2970303a. The `if (gArgs.GetIntArg("-rescan", 0) != 2)` predicate at line 3473 is a Dash-only construct (upstream's `-rescan` was boolean and had no such gate). With the startup `-rescan` registration removed in this PR, the predicate is always true, so the birthday-shortcut now runs unconditionally — including on the corruption-driven `NEED_RESCAN` path that this PR introduces by setting `rescan_height = 0` in `CWallet::Create`. Functionally this matches upstream post-bitcoin#23123 behaviour, so it is not a regression vs. upstream, but it is dead code that misleadingly hints at a `-rescan=2` full-genesis mode. Either drop the predicate so the birthday-shortcut block is plain code, or, if the PR's release-note promise that "wallets which require rescanning due to corruption will still be rescanned" is meant to imply true genesis recovery, gate it on `!rescan_required` instead so the corruption path skips the birthday optimisation.
In `doc/man/dashd.1`:
- [SUGGESTION] doc/man/dashd.1:152-556: Shipped manpages still advertise the removed -rescan option (carried forward from prior-3)
Carried forward from prior-3 — still valid at 2970303a. After this PR, `dashd` rejects `-rescan` on the command line and `rescan=` in the config as an unknown argument, but `doc/man/dashd.1` still references `-rescan` in the `-prune` incompatibility note (line 152) and fully documents `-rescan=<mode>` (line 554). The same stale entries exist in `doc/man/dash-qt.1`. These files carry a `help2man`-generated banner and are regenerated via `contrib/devtools/gen-manpages.py` at release time, so they will refresh out-of-band; flagging here so the release manager catches the regeneration rather than shipping documentation for a removed option.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Policy gate: an agent-reported missing upstream prerequisite was restored as a blocking finding. This PR is a full Bitcoin backport, and omitted upstream hunks require their prerequisite PRs unless the finding is explicitly allowlisted (intentional_exclusion / policy_override). The agent's original evidence is preserved in the finding(s) below.
Prior verifier summary (overridden by policy gate): Backport of bitcoin#23123 (remove -rescan) and bitcoin#23147 (RESCAN_REQUIRED → NEED_RESCAN). The latest delta (fdaf1e4..2970303) introduces no new defects; the merge resolution, NotifyWalletLoading flow, AttachChain signature change, and wallettool branch are correct. All four prior findings remain present on the current head (no commits in this delta touched the affected lines) — Dash-specific dead code and stale shipped docs that survived the backport. Carried forward as suggestion/nitpick; no new blocking issues.
🔴 14 blocking | 🟡 6 suggestion(s) | 💬 1 nitpick(s)
Findings not posted inline (9)
These findings could not be anchored to the current diff, but they are still part of this review.
- [SUGGESTION]
src/wallet/init.cpp:155-160: Dead -rescan mode validation after argument removal (carried forward) — Verified at current head:int rescan_mode = gArgs.GetIntArg("-rescan", 0);and therescan_mode < 0 || rescan_mode > 2guard still live here. This PR removes the-rescanAddArg registration (confirmed: noAddArg("-rescan", ...)remains anywhere undersrc/). Consequently `gArgs.GetIntArg(... - [SUGGESTION]
src/wallet/wallet.cpp:3473-3489: Dead-rescan=2(full-genesis) gate now contradicts corruption-recovery promise (carried forward) — Verified at current head line 3473:if (gArgs.GetIntArg("-rescan", 0) != 2) {still gates the wallet-birthday shortcut. With-rescanremoved by this PR, the condition is always true, so the birthday-skip path always runs — including on the new corruption-drivenNEED_RESCANpath (where `resc... - [SUGGESTION]
doc/man/dashd.1:152-557: Shipped manpages still advertise the removed -rescan option (carried forward) — Verified at current head:doc/man/dashd.1still references-rescanin the-pruneincompatibility note at line 152 and fully documents-rescan=<mode>at lines 554–557;doc/man/dash-qt.1carries the same stale entries. After this PR,dashd/dash-qtreject-rescan/rescan=as unknown... - [BLOCKING]
src/wallet/init.cpp:155-160: Dead -rescan mode validation after argument removal (carried forward from prior-1) — Carried forward from prior review. The PR removes the-rescanAddArg registration in src/wallet/init.cpp (verified: noAddArg("-rescan", ...)remains anywhere in src/). ConsequentlygArgs.GetIntArg("-rescan", 0)at line 155 always returns the default 0, making `rescan_mode < 0 || rescan_mod... - [BLOCKING]
src/wallet/wallet.cpp:3473-3489: Dead -rescan=2 (full-genesis) branch after argument removal (carried forward from prior-2) — Carried forward from prior review. With-rescanremoved (no AddArg, no live consumers in tests),gArgs.GetIntArg("-rescan", 0) != 2at line 3473 is always true. The birthday-shortcut block (lines 3474–3489) therefore always runs — including on the corruption-drivenNEED_RESCANpath introduc... - [BLOCKING]
doc/man/dashd.1:152-557: Shipped manpages still advertise the removed -rescan option (carried forward from prior-3) — Carried forward from prior review. After this PR,dashdrejects-rescanon the command line andrescan=in config as unknown, butdoc/man/dashd.1line 152 still references-rescanin the-pruneincompatibility note, and lines 554–557 fully document-rescan=<mode>. (Also note: the li... - [BLOCKING]
src/wallet/init.cpp:155-160: Carried forward: dead -rescan mode validation after bitcoin#23123 — Carried forward from the previous review. bitcoin#23123 removes the -rescan startup option, and the current head has no remaining AddArg("-rescan", ...) registration. This Dash-specific validation block still calls gArgs.GetIntArg("-rescan", 0), so it always receives the default after argument re... - [BLOCKING]
src/wallet/wallet.cpp:3470-3489: Carried forward: stale -rescan=2 full-rescan gate after bitcoin#23123 — Carried forward from the previous review. Dash previously had -rescan=, including mode 2 for a full-genesis rescan, but bitcoin#23123 removes the startup argument. The current code still says the birthday shortcut is skipped when a full rescan was requested and gates it on gArgs.GetIntArg("... - [BLOCKING]
doc/man/dashd.1:152-557: Carried forward: shipped manpages still document removed -rescan option — Carried forward from the previous review. The code now rejects -rescan because bitcoin#23123 removed the option registration, but the generated manpage still lists -rescan in the -prune incompatibility note and still documents -rescan=. The same stale entries remain in doc/man/dash-qt.1 at...
Carried-forward findings already raised (1)
These findings were not re-posted as new inline comments because an existing review thread already covers them.
- [BLOCKING] (deduped existing open thread)
test/functional/feature_notifications.py:92-97: Stale connect_nodes(0, 1) and lost startup-rescan coverage (carried forward from prior-4) — Carried forward from prior review. The pre-backport test stopped node 1 with-rescanand reconnected nodes; this PR replaced the restart with an RPCrescanblockchain()call (line 94) but leftself.connect_nodes(0, 1)(line 97) even though node 1 was never disconnected on this path. Function...
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/init.cpp`:
- [SUGGESTION] src/wallet/init.cpp:155-160: Dead -rescan mode validation after argument removal (carried forward)
Verified at current head: `int rescan_mode = gArgs.GetIntArg("-rescan", 0);` and the `rescan_mode < 0 || rescan_mode > 2` guard still live here. This PR removes the `-rescan` AddArg registration (confirmed: no `AddArg("-rescan", ...)` remains anywhere under `src/`). Consequently `gArgs.GetIntArg("-rescan", 0)` always returns 0, the range check can never fire, and the `InitWarning(_("Incorrect -rescan mode, falling back to default value"))` plus `gArgs.ForceRemoveArg("rescan")` are unreachable. Worse, since `-rescan` is no longer a registered argument, startup parameter parsing will reject `rescan=` from config / `-rescan` from CLI as unknown before this code is even reached. This is a Dash-specific cleanup the cherry-pick missed; drop the block (and the associated translation string can be cleaned up over time).
- [BLOCKING] src/wallet/init.cpp:155-160: Dead -rescan mode validation after argument removal (carried forward from prior-1)
Carried forward from prior review. The PR removes the `-rescan` AddArg registration in src/wallet/init.cpp (verified: no `AddArg("-rescan", ...)` remains anywhere in src/). Consequently `gArgs.GetIntArg("-rescan", 0)` at line 155 always returns the default 0, making `rescan_mode < 0 || rescan_mode > 2` unreachable — the warning, the InitWarning, and `gArgs.ForceRemoveArg("rescan")` are dead code. This is a Dash-specific extension (mode 1 vs 2) that upstream bitcoin#23123 did not need to consider; the merge resolution should drop it along with the registration. Not a missing upstream prerequisite — it is a Dash-side cleanup the cherry-pick missed.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [BLOCKING] src/wallet/init.cpp:155-160: Carried forward: dead -rescan mode validation after bitcoin#23123
Carried forward from the previous review. bitcoin#23123 removes the -rescan startup option, and the current head has no remaining AddArg("-rescan", ...) registration. This Dash-specific validation block still calls gArgs.GetIntArg("-rescan", 0), so it always receives the default after argument removal and the invalid-mode warning/removal path is now dead. This is a conflict-resolution cleanup miss from adapting Dash's prior -rescan=<mode> behavior to upstream's removal.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [SUGGESTION] src/wallet/init.cpp:155-160: Dead -rescan mode validation after argument removal (carried forward)
Verified at current head: `int rescan_mode = gArgs.GetIntArg("-rescan", 0);` and the `rescan_mode < 0 || rescan_mode > 2` guard still live here. This PR removes the `-rescan` AddArg registration (confirmed: no `AddArg("-rescan", ...)` remains anywhere under `src/`). Consequently `gArgs.GetIntArg("-rescan", 0)` always returns 0, the range check can never fire, and the `InitWarning(_("Incorrect -rescan mode, falling back to default value"))` plus `gArgs.ForceRemoveArg("rescan")` are unreachable. Worse, since `-rescan` is no longer a registered argument, startup parameter parsing will reject `rescan=` from config / `-rescan` from CLI as unknown before this code is even reached. This is a Dash-specific cleanup the cherry-pick missed; drop the block (and the associated translation string can be cleaned up over time).
- [BLOCKING] src/wallet/init.cpp:155-160: Dead -rescan mode validation after argument removal (carried forward from prior-1)
Carried forward from prior review. The PR removes the `-rescan` AddArg registration in src/wallet/init.cpp (verified: no `AddArg("-rescan", ...)` remains anywhere in src/). Consequently `gArgs.GetIntArg("-rescan", 0)` at line 155 always returns the default 0, making `rescan_mode < 0 || rescan_mode > 2` unreachable — the warning, the InitWarning, and `gArgs.ForceRemoveArg("rescan")` are dead code. This is a Dash-specific extension (mode 1 vs 2) that upstream bitcoin#23123 did not need to consider; the merge resolution should drop it along with the registration. Not a missing upstream prerequisite — it is a Dash-side cleanup the cherry-pick missed.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [BLOCKING] src/wallet/init.cpp:155-160: Carried forward: dead -rescan mode validation after bitcoin#23123
Carried forward from the previous review. bitcoin#23123 removes the -rescan startup option, and the current head has no remaining AddArg("-rescan", ...) registration. This Dash-specific validation block still calls gArgs.GetIntArg("-rescan", 0), so it always receives the default after argument removal and the invalid-mode warning/removal path is now dead. This is a conflict-resolution cleanup miss from adapting Dash's prior -rescan=<mode> behavior to upstream's removal.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
In `src/wallet/wallet.cpp`:
- [SUGGESTION] src/wallet/wallet.cpp:3473-3489: Dead `-rescan=2` (full-genesis) gate now contradicts corruption-recovery promise (carried forward)
Verified at current head line 3473: `if (gArgs.GetIntArg("-rescan", 0) != 2) {` still gates the wallet-birthday shortcut. With `-rescan` removed by this PR, the condition is always true, so the birthday-skip path always runs — including on the new corruption-driven `NEED_RESCAN` path (where `rescan_required=true` intentionally keeps `rescan_height=0`). That can advance `rescan_height` from genesis up toward `*tip_height` during automatic recovery, undercutting the release-note promise that wallets requiring rescanning due to corruption are still rescanned on startup. The fix is either to drop the dead `!= 2` guard (matching upstream Bitcoin's `AttachChain`) and unconditionally run the birthday optimization, or — if a full-genesis recovery path must be preserved for the NEED_RESCAN case — gate the birthday optimization on `!rescan_required` instead.
- [BLOCKING] src/wallet/wallet.cpp:3473-3489: Dead -rescan=2 (full-genesis) branch after argument removal (carried forward from prior-2)
Carried forward from prior review. With `-rescan` removed (no AddArg, no live consumers in tests), `gArgs.GetIntArg("-rescan", 0) != 2` at line 3473 is always true. The birthday-shortcut block (lines 3474–3489) therefore always runs — including on the corruption-driven `NEED_RESCAN` path introduced by this PR (line 3128 → rescan_required=true). The PR description promises that wallets requiring rescanning due to corruption will still be rescanned on startup; today that means from the wallet birthday onward, not from genesis. If a full-genesis recovery rescan was previously available via Dash's `-rescan=2`, that path is now gone. Either drop the dead `gArgs.GetIntArg("-rescan", 0) != 2` guard, or gate the birthday optimization on `!rescan_required` so corruption recovery preserves full-rescan semantics.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [BLOCKING] src/wallet/wallet.cpp:3470-3489: Carried forward: stale -rescan=2 full-rescan gate after bitcoin#23123
Carried forward from the previous review. Dash previously had -rescan=<mode>, including mode 2 for a full-genesis rescan, but bitcoin#23123 removes the startup argument. The current code still says the birthday shortcut is skipped when a full rescan was requested and gates it on gArgs.GetIntArg("-rescan", 0) != 2. Since -rescan is no longer registered, that condition is always true. Upstream has no mode=2 path, so this is not a missing Bitcoin prerequisite, but it is an incomplete Dash conflict resolution that leaves stale mode-specific logic in the rescan path.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [SUGGESTION] src/wallet/wallet.cpp:3473-3489: Dead `-rescan=2` (full-genesis) gate now contradicts corruption-recovery promise (carried forward)
Verified at current head line 3473: `if (gArgs.GetIntArg("-rescan", 0) != 2) {` still gates the wallet-birthday shortcut. With `-rescan` removed by this PR, the condition is always true, so the birthday-skip path always runs — including on the new corruption-driven `NEED_RESCAN` path (where `rescan_required=true` intentionally keeps `rescan_height=0`). That can advance `rescan_height` from genesis up toward `*tip_height` during automatic recovery, undercutting the release-note promise that wallets requiring rescanning due to corruption are still rescanned on startup. The fix is either to drop the dead `!= 2` guard (matching upstream Bitcoin's `AttachChain`) and unconditionally run the birthday optimization, or — if a full-genesis recovery path must be preserved for the NEED_RESCAN case — gate the birthday optimization on `!rescan_required` instead.
- [BLOCKING] src/wallet/wallet.cpp:3473-3489: Dead -rescan=2 (full-genesis) branch after argument removal (carried forward from prior-2)
Carried forward from prior review. With `-rescan` removed (no AddArg, no live consumers in tests), `gArgs.GetIntArg("-rescan", 0) != 2` at line 3473 is always true. The birthday-shortcut block (lines 3474–3489) therefore always runs — including on the corruption-driven `NEED_RESCAN` path introduced by this PR (line 3128 → rescan_required=true). The PR description promises that wallets requiring rescanning due to corruption will still be rescanned on startup; today that means from the wallet birthday onward, not from genesis. If a full-genesis recovery rescan was previously available via Dash's `-rescan=2`, that path is now gone. Either drop the dead `gArgs.GetIntArg("-rescan", 0) != 2` guard, or gate the birthday optimization on `!rescan_required` so corruption recovery preserves full-rescan semantics.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [BLOCKING] src/wallet/wallet.cpp:3470-3489: Carried forward: stale -rescan=2 full-rescan gate after bitcoin#23123
Carried forward from the previous review. Dash previously had -rescan=<mode>, including mode 2 for a full-genesis rescan, but bitcoin#23123 removes the startup argument. The current code still says the birthday shortcut is skipped when a full rescan was requested and gates it on gArgs.GetIntArg("-rescan", 0) != 2. Since -rescan is no longer registered, that condition is always true. Upstream has no mode=2 path, so this is not a missing Bitcoin prerequisite, but it is an incomplete Dash conflict resolution that leaves stale mode-specific logic in the rescan path.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
In `doc/man/dashd.1`:
- [SUGGESTION] doc/man/dashd.1:152-557: Shipped manpages still advertise the removed -rescan option (carried forward)
Verified at current head: `doc/man/dashd.1` still references `-rescan` in the `-prune` incompatibility note at line 152 and fully documents `-rescan=<mode>` at lines 554–557; `doc/man/dash-qt.1` carries the same stale entries. After this PR, `dashd`/`dash-qt` reject `-rescan`/`rescan=` as unknown, so the shipped docs contradict actual behavior and will steer users to a removed option instead of `rescanblockchain`. Note also that the line 152 `-prune` incompatibility list no longer matches `src/init.cpp:595` (which now lists `-txindex, -addressindex, -spentindex and -disablegovernance=false`). Regenerate the manpages via `contrib/devtools/gen-manpages.py`, or call out a deferred regen pass explicitly in the PR description.
- [BLOCKING] doc/man/dashd.1:152-557: Shipped manpages still advertise the removed -rescan option (carried forward from prior-3)
Carried forward from prior review. After this PR, `dashd` rejects `-rescan` on the command line and `rescan=` in config as unknown, but `doc/man/dashd.1` line 152 still references `-rescan` in the `-prune` incompatibility note, and lines 554–557 fully document `-rescan=<mode>`. (Also note: the line 152 incompatibility list does not even match current src/init.cpp:595, which lists `-txindex, -addressindex, -spentindex and -disablegovernance=false`.) The mirror entries in `doc/man/dash-qt.1` are similarly stale. Regenerate the manpages so they reflect the removed option.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [BLOCKING] doc/man/dashd.1:152-557: Carried forward: shipped manpages still document removed -rescan option
Carried forward from the previous review. The code now rejects -rescan because bitcoin#23123 removed the option registration, but the generated manpage still lists -rescan in the -prune incompatibility note and still documents -rescan=<mode>. The same stale entries remain in doc/man/dash-qt.1 at lines 154 and 556-559. This leaves shipped user documentation inconsistent with the backport.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [SUGGESTION] doc/man/dashd.1:152-557: Shipped manpages still advertise the removed -rescan option (carried forward)
Verified at current head: `doc/man/dashd.1` still references `-rescan` in the `-prune` incompatibility note at line 152 and fully documents `-rescan=<mode>` at lines 554–557; `doc/man/dash-qt.1` carries the same stale entries. After this PR, `dashd`/`dash-qt` reject `-rescan`/`rescan=` as unknown, so the shipped docs contradict actual behavior and will steer users to a removed option instead of `rescanblockchain`. Note also that the line 152 `-prune` incompatibility list no longer matches `src/init.cpp:595` (which now lists `-txindex, -addressindex, -spentindex and -disablegovernance=false`). Regenerate the manpages via `contrib/devtools/gen-manpages.py`, or call out a deferred regen pass explicitly in the PR description.
- [BLOCKING] doc/man/dashd.1:152-557: Shipped manpages still advertise the removed -rescan option (carried forward from prior-3)
Carried forward from prior review. After this PR, `dashd` rejects `-rescan` on the command line and `rescan=` in config as unknown, but `doc/man/dashd.1` line 152 still references `-rescan` in the `-prune` incompatibility note, and lines 554–557 fully document `-rescan=<mode>`. (Also note: the line 152 incompatibility list does not even match current src/init.cpp:595, which lists `-txindex, -addressindex, -spentindex and -disablegovernance=false`.) The mirror entries in `doc/man/dash-qt.1` are similarly stale. Regenerate the manpages so they reflect the removed option.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [BLOCKING] doc/man/dashd.1:152-557: Carried forward: shipped manpages still document removed -rescan option
Carried forward from the previous review. The code now rejects -rescan because bitcoin#23123 removed the option registration, but the generated manpage still lists -rescan in the -prune incompatibility note and still documents -rescan=<mode>. The same stale entries remain in doc/man/dash-qt.1 at lines 154 and 556-559. This leaves shipped user documentation inconsistent with the backport.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
In `test/functional/feature_notifications.py`:
- [BLOCKING] test/functional/feature_notifications.py:92-97: Carried forward: stale reconnect after replacing startup rescan with RPC rescan
Carried forward from the previous review. bitcoin#23123 replaced the notification test's startup -rescan coverage with rescanblockchain(), and Dash made the same adaptation, but the test still calls self.connect_nodes(0, 1) even though node 1 is no longer stopped or restarted on this path. This is functionally harmless, but it is stale test adaptation from the removed startup-rescan flow and the test no longer covers startup-rescan notifications.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [BLOCKING] test/functional/feature_notifications.py:92-97: Stale connect_nodes(0, 1) and lost startup-rescan coverage (carried forward from prior-4)
Carried forward from prior review. The pre-backport test stopped node 1 with `-rescan` and reconnected nodes; this PR replaced the restart with an RPC `rescanblockchain()` call (line 94) but left `self.connect_nodes(0, 1)` (line 97) even though node 1 was never disconnected on this path. Functionally harmless, but stale, and the test no longer covers startup-rescan notifications. Remove the stale reconnect and make the log text explicitly describe `rescanblockchain()` RPC coverage; if startup-recovery coverage is still required, add it separately (e.g., via a corruption-driven NEED_RESCAN scenario).
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
| self.nodes[1].rescanblockchain() | ||
| self.wait_until(lambda: len(os.listdir(self.walletnotify_dir)) == block_count, timeout=10) | ||
|
|
||
| self.connect_nodes(0, 1) |
There was a problem hiding this comment.
💬 Nitpick: Stale connect_nodes(0, 1) after switch from startup -rescan to rescanblockchain() (carried forward)
Verified at current head: the test replaces the prior stop/restart-with--rescan of node 1 with an in-process self.nodes[1].rescanblockchain() at line 94 but still calls self.connect_nodes(0, 1) at line 97 even though node 1 is never disconnected on this path. The reconnect is a harmless no-op, but it is stale flow and the case no longer exercises startup-rescan walletnotify coverage. Drop the stale reconnect and update the log text to describe the rescanblockchain() RPC coverage explicitly; if startup-recovery wallet notifications are still considered worth testing, add a separate stop/start scenario that triggers NEED_RESCAN via simulated wallet corruption.
source: ['claude', 'codex']
| self.log.info("test -walletnotify after rescan") | ||
| # restart node to rescan to force wallet notifications | ||
| self.start_node(1) | ||
| force_finish_mnsync(self.nodes[1]) | ||
| self.connect_nodes(0, 1) | ||
|
|
||
| # rescan to force wallet notifications | ||
| self.nodes[1].rescanblockchain() | ||
| self.wait_until(lambda: len(os.listdir(self.walletnotify_dir)) == block_count, timeout=10) | ||
|
|
||
| self.connect_nodes(0, 1) |
There was a problem hiding this comment.
🔴 Blocking: Carried forward: stale reconnect after replacing startup rescan with RPC rescan
Carried forward from the previous review. bitcoin#23123 replaced the notification test's startup -rescan coverage with rescanblockchain(), and Dash made the same adaptation, but the test still calls self.connect_nodes(0, 1) even though node 1 is no longer stopped or restarted on this path. This is functionally harmless, but it is stale test adaptation from the removed startup-rescan flow and the test no longer covers startup-rescan notifications.
Policy gate (backport-prereq-restore): For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. intentional_exclusion: true or a matching entry in policy_overrides). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
source: ['codex-backport-reviewer']
There was a problem hiding this comment.
Resolved in this update — Carried forward: stale reconnect after replacing startup rescan with RPC rescan no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Correction to review 4582622388: the backport policy gate over-escalated duplicated carried-forward findings as blockers. Using the verifier output before that policy bug: no new blocking issues were found; the remaining items are carried-forward suggestions/nitpicks.
Backport of bitcoin#23123 (remove -rescan) and bitcoin#23147 (RESCAN_REQUIRED → NEED_RESCAN). The latest delta (fdaf1e4..2970303) introduces no new defects; the merge resolution, NotifyWalletLoading flow, AttachChain signature change, and wallettool branch are correct. All four prior findings remain present on the current head (no commits in this delta touched the affected lines) — Dash-specific dead code and stale shipped docs that survived the backport. Carried forward as suggestion/nitpick; no new blocking issues.
🟡 6 suggestion(s) | 💬 1 nitpick(s)
Findings not posted inline (3)
These findings could not be anchored to the current diff, but they are still part of this review.
- [SUGGESTION]
src/wallet/init.cpp:155-160: Dead -rescan mode validation after argument removal (carried forward) — Verified at current head:int rescan_mode = gArgs.GetIntArg("-rescan", 0);and therescan_mode < 0 || rescan_mode > 2guard still live here. This PR removes the-rescanAddArg registration (confirmed: noAddArg("-rescan", ...)remains anywhere undersrc/). Consequently `gArgs.GetIntArg(... - [SUGGESTION]
src/wallet/wallet.cpp:3473-3489: Dead-rescan=2(full-genesis) gate now contradicts corruption-recovery promise (carried forward) — Verified at current head line 3473:if (gArgs.GetIntArg("-rescan", 0) != 2) {still gates the wallet-birthday shortcut. With-rescanremoved by this PR, the condition is always true, so the birthday-skip path always runs — including on the new corruption-drivenNEED_RESCANpath (where `resc... - [SUGGESTION]
doc/man/dashd.1:152-557: Shipped manpages still advertise the removed -rescan option (carried forward) — Verified at current head:doc/man/dashd.1still references-rescanin the-pruneincompatibility note at line 152 and fully documents-rescan=<mode>at lines 554–557;doc/man/dash-qt.1carries the same stale entries. After this PR,dashd/dash-qtreject-rescan/rescan=as unknown...
Carried-forward findings already raised (1)
These findings were not re-posted as new inline comments because an existing review thread already covers them.
- [NITPICK] (deduped existing open thread)
test/functional/feature_notifications.py:97: Stale connect_nodes(0, 1) after switch from startup -rescan to rescanblockchain() (carried forward) — Verified at current head: the test replaces the prior stop/restart-with--rescanof node 1 with an in-processself.nodes[1].rescanblockchain()at line 94 but still callsself.connect_nodes(0, 1)at line 97 even though node 1 is never disconnected on this path. The reconnect is a harmless no-...
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/init.cpp`:
- [SUGGESTION] src/wallet/init.cpp:155-160: Dead -rescan mode validation after argument removal (carried forward)
Verified at current head: `int rescan_mode = gArgs.GetIntArg("-rescan", 0);` and the `rescan_mode < 0 || rescan_mode > 2` guard still live here. This PR removes the `-rescan` AddArg registration (confirmed: no `AddArg("-rescan", ...)` remains anywhere under `src/`). Consequently `gArgs.GetIntArg("-rescan", 0)` always returns 0, the range check can never fire, and the `InitWarning(_("Incorrect -rescan mode, falling back to default value"))` plus `gArgs.ForceRemoveArg("rescan")` are unreachable. Worse, since `-rescan` is no longer a registered argument, startup parameter parsing will reject `rescan=` from config / `-rescan` from CLI as unknown before this code is even reached. This is a Dash-specific cleanup the cherry-pick missed; drop the block (and the associated translation string can be cleaned up over time).
- [SUGGESTION] src/wallet/init.cpp:155-160: Dead -rescan mode validation after argument removal (carried forward)
Verified at current head: `int rescan_mode = gArgs.GetIntArg("-rescan", 0);` and the `rescan_mode < 0 || rescan_mode > 2` guard still live here. This PR removes the `-rescan` AddArg registration (confirmed: no `AddArg("-rescan", ...)` remains anywhere under `src/`). Consequently `gArgs.GetIntArg("-rescan", 0)` always returns 0, the range check can never fire, and the `InitWarning(_("Incorrect -rescan mode, falling back to default value"))` plus `gArgs.ForceRemoveArg("rescan")` are unreachable. Worse, since `-rescan` is no longer a registered argument, startup parameter parsing will reject `rescan=` from config / `-rescan` from CLI as unknown before this code is even reached. This is a Dash-specific cleanup the cherry-pick missed; drop the block (and the associated translation string can be cleaned up over time).
In `src/wallet/wallet.cpp`:
- [SUGGESTION] src/wallet/wallet.cpp:3473-3489: Dead `-rescan=2` (full-genesis) gate now contradicts corruption-recovery promise (carried forward)
Verified at current head line 3473: `if (gArgs.GetIntArg("-rescan", 0) != 2) {` still gates the wallet-birthday shortcut. With `-rescan` removed by this PR, the condition is always true, so the birthday-skip path always runs — including on the new corruption-driven `NEED_RESCAN` path (where `rescan_required=true` intentionally keeps `rescan_height=0`). That can advance `rescan_height` from genesis up toward `*tip_height` during automatic recovery, undercutting the release-note promise that wallets requiring rescanning due to corruption are still rescanned on startup. The fix is either to drop the dead `!= 2` guard (matching upstream Bitcoin's `AttachChain`) and unconditionally run the birthday optimization, or — if a full-genesis recovery path must be preserved for the NEED_RESCAN case — gate the birthday optimization on `!rescan_required` instead.
- [SUGGESTION] src/wallet/wallet.cpp:3473-3489: Dead `-rescan=2` (full-genesis) gate now contradicts corruption-recovery promise (carried forward)
Verified at current head line 3473: `if (gArgs.GetIntArg("-rescan", 0) != 2) {` still gates the wallet-birthday shortcut. With `-rescan` removed by this PR, the condition is always true, so the birthday-skip path always runs — including on the new corruption-driven `NEED_RESCAN` path (where `rescan_required=true` intentionally keeps `rescan_height=0`). That can advance `rescan_height` from genesis up toward `*tip_height` during automatic recovery, undercutting the release-note promise that wallets requiring rescanning due to corruption are still rescanned on startup. The fix is either to drop the dead `!= 2` guard (matching upstream Bitcoin's `AttachChain`) and unconditionally run the birthday optimization, or — if a full-genesis recovery path must be preserved for the NEED_RESCAN case — gate the birthday optimization on `!rescan_required` instead.
In `doc/man/dashd.1`:
- [SUGGESTION] doc/man/dashd.1:152-557: Shipped manpages still advertise the removed -rescan option (carried forward)
Verified at current head: `doc/man/dashd.1` still references `-rescan` in the `-prune` incompatibility note at line 152 and fully documents `-rescan=<mode>` at lines 554–557; `doc/man/dash-qt.1` carries the same stale entries. After this PR, `dashd`/`dash-qt` reject `-rescan`/`rescan=` as unknown, so the shipped docs contradict actual behavior and will steer users to a removed option instead of `rescanblockchain`. Note also that the line 152 `-prune` incompatibility list no longer matches `src/init.cpp:595` (which now lists `-txindex, -addressindex, -spentindex and -disablegovernance=false`). Regenerate the manpages via `contrib/devtools/gen-manpages.py`, or call out a deferred regen pass explicitly in the PR description.
- [SUGGESTION] doc/man/dashd.1:152-557: Shipped manpages still advertise the removed -rescan option (carried forward)
Verified at current head: `doc/man/dashd.1` still references `-rescan` in the `-prune` incompatibility note at line 152 and fully documents `-rescan=<mode>` at lines 554–557; `doc/man/dash-qt.1` carries the same stale entries. After this PR, `dashd`/`dash-qt` reject `-rescan`/`rescan=` as unknown, so the shipped docs contradict actual behavior and will steer users to a removed option instead of `rescanblockchain`. Note also that the line 152 `-prune` incompatibility list no longer matches `src/init.cpp:595` (which now lists `-txindex, -addressindex, -spentindex and -disablegovernance=false`). Regenerate the manpages via `contrib/devtools/gen-manpages.py`, or call out a deferred regen pass explicitly in the PR description.
3 participants
What was done?
Various backports that have breaking changes. They are for
XmasDash Core v24How Has This Been Tested?
Run unit & functional tests
Breaking Changes
The
-rescanstartup parameter has been removed. Wallets which requirerescanning due to corruption will still be rescanned on startup.
Otherwise, please use the
rescanblockchainRPC to trigger a rescan.Checklist:
Go over all the following points, and put an
xin all the boxes that apply.