From 976313f0f4c492061689d84811b17ee1ea73cc8b Mon Sep 17 00:00:00 2001 From: Ori Newman Date: Tue, 12 Nov 2024 14:10:19 +0000 Subject: [PATCH 1/3] Some simplification to script number types --- crypto/txscript/src/data_stack.rs | 166 ++++++++++++-------------- crypto/txscript/src/lib.rs | 8 +- crypto/txscript/src/script_builder.rs | 4 +- 3 files changed, 79 insertions(+), 99 deletions(-) diff --git a/crypto/txscript/src/data_stack.rs b/crypto/txscript/src/data_stack.rs index cb5935bbbd..a98468d1b6 100644 --- a/crypto/txscript/src/data_stack.rs +++ b/crypto/txscript/src/data_stack.rs @@ -5,12 +5,47 @@ use kaspa_txscript_errors::SerializationError; use std::cmp::Ordering; 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 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 +146,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| i64::from(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 +183,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 +195,16 @@ impl OpcodeData> for Vec { #[inline] fn serialize(from: &SizedEncodeInt) -> Result { - Ok(serialize_i64(&from.0)) + // This is an optimzation for LEN 8. Without it, the function will fail when checking the serialized data length. + if LEN == 8 && from.0 == i64::MIN { + return Err(SerializationError::NumberTooLong(from.0)); + } + + let bytes = serialize_i64(&from.0); + if bytes.len() > LEN { + return Err(SerializationError::NumberTooLong(from.0)); + } + Ok(bytes) } } @@ -614,59 +600,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 f36307a60a..a82be592f6 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/script_builder.rs b/crypto/txscript/src/script_builder.rs index 466b8b4089..7a5b28ca5a 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) } From 760b8bbd3fd8e1b92dc72d8ec51fb2609ce96441 Mon Sep 17 00:00:00 2001 From: Ori Newman Date: Tue, 12 Nov 2024 15:21:46 +0000 Subject: [PATCH 2/3] Add TODO --- crypto/txscript/src/opcodes/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crypto/txscript/src/opcodes/mod.rs b/crypto/txscript/src/opcodes/mod.rs index c59bc27d91..5ee6fbaae1 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 { From 887270e056e63df7c0eb8b84f071075227efc533 Mon Sep 17 00:00:00 2001 From: Ori Newman Date: Tue, 12 Nov 2024 19:12:28 +0000 Subject: [PATCH 3/3] Address review comments --- crypto/txscript/src/data_stack.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/crypto/txscript/src/data_stack.rs b/crypto/txscript/src/data_stack.rs index a98468d1b6..898550fec2 100644 --- a/crypto/txscript/src/data_stack.rs +++ b/crypto/txscript/src/data_stack.rs @@ -3,6 +3,7 @@ use core::fmt::Debug; use core::iter; use kaspa_txscript_errors::SerializationError; use std::cmp::Ordering; +use std::num::TryFromIntError; use std::ops::Deref; #[derive(PartialEq, Eq, Debug, Default, PartialOrd, Ord)] @@ -20,6 +21,14 @@ impl From for SizedEncodeInt { } } +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 @@ -170,7 +179,7 @@ impl OpcodeData for Vec { impl OpcodeData for Vec { #[inline] fn deserialize(&self) -> Result { - OpcodeData::>::deserialize(self).map(|v| i64::from(v).try_into().expect("number is within i32 range")) + OpcodeData::>::deserialize(self).map(|v| v.try_into().expect("number is within i32 range")) } #[inline] @@ -195,11 +204,6 @@ impl OpcodeData> for Vec { #[inline] fn serialize(from: &SizedEncodeInt) -> Result { - // This is an optimzation for LEN 8. Without it, the function will fail when checking the serialized data length. - if LEN == 8 && from.0 == i64::MIN { - return Err(SerializationError::NumberTooLong(from.0)); - } - let bytes = serialize_i64(&from.0); if bytes.len() > LEN { return Err(SerializationError::NumberTooLong(from.0));