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

cleanup(cryptography): Remove unused shielded key and address code #5476

Merged
merged 10 commits into from Oct 29, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Oct 25, 2022

Motivation

We want to remove unused shielded key and address code before the audit, because it contains known bugs that break consensus rules.

We also want to remove all the code that is impacted by these known bugs:

Close #5446
Close #4691
Close #2496
Close #2041

Solution

This PR removes 3000 lines of unused cryptographic code:

  • all unused shielded key code
  • dependent unused shielded address, note, commitment, nullifier, and test code

As a consequence, it removes:

  • unimplemented cryptographic algorithms and conversions
  • buggy cryptographic algorithms and conversions
  • known panics during conversions
  • untested code

It also makes Zebra's code easier to review and audit, because some of the removed types had the same name as the types from the ECC crates.

Review

Anyone can review this PR, it's a high priority because it's needed for the audit.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
  • How do you know it works? Does it have tests?

@teor2345 teor2345 added A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup P-High 🔥 C-security Category: Security issues I-consensus Zebra breaks a Zcash consensus rule A-cryptography Area: Cryptography related I-lose-funds Zebra loses user funds labels Oct 25, 2022
@teor2345 teor2345 self-assigned this Oct 25, 2022
@github-actions github-actions bot added C-feature Category: New features C-removed C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Oct 25, 2022
@codecov
Copy link

codecov bot commented Oct 25, 2022

Codecov Report

Merging #5476 (d388faf) into main (7b47aac) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5476      +/-   ##
==========================================
- Coverage   79.32%   79.25%   -0.07%     
==========================================
  Files         309      305       -4     
  Lines       39348    37873    -1475     
==========================================
- Hits        31211    30016    -1195     
+ Misses       8137     7857     -280     

@teor2345
Copy link
Contributor Author

@teor2345 teor2345 marked this pull request as ready for review October 25, 2022 04:09
@teor2345 teor2345 requested review from a team as code owners October 25, 2022 04:09
@teor2345 teor2345 requested review from upbqdn and dconnolly and removed request for a team October 25, 2022 04:09
@teor2345
Copy link
Contributor Author

teor2345 commented Oct 26, 2022

Actually, I think I might be able to remove some of the broken Orchard code here.

@teor2345
Copy link
Contributor Author

Actually, I think I might be able to remove some of the broken Orchard code here.

I worked out why it's tricky with Orchard - we need most of the key types to generate valid random Orchard addresses for tests.

So I'm going to move the keys that aren't used in production into a test module, and add a note about what needs to be done to the code to make it production-quality. (We're not getting test code audited, so that achieves the goals of the ticket.)

I might not get to finish it all today.

@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 27, 2022
@teor2345
Copy link
Contributor Author

Ok, I think I'm done here.

See the PR description for a summary of these changes, it's almost all removed code.

@teor2345 teor2345 changed the title cleanup(cryptography): Remove unused shielded key and address code, or disable it in production builds cleanup(cryptography): Remove unused shielded key and address code Oct 27, 2022
@oxarbitrage
Copy link
Contributor

This seems to be done. @conradoplg or @dconnolly , can you please approve if that is the case ? Thank you.

Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

lgtm

If we do wallet work / scanning work in the future we can dig the sapling/orchard stuff out of the history and fix it up, if needed

mergify bot added a commit that referenced this pull request Oct 29, 2022
@mergify mergify bot merged commit 161bb80 into main Oct 29, 2022
@mergify mergify bot deleted the unused-shielded-keys branch October 29, 2022 20:59
@teor2345 teor2345 removed the request for review from upbqdn October 30, 2022 21:18
teor2345 added a commit that referenced this pull request Feb 6, 2023
…5476)

* Remove unused and buggy Sprout key and address code

* Remove unused, buggy Sapling address, key, and commitment code

* Delete unused Orchard key code

* Move almost all the buggy Orchard key code into a test-only module

* Remove Orchard keys and addresses that aren't used in production code

* Remove unused prf_expand() function and unimplemented poseidon_hash() function

* Remove unused Orchard key types

* Remove unused sinsemilla commit code

* Update zebra-chain/src/sprout/keys.rs

* Update zebra-chain/src/sprout/keys.rs

Co-authored-by: Deirdre Connolly <durumcrustulum@gmail.com>
@mpguerra mpguerra mentioned this pull request Apr 11, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cryptography Area: Cryptography related A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup C-feature Category: New features C-security Category: Security issues C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG I-consensus Zebra breaks a Zcash consensus rule I-lose-funds Zebra loses user funds
Projects
None yet
4 participants