Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EIP2315] Add tests for subroutines #685

Merged
merged 7 commits into from
May 24, 2020

Conversation

matkt
Copy link
Contributor

@matkt matkt commented Apr 27, 2020

Addition of tests for the https://eips.ethereum.org/EIPS/eip-2315. Tests generated with the current Besu implementation of the EIP2315.

The tests are those present in the description of the EIP.

Need update of retesteth ethereum/retesteth#90

@matkt matkt marked this pull request as ready for review April 27, 2020 15:40
@holiman
Copy link
Contributor

holiman commented Apr 28, 2020

Awesome, well done!
I will test the tests and report back. One more thing:

	// The code recursively calls itself. It should error when the returns-stack
	// grows above 1023
	state.SetCode(address, []byte{
		byte(vm.BEGINSUB),
		byte(vm.PUSH1), 0,
		byte(vm.JUMPSUB),
	})

This case is a bit problematic to test -- because since it exits with error, it will consume all gas regardless of whether it exits at step 3072 or 3075 or 3069. So in order to actually produce a difference in the poststate, we'd have to store a counter at e.g. slot zero, and increment every loop.

@holiman
Copy link
Contributor

holiman commented Apr 28, 2020

I can verify that the state-tests pass seems to be correct, at least according to the geth-implementation:

