Unified Addresses support #21460

cypt4 · 2023-12-25T12:37:52Z

Resolves brave/brave-browser#32239

Resolves

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 lint, 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:

brave-builds · 2023-12-26T11:11:50Z

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

brave-builds · 2023-12-26T14:02:37Z

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

brave-builds · 2023-12-26T17:03:07Z

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

supermassive · 2023-12-27T02:15:13Z

build/commands/scripts/updatePatches.js

@@ -36,6 +43,9 @@ module.exports = function RunCommand (options) {
    updatePatches(catapultDir, catapultPatchDir),
    // third_party/devtools-frontend/src
    updatePatches(devtoolsFrontendDir, devtoolsFrontendPatchDir),
+    // brave/third_party/bitcoin-core
+    updatePatches(bitcoinDir, bitcoinPatchDir),
+


nit: empty line

supermassive · 2023-12-27T02:18:53Z

components/brave_wallet_ui/utils/address-utils.ts

+  return /^t1[a-km-zA-HJ-NP-Z1-9]{33}$/.test(value) || /^u1[a-z0-9]+$/.test(value)
 }


nit: needs formatting

supermassive · 2023-12-27T02:23:22Z

components/brave_wallet/common/f4_jumble.h

+std::string GetHPersonalizer(uint8_t i);
+std::string GetGPersonalizer(uint8_t i, uint16_t j);
+


These two do not need to be public

They are used in tests

supermassive · 2023-12-27T02:24:57Z

components/brave_wallet/common/f4_jumble.cc

+std::vector<uint8_t> GetLeft(const std::vector<uint8_t>& message) {
+  size_t left_size =
+      std::min(static_cast<size_t>(kLeftSize), message.size() / 2);
+  return std::vector(message.begin(), message.begin() + left_size);
+}
+
+std::vector<uint8_t> GetRight(const std::vector<uint8_t>& message) {
+  size_t left_size =
+      std::min(static_cast<size_t>(kLeftSize), message.size() / 2);
+  return std::vector(message.begin() + left_size, message.end());
+}


~~static_cast<size_t>~~(kLeftSize)

supermassive · 2023-12-27T02:30:00Z

components/brave_wallet/common/f4_jumble.cc

+std::string GetHPersonalizer(uint8_t i) {
+  return {85,  65, 95,  70,  52, 74, 117,
+          109, 98, 108, 101, 95, 72, static_cast<char>(i),
+          0,   0};
+}
+
+std::string GetGPersonalizer(uint8_t i, uint16_t j) {
+  return {85,
+          65,
+          95,
+          70,
+          52,
+          74,
+          117,
+          109,
+          98,
+          108,
+          101,
+          95,
+          71,
+          static_cast<char>(i),
+          static_cast<char>(j & 0xFF),
+          static_cast<char>(j >> 8)};
+}
+


These should return std:array<uint8_t, 16>

supermassive · 2023-12-27T02:40:55Z

components/brave_wallet/common/f4_jumble_unittest.cc

