feat(platform)!: enhanced fetching of compacted address balance changes #2966
Conversation
📝 WalkthroughWalkthroughRemoved a boolean flag from verification APIs and reworked fetching and verification into a two-phase flow: a descending subset query to determine a start key followed by an ascending/proved main query with deduplication and root-hash consistency checks. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Drive
participant ProofStorage
Caller->>Drive: verify_compacted_address_balance_changes_v0(proof, start_height, limit)
Drive->>ProofStorage: verify_subset_query(proof, descending, insert_all)
ProofStorage-->>Drive: subset_results, subset_root
Drive->>Drive: determine start_key from subset_results (or fallback)
Drive->>ProofStorage: verify_main_query(proof, ascending from start_key)
ProofStorage-->>Drive: main_results, main_root
Drive->>Drive: compare subset_root == main_root
alt roots match
Drive-->>Caller: return (root_hash, verified_compacted_changes)
else roots differ
Drive-->>Caller: return proof_corruption_error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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
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 |
QuantumExplorer
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs (1)
218-245: Consider extracting the fallback key creation logic.The code for creating the fallback key
(start_block_height, start_block_height)is repeated three times (lines 229-232, 235-238, 241-244). This violates the DRY principle and makes maintenance more difficult.♻️ Suggested refactor to reduce duplication
+ // Helper to create fallback key + let create_fallback_key = || { + let mut key = Vec::with_capacity(16); + key.extend_from_slice(&start_block_height.to_be_bytes()); + key.extend_from_slice(&start_block_height.to_be_bytes()); + key + }; + // Determine the actual start key for the proved query // If we found a containing range, use its exact key // Otherwise use (start_block_height, start_block_height) since end_block >= start_block always let start_key = if let Some((key, _)) = desc_results.to_key_elements().into_iter().next() { if key.len() == 16 { let end_block = u64::from_be_bytes(key[8..16].try_into().unwrap()); // If this range contains start_block_height, use its exact key if end_block >= start_block_height { key } else { - // No containing range, use (start_block_height, start_block_height) - let mut key = Vec::with_capacity(16); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key + create_fallback_key() } } else { - let mut key = Vec::with_capacity(16); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key + create_fallback_key() } } else { - let mut key = Vec::with_capacity(16); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key + create_fallback_key() };packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs (1)
60-66: Minor: Consider using iterator methods for finding the smallest key.The current loop-based approach for finding the smallest key is correct but could be slightly more idiomatic using iterator methods.
♻️ Optional refactor using iterators
- // Find the first entry (smallest key) from the proof - // If it contains start_block_height, use its exact key - // Otherwise use (start_block_height, start_block_height) since end_block >= start_block always - let mut start_key: Option<Vec<u8>> = None; - for (_path, key, maybe_element) in &subset_results { - if maybe_element.is_some() && key.len() == 16 { - if start_key.is_none() || key < start_key.as_ref().unwrap() { - start_key = Some(key.clone()); - } - } - } + // Find the smallest 16-byte key from the proof + let start_key = subset_results + .iter() + .filter_map(|(_path, key, maybe_element)| { + if maybe_element.is_some() && key.len() == 16 { + Some(key.clone()) + } else { + None + } + }) + .min();
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/rs-drive-proof-verifier/src/proof.rspackages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rspackages/rs-drive/src/drive/saved_block_transactions/store_address_balances/v0/mod.rspackages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/mod.rspackages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
💤 Files with no reviewable changes (2)
- packages/rs-drive-proof-verifier/src/proof.rs
- packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rspackages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rspackages/rs-drive/src/drive/saved_block_transactions/store_address_balances/v0/mod.rs
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: dashpay/platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T13:10:23.481Z
Learning: Use `rs-drive-proof-verifier` for cryptographic proof verification
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
📚 Learning: 2024-10-08T13:28:03.529Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-10-08T13:28:03.529Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
Applied to files:
packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rspackages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-11-20T20:43:41.185Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/tests/strategy_tests/masternodes.rs:212-220
Timestamp: 2024-11-20T20:43:41.185Z
Learning: In `packages/rs-drive-abci/tests/strategy_tests/masternodes.rs`, the pattern of generating a `PrivateKey`, converting it to bytes, and reconstructing a `BlsPrivateKey` from those bytes is intentional. Avoid suggesting to simplify this code in future reviews.
Applied to files:
packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rspackages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rspackages/rs-drive/src/drive/saved_block_transactions/store_address_balances/v0/mod.rs
📚 Learning: 2024-11-22T08:19:14.448Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2345
File: packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:93-99
Timestamp: 2024-11-22T08:19:14.448Z
Learning: In `packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs`, the `insert_contract` method requires an owned `BlockInfo`, so cloning `block_info` is necessary when calling it.
Applied to files:
packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs
📚 Learning: 2024-11-20T16:05:40.200Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs:148-151
Timestamp: 2024-11-20T16:05:40.200Z
Learning: In `packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs`, when converting public keys from `QuorumForSavingV0` to `VerificationQuorum`, it's acceptable to use `.expect()` for public key conversion, as the conversion has been verified and panics are acceptable in this context.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-11-15T14:39:23.704Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2332
File: packages/rs-drive-proof-verifier/src/error.rs:21-23
Timestamp: 2024-11-15T14:39:23.704Z
Learning: In the `packages/rs-drive-proof-verifier`, avoid suggesting size limits for `proof_bytes` in errors unless there's a specific need, as it may not be necessary.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2025-11-25T13:10:23.481Z
Learnt from: CR
Repo: dashpay/platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T13:10:23.481Z
Learning: Use `rs-drive-proof-verifier` for cryptographic proof verification
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-10-03T11:51:06.980Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2201
File: packages/rs-platform-version/src/version/v2.rs:1186-1188
Timestamp: 2024-10-03T11:51:06.980Z
Learning: In the `IdentityTransitionVersions` structure within `packages/rs-platform-version/src/version/v2.rs`, the field `credit_withdrawal` does not need the `identity_` prefix since it is already encompassed within identity state transitions.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-10-06T16:18:07.994Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2215
File: packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs:119-120
Timestamp: 2024-10-06T16:18:07.994Z
Learning: In the `run_block_proposal` function in `packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs`, it's acceptable to pass `platform_state` to `perform_events_on_first_block_of_protocol_change`, even if `block_platform_state` has been updated.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-11-28T13:49:17.301Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2317
File: packages/rs-dapi-client/src/address_list.rs:175-180
Timestamp: 2024-11-28T13:49:17.301Z
Learning: In Rust code in `packages/rs-dapi-client/src/address_list.rs`, do not change the interface of deprecated methods like `add_uri`, even to fix potential panics.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
🧬 Code graph analysis (1)
packages/rs-drive/src/drive/saved_block_transactions/store_address_balances/v0/mod.rs (2)
packages/rs-drive/src/util/test_helpers/setup.rs (1)
setup_drive_with_initial_state_structure(49-63)packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/mod.rs (1)
verify_compacted_address_balance_changes(34-59)
⏰ 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). (12)
- GitHub Check: assign-milestone
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (3)
packages/rs-drive/src/drive/saved_block_transactions/store_address_balances/v0/mod.rs (1)
1094-1220: LGTM! Comprehensive test coverage for the new two-phase retrieval logic.The three new test functions effectively validate:
- Fetching compacted ranges when the start height falls within an existing range
- Proof generation and verification for compacted address balances
- Proof verification when querying from a block height contained within a compacted range
These tests provide solid coverage for the key scenarios introduced by the two-phase query approach.
packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs (1)
19-178: Well-designed two-phase query approach.The implementation correctly handles the complex scenario where a query start height may fall within an existing compacted range. The descending query (phase 1) finds potential containing ranges, and the ascending query (phase 2) fetches subsequent ranges, with proper deduplication logic to avoid returning duplicate entries.
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs (1)
40-90: Robust two-phase verification flow with root hash consistency check.The implementation correctly:
- Performs subset verification to inspect proof contents and determine the actual start key used during proof generation
- Executes main query verification using the discovered start key
- Validates that both verifications produce the same root hash, ensuring proof integrity
This approach properly handles cases where the start height falls within a compacted range, aligning with the two-phase proving logic in the fetch module.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs:
- Around line 56-67: The loop that sets start_key from subset_results must
validate that the candidate key actually relates to start_block_height: parse
the 16-byte key into its (start, end) block numbers and only accept the key when
maybe_element.is_some(), key.len() == 16, and either (start <=
start_block_height && start_block_height <= end) or (start >=
start_block_height); otherwise skip it. Update the logic around start_key
assignment in the loop that iterates over subset_results to perform this numeric
validation before setting start_key so a malicious unrelated range cannot be
chosen.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
🧠 Learnings (9)
📓 Common learnings
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
📚 Learning: 2024-10-08T13:28:03.529Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-10-08T13:28:03.529Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-11-20T16:05:40.200Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs:148-151
Timestamp: 2024-11-20T16:05:40.200Z
Learning: In `packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs`, when converting public keys from `QuorumForSavingV0` to `VerificationQuorum`, it's acceptable to use `.expect()` for public key conversion, as the conversion has been verified and panics are acceptable in this context.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-11-15T14:39:23.704Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2332
File: packages/rs-drive-proof-verifier/src/error.rs:21-23
Timestamp: 2024-11-15T14:39:23.704Z
Learning: In the `packages/rs-drive-proof-verifier`, avoid suggesting size limits for `proof_bytes` in errors unless there's a specific need, as it may not be necessary.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2025-11-25T13:10:23.481Z
Learnt from: CR
Repo: dashpay/platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T13:10:23.481Z
Learning: Use `rs-drive-proof-verifier` for cryptographic proof verification
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-10-03T11:51:06.980Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2201
File: packages/rs-platform-version/src/version/v2.rs:1186-1188
Timestamp: 2024-10-03T11:51:06.980Z
Learning: In the `IdentityTransitionVersions` structure within `packages/rs-platform-version/src/version/v2.rs`, the field `credit_withdrawal` does not need the `identity_` prefix since it is already encompassed within identity state transitions.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-11-20T20:43:41.185Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/tests/strategy_tests/masternodes.rs:212-220
Timestamp: 2024-11-20T20:43:41.185Z
Learning: In `packages/rs-drive-abci/tests/strategy_tests/masternodes.rs`, the pattern of generating a `PrivateKey`, converting it to bytes, and reconstructing a `BlsPrivateKey` from those bytes is intentional. Avoid suggesting to simplify this code in future reviews.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-11-28T13:49:17.301Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2317
File: packages/rs-dapi-client/src/address_list.rs:175-180
Timestamp: 2024-11-28T13:49:17.301Z
Learning: In Rust code in `packages/rs-dapi-client/src/address_list.rs`, do not change the interface of deprecated methods like `add_uri`, even to fix potential panics.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-10-06T16:18:07.994Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2215
File: packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs:119-120
Timestamp: 2024-10-06T16:18:07.994Z
Learning: In the `run_block_proposal` function in `packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs`, it's acceptable to pass `platform_state` to `perform_events_on_first_block_of_protocol_change`, even if `block_platform_state` has been updated.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
🔇 Additional comments (5)
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs (5)
17-34: LGTM - Clear documentation and proper setup.The two-step verification approach is well documented, and the function signature correctly reflects the removal of the
verify_subset_of_proofparameter as intended. Path construction follows existing patterns.
36-54: Subset query approach is sound.Using
insert_all()withverify_subset_queryallows discovering what ranges the prover included, which is essential for the two-phase verification strategy. The big-endian bincode configuration ensures consistent key ordering.
69-91: Root hash consistency check is essential.The fallback key construction correctly uses
(start_block_height, start_block_height)based on the invariant thatend_block >= start_block. The root hash comparison between subset and main verification (lines 87-91) is a critical security measure ensuring both verifications operate on the same tree state.
93-127: Correct parsing and error handling.The key parsing safely uses
.try_into().unwrap()after validatingkey.len() == 16. Error handling is comprehensive with descriptive messages for invalid key length, wrong element type, and deserialization failures.
129-129: Return value is consistent.Returns the verified root hash (validated equal between both verification phases) along with the processed compacted changes.
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs:
- Around line 66-79: The closure passed to results_map.first_key_value() in
verify_compacted_address_balance_changes currently ignores keys with invalid
length by returning None; change it to return a Result so that encountering
key.len() != 16 yields Err(ProofError::CorruptedProof(...)) instead of silently
falling back to the default key. Update the and_then closure to validate key
length, parse start_block/end_block only on valid 16-byte keys, and propagate
the error up so the caller of start_key (and the surrounding function) can
return the CorruptedProof result rather than treating the input as missing.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
🧠 Learnings (14)
📓 Common learnings
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
📚 Learning: 2024-10-08T13:28:03.529Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-10-08T13:28:03.529Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2025-11-25T13:10:23.481Z
Learnt from: CR
Repo: dashpay/platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T13:10:23.481Z
Learning: Use `rs-drive-proof-verifier` for cryptographic proof verification
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-11-15T14:39:23.704Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2332
File: packages/rs-drive-proof-verifier/src/error.rs:21-23
Timestamp: 2024-11-15T14:39:23.704Z
Learning: In the `packages/rs-drive-proof-verifier`, avoid suggesting size limits for `proof_bytes` in errors unless there's a specific need, as it may not be necessary.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-11-22T08:19:14.448Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2345
File: packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:93-99
Timestamp: 2024-11-22T08:19:14.448Z
Learning: In `packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs`, the `insert_contract` method requires an owned `BlockInfo`, so cloning `block_info` is necessary when calling it.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-11-20T16:05:40.200Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs:148-151
Timestamp: 2024-11-20T16:05:40.200Z
Learning: In `packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs`, when converting public keys from `QuorumForSavingV0` to `VerificationQuorum`, it's acceptable to use `.expect()` for public key conversion, as the conversion has been verified and panics are acceptable in this context.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-10-06T16:18:07.994Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2215
File: packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs:119-120
Timestamp: 2024-10-06T16:18:07.994Z
Learning: In the `run_block_proposal` function in `packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs`, it's acceptable to pass `platform_state` to `perform_events_on_first_block_of_protocol_change`, even if `block_platform_state` has been updated.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-10-03T11:51:06.980Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2201
File: packages/rs-platform-version/src/version/v2.rs:1186-1188
Timestamp: 2024-10-03T11:51:06.980Z
Learning: In the `IdentityTransitionVersions` structure within `packages/rs-platform-version/src/version/v2.rs`, the field `credit_withdrawal` does not need the `identity_` prefix since it is already encompassed within identity state transitions.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-11-20T20:43:41.185Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/tests/strategy_tests/masternodes.rs:212-220
Timestamp: 2024-11-20T20:43:41.185Z
Learning: In `packages/rs-drive-abci/tests/strategy_tests/masternodes.rs`, the pattern of generating a `PrivateKey`, converting it to bytes, and reconstructing a `BlsPrivateKey` from those bytes is intentional. Avoid suggesting to simplify this code in future reviews.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2025-10-01T08:37:32.168Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2790
File: packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs:65-0
Timestamp: 2025-10-01T08:37:32.168Z
Learning: In purchase document transitions (packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs), the `changed_data_values` field in `UniquenessOfDataRequestUpdateType::ChangedDocument` should be set to an empty BTreeSet (`Cow::Owned(BTreeSet::new())`) because purchase transitions do not modify document data properties (like "price"), only ownership and transfer metadata. An empty set signals the uniqueness validation logic to skip checking unique indexes on data properties.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-10-18T15:43:32.447Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2254
File: packages/rs-sdk/src/sdk.rs:654-658
Timestamp: 2024-10-18T15:43:32.447Z
Learning: In `packages/rs-sdk/src/sdk.rs`, within the `verify_metadata_height` function, when using `compare_exchange` on `last_seen_height`, it's acceptable to use `Ordering::Relaxed` for the failure ordering, as any inconsistency would only trigger an extra loop iteration without affecting correctness.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2025-02-03T23:39:10.579Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2450
File: packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/v0/methods.rs:10-12
Timestamp: 2025-02-03T23:39:10.579Z
Learning: Block interval calculations in token distribution logic should use checked arithmetic operations (checked_sub, checked_add) to prevent potential overflows, especially when dealing with block heights and intervals.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2025-06-18T03:44:14.385Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2673
File: packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs:1164-1197
Timestamp: 2025-06-18T03:44:14.385Z
Learning: QuantumExplorer determined that a CodeRabbit suggestion about fixing signable_bytes calculation in identity update tests with contract-bound keys was incorrect - the code flow is working as intended without the suggested changes.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
📚 Learning: 2024-10-21T01:03:42.458Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-dpp/src/core_types/validator_set/v0/mod.rs:299-299
Timestamp: 2024-10-21T01:03:42.458Z
Learning: In the `test_serialize_deserialize_validator_set_v0` test within `packages/rs-dpp/src/core_types/validator_set/v0/mod.rs`, deterministic BLS keys cannot be easily used; therefore, using `BlsPublicKey::generate()` is acceptable.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.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). (11)
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (9)
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs (9)
18-24: Documentation clearly explains the two-step verification process.The function documentation accurately describes the subset-then-main verification flow, making the implementation approach clear.
25-30: Good API simplification by removing the boolean flag.The removal of
verify_subset_of_proofstreamlines the API since verification is now consistently two-phase.
40-54: Subset verification step correctly examines proof contents.Using
insert_all()withverify_subset_queryis the appropriate approach to inspect all entries in the proof before determining the actual start key.
56-60: Proper use of BTreeMap for ordered key collection.Collecting results into a BTreeMap ensures proper key ordering, which is essential for finding the first entry that may contain the start block height.
81-89: Fallback key construction is correct and efficient.The deterministic default key using
(start_block_height, start_block_height)with big-endian encoding is semantically correct and consistent with the key format used throughout.
91-96: Main query verification correctly uses the discovered start key.The ascending range query from the discovered start key, followed by
verify_query, correctly implements the second phase of the two-step verification.
98-103: Excellent defensive check for proof consistency.Validating that both the subset and main verification produce the same root hash is a critical security measure that would detect proof tampering or verification logic errors.
105-139: Data decoding logic is thorough and well-validated.The implementation properly validates key lengths, element types, and handles deserialization errors with descriptive error messages. The use of
unwrap()is safe due to prior length checks.
36-38: No action needed. Bincode configuration is already consistent across prove and verify paths.All related operations use identical configuration:
bincode::config::standard().with_big_endian().with_no_limit():
- Encoding (compact_address_balances/v0/mod.rs:54-57)
- Verification (verify_compacted_address_balance_changes/v0/mod.rs:36-38)
- Cleanup (cleanup_expired_address_balances/v0/mod.rs:47-50)
- Fetch (fetch_compacted_address_balances/v0/mod.rs:44-46)
Likely an incorrect or invalid review comment.
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/rs-drive/src/drive/saved_block_transactions/store_address_balances/v0/mod.rs (3)
1094-1138: Good test coverage for the new fetching logic.The test correctly validates that ranges containing the specified start block height are retrieved, which aligns with the PR objectives.
♻️ Minor consistency improvement: use TEST_BLOCK_TIME_MS constant
For consistency with other tests in this file, consider using the
TEST_BLOCK_TIME_MSconstant (defined at line 191) instead of the hardcoded value:drive .store_address_balances_for_block_v0( &balances, block_height, - 1700000000000, + TEST_BLOCK_TIME_MS, None, &platform_version, )
1140-1179: Well-structured proof generation and verification test.The test appropriately validates the cryptographic proof workflow for compacted address balance changes.
♻️ Optional improvements
- For consistency with other tests, consider using the
TEST_BLOCK_TIME_MSconstant:drive .store_address_balances_for_block_v0( &balances, block_height, - 1700000000000, + TEST_BLOCK_TIME_MS, None, &platform_version, )
- Optionally, you could add an assertion to verify the actual balance data in the verified changes:
let (_, _, balances) = &verified_changes[0]; assert_eq!( balances.get(&ADDR_1), Some(&CreditOperation::AddToCredits(4000)), // 1000 * 4 blocks "should have correct merged balance" );
1181-1220: Excellent test for the containing range scenario.This test effectively validates the core enhancement described in the PR: ensuring that ranges containing the specified start block height are correctly retrieved and verified.
♻️ Minor consistency improvement
For consistency with other tests in this file, consider using the
TEST_BLOCK_TIME_MSconstant:drive .store_address_balances_for_block_v0( &balances, block_height, - 1700000000000, + TEST_BLOCK_TIME_MS, None, &platform_version, )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive/src/drive/saved_block_transactions/store_address_balances/v0/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-drive/src/drive/saved_block_transactions/store_address_balances/v0/mod.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
📚 Learning: 2024-11-20T20:43:41.185Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/tests/strategy_tests/masternodes.rs:212-220
Timestamp: 2024-11-20T20:43:41.185Z
Learning: In `packages/rs-drive-abci/tests/strategy_tests/masternodes.rs`, the pattern of generating a `PrivateKey`, converting it to bytes, and reconstructing a `BlsPrivateKey` from those bytes is intentional. Avoid suggesting to simplify this code in future reviews.
Applied to files:
packages/rs-drive/src/drive/saved_block_transactions/store_address_balances/v0/mod.rs
🧬 Code graph analysis (1)
packages/rs-drive/src/drive/saved_block_transactions/store_address_balances/v0/mod.rs (3)
packages/rs-drive/src/drive/initialization/v2/mod.rs (1)
drive(71-114)packages/rs-drive/src/util/test_helpers/setup.rs (1)
setup_drive_with_initial_state_structure(49-63)packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/mod.rs (1)
verify_compacted_address_balance_changes(34-59)
QuantumExplorer
deleted the
featimprove-fetch-compacted-address-balances
branch
Issue being fixed or feature implemented
Improves the logic for fetching compacted address balance changes to ensure that ranges containing the specified start block height are correctly retrieved.
What was done?
end_block >= start_block_height, including ranges that contain or start after the specified height.How Has This Been Tested?
Added unit tests to verify the new fetching logic, ensuring that it correctly retrieves compacted ranges based on various start heights.
Breaking Changes
None
Checklist
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
For repository code-owners and collaborators only
I have assigned this pull request to a milestone
Summary by CodeRabbit
Refactor
Performance/Reliability
Tests
✏️ Tip: You can customize this high-level summary in your review settings.