backport: Merge bitcoin/bitcoin#25977, 27357, 28583

[vijaydasmp]

What was done?

Backports from Bitcoin Core v26,

How Has This Been Tested?

Run unit & functional tests

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[coderabbitai]

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: 039c3b55-f5a4-448f-b9d8-6ed2cac26b28

📥 Commits

Reviewing files that changed from the base of the PR and between dc1f423 and 496defa.

📒 Files selected for processing (36)

• src/.bear-tidy-config
• src/.clang-tidy
• src/addrdb.cpp
• src/addrdb.h
• src/bench/wallet_loading.cpp
• src/external_signer.cpp
• src/httpserver.cpp
• src/init.cpp
• src/net_permissions.cpp
• src/net_processing.cpp
• src/node/blockstorage.cpp
• src/qt/rpcconsole.cpp
• src/rest.cpp
• src/rpc/blockchain.cpp
• src/rpc/rawtransaction.cpp
• src/rpc/server.cpp
• src/script/sign.cpp
• src/test/addrman_tests.cpp
• src/test/bip32_tests.cpp
• src/test/net_tests.cpp
• src/test/rpc_tests.cpp
• src/test/settings_tests.cpp
• src/test/sighash_tests.cpp
• src/test/txpackage_tests.cpp
• src/test/util/setup_common.cpp
• src/test/util/txmempool.cpp
• src/test/util_tests.cpp
• src/test/util_threadnames_tests.cpp
• src/test/validation_block_tests.cpp
• src/util/result.h
• src/validation.cpp
• src/validation.h
• src/wallet/rpc/backup.cpp
• src/wallet/salvage.cpp
• src/wallet/spend.cpp
• src/wallet/test/wallet_tests.cpp

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

• src/.bear-tidy-config
• src/.clang-tidy
• src/rpc/server.cpp
• src/rest.cpp
• src/node/blockstorage.cpp
• src/test/bip32_tests.cpp
• src/test/util_threadnames_tests.cpp
• src/test/addrman_tests.cpp
• src/wallet/spend.cpp
• src/test/sighash_tests.cpp
• src/external_signer.cpp
• src/test/net_tests.cpp
• src/bench/wallet_loading.cpp
• src/wallet/salvage.cpp
• src/test/settings_tests.cpp
• src/wallet/rpc/backup.cpp
• src/test/rpc_tests.cpp
• src/rpc/blockchain.cpp
• src/test/txpackage_tests.cpp
• src/wallet/test/wallet_tests.cpp
• src/qt/rpcconsole.cpp
• src/httpserver.cpp
• src/test/validation_block_tests.cpp
• src/script/sign.cpp
• src/test/util_tests.cpp
• src/test/util/setup_common.cpp
• src/net_processing.cpp

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

• src/rpc/rawtransaction.cpp
• src/addrdb.h
• src/util/result.h
• src/validation.cpp
• src/addrdb.cpp
• src/init.cpp

---

Walkthrough

This PR converts the addrman loading API to return util::Result<std::unique_ptr> (removing the out-parameter), updates call sites (e.g., AppInitMain) to handle the new result, adjusts util::Result to correctly support void via an internal alias and adds a default success constructor, moves the versionbits warning cache into ChainstateManager (removing the file-scope global), enables clang-tidy's modernize-use-emplace check, and applies widespread push_back → emplace_back updates across many files. Bear-tidy output exclusions were extended for two benchmark files.

Sequence Diagram(s)

sequenceDiagram
    participant AppInitMain
    participant LoadAddrman
    participant DiskDB
    participant AddrMan

    AppInitMain->>LoadAddrman: LoadAddrman(netgroupman, args)
    LoadAddrman->>DiskDB: Read addrman DB
    alt DB OK
        DiskDB-->>LoadAddrman: addrman data
        LoadAddrman->>AddrMan: construct AddrMan
        AddrMan-->>LoadAddrman: unique_ptr<AddrMan>
        LoadAddrman-->>AppInitMain: util::Result<std::unique_ptr<AddrMan>> (success)
        AppInitMain->>AppInitMain: node.addrman = std::move(*result)
    else DB error
        DiskDB-->>LoadAddrman: error
        LoadAddrman-->>AppInitMain: util::Error (via util::Result)
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

• UdjinM6

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 29.09% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies this as a backport of three specific Bitcoin Core PR numbers (25977, 27357, 28583), which directly corresponds to the changeset content described across multiple modified files implementing modernization changes and API refactoring from Bitcoin Core v26.
Description check	✅ Passed	The description accurately relates to the changeset by stating backports from Bitcoin Core v26 are being merged and that testing was performed via unit and functional tests, which aligns with the scope of changes across 40+ files in the PR.
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]

🧹 Nitpick comments (1)

src/util/result.h (1)

46-46: Constrain the default constructor to Result<void> using a requires clause.

Line 46 is intended only for void, but it is currently available for all specializations. This default constructor initializes the variant with std::monostate, which is semantically incorrect for Result<non-void> types. Constraining it improves API clarity and prevents accidental misuse.