+TEST(F4JumbleUnitTest, ApplyF4Jumble) {
+  {
+    std::vector<uint8_t> input = {};
+
+    std::vector<uint8_t> expected = {};
+
+    EXPECT_EQ(expected, ApplyF4Jumble(input));
+  }
+
+  {


We need to test Apply + Revert resulting in original input. Also covering corner cases for input sizes.
Something like

std::vector<uint8_t> input(1000, 'a'); auto input_span = make_span(input); for (int i = 0; i < input_span.size(); ++i) { auto d = input_span.subspan(0, i); EXPECT_EQ(d, RevertF4Jumble(ApplyF4Jumble(d))); }

supermassive · 2023-12-27T02:45:06Z

components/brave_wallet/common/f4_jumble.cc

+  if (personalizer.length() != sizeof(params.personal)) {
+    NOTREACHED();
+    return std::nullopt;
+  }
+


Personalizer should be a fixed size span, and this check should become a CHECK or a static assert

supermassive · 2023-12-27T02:46:40Z

components/brave_wallet/common/f4_jumble.cc

+  if (blake2b_init_param(&blake_state, &params) != 0) {
+    NOTREACHED();
+    return std::nullopt;
+  }
+  if (blake2b_update(&blake_state, payload.data(), payload.size()) != 0) {
+    NOTREACHED();
+    return std::nullopt;
+  }
+  std::vector<uint8_t> result(len);
+  if (blake2b_final(&blake_state, result.data(), len) != 0) {
+    NOTREACHED();
+    return std::nullopt;
+  }
+
+  return result;


blake2b_* calls fail only on invalid inputs. We should just CHECK the result and blake2b function should not return an optional

supermassive · 2023-12-27T02:50:27Z

components/brave_wallet/common/f4_jumble.cc

+  if (!hash || hash->size() != left.size()) {
+    return std::nullopt;
+  }
+  std::vector<uint8_t> result;


blake2b should never fail and always return hash size equal to left.size(). So this method never fails an no optional needed.
Same for g_round

supermassive · 2023-12-27T02:53:57Z

components/brave_wallet/common/zcash_utils.cc

+// https://zips.z.cash/zip-0316#encoding-of-unified-addresses
+enum AddrType {
+  P2PKH = 0x00,
+  P2PSH = 0x01,
+  Sapling = 0x02,
+  Orchard = 0x03,
+  End = 0x04
+};
+


The spec does not mention End = 0x04. What is it?

Just to check whether code is known (less than End)

supermassive · 2023-12-27T03:02:27Z

components/brave_wallet/common/zcash_utils.cc

+
+  for (const auto& part : parts.value()) {
+    if (part.first == AddrType::P2PKH) {
+      return PubkeyToTransparentAddress(part.second, false);


~~false~~ is_testnet ?

supermassive · 2023-12-27T03:14:58Z

components/brave_wallet/common/zcash_utils.cc

+  auto reverted = RevertF4Jumble(data);
+  if (!reverted || reverted->size() < 16) {
+    return std::nullopt;
+  }
+
+  auto parts = ParseUnifiedAddress(
+      base::make_span(*reverted).subspan(0, reverted->size() - 16));
+  if (!parts.has_value()) {
+    return std::nullopt;
+  }


Needs a comment and a constant for what is 16

brave-builds · 2023-12-27T15:34:06Z

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

supermassive · 2023-12-28T02:53:58Z

components/brave_wallet/common/zcash_utils.cc

+  }
+
+  auto reverted = RevertF4Jumble(data);
+  // HRP with 16 bytes padding which is appened to the end of message


supermassive · 2023-12-28T02:58:49Z

components/brave_wallet/common/zcash_utils.cc

+  }
+
+  auto parts = ParseUnifiedAddress(
+      base::make_span(*reverted).subspan(0, reverted->size() - 16));


kPaddedHrpSize

supermassive · 2023-12-28T03:03:33Z

components/brave_wallet/common/zcash_utils_unittest.cc

@@ -48,4 +48,13 @@ TEST(ZCashUtilsUnitTest, PubkeyToTransparentAddress) {
                false));
 }

