From 116dfb0f886f99bbe2a9f77b133e7649d6d6a833 Mon Sep 17 00:00:00 2001 From: Ori Newman Date: Wed, 13 Nov 2024 12:34:01 +0200 Subject: [PATCH] Some simplification to script number types (#594) * Some simplification to script number types * Add TODO * Address review comments --- crypto/txscript/src/data_stack.rs | 170 ++++++++++++-------------- crypto/txscript/src/lib.rs | 8 +- crypto/txscript/src/opcodes/mod.rs | 1 + crypto/txscript/src/script_builder.rs | 4 +- 4 files changed, 84 insertions(+), 99 deletions(-) diff --git a/crypto/txscript/src/data_stack.rs b/crypto/txscript/src/data_stack.rs index cb5935bbb..898550fec 100644 --- a/crypto/txscript/src/data_stack.rs +++ b/crypto/txscript/src/data_stack.rs @@ -3,14 +3,58 @@ use core::fmt::Debug; use core::iter; use kaspa_txscript_errors::SerializationError; use std::cmp::Ordering; +use std::num::TryFromIntError; use std::ops::Deref; -const DEFAULT_SCRIPT_NUM_LEN: usize = 4; -const DEFAULT_SCRIPT_NUM_LEN_KIP10: usize = 8; - -#[derive(PartialEq, Eq, Debug, Default)] +#[derive(PartialEq, Eq, Debug, Default, PartialOrd, Ord)] pub(crate) struct SizedEncodeInt(pub(crate) i64); +impl From for SizedEncodeInt { + fn from(value: i64) -> Self { + SizedEncodeInt(value) + } +} + +impl From for SizedEncodeInt { + fn from(value: i32) -> Self { + SizedEncodeInt(value as i64) + } +} + +impl TryFrom> for i32 { + type Error = TryFromIntError; + + fn try_from(value: SizedEncodeInt) -> Result { + value.0.try_into() + } +} + +impl PartialEq for SizedEncodeInt { + fn eq(&self, other: &i64) -> bool { + self.0 == *other + } +} + +impl PartialOrd for SizedEncodeInt { + fn partial_cmp(&self, other: &i64) -> Option { + self.0.partial_cmp(other) + } +} + +impl Deref for SizedEncodeInt { + type Target = i64; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl From> for i64 { + fn from(value: SizedEncodeInt) -> Self { + value.0 + } +} + pub(crate) type Stack = Vec>; pub(crate) trait DataStack { @@ -111,94 +155,36 @@ fn deserialize_i64(v: &[u8]) -> Result { } } -#[derive(Debug, Copy, Clone, PartialEq, Eq, Ord, PartialOrd)] -#[repr(transparent)] -pub struct Kip10I64(pub i64); - -impl From for i64 { - fn from(value: Kip10I64) -> Self { - value.0 - } -} - -impl PartialEq for Kip10I64 { - fn eq(&self, other: &i64) -> bool { - self.0.eq(other) - } -} - -impl PartialOrd for Kip10I64 { - fn partial_cmp(&self, other: &i64) -> Option { - self.0.partial_cmp(other) - } -} - -impl Deref for Kip10I64 { - type Target = i64; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} - -impl OpcodeData for Vec { - #[inline] - fn deserialize(&self) -> Result { - match self.len() > DEFAULT_SCRIPT_NUM_LEN_KIP10 { - true => Err(TxScriptError::NumberTooBig(format!( - "numeric value encoded as {:x?} is {} bytes which exceeds the max allowed of {}", - self, - self.len(), - DEFAULT_SCRIPT_NUM_LEN_KIP10 - ))), - false => deserialize_i64(self).map(Kip10I64), - } - } - - #[inline] - fn serialize(from: &Kip10I64) -> Result { - if from.0 == i64::MIN { - return Err(SerializationError::NumberTooLong(from.0)); - } - Ok(serialize_i64(&from.0)) - } -} +// TODO: Rename to DefaultSizedEncodeInt when KIP-10 is activated +pub type Kip10I64 = SizedEncodeInt<8>; impl OpcodeData for Vec { #[inline] fn deserialize(&self) -> Result { - match self.len() > DEFAULT_SCRIPT_NUM_LEN { - true => Err(TxScriptError::NumberTooBig(format!( - "numeric value encoded as {:x?} is {} bytes which exceeds the max allowed of {}", - self, - self.len(), - DEFAULT_SCRIPT_NUM_LEN - ))), - false => deserialize_i64(self), - } + // TODO: Change LEN to 8 once KIP-10 is activated + OpcodeData::>::deserialize(self).map(i64::from) } #[inline] fn serialize(from: &i64) -> Result { - if from == &i64::MIN { - return Err(SerializationError::NumberTooLong(*from)); - } - Ok(serialize_i64(from)) + // Note that serialization and deserialization use different LEN. + // This is because prior to KIP-10, only deserialization size was limited. + // It's safe to use 8 here because i32 arithmetic operations (which were the + // only ones that were supported prior to KIP-10) can't get to i64::MIN + // (the only i64 value that requires more than 8 bytes to serialize). + OpcodeData::>::serialize(&(*from).into()) } } impl OpcodeData for Vec { #[inline] fn deserialize(&self) -> Result { - let res = OpcodeData::::deserialize(self)?; - // TODO: Consider getting rid of clamp, since the call to deserialize should return an error - // if the number is not in the i32 range (this should be done with proper testing)? - Ok(res.clamp(i32::MIN as i64, i32::MAX as i64) as i32) + OpcodeData::>::deserialize(self).map(|v| v.try_into().expect("number is within i32 range")) } #[inline] fn serialize(from: &i32) -> Result { - Ok(OpcodeData::::serialize(&(*from as i64)).expect("should never happen")) + OpcodeData::>::serialize(&(*from).into()) } } @@ -206,7 +192,7 @@ impl OpcodeData> for Vec { #[inline] fn deserialize(&self) -> Result, TxScriptError> { match self.len() > LEN { - true => Err(TxScriptError::InvalidState(format!( + true => Err(TxScriptError::NumberTooBig(format!( "numeric value encoded as {:x?} is {} bytes which exceeds the max allowed of {}", self, self.len(), @@ -218,7 +204,11 @@ impl OpcodeData> for Vec { #[inline] fn serialize(from: &SizedEncodeInt) -> Result { - Ok(serialize_i64(&from.0)) + let bytes = serialize_i64(&from.0); + if bytes.len() > LEN { + return Err(SerializationError::NumberTooLong(from.0)); + } + Ok(bytes) } } @@ -614,59 +604,59 @@ mod tests { let kip10_tests = vec![ TestCase:: { serialized: hex::decode("0000008000").expect("failed parsing hex"), - result: Ok(Kip10I64(2147483648)), + result: Ok(Kip10I64::from(2147483648i64)), }, TestCase:: { serialized: hex::decode("0000008080").expect("failed parsing hex"), - result: Ok(Kip10I64(-2147483648)), + result: Ok(Kip10I64::from(-2147483648i64)), }, TestCase:: { serialized: hex::decode("0000009000").expect("failed parsing hex"), - result: Ok(Kip10I64(2415919104)), + result: Ok(Kip10I64::from(2415919104i64)), }, TestCase:: { serialized: hex::decode("0000009080").expect("failed parsing hex"), - result: Ok(Kip10I64(-2415919104)), + result: Ok(Kip10I64::from(-2415919104i64)), }, TestCase:: { serialized: hex::decode("ffffffff00").expect("failed parsing hex"), - result: Ok(Kip10I64(4294967295)), + result: Ok(Kip10I64::from(4294967295i64)), }, TestCase:: { serialized: hex::decode("ffffffff80").expect("failed parsing hex"), - result: Ok(Kip10I64(-4294967295)), + result: Ok(Kip10I64::from(-4294967295i64)), }, TestCase:: { serialized: hex::decode("0000000001").expect("failed parsing hex"), - result: Ok(Kip10I64(4294967296)), + result: Ok(Kip10I64::from(4294967296i64)), }, TestCase:: { serialized: hex::decode("0000000081").expect("failed parsing hex"), - result: Ok(Kip10I64(-4294967296)), + result: Ok(Kip10I64::from(-4294967296i64)), }, TestCase:: { serialized: hex::decode("ffffffffffff00").expect("failed parsing hex"), - result: Ok(Kip10I64(281474976710655)), + result: Ok(Kip10I64::from(281474976710655i64)), }, TestCase:: { serialized: hex::decode("ffffffffffff80").expect("failed parsing hex"), - result: Ok(Kip10I64(-281474976710655)), + result: Ok(Kip10I64::from(-281474976710655i64)), }, TestCase:: { serialized: hex::decode("ffffffffffffff00").expect("failed parsing hex"), - result: Ok(Kip10I64(72057594037927935)), + result: Ok(Kip10I64::from(72057594037927935i64)), }, TestCase:: { serialized: hex::decode("ffffffffffffff80").expect("failed parsing hex"), - result: Ok(Kip10I64(-72057594037927935)), + result: Ok(Kip10I64::from(-72057594037927935i64)), }, TestCase:: { serialized: hex::decode("ffffffffffffff7f").expect("failed parsing hex"), - result: Ok(Kip10I64(9223372036854775807)), + result: Ok(Kip10I64::from(9223372036854775807i64)), }, TestCase:: { serialized: hex::decode("ffffffffffffffff").expect("failed parsing hex"), - result: Ok(Kip10I64(-9223372036854775807)), + result: Ok(Kip10I64::from(-9223372036854775807i64)), }, // Minimally encoded values that are out of range for data that // is interpreted as script numbers with the minimal encoding diff --git a/crypto/txscript/src/lib.rs b/crypto/txscript/src/lib.rs index f36307a60..a82be592f 100644 --- a/crypto/txscript/src/lib.rs +++ b/crypto/txscript/src/lib.rs @@ -1196,17 +1196,11 @@ mod bitcoind_tests { // Read the JSON contents of the file as an instance of `User`. let tests: Vec = serde_json::from_reader(reader).expect("Failed Parsing {:?}"); - let mut had_errors = 0; - let total_tests = tests.len(); for row in tests { if let Err(error) = row.test_row(kip10_enabled) { - println!("Test: {:?} failed: {:?}", row.clone(), error); - had_errors += 1; + panic!("Test: {:?} failed for {}: {:?}", row.clone(), file_name, error); } } - if had_errors > 0 { - panic!("{}/{} json tests failed", had_errors, total_tests) - } } } } diff --git a/crypto/txscript/src/opcodes/mod.rs b/crypto/txscript/src/opcodes/mod.rs index c59bc27d9..5ee6fbaae 100644 --- a/crypto/txscript/src/opcodes/mod.rs +++ b/crypto/txscript/src/opcodes/mod.rs @@ -219,6 +219,7 @@ fn push_number( /// This macro helps to avoid code duplication in numeric opcodes where the only difference /// between KIP10_ENABLED and disabled states is the numeric type used (Kip10I64 vs i64). /// KIP10I64 deserializator supports 8-byte integers +// TODO: Remove this macro after KIP-10 activation. macro_rules! numeric_op { ($vm: expr, $pattern: pat, $count: expr, $block: expr) => { if $vm.kip10_enabled { diff --git a/crypto/txscript/src/script_builder.rs b/crypto/txscript/src/script_builder.rs index 466b8b408..7a5b28ca5 100644 --- a/crypto/txscript/src/script_builder.rs +++ b/crypto/txscript/src/script_builder.rs @@ -1,7 +1,7 @@ use std::iter::once; use crate::{ - data_stack::OpcodeData, + data_stack::{Kip10I64, OpcodeData}, opcodes::{codes::*, OP_1_NEGATE_VAL, OP_DATA_MAX_VAL, OP_DATA_MIN_VAL, OP_SMALL_INT_MAX_VAL}, MAX_SCRIPTS_SIZE, MAX_SCRIPT_ELEMENT_SIZE, }; @@ -232,7 +232,7 @@ impl ScriptBuilder { return Ok(self); } - let bytes: Vec<_> = OpcodeData::::serialize(&val)?; + let bytes: Vec<_> = OpcodeData::::serialize(&val.into())?; self.add_data(&bytes) }