Merged
Conversation
test: truncated extra bytes rejection
Benchmark for 7c72fddClick to view benchmark
|
Benchmark for 7c72fddClick to view benchmark
|
Contributor
Author
|
Hi @vinhtc27, I've noticed while writing tests for FFI, that My second question: Extra-bytes behavior for proof values is undocumented in tests; both Rust (rln/tests/protocol.rs:198) and FFI (rln/tests/ffi.rs:971) only test truncation, so any silent acceptance of trailing bytes would go unnoticed. Should we reject extra bytes with error or silently ignore them or just document in comment ? |
Benchmark for a55667fClick to view benchmark
|
Benchmark for a55667fClick to view benchmark
|
Benchmark for f7156a1Click to view benchmark
|
Benchmark for f7156a1Click to view benchmark
|
Benchmark for dc8c9deClick to view benchmark
|
Benchmark for dc8c9deClick to view benchmark
|
romanzac
added
the
run-coverage
Benchmark for 4288136Click to view benchmark
|
Benchmark for 4288136Click to view benchmark
|
|
Coverage report uploaded. Download HTML Report |
Contributor
Can you explain what you mean by “validate the tree depth”? The FFI is basically just a wrapper around the public API, so it behaves the same as the public API, and all validation is performed within the public API functionality.
You can safely ignore it. I’m working on another Zerokit feature (PR) that allows us to pack multiple message IDs at once, which will extend the serialized bytes at the end. If we add validation for extra bytes, it’s going to conflict with this. |
Contributor
Author
What I mean is large tree depth could result in a massive amount of memory consumed. Merkle tree of depth 50 would need ~32 PiB storing 32B leaves only. |
Contributor
That's okay. The user can input any depth value they want as long as they provide the correct resources, such as the proving key and graph file. The tree will grow leaf by leaf, so it won't be a problem in term of memory. |
Collaborator
|
@romanzac looks good Can you just check that |
romanzac
removed
the
run-coverage
Benchmark for ec3a8b8Click to view benchmark
|
Benchmark for ec3a8b8Click to view benchmark
|
Benchmark for 1466125Click to view benchmark
|
Benchmark for 1466125Click to view benchmark
|
Contributor
Author
Collaborator
Collaborator
…validation (#380) ## Description This PR fixes panic paths in `override_range`/`atomic_operation` flows by centralizing index/range validation and converting invalid inputs into explicit errors. Related to: #379 ## Changes - Added shared validator: `validate_override_range_inputs` in `utils/src/merkle_tree/override_range_validation.rs`. - Added `EmptyIndicesPolicy` enum (`Allow` / `Reject`) to make adapter-specific empty-index behavior explicit. - Wired shared validation into: - `rln/src/pm_tree_adapter.rs` - `utils/src/merkle_tree/full_merkle_tree.rs` - `utils/src/merkle_tree/optimal_merkle_tree.rs` - Replaced unchecked `start + leaves_len` arithmetic with `checked_add` in `set_range` paths for full/optimal trees. ## Testing - Added regression coverage in `rln/tests/ffi.rs` for: - valid `start` with problematic delete-index layout (previously could panic), - overflowing `start` (`usize::MAX`) in atomic operation. - Added unit tests for validator behavior (overflow, incompatible mixed offsets, empty-index policy). Covered scenarios: - OOB delete indices now return error (no panic). - Overflow in `start + leaves_len` now returns error (no panic). - Empty-index handling is explicit per implementation via `EmptyIndicesPolicy`. --- ## PR Lifecycle > [!IMPORTANT] > **Draft PRs** signal that work is still in progress and **will not trigger CI**. > Only mark your PR as **Ready for review** when you believe it is complete. > All CI checks **must pass** before requesting a review. ## Code Guidelines Please keep the following in mind (see [CONTRIBUTING.md](../CONTRIBUTING.md) for full details): ### Commits - Follow [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) (`feat(rln):`, `fix(utils):`, `chore:`, etc.) - Use the appropriate scope: `rln`, `rln-cli`, `rln-wasm`, `utils`, `ci` - GPG-sign your commits ### Error Handling - **No panics in library code.** Do not use `unwrap()`, `expect()`, or `panic!()` in production paths inside `rln/src/` or `utils/src/`. The only acceptable exception is an internal invariant that is statically guaranteed — and even then, prefer returning an error. - Use the project's `thiserror`-based error types (`RLNError`, `ProtocolError`, `UtilsError`, etc.) and propagate errors with `?`. - Provide context in error variants (e.g., `InsufficientData { expected, actual }`). - `unwrap()` is fine in **tests**. ### Code Style - Run `cargo fmt --all -- --check` to verify formatting (CI enforces this on stable). - Group imports: std first, then external crates, then local modules (see `rustfmt.toml`). - Use `pub(crate)` for items that should not be part of the public API. - Apply `Zeroize` / `ZeroizeOnDrop` to any struct holding secret material. ### Linting (mirrors CI) CI runs clippy across multiple crate/feature combinations. Run the relevant checks locally before pushing: ```bash # Default features — workspace root (rln + utils) cargo clippy --all-targets --tests --release -- -D warnings # Stateless feature — from rln/ cd rln && cargo clippy --all-targets --tests --release \ --features=stateless --no-default-features -- -D warnings # WASM target — from rln-wasm/ cd rln-wasm && cargo clippy --target wasm32-unknown-unknown \ --tests --release -- -D warnings ``` At minimum, run the default-features check. If your changes touch `stateless` or `rln-wasm`, run those checks as well. ## Checklist - [x] My PR title follows [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) format - [x] I have linked the related issue(s) - [x] `cargo fmt --all -- --check` produces no changes - [x] Clippy passes for all affected crate/feature combinations (see [Linting](#linting-mirrors-ci) above) - [x] `make test` passes locally - [x] No new `unwrap()` / `expect()` / `panic!()` in library code - [x] New code includes appropriate tests (unit / integration / WASM where applicable) - [x] I have run the CI coverage report — add the `run-coverage` label to enable it - [x] All CI checks pass and the PR is marked **Ready for review**
Benchmark for db9e347Click to view benchmark
|
Benchmark for db9e347Click to view benchmark
|
Collaborator
|
@romanzac please update branch and me can merge this pr as bug was fixed (if not let me know) |
Benchmark for 022c5f2Click to view benchmark
|
Benchmark for 022c5f2Click to view benchmark
|
romanzac
added
the
run-coverage
Benchmark for 022c5f2Click to view benchmark
|
Benchmark for 022c5f2Click to view benchmark
|
|
Coverage report uploaded. Download HTML Report |
Contributor
Author
All good. Thanks! |
seemenkina
approved these changes
Feb 26, 2026
Contributor
Author
|
Hi @vinhtc27 , please kindly add your review. |
Benchmark for c8de892Click to view benchmark
|
Benchmark for c8de892Click to view benchmark
|
|
Coverage report uploaded. Download HTML Report |
vinhtc27
approved these changes
Feb 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A batch of tests to cover serialization and FFI error cases for witness/proof values. C FFI coverage extension for out-of-bounds, bad depth, and invalid inputs.
Tests modified/added
rln/tests/ffi.rs:
rln/tests/protocol.rs:
Issues reported
Coverage changed
Before 88.05%
Download HTML Report
After 88.98%
Download HTML Report
Checklist
run-coveragelabel to this PR to enable it.