[user@work evm]$ bash testsubroutines.sh 
Testing /home/user/workspace/tests/GeneralStateTests/stSubroutine/simpleSubroutine.json
{"pc":0,"op":96,"gas":"0x5c878","gasCost":"0x3","memory":"0x","memSize":0,"stack":[],"returnStack":[],"depth":1,"refund":0,"opName":"PUSH1","error":""}
{"pc":2,"op":179,"gas":"0x5c875","gasCost":"0x8","memory":"0x","memSize":0,"stack":["0x4"],"returnStack":[],"depth":1,"refund":0,"opName":"JUMPSUB","error":""}
{"pc":4,"op":178,"gas":"0x5c86d","gasCost":"0x1","memory":"0x","memSize":0,"stack":[],"returnStack":["0x2"],"depth":1,"refund":0,"opName":"BEGINSUB","error":""}
{"pc":5,"op":183,"gas":"0x5c86c","gasCost":"0x2","memory":"0x","memSize":0,"stack":[],"returnStack":["0x2"],"depth":1,"refund":0,"opName":"RETURNSUB","error":""}
{"pc":3,"op":0,"gas":"0x5c86a","gasCost":"0x0","memory":"0x","memSize":0,"stack":[],"returnStack":[],"depth":1,"refund":0,"opName":"STOP","error":""}
{"output":"","gasUsed":"0xe","time":116215}
{"stateRoot": "8d2ab4d286b975b1695fd508b2a693a0c145b18ed3556af671d1c9854a68948f"}
[
  {
    "name": "simpleSubroutine",
    "pass": true,
    "fork": "Berlin"
  }
]
Testing /home/user/workspace/tests/GeneralStateTests/stSubroutine/subroutineAtEndOfCode.json
{"pc":0,"op":96,"gas":"0x5c878","gasCost":"0x3","memory":"0x","memSize":0,"stack":[],"returnStack":[],"depth":1,"refund":0,"opName":"PUSH1","error":""}
{"pc":2,"op":86,"gas":"0x5c875","gasCost":"0x8","memory":"0x","memSize":0,"stack":["0x5"],"returnStack":[],"depth":1,"refund":0,"opName":"JUMP","error":""}
{"pc":5,"op":91,"gas":"0x5c86d","gasCost":"0x1","memory":"0x","memSize":0,"stack":[],"returnStack":[],"depth":1,"refund":0,"opName":"JUMPDEST","error":""}
{"pc":6,"op":96,"gas":"0x5c86c","gasCost":"0x3","memory":"0x","memSize":0,"stack":[],"returnStack":[],"depth":1,"refund":0,"opName":"PUSH1","error":""}
{"pc":8,"op":179,"gas":"0x5c869","gasCost":"0x8","memory":"0x","memSize":0,"stack":["0x3"],"returnStack":[],"depth":1,"refund":0,"opName":"JUMPSUB","error":""}
{"pc":3,"op":178,"gas":"0x5c861","gasCost":"0x1","memory":"0x","memSize":0,"stack":[],"returnStack":["0x8"],"depth":1,"refund":0,"opName":"BEGINSUB","error":""}
{"pc":4,"op":183,"gas":"0x5c860","gasCost":"0x2","memory":"0x","memSize":0,"stack":[],"returnStack":["0x8"],"depth":1,"refund":0,"opName":"RETURNSUB","error":""}
{"pc":9,"op":0,"gas":"0x5c85e","gasCost":"0x0","memory":"0x","memSize":0,"stack":[],"returnStack":[],"depth":1,"refund":0,"opName":"STOP","error":""}
{"output":"","gasUsed":"0x1a","time":336982}
{"stateRoot": "805db559e0a7b054b323ca8f4b9f449d79a444bc1d5cac379bae9a242dde0b2a"}
[
  {
    "name": "subroutineAtEndOfCode",
    "pass": true,
    "fork": "Berlin"
  }
]
Testing /home/user/workspace/tests/GeneralStateTests/stSubroutine/subroutineInvalidJump.json
{"pc":0,"op":104,"gas":"0x5c878","gasCost":"0x3","memory":"0x","memSize":0,"stack":[],"returnStack":[],"depth":1,"refund":0,"opName":"PUSH9","error":""}
{"pc":10,"op":179,"gas":"0x5c875","gasCost":"0x8","memory":"0x","memSize":0,"stack":["0x1000000000000000c"],"returnStack":[],"depth":1,"refund":0,"opName":"JUMPSUB","error":""}
{"output":"","gasUsed":"0x5c878","time":1199045,"error":"evm: invalid jump destination"}
{"stateRoot": "b192e50b963cb534265da00cca90f272cd2c258b95d5c49edd37005307ade18f"}
[
  {
    "name": "subroutineInvalidJump",
    "pass": true,
    "fork": "Berlin"
  }
]
Testing /home/user/workspace/tests/GeneralStateTests/stSubroutine/subroutineShallowReturnStack.json
{"pc":0,"op":183,"gas":"0x5c878","gasCost":"0x2","memory":"0x","memSize":0,"stack":[],"returnStack":[],"depth":1,"refund":0,"opName":"RETURNSUB","error":""}
{"output":"","gasUsed":"0x5c878","time":92488,"error":"evm: invalid retsub"}
{"stateRoot": "19f388de55450373529c40d705d958602672f6dc33a0df093c17159942247de2"}
[
  {
    "name": "subroutineShallowReturnStack",
    "pass": true,
    "fork": "Berlin"
  }
]
Testing /home/user/workspace/tests/GeneralStateTests/stSubroutine/twoLevelsSubroutines.json
{"pc":0,"op":104,"gas":"0x5c878","gasCost":"0x3","memory":"0x","memSize":0,"stack":[],"returnStack":[],"depth":1,"refund":0,"opName":"PUSH9","error":""}
{"pc":10,"op":179,"gas":"0x5c875","gasCost":"0x8","memory":"0x","memSize":0,"stack":["0xc"],"returnStack":[],"depth":1,"refund":0,"opName":"JUMPSUB","error":""}
{"pc":12,"op":178,"gas":"0x5c86d","gasCost":"0x1","memory":"0x","memSize":0,"stack":[],"returnStack":["0xa"],"depth":1,"refund":0,"opName":"BEGINSUB","error":""}
{"pc":13,"op":96,"gas":"0x5c86c","gasCost":"0x3","memory":"0x","memSize":0,"stack":[],"returnStack":["0xa"],"depth":1,"refund":0,"opName":"PUSH1","error":""}
{"pc":15,"op":179,"gas":"0x5c869","gasCost":"0x8","memory":"0x","memSize":0,"stack":["0x11"],"returnStack":["0xa"],"depth":1,"refund":0,"opName":"JUMPSUB","error":""}
{"pc":17,"op":178,"gas":"0x5c861","gasCost":"0x1","memory":"0x","memSize":0,"stack":[],"returnStack":["0xa","0xf"],"depth":1,"refund":0,"opName":"BEGINSUB","error":""}
{"pc":18,"op":183,"gas":"0x5c860","gasCost":"0x2","memory":"0x","memSize":0,"stack":[],"returnStack":["0xa","0xf"],"depth":1,"refund":0,"opName":"RETURNSUB","error":""}
{"pc":16,"op":183,"gas":"0x5c85e","gasCost":"0x2","memory":"0x","memSize":0,"stack":[],"returnStack":["0xa"],"depth":1,"refund":0,"opName":"RETURNSUB","error":""}
{"pc":11,"op":0,"gas":"0x5c85c","gasCost":"0x0","memory":"0x","memSize":0,"stack":[],"returnStack":[],"depth":1,"refund":0,"opName":"STOP","error":""}
{"output":"","gasUsed":"0x1c","time":147835}
{"stateRoot": "0e2de1dc4001ee621577748b389f923822b0a9bb0208dc3f77164dc6b92c080a"}
[
  {
    "name": "twoLevelsSubroutines",
    "pass": true,
    "fork": "Berlin"
  }
]

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but let's wait a bit more and see if the EIP will change

