Skip to content

backport: Merge bitcoin#29404, 28144, 28118 - #7124

Merged
PastaPastaPasta merged 3 commits into
dashpay:developfrom
vijaydasmp:Feb_2026_04
Sep 13, 2026
Merged

PastaPastaPasta merged 3 commits into
dashpay:developfrom
vijaydasmp:Feb_2026_04

Conversation

@vijaydasmp

@vijaydasmp vijaydasmp commented Feb 2, 2026

Copy link
Copy Markdown

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

@github-actions

github-actions Bot commented Feb 2, 2026

Copy link
Copy Markdown

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@vijaydasmp vijaydasmp changed the title Backport: Merge bitcoin/bitcoin#29404 backport : Merge bitcoin#29404 Feb 2, 2026
@vijaydasmp vijaydasmp changed the title backport : Merge bitcoin#29404 backport: Merge bitcoin#29404 Feb 2, 2026
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#29404 backport: Merge bitcoin#29404, 28144, 28118 Feb 3, 2026
@github-actions

Copy link
Copy Markdown

This pull request has conflicts, please rebase.

@github-actions

Copy link
Copy Markdown

This pull request has conflicts, please rebase.

@vijaydasmp
vijaydasmp force-pushed the Feb_2026_04 branch 4 times, most recently from 2779ae2 to 2cd9132 Compare February 26, 2026 08:22
@vijaydasmp
vijaydasmp marked this pull request as ready for review February 26, 2026 11:28
@coderabbitai

coderabbitai Bot commented Feb 26, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

We couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting @coderabbitai full review.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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

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

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 2cd9132c407172000ff715ed822d5b7faae917ea and 10a457bf5298ac37364d9092721b0a3a62758d74.

📒 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

sockname = p2p_conn._transport.get_extra_info("socket").getsockname()
our_addr_and_port = f"{sockname[0]}:{sockname[1]}"
info = [peer for peer in self.getpeerinfo() if peer["addr"] == our_addr_and_port]
dst_addr_and_port = f"{p2p_conn.dstaddr}:{p2p_conn.dstport}"
info = [peer for peer in self.getpeerinfo() if peer["addr"] == our_addr_and_port and peer["addrbind"] == dst_addr_and_port]

@coderabbitai coderabbitai Bot Mar 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified this is still valid. I prepared the one-line optional-field fix in vijaydasmp#28; it preserves the exact addr+addrbind match while avoiding KeyError when getpeerinfo omits addrbind. Local py_compile/diff checks and the separate backport gate passed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thepastaclaw, thanks for confirming and for preparing the fix — appreciate you validating it locally with py_compile/diff checks and the backport gate. Once the fix from vijaydasmp#28 lands in this PR's branch, the KeyError concern will be resolved.

ヽ(・∀・)ノ


🧠 Learnings used
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 7398
File: test/functional/p2p_compactblocks.py:838-907
Timestamp: 2026-07-02T14:18:23.486Z
Learning: In dashpay/dash, avoid modifying Python files under test/functional that are known backports from Bitcoin Core (for example, files like test/functional/p2p_compactblocks.py) for minor stylistic changes or test-hygiene-only improvements. Keep them as close to the upstream Bitcoin Core version as possible to minimize divergence and make future backports easier; only change them when strictly necessary for correctness, compatibility, or to resolve a real failing test.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI on helper PR vijaydasmp#28 has completed at ffe9358. Every build, lint, and non-TSAN test check passed. The only red job is the known unrelated p2p_blocksonly.py TSAN relay timeout tracked in #7436; all three attempts timed out in second_peer.wait_for_tx with no ThreadSanitizer report. The helper fix itself needs no change.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thepastaclaw, thanks for the detailed CI confirmation. Good to see all build/lint/non-TSAN checks green on ffe9358, and that the p2p_blocksonly.py TSAN timeout is a known, already-tracked flake (#7436) unrelated to the addrbind fix. Once vijaydasmp#28 is merged into this PR's branch, the KeyError concern should be fully resolved here.

