backport: Merge bitcoin#29404, 28144, 28118

[vijaydasmp]

bitcoin backporting
🤕➡️🛌🩹➡️💪⚡💻🔙

[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]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 72f7a326-5b9b-4f60-b6c7-668207e5d378

📥 Commits

Reviewing files that changed from the base of the PR and between 19b26e4 and 318d851.

📒 Files selected for processing (56)

• src/addrdb.cpp
• src/addrman.cpp
• src/bench/verify_script.cpp
• src/bench/wallet_create.cpp
• src/bench/wallet_loading.cpp
• src/clientversion.cpp
• src/compat/compat.h
• src/crypto/chacha20poly1305.cpp
• src/crypto/muhash.h
• src/crypto/sha256.cpp
• src/netaddress.h
• src/netbase.h
• src/qt/addressbookpage.cpp
• src/qt/askpassphrasedialog.cpp
• src/qt/bitcoingui.cpp
• src/qt/coincontroldialog.cpp
• src/qt/guiutil.cpp
• src/qt/modaloverlay.cpp
• src/qt/notificator.cpp
• src/qt/paymentserver.cpp
• src/qt/paymentserver.h
• src/qt/sendcoinsentry.cpp
• src/qt/sendcoinsrecipient.h
• src/qt/test/apptests.cpp
• src/qt/test/optiontests.cpp
• src/qt/transactiondesc.cpp
• src/qt/walletmodel.cpp
• src/qt/walletmodel.h
• src/qt/walletmodeltransaction.cpp
• src/random.cpp
• src/rest.cpp
• src/rpc/external_signer.cpp
• src/rpc/mining.cpp
• src/rpc/node.cpp
• src/rpc/register.h
• src/rpc/server.cpp
• src/rpc/util.cpp
• src/serialize.h
• src/support/lockedpool.cpp
• src/sync.cpp
• src/test/script_tests.cpp
• src/test/system_tests.cpp
• src/test/util/setup_common.cpp
• src/validation.cpp
• src/validation.h
• src/wallet/init.cpp
• src/wallet/rpc/addresses.cpp
• src/wallet/rpc/backup.cpp
• src/wallet/rpc/wallet.cpp
• src/wallet/sqlite.cpp
• src/wallet/test/db_tests.cpp
• src/wallet/test/util.h
• src/wallet/walletdb.cpp
• src/warnings.cpp
• test/functional/p2p_getaddr_caching.py
• test/functional/test_framework/test_node.py

💤 Files with no reviewable changes (19)

• src/netbase.h
• src/netaddress.h
• src/qt/coincontroldialog.cpp
• src/sync.cpp
• src/qt/addressbookpage.cpp
• src/crypto/muhash.h
• src/qt/transactiondesc.cpp
• src/qt/sendcoinsentry.cpp
• src/qt/sendcoinsrecipient.h
• src/qt/walletmodeltransaction.cpp
• src/qt/askpassphrasedialog.cpp
• src/validation.h
• src/qt/paymentserver.h
• src/qt/test/apptests.cpp
• src/qt/walletmodel.h
• src/qt/walletmodel.cpp
• src/compat/compat.h
• src/qt/paymentserver.cpp
• src/support/lockedpool.cpp

✅ Files skipped from review due to trivial changes (32)

• src/rpc/external_signer.cpp
• src/bench/verify_script.cpp
• src/qt/modaloverlay.cpp
• src/test/script_tests.cpp
• src/bench/wallet_loading.cpp
• src/qt/guiutil.cpp
• src/wallet/sqlite.cpp
• src/wallet/rpc/backup.cpp
• src/warnings.cpp
• src/test/system_tests.cpp
• src/rpc/mining.cpp
• src/bench/wallet_create.cpp
• src/rpc/server.cpp
• src/rest.cpp
• src/qt/notificator.cpp
• src/rpc/util.cpp
• src/wallet/test/util.h
• src/clientversion.cpp
• src/qt/test/optiontests.cpp
• src/random.cpp
• src/test/util/setup_common.cpp
• src/wallet/init.cpp
• src/qt/bitcoingui.cpp
• src/wallet/rpc/wallet.cpp
• src/addrman.cpp
• src/wallet/test/db_tests.cpp
• src/wallet/rpc/addresses.cpp
• src/addrdb.cpp
• src/rpc/register.h
• src/crypto/sha256.cpp
• src/serialize.h
• src/rpc/node.cpp

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

• test/functional/test_framework/test_node.py
• src/crypto/chacha20poly1305.cpp
• test/functional/p2p_getaddr_caching.py

---

Walkthrough

