Fix custom cleanup sequence missing from metadata when other optimizer settings have default values

[clonker]

Extracted from PR #15834, see #15834 (comment).

The previous implementation did not make use of the yulOptimiserCleanupSteps. As @cameel suspected, providing a custom cleanup sequence under the default optimization sequence previously simply yielded a "optimizer": "enabled":true, "runs": 200} which potentially (or: most likely) doesn't reproduce the original compilation results.

I think it is a bit mitigated by there not being a specific field for the cleanup sequence in the standard-json input but one would rather have to reproduce the entire default optimization sequence, add a colon, and then modify the cleanup sequence.

[cameel]

This needs a changelog entry and a regression test that reproduces it.

Also, pinging @kuzdogan. Sourcify may need a workaround for this.

We should check which versions are affected (I guess it was introduced along with the cleanup sequence?).

[clonker]

We should check which versions are affected (I guess it was introduced along with the cleanup sequence?).

The setting was introduced in v0.8.18 (including it not being part of operator==, PR #13376).
But actually the bug first occurs in v0.8.26 - probably due to some refactoring in correspondence to the metadata output of the optimization settings and in that, relying on the bad comparison operators. I initially failed to take into account that the default sequence changed. It is indeed broken since it has been introduced in v0.8.18.

[clonker]

I don't like this test so much actually. We won't regress if the default sequence changes at some point and it goes unnoticed to update it here as well. Something more elaborate would be first asserting that with the correct sequence it does produce default output and then truncating it in one of these ~-prefixed tests...

[cameel]

Another option would be to add this case to boost-based metadata tests:

solidity/test/libsolidity/Metadata.cpp

Lines 452 to 459 in 9005353

	std::vector<std::tuple<std::string, std::string>> sequences =
	{
	// {"<optimizer sequence>", "<optimizer cleanup sequence>"}
	{"", ""},
	{"", "fDn"},
	{"dhfoDgvulfnTUtnIf", "" },
	{"dhfoDgvulfnTUtnIf", "fDn"}
	};

There you can refer to OptimiserSettings and the default sequence without hard-coding it.

[clonker]

Nice, yeah, much better! I had to refactor it a bit because it was testing against minimal settings plus yul optimizations which was already caught by the old comparison operator. For the regression to kick in, I now take standard settings and modify only the cleanup sequence.

[cameel]

Maybe let's just make it loop over all presets (OptimisationPreset)? This bug shows that we can't really assume that this is completely independent of other settings so this would give us a bit more coverage and also make the test more regular.

For completness, we should also have a test that non-default main sequence and default cleanup works.

We also don't really test that minimal translates to just enabled: false, standard to enabled: false and other presets to different things and we should.

[clonker]

Increased the coverage accordingly.

[cameel]

Another option would be to add this case to boost-based metadata tests:

solidity/test/libsolidity/Metadata.cpp

Lines 452 to 459 in 9005353

	std::vector<std::tuple<std::string, std::string>> sequences =
	{
	// {"<optimizer sequence>", "<optimizer cleanup sequence>"}
	{"", ""},
	{"", "fDn"},
	{"dhfoDgvulfnTUtnIf", "" },
	{"dhfoDgvulfnTUtnIf", "fDn"}
	};

There you can refer to OptimiserSettings and the default sequence without hard-coding it.

[cameel]

In terms of fixing things everything seems in order. We might want to still refactor the test a bit more to give us more coverage. See my previous comment.