Repository navigation
backport: bitcoin#26140, bitcoin#27482 - #7923
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughPer-peer header state and transaction-announcement tracking move from Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 30.77% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 6 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly identifies the pull request as a backport of Bitcoin Core changes 26140 and 27482. Description check The description directly explains the two backported changes, testing performed, verification results, and known caveats.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
|
✅ Final review complete — no blockers (commit 290ab76) · triage: normal |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Static verification of fad547f..478fd11 against the supplied reviews, upstream patches, and Dash-specific callers found no actionable issues. The networking state migration preserves compressed headers, DSTX handling, and asset-unlock retrieval; documentation adaptations and intentional upstream omissions are supported by source and commit history, and the overlapping base changes do not supersede this work. No builds or tests were run locally; diff whitespace validation passed, current CI lint and ClangFormat checks passed, and source-build checks remain pending with no failed checks reported.
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The changes refactor peer-state ownership and locking in src/net_processing.cpp alongside test and documentation updates, requiring substantive review but not directly changing consensus, funds handling, cryptography, network deserialization, or storage migrations. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
thepastaclaw
added
the
pastaclaw:approved
| std::chrono::microseconds m_headers_sync_timeout GUARDED_BY(NetEventsInterface::g_msgproc_mutex){0us}; | ||
|
|
||
| /** Whether this peer wants invs or headers (when possible) for block announcements */ | ||
| bool m_prefers_headers GUARDED_BY(NetEventsInterface::g_msgproc_mutex){false}; |
There was a problem hiding this comment.
m_prefers_headers is backported, but state.fPreferHeadersCompressed is still here.
There was a problem hiding this comment.
I tried a fix but it failed local gates in bitcoin#27379 (verify: Removal of the const TxRelay* GetTxRelay() const overload from struct Peer; Dash keeps it because its own IsInvInFilter(const Peer&, ...) still calls peer.GetTxRelay() on a const Peer; Upstream bitcoin#27379 only passes TxRelay to FindTxForGetData, drops the cs_main lock there, and puts mapRelay/g_relay_expiration under g_msgproc_mutex. It never touches header-announcement preferences. This move). bitcoin#27379 has had its repair lane and is being dropped from this branch.
🤖 backportsys, on behalf of the Dash backport pipeline.
There was a problem hiding this comment.
can you try to fix fPreferHeadersCompressed and 27379 at once?
There was a problem hiding this comment.
Moved the Dash-only CNodeState::fPreferHeadersCompressed into Peer as m_prefers_headers_compressed (GUARDED_BY g_msgproc_mutex), next to m_prefers_headers, to match bitcoin#26140's fPreferHeaders move. The SENDHEADERS2 handler no longer takes cs_main, and SendMessages reads the Peer field. bitcoin#27379 (bitcoin#26140 follow-ups: FindTxForGetData/mapRelay locking) is a separate upstream PR, so it should come in as its own backport commit instead of being folded into the bitcoin#26140 commit; nothing on this branch blocks it. (folded into the bitcoin#26140 commit)
🤖 backportsys, on behalf of the Dash backport pipeline.
| /** Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). */ | ||
| CTransactionRef FindTxForGetData(const Peer& peer, const uint256& txid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) | ||
| LOCKS_EXCLUDED(cs_main) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex); | ||
| CTransactionRef FindTxForGetData(const Peer::TxRelay& tx_relay, const uint256& txid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) |
There was a problem hiding this comment.
27379:
Removal of const Peer::GetTxRelay() overload not taken: Dash-only IsInvInFilter(const Peer&) still uses it, so it is not dead code in Dash
At least mark backport as partial, but better refactor IsInvInFilter and its usages to complete backport
There was a problem hiding this comment.
This branch does not backport bitcoin#27379; it only contains bitcoin#26140, bitcoin#27482 and bitcoin#15765. bitcoin#26140 upstream adds a const GetTxRelay() overload, which Dash already had, so the backport keeps it and removes nothing. Removing the overload and refactoring IsInvInFilter belongs in the bitcoin#27379 backport, and that is where it should be marked partial.
🤖 backportsys, on behalf of the Dash backport pipeline.
| #### Version | ||
|
|
||
| `dashconsensus_version` returns an `unsigned int` with the API version *(currently at an experimental `0`)*. | ||
| `dashconsensus_version` returns an `unsigned int` with the API version *(currently `0`)*. |
There was a problem hiding this comment.
15765: marked as DNM, see https://github.com/dashpay/dash//pull/4555#discussion_r751159861 for reference.
Any reason why it is backported again?
There was a problem hiding this comment.
Agreed: bitcoin#15765 was marked DNM in #4555 (we still use API version 0), and this version only changes a double space and the word 'experimental'. Dropping it from this branch. Dropping bitcoin#15765 from this branch; it is being rewritten without it.
🤖 backportsys, on behalf of the Dash backport pipeline.
478fd11 to
155d55b
Compare
thepastaclaw
removed
the
pastaclaw:approved
DCG-Claude
changed the title
|
Branch rewritten at
🤖 backportsys, on behalf of the Dash backport pipeline. |
155d55b to
63b98da
Compare
DCG-Claude
changed the title
|
Branch rewritten at
🤖 backportsys, on behalf of the Dash backport pipeline. |
![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: 63b98da2d5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * [ ] Write release notes (see below). To clear the release notes: `cp doc/release-notes-empty-template.md doc/release-notes.md` | ||
| * [ ] Update flatpak [metainfo file](contrib/flatpak/org.dash.dash-core.metainfo.xml) with latest release tag and estimated release date | ||
| * [ ] Update the following variables in [`src/chainparams.cpp`](/src/chainparams.cpp) for mainnet and testnet: | ||
| * [ ] Update the following variables in [`src/kernel/chainparams.cpp`](/src/kernel/chainparams.cpp) for mainnet and testnet: |
There was a problem hiding this comment.
The complete parent-to-commit diff contains no doc/shared-libraries.md hunk even though this backport's metadata says bitcoin#15765's fix to that file landed. A repo-wide check still finds the old located in ... and currently at an experimental ... text, so recording this merge would make the backport queue and review history treat bitcoin#15765 as complete while its advertised change is absent; include the generated 155d55b5cc change in the final tree, or remove bitcoin#15765 from the subject/table so it can be queued separately.
AGENTS.md reference: AGENTS.md:L197-L198
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This branch only carries bitcoin#26140 and bitcoin#27482. Neither commit message mentions bitcoin#15765, and the branch never touches doc/shared-libraries.md, so it doesn't claim bitcoin#15765 is done and nothing is missing here. The finding looks like it belongs to a different backport.
🤖 backportsys, on behalf of the Dash backport pipeline.
…sgproc_mutex to Peer 3a060ae scripted-diff: Rename nUnconnectingHeaders and fPreferHeaders (dergoegge) 279c53d [net processing] Move m_recently_announced_invs from CNodeState to Peer (dergoegge) 938a8e2 [net processing] Annotate m_recently_announced_invs as guarded by g_msgproc_mutex (dergoegge) 8a2cb1f [net processing] Move fPreferHeaders from CNodeState to Peer (dergoegge) 3605011 [net processing] Annotate fPreferHeaders as guarded by g_msgproc_mutex (dergoegge) 4b84e50 [net processing] Move m_headers_sync_timeout from CNodeState to Peer (dergoegge) 689b747 [net processing] Annotate m_headers_sync_timeout as guarded by g_msgproc_mutex (dergoegge) d8c0d1c [net processing] Move nUnconnectingHeaders from CNodeState to Peer (dergoegge) 5f80d8d [net processing] Annotate nUnconnectingHeaders as guarded by g_msgproc_mutex (dergoegge) 1d87137 [validation] Annotate ChainstateManager::m_best_header as guarded by cs_main (dergoegge) Pull request description: `nUnconnectingHeaders`, `m_headers_sync_timeout`, `fPreferHeaders` and `m_recently_announced_headers` are currently all `CNodeState` members even though they are only ever accessed from the message processing thread (therefore sufficiently guarded exclusively by `g_msgproc_mutex`). `CNodeState` exists purely to hold validation-specific state guarded by `cs_main` that is accessed by multiple threads. This PR adds thread-safety annotations for the above mentioned `CNodeState` members and moves them to `Peer`. ACKs for top commit: glozow: code review ACK 3a060ae, as in I am convinced these members shouldn't be guarded by cs_main and belong in Peer/TxRelay. clang checked the annotations for me. hebasto: ACK 3a060ae Tree-SHA512: 2db27c03f2c6ed36ad7dfbb4f862eeed3c3e57f845cf8abb9e7cada36f976257311892020bbcff513fbe662a881c93270e3a126946ceb0c3f94213b546bcaa81 Dash adaptations: - src/net_processing.cpp: FindTxForGetData takes (const Peer&, const uint256& txid) not GenTxid; Dash has no wtxid relay and keeps its asset-unlock instance-hash lookup - src/net_processing.cpp: upstream's new `const TxRelay* GetTxRelay() const` is dropped; Dash already has one (respecting m_can_tx_relay) and m_tx_relay_mutex is already mutable - src/net_processing.cpp: HandleFewUnconnectingHeaders keeps Dash's GETHEADERS/GETHEADERS2 choice and the msg_type in its log line, now using the cs_main-snapshotted best_header - src/net_processing.cpp: UpdatePeerStateForReceivedHeaders call keeps Dash's GetHeadersLimit(pfrom, uses_compressed) in place of MAX_HEADERS_RESULTS - src/net_processing.cpp: Dash's DSTX-aware tx announcement loop inserts announce_hash into tx_relay->m_recently_announced_invs, not the CNodeState copy - src/net_processing.cpp: Dash-only fPreferHeadersCompressed (SENDHEADERS2) and m_recent_object_requests stay in CNodeState; upstream only moves fPreferHeaders - src/net_processing.cpp: the headers-sync timeout check keeps Dash's max_tip_age condition; only state.m_headers_sync_timeout becomes peer->m_headers_sync_timeout - test/functional/p2p_sendheaders_compressed.py: same MAX_UNCONNECTING_HEADERS -> MAX_NUM_UNCONNECTING_HEADERS_MSGS rename as in Dash's fork of p2p_sendheaders.py
a2bef80 kernel: update m_assumed_* chain params for 25.x (fanquake) 4128e01 kernel: update chainTxData for 25.x (fanquake) 00b2b11 kernel: update nMinimumChainWork & defaultAssumeValid for 25.x (fanquake) 07fcc0a doc: update references to kernel/chainparams.cpp (fanquake) Pull request description: Update chainparams pre `25.x` branch off. Co-Author in the commits as a PR (bitcoin#27223) had previously been opened too-early to do the same. Note: Remember that some variance is expected in the `m_assumed_*` sizes. ACKs for top commit: achow101: ACK a2bef80 josibake: ACK bitcoin@a2bef80 gruve-p: ACK bitcoin@a2bef80 dergoegge: ACK a2bef80 on the new mainnet params Tree-SHA512: 0b19c2ef15c6b15863d6a560a1053ee223057c7bfb617ffd3400b1734cee8f75bc6fd7f04d8f8e3f5af6220659a1987951a1b36945d6fe17d06972004fd62610 Dash adaptations: - doc/release-process.md: fixes the chainparams.cpp path in both of Dash's checklist items, since Dash splits upstream's single item into minor/major-release steps - src/kernel/chainparams.cpp: keeps Dash's own nMinimumChainWork, defaultAssumeValid, m_assumed_* and chainTxData values instead of upstream's Bitcoin 25.x values Not applicable to Dash (intentionally omitted): - src/kernel/chainparams.cpp: Bitcoin 25.x chain data (main/test/signet chainwork, assumevalid, sizes, txstats); Dash has its own chain, values and release-time updates, and no signet
63b98da to
290ab76
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at 290ab76 against both upstream backports; no blocking correctness, Dash-integration, or backport-completeness defects were confirmed. One non-blocking commit-message correction remains. This was a static review: CI lint, multiple platform builds, and nowallet tests pass; ASan and multiprocess builds plus Linux, SQLite, and TSan tests remain pending, while the advisory predict_conflicts entry is cancelled rather than a code-check failure.
🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Correct the stale compressed-header adaptation note
<commit:bf546a200bb>:1
The Dash adaptations section of bf546a2 says that fPreferHeadersCompressed and m_recent_object_requests stay in CNodeState. The commit instead removes fPreferHeadersCompressed, introduces Peer::m_prefers_headers_compressed guarded by g_msgproc_mutex, and updates the SENDHEADERS2 handler and announcement-selection reads. Only m_recent_object_requests remains in CNodeState. Amend this adaptation bullet to describe the compressed-header preference move so the permanent backport record agrees with the implementation.
source: gpt-6.1-sol (phase2-reviewer: general, backport-reviewer, dash-core-commit-history)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The diff primarily relocates peer state and adjusts locking and callers in src/net_processing.cpp, including Dash compressed-header handling, without changing consensus, network deserialization, or another qualifying critical surface. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-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 `<commit:bf546a200bb>`:
- [SUGGESTION] <commit:bf546a200bb>:1: Correct the stale compressed-header adaptation note
The Dash adaptations section of bf546a200bba2879d0c65531e0f9b1dcef49a6ec says that `fPreferHeadersCompressed` and `m_recent_object_requests` stay in `CNodeState`. The commit instead removes `fPreferHeadersCompressed`, introduces `Peer::m_prefers_headers_compressed` guarded by `g_msgproc_mutex`, and updates the SENDHEADERS2 handler and announcement-selection reads. Only `m_recent_object_requests` remains in `CNodeState`. Amend this adaptation bullet to describe the compressed-header preference move so the permanent backport record agrees with the implementation.
thepastaclaw
removed
the
pastaclaw:commented
Issue being fixed or feature implemented
Backports 2 Bitcoin Core v0.26 pull request(s) that the Dash queue selected, including any discovered prerequisites: bitcoin#26140, bitcoin#27482.
What was done?
bf546a200b290ab768cbEach commit keeps the upstream subject (
partial Merge …only where a hunk is deferred to a prerequisite still to be backported, named in the commit body; a hunk Dash intentionally never wants is recorded in the commit body or the reviewer's note above and does not make the backport partial). Conflicts were resolved commit by commit; commits that needed no resolution were cherry-picked unchanged.How Has This Been Tested?
Recorded per commit, at that commit's own sha, not once for the branch:
290ab768cb, which is this branch's headGates that did not come back clean — please weigh these:
mech: warn — 13 invented line(s); 1 partial/prereq/low-risk finding(s)mech: warn — 2 invented line(s); 2 partial/prereq/low-risk finding(s)Breaking Changes
None beyond the upstream changes themselves.
Checklist:
Left for the reviewer; backportsys does not tick boxes on its own behalf.
Maintainer controls
Tick a box and backportsys acts on it within a few minutes, then clears the box. For anything else — a hunk to drop, a resolution to redo, a question — just leave a review comment; nothing here needs a box.
develop