backport: Merge bitcoin/bitcoin#26326: net: don't lock cs_main while reading blocks#7350
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
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:
WalkthroughThis PR adds a compile-time check that Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Peer
participant NetProcessing
participant Chainstate
participant Disk
Peer->>NetProcessing: request block data
NetProcessing->>Chainstate: get tip, block position, direct-fetch state
NetProcessing->>Disk: ReadBlockFromDisk(block_pos)
Disk-->>NetProcessing: block data
NetProcessing->>NetProcessing: compare returned hash to requested hash
NetProcessing-->>Peer: serve block response or disconnect
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
thepastaclaw
force-pushed
the
fix/getblocktxn-recent-disk-read
branch
from
6440dea to
97af883
Compare
|
✅ Review complete (commit bd3446f) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Targeted change narrows the cs_main hold window in GETBLOCKTXN by capturing the block file position under the lock and performing the disk read outside it, with a hash check that fails closed on stale/pruned/mismatched reads. Both agents concur the change is correct; only a minor log-granularity nitpick remains.
💬 1 nitpick(s)
|
Is this not code we inherited from bitcoin? it is rare we want to accept a "fix" in their code. |
|
Good question. I checked this against upstream Bitcoin: the change is actually bringing this path back in line with the current inherited Bitcoin pattern, not inventing a Dash-only behavior. Bitcoin master already copies the recent block position under So I agree with the general rule — we should be cautious about “fixing” inherited Bitcoin code — but in this case the safer path is to follow the upstream Bitcoin fix rather than keep Dash on the older wider-lock version. |
Hmm; it seems then, that there may be a backport that we should backport instead of doing this adhoc? |
|
Yep — I traced it more precisely and there is an exact upstream Bitcoin commit for this:
That commit adds the same So I agree with your process concern: this should be treated as a Bitcoin backport, not as an ad-hoc Dash-only fix. If you prefer, I can close this PR and re-run it through the normal backport flow; otherwise I can update the PR metadata/commit message to explicitly cite the upstream commit. |
|
This pull request has conflicts, please rebase. |
thepastaclaw
force-pushed
the
fix/getblocktxn-recent-disk-read
branch
from
97af883 to
c6ccbc6
Compare
|
This pull request has conflicts, please rebase. |
|
Rebased this onto current |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at c6ccbc6: the GETBLOCKTXN lock-scope reduction is correct — FlatFilePos is captured under cs_main, disk I/O runs lock-free, and the post-read hash check fails closed on stale/mismatched reads. No new in-scope defects in the latest delta. Carrying forward the prior nitpick about the log message conflating read failure with hash mismatch (already tracked in an unresolved inline thread).
💬 1 nitpick(s)
Note: GitHub does not allow me to submit APPROVE on my own PR, so this is posted as a COMMENT while preserving the verified non-blocking findings.
| if (!hash || *hash != req.blockhash) { | ||
| LogPrint(BCLog::NET, "Peer %d sent us a getblocktxn for a block we could not read\n", pfrom.GetId()); | ||
| return; |
There was a problem hiding this comment.
💬 Nitpick: Log line conflates disk read failure with hash mismatch
After cs_main is released, two operationally distinct failures share one log line: ReadBlockFromDisk returning std::nullopt (OpenBlockFile failure, deserialization error, PoW header check failure — typically pruning or corruption) and a successful read whose *hash != req.blockhash (the FlatFilePos captured under cs_main no longer points at the requested block, e.g. a race with reorg/prune/reindex in the very window this PR introduces). Splitting these into two messages would make the new lock-scope reduction easier to diagnose if it surfaces in the wild. Purely diagnostic quality; behavior is correct. Existing unresolved thread: #7350 (comment).
source: ['claude', 'codex']
|
Claw; please update this to actually be a backport, if it should so be instead of an adhoc change that doesn't accomplish / get shown in history as a backport |
thepastaclaw
force-pushed
the
fix/getblocktxn-recent-disk-read
branch
from
c6ccbc6 to
037d040
Compare
thepastaclaw
changed the title
|
Updated. I amended the PR head so the commit history now records this as a partial backport of bitcoin#26326, specifically bitcoin/bitcoin@613a45c ( |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 037d040. Source tree is unchanged from c6ccbc6; the push only rewrote commit metadata into proper Dash partial-backport form. The GETBLOCKTXN lock-scope reduction is correct: FlatFilePos captured under cs_main, disk read runs lock-free, post-read hash check fails closed on mismatch/missing/pruned. Only carried-forward finding is a low-confidence log-diagnostic nitpick about line 4496 conflating two distinct failure modes.
💬 1 nitpick(s)
Carried-forward findings already raised (1)
These findings were not re-posted as new inline comments because an existing review thread already covers them.
- [NITPICK] (deduped existing open thread)
src/net_processing.cpp:4495-4497: Log message conflates I/O failure with hash mismatch — Two operationally distinct failures share one log message:ReadBlockFromDiskreturningstd::nullopt(OpenBlockFile failure, deserialization error, PoW header check failure — typically pruning/corruption) and a successful read whose*hash != req.blockhash(the FlatFilePos captured under cs_m...
Note: GitHub does not allow me to approve my own PR, so this is posted as a COMMENT while preserving the verified nonblocking finding.
|
@PastaPastaPasta @UdjinM6 this is ready for human Core review. The PR history/title/body now identify it as a partial backport of bitcoin#26326, the branch is mergeable against |
…blocks 613a45c net: reduce LOCK(cs_main) scope in GETBLOCKTXN (Andrew Toth) 75d27fe net: reduce LOCK(cs_main) scope in ProcessGetBlockData (Andrew Toth) Pull request description: Inspired by bitcoin#11913 and bitcoin#26308. cs_main doesn't need to be locked while reading blocks. This removes the locks in net_processing. Dash notes: This backports bitcoin#26326. The upstream witness-block raw-data fast path is not present in Dash, so the ProcessGetBlockData adaptation applies to Dash's existing full, filtered, and compact block serving path.
thepastaclaw
force-pushed
the
fix/getblocktxn-recent-disk-read
branch
from
037d040 to
bd3446f
Compare
thepastaclaw
changed the title
|
Updated this into a full backport of bitcoin#26326 instead of the prior partial GETBLOCKTXN-only form. The branch now includes both upstream commits: 613a45c (GETBLOCKTXN) and 75d27fe (ProcessGetBlockData). GETBLOCKTXN now preserves the upstream assert invariant; the log/disconnect behavior is only in the sibling ProcessGetBlockData commit where upstream intentionally made that behavior change. Validation: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewer lanes attempted: opus general/commit-history/backport-reviewer all failed with extra-usage limit (resets Jul 10, 8am America/Chicago); gpt-5.5 general, gpt-5.5 dash-core-commit-history, and gpt-5.5 backport-reviewer completed. Verifier: gpt-5.5 after opus verifier failed with the same usage limit.
Prior reconciliation: the prior 037d040a nitpick about GETBLOCKTXN logging is FIXED at bd3446f6; the shared log branch was removed and the current path asserts hash && *hash == req.blockhash after the captured-position read.
Carried-forward prior findings: none.
New findings in latest delta: one backport-prerequisite finding was reported by the backport reviewer and promoted by the deterministic policy gate. The verifier otherwise found no general correctness or commit-history issues.
Note: posted as a COMMENT review because GitHub does not allow me to submit REQUEST_CHANGES on my own PR; the verified blocking finding is preserved below.
🔴 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_processing.cpp`:
- [BLOCKING] src/net_processing.cpp:2745-2747: Missing prerequisite: bitcoin#27125
Upstream bitcoin#26326's starting state already had bitcoin#27125's BlockManager disk-read methods, and the upstream changed lines call `m_chainman.m_blockman.ReadBlockFromDisk(*pblockRead, block_pos)` in ProcessGetBlockData and GETBLOCKTXN. Dash's starting state does not have that API; it still exposes namespace-level `node::ReadBlockFromDisk(CBlock&, FlatFilePos, Consensus::Params)` and this backport adapts the changed lines to that older helper. The missing upstream API was introduced by f0bb1021f0 in merge c2f2abd0a4 (`Merge bitcoin/bitcoin#27125: refactor, kernel: Decouple ArgsManager from blockstorage`), which is not an ancestor of this Dash head. This is a soft prerequisite gap: Dash has an equivalent local helper that returns the read block hash, so the conflict resolution looks correct and should preserve the intended post-lock disk-read validation, but the upstream dependency chain is incomplete relative to the modified section.
---
**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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Follow-up on my self-review note at #7350 (review): Handled by documenting the prerequisite decision in the PR body under "Backport prerequisite notes":
The reason is that upstream bitcoin#26326 uses the BlockManager No branch/code change needed for this follow-up; current CI is green. |
|
CI triage note for the old linux64_ubsan-test failure in run 28288653526: the red test was feature_asset_locks.py timing out while waiting for quorum list propagation in test_v24_fork / mine_quorum_2_nodes. The failing PR head c6ccbc6 changed only src/net_processing.cpp, so the failure was unrelated to this PR's GETBLOCKTXN/block-serving backport. I added this occurrence to the existing flaky-test issue #7310. The later final PR head bd3446f passed linux64_ubsan-test, and the PR is now merged, so no branch change is needed. |
4 participants
backport: Merge bitcoin#26326
Upstream backport
This PR backports bitcoin#26326:
net: reduce LOCK(cs_main) scope in GETBLOCKTXN)net: reduce LOCK(cs_main) scope in ProcessGetBlockData)Issue being fixed or feature implemented
Block-serving paths in
net_processingwere doing disk reads while holdingcs_main. A peer can request blocks or block transactions repeatedly, so disk I/O should not extend the validation/global chain lock hold time.What was done?
cs_main.cs_mainbefore reading the block from disk.GETBLOCKTXNinvariant that recent blocks cannot be pruned before the post-lock disk read.ProcessGetBlockDatabehavior: if the post-lock disk read fails because the block was pruned or cannot be loaded, log the condition, disconnect the peer, and return instead of asserting.CanDirectFetch()and the current tip while holdingcs_mainand use those cached values after the disk read.Dash note: upstream Bitcoin also has a witness-block raw-data fast path in
ProcessGetBlockData; Dash does not have the correspondingMSG_WITNESS_BLOCKpath here, so this backport adapts the shared full, filtered, and compact block-serving path.Backport prerequisite notes
intentional_exclusion: bitcoin/bitcoin#27125— upstream net: don't lock cs_main while reading blocks in net processing bitcoin/bitcoin#26326's changed lines use theBlockManager::ReadBlockFromDisk(..., FlatFilePos)API introduced by refactor, kernel: Decouple ArgsManager from blockstorage bitcoin/bitcoin#27125. Dash does not have that broader blockstorage refactor, but it already has the equivalent localnode::ReadBlockFromDisk(CBlock&, FlatFilePos, Consensus::Params)helper, which returns the read block hash. This backport intentionally adapts the changed lines to Dash's existing helper and keeps the post-lock hash validation instead of importing refactor, kernel: Decouple ArgsManager from blockstorage bitcoin/bitcoin#27125 as a prerequisite.How Has This Been Tested?
Tested on macOS arm64.
git diff --check upstream/develop...HEADtest/lint/lint-whitespace.py./autogen.sh./configure --without-gui --disable-bench --disable-fuzz-binarymake -j$(sysctl -n hw.ncpu) src/dashdBreaking Changes
None.
Checklist