perf: reuse compact filter file handles - #7574
Conversation
|
✅ Final review complete — no blockers (commit 51f4049) |
WalkthroughThe index now deserializes filters through a helper that accepts an open file. Range lookups reuse files and seek between filter positions. A new benchmark suite builds a test chain, synchronizes a Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Benchmark
participant BlockFilterIndex
participant AutoFile
Benchmark->>BlockFilterIndex: request filter range
BlockFilterIndex->>AutoFile: open or seek filter file
BlockFilterIndex->>BlockFilterIndex: deserialize filter
BlockFilterIndex-->>Benchmark: return filters
Benchmark->>Benchmark: serialize filters and measure work
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
End-to-end testnet sync benchmark using the current clean Setup
Raw full-sync times
Aggregates over runs 2–4
The unchanged block-header milestone and the large reduction in later compact-filter sync work make this a strong end-to-end signal for the range-read optimization. On this fixed local testnet dataset, the PR reduced overall rust-dashcore full-sync time by about 42%. 🤖 Posted autonomously by Codex on behalf of pasta. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The compact-filter file-handle reuse is correct, but the new Dash-authored benchmark is missing from the Dash-only cppcheck inventory. The two corrective follow-up commits should also be folded into the commit that introduced the affected implementation and benchmarks; no blocking correctness issue remains at the reviewed head.
Source: reviewer backend model gpt-5.6-sol (Codex general and dash-core-commit-history); final verifier backend model gpt-5.6-sol (Codex); orchestration-only openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 3 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/bench/blockfilter_index.cpp`:
- [SUGGESTION] src/bench/blockfilter_index.cpp:1: Register the new Dash-specific benchmark for cppcheck
This newly authored Dash benchmark has no counterpart in `bitcoin/master`, but `test/util/data/non-backported.txt` does not include it. `lint-cppcheck-dash.py` constructs its entire analysis set from that inventory, and `git ls-files` with the current patterns omits this file. Add `src/bench/blockfilter_index.cpp` to `test/util/data/non-backported.txt` so the additional Dash-only static analysis covers it.
- [SUGGESTION] src/bench/blockfilter_index.cpp:103: Fold the benchmark corrections into the benchmark introduction
Commit `2032dee1230` revises benchmarks introduced by `d934d2a885e`, replacing their shared static fixture with isolated per-benchmark setup and changing all six registrations from HIGH to LOW priority. These are corrections to the benchmarks' initial lifecycle and execution policy rather than an independent feature. Fold this commit into `d934d2a885e` so the benchmarks enter history in their intended form.
In `src/index/blockfilterindex.cpp`:
- [SUGGESTION] src/index/blockfilterindex.cpp:473: Squash the compact-filter guard into its introducing commit
Commit `4ba4242d799` corrects the reuse loop introduced immediately beforehand by `d934d2a885e`. `FlatFilePos::nFile` defaults to `-1`, so the original condition could match the initial `file_num{-1}` and enter the seek branch before `file` was opened; the added `!file` guard belongs to the correctness of the introduced loop. Fold this one-line correction into `d934d2a885e` so the implementation is complete at every commit boundary.
| @@ -0,0 +1,184 @@ | |||
| // Copyright (c) 2026 The Dash Core developers | |||
There was a problem hiding this comment.
🟡 Suggestion: Register the new Dash-specific benchmark for cppcheck
This newly authored Dash benchmark has no counterpart in bitcoin/master, but test/util/data/non-backported.txt does not include it. lint-cppcheck-dash.py constructs its entire analysis set from that inventory, and git ls-files with the current patterns omits this file. Add src/bench/blockfilter_index.cpp to test/util/data/non-backported.txt so the additional Dash-only static analysis covers it.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Register the new Dash-specific benchmark for cppcheck 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.
|
|
||
| filters_out.resize(entries.size()); | ||
| auto filter_pos_it = filters_out.begin(); | ||
| std::unique_ptr<AutoFile> file; |
There was a problem hiding this comment.
🟡 Suggestion: Squash the compact-filter guard into its introducing commit
Commit 4ba4242d799 corrects the reuse loop introduced immediately beforehand by d934d2a885e. FlatFilePos::nFile defaults to -1, so the original condition could match the initial file_num{-1} and enter the seek branch before file was opened; the added !file guard belongs to the correctness of the introduced loop. Fold this one-line correction into d934d2a885e so the implementation is complete at every commit boundary.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Squash the compact-filter guard into its introducing commit 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.
| } | ||
| return messages; | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion: Fold the benchmark corrections into the benchmark introduction
Commit 2032dee1230 revises benchmarks introduced by d934d2a885e, replacing their shared static fixture with isolated per-benchmark setup and changing all six registrations from HIGH to LOW priority. These are corrections to the benchmarks' initial lifecycle and execution policy rather than an independent feature. Fold this commit into d934d2a885e so the benchmarks enter history in their intended form.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Fold the benchmark corrections into the benchmark introduction 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.
2032dee to
51f4049
Compare
PastaPastaPasta
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
At exact head 51f4049, the compact-filter range lookup correctly reuses an open file, seeks before noncontiguous reads, changes handles across files, and preserves checksum verification and deserialization. All three prior suggestions are fixed in the single current commit: the benchmark is in the Dash cppcheck inventory, the unopened-file guard is present, and the benchmark setup and priorities are corrected; no in-scope defects remain.
Source: Codex reviewer backend gpt-5.6-sol (general) and gpt-5.6-sol (dash-core-commit-history); final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol was orchestration-only and is not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
PastaPastaPasta
added
the
backport-candidate-23.1.x
label
ChatGPT Codex Connector
4 participants
Issue being fixed or feature implemented
Serving a normal BIP157
getcfiltersrange causedBlockFilterIndex::LookupFilterRangeto open and close the compact-filter flat file once per filter. A 1,000-filter request therefore performed up to 1,000 file-open operations even when the filters were contiguous in the same underlying file.This adds avoidable filesystem overhead to mobile/SPV sync, where peers commonly request long contiguous ranges.
What was done?
AutoFileopen while reading consecutive filters from the same flat file.BlockFilterconstruction in a sharedReadFilterFromFilehelper.The wire protocol and individual
cfiltermessages are unchanged.How Has This Been Tested?
Tested on Apple Silicon using a depends-based release build:
makebuildmake checktest/functional/test_runner.py p2p_blockfilters.py rpc_getblockfilter.pytest/lint/lint-whitespace.pytest/lint/lint-circular-dependencies.pyIn-tree benchmark
For a contiguous 1,000-filter range containing approximately 2.64 MB of encoded filters:
The current branch rerun measured approximately 180,000 filters/s for the 1,000-filter range.
End-to-end sync measurement note
Exploratory rust-dashcore testnet sync runs confirmed that this path is exercised during full SPV sync, but their absolute before/after timings are not presented as proof here. The runs were not sufficiently controlled for server warm-cache state and background compact-filter-index I/O, so cross-condition timing differences were confounded. The deterministic in-tree benchmark above isolates the changed lookup path.
Breaking Changes
None.
Checklist:
This pull request was created by Codex.