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! |
|
⛔ Blockers found — Opus deferred (commit b6412af) |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR consolidates CConnman connection limits into Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
📥 Commits
Reviewing files that changed from the base of the PR and between d1eab6c and fa96c9e4c8112928d0904e663ba25303a2d9d75c.
📒 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
![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.
Probably this line should be removed from init.cpp
-connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.m_max_automatic_connections / 2);and added in net.h along these:
m_max_automatic_connections = connOptions.m_max_automatic_connections;
m_max_outbound_full_relay = std::min(MAX_OUTBOUND_FULL_RELAY_CONNECTIONS, m_max_automatic_connections);
m_max_outbound_block_relay = std::min(MAX_BLOCK_RELAY_ONLY_CONNECTIONS, m_max_automatic_connections - m_max_outbound_full_relay);
m_max_automatic_outbound = m_max_outbound_full_relay + m_max_outbound_block_relay + m_max_feeler;
m_max_inbound = std::max(0, m_max_automatic_connections - m_max_automatic_outbound);
+ m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, m_max_automatic_connections / 2);
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: adfe38a4f4 (bitcoin#28172) and fa96c9e4c8 (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 (fa96c9e4c8) shows only two merge commits (adfe38a4f4 for bitcoin#28172 and fa96c9e4c8 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']
There was a problem hiding this comment.
Resolved in this update — Comment references pre-rename member m_max_outbound 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.
vijaydasmp
changed the title
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.
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
28172 - OK
28464 requires changes
| #include <string> | ||
| #include <type_traits> | ||
| #include <utility> | ||
| #include <string_view> |
There was a problem hiding this comment.
nit:
#include <string>
+#include <string_view>
#include <type_traits>
#include <utility>
-#include <string_view>| 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.
Probably this line should be removed from init.cpp
-connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.m_max_automatic_connections / 2);and added in net.h along these:
m_max_automatic_connections = connOptions.m_max_automatic_connections;
m_max_outbound_full_relay = std::min(MAX_OUTBOUND_FULL_RELAY_CONNECTIONS, m_max_automatic_connections);
m_max_outbound_block_relay = std::min(MAX_BLOCK_RELAY_ONLY_CONNECTIONS, m_max_automatic_connections - m_max_outbound_full_relay);
m_max_automatic_outbound = m_max_outbound_full_relay + m_max_outbound_block_relay + m_max_feeler;
m_max_inbound = std::max(0, m_max_automatic_connections - m_max_automatic_outbound);
+ m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, m_max_automatic_connections / 2);
There was a problem hiding this comment.
💡 Codex Review
Line 1272 in bb29ffc
Clamp the post-reservation connection limit
On Unix nodes whose RLIMIT_NOFILE remains between 151 and 158 after RaiseFileDescriptorLimit (MIN_CORE_FILEDESCRIPTORS is 150 and this patch reserves 8 addnode FDs plus the message-capture FD), the clamp above sets nMaxConnections to 0, but this second min turns it negative. That negative value is passed to CConnman::Init(), producing negative outbound limits and a negative semaphore size; Interrupt() then posts zero grants because the loop bound is negative, so the open-connections thread can remain blocked during shutdown. Clamp this result to 0 like the previous calculation.
ℹ️ 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".
| nMaxConnections = connOptions.nMaxConnections; | ||
| m_max_outbound_full_relay = std::min(connOptions.m_max_outbound_full_relay, connOptions.nMaxConnections); | ||
| m_max_outbound_block_relay = connOptions.m_max_outbound_block_relay; | ||
| m_max_automatic_connections = connOptions.m_max_automatic_connections; |
There was a problem hiding this comment.
When this newly-derived automatic limit is set, m_max_outbound_onion is still copied from Options unchanged later, and AppInit now only assigns connOptions.m_max_automatic_connections = nMaxConnections; fresh evidence in this revision is that Options::m_max_outbound_onion still defaults to 0 and is never derived from this value. With Tor reachable, the onion-only branch in ThreadOpenConnections() and the onion eviction protection both see a zero budget, so the default up-to-two onion outbound peers are never opened or protected.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head bb29ffc, the latest delta removes the old read-before-assignment sequence but also removes Dash's only onion outbound-limit calculation, so the production option remains defaulted to zero and the same functional regression persists through a new omission mechanism. The stale renamed-member comment is carried forward, while no additional in-scope issue was confirmed. The descriptor-clamp edge case is valid but byte-identical to the merge base and both sides of upstream bitcoin#28464, so it is outside this PR.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (failed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
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/init.cpp`:
- [BLOCKING] src/init.cpp:2622: bitcoin#28464 merge resolution drops Dash's onion outbound-limit calculation
New in the latest delta, superseding the assignment-order mechanism reported by prior-1, prior-2, and prior-4. Before this backport, `AppInitMain()` assigned `connOptions.nMaxConnections` and then derived `m_max_outbound_onion` as `min(MAX_DESIRED_ONION_CONNECTIONS, nMaxConnections / 2)`. Upstream bitcoin#28464 moved its per-type connection-limit calculations into `CConnman::Init()`, and the current merge resolution removes Dash's additional onion calculation without moving it there. `CConnman::Options::m_max_outbound_onion` consequently remains at its default zero in `src/net.h:1201`, and `CConnman::Init()` copies that zero unchanged at line 1236. There are no other assignments in the tree, so the onion-preferred branch in `ThreadOpenConnections()` cannot request a peer and the Tor-peer eviction protection receives a limit of zero. Initialize the onion budget from the automatic connection limit, preferably alongside the other derived limits in `CConnman::Init()`.
| 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_automatic_connections = nMaxConnections; |
There was a problem hiding this comment.
🔴 Blocking: bitcoin#28464 merge resolution drops Dash's onion outbound-limit calculation
New in the latest delta, superseding the assignment-order mechanism reported by prior-1, prior-2, and prior-4. Before this backport, AppInitMain() assigned connOptions.nMaxConnections and then derived m_max_outbound_onion as min(MAX_DESIRED_ONION_CONNECTIONS, nMaxConnections / 2). Upstream bitcoin#28464 moved its per-type connection-limit calculations into CConnman::Init(), and the current merge resolution removes Dash's additional onion calculation without moving it there. CConnman::Options::m_max_outbound_onion consequently remains at its default zero in src/net.h:1201, and CConnman::Init() copies that zero unchanged at line 1236. There are no other assignments in the tree, so the onion-preferred branch in ThreadOpenConnections() cannot request a peer and the Tor-peer eviction protection receives a limit of zero. Initialize the onion budget from the automatic connection limit, preferably alongside the other derived limits in CConnman::Init().
| 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: ['codex']
There was a problem hiding this comment.
Resolved in this update — bitcoin#28464 merge resolution drops Dash's onion outbound-limit calculation 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.
|
This pull request has conflicts, please rebase. |
…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
bb29ffc to
25cb4de
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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
25cb4de to
b6412af
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The bitcoin#28172 string_view backport appears correctly adapted, but bitcoin#28464 still removes Dash's production initialization of the onion outbound budget, disabling onion-preferred connection attempts and onion eviction protection. The previously stale connection-member comment is fixed at this head.
Source: reviewers: gpt-5.6-sol (general), gpt-5.6-sol (dash-core-commit-history), and gpt-5.6-sol (backport-reviewer); final verifier: gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol was orchestration-only and is not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 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/net.h`:
- [BLOCKING] src/net.h:1256: bitcoin#28464 merge resolution drops Dash's onion outbound-limit calculation
Before this backport, `AppInitMain()` derived `m_max_outbound_onion` as `min(MAX_DESIRED_ONION_CONNECTIONS, nMaxConnections / 2)`. The backport now initializes only `connOptions.m_max_automatic_connections`, while `Options::m_max_outbound_onion` remains at its default zero and this line copies that zero into `CConnman`. No other production assignment exists. Consequently, the onion-preferred branch in `ThreadOpenConnections()` can never request an onion peer, and `GetMaxOutboundOnionNodeCount()` returns zero to the outbound eviction logic instead of preserving up to two onion peers. Derive the Dash-specific onion budget from `m_max_automatic_connections` alongside the other connection limits in `CConnman::Init()`.
| m_max_outbound_block_relay = std::min(MAX_BLOCK_RELAY_ONLY_CONNECTIONS, m_max_automatic_connections - m_max_outbound_full_relay); | ||
| m_max_automatic_outbound = m_max_outbound_full_relay + m_max_outbound_block_relay + m_max_feeler; | ||
| m_max_inbound = std::max(0, m_max_automatic_connections - m_max_automatic_outbound); | ||
| m_max_outbound_onion = connOptions.m_max_outbound_onion; |
There was a problem hiding this comment.
🔴 Blocking: bitcoin#28464 merge resolution drops Dash's onion outbound-limit calculation
Before this backport, AppInitMain() derived m_max_outbound_onion as min(MAX_DESIRED_ONION_CONNECTIONS, nMaxConnections / 2). The backport now initializes only connOptions.m_max_automatic_connections, while Options::m_max_outbound_onion remains at its default zero and this line copies that zero into CConnman. No other production assignment exists. Consequently, the onion-preferred branch in ThreadOpenConnections() can never request an onion peer, and GetMaxOutboundOnionNodeCount() returns zero to the outbound eviction logic instead of preserving up to two onion peers. Derive the Dash-specific onion budget from m_max_automatic_connections alongside the other connection limits in CConnman::Init().
| m_max_outbound_onion = connOptions.m_max_outbound_onion; | |
| m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, m_max_automatic_connections / 2); |
source: ['codex']
4 participants
Bitcoin Backporting work