The PR performs a systematic refactoring of build configuration include placement by moving #include <config/bitcoin-config.h> directives from shared headers (compat.h, muhash.h, netaddress.h, netbase.h, validation.h, and several utility/Qt files) into individual compilation units where configuration access is needed. This consolidates configuration-header dependencies to translation-unit scope.

Concurrently, RPC handlers in src/rpc/node.cpp are refactored to obtain a required NodeContext via EnsureAnyNodeContext(request.context) instead of optional GetContext, eliminating null-checks. The setmocktime handler now unconditionally iterates over chain_clients to call setMockTime(time), while mockscheduler uses a checked scheduler reference and invokes SyncWithValidationInterfaceQueue() immediately after MockForward().

RPC documentation is improved by adding HelpExampleRpc entries to the debug RPC and multiple address/index handlers. P2P address-caching test logic is simplified by removing explicit msg_getaddr transmission and instead relying on mock time advancement with observer checks. Test framework peer matching is tightened to require both advertised and bind addresses for uniqueness.

Sequence Diagram(s)

sequenceDiagram
  participant RPCClient
  participant setmocktime as RPC Handler<br/>(setmocktime)
  participant EnsureContext as EnsureAnyNodeContext
  participant ChainClients as chain_clients
  RPCClient->>setmocktime: invoke setmocktime(time)
  setmocktime->>EnsureContext: EnsureAnyNodeContext<br/>(request.context)
  EnsureContext-->>setmocktime: const NodeContext&
  setmocktime->>ChainClients: iterate and call<br/>setMockTime(time)
  ChainClients-->>setmocktime: complete
  setmocktime-->>RPCClient: return result

Loading

sequenceDiagram
  participant RPCClient
  participant mockscheduler as RPC Handler<br/>(mockscheduler)
  participant EnsureContext as EnsureAnyNodeContext
  participant Scheduler as node_context<br/>.scheduler
  participant InterfaceQueue as SyncWithValidationInterfaceQueue
  RPCClient->>mockscheduler: invoke mockscheduler(delta_ms)
  mockscheduler->>EnsureContext: EnsureAnyNodeContext<br/>(request.context)
  EnsureContext-->>mockscheduler: const NodeContext&
  mockscheduler->>Scheduler: CHECK_NONFATAL(...)<br/>->MockForward(delta_ms)
  Scheduler-->>mockscheduler: complete
  mockscheduler->>InterfaceQueue: SyncWithValidationInterfaceQueue()
  InterfaceQueue-->>mockscheduler: queue synced
  mockscheduler-->>RPCClient: return result

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

• thepastaclaw
• UdjinM6
• knst

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.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 uses only emoji and vague phrasing ('bitcoin backporting') without conveying meaningful information about what changes are included or their purpose.	Replace emoji-only description with meaningful details about the specific changes being backported and their impact on the codebase.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'backport: Merge bitcoin#29404, 28144, 28118' clearly summarizes the main change: merging three upstream Bitcoin pull requests into the Dash codebase.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/rpc/node.cpp (1)

7-11: Include order: config/bitcoin-config.h should be included before other headers.

The config header defines macros (e.g., HAVE_MALLOC_INFO used on line 43) that may affect how subsequent headers behave. Placing the config include after <addressindex.h> violates the standard pattern where config headers come first.

Suggested fix

+#if defined(HAVE_CONFIG_H)
+#include <config/bitcoin-config.h>
+#endif
+
 `#include` <addressindex.h>
-#if defined(HAVE_CONFIG_H)
-#include <config/bitcoin-config.h>
-#endif
-
 `#include` <chainparams.h>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/rpc/node.cpp` around lines 7 - 11, Move the config include so it appears
before any other headers: place `#if` defined(HAVE_CONFIG_H) / `#include`
<config/bitcoin-config.h> at the top of src/rpc/node.cpp (above `#include`
<addressindex.h>) so macros like HAVE_MALLOC_INFO are defined before other
headers are processed; update the include block around the existing preprocessor
directives accordingly to ensure config is included first.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/functional/test_framework/test_node.py`:
- Line 723: The peer matching assumes addrbind is always present and can raise
KeyError; update the filter in test_node.py where getpeerinfo() is iterated (the
list comprehension that builds info) to safely handle missing addrbind by using
peer.get("addrbind") (or "in" check) when comparing to dst_addr_and_port, so
replace peer["addrbind"] == dst_addr_and_port with a safe check such as
peer.get("addrbind") == dst_addr_and_port to avoid KeyError for peers without
addrbind while still matching addr and addrbind correctly for our_addr_and_port
and dst_addr_and_port.

---

Nitpick comments:
In `@src/rpc/node.cpp`:
- Around line 7-11: Move the config include so it appears before any other
headers: place `#if` defined(HAVE_CONFIG_H) / `#include` <config/bitcoin-config.h>
at the top of src/rpc/node.cpp (above `#include` <addressindex.h>) so macros like
HAVE_MALLOC_INFO are defined before other headers are processed; update the
include block around the existing preprocessor directives accordingly to ensure
config is included first.

