Update Simple Subroutines to the latest spec (#11731)

Author: adria0

.gitmodules

@@ -1,7 +1,7 @@
 [submodule "ethcore/res/ethereum/tests"]
 	path = ethcore/res/ethereum/tests
 	url = https://github.com/ethereum/tests.git
-	branch = v7.0.0
+	branch = develop
 [submodule "ethcore/res/wasm-tests"]
 	path = ethcore/res/wasm-tests
 	url = https://github.com/openethereum/wasm-tests

ethcore/evm/src/instructions.rs

@@ -321,11 +321,11 @@ enum_with_from_u8! {
 		LOG4 = 0xa4,
 
 		#[doc = "Marks the entry point to a subroutine."]
-		BEGINSUB = 0xb2,
-		#[doc = "Jumps to a defined BEGINSUB subroutine."]
-		JUMPSUB = 0xb3,
+		BEGINSUB = 0x5c,
 		#[doc = "Returns from a subroutine."]
-		RETURNSUB = 0xb7,
+		RETURNSUB = 0x5d,
+		#[doc = "Jumps to a defined BEGINSUB subroutine."]
+		JUMPSUB = 0x5e,
 
 		#[doc = "create a new account with associated code"]
 		CREATE = 0xf0,
@@ -593,8 +593,8 @@ lazy_static! {
 		arr[LOG3 as usize] = Some(InstructionInfo::new("LOG3", 5, 0, GasPriceTier::Special));
 		arr[LOG4 as usize] = Some(InstructionInfo::new("LOG4", 6, 0, GasPriceTier::Special));
 		arr[BEGINSUB as usize] = Some(InstructionInfo::new("BEGINSUB", 0, 0, GasPriceTier::Base));
-		arr[JUMPSUB as usize] = Some(InstructionInfo::new("JUMPSUB", 1, 0, GasPriceTier::Low));
-		arr[RETURNSUB as usize] = Some(InstructionInfo::new("RETURNSUB", 0, 0, GasPriceTier::VeryLow));
+		arr[JUMPSUB as usize] = Some(InstructionInfo::new("JUMPSUB", 1, 0, GasPriceTier::High));
+		arr[RETURNSUB as usize] = Some(InstructionInfo::new("RETURNSUB", 0, 0, GasPriceTier::Low));
 		arr[CREATE as usize] = Some(InstructionInfo::new("CREATE", 3, 1, GasPriceTier::Special));
 		arr[CALL as usize] = Some(InstructionInfo::new("CALL", 7, 1, GasPriceTier::Special));
 		arr[CALLCODE as usize] = Some(InstructionInfo::new("CALLCODE", 7, 1, GasPriceTier::Special));

ethcore/evm/src/interpreter/mod.rs

@@ -64,7 +64,7 @@ const TWO_POW_248: U256 = U256([0, 0, 0, 0x100000000000000]); //0x1 00000000 000
 
 /// Maximal subroutine stack size as specified in
 /// https://eips.ethereum.org/EIPS/eip-2315.
-pub const MAX_SUB_STACK_SIZE : usize = 1024;
+pub const MAX_SUB_STACK_SIZE : usize = 1023;
 
 /// Abstraction over raw vector of Bytes. Easier state management of PC.
 struct CodeReader {
@@ -432,7 +432,7 @@ impl<Cost: CostType> Interpreter<Cost> {
 					Err(e) => return InterpreterResult::Done(Err(e))
 				};
 				self.return_stack.push(self.reader.position);
-				self.reader.position = pos;
+				self.reader.position = pos + 1;
 			},	
 			InstructionResult::ReturnFromSubroutine(pos) => {
 				self.reader.position = pos;
@@ -553,7 +553,7 @@ impl<Cost: CostType> Interpreter<Cost> {
 				// ignore
 			},
 			instructions::BEGINSUB => {
-				// ignore
+				return Err(vm::Error::InvalidSubEntry);
 			},
 			instructions::JUMPSUB => {
 				if self.return_stack.len() >= MAX_SUB_STACK_SIZE {

ethcore/evm/src/interpreter/shared_cache.rs

@@ -160,7 +160,7 @@ fn test_find_sub_entrypoints() {
 	// given
 
 	// see https://eips.ethereum.org/EIPS/eip-2315 for disassembly
-	let code = hex!("6800000000000000000cb300b26011b3b7b2b7");
+	let code = hex!("6800000000000000000c5e005c60115e5d5c5d");
 
 	// when
 	let cache_item = SharedCache::find_jump_and_sub_destinations(&code);
@@ -180,7 +180,7 @@ fn test_find_jump_and_sub_allowing_unknown_opcodes() {
 	// 0000 5B   JUMPDEST       
 	// 0001 CC   ???
 	// 0002 B2   BEGINSUB
-	let code = hex!("5BCCB2");
+	let code = hex!("5BCC5C");
 
 	// when
 	let cache_item = SharedCache::find_jump_and_sub_destinations(&code);

ethcore/evm/src/tests.rs

@@ -790,10 +790,10 @@ fn test_jumps(factory: super::Factory) {
 evm_test!{test_subs_simple: test_subs_simple_int}
 fn test_subs_simple(factory: super::Factory) {
 	// as defined in https://eips.ethereum.org/EIPS/eip-2315
-	let code = hex!("6004b300b2b7").to_vec();
+	let code = hex!("60045e005c5d").to_vec();
 
 	let mut params = ActionParams::default();
-	params.gas = U256::from(13);
+	params.gas = U256::from(18);
 	params.code = Some(Arc::new(code));
 	let mut ext = FakeExt::new_berlin();
 
@@ -808,10 +808,10 @@ fn test_subs_simple(factory: super::Factory) {
 evm_test!{test_subs_two_levels: test_subs_two_levels_int}
 fn test_subs_two_levels(factory: super::Factory) {
 	// as defined in https://eips.ethereum.org/EIPS/eip-2315
-	let code = hex!("6800000000000000000cb300b26011b3b7b2b7").to_vec();
+	let code = hex!("6800000000000000000c5e005c60115e5d5c5d").to_vec();
 
 	let mut params = ActionParams::default();
-	params.gas = U256::from(26);
+	params.gas = U256::from(36);
 	params.code = Some(Arc::new(code));
 	let mut ext = FakeExt::new_berlin();
 
@@ -826,7 +826,7 @@ fn test_subs_two_levels(factory: super::Factory) {
 evm_test!{test_subs_invalid_jump: test_subs_invalid_jump_int}
 fn test_subs_invalid_jump(factory: super::Factory) {
 	// as defined in https://eips.ethereum.org/EIPS/eip-2315
-	let code = hex!("6801000000000000000cb300b26011b3b7b2b7").to_vec();
+	let code = hex!("6801000000000000000c5e005c60115e5d5c5d").to_vec();
 
 	let mut params = ActionParams::default();
 	params.gas = U256::from(24);
@@ -845,7 +845,7 @@ fn test_subs_invalid_jump(factory: super::Factory) {
 evm_test!{test_subs_shallow_return_stack: test_subs_shallow_return_stack_int}
 fn test_subs_shallow_return_stack(factory: super::Factory) {
 	// as defined in https://eips.ethereum.org/EIPS/eip-2315
-	let code = hex!("b75858").to_vec();
+	let code = hex!("5d5858").to_vec();
 
 	let mut params = ActionParams::default();
 	params.gas = U256::from(24);
@@ -865,7 +865,9 @@ evm_test!{test_subs_substack_limit: test_subs_substack_limit_int}
 fn test_subs_substack_limit(factory: super::Factory) {
 
 	//    PUSH <recursion_limit>
+	//    JUMP :a
 	// :s BEGINSUB
+    // :a JUMPDEST
 	//    DUP1
 	//    JUMPI :c
 	//    STOP
@@ -875,7 +877,7 @@ fn test_subs_substack_limit(factory: super::Factory) {
 	//    SUB
 	//    JUMPSUB :s
 
-	let mut code = hex!("610400b280600957005b600190036003b3").to_vec();
+	let mut code = hex!("6104006007565c5b80600d57005b6001900360065e").to_vec();
 	code[1..3].copy_from_slice(&(MAX_SUB_STACK_SIZE as u16).to_be_bytes()[..]);
 
 	let mut params = ActionParams::default();
@@ -888,12 +890,12 @@ fn test_subs_substack_limit(factory: super::Factory) {
 		test_finalize(vm.exec(&mut ext).ok().unwrap()).unwrap()
 	};
 
-	assert_eq!(gas_left, U256::from(963_115));
+	assert_eq!(gas_left, U256::from(959_049));
 }
 
 evm_test!{test_subs_substack_out: test_subs_substack_out_int}
 fn test_subs_substack_out(factory: super::Factory) {
-	let mut code = hex!("610400b280600957005b600190036003b3").to_vec();
+	let mut code = hex!("6104006007565c5b80600d57005b6001900360065e").to_vec();
 	code[1..3].copy_from_slice(&((MAX_SUB_STACK_SIZE+1) as u16).to_be_bytes()[..]);
 
 	let mut params = ActionParams::default();
@@ -912,10 +914,10 @@ fn test_subs_substack_out(factory: super::Factory) {
 
 evm_test!{test_subs_sub_at_end: test_subs_sub_at_end_int}
 fn test_subs_sub_at_end(factory: super::Factory) {
-	let code = hex!("600556b2b75b6003b3").to_vec();
+	let code = hex!("6005565c5d5b60035e").to_vec();
 
 	let mut params = ActionParams::default();
-	params.gas = U256::from(25);
+	params.gas = U256::from(30);
 	params.code = Some(Arc::new(code));
 	let mut ext = FakeExt::new_berlin();
 
@@ -927,6 +929,24 @@ fn test_subs_sub_at_end(factory: super::Factory) {
 	assert_eq!(gas_left, U256::from(0));
 }
 
+evm_test!{test_subs_walk_into_subroutine: test_subs_walk_into_subroutine_int}
+fn test_subs_walk_into_subroutine(factory: super::Factory) {
+	let code = hex!("5c5d00").to_vec();
+
+	let mut params = ActionParams::default();
+	params.gas = U256::from(100);
+	params.code = Some(Arc::new(code));
+	let mut ext = FakeExt::new_berlin();
+
+	let current = {
+		let vm = factory.create(params, ext.schedule(), ext.depth());
+		test_finalize(vm.exec(&mut ext).ok().unwrap())
+	};
+
+	let expected = Result::Err(vm::Error::InvalidSubEntry);
+	assert_eq!(current, expected);
+}
+
 evm_test!{test_calls: test_calls_int}
 fn test_calls(factory: super::Factory) {
 	let code = hex!("600054602d57600160005560006000600060006050610998610100f160006000600060006050610998610100f25b").to_vec();

ethcore/machine/src/executive.rs

@@ -359,6 +359,7 @@ impl<'a> CallCreateExecutive<'a> {
 				| Err(vm::Error::Reverted)
 				| Err(vm::Error::SubStackUnderflow {..})
 				| Err(vm::Error::OutOfSubStack {..})
+				| Err(vm::Error::InvalidSubEntry)
 				| Ok(FinalizationResult { apply_state: false, .. }) => {
 					if let Some(addr) = UNPRUNABLE_PRECOMPILE_ADDRESS {
 						if un_substate.touched.contains(&addr) {

ethcore/res/ethereum/tests

@@ -304,6 +304,7 @@ mod block_tests {
 	declare_test!{BlockchainTests_GeneralStateTest_stSStoreTest, "BlockchainTests/GeneralStateTests/stSStoreTest/"}
 	declare_test!{BlockchainTests_GeneralStateTest_stStackTests, "BlockchainTests/GeneralStateTests/stStackTests/"}
 	declare_test!{BlockchainTests_GeneralStateTest_stStaticCall, "BlockchainTests/GeneralStateTests/stStaticCall/"}
+	declare_test!{BlockchainTests_GeneralStateTest_stSubroutine, "BlockchainTests/GeneralStateTests/stSubroutine/"}
 	declare_test!{BlockchainTests_GeneralStateTest_stSystemOperationsTest, "BlockchainTests/GeneralStateTests/stSystemOperationsTest/"}
 	declare_test!{BlockchainTests_GeneralStateTest_stTimeConsuming, "BlockchainTests/GeneralStateTests/stTimeConsuming/"}
 	declare_test!{BlockchainTests_GeneralStateTest_stTransactionTest, "BlockchainTests/GeneralStateTests/stTransactionTest/"}

ethcore/src/json_tests/chain.rs

@@ -175,6 +175,7 @@ mod state_tests {
 	declare_test!{GeneralStateTest_stSStoreTest, "GeneralStateTests/stSStoreTest/"}
 	declare_test!{GeneralStateTest_stStackTests, "GeneralStateTests/stStackTests/"}
 	declare_test!{GeneralStateTest_stStaticCall, "GeneralStateTests/stStaticCall/"}
+	declare_test!{GeneralStateTest_stSubroutine, "GeneralStateTests/stSubroutine/"}
 	declare_test!{GeneralStateTest_stSystemOperationsTest, "GeneralStateTests/stSystemOperationsTest/"}
 	declare_test!{GeneralStateTest_stTimeConsuming, "GeneralStateTests/stTimeConsuming/"}
 	declare_test!{GeneralStateTest_stTransactionTest, "GeneralStateTests/stTransactionTest/"}

ethcore/src/json_tests/state.rs

@@ -38,6 +38,8 @@ pub enum Error {
 	SubStackUnderflow,
 	/// When execution would exceed defined subroutine Stack Limit
 	OutOfSubStack,
+	/// When the code walks into a subroutine, that is not allowed
+	InvalidSubEntry,
 	/// When builtin contract failed on input data
 	BuiltIn,
 	/// Returned on evm internal error. Should never be ignored during development.
@@ -61,6 +63,7 @@ impl<'a> From<&'a VmError> for Error {
 			VmError::BadInstruction { .. } => Error::BadInstruction,
 			VmError::StackUnderflow { .. } => Error::StackUnderflow,
 			VmError::OutOfStack { .. } => Error::OutOfStack,
+			VmError::InvalidSubEntry { .. } => Error::InvalidSubEntry,
 			VmError::BuiltIn { .. } => Error::BuiltIn,
 			VmError::Wasm { .. } => Error::Wasm,
 			VmError::Internal(_) => Error::Internal,
@@ -88,6 +91,7 @@ impl fmt::Display for Error {
 			BadInstruction => "Bad instruction",
 			StackUnderflow => "Stack underflow",
 			OutOfStack => "Out of stack",
+			InvalidSubEntry => "Invalid subroutine entry",
 			BuiltIn => "Built-in failed",
 			Wasm => "Wasm runtime error",
 			Internal => "Internal error",
@@ -118,6 +122,7 @@ impl Encodable for Error {
 			Reverted => 10,
 			SubStackUnderflow => 11,
 			OutOfSubStack => 12,
+			InvalidSubEntry => 13,
 		};
 
 		s.append_internal(&value);
@@ -142,6 +147,7 @@ impl Decodable for Error {
 			10 => Ok(Reverted),
 			11 => Ok(SubStackUnderflow),
 			12 => Ok(OutOfSubStack),
+			13 => Ok(InvalidSubEntry),
 			_ => Err(DecoderError::Custom("Invalid error type")),
 		}
 	}