Fix SmallFp from_random_bytes inconsistency with Fp (#1082)

* smallfp from random bytes fix

* update comment, changelog

* fmt

* Update CHANGELOG.md

---------

Co-authored-by: Weikeng Chen <w.k@berkeley.edu>

Author: z-tech

CHANGELOG.md

@@ -26,6 +26,8 @@
 
 ### Bugfixes
 
+- [\#1082](https://github.com/arkworks-rs/algebra/pull/1082) (`ark-ff`) Fix `SmallFp::from_random_bytes` / `from_be_bytes_mod_order` silently producing incorrect field elements by treating plaintext bytes as Montgomery-encoded.
+
 ## v0.5.0
 
 - [\#772](https://github.com/arkworks-rs/algebra/pull/772) (`ark-ff`) Implementation of `mul` method for `BigInteger`.

ff/src/fields/models/small_fp/field.rs

@@ -1,6 +1,6 @@
 use crate::fields::models::small_fp::small_fp_backend::{SmallFp, SmallFpConfig};
 use crate::{Field, LegendreSymbol, One, PrimeField, SqrtPrecomputation, Zero};
-use ark_serialize::{buffer_byte_size, CanonicalDeserialize, EmptyFlags, Flags};
+use ark_serialize::{buffer_byte_size, EmptyFlags, Flags};
 use core::iter;
 
 impl<P: SmallFpConfig> Field for SmallFp<P> {
@@ -86,9 +86,9 @@ impl<P: SmallFpConfig> Field for SmallFp<P> {
                 }
                 *b &= m;
             }
-            Self::deserialize_compressed(&result_bytes.as_slice()[..(P::NUM_BIG_INT_LIMBS * 8)])
-                .ok()
-                .and_then(|f| F::from_u8(flags).map(|flag| (f, flag)))
+            // Use from_bigint (not deserialize_compressed) since these are plaintext bytes, not Montgomery-encoded.
+            let bigint = result_bytes.to_bigint();
+            Self::from_bigint(bigint).and_then(|f| F::from_u8(flags).map(|flag| (f, flag)))
         }
     }
 

ff/src/fields/models/small_fp/small_fp_backend.rs

@@ -326,6 +326,7 @@ impl<P: SmallFpConfig> ark_std::rand::distributions::Distribution<SmallFp<P>>
     }
 }
 
+#[derive(Debug)]
 pub enum ParseSmallFpError {
     Empty,
     InvalidFormat,

test-templates/src/fields.rs

@@ -647,6 +647,41 @@ macro_rules! __test_small_field {
                 },
             }
         }
+
+        #[test]
+        fn test_from_be_bytes_mod_order() {
+            use ark_ff::BigInteger;
+            use ark_std::str::FromStr;
+            use $crate::num_bigint::BigUint;
+            use ark_std::{rand::Rng, string::ToString, vec};
+
+            let ref_modulus = BigUint::from_bytes_be(&<$field>::MODULUS.to_bytes_be());
+
+            let mut test_vectors = vec![
+                vec![0u8],
+                vec![1u8],
+                vec![255u8],
+                vec![1u8, 0u8],
+                vec![1u8, 0u8, 255u8],
+            ];
+
+            // Add random bytestrings of various lengths
+            for i in 1..64 {
+                let mut rng = ark_std::test_rng();
+                let data: Vec<u8> = (0..i).map(|_| rng.gen()).collect();
+                test_vectors.push(data);
+            }
+
+            for bytes in test_vectors {
+                let mut expected_biguint = BigUint::from_bytes_be(&bytes);
+                expected_biguint =
+                    expected_biguint.modpow(&BigUint::from_bytes_be(&[1u8]), &ref_modulus);
+                let expected_string = expected_biguint.to_string();
+                let expected = <$field>::from_str(&expected_string).unwrap();
+                let actual = <$field>::from_be_bytes_mod_order(&bytes);
+                assert_eq!(expected, actual, "from_be_bytes_mod_order failed on {:?}", bytes);
+            }
+        }
     };
 }