Fixing output of eth_call and Bytes deserialization (#2230)

* Fixing eth_call to builtins

* Fixing bytes deserialization

* Removing comment

ethcore/src/builtin.rs

@@ -17,14 +17,15 @@
 use crypto::sha2::Sha256 as Sha256Digest;
 use crypto::ripemd160::Ripemd160 as Ripemd160Digest;
 use crypto::digest::Digest;
-use util::*;
+use std::cmp::min;
+use util::{U256, H256, Hashable, FixedHash, BytesRef};
 use ethkey::{Signature, recover as ec_recover};
 use ethjson;
 
 /// Native implementation of a built-in contract.
 pub trait Impl: Send + Sync {
 	/// execute this built-in on the given input, writing to the given output.
-	fn execute(&self, input: &[u8], out: &mut [u8]);
+	fn execute(&self, input: &[u8], output: &mut BytesRef);
 }
 
 /// A gas pricing scheme for built-in contracts.
@@ -56,7 +57,7 @@ impl Builtin {
 	pub fn cost(&self, s: usize) -> U256 { self.pricer.cost(s) }
 
 	/// Simple forwarder for execute.
-	pub fn execute(&self, input: &[u8], output: &mut[u8]) { self.native.execute(input, output) }
+	pub fn execute(&self, input: &[u8], output: &mut BytesRef) { self.native.execute(input, output) }
 }
 
 impl From<ethjson::spec::Builtin> for Builtin {
@@ -108,14 +109,13 @@ struct Sha256;
 struct Ripemd160;
 
 impl Impl for Identity {
-	fn execute(&self, input: &[u8], output: &mut [u8]) {
+	fn execute(&self, input: &[u8], output: &mut BytesRef) {
-		let len = min(input.len(), output.len());
+		output.write(0, input);
-		output[..len].copy_from_slice(&input[..len]);
 	}
 }
 
 impl Impl for EcRecover {
-	fn execute(&self, i: &[u8], output: &mut [u8]) {
+	fn execute(&self, i: &[u8], output: &mut BytesRef) {
 		let len = min(i.len(), 128);
 
 		let mut input = [0; 128];
@@ -135,58 +135,34 @@ impl Impl for EcRecover {
 		if s.is_valid() {
 			if let Ok(p) = ec_recover(&s, &hash) {
 				let r = p.sha3();
-
+				output.write(0, &[0; 12]);
-				let out_len = min(output.len(), 32);
+				output.write(12, &r[12..r.len()]);
-
-				for x in &mut output[0.. min(12, out_len)] {
-					*x = 0;
-				}
-
-				if out_len > 12 {
-					output[12..out_len].copy_from_slice(&r[12..out_len]);
-				}
 			}
 		}
 	}
 }
 
 impl Impl for Sha256 {
-	fn execute(&self, input: &[u8], output: &mut [u8]) {
+	fn execute(&self, input: &[u8], output: &mut BytesRef) {
-		let out_len = min(output.len(), 32);
-
 		let mut sha = Sha256Digest::new();
 		sha.input(input);
 
-		if out_len == 32 {
-			sha.result(&mut output[0..32]);
-		} else {
 		let mut out = [0; 32];
 		sha.result(&mut out);
 
-			output.copy_from_slice(&out[..out_len])
+		output.write(0, &out);
-		}
 	}
 }
 
 impl Impl for Ripemd160 {
-	fn execute(&self, input: &[u8], output: &mut [u8]) {
+	fn execute(&self, input: &[u8], output: &mut BytesRef) {
-		let out_len = min(output.len(), 32);
-
 		let mut sha = Ripemd160Digest::new();
 		sha.input(input);
 
-		for x in &mut output[0.. min(12, out_len)] {
+		let mut out = [0; 32];
-			*x = 0;
+		sha.result(&mut out[12..32]);
-		}
 
-		if out_len >= 32 {
+		output.write(0, &out);
-			sha.result(&mut output[12..32]);
-		} else if out_len > 12 {
-			let mut out = [0; 20];
-			sha.result(&mut out);
-
-			output.copy_from_slice(&out[12..out_len])
-		}
 	}
 }
 
@@ -194,7 +170,7 @@ impl Impl for Ripemd160 {
 mod tests {
 	use super::{Builtin, Linear, ethereum_builtin, Pricer};
 	use ethjson;
-	use util::U256;
+	use util::{U256, BytesRef};
 
 	#[test]
 	fn identity() {
@@ -203,15 +179,15 @@ mod tests {
 		let i = [0u8, 1, 2, 3];
 
 		let mut o2 = [255u8; 2];
-		f.execute(&i[..], &mut o2[..]);
+		f.execute(&i[..], &mut BytesRef::Fixed(&mut o2[..]));
 		assert_eq!(i[0..2], o2);
 
 		let mut o4 = [255u8; 4];
-		f.execute(&i[..], &mut o4[..]);
+		f.execute(&i[..], &mut BytesRef::Fixed(&mut o4[..]));
 		assert_eq!(i, o4);
 
 		let mut o8 = [255u8; 8];
-		f.execute(&i[..], &mut o8[..]);
+		f.execute(&i[..], &mut BytesRef::Fixed(&mut o8[..]));
 		assert_eq!(i, o8[..4]);
 		assert_eq!([255u8; 4], o8[4..]);
 	}
@@ -224,16 +200,20 @@ mod tests {
 		let i = [0u8; 0];
 
 		let mut o = [255u8; 32];
-		f.execute(&i[..], &mut o[..]);
+		f.execute(&i[..], &mut BytesRef::Fixed(&mut o[..]));
 		assert_eq!(&o[..], &(FromHex::from_hex("e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855").unwrap())[..]);
 
 		let mut o8 = [255u8; 8];
-		f.execute(&i[..], &mut o8[..]);
+		f.execute(&i[..], &mut BytesRef::Fixed(&mut o8[..]));
 		assert_eq!(&o8[..], &(FromHex::from_hex("e3b0c44298fc1c14").unwrap())[..]);
 
 		let mut o34 = [255u8; 34];
-		f.execute(&i[..], &mut o34[..]);
+		f.execute(&i[..], &mut BytesRef::Fixed(&mut o34[..]));
 		assert_eq!(&o34[..], &(FromHex::from_hex("e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855ffff").unwrap())[..]);
+
+		let mut ov = vec![];
+		f.execute(&i[..], &mut BytesRef::Flexible(&mut ov));
+		assert_eq!(&ov[..], &(FromHex::from_hex("e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855").unwrap())[..]);
 	}
 
 	#[test]
@@ -244,15 +224,15 @@ mod tests {
 		let i = [0u8; 0];
 
 		let mut o = [255u8; 32];
-		f.execute(&i[..], &mut o[..]);
+		f.execute(&i[..], &mut BytesRef::Fixed(&mut o[..]));
 		assert_eq!(&o[..], &(FromHex::from_hex("0000000000000000000000009c1185a5c5e9fc54612808977ee8f548b2258d31").unwrap())[..]);
 
 		let mut o8 = [255u8; 8];
-		f.execute(&i[..], &mut o8[..]);
+		f.execute(&i[..], &mut BytesRef::Fixed(&mut o8[..]));
 		assert_eq!(&o8[..], &(FromHex::from_hex("0000000000000000").unwrap())[..]);
 
 		let mut o34 = [255u8; 34];
-		f.execute(&i[..], &mut o34[..]);
+		f.execute(&i[..], &mut BytesRef::Fixed(&mut o34[..]));
 		assert_eq!(&o34[..], &(FromHex::from_hex("0000000000000000000000009c1185a5c5e9fc54612808977ee8f548b2258d31ffff").unwrap())[..]);
 	}
 
@@ -272,46 +252,46 @@ mod tests {
 		let i = FromHex::from_hex("47173285a8d7341e5e972fc677286384f802f8ef42a5ec5f03bbfa254cb01fad000000000000000000000000000000000000000000000000000000000000001b650acf9d3f5f0a2c799776a1254355d5f4061762a237396a99a0e0e3fc2bcd6729514a0dacb2e623ac4abd157cb18163ff942280db4d5caad66ddf941ba12e03").unwrap();
 
 		let mut o = [255u8; 32];
-		f.execute(&i[..], &mut o[..]);
+		f.execute(&i[..], &mut BytesRef::Fixed(&mut o[..]));
 		assert_eq!(&o[..], &(FromHex::from_hex("000000000000000000000000c08b5542d177ac6686946920409741463a15dddb").unwrap())[..]);
 
 		let mut o8 = [255u8; 8];
-		f.execute(&i[..], &mut o8[..]);
+		f.execute(&i[..], &mut BytesRef::Fixed(&mut o8[..]));
 		assert_eq!(&o8[..], &(FromHex::from_hex("0000000000000000").unwrap())[..]);
 
 		let mut o34 = [255u8; 34];
-		f.execute(&i[..], &mut o34[..]);
+		f.execute(&i[..], &mut BytesRef::Fixed(&mut o34[..]));
 		assert_eq!(&o34[..], &(FromHex::from_hex("000000000000000000000000c08b5542d177ac6686946920409741463a15dddbffff").unwrap())[..]);
 
 		let i_bad = FromHex::from_hex("47173285a8d7341e5e972fc677286384f802f8ef42a5ec5f03bbfa254cb01fad000000000000000000000000000000000000000000000000000000000000001a650acf9d3f5f0a2c799776a1254355d5f4061762a237396a99a0e0e3fc2bcd6729514a0dacb2e623ac4abd157cb18163ff942280db4d5caad66ddf941ba12e03").unwrap();
 		let mut o = [255u8; 32];
-		f.execute(&i_bad[..], &mut o[..]);
+		f.execute(&i_bad[..], &mut BytesRef::Fixed(&mut o[..]));
 		assert_eq!(&o[..], &(FromHex::from_hex("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap())[..]);
 
 		let i_bad = FromHex::from_hex("47173285a8d7341e5e972fc677286384f802f8ef42a5ec5f03bbfa254cb01fad000000000000000000000000000000000000000000000000000000000000001b000000000000000000000000000000000000000000000000000000000000001b0000000000000000000000000000000000000000000000000000000000000000").unwrap();
 		let mut o = [255u8; 32];
-		f.execute(&i_bad[..], &mut o[..]);
+		f.execute(&i_bad[..], &mut BytesRef::Fixed(&mut o[..]));
 		assert_eq!(&o[..], &(FromHex::from_hex("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap())[..]);
 
 		let i_bad = FromHex::from_hex("47173285a8d7341e5e972fc677286384f802f8ef42a5ec5f03bbfa254cb01fad000000000000000000000000000000000000000000000000000000000000001b0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001b").unwrap();
 		let mut o = [255u8; 32];
-		f.execute(&i_bad[..], &mut o[..]);
+		f.execute(&i_bad[..], &mut BytesRef::Fixed(&mut o[..]));
 		assert_eq!(&o[..], &(FromHex::from_hex("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap())[..]);
 
 		let i_bad = FromHex::from_hex("47173285a8d7341e5e972fc677286384f802f8ef42a5ec5f03bbfa254cb01fad000000000000000000000000000000000000000000000000000000000000001bffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff000000000000000000000000000000000000000000000000000000000000001b").unwrap();
 		let mut o = [255u8; 32];
-		f.execute(&i_bad[..], &mut o[..]);
+		f.execute(&i_bad[..], &mut BytesRef::Fixed(&mut o[..]));
 		assert_eq!(&o[..], &(FromHex::from_hex("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap())[..]);
 
 		let i_bad = FromHex::from_hex("47173285a8d7341e5e972fc677286384f802f8ef42a5ec5f03bbfa254cb01fad000000000000000000000000000000000000000000000000000000000000001b000000000000000000000000000000000000000000000000000000000000001bffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap();
 		let mut o = [255u8; 32];
-		f.execute(&i_bad[..], &mut o[..]);
+		f.execute(&i_bad[..], &mut BytesRef::Fixed(&mut o[..]));
 		assert_eq!(&o[..], &(FromHex::from_hex("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap())[..]);
 
 		// TODO: Should this (corrupted version of the above) fail rather than returning some address?
 	/*	let i_bad = FromHex::from_hex("48173285a8d7341e5e972fc677286384f802f8ef42a5ec5f03bbfa254cb01fad000000000000000000000000000000000000000000000000000000000000001b650acf9d3f5f0a2c799776a1254355d5f4061762a237396a99a0e0e3fc2bcd6729514a0dacb2e623ac4abd157cb18163ff942280db4d5caad66ddf941ba12e03").unwrap();
 		let mut o = [255u8; 32];
-		f.execute(&i_bad[..], &mut o[..]);
+		f.execute(&i_bad[..], &mut BytesRef::Fixed(&mut o[..]));
 		assert_eq!(&o[..], &(FromHex::from_hex("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap())[..]);*/
 	}
 
@@ -336,7 +316,7 @@ mod tests {
 
 		let i = [0u8, 1, 2, 3];
 		let mut o = [255u8; 4];
-		b.execute(&i[..], &mut o[..]);
+		b.execute(&i[..], &mut BytesRef::Fixed(&mut o[..]));
 		assert_eq!(i, o);
 	}
 
@@ -357,7 +337,7 @@ mod tests {
 
 		let i = [0u8, 1, 2, 3];
 		let mut o = [255u8; 4];
-		b.execute(&i[..], &mut o[..]);
+		b.execute(&i[..], &mut BytesRef::Fixed(&mut o[..]));
 		assert_eq!(i, o);
 	}
 }

ethcore/src/engines/mod.rs

@@ -126,7 +126,7 @@ pub trait Engine : Sync + Send {
 	fn cost_of_builtin(&self, a: &Address, input: &[u8]) -> U256 { self.builtins().get(a).unwrap().cost(input.len()) }
 	/// Execution the builtin contract `a` on `input` and return `output`.
 	/// Panics if `is_builtin(a)` is not true.
-	fn execute_builtin(&self, a: &Address, input: &[u8], output: &mut [u8]) { self.builtins().get(a).unwrap().execute(input, output); }
+	fn execute_builtin(&self, a: &Address, input: &[u8], output: &mut BytesRef) { self.builtins().get(a).unwrap().execute(input, output); }
 
 	// TODO: sealing stuff - though might want to leave this for later.
 }

ethcore/src/executive.rs

@@ -193,7 +193,6 @@ impl<'a> Executive<'a> {
 					data: Some(t.data.clone()),
 					call_type: CallType::Call,
 				};
-				// TODO: move output upstream
 				let mut out = vec![];
 				(self.call(params, &mut substate, BytesRef::Flexible(&mut out), &mut tracer, &mut vm_tracer), out)
 			}

rpc/src/v1/types/bytes.rs

@@ -70,7 +70,7 @@ impl Visitor for BytesVisitor {
 	type Value = Bytes;
 
 	fn visit_str<E>(&mut self, value: &str) -> Result<Self::Value, E> where E: Error {
-		if value.len() >= 2 && &value[0..2] == "0x" {
+		if value.len() >= 2 && &value[0..2] == "0x" && value.len() & 1 == 0 {
 			Ok(Bytes::new(FromHex::from_hex(&value[2..]).unwrap_or_else(|_| vec![])))
 		} else {
 			Err(Error::custom("invalid hex"))
@@ -95,5 +95,21 @@ mod tests {
 		let serialized = serde_json::to_string(&bytes).unwrap();
 		assert_eq!(serialized, r#""0x0123456789abcdef""#);
 	}
+
+	#[test]
+	fn test_bytes_deserialize() {
+		let bytes1: Result<Bytes, serde_json::Error> = serde_json::from_str(r#""""#);
+		let bytes2: Result<Bytes, serde_json::Error> = serde_json::from_str(r#""0x123""#);
+
+		let bytes3: Bytes = serde_json::from_str(r#""0x""#).unwrap();
+		let bytes4: Bytes = serde_json::from_str(r#""0x12""#).unwrap();
+		let bytes5: Bytes = serde_json::from_str(r#""0x0123""#).unwrap();
+
+		assert!(bytes1.is_err());
+		assert!(bytes2.is_err());
+		assert_eq!(bytes3, Bytes(vec![]));
+		assert_eq!(bytes4, Bytes(vec![0x12]));
+		assert_eq!(bytes5, Bytes(vec![0x1, 0x23]));
+	}
 }
 

util/src/bytes.rs

@@ -20,6 +20,7 @@
 //! as
 
 use std::fmt;
+use std::cmp::min;
 use std::ops::{Deref, DerefMut};
 
 /// Slice pretty print helper
@@ -71,6 +72,32 @@ pub enum BytesRef<'a> {
 	Fixed(&'a mut [u8])
 }
 
+impl<'a> BytesRef<'a> {
+	/// Writes given `input` to this `BytesRef` starting at `offset`.
+	/// Returns number of bytes written to the ref.
+	/// NOTE can return number greater then `input.len()` in case flexible vector had to be extended.
+	pub fn write(&mut self, offset: usize, input: &[u8]) -> usize {
+		match *self {
+			BytesRef::Flexible(ref mut data) => {
+				let data_len = data.len();
+				let wrote = input.len() + if data_len > offset { 0 } else { offset - data_len };
+
+				data.resize(offset, 0);
+				data.extend_from_slice(input);
+				wrote
+			},
+			BytesRef::Fixed(ref mut data) if offset < data.len() => {
+				let max = min(data.len() - offset, input.len());
+				for i in 0..max {
+					data[offset + i] = input[i];
+				}
+				max
+			},
+			_ => 0
+		}
+	}
+}
+
 impl<'a> Deref for BytesRef<'a> {
 	type Target = [u8];
 
@@ -93,3 +120,60 @@ impl <'a> DerefMut for BytesRef<'a> {
 
 /// Vector of bytes.
 pub type Bytes = Vec<u8>;
+
+#[cfg(test)]
+mod tests {
+	use super::BytesRef;
+
+	#[test]
+	fn should_write_bytes_to_fixed_bytesref() {
+		// given
+		let mut data1 = vec![0, 0, 0];
+		let mut data2 = vec![0, 0, 0];
+		let (res1, res2) = {
+			let mut bytes1 = BytesRef::Fixed(&mut data1[..]);
+			let mut bytes2 = BytesRef::Fixed(&mut data2[1..2]);
+
+			// when
+			let res1 = bytes1.write(1, &[1, 1, 1]);
+			let res2 = bytes2.write(3, &[1, 1, 1]);
+			(res1, res2)
+		};
+
+		// then
+		assert_eq!(&data1, &[0, 1, 1]);
+		assert_eq!(res1, 2);
+
+		assert_eq!(&data2, &[0, 0, 0]);
+		assert_eq!(res2, 0);
+	}
+
+	#[test]
+	fn should_write_bytes_to_flexible_bytesref() {
+		// given
+		let mut data1 = vec![0, 0, 0];
+		let mut data2 = vec![0, 0, 0];
+		let mut data3 = vec![0, 0, 0];
+		let (res1, res2, res3) = {
+			let mut bytes1 = BytesRef::Flexible(&mut data1);
+			let mut bytes2 = BytesRef::Flexible(&mut data2);
+			let mut bytes3 = BytesRef::Flexible(&mut data3);
+
+			// when
+			let res1 = bytes1.write(1, &[1, 1, 1]);
+			let res2 = bytes2.write(3, &[1, 1, 1]);
+			let res3 = bytes3.write(5, &[1, 1, 1]);
+			(res1, res2, res3)
+		};
+
+		// then
+		assert_eq!(&data1, &[0, 1, 1, 1]);
+		assert_eq!(res1, 3);
+
+		assert_eq!(&data2, &[0, 0, 0, 1, 1, 1]);
+		assert_eq!(res2, 3);
+
+		assert_eq!(&data3, &[0, 0, 0, 0, 0, 1, 1, 1]);
+		assert_eq!(res3, 5);
+	}
+}