Implement shielding #23076

cypt4 · 2024-04-15T10:39:09Z

Adds shielding operations which means sending from transparent to shielded address.
See last commit until PR rebased on master.
This includes steps :
Create Tx flow:

Resolve transparent UTXOs for the selected account, also resolve last orchard tree root ( This is needed to construct Orchard tx bundle ) and latest block id(needed for tx header).
Construct unauthorized orchard bundle with provided shielded outputs and random number generator. Unauthorized bundle can supply us with orchard digest to use in signature digest construction.
Calculate signature digest for every transparent input, also calculate orchard sighash.
Apply sighash to the unauthorized orchard bundle. That returns authorized orchard bundle.
From authorized orchard bundle we can resolve orchard raw tx part to insert it into serialized ZCashTransaction.
Serialize transaction and send vie SendRawTransaction gRPC call.
Calculate and show tx id using https://zips.z.cash/zip-0244#txid-digest

Audit: https://github.com/brave/reviews/issues/1588

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:

rillian

The empty components/zcash/README.chromium is unnecessary, but was a nice clue there was vendored code hiding in here. :)

rillian · 2024-04-15T15:32:42Z

components/zcash/rs/mod.rs

+// License, v. 2.0. If a copy of the MPL was not distributed with this file,
+// You can obtain one at https://mozilla.org/MPL/2.0/.
+
+mod librustzcash;


This file is unnecessary. Having librustzcash/mod.rs is sufficient to declare the module.

rillian · 2024-04-15T18:42:59Z

components/zcash/rs/BUILD.gn

+    "//brave/third_party/rust/byteorder/v1:lib",
+    "//brave/third_party/rust/incrementalmerkletree/v0_5:lib",
+    "//brave/third_party/rust/nonempty/v0_7:lib",
    "//brave/third_party/rust/orchard/v0_7:lib",
+    "//third_party/rust/rand_core/v0_6:lib",


Please keep Cargo.toml in sync for direct dependencies. These are all referenced directly.

If you want to verify sufficient declarations by running cargo check within the local source directory, you can resolve against the patched orchard crate by adding a patch like to zcash/rs/Cargo.toml:

[patch.crates-io.orchard] path = "../../../third_party/rust/chromium_crates_io/vendor/orchard-0.7.1"

rillian · 2024-04-15T18:49:01Z

components/zcash/rs/lib.rs

