Addressing minor issues (#444)

* Addressing minor issues

* Fix issue with quickcheck

* Fix constrains

* Renamed `InstructionResult` into `PanicInstruction`.
Made `PanicReason` non-optional for `PanicInstruction`.

---------

Co-authored-by: Mitch Martin <[email protected]>

fuel-asm/src/encoding_tests.rs

@@ -102,39 +102,27 @@ fn panic_reason_description() {
         PanicReason::ExpectedParentInternalContext,
         PanicReason::IllegalJump,
         PanicReason::ArithmeticError,
+        PanicReason::ContractInstructionNotAllowed,
     ];
 
-    let pd = InstructionResult::error(PanicReason::Success, op::noop().into());
-    let w = Word::from(pd);
-    let pd_p = InstructionResult::from(w);
-    assert_eq!(pd, pd_p);
-
-    #[cfg(feature = "serde")]
-    {
-        let pd_s = bincode::serialize(&pd).expect("Failed to serialize instruction");
-        let pd_s: InstructionResult = bincode::deserialize(&pd_s).expect("Failed to deserialize instruction");
-
-        assert_eq!(pd_s, pd);
-    }
-
     for r in reasons {
         let b = r as u8;
-        let r_p = PanicReason::from(b);
+        let r_p = PanicReason::try_from(b).expect("Should get panic reason");
         let w = Word::from(r as u8);
-        let r_q = PanicReason::from(u8::try_from(w).unwrap());
+        let r_q = PanicReason::try_from(u8::try_from(w).unwrap()).expect("Should get panic reason");
         assert_eq!(r, r_p);
         assert_eq!(r, r_q);
 
         let op = op::ji(imm24);
-        let pd = InstructionResult::error(r, op.into());
+        let pd = PanicInstruction::error(r, op.into());
         let w = Word::from(pd);
-        let pd_p = InstructionResult::from(w);
+        let pd_p = PanicInstruction::try_from(w).expect("Should get panic reason");
         assert_eq!(pd, pd_p);
 
         #[cfg(feature = "serde")]
         {
             let pd_s = bincode::serialize(&pd).expect("Failed to serialize instruction");
-            let pd_s: InstructionResult = bincode::deserialize(&pd_s).expect("Failed to deserialize instruction");
+            let pd_s: PanicInstruction = bincode::deserialize(&pd_s).expect("Failed to deserialize instruction");
 
             assert_eq!(pd_s, pd);
         }

fuel-asm/src/lib.rs

@@ -8,7 +8,7 @@
 #![deny(unused_crate_dependencies)]
 
 mod args;
-mod instruction_result;
+mod panic_instruction;
 // This is `pub` to make documentation for the private `impl_instructions!` macro more accessible.
 #[macro_use]
 pub mod macros;
@@ -23,7 +23,7 @@ mod encoding_tests;
 #[doc(no_inline)]
 pub use args::{wideint, GMArgs, GTFArgs};
 pub use fuel_types::{RegisterId, Word};
-pub use instruction_result::InstructionResult;
+pub use panic_instruction::PanicInstruction;
 pub use panic_reason::PanicReason;
 
 /// Represents a 6-bit register ID, guaranteed to be masked by construction.

fuel-asm/src/instruction_result.rs → fuel-asm/src/panic_instruction.rs

@@ -1,15 +1,16 @@
+use crate::panic_reason::InvalidPanicReason;
 use crate::{Instruction, PanicReason, RawInstruction, Word};
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 /// Describe a panic reason with the instruction that generated it
-pub struct InstructionResult {
+pub struct PanicInstruction {
     reason: PanicReason,
     instruction: RawInstruction,
 }
 
-impl InstructionResult {
+impl PanicInstruction {
     /// Represents an error described by a reason and an instruction.
     pub const fn error(reason: PanicReason, instruction: RawInstruction) -> Self {
         Self { reason, instruction }
@@ -24,37 +25,29 @@ impl InstructionResult {
     pub const fn instruction(&self) -> &RawInstruction {
         &self.instruction
     }
-
-    /// This result represents success?
-    pub const fn is_success(&self) -> bool {
-        matches!(self.reason, PanicReason::Success)
-    }
-
-    /// This result represents error?
-    pub const fn is_error(&self) -> bool {
-        !self.is_success()
-    }
 }
 
 const WORD_SIZE: usize = core::mem::size_of::<Word>();
 const REASON_OFFSET: Word = (WORD_SIZE * 8 - 8) as Word;
 const INSTR_OFFSET: Word = REASON_OFFSET - (Instruction::SIZE * 8) as Word;
 
-impl From<InstructionResult> for Word {
-    fn from(r: InstructionResult) -> Word {
+impl From<PanicInstruction> for Word {
+    fn from(r: PanicInstruction) -> Word {
         let reason = Word::from(r.reason as u8);
         let instruction = Word::from(r.instruction);
         (reason << REASON_OFFSET) | (instruction << INSTR_OFFSET)
     }
 }
 
-impl From<Word> for InstructionResult {
-    fn from(val: Word) -> Self {
+impl TryFrom<Word> for PanicInstruction {
+    type Error = InvalidPanicReason;
+
+    fn try_from(val: Word) -> Result<Self, Self::Error> {
         // Safe to cast as we've shifted the 8 MSB.
         let reason_u8 = (val >> REASON_OFFSET) as u8;
         // Cast to truncate in order to remove the `reason` bits.
         let instruction = (val >> INSTR_OFFSET) as u32;
-        let reason = PanicReason::from(reason_u8);
-        Self { reason, instruction }
+        let reason = PanicReason::try_from(reason_u8)?;
+        Ok(Self { reason, instruction })
     }
 }

fuel-asm/src/panic_reason.rs

@@ -7,8 +7,6 @@ use core::{convert, fmt};
 #[non_exhaustive]
 /// Panic reason representation for the interpreter.
 pub enum PanicReason {
-    /// 0 is reserved for success, while any non-zero value indicates a failure.
-    Success = 0x00,
     /// Found `RVRT` instruction.
     Revert = 0x01,
     /// Execution ran out of gas.
@@ -82,8 +80,10 @@ pub enum PanicReason {
     /// For instance, division by zero produces this.
     /// These errors are ignored using the UNSAFEMATH flag.
     ArithmeticError = 0x23,
+    /// The contract instruction is not allowed in predicates.
+    ContractInstructionNotAllowed = 0x24,
     /// The byte can't be mapped to any known `PanicReason`.
-    UnknownPanicReason = 0x24,
+    UnknownPanicReason = 0x25,
 }
 
 impl fmt::Display for PanicReason {
@@ -106,11 +106,21 @@ impl From<convert::Infallible> for PanicReason {
     }
 }
 
-impl From<u8> for PanicReason {
-    fn from(b: u8) -> Self {
+/// Failed to parse a `u8` as a valid panic reason.
+#[derive(Debug, Eq, PartialEq)]
+pub struct InvalidPanicReason;
+
+impl TryFrom<u8> for PanicReason {
+    type Error = InvalidPanicReason;
+
+    /// Converts the `u8` into a `PanicReason`.
+    fn try_from(b: u8) -> Result<Self, Self::Error> {
+        if b == 0 {
+            return Err(InvalidPanicReason);
+        }
+
         use PanicReason::*;
-        match b {
-            0x00 => Success,
+        let reason = match b {
             0x01 => Revert,
             0x02 => OutOfGas,
             0x03 => TransactionValidity,
@@ -146,8 +156,20 @@ impl From<u8> for PanicReason {
             0x21 => ContractMismatch,
             0x22 => MessageDataTooLong,
             0x23 => ArithmeticError,
+            0x24 => ContractInstructionNotAllowed,
             _ => UnknownPanicReason,
-        }
+        };
+
+        Ok(reason)
+    }
+}
+
+#[cfg(feature = "std")]
+impl From<InvalidPanicReason> for std::io::Error {
+    fn from(_: InvalidPanicReason) -> Self {
+        use std::io;
+
+        io::Error::new(io::ErrorKind::InvalidInput, "Panic reason can't be zero")
     }
 }
 
@@ -172,14 +194,17 @@ mod tests {
 
     #[test]
     fn test_u8_panic_reason_round_trip() {
-        const LAST_PANIC_REASON: u8 = 0x24;
-        for i in 0..LAST_PANIC_REASON {
-            let reason = PanicReason::from(i);
+        const LAST_PANIC_REASON: u8 = PanicReason::UnknownPanicReason as u8;
+        let reason = PanicReason::try_from(0);
+        assert!(reason.is_err());
+
+        for i in 1..LAST_PANIC_REASON {
+            let reason = PanicReason::try_from(i).unwrap();
             let i2 = reason as u8;
             assert_eq!(i, i2);
         }
         for i in LAST_PANIC_REASON..=255 {
-            let reason = PanicReason::from(i);
+            let reason = PanicReason::try_from(i).unwrap();
             let i2 = reason as u8;
             assert_eq!(PanicReason::UnknownPanicReason as u8, i2);
         }

fuel-crypto/benches/signature.rs

@@ -100,7 +100,7 @@ fn signatures(c: &mut Criterion) {
 
     let mut group_sign = c.benchmark_group("sign");
 
-    group_sign.bench_with_input("fuel-crypto", &(fc_key, fc_message), |b, (key, message)| {
+    group_sign.bench_with_input("fuel-crypto-sign", &(fc_key, fc_message), |b, (key, message)| {
         b.iter(|| fuel_crypto::Signature::sign(black_box(key), black_box(message)))
     });
 
@@ -139,7 +139,7 @@ fn signatures(c: &mut Criterion) {
     let mut group_verify = c.benchmark_group("verify");
 
     group_verify.bench_with_input(
-        "fuel-crypto",
+        "fuel-crypto-verify",
         &(fc_public, fc_signature, fc_message),
         |b, (public, signature, message)| b.iter(|| signature.verify(black_box(public), black_box(message))),
     );
@@ -168,9 +168,11 @@ fn signatures(c: &mut Criterion) {
 
     let mut group_recover = c.benchmark_group("recover");
 
-    group_recover.bench_with_input("fuel-crypto", &(fc_signature, fc_message), |b, (signature, message)| {
-        b.iter(|| signature.recover(black_box(message)))
-    });
+    group_recover.bench_with_input(
+        "fuel-crypto-recover",
+        &(fc_signature, fc_message),
+        |b, (signature, message)| b.iter(|| signature.recover(black_box(message))),
+    );
 
     group_recover.bench_with_input(
         "secp256k1",

fuel-crypto/src/public.rs

@@ -112,7 +112,7 @@ mod use_std {
             Self::from_bytes_unchecked(bytes)
         }
 
-        pub(crate) fn _to_secp(&self) -> Result<Secp256k1PublicKey, Error> {
+        pub(crate) fn to_secp(&self) -> Result<Secp256k1PublicKey, Error> {
             let mut pk = [SECP_UNCOMPRESSED_FLAG; UNCOMPRESSED_PUBLIC_KEY_SIZE];
 
             debug_assert_eq!(SECP_UNCOMPRESSED_FLAG, pk[0]);

fuel-crypto/src/signature.rs

@@ -128,7 +128,7 @@ mod use_std {
     impl Signature {
         // Internal API - this isn't meant to be made public because some assumptions and pre-checks
         // are performed prior to this call
-        pub(crate) fn to_secp(&mut self) -> SecpRecoverableSignature {
+        fn to_secp(&mut self) -> SecpRecoverableSignature {
             let v = (self.as_mut()[32] >> 7) as i32;
 
             self.truncate_recovery_id();
@@ -143,7 +143,7 @@ mod use_std {
             signature
         }
 
-        pub(crate) fn from_secp(signature: SecpRecoverableSignature) -> Self {
+        fn from_secp(signature: SecpRecoverableSignature) -> Self {
             let (v, mut signature) = signature.serialize_compact();
 
             let v = v.to_i32();
@@ -154,7 +154,7 @@ mod use_std {
 
         /// Truncate the recovery id from the signature, producing a valid `secp256k1`
         /// representation.
-        pub(crate) fn truncate_recovery_id(&mut self) {
+        fn truncate_recovery_id(&mut self) {
             self.as_mut()[32] &= 0x7f;
         }
 
@@ -193,13 +193,13 @@ mod use_std {
         /// It takes the signature as owned because this operation is not idempotent. The taken
         /// signature will not be recoverable. Signatures are meant to be single use, so this
         /// avoids unnecessary copy.
-        pub fn verify(self, pk: &PublicKey, message: &Message) -> Result<(), Error> {
-            // TODO evaluate if its worthy to use native verify
-            //
-            // https://github.com/FuelLabs/fuel-crypto/issues/4
-
-            self.recover(message)
-                .and_then(|pk_p| (pk == &pk_p).then_some(()).ok_or(Error::InvalidSignature))
+        pub fn verify(mut self, pk: &PublicKey, message: &Message) -> Result<(), Error> {
+            let signature = self.to_secp().to_standard();
+            let message = message.to_secp();
+            let pk = pk.to_secp()?;
+            RECOVER_SECP
+                .verify_ecdsa(&message, &signature, &pk)
+                .map_err(|_| Error::InvalidSignature)
         }
     }
 }

fuel-tx/src/builder.rs

@@ -288,7 +288,7 @@ impl<Tx: Buildable> TransactionBuilder<Tx> {
     }
 
     #[cfg(feature = "std")]
-    pub fn _finalize(&mut self) -> Tx {
+    fn finalize_inner(&mut self) -> Tx {
         self.prepare_finalize();
 
         let mut tx = core::mem::take(&mut self.tx);
@@ -301,7 +301,7 @@ impl<Tx: Buildable> TransactionBuilder<Tx> {
     }
 
     #[cfg(feature = "std")]
-    pub fn _finalize_without_signature(&mut self) -> Tx {
+    pub fn finalize_without_signature_inner(&mut self) -> Tx {
         self.prepare_finalize();
 
         let mut tx = core::mem::take(&mut self.tx);
@@ -341,22 +341,22 @@ impl Finalizable<Mint> for TransactionBuilder<Mint> {
 #[cfg(feature = "std")]
 impl Finalizable<Create> for TransactionBuilder<Create> {
     fn finalize(&mut self) -> Create {
-        self._finalize()
+        self.finalize_inner()
     }
 
     fn finalize_without_signature(&mut self) -> Create {
-        self._finalize_without_signature()
+        self.finalize_without_signature_inner()
     }
 }
 
 #[cfg(feature = "std")]
 impl Finalizable<Script> for TransactionBuilder<Script> {
     fn finalize(&mut self) -> Script {
-        self._finalize()
+        self.finalize_inner()
     }
 
     fn finalize_without_signature(&mut self) -> Script {
-        self._finalize_without_signature()
+        self.finalize_without_signature_inner()
     }
 }
 

fuel-tx/src/lib.rs

@@ -16,7 +16,7 @@ extern crate core;
 pub mod consts;
 mod tx_pointer;
 
-pub use fuel_asm::{InstructionResult, PanicReason};
+pub use fuel_asm::{PanicInstruction, PanicReason};
 pub use fuel_types::{Address, AssetId, Bytes32, Bytes4, Bytes64, Bytes8, ContractId, MessageId, Salt, Word};
 pub use tx_pointer::TxPointer;
 

fuel-tx/src/receipt.rs

@@ -6,7 +6,7 @@ use crate::receipt::sizes::{
 use crate::Output;
 use alloc::vec::Vec;
 use derivative::Derivative;
-use fuel_asm::InstructionResult;
+use fuel_asm::PanicInstruction;
 use fuel_types::bytes::{self, padded_len_usize, SizedBytes, WORD_SIZE};
 use fuel_types::{fmt_truncated_hex, Address, AssetId, Bytes32, ContractId, MessageId, Nonce, Word};
 
@@ -58,7 +58,7 @@ pub enum Receipt {
 
     Panic {
         id: ContractId,
-        reason: InstructionResult,
+        reason: PanicInstruction,
         pc: Word,
         is: Word,
         #[derivative(PartialEq = "ignore", Hash = "ignore")]
@@ -186,7 +186,7 @@ impl Receipt {
         }
     }
 
-    pub const fn panic(id: ContractId, reason: InstructionResult, pc: Word, is: Word) -> Self {
+    pub const fn panic(id: ContractId, reason: PanicInstruction, pc: Word, is: Word) -> Self {
         Self::Panic {
             id,
             reason,
@@ -496,7 +496,7 @@ impl Receipt {
         }
     }
 
-    pub const fn reason(&self) -> Option<InstructionResult> {
+    pub const fn reason(&self) -> Option<PanicInstruction> {
         match self {
             Self::Panic { reason, .. } => Some(*reason),
             _ => None,