(っ˘̩╭╮˘̩)っ

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: ae9e403d

🟡 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

thepastaclaw commented Mar 30, 2026

Copy link
Copy Markdown
Collaborator

✅ Final review complete — no blockers (commit 1de5f31) · triage: low · Phase 2 only (queue backlog)

@vijaydasmp

Copy link
Copy Markdown
Author

Hello @UdjinM6, @knst requesting review

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Cumulative review at ae9e403d40 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: ae9e403d

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: ae9e403d

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

@knst knst left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

29404: missing changes in util/system.cpp [see common/system.cpp]
28144: OK
28118: OK

Comment thread src/test/system_tests.cpp
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
//

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

29404: missing changes in util/system.cpp [file renamed, in original backport it's common/system.cpp

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking it

@github-actions

Copy link
Copy Markdown

This pull request has conflicts, please rebase.

9d1dbbd scripted-diff: Fix bitcoin_config_h includes (TheCharlatan)

Pull request description:

  As mentioned in bitcoin#26924 (comment) and bitcoin#29263 (comment), it is currently not safe to remove `bitcoin-config.h` includes from headers because some unrelated file might be depending on it.

  See also bitcoin#26972 for discussion.

  Solve this by including the file directly everywhere it's required, regardless of whether or not it's already included by another header.

  There should be no functional change here, but it will allow us to safely remove includes from headers in the future.

  ~I'm afraid it's a bit tedious to reproduce these commits, but it's reasonably straightforward:~
  Edit: See note below

  ```bash
  # All commands executed from the src/ subdir.

  # Collect all tokens from bitcoin-config.h.in
  # Isolate the tokens and remove blank lines
  # Replace newlines with | and remove the last trailing one
  # Collect all files which use these tokens
  # Filter out subprojects (proper forwarding can be verified from Makefiles)
  # Filter out .rc files
  # Save to a text file
  git grep -E -l `grep undef config/bitcoin-config.h.in | cut -d" " -f2 | grep -v '^$' | tr '\n' '|' | sed 's/|$//'` | grep -v -e "^leveldb/" -e "^secp256k1/" -e "^crc32c/" -e "^minisketch/" -e "^Makefile" -e "\.rc$" > files-with-config-include.txt

  # Find all files from the above list which don't include bitcoin-config.h
  git grep -L -E "config/bitcoin-config.h" -- `cat files-with-config-include.txt`

  # Include them manually with the exception of some files in crypto:
  # crypto/sha256_arm_shani.cpp crypto/sha256_avx2.cpp crypto/sha256_sse41.cpp crypto/sha256_x86_shani.cpp
  # These are exceptions which don't use bitcoin-config.h, rather the Makefile.am adds these cppflags manually.

  # Commit changes. This should match the first commit of this PR.

  # Use the same search as above to find all files which DON'T use any config tokens
  git grep -E -L `grep undef config/bitcoin-config.h.in | cut -d" " -f2 | grep -v '^$' | tr '\n' '|' | sed 's/|$//'` | grep -v -e "^leveldb/" -e "^secp256k1/" -e "^crc32c/" -e "^minisketch/" -e "^Makefile" -e "\.rc$" > files-without-config-include.txt

  # Manually remove the includes and commit changes. This should match the second commit of this PR.
  ```

  Edit: I'll keep this old description for posterity, but the manual approach has been replaced with a scripted diff from TheCharlatan

ACKs for top commit:
  maflcko:
    ACK 9d1dbbd 🚪
  TheCharlatan:
    ACK 9d1dbbd
  hebasto:
    ACK 9d1dbbd, I have reviewed the code and it looks OK.
  fanquake:
    ACK 9d1dbbd

Tree-SHA512: f11ddc4ae6a887f96b954a6b77f310558ddb271088a3fda3edc833669c4251b7f392515224bbb8e5f67eb2c799b4ffed3b07d96454e82ec635c686d0df545872

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Phase 2 only (queue backlog)

No in-scope defects were identified when verifying the supplied Phase-2 assessments against head 37d4305. The configuration include in src/util/system.cpp is present, and the inspected connection-handshake and scheduler paths support the backported changes. Diff whitespace and Python syntax checks passed; native builds and functional tests were not independently run during verification, and Phase-1 review was intentionally skipped.

Review provenance

Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: backport-reviewer); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

  • Triage: normal by gpt-6-astra (effort low) — This is a broad backport spanning build/configuration headers, RPC, wallet, Qt, serialization, validation, and functional tests, but the listed changes are primarily mechanical include cleanups and targeted test fixes without the diff itself changing consensus, funds movement, cryptography, peer deserialization, or storage migrations.
  • Phase 1 reviewers: not run (skipped for throughput: 13 PRs queued, above the 10 limit)
  • Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
  • Phase 2 reviewers: gpt-6-astra — general (completed, effort high); agent phase2-reviewer, gpt-6-astra — backport-reviewer (completed, effort high); agent phase2-reviewer, gpt-6-astra — dash-core-commit-history (completed, effort high); agent phase2-reviewer