♻️ Proposed fix

-    Result() : m_variant{std::in_place_index_t<1>{}, std::monostate{}} {}  // constructor for void
+    Result() requires std::is_same_v<M, void>
+        : m_variant{std::in_place_index_t<1>{}, std::monostate{}} {} // constructor for void

🤖 Prompt for AI Agents

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

In `@src/util/result.h` at line 46, The default constructor currently available
for all Result<T> specializations should be constrained to only Result<void>;
update the constructor declaration for Result() to add a requires clause (e.g.,
requires std::is_void_v<T>) so it only participates when T is void, leaving
other specializations without this monostate constructor; modify the Result()
definition that initializes m_variant{std::in_place_index_t<1>{},
std::monostate{}} to use that requires constraint (or equivalent SFINAE) so
non-void Result<T> cannot be default-constructed into monostate.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@src/util/result.h`:
- Line 46: The default constructor currently available for all Result<T>
specializations should be constrained to only Result<void>; update the
constructor declaration for Result() to add a requires clause (e.g., requires
std::is_void_v<T>) so it only participates when T is void, leaving other
specializations without this monostate constructor; modify the Result()
definition that initializes m_variant{std::in_place_index_t<1>{},
std::monostate{}} to use that requires constraint (or equivalent SFINAE) so
non-void Result<T> cannot be default-constructed into monostate.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 6bf73ca4-aba1-482f-9bb9-90fadee4257f

📥 Commits

Reviewing files that changed from the base of the PR and between af926ae and 0c2eedd.

📒 Files selected for processing (38)

• src/.bear-tidy-config
• src/.clang-tidy
• src/addrdb.cpp
• src/addrdb.h
• src/bench/block_assemble.cpp
• src/bench/wallet_loading.cpp
• src/external_signer.cpp
• src/httpserver.cpp
• src/init.cpp
• src/net_permissions.cpp
• src/net_processing.cpp
• src/node/blockstorage.cpp
• src/qt/rpcconsole.cpp
• src/rest.cpp
• src/rpc/blockchain.cpp
• src/rpc/net.cpp
• src/rpc/rawtransaction.cpp
• src/rpc/server.cpp
• src/script/sign.cpp
• src/test/addrman_tests.cpp
• src/test/bip32_tests.cpp
• src/test/net_tests.cpp
• src/test/rpc_tests.cpp
• src/test/settings_tests.cpp
• src/test/sighash_tests.cpp
• src/test/txpackage_tests.cpp
• src/test/util/setup_common.cpp
• src/test/util/txmempool.cpp
• src/test/util_tests.cpp
• src/test/util_threadnames_tests.cpp
• src/test/validation_block_tests.cpp
• src/util/result.h
• src/validation.cpp
• src/validation.h
• src/wallet/rpc/backup.cpp
• src/wallet/salvage.cpp
• src/wallet/spend.cpp
• src/wallet/test/wallet_tests.cpp

[vijaydasmp]

Hello @UdjinM6 @knst requesting review

[thepastaclaw]

Code Review

Clean backport of 5 Bitcoin Core PRs. No blocking issues found. Backport prerequisite analysis confirms no missing dependencies. Minor emplace optimization nitpicks only.

Reviewed commit: 0c2eedd

[thepastaclaw]

Review Gate

Commit: 496defa3

• ✅ Debounce: 4777m ago (need 30m)

• ⏳ CI checks: build failure: win64-build / Build source, linux64_nowallet-build / Build source, linux64-build / Build source

• ✅ CodeRabbit review: comment found

• ✅ Off-peak hours: off-peak (01:03 PM PT Saturday)

• Run review now (check to override)

[UdjinM6]

• e1c4713 should say "28583", not "28586"
• 3b99d9e looks like DMN, already done in 7f49f0e
• db4fe78 looks too partial and would cause future merge conflicts, should be postponed till later imo

[vijaydasmp]

• e1c4713 should say "28583", not "28586"
• 3b99d9e looks like DMN, already done in 7f49f0e
• db4fe78 looks too partial and would cause future merge conflicts, should be postponed till later imo

checking it

[coderabbitai]

Actionable comments posted: 1

🤖 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/addrdb.cpp`:
- Line 221: Restore the explicit std::move on the return of the local variable
addrman so the move-only std::unique_ptr<AddrMan> is moved into the util::Result
wrapper; update the return statement that currently reads "return addrman;" to
use "return std::move(addrman);" to ensure the unique_ptr is moved (this affects
the return of util::Result<std::unique_ptr<AddrMan>> from the function that
constructs addrman).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 42397778-3079-4c2c-85e2-06653cf5cd34

📥 Commits

Reviewing files that changed from the base of the PR and between 0c2eedd and dc1f423.

📒 Files selected for processing (36)