@holiman
Copy link
Contributor

holiman commented Apr 28, 2020

Worth noting, though, is that two of the tests suffer from the same problem that I described regarding that one that exits with error.
If I run the tests without Eip-2315 enabled, two of them still pass - but for the wrong reason:

Testing /home/user/workspace/tests/GeneralStateTests/stSubroutine/subroutineInvalidJump.json
{"pc":0,"op":104,"gas":"0x5c878","gasCost":"0x3","memory":"0x","memSize":0,"stack":[],"depth":1,"refund":0,"opName":"PUSH9","error":""}
{"pc":10,"op":179,"gas":"0x5c875","gasCost":"0x3","memory":"0x","memSize":0,"stack":["0x1000000000000000c"],"depth":1,"refund":0,"opName":"opcode 0xb3 not defined","error":"invalid opcode: opcode 0xb3 not defined"}
{"output":"","gasUsed":"0x5c878","time":121676,"error":"invalid opcode: opcode 0xb3 not defined"}
{"stateRoot": "b192e50b963cb534265da00cca90f272cd2c258b95d5c49edd37005307ade18f"}
[
  {
    "name": "subroutineInvalidJump",
    "pass": true,
    "fork": "Berlin"
  }
]
Testing /home/user/workspace/tests/GeneralStateTests/stSubroutine/subroutineShallowReturnStack.json
{"pc":0,"op":183,"gas":"0x5c878","gasCost":"0x0","memory":"0x","memSize":0,"stack":[],"depth":1,"refund":0,"opName":"opcode 0xb7 not defined","error":"invalid opcode: opcode 0xb7 not defined"}
{"output":"","gasUsed":"0x5c878","time":143542,"error":"invalid opcode: opcode 0xb7 not defined"}
{"stateRoot": "19f388de55450373529c40d705d958602672f6dc33a0df093c17159942247de2"}
[
  {
    "name": "subroutineShallowReturnStack",
    "pass": true,
    "fork": "Berlin"
  }
]

I don't have any splendid ideas immediately on how to improve the accuracy of those tests

@matkt
Copy link
Contributor Author

matkt commented Apr 28, 2020

  • There's one more test that I didn't write to the EIP, because the trace didn't fit into markdown

I tried your bytecode which allows you to reach the limit of the rstack. But when we add a SSTORE to save a value, the storage in the postState remains empty in case of error.

A simple test without counter

  • 0x6001600055b26005b3 (store a simple value before the recursive call) : '0x00' does not exist
  • 0x6001600055 (store a simple value without the recursive call) : '0x00' exist

