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
📝 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, recalculate n_hash_funcs from aligned_bits, update allocation/BitVec init, comments, and tests (including 1‑byte and Dash Core vectors). 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 |
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.87% 66.90% +0.03%
=============================================
Files 313 313
Lines 64753 64816 +63
=============================================
+ Hits 43305 43368 +63
Misses 21448 21448
*This pull request uses carry forward flags. Click here to find out more.
|
| /// 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??
There was a problem hiding this comment.
Its just mirroring the c++ test cases.
| 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
There was a problem hiding this comment.
u32 cannot be less than 1
What about 0? Apparently unlikely to happen but maybe just an edge case issue in dash core then.
dash/src/bloom/filter.rs
Outdated
| 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
There was a problem hiding this comment.
Hm right, going to drop it.
| /// 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??
There was a problem hiding this comment.
Its just mirroring the c++ test cases.
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
a07f95c to
73ea5ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
dash/src/bloom/filter.rs (1)
352-361:⚠️ Potential issue | 🟠 MajorReplace hardcoded private key material and fixed mainnet parameter in test fixture.
Line 353 and Line 360 still embed explicit key material and a fixed
Network::Mainnet. Please switch this test to a non-sensitive public fixture path (e.g., fixed pubkey bytes / hash vector) without private key construction.As per coding guidelines,
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash/src/bloom/filter.rs` around lines 352 - 361, The test currently embeds secret material via privkey_bytes/secret_key and forces PrivateKey::new_uncompressed(... Network::Mainnet) — remove construction of any private key and the hardcoded Network::Mainnet: replace the block so it uses a fixed public-key fixture (e.g., a compressed/uncompressed pubkey byte array or hex string) and create the public key directly (use secp256k1::PublicKey::from_slice or PublicKey::from_str) instead of deriving it from a PrivateKey; if any network value is required, use a test-safe constant or derive it from the fixture rather than Network::Mainnet and remove references to privkey_bytes, secret_key, privkey and PrivateKey::new_uncompressed to ensure no secret or hardcoded network parameters remain.
🤖 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 58-62: Insert can panic when the computed filter length is zero
because insert performs hash % self.filter.len(); update the BloomFilter::insert
implementation to guard against an empty filter by returning early when
self.filter.is_empty() (or equivalently checking self.filter.len() == 0) before
performing any modulo/indexing, and add a brief comment referencing the
zero-length-filter case; alternatively, ensure creation code (where
filter_bits/filter_bytes are computed) enforces a minimum of 1 byte by clamping
filter_bytes = filter_bytes.max(1) so that insert never sees len == 0—pick one
approach and apply consistently to avoid the modulo-by-zero in insert (refer to
variables filter_bits, filter_bytes and the insert method/self.filter.len()).
---
Duplicate comments:
In `@dash/src/bloom/filter.rs`:
- Around line 352-361: The test currently embeds secret material via
privkey_bytes/secret_key and forces PrivateKey::new_uncompressed(...
Network::Mainnet) — remove construction of any private key and the hardcoded
Network::Mainnet: replace the block so it uses a fixed public-key fixture (e.g.,
a compressed/uncompressed pubkey byte array or hex string) and create the public
key directly (use secp256k1::PublicKey::from_slice or PublicKey::from_str)
instead of deriving it from a PrivateKey; if any network value is required, use
a test-safe constant or derive it from the fixture rather than Network::Mainnet
and remove references to privkey_bytes, secret_key, privkey and
PrivateKey::new_uncompressed to ensure no secret or hardcoded network parameters
remain.
| let filter_bits = | ||
| (-1.0 / ln2_squared * elements as f64 * false_positive_rate.ln()) as usize; | ||
| let filter_bits = filter_bits.min(MAX_BLOOM_FILTER_SIZE * 8); | ||
| let filter_bytes = filter_bits / 8; | ||
|
|
There was a problem hiding this comment.
Zero-byte filters make insert panic via modulo-by-zero.
With the new byte-first sizing, zero-length filters are now valid (also asserted in Line 287). That makes insert hit hash % self.filter.len() with len == 0, causing a panic on first insertion.
🐛 Proposed fix
pub fn insert(&mut self, data: &[u8]) {
+ if self.filter.is_empty() {
+ return; // Dash Core-compatible no-op for zero-byte filters
+ }
for i in 0..self.n_hash_funcs {
let seed = i.wrapping_mul(0xfba4c795).wrapping_add(self.n_tweak);
let hash = murmur3(data, seed);
let index = (hash as usize) % self.filter.len();
self.filter.set(index, true);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dash/src/bloom/filter.rs` around lines 58 - 62, Insert can panic when the
computed filter length is zero because insert performs hash % self.filter.len();
update the BloomFilter::insert implementation to guard against an empty filter
by returning early when self.filter.is_empty() (or equivalently checking
self.filter.len() == 0) before performing any modulo/indexing, and add a brief
comment referencing the zero-length-filter case; alternatively, ensure creation
code (where filter_bits/filter_bytes are computed) enforces a minimum of 1 byte
by clamping filter_bytes = filter_bytes.max(1) so that insert never sees len ==
0—pick one approach and apply consistently to avoid the modulo-by-zero in insert
(refer to variables filter_bits, filter_bytes and the insert
method/self.filter.len()).
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