Merge pull request #6399 from str4d/6396-retroactive-zip-216

Retroactively enable ZIP 216 before NU5 activation

src/rust/include/librustzcash.h

@@ -97,7 +97,6 @@ extern "C" {
     /// The result is also of length 32 and placed in `result`.
     /// Returns false if the diversifier or pk_d is not valid
     bool librustzcash_sapling_compute_cmu(
-        bool zip216_enabled,
         const unsigned char *diversifier,
         const unsigned char *pk_d,
         const uint64_t value,
@@ -111,7 +110,6 @@ extern "C" {
     /// the result is written to the 32-byte
     /// `result` buffer.
     bool librustzcash_sapling_ka_agree(
-        bool zip216_enabled,
         const unsigned char *p,
         const unsigned char *sk,
         unsigned char *result

src/rust/src/rustzcash.rs

@@ -356,7 +356,6 @@ pub extern "C" fn librustzcash_sapling_generate_r(result: *mut [c_uchar; 32]) {
 
 // Private utility function to get Note from C parameters
 fn priv_get_note(
-    zip216_enabled: bool,
     diversifier: *const [c_uchar; 11],
     pk_d: *const [c_uchar; 32],
     value: u64,
@@ -365,12 +364,7 @@ fn priv_get_note(
     let diversifier = Diversifier(unsafe { *diversifier });
     let g_d = diversifier.g_d().ok_or(())?;
 
-    let pk_d = de_ct(if zip216_enabled {
+    let pk_d = de_ct(jubjub::ExtendedPoint::from_bytes(unsafe { &*pk_d })).ok_or(())?;
-        jubjub::ExtendedPoint::from_bytes(unsafe { &*pk_d })
-    } else {
-        jubjub::AffinePoint::from_bytes_pre_zip216_compatibility(unsafe { *pk_d }).map(|p| p.into())
-    })
-    .ok_or(())?;
 
     let pk_d = de_ct(pk_d.into_subgroup()).ok_or(())?;
 
@@ -407,7 +401,8 @@ pub extern "C" fn librustzcash_sapling_compute_nf(
 ) -> bool {
     // ZIP 216: Nullifier derivation is not consensus-critical
     // (nullifiers are revealed, not calculated by consensus).
-    let note = match priv_get_note(true, diversifier, pk_d, value, rcm) {
+    // In any case, ZIP 216 is now enabled retroactively.
+    let note = match priv_get_note(diversifier, pk_d, value, rcm) {
         Ok(p) => p,
         Err(_) => return false,
     };
@@ -437,14 +432,13 @@ pub extern "C" fn librustzcash_sapling_compute_nf(
 /// Returns false if `diversifier` or `pk_d` is not valid.
 #[no_mangle]
 pub extern "C" fn librustzcash_sapling_compute_cmu(
-    zip216_enabled: bool,
     diversifier: *const [c_uchar; 11],
     pk_d: *const [c_uchar; 32],
     value: u64,
     rcm: *const [c_uchar; 32],
     result: *mut [c_uchar; 32],
 ) -> bool {
-    let note = match priv_get_note(zip216_enabled, diversifier, pk_d, value, rcm) {
+    let note = match priv_get_note(diversifier, pk_d, value, rcm) {
         Ok(p) => p,
         Err(_) => return false,
     };
@@ -461,17 +455,12 @@ pub extern "C" fn librustzcash_sapling_compute_cmu(
 /// the 32-byte `result` buffer.
 #[no_mangle]
 pub extern "C" fn librustzcash_sapling_ka_agree(
-    zip216_enabled: bool,
     p: *const [c_uchar; 32],
     sk: *const [c_uchar; 32],
     result: *mut [c_uchar; 32],
 ) -> bool {
     // Deserialize p
-    let p = match de_ct(if zip216_enabled {
+    let p = match de_ct(jubjub::ExtendedPoint::from_bytes(unsafe { &*p })) {
-        jubjub::ExtendedPoint::from_bytes(unsafe { &*p })
-    } else {
-        jubjub::AffinePoint::from_bytes_pre_zip216_compatibility(unsafe { *p }).map(|p| p.into())
-    }) {
         Some(p) => p,
         None => return false,
     };

src/rust/src/tests/key_agreement.rs

@@ -43,7 +43,6 @@ fn test_key_agreement() {
     let addr_pk_d = addr.pk_d().to_bytes();
 
     assert!(librustzcash_sapling_ka_agree(
-        true,
         &addr_pk_d,
         &esk,
         &mut shared_secret_sender
@@ -61,7 +60,6 @@ fn test_key_agreement() {
     // Create sharedSecret with ephemeral key
     let mut shared_secret_recipient = [0u8; 32];
     assert!(librustzcash_sapling_ka_agree(
-        true,
         &epk,
         &ivk_serialized,
         &mut shared_secret_recipient

src/rust/src/tests/notes.rs

@@ -649,7 +649,6 @@ fn notes() {
         // Compute commitment and compare with test vector
         let mut result = [0u8; 32];
         assert!(librustzcash_sapling_compute_cmu(
-            true,
             &tv.default_d,
             &tv.default_pk_d,
             tv.note_v,

src/wallet/wallet.cpp

@@ -4415,8 +4415,6 @@ std::optional<std::pair<
     SaplingPaymentAddress>> CWalletTx::RecoverSaplingNoteWithoutLeadByteCheck(SaplingOutPoint op, std::set<uint256>& ovks) const
 {
     auto output = this->vShieldedOutput[op.n];
-    // ZIP 216: This wallet method is not called from consensus rules.
-    bool zip216Enabled = true;
 
     for (auto ovk : ovks) {
         auto outPt = SaplingOutgoingPlaintext::decrypt(
@@ -4431,14 +4429,13 @@ std::optional<std::pair<
         }
 
         auto optDeserialized = SaplingNotePlaintext::attempt_sapling_enc_decryption_deserialization(
-            zip216Enabled, output.encCiphertext, output.ephemeralKey, outPt->esk, outPt->pk_d);
+            output.encCiphertext, output.ephemeralKey, outPt->esk, outPt->pk_d);
 
         // The transaction would not have entered the wallet unless
         // its plaintext had been successfully decrypted previously.
         assert(optDeserialized != std::nullopt);
 
         auto maybe_pt = SaplingNotePlaintext::plaintext_checks_without_height(
-            zip216Enabled,
             *optDeserialized,
             output.ephemeralKey,
             outPt->esk,

src/zcash/Note.cpp

@@ -66,9 +66,10 @@ SaplingNote::SaplingNote(
 std::optional<uint256> SaplingNote::cmu() const {
     uint256 result;
     uint256 rcm_tmp = rcm();
-    // ZIP 216: This method is only called from test code.
+    // We consider ZIP 216 active all of the time because blocks prior to NU5
+    // activation (on mainnet and testnet) did not contain Sapling transactions
+    // that violated its canonicity rule.
     if (!librustzcash_sapling_compute_cmu(
-            true,
             d.data(),
             pk_d.begin(),
             value(),
@@ -286,7 +287,6 @@ std::optional<SaplingNotePlaintext> SaplingNotePlaintext::plaintext_checks_witho
     uint256 cmu_expected;
     uint256 rcm = plaintext.rcm();
     if (!librustzcash_sapling_compute_cmu(
-        true,
         plaintext.d.data(),
         pk_d.begin(),
         plaintext.value(),
@@ -328,12 +328,10 @@ std::optional<SaplingNotePlaintext> SaplingNotePlaintext::decrypt(
     const uint256 &cmu
 )
 {
-    // The nu5Active flag passed in here enables the new consensus rules from ZIP 216
+    // We consider ZIP 216 active all of the time because blocks prior to NU5
-    // (https://zips.z.cash/zip-0216#specification) on the following fields:
+    // activation (on mainnet and testnet) did not contain Sapling transactions
-    //
+    // that violated its canonicity rule.
-    // - pk_d in the outCiphertext field of Sapling coinbase outputs.
+    auto ret = attempt_sapling_enc_decryption_deserialization(ciphertext, epk, esk, pk_d);
-    bool nu5Active = params.NetworkUpgradeActive(height, Consensus::UPGRADE_NU5);
-    auto ret = attempt_sapling_enc_decryption_deserialization(nu5Active, ciphertext, epk, esk, pk_d);
 
     if (!ret) {
         return std::nullopt;
@@ -347,19 +345,18 @@ std::optional<SaplingNotePlaintext> SaplingNotePlaintext::decrypt(
             return std::nullopt;
         }
 
-        return plaintext_checks_without_height(nu5Active, plaintext, epk, esk, pk_d, cmu);
+        return plaintext_checks_without_height(plaintext, epk, esk, pk_d, cmu);
     }
 }
 
 std::optional<SaplingNotePlaintext> SaplingNotePlaintext::attempt_sapling_enc_decryption_deserialization(
-    bool zip216Enabled,
     const SaplingEncCiphertext &ciphertext,
     const uint256 &epk,
     const uint256 &esk,
     const uint256 &pk_d
 )
 {
-    auto encPlaintext = AttemptSaplingEncDecryption(zip216Enabled, ciphertext, epk, esk, pk_d);
+    auto encPlaintext = AttemptSaplingEncDecryption(ciphertext, epk, esk, pk_d);
 
     if (!encPlaintext) {
         return std::nullopt;
@@ -381,7 +378,6 @@ std::optional<SaplingNotePlaintext> SaplingNotePlaintext::attempt_sapling_enc_de
 }
 
 std::optional<SaplingNotePlaintext> SaplingNotePlaintext::plaintext_checks_without_height(
-    bool zip216Enabled,
     const SaplingNotePlaintext &plaintext,
     const uint256 &epk,
     const uint256 &esk,
@@ -411,7 +407,6 @@ std::optional<SaplingNotePlaintext> SaplingNotePlaintext::plaintext_checks_witho
     uint256 cmu_expected;
     uint256 rcm = plaintext.rcm();
     if (!librustzcash_sapling_compute_cmu(
-        zip216Enabled,
         plaintext.d.data(),
         pk_d.begin(),
         plaintext.value(),

src/zcash/Note.hpp

@@ -193,7 +193,6 @@ public:
     );
 
     static std::optional<SaplingNotePlaintext> plaintext_checks_without_height(
-        bool zip216Enabled,
         const SaplingNotePlaintext &plaintext,
         const uint256 &epk,
         const uint256 &esk,
@@ -202,7 +201,6 @@ public:
     );
 
     static std::optional<SaplingNotePlaintext> attempt_sapling_enc_decryption_deserialization(
-        bool zip216Enabled,
         const SaplingEncCiphertext &ciphertext,
         const uint256 &epk,
         const uint256 &esk,

src/zcash/NoteEncryption.cpp

@@ -115,9 +115,9 @@ std::optional<SaplingEncCiphertext> SaplingNoteEncryption::encrypt_to_recipient(
     uint256 dhsecret;
 
     // The new consensus rules from ZIP 216 (https://zips.z.cash/zip-0216#specification)
-    // on pk_d are enabled unconditionally, as they MAY be enforced in advance of NU5
+    // on pk_d were enabled unconditionally, even before we started to apply them
-    // activation.
+    // retroactively.
-    if (!librustzcash_sapling_ka_agree(true, pk_d.begin(), esk.begin(), dhsecret.begin())) {
+    if (!librustzcash_sapling_ka_agree(pk_d.begin(), esk.begin(), dhsecret.begin())) {
         return std::nullopt;
     }
 
@@ -150,9 +150,10 @@ std::optional<SaplingEncPlaintext> AttemptSaplingEncDecryption(
 {
     uint256 dhsecret;
 
-    // ZIP 216: We can enable the rules unconditionally, because ephemeralKey has always
+    // We consider ZIP 216 active all of the time because blocks prior to NU5
-    // been required to not be small-order (https://zips.z.cash/zip-0216#specification).
+    // activation (on mainnet and testnet) did not contain Sapling transactions
-    if (!librustzcash_sapling_ka_agree(true, epk.begin(), ivk.begin(), dhsecret.begin())) {
+    // that violated its canonicity rule.
+    if (!librustzcash_sapling_ka_agree(epk.begin(), ivk.begin(), dhsecret.begin())) {
         return std::nullopt;
     }
 
@@ -180,7 +181,6 @@ std::optional<SaplingEncPlaintext> AttemptSaplingEncDecryption(
 }
 
 std::optional<SaplingEncPlaintext> AttemptSaplingEncDecryption (
-    bool zip216Enabled,
     const SaplingEncCiphertext &ciphertext,
     const uint256 &epk,
     const uint256 &esk,
@@ -189,7 +189,10 @@ std::optional<SaplingEncPlaintext> AttemptSaplingEncDecryption (
 {
     uint256 dhsecret;
 
-    if (!librustzcash_sapling_ka_agree(zip216Enabled, pk_d.begin(), esk.begin(), dhsecret.begin())) {
+    // We consider ZIP 216 active all of the time because blocks prior to NU5
+    // activation (on mainnet and testnet) did not contain Sapling transactions
+    // that violated its canonicity rule.
+    if (!librustzcash_sapling_ka_agree(pk_d.begin(), esk.begin(), dhsecret.begin())) {
         return std::nullopt;
     }
 

src/zcash/NoteEncryption.hpp

@@ -77,7 +77,6 @@ std::optional<SaplingEncPlaintext> AttemptSaplingEncDecryption(
 // Attempts to decrypt a Sapling note using outgoing plaintext.
 // This will not check that the contents of the ciphertext are correct.
 std::optional<SaplingEncPlaintext> AttemptSaplingEncDecryption (
-    bool zip216Enabled,
     const SaplingEncCiphertext &ciphertext,
     const uint256 &epk,
     const uint256 &esk,