@holiman
Copy link
Contributor

holiman commented Apr 28, 2020

But when we add a SSTORE to save a value, the storage in the postState remains empty in case of error.

Ah right (doh!)
I guess we could do one test that loops + stores up to "one below the limit", and check that we can get that far. Then another test where we try to loop one more time, and expect that one to fail.

@holiman
Copy link
Contributor

holiman commented Apr 28, 2020

Here's an example that does it five times:

The code recursively calls into a subroutine, N times. It should succeeed if N is below the threshold for rstack size
Bytecode: 0x610005600055b260016000540380156018576000556006b35b
From: 0x0000000000000000000000000000000000000000
To: 0x000000000000000000000000636F6E7472616374
Data: 0x
Gas: 50000
Value 0 wei

Pc Op Cost Stack RStack
0 PUSH2 3 [] []
3 PUSH1 3 [5] []
5 SSTORE 20000 [5,0] []
6 BEGINSUB 1 [] []
7 PUSH1 3 [] []
9 PUSH1 3 [1] []
11 SLOAD 200 [1,0] []
12 SUB 3 [1,5] []
13 DUP1 3 [4] []
14 ISZERO 3 [4,4] []
15 PUSH1 3 [4,0] []
17 JUMPI 10 [4,0,24] []
18 PUSH1 3 [4] []
20 SSTORE 5000 [4,0] []
21 PUSH1 3 [] []
23 JUMPSUB 8 [6] []
6 BEGINSUB 1 [] [23]
7 PUSH1 3 [] [23]
9 PUSH1 3 [1] [23]
11 SLOAD 200 [1,0] [23]
12 SUB 3 [1,4] [23]
13 DUP1 3 [3] [23]
14 ISZERO 3 [3,3] [23]
15 PUSH1 3 [3,0] [23]
17 JUMPI 10 [3,0,24] [23]
18 PUSH1 3 [3] [23]
20 SSTORE 5000 [3,0] [23]
21 PUSH1 3 [] [23]
23 JUMPSUB 8 [6] [23]
6 BEGINSUB 1 [] [23,23]
7 PUSH1 3 [] [23,23]
9 PUSH1 3 [1] [23,23]
11 SLOAD 200 [1,0] [23,23]
12 SUB 3 [1,3] [23,23]
13 DUP1 3 [2] [23,23]
14 ISZERO 3 [2,2] [23,23]
15 PUSH1 3 [2,0] [23,23]
17 JUMPI 10 [2,0,24] [23,23]
18 PUSH1 3 [2] [23,23]
20 SSTORE 5000 [2,0] [23,23]
21 PUSH1 3 [] [23,23]
23 JUMPSUB 8 [6] [23,23]
6 BEGINSUB 1 [] [23,23,23]
7 PUSH1 3 [] [23,23,23]
9 PUSH1 3 [1] [23,23,23]
11 SLOAD 200 [1,0] [23,23,23]
12 SUB 3 [1,2] [23,23,23]
13 DUP1 3 [1] [23,23,23]
14 ISZERO 3 [1,1] [23,23,23]
15 PUSH1 3 [1,0] [23,23,23]
17 JUMPI 10 [1,0,24] [23,23,23]
18 PUSH1 3 [1] [23,23,23]
20 SSTORE 5000 [1,0] [23,23,23]
21 PUSH1 3 [] [23,23,23]
23 JUMPSUB 8 [6] [23,23,23]
6 BEGINSUB 1 [] [23,23,23,23]
7 PUSH1 3 [] [23,23,23,23]
9 PUSH1 3 [1] [23,23,23,23]
11 SLOAD 200 [1,0] [23,23,23,23]
12 SUB 3 [1,1] [23,23,23,23]
13 DUP1 3 [0] [23,23,23,23]
14 ISZERO 3 [0,0] [23,23,23,23]
15 PUSH1 3 [0,1] [23,23,23,23]
17 JUMPI 10 [0,1,24] [23,23,23,23]
24 JUMPDEST 1 [0] [23,23,23,23]
25 STOP 0 [0] [23,23,23,23]

