refactor: reduced amount of block hashes computed#834
Conversation
📝 WalkthroughWalkthroughThis PR migrates dash-spv's header storage APIs from raw ChangesStorage layer HashedBlockHeader migration
Sync managers/pipelines adopt hashed headers and blocks
WalletInterface process_block_for_wallets explicit block_hash
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dash-spv/src/storage/block_headers.rs (1)
165-169: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRemove stale hash mappings on height rewrites.
SegmentCache::insertonly blocks overwrites withdebug_assert!, so release builds can replace an existing header at the same height. Clear the oldhash -> heightentries for the overwritten range before inserting the replacement batch, or add a runtime reject if rewrites should never happen.🤖 Prompt for 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. In `@dash-spv/src/storage/block_headers.rs` around lines 165 - 169, The stale hash mappings are left behind when `SegmentCache::insert` overwrites headers at an existing height in release builds. Update `BlockHeaders::store_items_at_height` to remove any old `header_hash_index` entries for the overwritten height range before calling `store_items_at_height`, or add a runtime check to reject rewrites if they are not allowed; use the existing `header_hash_index` and `store_items_at_height` logic to locate and fix the replacement path.
🧹 Nitpick comments (2)
key-wallet-manager/src/wallet_interface.rs (1)
68-74: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument the
block_hash/blockinvariant.The new
block_hashparameter is caller-supplied and must correspond toblock's actual hash for correctBlockInfoconstruction downstream, but nothing in the trait doc states this contract. Since implementers no longer independently verify the hash (that's the point of the optimization), a mismatch would silently corrupt storedBlockInfo/event data.Consider adding a doc line clarifying the invariant, e.g. "
block_hashmust be the hash ofblock(callers typically already have it fromHashedBlock/HashedBlockHeaderand should avoid recomputing it)."🤖 Prompt for 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. In `@key-wallet-manager/src/wallet_interface.rs` around lines 68 - 74, Document the new caller-supplied block_hash contract on process_block_for_wallets: it must always match the actual hash of block, since implementers now rely on that value for BlockInfo/event construction and no longer re-verify it. Update the trait documentation near process_block_for_wallets in wallet_interface.rs to state this invariant clearly, and mention that callers should pass the hash already available from HashedBlock or HashedBlockHeader rather than recomputing it.key-wallet-manager/src/process_block.rs (1)
20-31: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winGuard the
block_hashinvariant with a debug assertion.
block_hashis now trusted verbatim to buildBlockInfo, which is persisted intoTransactionRecord.contextand surfaced inBlockProcessedevents. If a caller ever passes a hash that doesn't matchblock, this will silently corrupt that context with no compile-time or runtime signal. Adebug_assert_eq!costs nothing in release builds (preserving the perf goal of this PR) while catching integration bugs during development/tests.🛡️ Proposed guard
) -> BlockProcessingResult { let mut result = BlockProcessingResult::default(); if wallets.is_empty() { return result; } + debug_assert_eq!( + block_hash, + block.block_hash(), + "block_hash must match block.block_hash()" + ); let info = BlockInfo::new(height, block_hash, block.header.time);🤖 Prompt for 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. In `@key-wallet-manager/src/process_block.rs` around lines 20 - 31, Add a debug-time invariant check in process_block_for_wallets before constructing BlockInfo: verify the provided block_hash matches the hash derived from block and fail fast in debug builds if it does not. Keep the check close to the BlockInfo::new call so the trusted hash used for TransactionRecord.context and BlockProcessed stays validated without affecting release performance.
🤖 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.
Outside diff comments:
In `@dash-spv/src/storage/block_headers.rs`:
- Around line 165-169: The stale hash mappings are left behind when
`SegmentCache::insert` overwrites headers at an existing height in release
builds. Update `BlockHeaders::store_items_at_height` to remove any old
`header_hash_index` entries for the overwritten height range before calling
`store_items_at_height`, or add a runtime check to reject rewrites if they are
not allowed; use the existing `header_hash_index` and `store_items_at_height`
logic to locate and fix the replacement path.
---
Nitpick comments:
In `@key-wallet-manager/src/process_block.rs`:
- Around line 20-31: Add a debug-time invariant check in
process_block_for_wallets before constructing BlockInfo: verify the provided
block_hash matches the hash derived from block and fail fast in debug builds if
it does not. Keep the check close to the BlockInfo::new call so the trusted hash
used for TransactionRecord.context and BlockProcessed stays validated without
affecting release performance.
In `@key-wallet-manager/src/wallet_interface.rs`:
- Around line 68-74: Document the new caller-supplied block_hash contract on
process_block_for_wallets: it must always match the actual hash of block, since
implementers now rely on that value for BlockInfo/event construction and no
longer re-verify it. Update the trait documentation near
process_block_for_wallets in wallet_interface.rs to state this invariant
clearly, and mention that callers should pass the hash already available from
HashedBlock or HashedBlockHeader rather than recomputing it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a24b8ce7-a969-4e56-b98d-520ab6f54572
📒 Files selected for processing (19)
dash-spv/benches/storage.rsdash-spv/src/client/lifecycle.rsdash-spv/src/storage/block_headers.rsdash-spv/src/storage/mod.rsdash-spv/src/sync/block_headers/manager.rsdash-spv/src/sync/blocks/manager.rsdash-spv/src/sync/blocks/pipeline.rsdash-spv/src/sync/blocks/sync_manager.rsdash-spv/src/sync/chainlock/manager.rsdash-spv/src/sync/filter_headers/pipeline.rsdash-spv/src/sync/filters/manager.rsdash-spv/src/sync/filters/pipeline.rsdash-spv/src/sync/masternodes/manager.rsdash-spv/src/sync/masternodes/sync_manager.rskey-wallet-manager/src/event_tests.rskey-wallet-manager/src/process_block.rskey-wallet-manager/src/test_utils/mock_wallet.rskey-wallet-manager/src/wallet_interface.rskey-wallet-manager/tests/spv_integration_tests.rs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #834 +/- ##
==========================================
- Coverage 73.16% 72.93% -0.23%
==========================================
Files 323 323
Lines 72438 72569 +131
==========================================
- Hits 52999 52931 -68
- Misses 19439 19638 +199
|
ZocoLini
changed the title
1 participant
The Block header storage now only works with Hashed block headers, avoiding block hash recomputation
Method that require the hash ask for it si the caller can supply an already computed one
This PR improves dash-spv performance and, somehow, gives stability to the benchmark I am using