Conversation
WalkthroughThe changes replace the use of the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BloomFilter
participant KeccakHasher
Caller->>BloomFilter: probe_index_keccak(value_bytes, iteration, capacity)
BloomFilter->>KeccakHasher: hash(concatenated value_bytes + iteration)
KeccakHasher-->>BloomFilter: keccak_hash_result
BloomFilter->>BloomFilter: Compute index from hash chunks
BloomFilter-->>Caller: index
sequenceDiagram
participant Caller
participant KeccakHasher
participant solana_nostd_keccak
Caller->>KeccakHasher: hashv(input_slices)
KeccakHasher->>solana_nostd_keccak: hashv(input_slices)
solana_nostd_keccak-->>KeccakHasher: hash_result
KeccakHasher-->>Caller: hash_result
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ananas-block
force-pushed
the
jorrit/chore-bloom-filter-opt
branch
from
8fffb35 to
c478e32
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: 2
🧹 Nitpick comments (1)
program-libs/bloom-filter/Cargo.toml (1)
15-15: Same feature-flag concern as inlight-hasherMirror the minimal-feature declaration here as well to keep both crates in sync and avoid duplicate code paths being built.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
program-libs/bloom-filter/Cargo.toml(1 hunks)program-libs/bloom-filter/src/lib.rs(1 hunks)program-libs/hasher/Cargo.toml(1 hunks)program-libs/hasher/src/keccak.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Test program-libs
- GitHub Check: stateless-js
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test sdk-libs
- GitHub Check: lint
🔇 Additional comments (1)
program-libs/hasher/Cargo.toml (1)
26-26: Pinningsolana-nostd-keccakwithout feature flags may bloat the WASM/BPF build
solana-nostd-keccak = "0.1.3"pulls in all default features of the crate.
If the crate exposesstd/allocgated features, they are compiled for the BPF target unless you explicitly disable them, increasing binary size.-solana-nostd-keccak = "0.1.3" +solana-nostd-keccak = { version = "0.1.3", default-features = false, features = ["keccak"] }Validate the exact feature set the crate provides and enable only what’s needed.
ananas-block
force-pushed
the
jorrit/chore-bloom-filter-opt
branch
2 times, most recently
from
594da88 to
31721fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
program-libs/bloom-filter/src/lib.rs (1)
71-76:⚠️ Potential issueStill prone to slice-length & 32-vs-64 bit panics
The two root problems flagged in the previous review are still present:
value_bytesis a generic slice.copy_from_slicewill panic if the caller passes fewer than 32 bytes.iterationisusize; on a 32-bit targetto_le_bytes()yields only 4 bytes, but you blindly copy 8 bytes intocombined_bytes[32..], causing another panic.-pub fn probe_index_keccak(value_bytes: &[u8], iteration: usize, capacity: &u64) -> usize { - let iter_bytes = iteration.to_le_bytes(); +pub fn probe_index_keccak(value_bytes: &[u8; 32], iteration: u64, capacity: u64) -> usize { + let iter_bytes = iteration.to_le_bytes(); // always 8 bytes(you can still take
&capacityat the call-site if you prefer).Failing to fix these will crash in production the first time an undersized slice is supplied or the code is compiled for a 32-bit environment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
program-libs/bloom-filter/Cargo.toml(1 hunks)program-libs/bloom-filter/src/lib.rs(2 hunks)program-libs/hasher/Cargo.toml(1 hunks)program-libs/hasher/src/keccak.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- program-libs/hasher/Cargo.toml
- program-libs/bloom-filter/Cargo.toml
- program-libs/hasher/src/keccak.rs
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: stateless-js
- GitHub Check: Test sdk-libs
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test program-libs
- GitHub Check: lint
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
program-libs/bloom-filter/src/lib.rs (1)
104-107: Surface API and internal helper are now inconsistent
_insertpasses a&[u8;32]toprobe_index_keccak, which currently accepts&[u8]. Once the helper’s signature is tightened (see first comment), remember to update this call site; otherwise you’ll get a mismatched-type compile error.
| let mut index = 0u64; | ||
| for chunk in hash.chunks(8) { | ||
| let value = u64::from_le_bytes(chunk.try_into().unwrap()); | ||
| index = value.wrapping_add(index) % *capacity; | ||
| } | ||
| index as usize |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential usize overflow & extra modulo work
index is accumulated as u64 but returned as usize. On a 32-bit build capacity may legitimately exceed usize::MAX, making the cast truncating and producing out-of-range bit indices (bits[index] will panic). Either:
- Store
capacityasusizeeverywhere, or - Keep the function
u64throughout and check/guard before indexing.
Also, taking mod for every 8-byte chunk is unnecessary; taking a single u64 from the hash and modding once is sufficient and faster.
🤖 Prompt for AI Agents
In program-libs/bloom-filter/src/lib.rs around lines 79 to 84, the code
accumulates an index as u64 and then casts it to usize, which can cause overflow
on 32-bit systems if capacity exceeds usize::MAX, leading to panics when
indexing. To fix this, either change capacity to usize throughout the code to
ensure safe indexing or keep the index as u64 and add checks before indexing to
prevent out-of-range access. Additionally, optimize by removing the modulo
operation inside the loop and instead take a single u64 from the hash and apply
modulo once after accumulation to improve performance.
31721fa to
39b6144
Compare
ananas-block
force-pushed
the
jorrit/chore-bloom-filter-opt
branch
from
39b6144 to
287e260
Compare
2 participants
Issue:
Solution:
no-std-keccakhashes which are a couple CU cheaper than fastmurmur3-> this makes it harder to construct an account to produce a bloom filter collision
Summary by CodeRabbit
Refactor
Chores