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

Implement ZCash transparent tx signing #20895

Merged
merged 1 commit into from
Nov 29, 2023
Merged

Implement ZCash transparent tx signing #20895

merged 1 commit into from
Nov 29, 2023

Conversation

cypt4
Copy link
Collaborator

@cypt4 cypt4 commented Nov 8, 2023 •
edited
Loading

Implement ZCash transparent tx signing
Resolves brave/brave-browser#33661

Tx id digest and signature per-input digest is calculated via https://zips.z.cash/zip-0244.
Calculation is implemented in ZCashSerializer.
Nu5 Tx format https://zips.z.cash/zip-0225. Implemented in ZCashSerializer::SerializeRawTransaction.

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:

@github-actions github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) feature/web3/wallet feature/web3/wallet/core labels Nov 8, 2023
@brave-builds
Copy link
Collaborator

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

@cypt4 cypt4 changed the title WIP ZCash sign Implement ZCash transparent tx signing Nov 8, 2023
@brave-builds
Copy link
Collaborator

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

@cypt4 cypt4 marked this pull request as ready for review November 8, 2023 20:51
@cypt4 cypt4 requested review from a team as code owners November 8, 2023 20:51
supermassive
supermassive reviewed Nov 9, 2023
View reviewed changes
components/brave_wallet/browser/zcash/zcash_wallet_service.cc
@@ -267,7 +286,7 @@ absl::optional<std::string> ZCashWalletService::GetUnusedChangeAddress(
// TODO(cypt4): this always returns first change address. Should return
// first unused change address.
return keyring_service_->GetZCashAddress(
account_id, mojom::ZCashKeyId(account_id.bitcoin_account_index, 1, 0));
account_id, mojom::ZCashKeyId(account_id.bitcoin_account_index, 0, 0));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This now returns first receive address. Is this expected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it simplifies testing until discovery is implemented

@brave-builds
Copy link
Collaborator

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

supermassive
supermassive approved these changes Nov 16, 2023
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 with some nits

Douglashdaniel
Douglashdaniel approved these changes Nov 20, 2023
View reviewed changes
Copy link
Contributor

@Douglashdaniel Douglashdaniel left a comment

Choose a reason for hiding this comment

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

++ Desktop Front-end

@cypt4 cypt4 requested a review from a team as a code owner November 22, 2023 21:13
kdenhartog
kdenhartog reviewed Nov 23, 2023
View reviewed changes
Copy link
Member

@kdenhartog kdenhartog left a comment

Choose a reason for hiding this comment

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

My apologies on the delay in getting this review back to you. There's one blocking concern around the rule of 2, but everything else is just questions to make sure my understanding isn't missing anything.

components/brave_wallet/browser/bitcoin/bitcoin_serializer.cc
@@ -21,13 +22,13 @@ constexpr uint32_t kWitnessScaleFactor = 4;
namespace {

void PushOutpoint(const BitcoinTransaction::Outpoint& outpoint,
BitcoinSerializerStream& stream) {
BtcLikeSerializerStream& stream) {
Copy link
Member

Choose a reason for hiding this comment

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

would it be more useful to refer to this as a UtxoSerializerStream instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@supermassive WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use naming which refers to origin of this binary format. And there are some utxo-based crypto currencies that are not based on bitcoin.

components/brave_wallet/browser/brave_wallet_utils.cc
@@ -598,7 +598,8 @@ const mojom::NetworkInfo* GetZCashMainnet() {
static base::NoDestructor<mojom::NetworkInfo> network_info(
{chain_id,
"ZCash Mainnet",
{""}, // TODO(cypt4): explorer url
{"https://zcashblockexplorer.com/transactions"}, // TODO(cypt4):
Copy link
Member

Choose a reason for hiding this comment

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

nit: might be worth refactoring all of these urls into the constants file

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with this being done in a follow up PR as well.

Copy link
Collaborator Author

@cypt4 cypt4 Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can move whole list of default network infos to another file.
@supermassive wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This whole file needs some cleanup I think :)

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

namespace brave_wallet {

TEST(ZCashSerializerTest, HashPrevouts) {
Copy link
Member

Choose a reason for hiding this comment

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

are these standardized test vectors listed somewhere or just tests to make sure code doesn't break if we refactor in the future?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are vectors, but they mostly depend on shielded parts too, so we can't just borrow them at this moment.
I've added a couple of tests based on real transactions here: ZCashSerializerTest::TxId_TransparentOnly and
ZCashWalletServiceUnitTest, SignAndPostTransaction.
Tests for Hash* are just making easier to find which part is actually broken.

Copy link
Member

Choose a reason for hiding this comment

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

We can likely generate these from here: https://github.com/zcash/zcash-test-vectors

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As i see there is no intermediate data like sub-digests in such vectors.
I think we can just reuse published vectors when shielded will be implemented for top-level things like tx id and sig digest.

components/brave_wallet/browser/zcash/zcash_transaction.h Show resolved Hide resolved
components/brave_wallet/browser/zcash/zcash_wallet_service.cc Show resolved Hide resolved
kdenhartog
kdenhartog approved these changes Nov 27, 2023
View reviewed changes
Copy link
Member

@kdenhartog kdenhartog left a comment

Choose a reason for hiding this comment

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

Approving, but we'll need to address some issues in follow up PRs

@brave-builds
Copy link
Collaborator

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

@brave-builds
Copy link
Collaborator

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

@brave-builds
Copy link
Collaborator

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

@cypt4
Copy link
Collaborator Author

cypt4 commented Nov 29, 2023

/build android

nuo-xu
nuo-xu approved these changes Nov 29, 2023
View reviewed changes
Copy link
Contributor

@nuo-xu nuo-xu left a comment

Choose a reason for hiding this comment

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

iOS++

@cypt4 cypt4 merged commit 8c95bd6 into master Nov 29, 2023
17 checks passed
@cypt4 cypt4 deleted the brave_33661 branch November 29, 2023 15:01
@github-actions github-actions bot added this to the 1.62.x - Nightly milestone Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmarier fmarier fmarier left review comments

@supermassive supermassive supermassive approved these changes

@github-actions github-actions[bot] github-actions[bot] left review comments

@nuo-xu nuo-xu nuo-xu approved these changes

@kdenhartog kdenhartog kdenhartog approved these changes

@Douglashdaniel Douglashdaniel Douglashdaniel approved these changes

Assignees

@bridiver bridiver

@cypt4 cypt4

Labels
CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet/core feature/web3/wallet
Projects
None yet
Milestone
1.62.x - Release
Development

Successfully merging this pull request may close these issues.

[ZCash] Implement ZCash transparent tx creation and signing
9 participants
@cypt4 @brave-builds @fmarier @supermassive @nuo-xu @kdenhartog @Douglashdaniel @bridiver @thypon