fix: Avoid a redundant check containing curve point multiplication

[upbqdn]

Motivation

The function try_output_recovery_with_ock calls parse_note_plaintext_without_memo_ovk from here. The second function validates the ephemeral keys. The first function then calls check_note_validity, which also contains the same check of the same ephemeral keys. These checks both contain a curve point multiplication, and since the second check is redundant, removing it increases performance.

Closes #802.

Solution

• Don't call check_note_validity, but still check the commitment bytes.

As mentioned in ZcashFoundation/zebra#6392 (comment), I initially thought of refactoring check_note_validity directly, which wouldn't be worth it since it's called in other places. Only then I thought of not calling check_note_validity at all, which is a much simpler solution that avoids the redundant check.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (bc55893) 71.58% compared to head (dce52ab) 71.58%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #840      +/-   ##
==========================================
- Coverage   71.58%   71.58%   -0.01%     
==========================================
  Files         125      125              
  Lines       11847    11846       -1     
==========================================
- Hits         8481     8480       -1     
  Misses       3366     3366              

Impacted Files	Coverage Δ
components/zcash_note_encryption/src/lib.rs	88.66% <100.00%> (-0.08%)	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[daira]

This will require three ACKs since try_output_recovery_with_ock is used in consensus code.

[str4d]

In a gardening PR today, @daira, @nuttycom and I looked at this. We determined that while this change is correct, it leaves the burden of enforcing this necessary check on the domain implementations. I've opened #848 and zcash/orchard#394 with an alternative approach that instead reduces the requirements on the domain implementations, and relies on the central check instead.

[nuttycom]

Subsumed by #848