• src/.bear-tidy-config
• src/.clang-tidy
• src/addrdb.cpp
• src/addrdb.h
• src/bench/wallet_loading.cpp
• src/external_signer.cpp
• src/httpserver.cpp
• src/init.cpp
• src/net_permissions.cpp
• src/net_processing.cpp
• src/node/blockstorage.cpp
• src/qt/rpcconsole.cpp
• src/rest.cpp
• src/rpc/blockchain.cpp
• src/rpc/rawtransaction.cpp
• src/rpc/server.cpp
• src/script/sign.cpp
• src/test/addrman_tests.cpp
• src/test/bip32_tests.cpp
• src/test/net_tests.cpp
• src/test/rpc_tests.cpp
• src/test/settings_tests.cpp
• src/test/sighash_tests.cpp
• src/test/txpackage_tests.cpp
• src/test/util/setup_common.cpp
• src/test/util/txmempool.cpp
• src/test/util_tests.cpp
• src/test/util_threadnames_tests.cpp
• src/test/validation_block_tests.cpp
• src/util/result.h
• src/validation.cpp
• src/validation.h
• src/wallet/rpc/backup.cpp
• src/wallet/salvage.cpp
• src/wallet/spend.cpp
• src/wallet/test/wallet_tests.cpp

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

• src/.clang-tidy
• src/.bear-tidy-config
• src/test/util_threadnames_tests.cpp
• src/external_signer.cpp
• src/bench/wallet_loading.cpp
• src/test/addrman_tests.cpp
• src/rest.cpp
• src/test/bip32_tests.cpp
• src/rpc/server.cpp
• src/wallet/salvage.cpp
• src/test/sighash_tests.cpp
• src/wallet/spend.cpp
• src/test/rpc_tests.cpp
• src/test/validation_block_tests.cpp
• src/rpc/rawtransaction.cpp
• src/node/blockstorage.cpp
• src/wallet/rpc/backup.cpp
• src/validation.h
• src/test/settings_tests.cpp
• src/qt/rpcconsole.cpp
• src/rpc/blockchain.cpp
• src/test/net_tests.cpp
• src/wallet/test/wallet_tests.cpp
• src/script/sign.cpp
• src/test/util/setup_common.cpp
• src/net_processing.cpp
• src/test/util_tests.cpp

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

• src/test/txpackage_tests.cpp
• src/httpserver.cpp
• src/util/result.h
• src/addrdb.h
• src/init.cpp

[thepastaclaw]

Quick review pass on the current head (1196ab11 on my side):

• Udjin's earlier requested-changes review appears to have been against an older iteration. Those three points are no longer true on the live series:
  • the 28586 typo is already fixed (28583)
  • the partial bitcoin#27594 commit is no longer in the branch
  • the current branch is now just the three intended backports (25977, 27357, 28583)
• I did find one real delta from upstream in the bitcoin#25977 backport: LoadAddrman() dropped upstream's return std::move(addrman); line, while keeping the comment that says it is temporarily needed for the clang workaround. I restored that line and folded it into the original 25977 backport commit.

I wasn't able to push directly to vijaydasmp/dash from my token (403), so I pushed the fixed branch here instead:

• thepastaclaw:tracker-992

If you want to pull/cherry-pick it, that branch contains the clean amended history for this PR.

Also, current CI is otherwise green apart from linux64_tsan-test, which failed in interface_zmq_dash.py --legacy-wallet and looks unrelated to the touched backport files.

[thepastaclaw]

Rechecked this today: the live PR head is still dc1f4232, so the amended fix branch has not been picked up here yet.

The only real upstream delta I found remains the missing return std::move(addrman); in LoadAddrman() from the bitcoin#25977 backport. The clean rewritten series is still available at:

• thepastaclaw:tracker-992
• tip: 1196ab11

I also re-tested whether I can update vijaydasmp:March_2026_6 directly from this account. GitHub still rejects the push with HTTP 403 even though maintainerCanModify shows true, so I cannot move the PR head myself from here.

@vijaydasmp if you want to pick up the cleaned-up history as-is, the rescue branch above is ready to use.

[vijaydasmp]

Rechecked this today: the live PR head is still dc1f4232, so the amended fix branch has not been picked up here yet.

The only real upstream delta I found remains the missing return std::move(addrman); in LoadAddrman() from the bitcoin#25977 backport. The clean rewritten series is still available at:

• thepastaclaw:tracker-992
• tip: 1196ab11

I also re-tested whether I can update vijaydasmp:March_2026_6 directly from this account. GitHub still rejects the push with HTTP 403 even though maintainerCanModify shows true, so I cannot move the PR head myself from here.

@vijaydasmp if you want to pick up the cleaned-up history as-is, the rescue branch above is ready to use.

checking

[thepastaclaw]

Code Review

Backport of three upstream Bitcoin Core refactors (bitcoin#25977 util::Result, bitcoin#27357 m_warningcache, bitcoin#28583 modernize-use-emplace). The Dash-specific merge resolutions around LoadAddrman and m_warningcache are wired up correctly at every remaining call site, no Dash subsystem (LLMQ, evo, masternode, governance, CoinJoin) is touched, and no consensus behavior changes. All agent findings were examined; none rise to a real correctness problem in this SHA.

Reviewed commit: dc1f423