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.

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

add(chain): Adds ViewingKey type in zebra-chain #8198

Merged
merged 12 commits into from
Jan 30, 2024
Merged

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Jan 25, 2024

Motivation

This PR adds a ViewingKey type to zebra-chain behind the shielded-scan feature for parsing viewing keys into.

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?
For significant changes:
  • Is there a summary in the CHANGELOG?
  • Can these changes be split into multiple PRs?

If a checkbox isn't relevant to the PR, mark it as done.

Complex Code or Requirements

Safely hashing viewing keys may need special attention.

Solution

Adds:

  • shielded-scan feature to zebra-chain to depend on zcash_client_backend
  • ViewingKey, SaplingViewingKey, and OrchardViewingKey types
  • parse() method to ViewingKey for decoding keys

Testing

  • Tests that zecpages sapling efvk parses successfully

Review

Anyone can review.

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

@arya2 arya2 added A-rpc Area: Remote Procedure Call interfaces A-blockchain-scanner Area: Blockchain scanner of shielded transactions P-Medium ⚡ labels Jan 25, 2024
@arya2 arya2 self-assigned this Jan 25, 2024
@arya2 arya2 requested review from a team as code owners January 25, 2024 19:36
@arya2 arya2 requested review from upbqdn and removed request for a team January 25, 2024 19:36
@arya2 arya2 added the A-cryptography Area: Cryptography related label Jan 25, 2024
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

@arya2 could you please describe how we are going to use the key hashes?

zebra-chain/src/primitives/viewing_key.rs Outdated Show resolved Hide resolved
zebra-chain/src/primitives/viewing_key.rs Outdated Show resolved Hide resolved
zebra-chain/src/primitives/viewing_key.rs Outdated Show resolved Hide resolved
@arya2 arya2 force-pushed the key-enum-and-hash branch from fd772f2 to b1d9723 Compare January 27, 2024 00:37
@arya2
Copy link
Contributor Author

arya2 commented Jan 27, 2024

please describe how we are going to use the key hashes?

I added to its docs that it's for authorizing access via the gRPC interface, it's so that the client doesn't have to keep re-sending their viewing keys once they're registered in the scanner.

zebra-chain/src/primitives/viewing_key.rs Outdated Show resolved Hide resolved
zebra-chain/Cargo.toml Outdated Show resolved Hide resolved
@arya2 arya2 requested a review from upbqdn January 27, 2024 02:53
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

I still struggle to see the advantages of hashing the keys instead of using them directly. What would be worse if we didn't hash the keys?

@arya2
Copy link
Contributor Author

arya2 commented Jan 29, 2024

I still struggle to see the advantages of hashing the keys instead of using them directly. What would be worse if we didn't hash the keys?

It could be useful if the private key for the TLS encryption were compromised.

@arya2 arya2 requested a review from upbqdn January 29, 2024 18:00
@arya2 arya2 changed the title add(chain): Adds ViewingKey and KeyHash types in zebra-chain add(chain): Adds ViewingKey type in zebra-chain Jan 30, 2024
upbqdn
upbqdn previously approved these changes Jan 30, 2024
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

Looks great.

We'll need tests that check the "to bytes" and "from bytes" round-trips, but we can do them later.

zebra-chain/src/primitives/viewing_key/sapling.rs Outdated Show resolved Hide resolved
mergify bot added a commit that referenced this pull request Jan 30, 2024
@mergify mergify bot merged commit 768eb90 into main Jan 30, 2024
132 checks passed
@mergify mergify bot deleted the key-enum-and-hash branch January 30, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-blockchain-scanner Area: Blockchain scanner of shielded transactions A-cryptography Area: Cryptography related A-rpc Area: Remote Procedure Call interfaces P-Medium ⚡
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants