Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ZCash] Upgrade ZCashOrchardStorage for shard tree purposes #26534

Merged
merged 2 commits into from
Nov 18, 2024
Merged

[ZCash] Upgrade ZCashOrchardStorage for shard tree purposes #26534

merged 2 commits into from
Nov 18, 2024

Conversation

cypt4
Copy link
Collaborator

@cypt4 cypt4 commented Nov 13, 2024 •
edited
Loading

Resolves brave/brave-browser#42281

Part of #26477

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@cypt4 cypt4 requested a review from a team as a code owner November 13, 2024 19:47
@cypt4 cypt4 requested a review from supermassive November 13, 2024 19:47
@cypt4 cypt4 force-pushed the brave_42281 branch 2 times, most recently from eebde04 to 6ce4728 Compare November 14, 2024 08:44
@cypt4 cypt4 changed the title Upgrade ZCashOrchardStorage for shard tree purposes [ZCash] Upgrade ZCashOrchardStorage for shard tree purposes Nov 15, 2024
@cypt4 cypt4 requested a review from supermassive November 18, 2024 09:32
@github-actions GitHub Actions
Copy link
Contributor

[puLL-Merge] - brave/brave-core@26534

Description

This pull request introduces significant changes to the Brave Wallet's ZCash implementation, particularly focusing on the Orchard protocol. The changes include updates to data structures, storage mechanisms, and synchronization processes for ZCash Orchard notes and transactions.

Changes

Changes

  1. components/brave_wallet/browser/internal/orchard_block_scanner.cc:

    • Modified OrchardBlockScanner::Result to use OrchardNoteSpend instead of OrchardNullifier.
    • Updated ScanBlocks method to use OrchardNoteSpend.

  2. components/brave_wallet/browser/zcash/rust/orchard_block_decoder_impl.cc:

    • Updated ScanBlock method to populate more fields in OrchardNote.

  3. components/brave_wallet/browser/zcash/zcash_orchard_storage.cc:

    • Significant changes to the database schema and operations.
    • Added new tables for storing shard trees, checkpoints, and other Orchard-related data.
    • Implemented methods for managing Orchard shards, checkpoints, and tree states.

  4. components/brave_wallet/browser/zcash/zcash_shield_sync_service.cc:

    • Updated account initialization and chain state verification processes.
    • Modified note updating process to use OrchardNoteSpend.

  5. components/brave_wallet/common/zcash_utils.cc and zcash_utils.h:

    • Added new data structures and constants for Orchard protocol.
    • Implemented serialization and deserialization methods for Orchard notes and inputs.

  6. components/services/brave_wallet/public/mojom/zcash_decoder.mojom:

    • Added a new SubtreeRoot structure.

Possible Issues

  1. The changes to the database schema may require careful migration handling for existing users.
  2. The introduction of new data structures and methods might require updates to other parts of the codebase that interact with ZCash functionality.

Security Hotspots

  1. The implementation of ResetAccountSyncState in zcash_orchard_storage.cc clears sensitive data. Ensure that this operation is performed securely and that no residual data remains.
  2. The handling of spending keys and full view keys in OrchardSpendsBundle should be reviewed to ensure proper security measures are in place for these sensitive cryptographic materials.

These changes appear to be a significant update to the ZCash Orchard implementation in Brave Wallet, introducing more comprehensive support for the protocol's features and improving the overall structure of the related codebase.

supermassive
supermassive reviewed Nov 18, 2024
View reviewed changes
components/brave_wallet/common/zcash_utils.cc Outdated
Comment on lines 111 to 113
// static

// static
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: static

supermassive
supermassive reviewed Nov 18, 2024
View reviewed changes
components/brave_wallet/common/zcash_utils.cc Outdated
Comment on lines 117 to 124
dict.Set("addr", base::HexEncode(addr.data(), addr.size()));
dict.Set("block_id", base::NumberToString(block_id));
dict.Set("nullifier", base::HexEncode(nullifier.data(), nullifier.size()));
dict.Set("amount", base::NumberToString(amount));
dict.Set("orchard_commitment_tree_position",
base::NumberToString(orchard_commitment_tree_position));
dict.Set("rho", base::HexEncode(rho.data(), rho.size()));
dict.Set("seed", base::HexEncode(seed.data(), seed.size()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does short version like base::HexEncode(smth) work?

supermassive
supermassive approved these changes Nov 18, 2024
View reviewed changes
Copy link
Collaborator

@supermassive supermassive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wallet core lgtm

@cypt4 cypt4 merged commit 90324f5 into master Nov 18, 2024
19 checks passed
@cypt4 cypt4 deleted the brave_42281 branch November 18, 2024 12:47
@github-actions github-actions bot added this to the 1.75.x - Nightly milestone Nov 18, 2024
@brave-builds
Copy link
Collaborator

Released in v1.75.28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@supermassive supermassive supermassive approved these changes

Assignees

@cypt4 cypt4

Labels
feature/web3/wallet/core feature/web3/wallet puLL-Merge
Projects
None yet
Milestone
1.75.x - Beta
Development

Successfully merging this pull request may close these issues.

[ZCash] ZCashOrchardStorage upgrade for shard tree
3 participants
@cypt4 @brave-builds @supermassive