backport: Merge bitcoin/bitcoin#28815, 28931, 26679

[vijaydasmp]

backport

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[github-actions]

This pull request has conflicts, please rebase.

[github-actions]

This pull request has conflicts, please rebase.

[coderabbitai]

Walkthrough

This 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

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	The description is extremely vague, consisting of only the word 'backport' with no meaningful details about what changes are being made or why.	Expand the description to explain the purpose of these backports and what functionality they introduce or improve.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically identifies the backport as merging three bitcoin/bitcoin PRs (#28815, #28931, #26679), which is the primary purpose of the changeset.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[knst]

fixes for 26679 are required; other 2 backports are good overall, see nits

[knst]

nit: formatting is different (two missing \n) with original 28815:

[knst]

nit(28931): remove CScriptWitness because no segwit in dash core

[knst]

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]

ok checking

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

src/wallet/wallet.cpp (1)

3284-3300: ⚠️ Potential issue | 🟠 Major

Preserve full-rescan semantics for -rescan=2.

This wallet-birthday shortcut now runs unconditionally. When -rescan=2 is set, rescan_height starts 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 != 2 condition (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 unused XOnlyPubKey forward declaration.

XOnlyPubKey is 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1c0e1e6 and 4687382.

📒 Files selected for processing (8)

• src/script/interpreter.h
• src/test/fuzz/bloom_filter.cpp
• src/test/fuzz/coins_view.cpp
• src/test/fuzz/fuzz.h
• src/test/fuzz/policy_estimator.cpp
• src/test/fuzz/rpc.cpp
• src/test/fuzz/script_flags.cpp
• src/wallet/wallet.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

• src/test/fuzz/fuzz.h

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

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.

Suggested change

	#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.

[coderabbitai]

♻️ Duplicate comments (2)

src/script/interpreter.h (1)

9-12: ⚠️ Potential issue | 🟡 Minor

Remove the duplicate script_error.h include.

Line 9 duplicates Line 12. Keep only the // IWYU pragma: export include to avoid redundant includes.

🔧 Proposed fix

-#include <script/script_error.h>
 `#include` <consensus/amount.h>
 `#include` <primitives/transaction.h>
 `#include` <script/script_error.h> // IWYU pragma: export

Based on learnings, backported changes should match upstream PRs closely.

🤖 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=2 full-rescan behavior is effectively bypassed.

At Line 3291, the key-birthday optimization now runs unconditionally. That can advance rescan_height and skip historical blocks even when a full rescan is requested.

🔧 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;
             }
         }

Based on learnings, behavior changes in backport PRs should be avoided unless explicitly intended.

🤖 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

📥 Commits

Reviewing files that changed from the base of the PR and between 4687382 and 75fb4a7.

📒 Files selected for processing (8)

• src/script/interpreter.h
• src/test/fuzz/bloom_filter.cpp
• src/test/fuzz/coins_view.cpp
• src/test/fuzz/fuzz.h
• src/test/fuzz/policy_estimator.cpp
• src/test/fuzz/rpc.cpp
• src/test/fuzz/script_flags.cpp
• src/wallet/wallet.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

• src/test/fuzz/coins_view.cpp