[ZCash] Implement Zcash sync process and Orchard inputs spending #27018

cypt4 · 2024-12-13T20:16:52Z

Sec review: https://github.com/brave/reviews/issues/1834

Implements basic sync process for the Orchard protocol.
Entry point for the sync process is the per-account ZCashShieldSyncService class.
Sync process includes:

Verifying chain state. In some cases chain reorg may happen so we need to check whether latest scanned block hash hasn't been changed. Otherwise we need to clean OrchardStorage to remove notes, nullifiers and commitments that are not valid.
Preparing a set of scan ranges to scan. Each scan range ends with updating of OrchardSyncState with the discovered data.
Scanning ranges one-by-one. This includes reading compact blocks from the gRPC node and extracting related incoming notes and nullifiers for spends along with a set of commitment tree leafs to be inserted to the shard tree in the future. Extracting is done on a separate sequence. This is done using OrchardBlockScanner class.
Inserting extracted leafs to the shard tree(OrchardSyncState::ApplyScanResults).

ZCashCreateOrchardToOrchardTransactionTask then could be used to construct shielded transaction.
ZCashCompleteTransactionTask completes transactions by calculating witness for every shielded input and signing the transaction.

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:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

brave-builds · 2024-12-13T21:26:20Z

A Storybook has been deployed to preview UI for the latest push

components/brave_wallet/browser/zcash/zcash_tx_manager.cc

components/brave_wallet/browser/zcash/zcash_scan_blocks_task.h

components/brave_wallet/browser/zcash/zcash_rpc.cc

components/brave_wallet/browser/internal/orchard_sync_state.cc

brave/brave-browser#42898

Douglashdaniel

++ Wallet Front-end

components/brave_wallet/browser/zcash/zcash_blocks_batch_scan_task.cc

supermassive · 2025-01-08T08:28:38Z

components/brave_wallet/browser/zcash/zcash_blocks_batch_scan_task.cc

+    auto frontier_bytes = PrefixedHexStringToBytes(
+        base::StrCat({"0x", frontier_tree_state_.value()->orchardTree}));


It looks like it should be HexStringToBytes(frontier_tree_state_.value()->orchardTree)

PrefixedHexStringToBytes is a bit more complicated: for ex. it appends 0 if input size is odd. I'd prefer to use it instead.

PrefixedHexStringToBytes is for EVM's encoding of integers. It is not supposed to be used unless it is explictly required by format of hex string.
As afar as I can see orchardTree is just hex encoded bytes, so decoding to bytes should be done with HexStringToBytes and fail in case of odd size

supermassive · 2025-01-08T08:31:14Z

components/brave_wallet/browser/zcash/zcash_rpc.cc

  GURL request_url = MakeGetLightdInfoURL(GetNetworkURL(chain_id));



Is it unintentinonal removal?

supermassive · 2025-01-08T08:41:09Z

components/brave_wallet/browser/zcash/zcash_scan_blocks_task.cc

+  initial_ranges_count_ =
+      std::ceil(static_cast<double>((to - from + 1)) / kBatchSize);
+  scan_ranges_ = std::deque<ScanRange>();


Can it be rewritten somehow so it's easier to track if always to >= form?

supermassive · 2025-01-08T08:44:44Z

components/brave_wallet/browser/zcash/zcash_shield_sync_service.cc

+    case ZCashShieldSyncService::ErrorCode::kFailedToVerifyChainState:
      return 7;
+    case ZCashShieldSyncService::ErrorCode::kFailedToUpdateSubtreeRoots:
+      return 8;
+    case ZCashShieldSyncService::ErrorCode::kDatabaseError:
+      return 9;
+    case ZCashShieldSyncService::ErrorCode::kScannerError:
+      return 10;
  }


would that be enough to just return static_cast<size_t>(error); ?
And why it returns size_t?

It was enum class, reworked using simple enum

supermassive · 2025-01-08T08:46:54Z

components/brave_wallet/browser/zcash/zcash_shield_sync_service.cc

    return;