@holiman
Copy link
Contributor

holiman commented Apr 28, 2020

0x610400600055b260016000540380156018576000556006b35b sets N to 1024 (succeeds on geth)
0x610401600055b260016000540380156018576000556006b35b sets N to 1025 (fails on geth)

This is the underlying instrutions:

		code := []byte{
			byte(vm.PUSH2), 0x04, 0x01, //value : 0x400 = 1024
			byte(vm.PUSH1), 0, // location
			byte(vm.SSTORE), // set slot 0 to 1024
			byte(vm.BEGINSUB),
			// if slot 0 == 0 then break
			byte(vm.PUSH1),1, // push the 1 for the sub
			byte(vm.PUSH1), 0, // slot number
			byte(vm.SLOAD), // slot 0 on stack
			byte(vm.SUB), // slot 0 minus one
			byte(vm.DUP1), // dup it
			byte(vm.ISZERO), 
			byte(vm.PUSH1), 24, // jumpdestination if we want to exit
			byte(vm.JUMPI), // jump if the slot reached zero
			// store it again (the new value we dup:ed earlier)
			byte(vm.PUSH1), 0, // location
			byte(vm.SSTORE), // set slot 0 to new number
			// jump into subroutine again
			byte(vm.PUSH1), 6,
			byte(vm.JUMPSUB),
			// exit label
			byte(vm.JUMPDEST),
		}

@holiman
Copy link
Contributor

holiman commented Apr 28, 2020

Oh, and the code below only "stores back" the counter after the zero-check, so it should end up with 1 on slot 0 in the success-case, because if it's zero it jumps out instead

@matkt
Copy link
Contributor Author

matkt commented Apr 28, 2020

Oh, and the code below only "stores back" the counter after the zero-check, so it should end up with 1 on slot 0 in the success-case, because if it's zero it jumps out instead

I had not seen your message and I added my own implementation which is very close to yours. As you have already documented yours I will update my tests

@matkt
Copy link
Contributor Author

matkt commented Apr 28, 2020

0x610400600055b260016000540380156018576000556006b35b sets N to 1024 (succeeds on geth)
0x610401600055b260016000540380156018576000556006b35b sets N to 1025 (fails on geth)

This is the underlying instrutions:

		code := []byte{
			byte(vm.PUSH2), 0x04, 0x01, //value : 0x400 = 1024
			byte(vm.PUSH1), 0, // location
			byte(vm.SSTORE), // set slot 0 to 1024
			byte(vm.BEGINSUB),
			// if slot 0 == 0 then break
			byte(vm.PUSH1),1, // push the 1 for the sub
			byte(vm.PUSH1), 0, // slot number
			byte(vm.SLOAD), // slot 0 on stack
			byte(vm.SUB), // slot 0 minus one
			byte(vm.DUP1), // dup it
			byte(vm.ISZERO), 
			byte(vm.PUSH1), 24, // jumpdestination if we want to exit
			byte(vm.JUMPI), // jump if the slot reached zero
			// store it again (the new value we dup:ed earlier)
			byte(vm.PUSH1), 0, // location
			byte(vm.SSTORE), // set slot 0 to new number
			// jump into subroutine again
			byte(vm.PUSH1), 6,
			byte(vm.JUMPSUB),
			// exit label
			byte(vm.JUMPDEST),
		}

I just put your implementation. tests work well on Besu

Copy link
Collaborator

@winsvega winsvega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some tests with 1023/24/25 subcalls already. You can also peek if there are some ways to organize stack depth calls for tests

@matkt
Copy link
Contributor Author

matkt commented May 18, 2020

