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

Orchard: ensure that ivk = 0 is rejected if that negligible case occurs #3869

Closed
dconnolly opened this issue Mar 14, 2022 · 1 comment · Fixed by #3962
Closed

Orchard: ensure that ivk = 0 is rejected if that negligible case occurs #3869

dconnolly opened this issue Mar 14, 2022 · 1 comment · Fixed by #3962
Labels
A-consensus Area: Consensus rule updates A-cryptography Area: Cryptography related A-rust Area: Updates to Rust code NU-5 Network Upgrade: NU5 specific tasks

Comments

@dconnolly
Copy link
Contributor

Motivation

"In a pairing today, @daira (ze/hir) and I discovered a bug in the orchard crate, that arose from the interaction of the fixes for the two circuit bugs that were found and fixed in zcashd 4.5.1. If we'd recognised this current bug at the time of those fixes, we'd probably have altered how we changed the circuit (but making that change now would be too noisy and require further analysis). As it is, we have a simple fix that just requires changing the outside-circuit code in orchard (to ensure that ivk = 0 is rejected if that negligible case occurs) and clarifying some type definitions in the protocol spec. zcash/orchard#294 "

Specifications

zcash/orchard#294

So we need to map the identity to an error, not just the ⊥, in

fn from(fvk: FullViewingKey) -> Self {

Related Work

zcash/orchard#294

@dconnolly dconnolly added A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code NU-5 Network Upgrade: NU5 specific tasks P-Medium ⚡ labels Mar 14, 2022
@dconnolly dconnolly self-assigned this Mar 14, 2022
@conradoplg
Copy link
Collaborator

The spec changes affects two places:

  • Orchard key generation
  • Incoming viewing key parsing

Which are either not supported, or not used in regular node usage in Zebra. (The closest I could think of is the consensus rule that shielded coinbase txs must be decryptable with an all-zero outgoing viewing key, but that's not affected by the spec change)

Therefore we don't need to do this now, and can schedule this maybe for the wallet epic. @dconnolly can you please check if this reasoning is correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-cryptography Area: Cryptography related A-rust Area: Updates to Rust code NU-5 Network Upgrade: NU5 specific tasks
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants