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

Emscripten binaries for version 0.3.6 in solc-bin were not built from the commit tagged as v0.3.6 #10846

Open
cameel opened this issue Jan 25, 2021 · 5 comments
Labels
bug 🐛 low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. should have We like the idea but it’s not important enough to be a part of the roadmap. solcbin

Comments

@cameel
Copy link
Member

cameel commented Jan 25, 2021

In solc-bin we have soljson-v0.3.6+commit.3fc68da5.js and soljson-v0.3.6+commit.3fc68da5.js, both built from 3fc68da. Tag v0.3.6 points at commit 988fe5e (i.e. one commit after 3fc68da).

This is a problem because they will produce different bytecode than releases from other platforms built on v0.3.6 (e.g. the recent macOS rebuilds).

The binaries in solc-bin are not necessarily the files that were released - release v0.3.6 has no binaries attached at all - so I think we could rebuild on the tag and put both in the repo.

@cameel
Copy link
Member Author

cameel commented Jan 25, 2021

@chriseth I have manually compared the Linux and wasm reports for 0.3.6 and bytecode differs only in a handful of cases. All of these contain libraries and the only difference is this small fragment:

< 03063fc68da5
---
> 0306cbf55226

I investigated this and turns out it's the version stamp for libraries added by injectVersionStampIntoSub():

void CompilerContext::injectVersionStampIntoSub(size_t _subIndex)
{
	eth::Assembly& sub = m_asm.sub(_subIndex);
	sub.injectStart(Instruction::POP);
	sub.injectStart(fromBigEndian<u256>(binaryVersion()));
}

Note that the stamp in each case consists of 03 06 (corresponding to 0.3.6) followed by the first 8 digits of the commit hash.

Surprisingly, cbf55226 is not a valid commit and definitely not the commit v0.3.6 points at (988fe5e). It's probably the commit ID that was created after the script cherry-picked necessary fixes. It should not happen though because I did set commit_hash.txt to get the same commit ID as in the emscripten case. This method worked for >= 0.4.0 (bytecode reports match emscripten) so I wonder if 0.3.6 maybe just did not not support commit_hash.txt? I need to check that.

Another unexpected thing is that I did not see for >= 0.4.0 is that there are a few files for which one of the binaries errored out and the other did not.

Full bytecode diff

Here are the whole reports if you want to take a look:

For convenience, here's one of the test cases that have this difference in the bytecode:

1c1
< test_06bdc56c19e023d03ff89762979800ef24c770577d9138159f8b62b30650da2b_solidityendtoendtest_cpp.sol:D 6060604052607c8060106000396000f36503063fc68da55060606040526000357c010000000000000000000000000000000000000000000000000000000090048063eee9720614604157603d565b6007565b60556004808035906020019091905050606b565b6040518082815260200191505060405180910390f35b60008160020290506077565b91905056
---
> test_06bdc56c19e023d03ff89762979800ef24c770577d9138159f8b62b30650da2b_solidityendtoendtest_cpp.sol:D 6060604052607c8060106000396000f3650306cbf552265060606040526000357c010000000000000000000000000000000000000000000000000000000090048063eee9720614604157603d565b6007565b60556004808035906020019091905050606b565b6040518082815260200191505060405180910390f35b60008160020290506077565b91905056

Full bytecode diff for a test case where one of the binaries errors out

< test_17921e1d1ec89741ca84ac79266675a6e3afb8a468111859ace1e73eaf606c30_gasmeter_cpp.sol: <ERROR>
---
> test_17921e1d1ec89741ca84ac79266675a6e3afb8a468111859ace1e73eaf606c30_gasmeter_cpp.sol:test 606060405260c68060106000396000f360606040526000357c010000000000000000000000000000000000000000000000000000000090048063b3de648b146041578063e420264a14605757603f565b005b605560048080359060200190919050506081565b005b606b600480803590602001909190505060b3565b6040518082815260200191505060405180910390f35b600781111560a357600160956008830a60b3565b0160016000508190555060af565b60016000600050819055505b5b50565b6000600160005054905060c1565b91905056
> test_17921e1d1ec89741ca84ac79266675a6e3afb8a468111859ace1e73eaf606c30_gasmeter_cpp.sol:test <NO METADATA>

@axic
Copy link
Member

axic commented Jan 26, 2021

I think it would be useful documenting all these in a section in the documentation (instead of issues). By "documenting all these" I mean what people should expect from which distributed version. It may be a better option than trying to resolve problems for ancient releases. Of course documenting it would not be possible without the meticulous investigative work you have done with these.

@cameel
Copy link
Member Author

cameel commented Jan 26, 2021

Yeah, documenting them sounds like a good idea. But I think we still should fix at least those problems that are easy to fix (like for example renaming the Linux 0.4.10) :)

Do you think it should be documented in the main docs or is the solc-bin README (or separate READMEs in the directories containing the binaries) a better place?

@axic
Copy link
Member

axic commented Jan 26, 2021

But I think we still should fix at least those problems that are easy to fix (like for example renaming the Linux 0.4.10) :)

Absolutely, where it is easy to fix, go ahead. But probably it is not worth spending days/weeks on these :)

Do you think it should be documented in the main docs or is the solc-bin README (or separate READMEs in the directories containing the binaries) a better place?

I think the main documentation is better, nobody reads those readmes too much.

@cameel
Copy link
Member Author

cameel commented Jan 26, 2021

OK. So I'll add a section the docs on that.

@cameel cameel added low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. nice to have We don’t see a good reason not to have it but won’t go out of our way to implement it. should have We like the idea but it’s not important enough to be a part of the roadmap. and removed nice to have We don’t see a good reason not to have it but won’t go out of our way to implement it. labels Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. should have We like the idea but it’s not important enough to be a part of the roadmap. solcbin
Projects
None yet
Development

No branches or pull requests

2 participants