+    }
+
+    fn next_u64(&mut self) -> u64 {
+        self.0 += 1;


This seems not great for a CryptoRng. What about using rand_chacha::ChaCha20Rng::seed_from_u64() instead of this mock?

This is used only in tests, do you think we really need to use complicated randomness there?

It's only used in test code, but it's available in all builds. I'm concerned about someone else using it accidentally, sometime in the future, where they need something else.

rand_chacha is a SeedableRng + CryptoRng we already have in tree, so using it is also just less code.

We have discussed with @kdenhartog. He is ok with keeping mock rng since it's call is protected by CHECK_IS_TESTS. Otherwise i'd need to recapture testvectors.

rillian · 2024-04-15T18:55:43Z

components/zcash/rs/librustzcash/mod.rs

+// License, v. 2.0. If a copy of the MPL was not distributed with this file,
+// You can obtain one at https://mozilla.org/MPL/2.0/.
+
+#[allow(dead_code)]


Why are we importing code we're not using?

Just to reduce changes in the librustzcash files

rillian · 2024-04-15T19:00:08Z

components/zcash/rs/librustzcash/orchard.rs

+// License, v. 2.0. If a copy of the MPL was not distributed with this file,
+// You can obtain one at https://mozilla.org/MPL/2.0/.
+
+// https://github.com/zcash/librustzcash/blob/zcash_primitives-0.15.0/zcash_primitives/src/transaction/components/orchard.rs


Can you share something about the motivation here? I really don't like this kind of cut-and-paste vendoring because it's hard to update. Or really, hard to remember it needs updating, since it's not visible to normal advisory notification tools.

Is this an attempt to strip the (large) dependency list of the published zcash_primitives and zcash_encoding crates? Has the code been customized beyond trimming it? I'd just like to clear about the maintenance and review tradeoffs we're making here.

Sure, there is a quite big librustzcash crate and it is not clear yet which parts we can reuse directly.
Idea is just to copy necessary minimum of code and then decide if we can switch to vendoring istead of copying. Also there are some excess things in zcash_primitives that we don't need - like transparent or sapling support, tx builder, etc.

Has the code been customized beyond trimming it

There is a little of customization like changing "use" declarations. Probably it will be customized deeply in the future

@kdenhartog iirc you've looked at the zcash crates. I'm curious how you see this approach?

rillian · 2024-04-15T19:14:25Z

components/zcash/rs/lib.rs

+                        .ok_or(Error::BuildError)
+                })
+        },
+        OrchardRandomSource::MockRng(mut rng) => {


You could avoid the duplication here by implementing a forwarding RngCore on the OrchardRandomSource wrapper, but that wouldn't be less code so probably not worth it.

brave-builds · 2024-04-16T10:59:58Z

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

supermassive · 2024-04-19T10:28:44Z

wallet core looks ok in general, will wait for base PR gets merged

bridiver · 2024-04-22T18:44:52Z

components/zcash/rs/BUILD.gn

@@ -9,11 +9,24 @@ rust_static_library("rust_lib") {
  crate_name = "zcash_cxx"
  crate_root = "lib.rs"
  allow_unsafe = true
-  sources = [ "lib.rs" ]
+
+  sources = [


why is this code in services?

bridiver · 2024-04-22T18:45:25Z

third_party/rust/chromium_crates_io/Cargo.toml

@@ -26,6 +26,7 @@ adblock-cxx = { version = "1" }
 constellation-cxx = "0.1"
 challenge-bypass-ristretto-cxx = "1"
 orchard = { version = "0.7.0",  default-features = false }
+incrementalmerkletree = { version = "0.5.1", features = ["legacy-api"]}


we should not be adding this directly to chromium_crates_io/Cargo.toml. It should go in a Cargo.toml file along with the relevant code

supermassive · 2024-06-18T07:33:55Z

components/brave_wallet/browser/zcash/create_shield_all_transaction_task.cc

+      base::StrCat({"0x", tree_state_.value()->orchardTree}));
+  if (!state_tree_bytes) {
+    error_ = l10n_util::GetStringUTF8(IDS_WALLET_INTERNAL_ERROR);
+    Iterate();


Let's do this through PostTask to avoid Iterate()->CompleteTransaction()->Iterate() recursion

supermassive · 2024-06-18T07:38:46Z

components/brave_wallet/browser/zcash/create_shield_all_transaction_task.cc

+          account_id_, mojom::ZCashKeyId::New(account_id_->account_index,
+                                              1 /* internal */, 0));
+  if (!addr_bytes) {


mojom::ZCashKeyId::New(account_id_->account_index, 1 /* internal */, 0)
This most likely needs a seprate function with comment that we have only one internal(change) address for an account

supermassive · 2024-06-18T07:39:46Z

components/brave_wallet/browser/internal/orchard_manager.h

+
+namespace brave_wallet {
+
+class OrchardManager {


OrchardBundleManager maybe?

or OrchardBundleWrapper

supermassive · 2024-06-18T08:37:54Z

components/brave_wallet/common/zcash_utils.h

@@ -49,6 +53,15 @@ struct DecodedZCashAddress {
  bool testnet = false;
 };

+struct OrchardOutput {
+  uint64_t value;


josheleonard

Desktop Frontend ++

brave-builds · 2024-06-18T17:30:35Z

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

components/brave_wallet/browser/zcash/create_shield_all_transaction_task.cc

brave-builds · 2024-06-19T14:14:22Z

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

nuo-xu · 2024-06-19T16:05:07Z

components/brave_wallet/browser/zcash/create_shield_all_transaction_task.cc

+    std::unique_ptr<OrchardBundleManager> orchard_bundle_manager,
+    std::array<uint8_t, kZCashDigestSize> sighash) {
+#if BUILDFLAG(IS_IOS)
+  DCHECK(!web::WebThread::CurrentlyOn(web::WebThread::UI))


missed a ;

brave-builds · 2024-06-19T17:48:14Z

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

Resolves brave/brave-browser#37201

Resolves brave/brave-browser#38604

supermassive · 2024-06-20T07:44:26Z

components/brave_wallet/browser/zcash/create_shield_all_transaction_task.cc

+#if BUILDFLAG(IS_IOS)
+#include "ios/web/public/thread/web_thread.h"
+#else
+#include "content/public/browser/browser_thread.h"
+#endif


Let's not depend on content from here

supermassive

wallet core lgtm

github-actions · 2024-06-20T09:05:29Z

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

Here is a review of the pull request:

Description

This PR adds support for Orchard shielded transactions in the Brave Wallet. It allows creating transactions that shield transparent funds into an Orchard shielded address. The main changes include:

Adding APIs and UI for shielding transparent ZEC funds into Orchard
Integrating rust-based Orchard cryptographic primitives
Modifying the ZEC transaction format to include Orchard data
Updating ZEC transaction signing to handle the Orchard bundle

The motivation appears to be to enhance user privacy by allowing funds to be shielded using the latest Orchard protocol.

Possible Issues

The Orchard bundle creation uses randomness. Need to ensure the randomness source is cryptographically secure. The PR overrides it for testing which is good.
Orchard transactions will likely be larger in size than transparent ones. May need to consider performance impact if many Orchard transactions are created.
User education may be needed around the privacy benefits and tradeoffs of shielding.

Security Hotspots

Randomness generation for Orchard bundles in orchard_bundle_manager.cc. Ensure the random source has sufficient entropy and cannot be manipulated by an attacker. Using a hardware random source is ideal.
Avoid logging any sensitive Orchard data, addresses or transaction details that could impact privacy.
Validate and bounds check the outputs parameter passed to create_orchard_bundle. Ensure it cannot exceed reasonable limits and cause memory issues.

Changes

Changes

Cargo.toml

Adds new rust dependencies rand and zcash_primitives

brave/components/brave_wallet/browser/zcash/rust/Cargo.toml

Adds new rust dependencies rand and zcash_primitives

orchard_bundle_manager.h, orchard_bundle_manager.cc

Adds new OrchardBundleManager class to manage Orchard bundle states

zcash_transaction.h, zcash_transaction.cc

Adds OrchardOutput and OrchardPart structs to ZCashTransaction
Updates transaction serialization

zcash_rpc.h, zcash_rpc.cc

New APIs GetLatestTreeState and GetTreeState to retrieve Orchard tree state

zcash_wallet_service.h, zcash_wallet_service.cc

Adds ShieldFunds API to allow shielding funds into Orchard

Overall this is a significant enhancement to add Orchard shielded transaction support. The security hotspots noted should be carefully reviewed, but with proper precautions this can allow users to take advantage of the latest ZEC privacy features. Thorough testing of the Orchard transactions is recommended. Documentation on the privacy implications and changes to transaction signing would also be beneficial. With those considerations, this looks like a good improvement to the Brave Wallet's ZEC capabilities.

brave-builds · 2024-06-20T09:55:04Z

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

brave-builds · 2024-06-20T12:10:24Z

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

github-actions bot added CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet feature/web3/wallet/core labels Apr 15, 2024

github-actions bot assigned cypt4 Apr 15, 2024

cypt4 force-pushed the brave_37201_7 branch from c7ea475 to 4eba15d Compare April 15, 2024 15:06

cypt4 marked this pull request as ready for review April 15, 2024 15:07

cypt4 requested review from a team and bridiver as code owners April 15, 2024 15:07

cypt4 force-pushed the brave_37201_7 branch 2 times, most recently from 02fe8cd to 0ad3180 Compare April 15, 2024 15:30

rillian reviewed Apr 15, 2024

View reviewed changes

cypt4 force-pushed the brave_37201_2 branch from 30fc1ef to 7505ae5 Compare April 15, 2024 20:42

cypt4 requested review from a team as code owners April 15, 2024 20:42

cypt4 force-pushed the brave_37201_7 branch from 0ad3180 to 568524e Compare April 16, 2024 08:29

supermassive self-requested a review April 16, 2024 10:13

cypt4 force-pushed the brave_37201_7 branch from 568524e to 6d2a2c6 Compare April 16, 2024 10:16

cypt4 force-pushed the brave_37201_2 branch 4 times, most recently from e2e4f6d to 37373ae Compare April 18, 2024 16:39

bridiver reviewed Apr 22, 2024

View reviewed changes

supermassive reviewed Jun 18, 2024

View reviewed changes

josheleonard approved these changes Jun 18, 2024

View reviewed changes

kylehickinson reviewed Jun 18, 2024

View reviewed changes

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

cypt4 force-pushed the brave_37201_7 branch from 2f308d1 to ae1e062 Compare June 19, 2024 13:28

cypt4 requested review from bridiver, supermassive, rillian, kylehickinson, thypon and kdenhartog June 19, 2024 13:31

nuo-xu reviewed Jun 19, 2024

View reviewed changes

cypt4 added 4 commits June 20, 2024 14:35

Implement shielding

b523a87

Resolves brave/brave-browser#37201

Build fix

bac4b11

Add zcash_primitives crate

f29d0a3

Resolves brave/brave-browser#38604

Build fix

3a0936b

supermassive reviewed Jun 20, 2024

View reviewed changes

supermassive approved these changes Jun 20, 2024

View reviewed changes

Review fix

20593ae

cypt4 force-pushed the brave_37201_7 branch from 8678a4f to 20593ae Compare June 20, 2024 09:03

Build fix

1cd6c9e

cypt4 requested a review from nuo-xu June 20, 2024 11:38


		namespace brave_wallet {

		class OrchardManager {

Implement shielding #23076

Are you sure you want to change the base?

Implement shielding #23076

Conversation

cypt4 commented Apr 15, 2024 • edited

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

rillian 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

cypt4 Apr 16, 2024 • edited

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

cypt4 Apr 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brave-builds commented Apr 16, 2024

supermassive commented Apr 19, 2024

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

josheleonard left a comment

Choose a reason for hiding this comment

brave-builds commented Jun 18, 2024

brave-builds commented Jun 19, 2024

Choose a reason for hiding this comment

brave-builds commented Jun 19, 2024

Choose a reason for hiding this comment

supermassive left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 20, 2024

Description

Possible Issues

Security Hotspots

Changes

brave-builds commented Jun 20, 2024

brave-builds commented Jun 20, 2024

cypt4 commented Apr 15, 2024 •

edited

cypt4 Apr 16, 2024 •

edited

cypt4 Apr 16, 2024 •

edited