---

ℹ️ Review info

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2cd9132 and 10a457b.

📒 Files selected for processing (56)

• src/addrdb.cpp
• src/addrman.cpp
• src/bench/verify_script.cpp
• src/bench/wallet_create.cpp
• src/bench/wallet_loading.cpp
• src/clientversion.cpp
• src/compat/compat.h
• src/crypto/chacha20poly1305.cpp
• src/crypto/muhash.h
• src/crypto/sha256.cpp
• src/netaddress.h
• src/netbase.h
• src/qt/addressbookpage.cpp
• src/qt/askpassphrasedialog.cpp
• src/qt/bitcoingui.cpp
• src/qt/coincontroldialog.cpp
• src/qt/guiutil.cpp
• src/qt/modaloverlay.cpp
• src/qt/notificator.cpp
• src/qt/paymentserver.cpp
• src/qt/paymentserver.h
• src/qt/sendcoinsentry.cpp
• src/qt/sendcoinsrecipient.h
• src/qt/test/apptests.cpp
• src/qt/test/optiontests.cpp
• src/qt/transactiondesc.cpp
• src/qt/walletmodel.cpp
• src/qt/walletmodel.h
• src/qt/walletmodeltransaction.cpp
• src/random.cpp
• src/rest.cpp
• src/rpc/external_signer.cpp
• src/rpc/mining.cpp
• src/rpc/node.cpp
• src/rpc/register.h
• src/rpc/server.cpp
• src/rpc/util.cpp
• src/serialize.h
• src/support/lockedpool.cpp
• src/sync.cpp
• src/test/script_tests.cpp
• src/test/system_tests.cpp
• src/test/util/setup_common.cpp
• src/validation.cpp
• src/validation.h
• src/wallet/init.cpp
• src/wallet/rpc/addresses.cpp
• src/wallet/rpc/backup.cpp
• src/wallet/rpc/wallet.cpp
• src/wallet/sqlite.cpp
• src/wallet/test/db_tests.cpp
• src/wallet/test/util.h
• src/wallet/walletdb.cpp
• src/warnings.cpp
• test/functional/p2p_getaddr_caching.py
• test/functional/test_framework/test_node.py

💤 Files with no reviewable changes (19)

• src/netaddress.h
• src/qt/sendcoinsentry.cpp
• src/validation.h
• src/support/lockedpool.cpp
• src/qt/sendcoinsrecipient.h
• src/qt/addressbookpage.cpp
• src/qt/transactiondesc.cpp
• src/qt/paymentserver.cpp
• src/crypto/muhash.h
• src/netbase.h
• src/qt/walletmodel.h
• src/qt/askpassphrasedialog.cpp
• src/qt/test/apptests.cpp
• src/qt/walletmodeltransaction.cpp
• src/sync.cpp
• src/qt/walletmodel.cpp
• src/qt/paymentserver.h
• src/qt/coincontroldialog.cpp
• src/compat/compat.h

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

• src/wallet/test/util.h
• src/random.cpp
• src/serialize.h
• src/wallet/rpc/addresses.cpp
• src/bench/wallet_loading.cpp
• src/wallet/rpc/wallet.cpp
• src/rpc/mining.cpp
• src/wallet/sqlite.cpp
• src/bench/verify_script.cpp
• src/wallet/init.cpp
• src/qt/modaloverlay.cpp
• src/rest.cpp
• src/test/system_tests.cpp
• src/test/script_tests.cpp
• src/qt/guiutil.cpp
• src/qt/notificator.cpp
• src/addrdb.cpp
• src/crypto/sha256.cpp
• src/wallet/rpc/backup.cpp
• src/rpc/server.cpp
• src/rpc/external_signer.cpp
• test/functional/p2p_getaddr_caching.py
• src/crypto/chacha20poly1305.cpp

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Handle optional addrbind safely in peer matching.

Line 723 assumes peer["addrbind"] always exists, but getpeerinfo documents addrbind as optional (src/rpc/net.cpp Line 108-115, 207-211). This can throw KeyError and reintroduce intermittent test failures.

Proposed fix

-            info = [peer for peer in self.getpeerinfo() if peer["addr"] == our_addr_and_port and peer["addrbind"] == dst_addr_and_port]
+            peers = [peer for peer in self.getpeerinfo() if peer["addr"] == our_addr_and_port]
+            info = [peer for peer in peers if peer.get("addrbind") == dst_addr_and_port]
+            if not info:
+                # Fallback for peers where addrbind is not reported by getpeerinfo.
+                info = peers

