[ZCash ] Implement shard tree and shard storage #26477

cypt4 · 2024-11-11T17:24:48Z

Resolves brave/brave-browser#39314

Crate pr : #26474

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:

bridiver · 2024-11-13T17:47:58Z

components/brave_wallet/browser/BUILD.gn

@@ -312,10 +312,14 @@ static_library("browser") {

  if (enable_orchard) {
    sources += [
+      "zcash/orchard_shard_tree_delegate_impl.cc",


these really need to move into their own BUILD.gn file in zcash directory

meaning all of the files here, not just the new ones

bridiver · 2024-11-13T17:56:14Z

components/brave_wallet/browser/zcash/rust/BUILD.gn

@@ -57,6 +70,17 @@ source_set("orchard_impl") {
  ]
 }

+source_set("shard_store") {
+  visibility = [ ":*" ]
+  sources = [ "cxx/src/shard_store.h" ]


we don't want to use cxx directories anymore and it's not clear to me why this would go somewhere other than the existing ochrard_impl target. In a separate PR we should move all the files in orchard_impl into an internal directory

Also generally speaking you shouldn't create a target in one directory that includes files in a subdirectory, especially if those are the only files it includes.

it's not clear to me why this would go somewhere other than the existing ochrard_impl target.

There will be cycle dependency otherwise rust_lib depends on shard_store.h and orchard_impl depends on rust_lib

I would use qr_code_generator as an example to follow for this which would probably mean shard_store.h moves into this directory.

it's a bit confusing that this directory is called rust and also contains cpp files, but we should probably rename it in a separate PR

Moved to the zcash/rust directory

components/brave_wallet/browser/zcash/rust/orchard_shard_tree_impl.cc

components/brave_wallet/browser/zcash/rust/cxx/src/shard_store.h

bridiver · 2024-11-13T18:16:00Z

components/brave_wallet/common/zcash_utils.h

+// Note witness is a Merkle path in the Orchard commitment tree from the
+// note to the tree root according some selected anchor(selected right border in
+// the commitment tree).
+struct OrchardNoteWitness {


This entire file is a problem, we should not just be dumping a bunch of structs in here like this. You don't necessarily need a file for each, but they should be organized in some sensible way because this file is a mess

it seems like these probably belong with https://github.com/brave/brave-core/pull/26477/files#r1840956570

bridiver · 2024-11-13T18:16:11Z

components/brave_wallet/common/zcash_utils.h

+  std::vector<uint8_t> frontier;
+};
+
+class OrchardShardTreeDelegate {


This definitely should go in its own file

Moved to separate file

components/brave_wallet/common/zcash_utils.h

components/brave_wallet/browser/zcash/rust/lib.rs

Resolves brave/brave-browser#39314

bridiver · 2024-11-20T00:43:35Z

components/brave_wallet/browser/zcash/rust/orchard_decoded_blocks_bunde.h

+
+namespace brave_wallet::orchard {
+
+class OrchardDecodedBlocksBundle {


please provide some comments for these new classes and also the need unit tests

bridiver · 2024-11-20T00:44:22Z

components/brave_wallet/browser/zcash/rust/orchard_decoded_blocks_bunde_impl.cc

+OrchardDecodedBlocksBundleImpl::GetDiscoveredNotes() {
+  std::vector<OrchardNote> result;
+
+  for (size_t i = 0; i < batch_decode_result_->size(); i++) {


How big could this list be? Is there an upper bound?

We should be calling result.reserve(batch_decode_result_->size()) before looping.

Shouldn't be too big, this is sort of incoming transactions, in most cases it should be less than 10, i guess

bridiver · 2024-11-20T00:51:19Z

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

-          orchard_action->ciphertext.size() != kOrchardCipherTextSize) {
-        return std::nullopt;
-      }
+std::unique_ptr<OrchardDecodedBlocksBundle> OrchardBlockDecoderImpl::ScanBlocks(


unit tests?

components/brave_wallet/browser/zcash/rust/orchard_shard_tree_impl.h

bridiver · 2024-11-25T20:34:13Z

components/brave_wallet/common/orchard_shard_tree_delegate.cc

+
+namespace brave_wallet {
+
+OrchardTreeState::OrchardTreeState() = default;


this does not belong in common. Only code used in both browser and renderer process belongs in common

Moved to orchard_types

cdesouza-chromium · 2024-11-27T16:41:53Z

components/brave_wallet/browser/internal/orchard_shard_tree_manager.cc

+OrchardShardTreeManager::CalculateWitness(
+    const std::vector<OrchardInput>& notes,
+    uint32_t checkpoint_position) {
+  std::vector<OrchardInput> result;


result.resever(notes.size());

cdesouza-chromium · 2024-11-27T16:45:46Z

components/brave_wallet/browser/internal/orchard_shard_tree_manager.cc

+      return base::unexpected(witness.error());
+    }
+    result.push_back(input);
+    result.back().witness = witness.value();


std::move

cdesouza-chromium · 2024-11-27T17:02:32Z

components/brave_wallet/browser/internal/orchard_shard_tree_manager.h

+  static std::unique_ptr<OrchardShardTreeManager> Create(
+      std::unique_ptr<OrchardShardTreeDelegate> delegate);
+
+  // Creates shard tree size of 8 for testing.
+  static std::unique_ptr<OrchardShardTreeManager> CreateForTesting(
+      std::unique_ptr<OrchardShardTreeDelegate> delegate);


Do we really need these factory functions? Do you think it would be helpful if we simplified instantiation by just having it happening in place, rather than having this hidden behind this class?

This file was deleted

cdesouza-chromium · 2024-11-27T17:04:33Z

components/brave_wallet/browser/internal/orchard_shard_tree_manager_unittest.cc

+  auto result = CreateResultForTesting(prior_tree_state, commitments);
+  tree_manager()->InsertCommitments(std::move(result));
+
+  {


is there a particular reason to have these blocks at the end of the test?

cdesouza-chromium · 2024-11-27T17:06:21Z

components/brave_wallet/browser/internal/orchard_shard_tree_manager_unittest.cc

+      if (i == 2) {
+        commitments.push_back(CreateCommitment(
+            CreateMockCommitmentValue(i, kDefaultCommitmentSeed), true,
+            std::nullopt));
+      } else if (i == 3) {
+        commitments.push_back(CreateCommitment(
+            CreateMockCommitmentValue(i, kDefaultCommitmentSeed), false, 1));
+      } else if (i == 5) {
+        commitments.push_back(CreateCommitment(
+            CreateMockCommitmentValue(i, kDefaultCommitmentSeed), false, 2));
+      } else {


A switch will make this terser and easier to read, don't you think?

components/brave_wallet/browser/internal/orchard_storage/orchard_shard_tree_delegate.h

cdesouza-chromium · 2024-11-27T17:13:12Z

components/brave_wallet/browser/internal/orchard_storage/orchard_shard_tree_delegate.h

+  ~OrchardShardTreeDelegate();
+
+  base::expected<std::optional<OrchardShardTreeCap>, Error> GetCap() const;
+  base::expected<bool, Error> PutCap(const OrchardShardTreeCap& cap);


There's an idiom for when you want to signal failure, but you don't have an error code:

base::expected<T, void>

I'm bringing this up because we have an Error enum with a single value. I think using void for type would better communicate the fact that we are passing an error, but we don't have anything useful about the error.

This file was deleted

cdesouza-chromium · 2024-11-27T17:18:01Z

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

+        base::ranges::copy(orchard_action->nullifier,
+                           orchard_compact_action.nullifier.begin());
+        base::ranges::copy(orchard_action->cmx,
+                           orchard_compact_action.cmx.begin());
+        base::ranges::copy(orchard_action->ephemeral_key,
+                           orchard_compact_action.ephemeral_key.begin());
+        base::ranges::copy(orchard_action->ciphertext,
+                           orchard_compact_action.enc_cipher_text.begin());


These copies are supposed to be something like:

base::span(orchard_compact_action.nullifier).copy_from(orchard_action->nullifier); base::span(orchard_compact_action.cmx).copy_from(orchard_action->cmx); base::span(orchard_compact_action.ephemeral_key).copy_from(orchard_action->ephemeral_key); base::span(orchard_compact_action.enc_cipher_text).copy_from(orchard_action->ciphertext);

cypt4 requested a review from a team as a code owner November 11, 2024 17:24

github-actions bot assigned cypt4 Nov 11, 2024

github-actions bot added feature/web3/wallet feature/web3/wallet/core labels Nov 11, 2024

cypt4 force-pushed the shard_crate_impl branch from 9192b28 to a3ccf60 Compare November 11, 2024 18:53

cypt4 requested review from fmarier, a team and bridiver as code owners November 11, 2024 18:53

cypt4 force-pushed the shard_tree_impl_1 branch 3 times, most recently from 2eb3bc5 to 2d67e9d Compare November 11, 2024 20:18

cypt4 changed the title ~~Implelent shard tree and shard storage~~ [ZCash ] Implement shard tree and shard storage Nov 11, 2024

cypt4 force-pushed the shard_crate_impl branch from a3ccf60 to 284506d Compare November 12, 2024 16:52

cypt4 force-pushed the shard_tree_impl_1 branch from 2d67e9d to 6f8de17 Compare November 13, 2024 15:32

bridiver reviewed Nov 13, 2024

View reviewed changes

components/brave_wallet/browser/zcash/rust/orchard_shard_tree_impl.cc Outdated Show resolved Hide resolved

bridiver reviewed Nov 13, 2024

View reviewed changes

components/brave_wallet/browser/zcash/rust/cxx/src/shard_store.h Outdated Show resolved Hide resolved

bridiver reviewed Nov 13, 2024

View reviewed changes

components/brave_wallet/common/zcash_utils.h Outdated Show resolved Hide resolved

cypt4 mentioned this pull request Nov 13, 2024

[ZCash] Upgrade ZCashOrchardStorage for shard tree purposes #26534

Merged

24 tasks

antonok-edm reviewed Nov 15, 2024

View reviewed changes

components/brave_wallet/common/zcash_utils.h Outdated Show resolved Hide resolved

components/brave_wallet/browser/zcash/rust/lib.rs Outdated Show resolved Hide resolved

cypt4 added 4 commits November 18, 2024 23:53

Implelent shard tree and shard storage

7974d84

Resolves brave/brave-browser#39314

Rename ShardStoreContext

8e6b0c9

Switch to using class instead of functions in lib.rs

529032d

Get rid of clone in impl_result::unwrap

9e7ed49

cypt4 force-pushed the shard_tree_impl_1 branch from 3964309 to 9e7ed49 Compare November 18, 2024 19:54

cypt4 requested a review from bsclifton as a code owner November 18, 2024 19:54

bridiver reviewed Nov 20, 2024

View reviewed changes

cypt4 force-pushed the shard_tree_impl_1 branch 2 times, most recently from 80425d6 to c754ce2 Compare November 20, 2024 20:30

cypt4 added 2 commits November 21, 2024 00:36

Review fix #1

2b8843c

Review fix #2

67584a9

cypt4 force-pushed the shard_tree_impl_1 branch 2 times, most recently from 443fd78 to b801c0f Compare November 22, 2024 16:17

cypt4 added 2 commits November 25, 2024 15:28

Review fix#3

afba5d2

Use result wrappers for cpp->rust callbacks

48bb0c5

cypt4 force-pushed the shard_tree_impl_1 branch from b801c0f to 48bb0c5 Compare November 25, 2024 19:46

bridiver reviewed Nov 25, 2024

View reviewed changes

components/brave_wallet/browser/zcash/rust/orchard_shard_tree_impl.h Show resolved Hide resolved

bridiver reviewed Nov 25, 2024

View reviewed changes

antonok-edm approved these changes Nov 25, 2024

View reviewed changes

cypt4 added 3 commits November 26, 2024 19:39

Add macro for rust result wrappers

f4eef33

Get rid of OrchardShardTreeDelegateImpl

16b618d

Rename ShardTreeDelegate

0c59073

cypt4 force-pushed the shard_tree_impl_1 branch from a189121 to 0c59073 Compare November 27, 2024 16:08

cdesouza-chromium reviewed Nov 27, 2024

View reviewed changes

components/brave_wallet/browser/internal/orchard_storage/orchard_shard_tree_delegate.h Outdated Show resolved Hide resolved

cdesouza-chromium reviewed Nov 27, 2024

View reviewed changes

cypt4 added 3 commits November 27, 2024 22:40

Merge OrchardShardTreeManager to ZCashOrchardSyncState

a3622f1

Remove OrchardShardTreeDelegate

8a32651

Review fix

3bbf4fb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ZCash ] Implement shard tree and shard storage #26477

[ZCash ] Implement shard tree and shard storage #26477

cypt4 commented Nov 11, 2024 •

edited

Loading

bridiver Nov 13, 2024

bridiver Nov 13, 2024

bridiver Nov 13, 2024

cypt4 Nov 15, 2024

bridiver Nov 20, 2024

bridiver Nov 20, 2024

cypt4 Nov 28, 2024

bridiver Nov 13, 2024 •

edited

Loading

bridiver Nov 13, 2024

cypt4 Nov 21, 2024

bridiver Nov 13, 2024

cypt4 Nov 20, 2024

bridiver Nov 20, 2024

bridiver Nov 20, 2024

cdesouza-chromium Nov 20, 2024

cypt4 Nov 21, 2024

bridiver Nov 20, 2024

bridiver Nov 25, 2024

cypt4 Nov 28, 2024

cdesouza-chromium Nov 27, 2024

cypt4 Nov 28, 2024

cdesouza-chromium Nov 27, 2024

cypt4 Nov 28, 2024

cdesouza-chromium Nov 27, 2024

cypt4 Nov 28, 2024

cdesouza-chromium Nov 27, 2024

cypt4 Nov 28, 2024

cdesouza-chromium Nov 27, 2024

cypt4 Nov 28, 2024

cdesouza-chromium Nov 27, 2024 •

edited

Loading

cypt4 Nov 28, 2024

cdesouza-chromium Nov 27, 2024

cypt4 Nov 28, 2024


		namespace brave_wallet::orchard {

		class OrchardDecodedBlocksBundle {


		namespace brave_wallet {

		OrchardTreeState::OrchardTreeState() = default;

[ZCash ] Implement shard tree and shard storage #26477

Are you sure you want to change the base?

[ZCash ] Implement shard tree and shard storage #26477

Conversation

cypt4 commented Nov 11, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

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

bridiver Nov 13, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdesouza-chromium Nov 27, 2024 • edited Loading

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

cypt4 commented Nov 11, 2024 •

edited

Loading

bridiver Nov 13, 2024 •

edited

Loading

cdesouza-chromium Nov 27, 2024 •

edited

Loading