+TEST(ZCashUtilsUnitTest, ExtractTransparentPart) {
+  auto transparent_past = ExtractTransparentPart(


transparent_part

supermassive · 2023-12-28T03:04:02Z

components/brave_wallet_ui/utils/address-utils.ts

 export function isValidZecAddress(value: string): boolean {
-  return /^t1[a-km-zA-HJ-NP-Z1-9]{33}$/.test(value)
+  return /^t1[a-km-zA-HJ-NP-Z1-9]{33}$/.test(value) ||
+         /^u1[a-z0-9]+$/.test(value)
 }



this should handle testnet also

brave-builds · 2024-01-09T14:53:46Z

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

mkarolin

strings++

Resolves brave/brave-browser#32239

brave-builds · 2024-01-10T12:42:12Z

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

goodov

f4_jumble.cc, chromium_src/patches lgtm.

brave-builds · 2024-01-10T14:44:04Z

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

components/brave_wallet/browser/zcash/zcash_wallet_service.cc

components/brave_wallet/common/f4_jumble.cc

bridiver

util.js changes ok

github-actions · 2024-01-11T13:10:20Z

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

Description

The pull request appears to be integrating Bitcoin Core support into Brave by adding patching capabilities and updating utility functions for ZCash addresses. It includes the proper patching for the new bitcoin-core directory, modification to the patching scripts, replacing absl::nullopt with std::nullopt, adding internationalization/localization improvements, and adding new functionalities related with F4 jumbling for unified addresses in ZCash.

Changes

Changes

Patch Updates

build/commands/lib/util.js
- Added support for the patching of Bitcoin Core dependencies.
build/commands/scripts/updatePatches.js
- Updated the script to handle patching of Bitcoin Core related sources.
components/brave_wallet/browser/keyring_service.cc
- Replaced absl::nullopt with std::nullopt.

ZCash Address Handling

components/brave_wallet/browser/zcash/zcash_rpc.cc
- Improved error handling by replacing generic error strings with localized string identifiers.
components/brave_wallet/browser/zcash/zcash_wallet_service.cc
- Made similar improvements for error messages and implemented unified address extraction logic for ZCash.
components/brave_wallet/browser/zcash/zcash_wallet_service_tasks.cc
- Updated error message handling in transaction tasks.

New Features (F4 Jumble)

components/brave_wallet/common/BUILD.gn
- Added new sources for the F4 jumble implementation.
components/brave_wallet/common/DEPS
- Included new third-party dependencies used in the F4 jumble implementation.
components/brave_wallet/common/f4_jumble.cc
- New implementation file providing logic for F4 jumbling (a form of encryption/obfuscation).
components/brave_wallet/common/f4_jumble.h
- Header file for F4 jumble utilities.
components/brave_wallet/common/f4_jumble_unittest.cc
- Unit tests covering the F4 jumble functionalities.
components/brave_wallet/common/zcash_utils.cc
- Additional utility functions to handle parsing and interaction with unified ZCash addresses.
components/brave_wallet/common/zcash_utils.h
- Headers for the new ZCash utility functions.
components/brave_wallet/common/zcash_utils_unittest.cc
- Unit tests for the ZCash utility functions.
components/brave_wallet_ui/utils/address-utils.ts
- Updates to utility functions to accommodate unified ZCash addresses.

Localization

components/resources/wallet_strings.grdp
- Added a new string definition for unified address error messaging.

Dependency Patching

a/patches/brave/third_party/bitcoin-core/src/src-bech32.cpp.patch
- New patch file needed for Bitcoin Core dependency.

Build Script

script/brave_license_helper.py
- Adjustment to the licensing helper script to cover the new patch directory structure.

Security Hotspots

components/brave_wallet/common/f4_jumble.cc
- Introduction of new cryptographic utility, which should be reviewed for potential vulnerabilities in its implementation, such as information leakage or improper usage.
components/brave_wallet/browser/zcash/zcash_wallet_service_tasks.cc / components/brave_wallet/browser/zcash/zcash_wallet_service.cc / components/brave_wallet/common/zcash_utils.cc
- For all the files related to ZCash address handling, there is a risk of mishandling the addresses, leading to potentially leaking user information or sending funds to incorrect addresses due to parsing and handling errors.
Dependency Patching (a/patches/brave/third_party/bitcoin-core/src/src-bech32.cpp.patch)
- Any new patch introduces a risk of vulnerability if not properly reviewed. This specific patch should be checked for changes aligning with best practices for cryptographic algorithm modifications or potential introduction of weaknesses.

brave-builds · 2024-01-11T13:58:50Z

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

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 25, 2023

github-actions bot assigned cypt4 Dec 25, 2023

cypt4 force-pushed the brave_32239 branch 3 times, most recently from 367c99b to f7bcf22 Compare December 26, 2023 10:36

cypt4 force-pushed the brave_32239 branch from f7bcf22 to 8d22dcc Compare December 26, 2023 13:31

cypt4 force-pushed the brave_32239 branch from 8d22dcc to c505495 Compare December 26, 2023 16:30

supermassive reviewed Dec 27, 2023

View reviewed changes

cypt4 force-pushed the brave_32239 branch from c505495 to e35bab4 Compare December 27, 2023 15:00

supermassive reviewed Dec 28, 2023

View reviewed changes

mkarolin approved these changes Jan 9, 2024

View reviewed changes

cypt4 added 6 commits January 10, 2024 18:31

Unified Addresses support

ac0e029

Resolves brave/brave-browser#32239

Fix patches

4328cbd

Review fix

056781b

Review fix

a3c240c

review fix

384cf25

Review fix

521864f

cypt4 force-pushed the brave_32239 branch from ebb5d59 to c36571a Compare January 10, 2024 11:47

Rebase

52fa28e

cypt4 force-pushed the brave_32239 branch from c36571a to 52fa28e Compare January 10, 2024 13:25

goodov approved these changes Jan 10, 2024

View reviewed changes

cdesouza-chromium reviewed Jan 10, 2024

View reviewed changes

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

cdesouza-chromium reviewed Jan 10, 2024

View reviewed changes

components/brave_wallet/common/f4_jumble.cc Outdated Show resolved Hide resolved

cdesouza-chromium reviewed Jan 10, 2024

View reviewed changes

components/brave_wallet/common/f4_jumble.cc Outdated Show resolved Hide resolved

cdesouza-chromium reviewed Jan 10, 2024

View reviewed changes

components/brave_wallet/common/f4_jumble.cc Outdated Show resolved Hide resolved

cdesouza-chromium reviewed Jan 10, 2024

View reviewed changes

components/brave_wallet/common/f4_jumble.cc Outdated Show resolved Hide resolved

cypt4 mentioned this pull request Jan 10, 2024

ZCash transparent release brave/qa-resources#577

Open

bridiver approved these changes Jan 10, 2024

View reviewed changes

Review fix

7928def

cdesouza-chromium approved these changes Jan 12, 2024

View reviewed changes

cypt4 merged commit 3a58a81 into master Jan 12, 2024
18 checks passed

cypt4 deleted the brave_32239 branch January 12, 2024 07:14

github-actions bot added this to the 1.63.x - Nightly milestone Jan 12, 2024

This was referenced Jan 12, 2024

Revert "Unified Addresses support" #21567

Merged

Implement unified addresses for Zcash #21574

Merged

		return /^t1[a-km-zA-HJ-NP-Z1-9]{33}$/.test(value) \|\| /^u1[a-z0-9]+$/.test(value)
		}

		std::string GetHPersonalizer(uint8_t i);
		std::string GetGPersonalizer(uint8_t i, uint16_t j);

Unified Addresses support #21460

Unified Addresses support #21460

Conversation

cypt4 commented Dec 25, 2023

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Dec 26, 2023

brave-builds commented Dec 26, 2023

brave-builds commented Dec 26, 2023

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

supermassive Dec 27, 2023 • 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

cypt4 Dec 27, 2023 • 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

brave-builds commented Dec 27, 2023

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

brave-builds commented Jan 9, 2024

mkarolin left a comment

Choose a reason for hiding this comment

brave-builds commented Jan 10, 2024

goodov left a comment

Choose a reason for hiding this comment

brave-builds commented Jan 10, 2024

bridiver left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 11, 2024

Description

Changes

Patch Updates

ZCash Address Handling

New Features (F4 Jumble)

Localization

Dependency Patching

Build Script

Security Hotspots

brave-builds commented Jan 11, 2024

supermassive Dec 27, 2023 •

edited

Loading

cypt4 Dec 27, 2023 •

edited

Loading