feat(tests): converting calldataload and calldatasize tests

[pacrob]

🗒️ Description

🔗 Related Issues

✅ Checklist

• All: Set appropriate labels for the changes.
• All: Considered squashing commits to improve commit history.
• All: Added an entry to CHANGELOG.md.
• All: Considered updating the online docs in the ./docs/ directory.
• Tests: All converted JSON/YML tests from ethereum/tests have been added to converted-ethereum-tests.txt.
• Tests: A PR with removal of converted JSON/YML tests from ethereum/tests have been opened.
• remove calldataload and calldatasize tests tests#1477
• Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
• used geth version 1.15.2-stable-c8c62daf
• Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

[winsvega]

is there better way to declare bytes ? more readable

[pacrob]

shortened and labeled!

[winsvega]

evmone build script failing

[pacrob]

OK, I think it's gtg. Still getting coverage loss though. How can I see what's lost , and is there a solution? I think my previous PR had the same issue.

===============
COVERAGE REPORT
/tests/DIFF/index.html
New covered Lines: 180
New covered function Paths: 77

ERROR: coverage is lost!
Lost coverage Lines: 26
Lost coverage function Paths: 5

[marioevz]

Hey, some comments which I think should make the test a bit more EEST-like, but we might lose irrelevant coverage (Like the SHA3 function which is IMO is out-of-scope for these tests) but that's ok.

[marioevz]

Suggested change

	("0002", "01", Account(storage={0x00: 0x02})),
	("0002", "01", 0x02),

Similar comment to previous file, Account(storage=... can be defined inside the function to make the parametrization more readable.

[marioevz]

No need to encode this as call data to be decoded by the contract, args_size and sha3_len could be integers and they could be placed inside of the contract directly:

    to = pre.deploy_contract(
        code=(
            Op.MSTORE(offset=0x0, value=Op.SHA3(offset=0x0, size=sha3_len))
            + Op.CALL(
                gas=Op.SUB(Op.GAS(), 0x100),
                address=address,
                value=0x0,
                args_offset=0x0,
                args_size=args_size,
                ret_offset=0x0,
                ret_size=0x0,
            )
        )
    )

Although I'm having trouble understanding why the original test is using SHA3 in the first place since the memory contents are irrelevant, and my best guess is to just have a non-zero filled memory.

I think we should replace all of this with calldata just like the comment in test_calldataload.py, and also parametrize this function to test calldatasize at the top-level call (returns len(tx.data)) or a sub-level call (returns arg_size).

[pacrob]

@marioevz I simplified it a bit, but I'm pretty sure it's still checking correctly. Anything I'm missing?

[winsvega]

lets add push32, mstore8, add opcodes to default opcodes exempt from coverage check requirements.
it is in the file tests/homestead/coverage/test_coverage.py

[pacrob]

lets add push32, mstore8, add opcodes to default opcodes exempt from coverage check requirements. it is in the file tests/homestead/coverage/test_coverage.py

Not all fixed, but better. Thanks!

before:

ERROR: coverage is lost!
Lost coverage Lines: 34
Lost coverage function Paths: 11

after:

ERROR: coverage is lost!
Lost coverage Lines: 33
Lost coverage function Paths: 3

[pacrob]

Latest coverage fail:

Lost coverage Lines: 56
Lost coverage function Paths: 9

Looking into it:

expected:

• Lost 22 lines and 6 functions from lib/evmone due to no longer using keccak or calldataload in the calldatasize test
• Lost 7 lines test/state/host.cpp, 3 lines and one function in test/state/host.cpp due to not including a value in the transaction in the calldatasize test.

unknown:

• Lost 2 lines in evmc/include/evmc, showing that the test is no longer checking Prague. Why might that be happening?
• Leaving 22 lines and 2 functions across mpt.cpp, state.cpp, and system_contracts.cpp. Not sure of source.

I'll keep poking, but if you know whether the last 2 can be fixed or should be ignored, let me know!

[winsvega]

About sha3

I don't get what are the inputs for call in Oris test.
I suggest we debug original test, derrive the inputs and reproduce in python.

[marioevz]

Hey, some comments, sorry about the back and forth but hopefully this is the last one :)

[marioevz]

Suggested change

	address_b = pre.deploy_contract(
	Om.MSTORE(calldata, 0x0)
	+ Op.CALL(
	gas=Op.SUB(Op.GAS(), 0x100),
	address=address_a,
	value=0x0,
	args_offset=0x0,
	args_size=len(calldata),
	ret_offset=0x0,
	ret_size=0x0,
	)
	)
	to = pre.deploy_contract(
	code=(
	Op.ADD(0x1000, Op.CALLDATALOAD(offset=0x4))
	+ Op.CALL(
	gas=Op.SUB(Op.GAS(), 0x100),
	address=address_b,
	value=0x0,
	args_offset=0x0,
	args_size=0x0,
	ret_offset=0x0,
	ret_size=0x0,
	)
	+ Op.STOP
	),
	)
	to = pre.deploy_contract(
	Om.MSTORE(calldata, 0x0)
	+ Op.CALL(
	gas=Op.SUB(Op.GAS(), 0x100),
	address=address_a,
	value=0x0,
	args_offset=0x0,
	args_size=len(calldata),
	ret_offset=0x0,
	ret_size=0x0,
	)
	)

Can be simplified like this.

It might result in a drop in coverage because we eliminate the use of the ADD opcode, but it's not the focus of the test so it's ok.

Reason behind these extra contracts in the original test is because the original yml hardcoded many contracts in the same test, but it's no longer necessary.

[marioevz]

We could also do a variant of the test where the entry point is address_a to verify that Op.CALLDATALOAD behaves correctly when it's fetching data from the transaction's calldata.

@pytest.mark.parametrize("calldata_source", ["tx", "contract"])

And if calldata_source == "tx":

tx = Transaction(
        data=calldata,
        gas_limit=100_000,
        protected=fork >= Byzantium,
        sender=pre.fund_eoa(),
        to=address_a,
    )

And the to contract is omitted entirely.

[marioevz]

Same comment as the other test where we can parametrize by calldata_source.

[pacrob]

Hey, @marioevz, I've addressed your recent feedback. Let me know if you have more!

[marioevz]

LGTM, thanks!

[marioevz]

Checking the lost coverage and it's due of the following reasons:

• Removal of SHA3 opcode.
• Account deletion code branches.
• Value transferring code branches.
  The lost coverage is not relevant to the test, so we can safely proceed to merge.