📝 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

	info = [peer for peer in self.getpeerinfo() if peer["addr"] == our_addr_and_port and peer["addrbind"] == dst_addr_and_port]
	peers = [peer for peer in self.getpeerinfo() if peer["addr"] == our_addr_and_port]
	info = [peer for peer in peers if peer.get("addrbind") == dst_addr_and_port]
	if not info:
	# Fallback for peers where addrbind is not reported by getpeerinfo.
	info = peers

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/functional/test_framework/test_node.py` at line 723, The peer matching
assumes addrbind is always present and can raise KeyError; update the filter in
test_node.py where getpeerinfo() is iterated (the list comprehension that builds
info) to safely handle missing addrbind by using peer.get("addrbind") (or "in"
check) when comparing to dst_addr_and_port, so replace peer["addrbind"] ==
dst_addr_and_port with a safe check such as peer.get("addrbind") ==
dst_addr_and_port to avoid KeyError for peers without addrbind while still
matching addr and addrbind correctly for our_addr_and_port and
dst_addr_and_port.

[thepastaclaw]

Code Review

Clean backport of three Bitcoin Core PRs: bitcoin-config.h includes cleanup (bitcoin#29404), p2p_getaddr_caching intermittent fix (bitcoin#28144), and SyncWithValidationInterfaceQueue in mockscheduler (bitcoin#28118). All upstream changes are correctly applied with appropriate Dash adaptations. The scripted-diff in bitcoin#29404 placed config includes mid-includes (after Dash-specific headers) in several files due to AWK boundary detection — cosmetic only, all files compile correctly.

Reviewed commit: ae9e403

🟡 1 suggestion(s)

1 additional finding

🟡 suggestion: Missing direct config include for src/util/system.cpp

src/util/system.cpp (lines 1-7)

Upstream bitcoin#29404 adds a direct #include <config/bitcoin-config.h> to src/common/system.cpp (renamed from src/util/system.cpp in bitcoin#28076). Dash still has the file at its original path and the backport does not add the config include. The file uses config symbols (HAVE_MALLOPT_ARENA_MAX, HAVE_FDATASYNC, etc.) but receives them transitively via util/system.h. Not blocking — matches upstream intent to eventually remove transitive config includes from headers.

🤖 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/util/system.cpp`:
- [SUGGESTION] lines 1-7: Missing direct config include for src/util/system.cpp
  Upstream `bitcoin#29404` adds a direct `#include <config/bitcoin-config.h>` to `src/common/system.cpp` (renamed from `src/util/system.cpp` in `bitcoin#28076`). Dash still has the file at its original path and the backport does not add the config include. The file uses config symbols (`HAVE_MALLOPT_ARENA_MAX`, `HAVE_FDATASYNC`, etc.) but receives them transitively via `util/system.h`. Not blocking — matches upstream intent to eventually remove transitive config includes from headers.

[thepastaclaw]

✅ Review complete (commit 318d851)

[vijaydasmp]

Hello @UdjinM6, @knst requesting review

[thepastaclaw]

Code Review

Cumulative review at ae9e403 of three Bitcoin Core backports (bitcoin#29404 config include cleanup, bitcoin#28144 p2p_getaddr_caching deflake, bitcoin#28118 mockscheduler SyncWithValidationInterfaceQueue). Merge resolutions are correct and all upstream adaptations check out. The only carry-forward note from prior reviews is a non-blocking suggestion about bitcoin#29404 not reaching Dash's src/util/system.cpp because the upstream rename in bitcoin#28076 was never backported.

Reviewed commit: ae9e403

Fresh dispatcher run for this queue item. A same-SHA review already existed, so this records the fresh verification without duplicating inline threads.

Code Review

Reviewed commit: ae9e403

🟡 1 suggestion(s)

🤖 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/util/system.cpp`:
- [SUGGESTION] lines 1-7: Missing direct config include for src/util/system.cpp (bitcoin#29404 prereq)
  Upstream bitcoin#29404 adds a direct `#include <config/bitcoin-config.h>` to `src/common/system.cpp`, the file that upstream bitcoin#28076 renamed from `src/util/system.cpp`. Because Dash has not backported #28076, the file still lives at `src/util/system.cpp` and the scripted-diff did not target it. Verified: line 7 begins with `#include <util/system.h>` and no direct config include is present, while the file consumes config macros later (`HAVE_MALLOPT_ARENA_MAX` at lines 60/1418, `HAVE_FDATASYNC` at line 1233). Non-blocking: the macros are still supplied transitively via `util/system.h`, so behavior is unchanged. The full alignment requires backporting bitcoin#28076 first; a partial fix is to add the direct include manually here to match #29404's intent of making config dependencies explicit.

[github-actions]

This pull request has conflicts, please rebase.

[thepastaclaw]

@vijaydasmp could you please rebase this PR, or close it if it’s no longer needed? Thanks!

[thepastaclaw]

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): Cumulative review at 19b26e4 of bitcoin#29404+bitcoin#28144+bitcoin#28118 backport. Prior finding about src/util/system.cpp lacking a direct config include is STILL VALID — both reviewers reconfirmed, and it remains a soft prerequisite gap because Dash hasn't backported bitcoin#28076's rename to common/system.cpp. One new latest-delta finding: src/qt/addressbookpage.cpp had its direct config include removed by the scripted-diff but the Dash-side file still uses USE_QRCODE in four places, so the bitcoin#29404 invariant is broken for that translation unit on the Dash divergence. One stylistic nitpick on src/rpc/external_signer.cpp where the config include sits below another include. No blockers; mockscheduler/setmocktime EnsureAnyNodeContext adaptation and p2p_getaddr_caching test fix are correct.

