backport: Merge bitcoin/bitcoin#28815, 28931, 26679#7092
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
vijaydasmp
force-pushed
the
Jan_2026_6
branch
3 times, most recently
from
66b11f8 to
c027286
Compare
vijaydasmp
changed the title
vijaydasmp
force-pushed
the
Jan_2026_6
branch
from
bc3ba7e to
b076563
Compare
vijaydasmp
changed the title
vijaydasmp
force-pushed
the
Jan_2026_6
branch
from
b8a6809 to
b076563
Compare
|
This pull request has conflicts, please rebase. |
vijaydasmp
force-pushed
the
Jan_2026_6
branch
from
5e143f3 to
63a91ab
Compare
|
This pull request has conflicts, please rebase. |
vijaydasmp
force-pushed
the
Jan_2026_6
branch
from
63a91ab to
c8eba8f
Compare
vijaydasmp
changed the title
vijaydasmp
force-pushed
the
Jan_2026_6
branch
from
c8eba8f to
0bed3e3
Compare
vijaydasmp
changed the title
vijaydasmp
force-pushed
the
Jan_2026_6
branch
from
0bed3e3 to
1c0e1e6
Compare
vijaydasmp
changed the title
WalkthroughThis PR updates header includes and forward declarations in src/script/interpreter.h, adds a propagated good_data guard to multiple fuzz tests (bloom_filter, coins_view, policy_estimator, rpc, script_flags) so fuzz loops abort on invalid deserialization, expands LIMITED_WHILE documentation, and modifies CWallet::AttachChain to compute the earliest ScriptPubKeyMan key time and call findFirstBlockWithTimeAndHeight to adjust or skip rescans based on the chain tip and any matching block. Sequence Diagram(s)sequenceDiagram
participant Wallet as CWallet
participant KeyMgr as ScriptPubKeyMan (all)
participant Chain as Chainstate / BlockIndex
participant Rescan as Rescan logic
Wallet->>KeyMgr: query time_first_key across managers
alt time_first_key found
KeyMgr-->>Wallet: time_first_key
Wallet->>Chain: findFirstBlockWithTimeAndHeight(time_first_key)
alt matching block found
Chain-->>Wallet: block_height
Wallet->>Rescan: start rescan at block_height
else no block found
Chain-->>Wallet: no block
Wallet->>Rescan: set rescan height = chain tip (skip rescan)
end
else no time_first_key
Wallet->>Rescan: set rescan height = chain tip (skip rescan)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator) | ||
| { | ||
| FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); | ||
| bool good_data{true}; |
There was a problem hiding this comment.
nit: formatting is different (two missing \n) with original 28815:
| class uint256; | ||
| class CScriptNum; | ||
| class XOnlyPubKey; | ||
| struct CScriptWitness; |
There was a problem hiding this comment.
nit(28931): remove CScriptWitness because no segwit in dash core
| // 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) { |
There was a problem hiding this comment.
26679: bitcoin doesn't have a line if (gArgs.GetIntArg("-rescan", 0) != 2) { and the previous unless a full rescan was requested which has been introduced by #3828
This -rescan feature is subject to remove in Dash Core v24; but it it is still not removed; please fix 26679 for now.
vijaydasmp
force-pushed
the
Jan_2026_6
branch
from
1c0e1e6 to
4687382
Compare
![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
♻️ Duplicate comments (2)
src/wallet/wallet.cpp (1)
3284-3300:⚠️ Potential issue | 🟠 MajorPreserve full-rescan semantics for
-rescan=2.This wallet-birthday shortcut now runs unconditionally. When
-rescan=2is set,rescan_heightstarts at 0 but gets advanced to the wallet birth time (or tip), so a “full rescan” no longer scans from genesis. Please gate this block behind the existing-rescan != 2condition (and drop the now-empty check later) to keep the full-rescan option intact.Suggested fix (guard the shortcut)
- // No need to read and scan block if block was created before - // our wallet birthday (as adjusted for block time variability) - 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) { - FoundBlock found = FoundBlock().height(rescan_height); - chain.findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, found); - if (!found.found) { - // We were unable to find a block that had a time more recent than our earliest timestamp - // or a height higher than the wallet was synced to, indicating that the wallet is newer than the - // current chain tip. Skip rescanning in this case. - rescan_height = *tip_height; - } - } + if (gArgs.GetIntArg("-rescan", 0) != 2) { + // No need to read and scan block if block was created before + // our wallet birthday (as adjusted for block time variability) + 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) { + FoundBlock found = FoundBlock().height(rescan_height); + chain.findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, found); + if (!found.found) { + // We were unable to find a block that had a time more recent than our earliest timestamp + // or a height higher than the wallet was synced to, indicating that the wallet is newer than the + // current chain tip. Skip rescanning in this case. + rescan_height = *tip_height; + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.cpp` around lines 3284 - 3300, The unconditional wallet-birthday shortcut that advances rescan_height should be skipped when the user requested a full rescan via -rescan=2; wrap the whole block that computes time_first_key, calls GetAllScriptPubKeyMans()/GetTimeFirstKey(), and invokes chain.findFirstBlockWithTimeAndHeight(...) in a guard that checks the rescan mode is not the special full-rescan value (i.e. only run when -rescan != 2), and remove or avoid leaving any now-empty conditional later that handled this case; ensure rescan_height remains 0 for the full-rescan path so genesis is scanned.src/test/fuzz/policy_estimator.cpp (1)
24-27: Formatting divergence from upstream bitcoin#28815 already noted.Two missing newlines around
bool good_data{true};and the trailing whitespace on line 27 diverge from the original upstream diff. This was already flagged in review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/policy_estimator.cpp` around lines 24 - 27, Add the missing blank line(s) around the declaration bool good_data{true}; so it matches upstream formatting (one blank line before and after the declaration) and remove the trailing whitespace at the end of the LIMITED_WHILE(...) line; update the lines containing the bool good_data{true}; declaration and the LIMITED_WHILE(...) invocation to ensure proper spacing and no trailing spaces.
🧹 Nitpick comments (1)
src/script/interpreter.h (1)
23-23: Remove the unusedXOnlyPubKeyforward declaration.
XOnlyPubKeyis a Taproot/BIP-340 type that Dash does not support. The forward declaration at line 23 is unused throughout the codebase and should be removed alongside any other taproot-specific references.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/script/interpreter.h` at line 23, Remove the unused forward declaration of the Taproot/BIP-340 type XOnlyPubKey from interpreter.h: delete the line declaring "class XOnlyPubKey;" and search the same header and related source files for any remaining taproot-specific references or comments (e.g., XOnlyPubKey mentions) to remove or replace so no unused forward declarations or Dash-incompatible taproot types remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/script/interpreter.h`:
- Around line 9-12: In src/script/interpreter.h remove the pre-existing `#include`
<script/script_error.h> duplicate and keep only the annotated include line that
has "// IWYU pragma: export"; ensure the file contains a single include for
script_error.h (the one with the pragma) and that no other duplicate includes
remain.
---
Duplicate comments:
In `@src/test/fuzz/policy_estimator.cpp`:
- Around line 24-27: Add the missing blank line(s) around the declaration bool
good_data{true}; so it matches upstream formatting (one blank line before and
after the declaration) and remove the trailing whitespace at the end of the
LIMITED_WHILE(...) line; update the lines containing the bool good_data{true};
declaration and the LIMITED_WHILE(...) invocation to ensure proper spacing and
no trailing spaces.
In `@src/wallet/wallet.cpp`:
- Around line 3284-3300: The unconditional wallet-birthday shortcut that
advances rescan_height should be skipped when the user requested a full rescan
via -rescan=2; wrap the whole block that computes time_first_key, calls
GetAllScriptPubKeyMans()/GetTimeFirstKey(), and invokes
chain.findFirstBlockWithTimeAndHeight(...) in a guard that checks the rescan
mode is not the special full-rescan value (i.e. only run when -rescan != 2), and
remove or avoid leaving any now-empty conditional later that handled this case;
ensure rescan_height remains 0 for the full-rescan path so genesis is scanned.
---
Nitpick comments:
In `@src/script/interpreter.h`:
- Line 23: Remove the unused forward declaration of the Taproot/BIP-340 type
XOnlyPubKey from interpreter.h: delete the line declaring "class XOnlyPubKey;"
and search the same header and related source files for any remaining
taproot-specific references or comments (e.g., XOnlyPubKey mentions) to remove
or replace so no unused forward declarations or Dash-incompatible taproot types
remain.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/script/interpreter.hsrc/test/fuzz/bloom_filter.cppsrc/test/fuzz/coins_view.cppsrc/test/fuzz/fuzz.hsrc/test/fuzz/policy_estimator.cppsrc/test/fuzz/rpc.cppsrc/test/fuzz/script_flags.cppsrc/wallet/wallet.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/fuzz/fuzz.h
| #include <script/script_error.h> | ||
| #include <consensus/amount.h> | ||
| #include <primitives/transaction.h> | ||
| #include <script/script_error.h> // IWYU pragma: export |
There was a problem hiding this comment.
Remove the pre-existing script_error.h include — it is now duplicated.
Lines 9 and 12 include the same header. The upstream backport was intended to annotate the existing include with // IWYU pragma: export (a single-line edit), but the backport landed as an additional line, leaving both. Line 9 should be removed.
🔧 Proposed fix
-#include <script/script_error.h>
`#include` <consensus/amount.h>
`#include` <primitives/transaction.h>
`#include` <script/script_error.h> // IWYU pragma: export📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <script/script_error.h> | |
| #include <consensus/amount.h> | |
| #include <primitives/transaction.h> | |
| #include <script/script_error.h> // IWYU pragma: export | |
| `#include` <consensus/amount.h> | |
| `#include` <primitives/transaction.h> | |
| `#include` <script/script_error.h> // IWYU pragma: export |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/script/interpreter.h` around lines 9 - 12, In src/script/interpreter.h
remove the pre-existing `#include` <script/script_error.h> duplicate and keep only
the annotated include line that has "// IWYU pragma: export"; ensure the file
contains a single include for script_error.h (the one with the pragma) and that
no other duplicate includes remain.
4687382 to
75fb4a7
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/script/interpreter.h (1)
9-12:⚠️ Potential issue | 🟡 MinorRemove the duplicate
script_error.hinclude.Line 9 duplicates Line 12. Keep only the
// IWYU pragma: exportinclude to avoid redundant includes.Based on learnings, backported changes should match upstream PRs closely.🔧 Proposed fix
-#include <script/script_error.h> `#include` <consensus/amount.h> `#include` <primitives/transaction.h> `#include` <script/script_error.h> // IWYU pragma: export🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/script/interpreter.h` around lines 9 - 12, The header includes in interpreter.h contain a duplicate include of script_error.h; remove the redundant plain include and keep only the line that has "script/script_error.h" with the "// IWYU pragma: export" comment so that the file retains the intended export pragma and avoids redundant includes (update the include list involving script_error.h accordingly).src/wallet/wallet.cpp (1)
3286-3300:⚠️ Potential issue | 🟠 Major
-rescan=2full-rescan behavior is effectively bypassed.At Line 3291, the key-birthday optimization now runs unconditionally. That can advance
rescan_heightand skip historical blocks even when a full rescan is requested.Based on learnings, behavior changes in backport PRs should be avoided unless explicitly intended.🔧 Proposed fix
- if (time_first_key) { + if (time_first_key && gArgs.GetIntArg("-rescan", 0) != 2) { FoundBlock found = FoundBlock().height(rescan_height); chain.findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, found); if (!found.found) { // We were unable to find a block that had a time more recent than our earliest timestamp // or a height higher than the wallet was synced to, indicating that the wallet is newer than the // current chain tip. Skip rescanning in this case. rescan_height = *tip_height; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.cpp` around lines 3286 - 3300, The key-birthday optimization (loop computing time_first_key and calling chain.findFirstBlockWithTimeAndHeight which can advance rescan_height) runs unconditionally and can bypass an explicit full-rescan request (-rescan=2); wrap the entire time_first_key computation and subsequent chain.findFirstBlockWithTimeAndHeight/rescan_height update in a guard that skips this optimization when a full rescan was requested (e.g. check the full-rescan flag or mode before the loop and only run the time_first_key/FoundBlock logic when NOT doing a full rescan), leaving the rest of rescan logic unchanged; ensure the guard references the same symbols (time_first_key, walletInstance->GetAllScriptPubKeyMans(), chain.findFirstBlockWithTimeAndHeight, rescan_height, tip_height) so the behavior only changes when full rescan is requested.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/script/interpreter.h`:
- Around line 9-12: The header includes in interpreter.h contain a duplicate
include of script_error.h; remove the redundant plain include and keep only the
line that has "script/script_error.h" with the "// IWYU pragma: export" comment
so that the file retains the intended export pragma and avoids redundant
includes (update the include list involving script_error.h accordingly).
In `@src/wallet/wallet.cpp`:
- Around line 3286-3300: The key-birthday optimization (loop computing
time_first_key and calling chain.findFirstBlockWithTimeAndHeight which can
advance rescan_height) runs unconditionally and can bypass an explicit
full-rescan request (-rescan=2); wrap the entire time_first_key computation and
subsequent chain.findFirstBlockWithTimeAndHeight/rescan_height update in a guard
that skips this optimization when a full rescan was requested (e.g. check the
full-rescan flag or mode before the loop and only run the
time_first_key/FoundBlock logic when NOT doing a full rescan), leaving the rest
of rescan logic unchanged; ensure the guard references the same symbols
(time_first_key, walletInstance->GetAllScriptPubKeyMans(),
chain.findFirstBlockWithTimeAndHeight, rescan_height, tip_height) so the
behavior only changes when full rescan is requested.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/script/interpreter.hsrc/test/fuzz/bloom_filter.cppsrc/test/fuzz/coins_view.cppsrc/test/fuzz/fuzz.hsrc/test/fuzz/policy_estimator.cppsrc/test/fuzz/rpc.cppsrc/test/fuzz/script_flags.cppsrc/wallet/wallet.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/fuzz/coins_view.cpp
|
Hello @knst requesting review |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The reported blocking include issue in src/test/fuzz/policy_estimator.cpp is not verified against this source tree.
Reviewed commit: 359c9f3
![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: de72b9a4d5
ℹ️ 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".
| if (time_first_key) { | ||
| FoundBlock found = FoundBlock().height(rescan_height); | ||
| chain.findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, found); |
There was a problem hiding this comment.
The birthday-based findFirstBlockWithTimeAndHeight adjustment now runs unconditionally, so -rescan=2 no longer guarantees a scan from genesis. In this commit the previous guard for full rescans was effectively removed (the later if (gArgs.GetIntArg("-rescan", 0) != 2) block is empty), which means users requesting a full rescan can still start at wallet birthday and miss historical wallet transactions if key birth times are inaccurate or metadata is incomplete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d19fee26bf
ℹ️ 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".
| if (gArgs.GetIntArg("-rescan", 0) != 2) { | ||
| 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)); | ||
| } | ||
| } |
There was a problem hiding this comment.
-rescan=2 is set
This conditional is now empty, so the birthday-based rescan_height optimization above runs even in full-rescan mode. With -rescan=2, users expect a scan from genesis, but the current flow can still advance to a key-birthday-derived height and skip older blocks; if birth times are missing or inaccurate, historical wallet transactions can be missed.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified 1 blocking merge resolution bug: the Dash-specific -rescan=2 guard was dropped when the birthday optimization code was moved during the cherry-pick of bitcoin#26679, making the optimization unconditional and breaking forced full rescans.
Reviewed commit: d19fee2
🔴 1 blocking
🤖 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/wallet.cpp`:
- [BLOCKING] lines 3366-3419: Cherry-pick dropped Dash's `-rescan=2` guard, making birthday optimization unconditional
Verified against the checked-out source. Dash still has a distinct `-rescan=2` mode: `src/wallet/init.cpp:68-69` documents `-rescan=<mode>` with `2 = start from genesis block`, and `src/wallet/init.cpp:151-154` validates modes up to 2. In the parent of this cherry-pick (`df12fd303b8`), the birthday-based `findFirstBlockWithTimeAndHeight(..., rescan_height, ...)` logic was inside `if (gArgs.GetIntArg("-rescan", 0) != 2)`, so `-rescan=2` forced a full rescan from genesis. In the current commit, the new 'wallet newer than tip' optimization was moved above that guard and now runs unconditionally at `src/wallet/wallet.cpp:3366-3381`, including `rescan_height = *tip_height` when no qualifying block is found. The old guard remains only as an empty block at `src/wallet/wallet.cpp:3418-3419`, so `-rescan=2` no longer disables the birthday optimization and can skip rescanning entirely for wallets newer than tip. The preserved behavior should have been to keep the birthday/time-first-key optimization, including the new skip-to-tip path, inside the `-rescan != 2` condition.
| @@ -3396,21 +3414,9 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf | |||
|
|
|||
| chain.initMessage(_("Rescanning…").translated); | |||
| walletInstance->WalletLogPrintf("Rescanning last %i blocks (from block %i)...\n", *tip_height - rescan_height, rescan_height); | |||
|
|
|||
| // 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) { | |||
| 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)); | |||
| } | |||
| } | |||
There was a problem hiding this comment.
🔴 Blocking: Cherry-pick dropped Dash's -rescan=2 guard, making birthday optimization unconditional
Verified against the checked-out source. Dash still has a distinct -rescan=2 mode: src/wallet/init.cpp:68-69 documents -rescan=<mode> with 2 = start from genesis block, and src/wallet/init.cpp:151-154 validates modes up to 2. In the parent of this cherry-pick (df12fd303b8), the birthday-based findFirstBlockWithTimeAndHeight(..., rescan_height, ...) logic was inside if (gArgs.GetIntArg("-rescan", 0) != 2), so -rescan=2 forced a full rescan from genesis. In the current commit, the new 'wallet newer than tip' optimization was moved above that guard and now runs unconditionally at src/wallet/wallet.cpp:3366-3381, including rescan_height = *tip_height when no qualifying block is found. The old guard remains only as an empty block at src/wallet/wallet.cpp:3418-3419, so -rescan=2 no longer disables the birthday optimization and can skip rescanning entirely for wallets newer than tip. The preserved behavior should have been to keep the birthday/time-first-key optimization, including the new skip-to-tip path, inside the -rescan != 2 condition.
source: ['claude']
🤖 Fix this 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/wallet.cpp`:
- [BLOCKING] lines 3366-3419: Cherry-pick dropped Dash's `-rescan=2` guard, making birthday optimization unconditional
Verified against the checked-out source. Dash still has a distinct `-rescan=2` mode: `src/wallet/init.cpp:68-69` documents `-rescan=<mode>` with `2 = start from genesis block`, and `src/wallet/init.cpp:151-154` validates modes up to 2. In the parent of this cherry-pick (`df12fd303b8`), the birthday-based `findFirstBlockWithTimeAndHeight(..., rescan_height, ...)` logic was inside `if (gArgs.GetIntArg("-rescan", 0) != 2)`, so `-rescan=2` forced a full rescan from genesis. In the current commit, the new 'wallet newer than tip' optimization was moved above that guard and now runs unconditionally at `src/wallet/wallet.cpp:3366-3381`, including `rescan_height = *tip_height` when no qualifying block is found. The old guard remains only as an empty block at `src/wallet/wallet.cpp:3418-3419`, so `-rescan=2` no longer disables the birthday optimization and can skip rescanning entirely for wallets newer than tip. The preserved behavior should have been to keep the birthday/time-first-key optimization, including the new skip-to-tip path, inside the `-rescan != 2` condition.
|
✅ Review complete (commit acb589e) |
|
@vijaydasmp there seem to be valid findings by thepastaclaw; please investigate and respond to as needed |
Sure I am checking it |
fabb504 fuzz: Avoid timeout and bloat in fuzz targets (MarcoFalke) Pull request description: If the fuzz input contains invalid data *in a loop*, abort early. This will teach the fuzz engine to look for useful data and avoids bloating the fuzz input folder with useless (repeated) data. ACKs for top commit: dergoegge: utACK fabb504 brunoerg: crACK fabb504 Tree-SHA512: 26da100d7558ae6fdd5292fb146d8858b2af8f78c546ca2509b9d27b33a33e9462ecb6035de142f9f36dd5de32f8cbad099d6c7a697902d23e1bb621cd27dc88
faf1fb2 Fix IWYU for the script_flags fuzz target (MarcoFalke) fa71285 fuzz: Limit fuzz buffer size in script_flags target (MarcoFalke) fa6b87b fuzz: CDataStream -> DataStream in script_flags (MarcoFalke) Pull request description: Most fuzz targets have an upper limit on the buffer size to avoid excessive runtime. Do the same for `script_flags` to avoid timeouts such as bitcoin#28812 (comment) Also, fix iwyu. Also, remove legacy `CDataStream`. ACKs for top commit: dergoegge: ACK faf1fb2 brunoerg: utACK faf1fb2 Tree-SHA512: 9301917b353f7409e448b6fd3635de19330856e0742431db5ef04e62873501b5b4cd6cb78ad81ada2747fa2bdae033115b5951d10489dd5d0d320426c8b96bee
… than tip 3784009 wallet: Skip rescanning if wallet is more recent than tip (Andrew Chow) Pull request description: If a wallet has key birthdates that are more recent than the currrent chain tip, or a bestblock height higher than the current tip, we should not attempt to rescan as there is nothing to scan for. Fixes bitcoin#26655 ACKs for top commit: ishaanam: re-utACK 3784009 w0xlt: utACK bitcoin@3784009 furszy: Code review ACK 3784009 Tree-SHA512: f0d90b62940d97d50f21e1e01fa6dcb54409fad819cea4283612825c4d93d733df323cd92787fed43956b0a8e386a5bf88218f1f5749c913398667a5c8f54470
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Rechecked the blocking -rescan=2 regression at acb589e3.
The birthday-based findFirstBlockWithTimeAndHeight(...) optimization is now back under Dash's gArgs.GetIntArg("-rescan", 0) != 2 guard, so a forced full rescan still starts from genesis. My earlier blocking review point is resolved.
|
Hi @PastaPastaPasta resolved the review by thepastaclaw , requesting review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Three-PR backport batch (bitcoin#28815, bitcoin#28931, bitcoin#26679) is clean and behaviorally correct. Verified that the Dash-side prerequisites (FoundBlock::found, findFirstBlockWithTimeAndHeight, LIMITED_WHILE, DataStream) are all present, and the wallet skip-rescan logic in CWallet::AttachChain works as intended for -rescan=0/1/2. No blocking issues. Minor leftovers from the upstream IWYU cleanup (dead forward declarations) and one out-of-scope observation: the Qt manual-rescan path in src/wallet/interfaces.cpp::startRescan still ignores findFirstBlockWithTimeAndHeight's result and would fall back to a genesis scan when the wallet is newer than the tip — the same bug class this PR fixes for AttachChain.
Fresh dispatcher run for the current SHA. review_poster.py detected an older same-SHA PastaClaw review, so I am posting this run as a top-level review to complete the gate without duplicating inline threads.
Reviewed commit: acb589e
🟡 1 suggestion(s) | 💬 3 nitpick(s)
4 additional findings
🟡 suggestion: Qt manual-rescan path still genesis-scans when wallet is newer than tip
src/wallet/interfaces.cpp (lines 186-189)
startRescan(from_genesis=false) calls findFirstBlockWithTimeAndHeight and discards the FoundBlock temporary, so the .found flag is never inspected. When the wallet's earliest key time is later than the chain tip, the lookup returns false and rescan_height stays at 0, causing the subsequent ScanForWalletTransactions call to scan from genesis instead of becoming a no-op. This is the same defect bitcoin#26679 fixes for CWallet::AttachChain; the Qt/manual-rescan entry point (invoked from RescanWalletActivity via walletcontroller.cpp:523-530) is left with the buggy behavior and can still trigger a full rescan or fail on pruned nodes. Out of scope for a strict cherry-pick of bitcoin#26679, but worth a follow-up so the fix is consistent across both wallet entry points.
💡 Suggested change
if (time_first_key) {
FoundBlock found = FoundBlock().height(rescan_height);
m_wallet->chain().findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height,
found);
if (!found.found) {
const std::optional<int> tip_height = m_wallet->chain().getHeight();
if (tip_height) rescan_height = *tip_height;
}
}
💬 nitpick: Dead `class XOnlyPubKey;` forward declaration
src/script/interpreter.h (line 22)
The IWYU cleanup in bitcoin#28931 added a forward declaration for XOnlyPubKey, an upstream Taproot/BIP340 type. Dash has not backported Taproot and the symbol is undefined and unreferenced anywhere else in the tree. The forward declaration is harmless (no use makes it require a complete type), but it is misleading and can be dropped from the Dash port — or knowingly left in place as a marker for a future Taproot backport.
💬 nitpick: Unused `enum class ChainType;` forward declaration
src/test/fuzz/rpc.cpp (line 37)
The IWYU cleanup added enum class ChainType; because upstream TestingSetup's constructor takes a ChainType. Dash's TestingSetup still takes const std::string& chain_name (line 41), so the forward declaration is dead. Drop it to avoid confusing future readers.
💬 nitpick: Two separate reads of `-rescan` in AttachChain due to un-backported `rescan_required` refactor
src/wallet/wallet.cpp (lines 3353-3394)
Upstream consolidated the -rescan consultations behind a single rescan_required parameter before bitcoin#26679 landed. Dash hasn't taken that earlier refactor, so AttachChain now reads gArgs.GetBoolArg("-rescan", false) at line 3353 and gArgs.GetIntArg("-rescan", 0) at line 3378. Behavior is correct for -rescan=0/1/2, but the divergence is fragile if anyone later changes -rescan parsing. Track the upstream rescan_required refactor as a follow-up to consolidate both reads behind a single source of truth.
🤖 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/interfaces.cpp`:
- [SUGGESTION] lines 186-189: Qt manual-rescan path still genesis-scans when wallet is newer than tip
`startRescan(from_genesis=false)` calls `findFirstBlockWithTimeAndHeight` and discards the `FoundBlock` temporary, so the `.found` flag is never inspected. When the wallet's earliest key time is later than the chain tip, the lookup returns false and `rescan_height` stays at 0, causing the subsequent `ScanForWalletTransactions` call to scan from genesis instead of becoming a no-op. This is the same defect bitcoin#26679 fixes for `CWallet::AttachChain`; the Qt/manual-rescan entry point (invoked from `RescanWalletActivity` via `walletcontroller.cpp:523-530`) is left with the buggy behavior and can still trigger a full rescan or fail on pruned nodes. Out of scope for a strict cherry-pick of #26679, but worth a follow-up so the fix is consistent across both wallet entry points.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Three-PR backport batch (bitcoin#28815, bitcoin#28931, bitcoin#26679) is a clean cherry-pick. The wallet skip-rescan logic in CWallet::AttachChain is functionally correct for -rescan=0/1/2, and the fuzz changes are runtime/corpus improvements with no behavioral risk. No blocking issues. Findings are limited to two dead IWYU forward declarations (XOnlyPubKey, ChainType — types that don't exist in Dash), a cosmetic dual-read of -rescan stemming from an un-backported upstream refactor, and an out-of-diff observation that the Qt manual-rescan path has the same FoundBlock::found defect this PR fixes for AttachChain.
Reviewed commit: acb589e
🟡 2 suggestion(s) | 💬 2 nitpick(s)
1 additional finding
🟡 suggestion: Qt manual-rescan path has the same FoundBlock::found defect this PR fixes
src/wallet/interfaces.cpp (lines 186-189)
startRescan(from_genesis=false) passes a temporary FoundBlock().height(rescan_height) to findFirstBlockWithTimeAndHeight and never inspects .found. When the wallet's earliest key time is later than the chain tip, the lookup returns false and rescan_height stays at 0, causing a genesis rescan (or a pruned-node failure) from the GUI. This is the exact defect bitcoin#26679 fixes for CWallet::AttachChain. Out of scope for a strict cherry-pick, but worth a follow-up so both wallet entry points behave consistently — bind the FoundBlock to a named local, then fall back to tip height when !found.found.
🤖 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/interfaces.cpp`:
- [SUGGESTION] lines 186-189: Qt manual-rescan path has the same FoundBlock::found defect this PR fixes
`startRescan(from_genesis=false)` passes a temporary `FoundBlock().height(rescan_height)` to `findFirstBlockWithTimeAndHeight` and never inspects `.found`. When the wallet's earliest key time is later than the chain tip, the lookup returns false and `rescan_height` stays at 0, causing a genesis rescan (or a pruned-node failure) from the GUI. This is the exact defect bitcoin#26679 fixes for `CWallet::AttachChain`. Out of scope for a strict cherry-pick, but worth a follow-up so both wallet entry points behave consistently — bind the `FoundBlock` to a named local, then fall back to tip height when `!found.found`.
In `src/wallet/wallet.cpp`:
- [SUGGESTION] lines 3353-3378: Two separate reads of `-rescan` in AttachChain due to un-backported `rescan_required` refactor
Upstream consolidated `-rescan` consultations into a single `rescan_required` parameter prior to #26679. Dash didn't take that earlier refactor, so `AttachChain` now reads `gArgs.GetBoolArg("-rescan", false)` at line 3353 and `gArgs.GetIntArg("-rescan", 0)` at line 3378. Behavior is correct today for `-rescan=0/1/2`, but two reads of `-rescan` (one as bool, one as int) is fragile if anyone later changes how the value is parsed. Consider tracking the upstream `rescan_required` refactor as a follow-up to consolidate both call sites behind a single source of truth.
|
|
||
| if (tip_height && *tip_height != rescan_height) | ||
| { | ||
| // 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) { |
There was a problem hiding this comment.
🟡 Suggestion: Two separate reads of -rescan in AttachChain due to un-backported rescan_required refactor
Upstream consolidated -rescan consultations into a single rescan_required parameter prior to bitcoin#26679. Dash didn't take that earlier refactor, so AttachChain now reads gArgs.GetBoolArg("-rescan", false) at line 3353 and gArgs.GetIntArg("-rescan", 0) at line 3378. Behavior is correct today for -rescan=0/1/2, but two reads of -rescan (one as bool, one as int) is fragile if anyone later changes how the value is parsed. Consider tracking the upstream rescan_required refactor as a follow-up to consolidate both call sites behind a single source of truth.
source: ['claude', 'codex']
🤖 Fix this 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/wallet.cpp`:
- [SUGGESTION] lines 3353-3378: Two separate reads of `-rescan` in AttachChain due to un-backported `rescan_required` refactor
Upstream consolidated `-rescan` consultations into a single `rescan_required` parameter prior to #26679. Dash didn't take that earlier refactor, so `AttachChain` now reads `gArgs.GetBoolArg("-rescan", false)` at line 3353 and `gArgs.GetIntArg("-rescan", 0)` at line 3378. Behavior is correct today for `-rescan=0/1/2`, but two reads of `-rescan` (one as bool, one as int) is fragile if anyone later changes how the value is parsed. Consider tracking the upstream `rescan_required` refactor as a follow-up to consolidate both call sites behind a single source of truth.
| class CTxOut; | ||
| class uint256; | ||
| class CScriptNum; | ||
| class XOnlyPubKey; |
There was a problem hiding this comment.
💬 Nitpick: Dead class XOnlyPubKey; forward declaration
The IWYU cleanup in bitcoin#28931 carried over a forward declaration for XOnlyPubKey (a BIP340/Taproot type). Taproot has not been backported, so XOnlyPubKey is referenced exactly once in the entire tree — this declaration. Harmless at compile time but misleading. Drop it, or knowingly retain as a marker for a future Taproot backport.
source: ['claude', 'codex']
| #include <stdexcept> | ||
| #include <string> | ||
| #include <vector> | ||
| enum class ChainType; |
There was a problem hiding this comment.
💬 Nitpick: Dead enum class ChainType; forward declaration
The IWYU cleanup added enum class ChainType; because upstream's TestingSetup constructor takes a ChainType. Dash's RPCFuzzTestingSetup still takes const std::string& chain_name (line 41), and ChainType is referenced nowhere else in the tree. Drop the dead declaration.
source: ['claude', 'codex']
6 participants
backport