+  } else {
+    verify_chain_state_task_.reset();


It seems verify_chain_state_task_.reset(); should be done with chain_state_verified_ = true

supermassive · 2025-01-08T08:51:25Z

components/brave_wallet/browser/zcash/zcash_verify_chain_state_task.h

+
+namespace brave_wallet {
+
+class ZCashVerifyChainStateTask {


A brief comment would be helpful for all such task classes

components/brave_wallet/browser/zcash/zcash_verify_chain_state_task.cc

supermassive · 2025-01-08T09:02:33Z

components/brave_wallet/browser/zcash/zcash_create_transparent_to_orchard_transaction_task.h

  OrchardAddrRawPart receiver_;
  std::optional<OrchardMemo> memo_;
  uint64_t amount_;

+  bool started_ = false;


Why we need these started_ flags for tasks?

Just to ensure that Start is called only once

supermassive · 2025-01-08T09:05:06Z

components/brave_wallet/browser/zcash/zcash_create_orchard_to_orchard_transaction_task.cc

+  orchard_output.value = zcash_transaction.TotalInputsAmount() -
+                         zcash_transaction.fee() - pick_result->change;
+  orchard_output.addr = receiver_;


Needs some explanation or guards so this does not overflow

To avoid copy paste that should be based on base::CheckedNumeric

github-actions · 2025-01-08T20:19:40Z

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

Description

This PR introduces significant changes to the Brave Wallet's ZCash implementation, particularly focusing on the Orchard protocol and shielded transactions. The changes include new task classes for handling various ZCash operations, improvements to the transaction creation and signing process, and updates to the user interface for ZCash-related features.

Possible Issues

The complexity of the ZCash implementation has increased, which may make future maintenance and debugging more challenging.
The changes to the transaction creation process may require thorough testing to ensure compatibility with existing wallet functions.

Security Hotspots

The ZCashCompleteTransactionTask class handles sensitive operations like signing transactions. Ensure that proper key management and secure signing practices are implemented.
The ZCashScanBlocksTask processes blockchain data. Validate that it properly handles potentially malicious or corrupted block data.
The ApplyOrchardSignatures function in zcash_complete_transaction_task.cc is executed on a background thread. Ensure that this doesn't introduce any race conditions or thread safety issues.

Changes

Changes

components/brave_wallet/browser/zcash/zcash_wallet_service.h/.cc:
- Added new methods for creating Orchard-to-Orchard and Transparent-to-Orchard transactions.
- Implemented chain tip status retrieval and sync state reset functionality.
components/brave_wallet/browser/zcash/zcash_blocks_batch_scan_task.h/.cc:
- New class for batch scanning of ZCash blocks.
components/brave_wallet/browser/zcash/zcash_scan_blocks_task.h/.cc:
- New class for managing the overall block scanning process.
components/brave_wallet/browser/zcash/zcash_complete_transaction_task.h/.cc:
- New class for completing ZCash transactions, including signing and Orchard part generation.
components/brave_wallet/browser/zcash/zcash_create_orchard_to_orchard_transaction_task.h/.cc:
- New class for creating transactions within the Orchard pool.
components/brave_wallet/common/brave_wallet.mojom:
- Updated ZCash-related structs and interfaces to support new functionality.
components/brave_wallet_ui/:
- Updated UI components to support new ZCash features and transaction types.

sequenceDiagram
    participant User
    participant UI
    participant ZCashWalletService
    participant ZCashScanBlocksTask
    participant ZCashBlocksBatchScanTask
    participant ZCashCompleteTransactionTask
    participant Blockchain

    User->>UI: Initiate ZCash operation
    UI->>ZCashWalletService: Request operation
    ZCashWalletService->>ZCashScanBlocksTask: Start block scanning
    loop For each batch
        ZCashScanBlocksTask->>ZCashBlocksBatchScanTask: Scan batch
        ZCashBlocksBatchScanTask->>Blockchain: Fetch blocks
        Blockchain-->>ZCashBlocksBatchScanTask: Return block data
        ZCashBlocksBatchScanTask-->>ZCashScanBlocksTask: Return batch result
    end
    ZCashScanBlocksTask-->>ZCashWalletService: Scanning complete
    ZCashWalletService->>ZCashCompleteTransactionTask: Complete transaction
    ZCashCompleteTransactionTask->>Blockchain: Submit transaction
    Blockchain-->>ZCashCompleteTransactionTask: Transaction result
    ZCashCompleteTransactionTask-->>ZCashWalletService: Operation result
    ZCashWalletService-->>UI: Operation complete
    UI-->>User: Display result

supermassive · 2025-01-15T03:28:24Z

components/brave_wallet/browser/zcash/zcash_create_transparent_to_orchard_transaction_task.cc

@@ -57,13 +70,15 @@ void ZCashCreateShieldTransactionTask::WorkOnTask() {

  if (!transaction_ && !CreateTransaction()) {


why transaction_ is checked here?

supermassive · 2025-01-15T03:44:43Z

components/brave_wallet/browser/zcash/zcash_wallet_service.cc

-          std::move(start_address), std::move(callback)));
-  task->ScheduleWorkOnTask();
+  auto task = base::MakeRefCounted<ZCashDiscoverNextUnusedZCashAddressTask>(
+      base::PassKey<class ZCashWalletService>(), weak_ptr_factory_.GetWeakPtr(),


don't need class here most likely

github-actions bot assigned cypt4 Dec 13, 2024

github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet feature/web3/wallet/core labels Dec 13, 2024

cypt4 force-pushed the brave_42898 branch 2 times, most recently from 73a1e11 to 21e1e2d Compare December 16, 2024 21:15

cypt4 changed the title ~~Implement Zcash sync process~~ [ZCash] Implement Zcash sync process and Orchard inputs spending Dec 16, 2024

supermassive reviewed Dec 19, 2024

View reviewed changes

components/brave_wallet/browser/zcash/zcash_tx_manager.cc Outdated Show resolved Hide resolved

supermassive reviewed Dec 19, 2024

View reviewed changes

components/brave_wallet/browser/zcash/zcash_scan_blocks_task.h Outdated Show resolved Hide resolved

supermassive reviewed Dec 19, 2024

View reviewed changes

components/brave_wallet/browser/zcash/zcash_rpc.cc Outdated Show resolved Hide resolved

supermassive reviewed Dec 19, 2024

View reviewed changes

components/brave_wallet/browser/internal/orchard_sync_state.cc Outdated Show resolved Hide resolved

cypt4 added 2 commits December 30, 2024 23:17

Implement Zcash sync process and Orchard inputs spending

1b7254c

brave/brave-browser#42898

Review fix and tests

ac5e911

cypt4 force-pushed the brave_42898 branch from fcabd5a to ac5e911 Compare December 30, 2024 21:44

cypt4 marked this pull request as ready for review December 30, 2024 21:45

cypt4 requested review from a team as code owners December 30, 2024 21:45

Build fix

efe7414

github-actions bot added the puLL-Merge label Jan 3, 2025

Douglashdaniel approved these changes Jan 7, 2025

View reviewed changes

supermassive reviewed Jan 8, 2025

View reviewed changes

components/brave_wallet/browser/zcash/zcash_blocks_batch_scan_task.cc Show resolved Hide resolved

supermassive reviewed Jan 8, 2025

View reviewed changes

components/brave_wallet/browser/zcash/zcash_verify_chain_state_task.cc Show resolved Hide resolved

supermassive reviewed Jan 8, 2025

View reviewed changes

Build fix & tests

a10fa33

cypt4 force-pushed the brave_42898 branch from e377474 to a10fa33 Compare January 8, 2025 20:18

Fix iOS build

61554b5

cypt4 requested a review from supermassive January 9, 2025 10:20

supermassive reviewed Jan 15, 2025

View reviewed changes

This was referenced Jan 15, 2025

[ZCash] Implement Zcash sync process and Orchard inputs spending # 1 brave/brave-browser#43312

Open

[ZCash] Implement ShardTree sync process #27255

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ZCash] Implement Zcash sync process and Orchard inputs spending #27018

[ZCash] Implement Zcash sync process and Orchard inputs spending #27018

cypt4 commented Dec 13, 2024 •

edited

Loading

brave-builds commented Dec 13, 2024

Douglashdaniel left a comment

supermassive Jan 8, 2025

cypt4 Jan 8, 2025

supermassive Jan 15, 2025

supermassive Jan 8, 2025

cypt4 Jan 8, 2025

supermassive Jan 8, 2025

cypt4 Jan 8, 2025

supermassive Jan 8, 2025

cypt4 Jan 8, 2025

supermassive Jan 8, 2025

cypt4 Jan 8, 2025

supermassive Jan 8, 2025

cypt4 Jan 8, 2025

supermassive Jan 8, 2025

cypt4 Jan 8, 2025

supermassive Jan 8, 2025

cypt4 Jan 8, 2025

supermassive Jan 15, 2025

github-actions bot commented Jan 8, 2025

Changes

supermassive Jan 15, 2025

supermassive Jan 15, 2025

		auto frontier_bytes = PrefixedHexStringToBytes(
		base::StrCat({"0x", frontier_tree_state_.value()->orchardTree}));

		GURL request_url = MakeGetLightdInfoURL(GetNetworkURL(chain_id));

		@@ -57,13 +70,15 @@ void ZCashCreateShieldTransactionTask::WorkOnTask() {

		if (!transaction_ && !CreateTransaction()) {

[ZCash] Implement Zcash sync process and Orchard inputs spending #27018

Are you sure you want to change the base?

[ZCash] Implement Zcash sync process and Orchard inputs spending #27018

Conversation

cypt4 commented Dec 13, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Dec 13, 2024

Douglashdaniel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 8, 2025

Description

Possible Issues

Security Hotspots

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypt4 commented Dec 13, 2024 •

edited

Loading