🔴 2 blocking | 🟡 2 suggestion(s) | 💬 1 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/util/system.cpp`:
- [SUGGESTION] src/util/system.cpp:5-9: Carried-forward (STILL VALID): missing direct bitcoin-config.h include in src/util/system.cpp
  Verified at 19b26e41: src/util/system.cpp still goes straight from the license header to `#include <util/system.h>` with no direct `#include <config/bitcoin-config.h>`, even though the file uses HAVE_MALLOPT_ARENA_MAX (lines 60, 1418), HAVE_FDATASYNC (line 1233), HAVE_POSIX_FALLOCATE (line 1319), and HAVE_SYSTEM (line 1364). Upstream bitcoin#29404's scripted-diff added the direct include to the same translation unit, but upstream's file path is src/common/system.cpp after bitcoin#28076's rename, which Dash hasn't backported — so the scripted-diff invocation in this PR missed Dash's still-unrenamed src/util/system.cpp.

  This builds today only because src/util/system.h:14-16 still pulls in `<config/bitcoin-config.h>` transitively. That transitive guarantee is exactly what #29404 is preparing to remove; once a future backport drops the include from headers, this file will silently lose its HAVE_* definitions and fall through to the disabled #else branches (no fdatasync, no posix_fallocate, no mallopt tuning). To honor the upstream invariant for the Dash file layout, add the standard guarded include here.
