Merge bitcoin/bitcoin#26684: bench: add readblock benchmark#1046
Merge bitcoin/bitcoin#26684: bench: add readblock benchmark#1046DashCoreAutoGuix wants to merge 208 commits into
Conversation
This change enables compact block filters (BIP157) functionality by default: - Sets DEFAULT_BLOCKFILTERINDEX to "basic" instead of "0" - Sets DEFAULT_PEERBLOCKFILTERS to true instead of false This improves privacy for light clients and enables better pruned node support.
Documents the change to enable BIP157 block filters by default.
Updates the release notes file name and PR reference from 6708 to 6711.
When blockfilterindex defaults to "basic" but no explicit command line
argument is provided, GetArgs("-blockfilterindex") returns an empty vector.
This caused the validation logic to fail with "Cannot set -peerblockfilters
without -blockfilterindex" error.
Add check for empty names vector and use the default value when needed.
Three tests needed updates to work with the new defaults where -blockfilterindex=basic and -peerblockfilters=true: - p2p_blockfilters.py: Explicitly disable peerblockfilters for node 1 which doesn't serve compact filters - rpc_getblockfilter.py: Explicitly disable both options for nodes that should not have block filter index - rpc_misc.py: Explicitly disable both options when testing the absence of indices 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…faults Node 1 was configured with only -coinstatsindex=1, but with the new defaults it was also getting -blockfilterindex=basic enabled. The test expects node 1 to only have coinstatsindex, so we need to explicitly disable blockfilterindex and peerblockfilters. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
When BIP157 filters are enabled by default, the restart_without_indices function needs to explicitly disable both blockfilterindex and coinstatsindex to avoid conflicts with pruning.
With peerblockfilters now enabled by default, the test must explicitly disable it when disabling blockfilterindex to avoid initialization errors. Co-Authored-By: Claude <noreply@anthropic.com>
…lock and related code
… RVO CFinalCommitment is a heavy object to create and copy
…eNodes' which triggers on each send / receive bytes
… member `IsMiningPhase` be anonymous function
Firstly, production code (even in debug mode) should not operate as unit test by calling heavy GetMN
just to be sure that implementation is not broken.
Secondly, since mn_rr hard-fork is activated it doesn't affect any new blocks; but it is used for
validation of old blocks only.
Thirdly, it removes useless log record (especially now when no more 4x payments for Evo Nodes).
2025-05-31T19:40:51.578629Z (mocktime: 2014-12-04T17:29:28Z) [httpworker] [evo/deterministicmns.cpp:1023] [BuildNewListFromBlock] [mnpayments] CDeterministicMNManager::BuildNewListFromBlock -- MN ae67835776b01b4ad9b153c829bf30fe70e8c3699a09ddcebe431712c38cee63, nConsecutivePayments=0
Firstly, it's easier to spot changes in mocktime and calculate how long if time is logged in seconds. Secondly, the start time `2014-12-04T17:15:37Z` (genesis block) is not round number which makes calculation of "how much exactly time pass" even more difficult.
show penalty changes when banning MNs
Should help debugging when you have a payment with a correct address but with an invalid amount.
Random session ids make it much easier to debug comparing to monotonously increasing ones and also you don't have to track the next id in this case.
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 99 files out of 206 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the WalkthroughA new benchmarking source file, Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Backport Verification - Issues FixedI found 1 issue and automatically applied fixes. Original Bitcoin commit: Issues Found and Fixed:
Verification Details:
Reviewer Feedback Analysis:PastaPastaPasta comments reviewed: 0 No reviewer feedback to address - clean PR with only the compatibility fix needed. The backport should now be complete and faithful to the original. The benchmark functionality for testing block read performance has been successfully adapted to Dash's serialization system. ✅ |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d4202b5 and 088c440d7dbe6ba143bc418cc8ba9e4794f42ff7.
📒 Files selected for processing (2)
src/Makefile.bench.include(1 hunks)src/bench/readblock.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**
⚙️ CodeRabbit Configuration File
**: # CodeRabbit AI Review Instructions for Dash BackportsYour Role
You are reviewing Bitcoin Core backports to Dash Core. Your ONLY job is to validate that the Dash commit faithfully represents the original Bitcoin commit with minimal, necessary adaptations.
Critical Validation Rules
1. File Operations Must Match (AUTO-REJECT if violated)
- If Bitcoin modifies an existing file → Dash MUST modify (not create new)
- If Bitcoin creates a new file → Dash creates
- If Bitcoin deletes a file → Dash deletes
- Common failure: Bitcoin modifies keys.txt, Dash creates new file with 58 keys
2. Size Ratio Check (80-150% of Bitcoin)
- Count functional lines changed (exclude comments/whitespace)
- Dash changes should be 80-150% of Bitcoin's size
- Red flag: 2-line Bitcoin fix becoming 150+ lines in Dash
3. No Scope Creep
- Reject if you see: "TODO:", "FIXME:", "while we're here", "also fix"
- No unrelated refactoring or style changes
- Only Bitcoin's intended changes + minimal Dash adaptations
4. Bitcoin-Specific Code Detection
- Auto-reject witness/segwit code:
msg_wtxidrelay,MSG_WTX, witness imports- Auto-reject RBF (replace-by-fee) functionality
- Note: PSBT is supported in Dash (don't flag)
5. Mandatory Adaptations Only
bitcoin→dashin strings/pathsBitcoin→Dashin user-facing text- Port numbers: 8332→9998 (RPC), 8333→9999 (P2P)
- Hardcoded test values specific to Dash
- No other changes unless absolutely required
6. Completeness Check
- All files changed in Bitcoin must be present
- Extra files need clear justification (Dash-specific compatibility)
- Missing files = incomplete backport
Review Process
- First: Check file operations match exactly
- Second: Calculate size ratio
- Third: Scan for scope creep patterns
- Fourth: Detect Bitcoin-specific code
- Fifth: Verify all changes are minimal adaptations
Output Format
VALIDATION: [PASS/FAIL] File Operatio...
Files:
src/Makefile.bench.includesrc/bench/readblock.cpp
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
C++20 codebase should be placed under src/
Files:
src/bench/readblock.cpp
src/bench/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Performance benchmarks should be placed in src/bench/ and use nanobench
Files:
src/bench/readblock.cpp
🧠 Learnings (5)
📓 Common learnings
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T22:03:12.364Z
Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T19:54:21.426Z
Learning: In Dash backports from Bitcoin Core, including necessary compilation fixes (such as API compatibility changes like UniValue get_int() → getInt<int>()) alongside the core backport is standard and expected practice. These compatibility fixes ensure the backported code compiles in Dash's evolved codebase while preserving Bitcoin's original functionality and intent.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T21:29:32.827Z
Learning: DashCoreAutoGuix successfully fixed scope creep in Bitcoin Core commit fcdb39d3ee backport by removing the parse test case from src/test/uint256_tests.cpp that was not part of the original Bitcoin commit. The fix was implemented in commit 16748115ce and verified through range-diff analysis.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-27T22:35:10.176Z
Learning: In Dash backports, src/dashbls files are vendored dependencies that should not be modified during Bitcoin Core backports unless there is specific justification. Unauthorized modifications to vendored dependencies should be removed to maintain code integrity.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T17:13:35.087Z
Learning: When backporting Bitcoin Core commits that use Python's textwrap.dedent() function in test files, the textwrap import statement needs to be explicitly added if it's missing in the Dash test file.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: In Dash backports from Bitcoin Core test files, the `address_to_scriptpubkey` function should be imported from `test_framework.wallet` instead of `test_framework.address` as the import location differs between Bitcoin and Dash test frameworks.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T17:13:35.087Z
Learning: In Dash backports from Bitcoin Core, when the DIFFICULTY_ADJUSTMENT_INTERVAL constant is missing, it should be defined as 24 for Dash (different from Bitcoin's value), as seen in the getnetworkhashps RPC backport fix.
Learnt from: knst
PR: DashCoreAutoGuix/dash#436
File: src/univalue/include/univalue.h:86-86
Timestamp: 2025-08-04T18:16:45.145Z
Learning: UniValue (src/univalue) is no longer a vendored dependency in Dash as of PR #6775, so modifications to UniValue files are permitted when needed for Bitcoin Core backports and other legitimate changes.
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be placed in src/bench/ and use nanobench
📚 Learning: applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : performance benchmarks should be placed in src/bench/...
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be placed in src/bench/ and use nanobench
Applied to files:
src/Makefile.bench.includesrc/bench/readblock.cpp
📚 Learning: during dash backport verification of bitcoin core commit 06d469c, scope creep was detected when a...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.
Applied to files:
src/Makefile.bench.include
📚 Learning: applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : unit tests should be placed in s...
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Applied to files:
src/Makefile.bench.include
📚 Learning: dashcoreautoguix successfully completed a complex bitcoin core backport (pr bitcoin#29412) for block mutati...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.
Applied to files:
src/bench/readblock.cpp
🔇 Additional comments (4)
src/Makefile.bench.include (1)
55-55: LGTM: Clean benchmark integration.The addition of
bench/readblock.cppto the build sources is exactly what's expected for adding a new benchmark to the suite. The change is minimal and correctly placed in alphabetical order.src/bench/readblock.cpp (3)
24-36: LGTM: Well-structured block read benchmark.The
ReadBlockFromDiskTestbenchmark correctly:
- Uses nanobench framework as required
- Sets up proper test environment with ChainType::MAIN
- Writes test block data once, then benchmarks the read operation
- Asserts successful reads for validation
- Follows clean benchmark patterns
38-50: LGTM: Complementary raw block read benchmark.The
ReadRawBlockFromDiskTestprovides good coverage by benchmarking raw block data reading separately from the deserialized block reading, using the same solid benchmark structure.
52-53: LGTM: Appropriate benchmark registration.Both benchmarks are correctly registered with HIGH priority, which is appropriate for fundamental disk I/O operations that are critical to node performance.
Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: fanquake <fanquake@gmail.com> Co-authored-by: pasta <pasta@pastas-Mac-Studio.local>
…and logging Co-authored-by: fanquake <fanquake@gmail.com> Co-authored-by: pasta <pasta@pastas-Mac-Studio.local>
… net(base).cpp Co-authored-by: fanquake <fanquake@gmail.com>
…back Co-authored-by: fanquake <fanquake@gmail.com>
…ntrol / sendcoins dialog Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
…our headers Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: glozow <gloriajzhao@gmail.com>
…achChain fails Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: fanquake <fanquake@gmail.com> Co-authored-by: Claude Code <claude@anthropic.com>
Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: fanquake <fanquake@gmail.com>
1c8c746 test: remove unused EncodeDecimal from test framework util (pasta) 4ffee8c Merge bitcoin#28014: ci: re-enable gui tests for s390x (PastaBot) 479c23b Merge bitcoin#29481: doc: Update OpenBSD build docs for 7.4 (PastaBot) 265eebd Merge bitcoin#29470: test: Add option to skip python unit tests (PastaBot) 33a294a Merge bitcoin#29469: doc: document preference for list-initialization (PastaBot) 7bb828b Merge bitcoin#29456: docs: ci multi-arch requires qemu (PastaBot) 6640a79 Merge bitcoin#29243: wallet: Reset chain notifications handler if AttachChain fails (PastaBot) e26ae22 Merge bitcoin#29235: doc: refer to "Node relay options" in policy/README (PastaBot) cbd2802 Merge bitcoin#29237: depends: Allow PATH with spaces in directory names. (PastaBot) 92742d8 Merge bitcoin#27928: test: Add more tests for the BIP21 implementation (PastaBot) d6a4ff5 Merge bitcoin#28040: wallet: sqlite: don't include sqlite files from our headers (PastaBot) 0a4441e Merge bitcoin-core/gui#719: Remove confusing "Dust" label from coincontrol / sendcoins dialog (PastaBot) 23c0ea2 Merge bitcoin#28011: test: Rename EncodeDecimal to serialization_fallback (PastaBot) 8b77a80 Merge bitcoin#27530: Remove now-unnecessary poll, fcntl includes from net(base).cpp (PastaBot) 5030a4d Merge bitcoin#27947: MaybePunishNodeForTx: Remove unused message arg and logging (PastaBot) 56686df Merge bitcoin#27949: http: update libevent workaround to correct version (PastaBot) c060a2a Merge bitcoin#27689: doc: remove mention of glibc 2.10+ (PastaBot) Pull request description: ## Issue being fixed or feature implemented Batch of automatically done backports by claude. ## What was done? See commits ## How Has This Been Tested? CI ## Breaking Changes None ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 1c8c746 Tree-SHA512: 8b9487375093de53dca15b15eadfb7a34b6319dd367bd204d71e0407316db891f71ba33f97327ed021423ee060cd7f095313018d80b3e526527c43ea0f8cb167
…block filters (BIP-157/158) 4b90441 docs: indicate that blockfilter will be re-created (PastaPastaPasta) b2522a2 chore: make clang format happy (UdjinM6) 09490b7 chore: drop useless feature_blockfilter_version.py (UdjinM6) 4750246 doc: shrink release notes (UdjinM6) f28bf6d feat: start blockfilter sync from scratch on db version upgrades (UdjinM6) d3f1b9e test: remove wallet dependency check from blockfilter tests (pasta) 492c109 feat: add versioning to blockfilter index to detect incompatible format (pasta) bf352e7 evo: use Span<const unsigned char> in specialtx filter extraction callback to avoid temporary allocations; adjust helpers and call site (pasta) c734b64 tests: p2p_blockfilters: use assert_greater_than for clearer failures (pasta) a5bab55 test: remove wallet dependency from p2p_blockfilters.py and refactor duplicated code (Konstantin Akimov) a1ecf5d tests: p2p_blockfilters: skip when wallet module is not available (pasta) 9266b9a refactor: break circular dependency over evo/assetlock and llmq/signing (Konstantin Akimov) 1c9809f filters: add cross-reference comments to keep bloom and compact filter extraction in sync (pasta) 7a33c14 tests: compact filters: fix F841, verify tx inclusion, and cleanups (pasta) 3fb9183 test: improve p2p_blockfilters special transaction tests (pasta) 8d08a54 Apply suggestions from code review (PastaPastaPasta) 0ea85f7 fix: remove unused variable in blockfilter_tests (pasta) 773fe9b lint: add specialtx_filter circular dependencies to allowed list (pasta) 8ae2280 feat: add special transaction support to compact block filters (BIP-157/158) (pasta) Pull request description: ## Summary - Implements extraction of special transaction fields for compact block filters - Achieves feature parity between bloom filters and compact block filters for special transactions - Enables SPV clients to detect special transactions using either filtering mechanism ## What this PR does This PR adds support for Dash special transactions to the compact block filter implementation (BIP-157/158). Previously, only bloom filters could detect special transaction fields, leaving compact block filter users unable to track masternode-related transactions and platform operations. ### Implementation Details The implementation uses a delegation pattern to extract special transaction fields without creating circular dependencies: - `ExtractSpecialTxFilterElements()` function extracts relevant fields from each special transaction type - Fields extracted match exactly those in the bloom filter implementation (`CheckSpecialTransactionMatchesAndUpdate`) - All 5 special transaction types are supported: - **ProRegTx**: collateral outpoint, owner/voting keys, payout script - **ProUpServTx**: ProTx hash, operator payout script - **ProUpRegTx**: ProTx hash, voting key, payout script - **ProUpRevTx**: ProTx hash - **AssetLockTx**: credit output scripts (excluding OP_RETURN) ### Testing Comprehensive test coverage includes: - **Unit tests**: Test all 5 special transaction types with realistic payloads - **Functional tests**: Create actual AssetLockTx transactions with proper signatures - **Edge cases**: Empty scripts, OP_RETURN exclusion, multiple outputs ## Test plan - [x] Unit tests pass (`make check`) - [x] Functional test passes (`test/functional/p2p_blockfilters.py`) - [x] All special transaction types covered in tests - [x] Verified special fields are added to filters (via debug logging) 🤖 Generated with [Claude Code](https://claude.ai/code) ACKs for top commit: knst: utACK 4b90441 UdjinM6: utACK 4b90441 Tree-SHA512: 673b47ef8626761cdffc659c6ca0ffd214b6d3e7ea6086976528ec79967f5a6a6d864b63cc1e5bac38f079fa092c49637f334c058231b4f6c687f9dae585db27
5280200 to
57884f0
Compare
|
🔧 Validation FIXED: Applied 1 fix for Bitcoin backport compliance Fixes Applied:
Validation Results:✅ Size ratio: 101.9% (within 80-150% range) The missing file was causing all CI build failures (5 jobs failing). This fix should resolve the build issues and allow CI to pass. |
Backports bitcoin#26684
Original commit: 00bf4a1
Backported from Bitcoin Core v0.27
Summary by CodeRabbit
New Features
Chores