[isoltest] Add support for events using call side-effects.

[aarlt]

Fixes #6902.

[chriseth]

Please try to parse the errors and display them nicely. If this is more than a day's work, then at least try to display the error signature instead of its hash and split the data by chunks of 0x20 bytes and format them as "unknown data".

[aarlt]

@cameel Thanks for your review! I think I covered everything, except the actual parameter decoding. It should not be that complicated, but I will add this tomorrow.

[cameel]

OK. I'll take another look today. From what I've seen yesterday, most of the issues I found were cosmetic so this should be quick.

If the decoding turns out to be time-consuming, feel free to just skip it in this PR. It's just nice to have, not anything critical.

[cameel]

Reviewed again, including the older comments. All of those older ones seem solved or irrelevant. So the remaining things (apart from some minor tweaks) are:

• assert for number of parameters is probably wrong ([isoltest] Add support for events using call side-effects. #11050 (comment))
• test for a conflict ([isoltest] Add support for events using call side-effects. #11050 (comment))
• data decoding ([isoltest] Add support for events using call side-effects. #11050 (comment)) - optional, skip if needs time

[aarlt]

@cameel I added a very simple parameter decoder. I tried to implement a more advanced decoder, but it would take more time to implement this completely. Right now it only uses the parameter types for indexed/nonIndexed events for simple bool's. By default everything is just decoded as hex, but if the hex data consists of printable ascii characters, the parameter will be decoded as a string. For this none of the provided type information is used. Also the parameters are just decoded in 32 byte chunks.

However, at least we have the parameter type information available, so if needed we could improve the decoding in another PR.

[aarlt]

I just saw that chk_spelling fails, because the strings are split into 32 byte chunks. I think it should be easily possible to just merge multiple string literals.

[chriseth]

Suggested change

	LogRecord(util::h160 _creator, bytes _data, std::vector<util::h256> _topics)
	: creator(_creator), data(std::move(_data)), topics(std::move(_topics)) {}
	LogRecord(util::h160 _creator, bytes _data, std::vector<util::h256> _topics):
	creator(_creator), data(std::move(_data)), topics(std::move(_topics)) {}

[chriseth]

Why don't you move the creator? Also do you really need a constructor or would {}-constructing be enough?

[aarlt]

I was also wondering why the constructor is needed, but it seem to be needed.

[cameel]

It's because of the logs.emplace_back() call in ExecutionFramework::recordedLogs(). Apparently it tries to execute LogRecord() rather than LogRecord{} and a struct does not have the former. This is supposed to change in C++20 from what I've read.

So we could leave it as is or do logs.push_back({x, y, z ...}) instead which will allow us to remove the constructor.

Why don't you move the creator?

I wanted to point that out originally but then noticed that creator is h160, which is FixedHash, which is std::array so there's no benefit from moving it. It being const& parameter would make more sense.

[chriseth]

Maybe just change the string in snark.sol, we can think about a better solution later.

[cameel]

Looks fine now. I have two small suggestions and you need to update gas cost in snark.sol but other than that I think we're done here.