- [BLOCKING] src/util/system.cpp:1-25: Carried-forward: src/util/system.cpp missing direct config include (bitcoin#29404 soft prereq via unbackported bitcoin#28076)
  STATUS: STILL VALID (prior finding from ae9e403d revalidated against current head 19b26e41).

  Upstream bitcoin#29404's scripted-diff added a direct `#include <config/bitcoin-config.h>` to every .cpp/.h that references a HAVE_* token from bitcoin-config.h.in. The equivalent file in upstream is `src/common/system.cpp` — renamed from `src/util/system.cpp` by bitcoin#28076, which Dash has not backported. Because Dash still has the file at the old path, the scripted-diff invocation in this PR did not reach it.

  Evidence at current head:
    - src/util/system.cpp top (lines 1-25) shows no `#include <config/bitcoin-config.h>` directive (only `#include <util/system.h>` and unrelated headers).
    - The file uses config tokens: `HAVE_MALLOPT_ARENA_MAX` at line 60 and line 1418.
    - The config header is currently pulled in transitively via `src/util/system.h:15` which includes `<config/bitcoin-config.h>`.

  Impact (soft, non-blocking per the agent (restored to blocking by policy)): build is currently correct because of the transitive include path through util/system.h, so this is not a compile or runtime issue today. However, bitcoin#29404's explicit purpose was to make every consumer of HAVE_* tokens independent of header-include order so that future cleanups (removing the include from headers) cannot silently break .cpp files. Leaving src/util/system.cpp without a direct include defeats that invariant for this file specifically and will become an active bug if/when a later backport removes the include from util/system.h.

  Upstream PRs in the dependency chain:
    - bitcoin#28076 (renamed util/system.{h,cpp} -> common/system.{h,cpp}) — not yet backported in Dash. Without it, the scripted-diff in #29404 misses the file at its current Dash path.

  Recommended adaptation (not applied in this read-only review): add the standard guarded include at the top of src/util/system.cpp matching the pattern used elsewhere in this PR:
      #if defined(HAVE_CONFIG_H)
      #include <config/bitcoin-config.h>
      #endif

  Flagging as `suggestion` per the soft-prereq rubric: cherry-pick functions correctly today, but the upstream prerequisite (#28076) is absent and the adaptation needed to keep #29404's invariant on the Dash path was not made.

  ---
  **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/util/system.cpp:7: Missing adaptation: bitcoin#29404 direct config include
  Carried-forward prior finding: STILL VALID at 19b26e41. Upstream bitcoin#29404 merge 45b2a91897 adds `#if defined(HAVE_CONFIG_H)` / `#include <config/bitcoin-config.h>` to `src/common/system.cpp` before including `common/system.h`. That upstream path is the result of bitcoin#28076's rename of `src/util/system.cpp` to `src/common/system.cpp` (commit 7d3b35004b inside merge 6342348072). Dash has not backported that rename, so the adapted target is still `src/util/system.cpp`. Current head still goes directly from the license header to `#include <util/system.h>` with no direct config include, while the file uses config-derived macros such as `HAVE_MALLOPT_ARENA_MAX`, `HAVE_FDATASYNC`, `HAVE_POSIX_FALLOCATE`, and `HAVE_SYSTEM`. This misses the upstream #29404 prerequisite/adaptation; the fix is to add the same guarded config include at the top of Dash's `src/util/system.cpp`.

  ---
  **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/qt/addressbookpage.cpp`:
- [SUGGESTION] src/qt/addressbookpage.cpp:1-7: Restore direct bitcoin-config.h include — Dash addressbookpage.cpp uses USE_QRCODE (upstream does not)
  This PR's scripted-diff removed the direct `#include <config/bitcoin-config.h>` from src/qt/addressbookpage.cpp to mirror upstream bitcoin#29404. The removal is correct for upstream because Bitcoin Core's addressbookpage.cpp does not reference any bitcoin-config.h token. Dash's copy of this file diverges: it uses `USE_QRCODE` at lines 71, 107, 258, and 266, and `USE_QRCODE` is defined via `AC_DEFINE([USE_QRCODE], [1], …)` in configure.ac:1899, so it lives in bitcoin-config.h.

  After this PR, the file compiles only because of unrelated transitive includes through other qt/ headers. That contradicts the explicit invariant #29404 establishes — every translation unit using a bitcoin-config.h token must include the header directly — and will silently disable the QR address-book actions in QR-enabled Qt builds if a later header cleanup ever removes the transitive path. Add the direct guarded include back at the top of the file (the same pattern used in src/qt/bitcoingui.cpp, src/qt/guiutil.cpp, etc., in this same backport).

[thepastaclaw]

🟡 Suggestion: Restore direct bitcoin-config.h include — Dash addressbookpage.cpp uses USE_QRCODE (upstream does not)

This PR's scripted-diff removed the direct #include <config/bitcoin-config.h> from src/qt/addressbookpage.cpp to mirror upstream bitcoin#29404. The removal is correct for upstream because Bitcoin Core's addressbookpage.cpp does not reference any bitcoin-config.h token. Dash's copy of this file diverges: it uses USE_QRCODE at lines 71, 107, 258, and 266, and USE_QRCODE is defined via AC_DEFINE([USE_QRCODE], [1], …) in configure.ac:1899, so it lives in bitcoin-config.h.

After this PR, the file compiles only because of unrelated transitive includes through other qt/ headers. That contradicts the explicit invariant bitcoin#29404 establishes — every translation unit using a bitcoin-config.h token must include the header directly — and will silently disable the QR address-book actions in QR-enabled Qt builds if a later header cleanup ever removes the transitive path. Add the direct guarded include back at the top of the file (the same pattern used in src/qt/bitcoingui.cpp, src/qt/guiutil.cpp, etc., in this same backport).

Suggested change

	// Distributed under the MIT software license, see the accompanying
	// file COPYING or http://www.opensource.org/licenses/mit-license.php.
	#if defined(HAVE_CONFIG_H)
	#include <config/bitcoin-config.h>
	#endif
	#include <qt/addressbookpage.h>
	#include <qt/forms/ui_addressbookpage.h>
	// Copyright (c) 2011-2021 The Bitcoin Core developers
	// Copyright (c) 2014-2025 The Dash Core developers
	// Distributed under the MIT software license, see the accompanying
	// file COPYING or http://www.opensource.org/licenses/mit-license.php.
	#if defined(HAVE_CONFIG_H)
	#include <config/bitcoin-config.h>
	#endif
	#include <qt/addressbookpage.h>
	#include <qt/forms/ui_addressbookpage.h>

_{source: ['codex']}

[thepastaclaw]

💬 Nitpick: bitcoin-config.h include placed after another header (and stray blank line)

The adapted hunk places <config/bitcoin-config.h> after #include <chainparamsbase.h> and leaves a double blank line before the next includes. Upstream bitcoin#29404's convention — and every other file touched in this same backport (rest.cpp, random.cpp, rpc/mining.cpp, rpc/server.cpp, etc.) — puts the guarded config-include block first, immediately after the license header, so any token it defines (HAVE_*, ENABLE_EXTERNAL_SIGNER, etc.) is visible to every subsequent header. The same pattern recurs in src/rpc/util.cpp:5-9, src/wallet/init.cpp:6-10, and src/wallet/rpc/wallet.cpp:7-10. Not a functional bug today, but it diverges from the scripted-diff's intent and makes the cleanup easier to break later.

Suggested change

	#include <chainparamsbase.h>
	#if defined(HAVE_CONFIG_H)
	#include <config/bitcoin-config.h>
	#endif
	// Copyright (c) 2018-2021 The Bitcoin Core developers
	// Distributed under the MIT software license, see the accompanying
	// file COPYING or http://www.opensource.org/licenses/mit-license.php.
	#if defined(HAVE_CONFIG_H)
	#include <config/bitcoin-config.h>
	#endif
	#include <chainparamsbase.h>
	#include <external_signer.h>
	#include <rpc/server.h>
	#include <util/strencodings.h>
	#include <rpc/protocol.h>

_{source: ['claude']}

[thepastaclaw]

Resolved in this update — bitcoin-config.h include placed after another header (and stray blank line) 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]

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): Cumulative review at 318d851. The latest delta resolves the prior src/rpc/external_signer.cpp include-ordering nitpick (config include now precedes chainparamsbase.h). Two carried-forward findings remain STILL VALID: src/util/system.cpp lacks a direct #include <config/bitcoin-config.h> despite consuming HAVE_MALLOPT_ARENA_MAX/HAVE_FDATASYNC/HAVE_POSIX_FALLOCATE/HAVE_SYSTEM (soft prereq via unbackported bitcoin#28076 rename), and src/qt/addressbookpage.cpp lacks the same direct include despite using USE_QRCODE at four sites. Per backport-prereq-restore policy the missing-prereq finding remains blocking; the Dash-only USE_QRCODE divergence is treated as a suggestion.

