Conversation
WalkthroughThe changes introduce a Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant BatchedQueueAccount
participant BatchedQueueMetadata
Test->>BatchedQueueAccount: init(..., tree_capacity)
BatchedQueueAccount->>BatchedQueueMetadata: init(..., tree_capacity)
BatchedQueueMetadata-->>BatchedQueueAccount: stores tree_capacity
BatchedQueueAccount-->>Test: returns initialized account with tree_capacity
sequenceDiagram
participant Validator
participant Params
Validator->>Params: validate_batched_tree_params(params)
Params-->>Validator: checks root_history_capacity >= (output_batches + input_batches)
Validator->>Params: validate_batched_address_tree_params(params)
Params-->>Validator: checks root_history_capacity >= input_batches
sequenceDiagram
participant MerkleTreeAccount
participant QueueMetadata
MerkleTreeAccount->>MerkleTreeAccount: tree_is_full(batch_size)
MerkleTreeAccount-->>Caller: returns true if next_index + batch_size >= capacity
Estimated code review effort3 (~45 minutes) Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 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 (
|
![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
🔭 Outside diff range comments (3)
program-libs/batched-merkle-tree/src/queue.rs (3)
61-78: Validatetree_capacityupfront
tree_capacityis blindly assigned but never sanity-checked.
Consider asserting that:
tree_capacity > 0tree_capacity >= batch_size * NUM_BATCHEStree_capacity.is_power_of_two()(if height-based)This prevents silent overflow/under-allocation later on.
421-423: Potential off-by-one & overflow intree_is_fullEquality misses the case where
next_indexhas already exceeded capacity, returningfalseagain:- self.tree_capacity == self.batch_metadata.next_index + self.batch_metadata.next_index >= self.tree_capacityCombine this with an internal guard in
insert_into_current_batchto early-abort when full.
215-243: Guard insertions against overflowing the tree
insert_into_current_batchdoesn’t callcheck_tree_is_full; callers might forget and push past capacity.Add at the very top:
self.check_tree_is_full()?;Ensures safety at the API boundary.
🧹 Nitpick comments (1)
program-libs/batched-merkle-tree/src/initialize_state_tree.rs (1)
260-273: Enhance test coverage for edge cases.While the panic test is good, consider adding more comprehensive test cases to cover edge scenarios like non-divisible batch sizes and boundary conditions.
Additional test cases to consider:
#[test] fn test_validate_root_history_capacity_edge_cases() { // Test with non-perfectly divisible batch sizes let params = InitStateTreeAccountsInstructionData { root_history_capacity: 21, // Should be enough for (101/10) + (101/10) = 20.2 -> 20 input_queue_batch_size: 101, output_queue_batch_size: 101, input_queue_zkp_batch_size: 10, output_queue_zkp_batch_size: 10, ..Default::default() }; validate_batched_tree_params(params); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
program-libs/batched-merkle-tree/src/batch.rs(1 hunks)program-libs/batched-merkle-tree/src/initialize_address_tree.rs(2 hunks)program-libs/batched-merkle-tree/src/initialize_state_tree.rs(2 hunks)program-libs/batched-merkle-tree/src/queue.rs(10 hunks)program-libs/batched-merkle-tree/tests/queue.rs(2 hunks)programs/account-compression/src/instructions/migrate_state.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Forester e2e test
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: Test batched-merkle-tree-simulate
🔇 Additional comments (5)
program-libs/batched-merkle-tree/src/initialize_state_tree.rs (2)
165-165: LGTM: Tree capacity calculation is correct.The addition of
2u64.pow(params.height)as the tree capacity parameter is mathematically correct for a binary Merkle tree, where the capacity equals 2^height.
253-258: LGTM: Basic validation test is appropriate.The test verifies that default parameters pass validation, which is a good baseline check.
program-libs/batched-merkle-tree/src/batch.rs (1)
992-992: LGTM! Clean refactoring removes redundant assignment.The change eliminates the redundant manual assignment of
tree_capacityby passing the value directly to theinitfunction, which is more consistent with the new initialization pattern.program-libs/batched-merkle-tree/tests/queue.rs (1)
74-74: LGTM! Tests correctly updated for new initialization signature.The addition of the
tree_capacityparameter (1024 = 2^10) to theBatchedQueueAccount::initcalls properly aligns with the API changes and uses appropriate test values.Also applies to: 103-103
programs/account-compression/src/instructions/migrate_state.rs (1)
218-218: LGTM! Test helper correctly uses capacity from metadata.The change properly passes
account.tree_capacityfrom theBatchedQueueMetadatastruct to the initialization function, ensuring consistency between the metadata and the initialized account.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
programs/system/src/errors.rs (1)
115-118: Consider improving error message clarity.The new error variants follow the correct pattern, but the error messages could be more descriptive:
BorrowingDataFailed- Consider specifying what type of data borrowing failedDuplicateAccountInInputsAndReadOnly- Consider adding context about why this is invalid- #[error("Borrowing data failed")] + #[error("Failed to borrow account data")] BorrowingDataFailed, - #[error("DuplicateAccountInInputsAndReadOnly")] + #[error("Account appears in both inputs and read-only accounts")] DuplicateAccountInInputsAndReadOnly,programs/system/src/processor/process.rs (1)
352-368: Verify the efficiency of the nested loop approach.The implementation correctly prevents duplicate accounts between inputs and read-only accounts. However, the O(n×m) complexity could be optimized for large transactions.
Consider using a HashSet for better performance with large account lists:
+use std::collections::HashSet; -#[inline(always)] -fn check_no_duplicate_accounts_in_inputs_and_read_only( - input_nullifiers: &ZeroCopySliceMut<'_, u8, InsertNullifierInput, false>, - read_only_accounts: &[ZPackedReadOnlyCompressedAccount], -) -> Result<()> { - for read_only_account in read_only_accounts { - for input_nullifier in input_nullifiers.iter() { - if read_only_account.account_hash == input_nullifier.account_hash { - return Err(SystemProgramError::DuplicateAccountInInputsAndReadOnly.into()); - } - } - } - Ok(()) -} +#[inline(always)] +fn check_no_duplicate_accounts_in_inputs_and_read_only( + input_nullifiers: &ZeroCopySliceMut<'_, u8, InsertNullifierInput, false>, + read_only_accounts: &[ZPackedReadOnlyCompressedAccount], +) -> Result<()> { + let input_hashes: HashSet<[u8; 32]> = input_nullifiers + .iter() + .map(|input| input.account_hash) + .collect(); + + for read_only_account in read_only_accounts { + if input_hashes.contains(&read_only_account.account_hash) { + return Err(SystemProgramError::DuplicateAccountInInputsAndReadOnly.into()); + } + } + Ok(()) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
anchor-programs/system/src/errors.rs(1 hunks)program-tests/system-cpi-v2-test/tests/invoke_cpi_with_read_only.rs(1 hunks)program-tests/utils/src/e2e_test_env.rs(1 hunks)programs/system/src/errors.rs(2 hunks)programs/system/src/processor/process.rs(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- anchor-programs/system/src/errors.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: cli-v2
- GitHub Check: cli-v1
- GitHub Check: Test program-libs-slow
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-fast
- GitHub Check: stateless-js-v2
- GitHub Check: Test sdk-libs
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: lint
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (system-cpi-test-v2-other, ["cargo-test-sbf -p system-cpi-v2-test -- --skip funct...
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: Forester e2e test
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (system-cpi-test-v2-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: stateless-js-v1
🔇 Additional comments (6)
programs/system/src/errors.rs (1)
188-189: LGTM: Error codes follow correct sequential pattern.The error code assignments (6052, 6053) correctly follow the sequential numbering pattern and are positioned appropriately in the match statement.
program-tests/utils/src/e2e_test_env.rs (1)
2494-2496: LGTM! Effective duplicate prevention logic.This change correctly prevents duplicate accounts from appearing in both
input_accountsandread_only_accountscollections, which aligns with the new validation logic mentioned in the AI summary that rejects transactions with duplicate compressed accounts.program-tests/system-cpi-v2-test/tests/invoke_cpi_with_read_only.rs (1)
2829-2930: LGTM: Well-structured test for duplicate account validation.The new test function correctly validates the runtime check for duplicate accounts in inputs and read-only lists. The test setup is appropriate, creates the necessary state, and properly asserts the expected error condition.
A few observations:
- The test follows established patterns from other tests in the file
- Proper use of
serialattribute to avoid conflicts with other tests- Correct error assertion using
SystemProgramError::DuplicateAccountInInputsAndReadOnly- Good test isolation by creating its own compressed account
programs/system/src/processor/process.rs (3)
190-190: Good refactoring to enable duplicate account validation.Moving the
read_only_accountsretrieval earlier in the process enables the new duplicate account validation while maintaining the existing logic flow.
216-220: Essential security validation correctly placed.The duplicate account check is appropriately positioned after input processing but before proof verification, ensuring that invalid transactions are caught early in the process.
228-348: Step numbering updates maintain clarity.The comment updates correctly reflect the new step sequence after introducing the duplicate account validation step.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests