fix(drive): bind and bound proof decoding - #4165
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds two-part authenticated compacted-balance proofs, bounded decoding for proof-derived data, shared decoder integration across contract and voting verification, and resilience updates for CI, emulator tests, SDK cleanup, and JNI formatting. ChangesProof verification hardening
Build and SDK tooling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Prover
participant GroveDB
participant Verifier
Prover->>GroveDB: Generate predecessor proof
Prover->>GroveDB: Generate forward proof
Prover->>Verifier: Submit encoded proof envelope
Verifier->>Verifier: Verify both proofs and compare roots
Verifier->>Verifier: Decode authenticated balance rows
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
🕓 Ready for review — 32 ahead in queue (commit f1f63eb) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #4165 +/- ##
============================================
- Coverage 87.63% 87.46% -0.17%
============================================
Files 2624 2652 +28
Lines 331678 334936 +3258
============================================
+ Hits 290659 292968 +2309
- Misses 41019 41968 +949
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The proof-envelope and vote-reference changes are sound, but the contract-proof paths now use a decoder with a 15,000-byte resource limit even though the protocol accepts serialized contracts up to 65,000 bytes. This creates a blocking availability regression for valid contracts and also leaves the proof decoder accepting trailing bytes instead of enforcing exact consumption.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/verify/contract/verify_contract/v0/mod.rs`:
- [BLOCKING] packages/rs-drive/src/verify/contract/verify_contract/v0/mod.rs:140-143: Use a protocol-sized, exact-consumption contract decoder
The newly selected `DataContract::versioned_limit_deserialize` helper is incompatible with contracts accepted into authenticated state. It applies `CONTRACT_DESERIALIZATION_LIMIT = 15000`, while all current platform versions set `platform_version.dpp.contract_versions.max_serialized_size` to 65,000 and Drive permits serialized contracts up to that value. Bincode's limit also accounts for nested container allocation claims, so the decoder can reject valid contracts even before their serialized length reaches 15,000 bytes. Such contracts remain valid and stored by Platform but become unavailable through `verify_contract`, `verify_contract_history`, and `verify_contract_return_serialization`, causing a targeted denial of service for proof-verifying clients. The helper also discards the consumed-byte count returned by `borrow_decode_from_slice`, so it accepts a complete contract followed by trailing bytes. Replace these three calls with a proof-specific bounded decoder whose budget is compatible with the protocol-version-specific contract limit and which rejects input unless the decoder consumes the entire byte slice.
…ecode-bounds # Conflicts: # packages/swift-sdk/build_ios.sh
…failure branches Review follow-ups on the proof-decoding hardening: - verify_recent_address_balance_changes (both the inclusive and _after variants) still decoded proof-derived per-block rows with an unlimited bincode budget — the same hostile-length-prefix exposure this branch closes in the compacted verifier. Route both sites through a bounded, exact-consumption row decoder with the same 1 MiB semantic budget, and pin it with a hostile-prefix + trailing-bytes test. - Cover the two untested verifier branches of the compacted two-proof envelope: a predecessor that exists but does not contain the requested height must fall back to the request-derived start key, and envelope halves committing to different roots must be rejected (exercised by splicing proofs generated across a state mutation that leaves the derived start key unchanged, so only the root binding can catch the mix). - The 16 MiB envelope budget's comment claimed to match the DAPI transport limit; no transport tier uses that number (tonic clients default to 4 MiB, the server encodes at most 32 MiB). Reword it as the standalone bound it is. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs (1)
160-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider sharing the bounded row decoder instead of inlining it.
This inline size-guard +
with_limitdecode + trailing-byte check duplicates the pattern already extracted asdecode_address_balance_rowinverify_recent_address_balance_changes/v0/mod.rs(and mirrorsbounded_decode.rs). Since these are security-critical decode limits, keeping duplicate copies risks divergence if one budget changes. A small shared generic helper over the row map type would keep the limits in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs` around lines 160 - 181, The compacted address balance verification path should reuse a shared bounded row decoder instead of duplicating the size guard, limited bincode configuration, and trailing-byte validation. Generalize or reuse decode_address_balance_row from the recent address balance verification module so it supports the BTreeMap<PlatformAddress, BlockAwareCreditOperation> row type, then replace the inline decoding block while preserving its existing corrupted-proof error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/kotlin-sdk-build.yml:
- Around line 175-176: Update the keyguard dismissal sequence around `adb shell
input text` and `KEYCODE_ENTER` so the workflow waits until the bouncer is ready
before injecting the PIN. Move the existing delay before PIN entry or add an
equivalent readiness check, while preserving the subsequent unlock steps.
---
Nitpick comments:
In
`@packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs`:
- Around line 160-181: The compacted address balance verification path should
reuse a shared bounded row decoder instead of duplicating the size guard,
limited bincode configuration, and trailing-byte validation. Generalize or reuse
decode_address_balance_row from the recent address balance verification module
so it supports the BTreeMap<PlatformAddress, BlockAwareCreditOperation> row
type, then replace the inline decoding block while preserving its existing
corrupted-proof error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 378dbf13-55e2-49fe-a4c1-26466f84fdb9
📒 Files selected for processing (15)
.github/workflows/kotlin-sdk-build.yml.github/workflows/tests-rs-workspace.ymlpackages/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/mod.rspackages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rspackages/rs-drive/src/verify/address_funds/verify_recent_address_balance_changes/v0/mod.rspackages/rs-drive/src/verify/bounded_decode.rspackages/rs-drive/src/verify/contract/verify_contract/v0/mod.rspackages/rs-drive/src/verify/contract/verify_contract_history/v0/mod.rspackages/rs-drive/src/verify/contract/verify_contract_return_serialization/v0/mod.rspackages/rs-drive/src/verify/mod.rspackages/rs-drive/src/verify/voting/verify_identity_votes_given_proof/v0/mod.rspackages/rs-drive/src/verify/voting/verify_masternode_vote/v0/mod.rspackages/rs-unified-sdk-jni/src/tx_decode.rspackages/swift-sdk/build_ios.sh
2 participants
Summary
This addresses security review findings DS-CAND-008, 009, 037, 104, and 127 through 130.
Compatibility
The compacted-balance proof payload is now a two-proof envelope. Producers and verifiers should be deployed together; this does not change consensus state or protobuf schema.
Validation
Summary by CodeRabbit
Bug Fixes
Reliability