iron-fish / ironfish

A novel cryptocurrency focused on privacy and accessibility.
https://ironfish.network
Mozilla Public License 2.0
964 stars 579 forks source link

Fast construction of `NoteEncrypted` during wallet scans #5075

Closed andiflabs closed 1 week ago

andiflabs commented 1 week ago

Summary

Wallet scans get the blocks/transactions/notes from the chain db. The chain db contains data that has already been validated in the past (or that otherwise is assumed to be valid). For this reason, it should be possible to speed up scans by skipping expensive validation performed when parsing notes.

More specifically, NoteEncrypted parses some elliptic curve points in compressed form. The regular parsing done through jubjub::SubgroupPoint::from_bytes() involves an expensive check: it checks whether the uncompressed point belongs to the prime-order subgroup. This check is perfromed using elliptic curve point multiplication, which is the most predominant and slowest operation happening during wallet scans.

Due to this check, for each note, the wallet currently performs two elliptic curve point multiplications: one to parse the note, another to attempt the actual decryption. By using from_bytes_unchecked() instead of from_bytes() we can skip the first multiplication, effectively improving note decryption performance by a factor of almost 2x.

This change only affects the performance of wallet scans. Note parsing and decryption outside of a scan is unaffected at the moment. In the future, we can disable validation in other contexts as we see fit.

Testing Plan

Observe the number of elliptic curve point multiplications happening during a scan before and after this change (using the feature introduced in GH-5074).

Documentation

N/A

Breaking Change

N/A

NullSoldier commented 1 week ago

I've reviewed this but mostly from a learning perspective, not a bug perspective so take my approve with a grain of salt.

I did ultimately track down the difference in checked vs unchecked to here, if I'm correct. https://github.com/iron-fish/jubjub/blob/47dfe5181ccf39166c0c479c35c0644d708f4294/src/lib.rs#L1424-L1433

andiflabs commented 1 week ago

I've reviewed this but mostly from a learning perspective, not a bug perspective so take my approve with a grain of salt.

I did ultimately track down the difference in checked vs unchecked to here, if I'm correct. https://github.com/iron-fish/jubjub/blob/47dfe5181ccf39166c0c479c35c0644d708f4294/src/lib.rs#L1424-L1433

Yes, that's correct. The checked function calls into_subgroup(), the unchecked variant doesn't. into_subgroup() calls is_torsion_free(), which in turn calls multiply(), which is the expensive operation that we want to get rid of.