I just updated the tests to manage the last PR (enter only via jumpsub) ethereum/EIPs#2646

  • I updated shouldSucceedWhenReturnStackGrowsUntil1023 and shouldErrorWhenReturnStackGrowsAbove1023 by adding a JUMPSUB at the start to not go through the BEGINSUB and to not break the test with the last update
Before : 0x610400600055b260016000540380156018576000556006b35b 
After :  0x61[03FF]600055[6009b3]b2600160005403801560[1B]5760005560[09]b35b
  • I added three new tests for Subroutines can only be entered via JUMPSUB and not BEGINSUB. Attempted execution of a BEGINSUB causes an abort: terminate execution with an OOG (Out Of Gas) exception

shouldErrorWhenExecuteBeginSub

Bytecode: `0x610400b2`
LLL :  (asm 0x0400 BEGINSUB)
PC Opcode Cost Stack RStack
0 PUSH2 3 [] []
3 BEGINSUB 0 [1024] []

Error: at pc=3, op=BEGINSUB: evm: out of gas

shouldErrorWhenSubroutineEnteredViaBeginSub

Bytecode: `0x6004b300b2b2b7`
LLL :  (asm 0x04 JUMPSUB STOP BEGINSUB BEGINSUB RETURNSUB)
PC Opcode Cost Stack RStack
0 PUSH1 3 [] []
2 JUMPSUB 8 [4] []
5 BEGINSUB 0 [] [2]

Error: at pc=5, op=BEGINSUB: evm: out of gas

beginSubAtEndOfCode

Bytecode: `0x6003b3b2`
LLL : (asm 0x03 JUMPSUB BEGINSUB)
PC Opcode Cost Stack RStack
0 PUSH1 3 [] []
2 JUMPSUB 8 [3] []
4 STOP 0 [] []

@holiman
Copy link
Contributor

holiman commented May 19, 2020

Hm, I'm actually getting an error now

    --- FAIL: TestState/shouldSucceedWhenReturnStackGrowsUntil1023.json (0.09s)
            state_test.go:96: post state root mismatch: got abe6b4c387d5363f896de0c5c3aa9f2762f1e4931975b83dbecdcea962ec5695, want 4b4ded39ed6ea35bbc1b1581b1f657ccb15a834fb6ce641c2280456a9fd4178b

@matkt
Copy link
Contributor Author

matkt commented May 19, 2020

Hm, I'm actually getting an error now

    --- FAIL: TestState/shouldSucceedWhenReturnStackGrowsUntil1023.json (0.09s)
            state_test.go:96: post state root mismatch: got abe6b4c387d5363f896de0c5c3aa9f2762f1e4931975b83dbecdcea962ec5695, want 4b4ded39ed6ea35bbc1b1581b1f657ccb15a834fb6ce641c2280456a9fd4178b

are you testing with the latest updates?

@holiman
Copy link
Contributor

holiman commented May 19, 2020

Sorry, my bad. We haven't enabled 'Berlin', so I added a hack to redefine "Berlin" as "Byzantium+2315", but the correct thing was Istanbul+2315. With that, the tests matches geth! Well done!

@holiman
Copy link
Contributor

holiman commented May 19, 2020

I guess besu doesn't use the base+EIP notation? It would be pretty handy if other clients supported that, because it makes things easier -- in the case where we have multiple eips going into a fork eventually, and it's not certain which ones were in when a specific test was generated.

@matkt
Copy link
Contributor Author

matkt commented May 19, 2020

I guess besu doesn't use the base+EIP notation? It would be pretty handy if other clients supported that, because it makes things easier -- in the case where we have multiple eips going into a fork eventually, and it's not certain which ones were in when a specific test was generated.

Yes, for the moment it is not managed. we will try to manage this feature as soon as possible

@holiman holiman merged commit 66b1043 into ethereum:develop May 24, 2020
@matkt
Copy link
Contributor Author

matkt commented May 25, 2020

@holiman thank you for your review on this PR. As the tests are pushed in develop does that mean that it confirms a JUMPSUB which costs 8?

@holiman
Copy link
Contributor

holiman commented May 25, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants