[ewasm] Polyfill: callvalue, gasprice & difficulty.

[aarlt]

No description provided.

[axic]

All of these tests would make more sense with having mstore(0, <some random number>) mstore(32, <some random number>) at the beginning to clobber the space the polyfill is writing to.

[aarlt]

I agree. We should improve the ewasm translation tests. I think the memory there should not be initialized with zero's. So we could easily check how many bytes where written by the e.g. polyfills.

[axic]

It should be initialised to zeroes, as it is done so on wasm and evm.

However we should change the tests to explicitly make those slots dirty.

[axic]

This PR still waits for adding these clobbering instructions to these files.

[cameel]

@axic Is 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff good enough for that or do you think it should actually be something looking more random?

[axic]

So actually instead of sstore() we wanted to use mstore() for clobbering, however that starts at actual memory 0x40 and the lower 0x40 bytes are used as scratch space in the translators:

+++ b/test/libyul/ewasmTranslationTests/origin.yul
@@ -1,9 +1,13 @@
 {
+  mstore(0, 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff)
+  mstore(32, 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff)
   sstore(0, origin())
 }
 // ----
 // Trace:
 // Memory dump:
 //     20: 0000000000000000000000000000000000000000000000000000000033333333
+//     40: ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
+//     60: ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
 // Storage dump:
 //   0000000000000000000000000000000000000000000000000000000000000000: 0000000000000000000000000000000000000000000000000000000033333333

I think we should just remove sstores from this PR and try to figure out clobbering later.

[axic]

Need to rebase due to other polyfill changes were merged.

[leonardoalt]

@axic was that an "approve after rebase"?

[chriseth]

This does not need a bswap?

[cameel]

@aarlt Yeah, mload_internal() does the endianness swap internally. Why doesn't it need it? And why for example balance() still does?

[aarlt]

I checked the hera implementation EthereumInterface::eeiGetCallValue(uint32_t resultOffset) is using storeUint128(m_msg.value, resultOffset); to write the actual value. storeUint128 uses storeMemoryReverse to store that value. That means the value should be big-endian in memory and therefore the usage of bswap should be needed here.

The strange thing is that the tests only run correctly, if we don't do the byte-swap.

So I thought that we store the value wrong within the call polyfill. After checking I saw we store it correctly, we retrieve the value and use mstore_internal. That means we definitely should have the value as big-endian in memory:

	let v1, v2 := u256_to_u128(c1, c2, c3, c4)
	mstore_internal(32:i32, 0, 0, v1, v2)
	// ...
	x4 := i64.extend_i32_u(eth.call(g, 12:i32, 32:i32, to_internal_i32ptr(d1, d2, d3, d4), u256_to_i32(e1, e2, e3, e4)))

So I checked then what hera is actually doing in eeiCall. Here we load the memory correctly call_message.value = loadUint128(valueOffset);, during the load we also swap big-endian back to little-endian, because loadUint128 uses loadMemoryReverse.

However, I noticed that eeiCall gets never executed. So I took a look at the ExecutionFramework, especially ExecutionFramework::sendMessage. Here we setup the message value with message.value = EVMHost::convertToEVMC(_value);. And looking at message.value, it is already big-endian (the higher addresses store smallest parts of the value). That means if this value is stored using storeUint128 via eth.getCallValue the bytes will get swapped again to little-endian. Thats why I noticed that the bswap is not needed here.

I guess that the message values need to be in big-endian, before they arrive at an vm. If we would change it here, the current tests via evm-one would also fail. I think the interaction with an evm from the test-framework perspective should be identical, for the evmone- and the ewasm-case. So I think it is a problem in hera.

I will take a look what is the best option to fix hera tomorrow.

@axic would you agree that this is a hera problem?

[axic]

From https://github.com/ewasm/design/blob/master/eth_interface.md:

u128: a 128 bit number, represented as a 16 bytes long little endian unsigned integer in memory

getExternalBalance
Gets balance of the given account and loads it into memory at the given offset.

Parameters

addressOffset i32ptr the memory offset to load the address from (address)
resultOffset i32ptr the memory offset to load the balance into (u128)

The byteswapping in storeUint128 is because EVMC considers the value big endian, so this swaps it for wasm.

[chriseth]

This is still to do, I think: https://github.com/ethereum/solidity/pull/10406/files#r532985621

[aarlt]

@axic Please review

[leonardoalt]

Ah, tests here are also failing with "Invalid code generated", wondering if it's something common between the 2 PRs

[aarlt]

@leonardoalt #10686 uses some features that are not yet ready in ewasm. I just disabled the ewasm execution of these tests.

[cameel]

What is the purpose of this PR? Looking at the title I expected that it implements the three polyfills but I see that polyfills are already implemented and you seems to be just changing the endianness of their results. So is this actually a bugfix?

If changing the endianness is what it's meant to do then the code looks fine now. I did not find any actual problems.

[aarlt]

So is this actually a bugfix?

If changing the endianness is what it's meant to do then the code looks fine now. I did not find any actual problems.

@cameel Exactly. This PR is a bugfix. The original polyfills where implemented without any real ewasm vm. After we have plugged in hera, we noticed that the endianess was implemented wrong.

[cameel]

@aarlt Ok. I'd suggest renaming in then since the title is the only thing you have for context when starting to review it :)

[cameel]

I think this is good to go.

@axic, @leonardoalt, @chriseth It looks to me like all your comments were addressed. Is there anything else to do here?

Actually, there's still a pending question about bswap in value().

[aarlt]

We should add a test that gets called with a value from the test framework and triggers another call with value to another function. Not sure whether we have such test already.

[chriseth]

Needs rebase.

[leonardoalt]

Still needs rebase.

[leonardoalt]

Ping @aarlt

[cameel]

We discussed this today on the call. This is basically just blocked by #10406 (comment) (and also needs a rebase). But we decided that we could just merge it as is and deal with problems later when/if we encounter them.

[axic]

Removed the sstore and rebased. Since we do not have #10356 merged, I did not try forcing which tests would work now, I think we can do that separately?