🔴 5 blocking

5 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/util/system.cpp`:
- [BLOCKING] src/util/system.cpp:1-7: Carried-forward (STILL VALID): bitcoin#29404 adaptation missing — src/util/system.cpp lacks direct config include
  Verified at HEAD 318d851f: src/util/system.cpp goes straight from the license header (line 5) to `#include <util/system.h>` (line 7) with no direct `#include <config/bitcoin-config.h>`. The file consumes HAVE_MALLOPT_ARENA_MAX (lines 60, 1418), HAVE_FDATASYNC (line 1233), HAVE_POSIX_FALLOCATE (line 1319), and HAVE_SYSTEM (line 1364) — all defined in bitcoin-config.h.

  Upstream bitcoin#29404's scripted-diff added the guarded direct include to every translation unit that uses a HAVE_* token. Upstream's path is `src/common/system.cpp`, renamed from `src/util/system.cpp` by bitcoin#28076 which Dash has not backported, so the scripted-diff invocation missed Dash's still-unrenamed path. The file builds today only via transitive inclusion through util/system.h — exactly the brittle dependency that #29404 is intended to eliminate. If a future header cleanup removes the transitive path, the HAVE_* tokens silently fall through to disabled #else branches (no fdatasync, no posix_fallocate, no mallopt tuning) with no compile error.

  This is a missing prerequisite/adaptation for the full bitcoin#29404 backport. Either backport bitcoin#28076's util→common rename, or add the guarded direct include at the top of src/util/system.cpp as a Dash-side adaptation. (Consolidates prior findings #1, #2, #3.)
- [BLOCKING] src/util/system.cpp:1-25: Carried-forward (STILL VALID): src/util/system.cpp missing direct config include (bitcoin#29404 soft prereq via unbackported bitcoin#28076)
  STATUS at HEAD 318d851f: STILL VALID. Re-read of src/util/system.cpp:1-30 confirms the file still goes from the license header straight to `#include <util/system.h>` (line 7) with no direct `#include <config/bitcoin-config.h>`. The file uses config tokens HAVE_MALLOPT_ARENA_MAX (lines 60, 1418), HAVE_FDATASYNC (line 1233), HAVE_POSIX_FALLOCATE (line 1319), and HAVE_SYSTEM (line 1364).

  Upstream bitcoin#29404's scripted-diff added the guarded direct include to every .cpp/.h consuming HAVE_* tokens. Upstream's path is src/common/system.cpp (renamed by bitcoin#28076); Dash hasn't backported bitcoin#28076, so the scripted-diff invocation in this PR missed Dash's still-unrenamed src/util/system.cpp. Today the build is correct only through the transitive include via src/util/system.h, which is exactly the brittle pattern #29404 was meant to eliminate.

  Missing prerequisite upstream PR: bitcoin#28076 (util/system.{h,cpp} -> common/system.{h,cpp} rename). Without it, the #29404 invariant is not honored on the Dash file layout for this translation unit.

  Fix: add the standard guarded include immediately after the license header, matching the pattern used elsewhere in this PR.

  ---
  **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/util/system.cpp:7: Carried-forward (STILL VALID): missing adaptation of bitcoin#29404 direct config include
  STATUS at HEAD 318d851f: STILL VALID. Upstream bitcoin#29404 merge 45b2a91897 added `#if defined(HAVE_CONFIG_H)` / `#include <config/bitcoin-config.h>` to src/common/system.cpp before including common/system.h. That upstream path is the product of bitcoin#28076's rename of src/util/system.cpp to src/common/system.cpp (commit 7d3b35004b inside merge 6342348072). Dash has not backported bitcoin#28076, so the adapted target is still src/util/system.cpp. Current head still goes directly from the license header to `#include <util/system.h>` (line 7) with no direct config include, while the file uses HAVE_MALLOPT_ARENA_MAX, HAVE_FDATASYNC, HAVE_POSIX_FALLOCATE, and HAVE_SYSTEM. The #29404 adaptation for the Dash path is still missing.

  Fix: add the same guarded config include block at the top of Dash's src/util/system.cpp.

  ---
  **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/util/system.cpp:7: Missing prerequisite: bitcoin#28076 leaves bitcoin#29404 hunk unapplied
  Carried-forward prior findings 1, 2, and 3 are STILL VALID at 318d851f. Upstream bitcoin#29404 merge 45b2a91897 adds the guarded `#include <config/bitcoin-config.h>` to `src/common/system.cpp` before including `common/system.h`. Dash still has this translation unit at `src/util/system.cpp`; `git ls-tree` at current head shows `src/util/system.cpp` and no `src/common/system.cpp`. The upstream path mismatch comes from the unbackported upstream move `7d3b35004b refactor: Move system from util to common library` in the dependency chain before #29404. Current Dash `src/util/system.cpp` still goes directly from the license header to `#include <util/system.h>` at line 7, while the same file uses config-derived macros `HAVE_MALLOPT_ARENA_MAX`, `HAVE_FDATASYNC`, `HAVE_POSIX_FALLOCATE`, and `HAVE_SYSTEM`. It currently builds through a transitive include from `util/system.h`, but that is exactly the dependency #29404 is eliminating. Add the guarded config include directly to Dash's still-unrenamed file.

  ---
  **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/qt/addressbookpage.cpp`:
- [BLOCKING] src/qt/addressbookpage.cpp:6-14: Missing adaptation: bitcoin#29404 removed a config include Dash still needs
  Carried-forward prior finding 4 is STILL VALID at 318d851f. Upstream bitcoin#29404 removes `#include <config/bitcoin-config.h>` from `src/qt/addressbookpage.cpp` because upstream's file no longer references config tokens. The Dash backport mirrored that removal, but Dash's `src/qt/addressbookpage.cpp` still uses `USE_QRCODE` at lines 71, 107, 258, and 266; Dash defines `USE_QRCODE` through `configure.ac`, so it is provided by `bitcoin-config.h`. The current file starts with `#include <qt/addressbookpage.h>` at line 6 and has no direct config include. This is a Dash-side adaptation gap in the #29404 invariant: the file now relies on unrelated transitive includes for a config macro.

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

[thepastaclaw]

Correction to my automated review at 318d851: the auto-resolve reply on the src/qt/addressbookpage.cpp thread was posted in error and I deleted it.

Reconciliation status:

• src/rpc/external_signer.cpp include-ordering nit: FIXED in this delta.
• src/qt/addressbookpage.cpp direct config include / USE_QRCODE: STILL VALID, but this is a suggestion-level Dash adaptation issue, not a resolved finding.
• src/util/system.cpp direct config include / util: Replace std::filesystem with util/fs.h bitcoin/bitcoin#28076 prerequisite gap: STILL VALID and remains the blocking issue group.

So the review should be read as: changes requested due to the src/util/system.cpp backport-prereq/adaptation gap, with the addressbook config include carried forward as a suggestion.