fix: align bloom filter size/hash calculation with Dash Core#529
fix: align bloom filter size/hash calculation with Dash Core#529xdustinface wants to merge 1 commit intov0.42-devfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRewrote Bloom filter sizing and hashing to align with Dash Core: compute filter_bits from elements and false_positive_rate, clamp to ≥8 bits, derive filter_bytes/aligned_bits, and recalculate n_hash_funcs from aligned_bits. Allocation, BitVec init, comments, and tests (including 1-byte edge cases and Dash Core vectors) were updated. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Fix `BloomFilter` size calculation to compute filter size in bytes first (integer division), then convert to bits (bytes*8), matching how Dash Core implementation sizes the filter. The previous code used `ceil` on bits directly, leading to wrong filter sizes and hash moduli that caused bloom filter mismatches.
xdustinface
force-pushed
the
fix/bloom-calculation
branch
from
b9c5d69 to
a07f95c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash/src/bloom/filter.rs`:
- Around line 355-363: Replace the hardcoded private key bytes, SecretKey
creation, and the fixed Network::Mainnet use in the test: instead of building a
PrivateKey from privkey_bytes and calling PrivateKey::new_uncompressed(...
Network::Mainnet) (symbols: privkey_bytes, secret_key,
PrivateKey::new_uncompressed, pubkey, secp), use a non-sensitive test
fixture—either construct a PublicKey directly from a constant pubkey hex (e.g.,
PublicKey::from_slice(...)) so no private material is present, or derive a
deterministic SecretKey from a constant seed for test purposes and use a
test/regtest network constant (not Mainnet) pulled from test config; update
places that expect privkey/pubkey to accept the new fixture.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #529 +/- ##
=============================================
- Coverage 66.92% 66.90% -0.02%
=============================================
Files 313 313
Lines 64729 64877 +148
=============================================
+ Hits 43322 43409 +87
- Misses 21407 21468 +61
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
| /// https://github.com/dashpay/dash/blob/b66b56c3019fe7ab0c9f35dd9894c0ded4d9d420/src/test/bloom_tests.cpp#L51-L74 | ||
| #[test] | ||
| fn test_bloom_filter_dash_core_compatibility_with_tweak() { | ||
| let mut filter = BloomFilter::new(3, 0.01, 2147483649, BloomFlags::All).unwrap(); |
There was a problem hiding this comment.
I dont see difference beetwen this test and test_bloom_filter_dash_core_compatibility other than the filter constructor, maybe a for loop over a collection containing both bloom filters?? Maybe extract the logic into a function encapsulated in the test function and call it with both filters??
| let aligned_bits = filter_bytes * 8; | ||
|
|
||
| let n_hash_funcs = (aligned_bits as f64 / elements as f64 * ln2) as u32; | ||
| let n_hash_funcs = n_hash_funcs.clamp(1, MAX_HASH_FUNCS); |
There was a problem hiding this comment.
using clamp is kinda unnecessary, u32 cannot be less than 1 and the C++ code is not checking it either
| let filter_size = filter_size.clamp(1, MAX_BLOOM_FILTER_SIZE * 8); | ||
| let filter_bits = | ||
| (-1.0 / ln2_squared * elements as f64 * false_positive_rate.ln()) as usize; | ||
| let filter_bits = filter_bits.clamp(8, MAX_BLOOM_FILTER_SIZE * 8); |
There was a problem hiding this comment.
I dont see the C++ code checking if the value is less than 8, and I dont know enough about BloomFilters implementation right now, so I have to trust you
| /// Expected serialized bytes: "038fc16b080000000000000001" | ||
| #[test] | ||
| fn test_bloom_create_insert_key() { | ||
| let secp = secp256k1::Secp256k1::new(); |
There was a problem hiding this comment.
This unit test is testing the same paths (in BloomFilter) the test_bloom_filter_dash_core_compatibility function is testing, right??
2 participants
Fix
BloomFiltersize calculation to compute filter size in bytes first (integer division), then convert to bits (bytes*8), matching how Dash Core implementation sizes the filter.The previous code used
ceilon bits directly, leading to wrong filter sizes and hash moduli that caused bloom filter mismatches.Summary by CodeRabbit
Bug Fixes
Tests