backport: Merge bitcoin#28172, 28464#7186
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
a0ab1c7 to
8d656d4
Compare
vijaydasmp
changed the title
3528510 to
fa96c9e
Compare
|
This pull request has conflicts, please rebase. |
|
@vijaydasmp could you please rebase this PR, or close it if it’s no longer needed? Thanks! |
|
✅ Final review complete — no blockers (commit 062b3c5) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR consolidates CConnman connection-capacity configuration into Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/init.cpp`:
- Around line 2553-2554: Assign m_max_automatic_connections before computing
m_max_outbound_onion in the init logic, since the current ordering in the
connection setup leaves connOptions.m_max_automatic_connections at its default
when the onion budget is derived. Update the code around the connOptions
assignments so m_max_automatic_connections is set from nMaxConnections first,
then use that value to calculate m_max_outbound_onion.
🪄 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: d71e2c47-6e6d-42e0-b4e9-685b035d5fa7
📒 Files selected for processing (7)
src/init.cppsrc/net.cppsrc/net.hsrc/rpc/util.cppsrc/rpc/util.hsrc/test/denialofservice_tests.cppsrc/util/strencodings.h
💤 Files with no reviewable changes (1)
- src/util/strencodings.h
| connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.m_max_automatic_connections / 2); | ||
| connOptions.m_max_automatic_connections = nMaxConnections; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/init.cpp around the reported lines =="
sed -n '2538,2562p' src/init.cpp | cat -n
echo
echo "== m_max_outbound_onion declaration/initialization =="
rg -n "m_max_outbound_onion|m_max_automatic_connections" src/net.h src -g '!src/leveldb/**' -g '!src/secp256k1/**'
echo
echo "== call sites / consumers =="
rg -n "GetMaxOutboundOnionNodeCount|m_max_outbound_onion|nOutboundOnionRelay" src -g '!src/leveldb/**' -g '!src/secp256k1/**'Repository: dashpay/dash
Length of output: 5311
Assign m_max_automatic_connections before deriving onion capacity
m_max_outbound_onion is computed from the default 0 here, so it becomes 0 and the onion outbound budget never opens. Move the m_max_automatic_connections = nMaxConnections assignment above this line.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/init.cpp` around lines 2553 - 2554, Assign m_max_automatic_connections
before computing m_max_outbound_onion in the init logic, since the current
ordering in the connection setup leaves connOptions.m_max_automatic_connections
at its default when the onion budget is derived. Update the code around the
connOptions assignments so m_max_automatic_connections is set from
nMaxConnections first, then use that value to calculate m_max_outbound_onion.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa96c9e4c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.nMaxConnections / 2); | ||
| connOptions.nMaxAddnode = MAX_ADDNODE_CONNECTIONS; | ||
| connOptions.nMaxFeeler = MAX_FEELER_CONNECTIONS; | ||
| connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.m_max_automatic_connections / 2); |
There was a problem hiding this comment.
When Tor/onion is reachable, this computes m_max_outbound_onion from connOptions.m_max_automatic_connections while that option is still its default 0, so the onion target is always initialized to 0 before the real nMaxConnections value is assigned on the next line. As a result the onion-only branch in ThreadOpenConnections() never runs and existing outbound onion peers get no reserved protection, regressing the intended default of up to two onion outbound peers; assign m_max_automatic_connections first or compute from nMaxConnections.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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): Verified one blocking merge-resolution bug: at src/init.cpp:2553 the Dash-specific line initializing connOptions.m_max_outbound_onion reads connOptions.m_max_automatic_connections before it is assigned on line 2554. Because Options::m_max_automatic_connections defaults to 0 (src/net.h:1189), m_max_outbound_onion is silently forced to 0, and CConnman propagates this into m_max_outbound_onion at src/net.h:1223, disabling Dash's outbound-onion preference in CConnman::ThreadOpenConnections. Also flagging a stale comment and a PR-title/scope mismatch (bitcoin#28649 is advertised but was already backported in 0528cfd, not in this PR). Reviewer coverage note: all three codex lanes (codex-general, codex-dash-core-commit-history, codex-backport-reviewer) failed to run due to a model configuration error and produced no output; only the three Claude lanes were successful.
4 blocking | 2 nitpick(s)
Source: reviewers: opus (backport-reviewer), opus (dash-core-commit-history), opus (general), gpt-5.5[high] (backport-reviewer) [failed], gpt-5.5[high] (dash-core-commit-history) [failed], gpt-5.5[high] (general) [failed]; verifier: opus; specialists: dash-core-commit-history, backport-reviewer.
Reviewer coverage note: the Claude/Opus lanes completed for general, dash-core-commit-history, and backport-reviewer. The three Codex lanes failed before review because ACP rejected the configured gpt-5.5[high] model override, so their outputs were not used.
Policy gate actions:
- Promoted from
claude-backport-reviewer: bitcoin#28464 merge resolution: m_max_outbound_onion uses m_max_automatic_connections before it is assigned - Promoted from
claude-backport-reviewer: Comment references pre-rename member m_max_outbound after 28464 rename - Promoted from
claude-backport-reviewer: PR title advertises bitcoin#28649 but the commit is not in the branch - summary_overridden
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/init.cpp`:
- [BLOCKING] src/init.cpp:2553-2554: m_max_outbound_onion computed before m_max_automatic_connections is assigned — value is always 0 (source: claude)
The bitcoin#28464 merge resolution swapped the initialization order of two adjacent fields on `connOptions`:
```cpp
CConnman::Options connOptions;
connOptions.nLocalServices = nLocalServices;
connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.m_max_automatic_connections / 2);
connOptions.m_max_automatic_connections = nMaxConnections;
Options::m_max_automatic_connections default-initializes to 0 (src/net.h:1189), and the assignment on line 2554 happens after the read on line 2553. So m_max_outbound_onion is always std::min(MAX_DESIRED_ONION_CONNECTIONS, 0 / 2) == 0.
That zero is then copied verbatim into CConnman::m_max_outbound_onion in CConnman::Init (src/net.h:1223). The outbound-onion preference in CConnman::ThreadOpenConnections (nOutboundOnionRelay < m_max_outbound_onion at src/net.cpp:3128) can never be true, and GetMaxOutboundOnionNodeCount() returns 0. The dedicated "prefer onion" outbound path Dash provided pre-backport is silently disabled — a functional regression not covered by any test.
Pre-backport, the analogous line read connOptions.nMaxConnections / 2 after nMaxConnections had been assigned on the preceding line, so it worked. Upstream Bitcoin sidesteps the whole issue by removing m_max_outbound_onion from Options and deriving it inside CConnman::Init(); the Dash merge kept it in Options but reversed the two assignments.
Fix by swapping the order (so m_max_automatic_connections is assigned first) or by referencing the local nMaxConnections directly.
-
[NITPICK] src/init.cpp:2551-2555: PR title advertises bitcoin#28649 but it is not in this PR (source: claude)
The PR title isbackport: Merge bitcoin#28172, 28649, 28464, but the branch relative todevelopcontains only two merges: adfe38a (bitcoin#28172) and fa96c9e (bitcoin#28464). bitcoin#28649 ("Do the SOCKS5 handshake reliably") was already backported separately in commit 0528cfd and is not part of this stack. Update the PR title/description to accurately reflect the two contained backports so upstream-tracking metadata stays consistent. -
[BLOCKING] src/init.cpp:2553-2554: bitcoin#28464 merge resolution: m_max_outbound_onion uses m_max_automatic_connections before it is assigned [policy-gated] (source: claude-backport-reviewer)
Upstream bitcoin#28464 replaced the blockconnOptions.nMaxConnections = nMaxConnections; connOptions.m_max_outbound_full_relay = std::min(MAX_OUTBOUND_FULL_RELAY_CONNECTIONS, connOptions.nMaxConnections); connOptions.m_max_outbound_block_relay = std::min(MAX_BLOCK_RELAY_ONLY_CONNECTIONS, connOptions.nMaxConnections-connOptions.m_max_outbound_full_relay); connOptions.nMaxAddnode = MAX_ADDNODE_CONNECTIONS; connOptions.nMaxFeeler = MAX_FEELER_CONNECTIONS;with a single assignment:
connOptions.m_max_automatic_connections = nMaxConnections;Dash has an extra Dash-specific line for outbound onion:
connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.nMaxConnections / 2);Pre-merge this was safe because
connOptions.nMaxConnectionshad already been set on the preceding line. In the merge resolution, the onion line was updated to referenceconnOptions.m_max_automatic_connectionsand placed BEFORE that field is assigned:connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.m_max_automatic_connections / 2); connOptions.m_max_automatic_connections = nMaxConnections;At this point
connOptions.m_max_automatic_connectionsis still 0 (the default-constructed value in the Options struct after 28464), so this evaluates tostd::min(MAX_DESIRED_ONION_CONNECTIONS, 0) = 0. The value 0 is then propagated intoCConnman::m_max_outbound_onionbyCConnman::Init, and the outbound-onion preference inCConnman::ThreadOpenConnections(nOutboundOnionRelay < m_max_outbound_onion, src/net.cpp:3128) becomes a permanently-false condition. Effectively the Dash-specific onion-outbound preference is silently disabled after this backport.Fix: either swap the two lines so
m_max_automatic_connectionsis assigned first, or reference the localnMaxConnectionsvariable directly (std::min(MAX_DESIRED_ONION_CONNECTIONS, nMaxConnections / 2)). Not a missing upstream prerequisite — a Dash merge-resolution error interacting with a Dash-only line.
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: trueor a matching entry inpolicy_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/init.cpp:2551-2554: PR title advertises bitcoin#28649 but the commit is not in the branch [policy-gated] (source: claude-backport-reviewer)
The PR title 'backport: Merge bitcoin#28172, 28649, 28464' claims three upstream backports, butgit log origin/develop..HEADon the head SHA (fa96c9e) shows only two merge commits (adfe38a for bitcoin#28172 and fa96c9e for bitcoin#28464). bitcoin#28649 (netbase: reliable SOCKS5 handshake via SendComplete + Sock::SendComplete Span refactor) is absent. Not a code-completeness issue for what is here, but reviewers/mergers should either drop bitcoin#28649 from the PR title/description or add the commit before merging so upstream-tracking metadata stays accurate.
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: trueor a matching entry inpolicy_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/net.h:
-
[NITPICK] src/net.h:1847: Comment references pre-rename member m_max_outbound (source: claude)
The bitcoin#28464 scripted-diff renamedm_max_outbound→m_max_automatic_outboundthroughout net.h/net.cpp. The comment on them_max_outbound_onionfield still reads// ... does not increase m_max_outbound. Upstream updated this comment via the scripted-diff; the Dash cherry-pick left it referencing the now-nonexistent member. Cosmetic only. -
[BLOCKING] src/net.h:1847: Comment references pre-rename member m_max_outbound after 28464 rename [policy-gated] (source: claude-backport-reviewer)
The scripted-diff commit in bitcoin#28464 renamedm_max_outboundtom_max_automatic_outboundthroughout net.h/net.cpp. The comment on them_max_outbound_onionfield still says '...does not increase m_max_outbound'. Upstream updated all occurrences (including this comment) via the scripted-diff; the Dash cherry-pick left the comment referencing the now-nonexistent member. Cosmetic only, no functional impact.
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: trueor a matching entry inpolicy_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.
</details>
| connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.m_max_automatic_connections / 2); | ||
| connOptions.m_max_automatic_connections = nMaxConnections; |
There was a problem hiding this comment.
🔴 Blocking: m_max_outbound_onion computed before m_max_automatic_connections is assigned — value is always 0
The bitcoin#28464 merge resolution swapped the initialization order of two adjacent fields on connOptions:
CConnman::Options connOptions;
connOptions.nLocalServices = nLocalServices;
connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.m_max_automatic_connections / 2);
connOptions.m_max_automatic_connections = nMaxConnections;Options::m_max_automatic_connections default-initializes to 0 (src/net.h:1189), and the assignment on line 2554 happens after the read on line 2553. So m_max_outbound_onion is always std::min(MAX_DESIRED_ONION_CONNECTIONS, 0 / 2) == 0.
That zero is then copied verbatim into CConnman::m_max_outbound_onion in CConnman::Init (src/net.h:1223). The outbound-onion preference in CConnman::ThreadOpenConnections (nOutboundOnionRelay < m_max_outbound_onion at src/net.cpp:3128) can never be true, and GetMaxOutboundOnionNodeCount() returns 0. The dedicated "prefer onion" outbound path Dash provided pre-backport is silently disabled — a functional regression not covered by any test.
Pre-backport, the analogous line read connOptions.nMaxConnections / 2 after nMaxConnections had been assigned on the preceding line, so it worked. Upstream Bitcoin sidesteps the whole issue by removing m_max_outbound_onion from Options and deriving it inside CConnman::Init(); the Dash merge kept it in Options but reversed the two assignments.
Fix by swapping the order (so m_max_automatic_connections is assigned first) or by referencing the local nMaxConnections directly.
| connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.m_max_automatic_connections / 2); | |
| connOptions.m_max_automatic_connections = nMaxConnections; | |
| connOptions.m_max_automatic_connections = nMaxConnections; | |
| connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.m_max_automatic_connections / 2); |
source: ['claude']
There was a problem hiding this comment.
Correction: this prior finding is still valid at 062b3c5c40, not resolved. The current head still reads connOptions.m_max_automatic_connections for the onion limit one line before assigning that member from nMaxConnections, so the prior zero-value initialization bug remains.
Corrected after the exact-SHA verifier reconciliation; the earlier auto-resolved text was produced by duplicate-thread suppression and was wrong.
| @@ -1841,9 +1847,11 @@ friend class CNode; | |||
| // How many onion outbound peers we want; don't care if full or block only; does not increase m_max_outbound | |||
There was a problem hiding this comment.
💬 Nitpick: Comment references pre-rename member m_max_outbound
The bitcoin#28464 scripted-diff renamed m_max_outbound → m_max_automatic_outbound throughout net.h/net.cpp. The comment on the m_max_outbound_onion field still reads // ... does not increase m_max_outbound. Upstream updated this comment via the scripted-diff; the Dash cherry-pick left it referencing the now-nonexistent member. Cosmetic only.
| // How many onion outbound peers we want; don't care if full or block only; does not increase m_max_outbound | |
| // How many onion outbound peers we want; don't care if full or block only; does not increase m_max_automatic_outbound |
source: ['claude']
| CConnman::Options connOptions; | ||
| connOptions.nLocalServices = nLocalServices; | ||
| connOptions.nMaxConnections = nMaxConnections; | ||
| connOptions.m_max_outbound_full_relay = std::min(MAX_OUTBOUND_FULL_RELAY_CONNECTIONS, connOptions.nMaxConnections); | ||
| connOptions.m_max_outbound_block_relay = std::min(MAX_BLOCK_RELAY_ONLY_CONNECTIONS, connOptions.nMaxConnections-connOptions.m_max_outbound_full_relay); | ||
| connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.nMaxConnections / 2); | ||
| connOptions.nMaxAddnode = MAX_ADDNODE_CONNECTIONS; | ||
| connOptions.nMaxFeeler = MAX_FEELER_CONNECTIONS; | ||
| connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.m_max_automatic_connections / 2); | ||
| connOptions.m_max_automatic_connections = nMaxConnections; | ||
| connOptions.uiInterface = &uiInterface; |
There was a problem hiding this comment.
💬 Nitpick: PR title advertises bitcoin#28649 but it is not in this PR
The PR title is backport: Merge bitcoin#28172, 28649, 28464, but the branch relative to develop contains only two merges: adfe38a (bitcoin#28172) and fa96c9e (bitcoin#28464). bitcoin#28649 ("Do the SOCKS5 handshake reliably") was already backported separately in commit 0528cfd and is not part of this stack. Update the PR title/description to accurately reflect the two contained backports so upstream-tracking metadata stays consistent.
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — PR title advertises bitcoin#28649 but it is not in this PR 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.
| connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.m_max_automatic_connections / 2); | ||
| connOptions.m_max_automatic_connections = nMaxConnections; |
There was a problem hiding this comment.
🔴 Blocking: bitcoin#28464 merge resolution: m_max_outbound_onion uses m_max_automatic_connections before it is assigned
Upstream bitcoin#28464 replaced the block
connOptions.nMaxConnections = nMaxConnections;
connOptions.m_max_outbound_full_relay = std::min(MAX_OUTBOUND_FULL_RELAY_CONNECTIONS, connOptions.nMaxConnections);
connOptions.m_max_outbound_block_relay = std::min(MAX_BLOCK_RELAY_ONLY_CONNECTIONS, connOptions.nMaxConnections-connOptions.m_max_outbound_full_relay);
connOptions.nMaxAddnode = MAX_ADDNODE_CONNECTIONS;
connOptions.nMaxFeeler = MAX_FEELER_CONNECTIONS;
with a single assignment:
connOptions.m_max_automatic_connections = nMaxConnections;
Dash has an extra Dash-specific line for outbound onion:
connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.nMaxConnections / 2);
Pre-merge this was safe because connOptions.nMaxConnections had already been set on the preceding line. In the merge resolution, the onion line was updated to reference connOptions.m_max_automatic_connections and placed BEFORE that field is assigned:
connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.m_max_automatic_connections / 2);
connOptions.m_max_automatic_connections = nMaxConnections;
At this point connOptions.m_max_automatic_connections is still 0 (the default-constructed value in the Options struct after 28464), so this evaluates to std::min(MAX_DESIRED_ONION_CONNECTIONS, 0) = 0. The value 0 is then propagated into CConnman::m_max_outbound_onion by CConnman::Init, and the outbound-onion preference in CConnman::ThreadOpenConnections (nOutboundOnionRelay < m_max_outbound_onion, src/net.cpp:3128) becomes a permanently-false condition. Effectively the Dash-specific onion-outbound preference is silently disabled after this backport.
Fix: either swap the two lines so m_max_automatic_connections is assigned first, or reference the local nMaxConnections variable directly (std::min(MAX_DESIRED_ONION_CONNECTIONS, nMaxConnections / 2)). Not a missing upstream prerequisite — a Dash merge-resolution error interacting with a Dash-only line.
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.
source: ['claude-backport-reviewer']
There was a problem hiding this comment.
Correction: this prior finding is still valid at 062b3c5c40, not resolved. The current head still computes connOptions.m_max_outbound_onion from connOptions.m_max_automatic_connections at src/init.cpp:2622 before assigning m_max_automatic_connections = nMaxConnections at src/init.cpp:2623, so the onion outbound target is derived from the default zero value.
Corrected after the exact-SHA verifier reconciliation; the earlier auto-resolved text was produced by duplicate-thread suppression and was wrong.
| @@ -1841,9 +1847,11 @@ friend class CNode; | |||
| // How many onion outbound peers we want; don't care if full or block only; does not increase m_max_outbound | |||
There was a problem hiding this comment.
🔴 Blocking: Comment references pre-rename member m_max_outbound after 28464 rename
The scripted-diff commit in bitcoin#28464 renamed m_max_outbound to m_max_automatic_outbound throughout net.h/net.cpp. The comment on the m_max_outbound_onion field still says '...does not increase m_max_outbound'. Upstream updated all occurrences (including this comment) via the scripted-diff; the Dash cherry-pick left the comment referencing the now-nonexistent member. Cosmetic only, no functional impact.
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.
source: ['claude-backport-reviewer']
There was a problem hiding this comment.
Correction: this prior finding is still valid at 062b3c5c40, not resolved. The comment at src/net.h:1862 still says the onion preference does not increase m_max_outbound, but bitcoin#28464 renamed the corresponding field to m_max_automatic_outbound.
Corrected after the exact-SHA verifier reconciliation; the earlier auto-resolved text was produced by duplicate-thread suppression and was wrong.
| CConnman::Options connOptions; | ||
| connOptions.nLocalServices = nLocalServices; | ||
| connOptions.nMaxConnections = nMaxConnections; | ||
| connOptions.m_max_outbound_full_relay = std::min(MAX_OUTBOUND_FULL_RELAY_CONNECTIONS, connOptions.nMaxConnections); | ||
| connOptions.m_max_outbound_block_relay = std::min(MAX_BLOCK_RELAY_ONLY_CONNECTIONS, connOptions.nMaxConnections-connOptions.m_max_outbound_full_relay); | ||
| connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.nMaxConnections / 2); | ||
| connOptions.nMaxAddnode = MAX_ADDNODE_CONNECTIONS; | ||
| connOptions.nMaxFeeler = MAX_FEELER_CONNECTIONS; | ||
| connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.m_max_automatic_connections / 2); | ||
| connOptions.m_max_automatic_connections = nMaxConnections; |
There was a problem hiding this comment.
🔴 Blocking: PR title advertises bitcoin#28649 but the commit is not in the branch
The PR title 'backport: Merge bitcoin#28172, 28649, 28464' claims three upstream backports, but git log origin/develop..HEAD on the head SHA (fa96c9e) shows only two merge commits (adfe38a for bitcoin#28172 and fa96c9e for bitcoin#28464). bitcoin#28649 (netbase: reliable SOCKS5 handshake via SendComplete + Sock::SendComplete Span refactor) is absent. Not a code-completeness issue for what is here, but reviewers/mergers should either drop bitcoin#28649 from the PR title/description or add the commit before merging so upstream-tracking metadata stays accurate.
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.
source: ['claude-backport-reviewer']
There was a problem hiding this comment.
Resolved in this update — PR title advertises bitcoin#28649 but the commit is not in the branch 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.
vijaydasmp
changed the title
…erals to Parse{Hash,Hex}
bb91131 doc: remove out-of-date external link in src/util/strencodings.h (Jon Atack)
7d494a4 refactor: use string_view to pass string literals to Parse{Hash,Hex} (Jon Atack)
Pull request description:
as `string_view` is optimized to be trivially copiable, whereas the current code creates a `std::string` copy at each call.
These utility methods are called by quite a few RPCs and tests, as well as by each other.
```
$ git grep "ParseHashV\|ParseHashO\|ParseHexV\|ParseHexO" | wc -l
61
```
Also remove an out-of-date external link.
ACKs for top commit:
jonatack:
Rebased per `git range-diff c9273f6 b94581a bb91131` for an include header from the merge of bitcoin#28230. Should be trivial to re-ACK.
maflcko:
lgtm ACK bb91131
ns-xvrn:
ACK bitcoin@bb91131
achow101:
ACK bb91131
brunoerg:
crACK bb91131
Tree-SHA512: 9734fe022c9e43fd93c23a917770d332dbbd3132c80a234059714c32faa6469391e59349954749fc86c4ef0b18d5fd99bf8f4b7b82d9f799943799c1253272ae
df69b22 doc: improve documentation around connection limit maximums (Amiti Uttarwar) adc171e scripted-diff: Rename connection limit variables (Amiti Uttarwar) e9fd9c0 net: add m_max_inbound to connman (Amiti Uttarwar) c25e0e0 net, refactor: move calculations for connection type limits into connman (Amiti Uttarwar) Pull request description: This is joint work with amitiuttarwar. This has the first few commits of bitcoin#28463. It is not strictly a prerequisite for that, but has changes that in our opinion make sense on their own. It improves the handling of maximum numbers for different connection types (that are set during init and don’t change after) by: * moving all calculations into one place, `CConnMan::Init()`. Before, they were dispersed between `Init`, `CConnman::Init` and other parts of `CConnman`, resulting in some duplicated test code. * removing the possibility of having a negative maximum of inbound connections, which is hard to argue about * renaming of variables and doc improvements ACKs for top commit: amitiuttarwar: co-author review ACK df69b22 naumenkogs: ACK df69b22 achow101: ACK df69b22 Tree-SHA512: 913d56136bc1df739978de50db67302f88bac2a9d34748ae96763288d97093e998fc0f94f9b6eff12867712d7e86225af6128f4170bf2b5b8ab76f024870a22c
There was a problem hiding this comment.
Code Review
Cumulative re-review for 062b3c5c40d5816b71a84199c7712448e074497a after the push from fa96c9e4. The title cleanup is fixed, and the latest delta did not introduce new findings, but the two substantive bitcoin#28464 carry-forward issues remain in the current head.
Source: reviewers = claude/opus + codex/gpt-5.5 for general, dash-core-commit-history, and backport-reviewer; verifier = claude/opus; policy gate = review_policy.enforce_backport_prereq_policy.
Carried-forward prior findings still valid
- [blocking]
src/init.cpp:2622-2623—m_max_outbound_onionis still computed beforem_max_automatic_connectionsis assigned. The current code readsconnOptions.m_max_automatic_connectionsfor the onion outbound target at line 2622, then assignsconnOptions.m_max_automatic_connections = nMaxConnectionsat line 2623. SinceCConnman::Optionsdefault-initializes this member to 0, Dash's onion outbound preference is still disabled unless the assignment is moved before the onion calculation. - [nitpick / policy-carried]
src/net.h:1862— the comment still references the pre-renamem_max_outboundmember after the bitcoin#28464 rename. The field is nowm_max_automatic_outboundat line 1867, so the comment should say it does not increasem_max_automatic_outbound.
Prior findings fixed at this head
- prior-4 / prior-6: the PR title no longer advertises bitcoin#28649; it now names only bitcoin#28172 and bitcoin#28464, matching the actual stack.
New findings in the latest delta: none beyond the carried-forward items above.
3 participants
Bitcoin Backporting work