Remove redundant checks during note encryption #394
Conversation
The consistency check between `esk` and `ephemeral_key` is checked inside `zcash_note_encryption::try_output_recovery_with_ock`, and the requirement to check it inside the `Domain` implementation is being lifted in zcash/librustzcash#848. Removing the check here improves performance, both because we avoid an extra scalar multiplication from `esk.derive_public()`, and because we avoid an unnecessary `spec::diversify_hash()` call which is expensive for Orchard.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #394 +/- ##
==========================================
- Coverage 83.42% 83.06% -0.37%
==========================================
Files 32 32
Lines 2691 2627 -64
==========================================
- Hits 2245 2182 -63
+ Misses 446 445 -1
☔ View full report in Codecov by Sentry. |
| _esk: &Self::EphemeralSecretKey, | ||
| _ephemeral_key: &EphemeralKeyBytes, |
There was a problem hiding this comment.
| _esk: &Self::EphemeralSecretKey, | |
| _ephemeral_key: &EphemeralKeyBytes, |
See https://github.com/zcash/librustzcash/pull/848/files#r1199198303
It may seem as though this is making life more difficult because removing these arguments is a breaking change, but strictly speaking the change to the guarantee of the method is a breaking API change anyway, even if not one that would otherwise be detected by the Rust compiler.
That is, it would be incorrect to mix a version of orchard from after this PR with a version of zcash_note_encryption from before zcash/librustzcash#848 — not (as it happens) because it would be insecure per se, but because that usage hasn't been reviewed.
There was a problem hiding this comment.
Mixing in the way you describe cannot happen, because the traits would not be version-compatible. Rust code is never dynamically linked internally.
Looking at the full report, it's actually an increase in coverage — which makes sense, because we previously had a error case that wasn't reachable. I think the reported overall decrease is due to nondeterminism. |
The consistency check between
eskandephemeral_keyis checked insidezcash_note_encryption::try_output_recovery_with_ock, and the requirement to check it inside theDomainimplementation is being lifted in zcash/librustzcash#848.Removing the check here improves performance, both because we avoid an extra scalar multiplication from
esk.derive_public(), and because we avoid an unnecessaryspec::diversify_hash()call which is expensive for Orchard.