@thepastaclaw thepastaclaw added the pastaclaw:approved thepastaclaw's latest review approved this PR label Sep 12, 2026
@thepastaclaw thepastaclaw removed the pastaclaw:approved thepastaclaw's latest review approved this PR label Sep 12, 2026

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Phase 2 only (queue backlog)

The backport changes are otherwise narrow and consistent with the intended Bitcoin Core updates. However, peer matching in the functional-test framework directly indexes the optional addrbind field, so valid getpeerinfo responses can raise KeyError and make tests fail intermittently.

🔴 1 blocking

Review provenance

Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: backport-reviewer); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

  • Triage: low by gpt-6-astra (effort low) — The diff consists of mechanical configuration-header include cleanup and small, contained test reliability changes to mock scheduler synchronization and P2P connection matching, without changing critical production behavior.
  • Phase 1 reviewers: not run (skipped for throughput: 16 PRs queued, above the 10 limit)
  • Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
  • Phase 2 reviewers: gpt-6-astra — general (completed, effort medium); agent phase2-reviewer, gpt-6-astra — backport-reviewer (completed, effort medium); agent phase2-reviewer, gpt-6-astra — dash-core-commit-history (completed, effort medium); agent phase2-reviewer
🤖 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 `test/functional/test_framework/test_node.py`:
- [BLOCKING] test/functional/test_framework/test_node.py:753: Handle optional addrbind without raising KeyError
  `getpeerinfo` does not guarantee that every peer object contains `addrbind`; the field is optional for some connection types and configurations. The new list comprehension indexes `peer["addrbind"]`, so a valid peer response that omits the field raises `KeyError` before the connection can be verified. Filter by the required local address first, compare `addrbind` with a safe lookup, and fall back to the matching peers when `addrbind` is unavailable.

our_addr_and_port = f"{sockname[0]}:{sockname[1]}"
info = [peer for peer in self.getpeerinfo() if peer["addr"] == our_addr_and_port]
dst_addr_and_port = f"{p2p_conn.dstaddr}:{p2p_conn.dstport}"
info = [peer for peer in self.getpeerinfo() if peer["addr"] == our_addr_and_port and peer["addrbind"] == dst_addr_and_port]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: Handle optional addrbind without raising KeyError

getpeerinfo does not guarantee that every peer object contains addrbind; the field is optional for some connection types and configurations. The new list comprehension indexes peer["addrbind"], so a valid peer response that omits the field raises KeyError before the connection can be verified. Filter by the required local address first, compare addrbind with a safe lookup, and fall back to the matching peers when addrbind is unavailable.

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:
info = peers

source: gpt-6-astra (phase2-reviewer: general)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved (re-reviewed at 1de5f318): The current peer-matching code uses peer.get("addrbind") and falls back to address-only candidates, so an omitted addrbind no longer raises KeyError. Isolated checks passed for a missing addrbind, connections sharing a source address but using different destination binds, and an unrelated peer without addrbind.

@thepastaclaw thepastaclaw added the pastaclaw:changes-requested thepastaclaw's latest review requested changes label Sep 12, 2026
…ching.py

8a20f76 test: drop duplicate getaddrs from p2p_getaddr_caching (Martin Zumsande)
feb0096 test: fix intermittent failure in p2p_getaddr_caching (Martin Zumsande)

Pull request description:

  Fixes bitcoin#28133

  In the consistency check, it's not enough to check that our address/port is unique, only the combination of source and target must be unique. Otherwise, the OS may reuse ports for connections to different `-addrbind`, which was happening in the failed runs.

  While at it, the second commit cleans up duplicate `getaddr` messages in `p2p_getaddr_caching.py` that do nothing but generate `Ignoring repeated "getaddr"` log messages (and cleans up some whitespace the python linter complains about).

ACKs for top commit:
  vasild:
    ACK 8a20f76

Tree-SHA512: eabe4727d7887f729074076f6333a918bba8cb34b8e3baaa83f167b441b0daa24f7c4824abcf03a9538a2ef14b2d826ff19aeffcb93a6c20735253a9678aac9c

Fixing Linting error - 'time' module imported but not used
…ckscheduler RPC

fabef12 refactor: Use EnsureAnyNodeContext (MarcoFalke)
fa16406 test: Add SyncWithValidationInterfaceQueue to mockscheduler RPC (MarcoFalke)

Pull request description:

  There should be no risk or downside in adding a call to `SyncWithValidationInterfaceQueue` here. In fact, it will make tests less brittle. For example,

  * If one sets the timeouts in `test/functional/feature_fee_estimation.py` to `0`, on `master` the test will fail and here it will pass.
  * It may avoid a rare (theoretic) intermittent issue in https://github.com/bitcoin/bitcoin/pull/28108/files#r1268966663

ACKs for top commit:
  TheCharlatan:
    ACK fabef12
  furszy:
    Code review ACK fabef12. Convinced by checking all current tests usages.

Tree-SHA512: c9e9a536a8721d1b3f267a66b40578b34948892301affdcad121ef8e02bf17037305d0dd53aa94b1b064753e66f9cfb31823b916b707a9d812627f502b818003
@thepastaclaw thepastaclaw removed the pastaclaw:changes-requested thepastaclaw's latest review requested changes label Sep 12, 2026

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Phase 2 only (queue backlog)

Reviewed the final head and found no actionable correctness or Dash-specific integration issues. The previously reported optional-addrbind KeyError is fixed, and src/util/system.cpp includes the required configuration header for the bitcoin#29404 backport. No in-scope findings remain.

Review provenance

Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: backport-reviewer); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

  • Triage: low by gpt-6-astra (effort low) — The broad file count is mostly mechanical configuration-header relocation, while substantive changes are small and contained to mock-time RPC synchronization and functional-test reliability, without changing critical production surfaces.
  • Phase 1 reviewers: not run (skipped for throughput: 13 PRs queued, above the 10 limit)
  • Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
  • Phase 2 reviewers: gpt-6-astra — general (completed, effort medium); agent phase2-reviewer, gpt-6-astra — backport-reviewer (completed, effort medium); agent phase2-reviewer, gpt-6-astra — dash-core-commit-history (completed, effort medium); agent phase2-reviewer

@thepastaclaw thepastaclaw added the pastaclaw:approved thepastaclaw's latest review approved this PR label Sep 12, 2026
@PastaPastaPasta
PastaPastaPasta merged commit 8be1a17 into dashpay:develop Sep 13, 2026
49 of 50 checks passed
@thepastaclaw thepastaclaw removed the pastaclaw:approved thepastaclaw's latest review approved this PR label Sep 13, 2026
@UdjinM6 UdjinM6 added this to the 24 milestone Sep 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants