Repository navigation
perf(drive-abci): read shielded encrypted notes in one chunk-aligned range read - #5030
Merged
Merged
Conversation
…range read The non-proved branch of getShieldedEncryptedNotes called commitment_tree_get_value once per position. For a position inside a compacted chunk that call reads and deserializes the whole chunk blob (2048 entries) to return one entry, so a single page of up to max_query_chunks x 2048 notes deserialized each chunk once per note. The branch now makes one commitment_tree_get_range call, which reads and deserializes each chunk the page overlaps once. Validation, the note mapping, and the stop at the end of the tree are unchanged, so every request on healthy state gets the same response as before. A new test inserts one compacted chunk plus five buffered notes and checks nine pages against the old per-position read. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reachedNext included review available in 50 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
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 |
Collaborator
|
🔍 Review in progress — actively reviewing now (commit 24efe07) · triage: low |
4 of 12 tasks
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
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.
Basic explanation
What this does: A wallet that holds shielded funds finds its notes by asking a node for pages of encrypted notes and trying to decrypt each one. The node stores finished notes in bundles of 2048. To answer one page, the old code unpacked a whole bundle again for every single note it returned, so a full page unpacked the same bundle up to 2048 times. The node now unpacks each bundle once per page.
Value: Serving a page gets much cheaper. In a debug build, reading 8192 notes went from 64 s to under 3 s. Wallets scanning for shielded funds get answers sooner, and nodes spend far less CPU on this request.
Risks: Low. Wallets get exactly the same answer as before, and nothing here touches consensus, so nodes cannot disagree because of it. The one visible difference is on a node whose database is already damaged: it now returns an error instead of a quietly shortened page. The new test is slow (about a minute in a debug build) because it reruns the old note-by-note read to compare against.
Issue being fixed or feature implemented
The non-proved branch of
getShieldedEncryptedNotesread one note at a time withcommitment_tree_get_value. For a position inside a compacted chunk, that call reads and deserializes the whole chunk blob (2048 entries of about 344 bytes) to return a single entry. A page can hold up tomax_query_chunks × 2048notes (8192 withDRIVE_ABCI_QUERY_VERSIONSv1), so serving one page deserialized each chunk it covered once per note in it.Measured in a debug build on a pool of 8192 notes: 64 s reading one position at a time, against 2.9 s for a full walk with the range read (and that walk also inserted every note).
What was done?
The non-proved branch in
packages/rs-drive-abci/src/query/shielded/encrypted_notes/v0/mod.rsnow makes onecommitment_tree_get_range(pool_path, &[SHIELDED_NOTES_KEY], start_index, limit, ..)call. GroveDB's range read is chunk-aligned: it reads and deserializes each chunk the page overlaps once, then reads the buffered notes one by one.Unchanged:
start_indexmust be chunk-aligned,count == 0orcount > maxbecomesmax, and the limit is clamped tou16.cmx 32 || rho 32 || cv_net 32 || encrypted_note rest) becomes anEncryptedNoteexactly as before, still stopping at the first value of 96 bytes or less.get_rangealready clamps to the tree'stotal_count.Example, on a pool holding 2048 + 5 notes (one compacted chunk plus five buffered notes):
Notes for reviewers
The response is the same for every request on healthy state. It differs only in cases a valid request on healthy state cannot reach:
start_indexnearu64::MAX: the old loop'sstart_index + limitpanicked on overflow in debug builds and gave an empty page in release. The range read saturates, so the page is empty in both.Queries are not consensus, so there is no protocol version change.
How Has This Been Tested?
New test
test_v0_range_read_matches_per_position_reads_across_chunk_and_buffer:commitment_tree_get_valueloop to build a reference, and checks every note's cmx, rho and cv_net against its tags.The test takes about 60 s in a debug build. Nearly all of that is the one-at-a-time reference walk over the compacted chunk, which is the cost this PR removes from the query.
Ran locally:
cargo test -p drive-abci --lib query::shielded: 37 passed, 0 failedcargo clippy -p drive-abci --all-targets -- -D warnings: cleanBreaking Changes
None. Query responses are unchanged, and no consensus code or protocol version is touched.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
24efe07/skip-botsproceeds without the ones not yet reported/self-reviewedonce the bots are doneWhen every box is checked the
PR Hygienecheck passes and this can merge.