Skip to content

Option to not store Swarm hash of a metadata file into the bytecode #1571

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

Closed
ethernomad opened this issue Jan 17, 2017 · 21 comments
Closed

Option to not store Swarm hash of a metadata file into the bytecode #1571

ethernomad opened this issue Jan 17, 2017 · 21 comments
Assignees

Comments

@ethernomad
Copy link
Contributor

It isn't possible to use the etherscan.io verification facility now that this functionality has been added.

Can it be made a command line option?

@chriseth
Copy link
Contributor

chriseth commented Jan 17, 2017

Can you explain a bit more why this breaks etherscan verification? The idea was that it actually helps verification...

@ethernomad
Copy link
Contributor Author

ethernomad commented Jan 17, 2017

The problem is that the bytecode is now dependent on more things. You can use the same compiler version, but if you use Linux instead of browser solidity or Windows you will get a different hash.

Another problem is the import directive. etherscan doesn't support this as you have to paste in a single file. Normally you would get around this problem by manually replacing the import directive with the contents of the imported file. But now this changes the hash and it won't verify.

@chriseth
Copy link
Contributor

chriseth commented Jan 17, 2017

Do you have examples of getting different hashes on linux and windows? I assume this is mostly related to file names because they are part of the metadata and thus of the bytecode.

I would not recommend replacing the import directive by the file contents, this modifies the code. Instead, please call the compile with multiple inputs.

So my feeling is that requiring the user to supply file names or even better, require them to publish the metadata file instead of just the source code, would solve the problem.

@ethernomad
Copy link
Contributor Author

The operating system is in the metadata:

{"compiler":{"version":"0.4.8+commit.60cc1668.Linux.g++"}

Replacing import directives with file contents is a requirement of how etherscan's verification works.

I'm sure a better verification system can be built using the metadata hash. My request is that there should still be the option to use the existing etherscan verification system with the latest compiler until etherscan deploy a new system that uses the metadata hash.

@chriseth
Copy link
Contributor

If etherscan modifies the source in such a way, then things like import {X} from "b.sol"; or import "b.sol" as Y; do not work, and I guess that the larger contract systems make heavy use of this.

I really want to urge people to use the metadata hash, so I don't think that removing it is a good way to push it forward. Would it be an option to ignore the metadata hash in the etherscan's verification procedure and also warn the users about this?

@axic
Copy link
Member

axic commented Jan 17, 2017

While I like having these optional, the goal is to have it in a way which wouldn't interfere with other ways.

@mtbitcoin As an intermediate measure, would it be possible to add the hash parsing to etherscan and ignore that bit when comparing the outputs? See this code how to match the hash: https://github.com/ethereum/browser-solidity/blob/master/src/app/renderer.js#L94

And here's a more detailed explanation: http://solidity.readthedocs.io/en/develop/miscellaneous.html?highlight=metadata#encoding-of-the-metadata-hash-in-the-bytecode

@ethernomad
Copy link
Contributor Author

Okay I guess it's up to etherscan to fix their verification system.

@axic
Copy link
Member

axic commented Jan 17, 2017

As per future verification work, having different versions could manifest as problems, albeit not necessarily:

{"compiler":{"version":"0.4.8+commit.60cc1668.Linux.g++"}

We aim to use this to lookup the compiler version, and very likely, most implementations would resort to using solc-js to rerun the compilation.

For verification, the bytecode would need to be compared entirely, including the hash. This will obviously fail if it was compiled with a Linux or Windows solc.

Either we ignore the hash or truncate the version number not to include the OS.

@chriseth
Copy link
Contributor

chriseth commented Jan 17, 2017

Note that the behaviour of some weird contracts might depend on the metadata hash as you can read it using assembly, so a proper verification also has to check the metadata hash.

@holiman
Copy link
Contributor

holiman commented Jan 17, 2017

Wouldn't a solution be to have an option to the compiler, --metadata='0.4.8+commit.60cc1668.Linux.g++'. So if you want to verify, just take the metadata from the contract?

@chriseth
Copy link
Contributor

@axic @holiman yes, I think we should just truncate the version. If it does not compile the same way on a different operating system the security of the contract is in danger anyway. I also considered @holiman's suggestion of adding a "fake version string", but I think that opens the door for manipulation.

@chriseth
Copy link
Contributor

chriseth commented Jan 17, 2017

If you really rely on some specific operating system / architecture, you can provide that as a comment in the source code.

@axic
Copy link
Member

axic commented Jan 17, 2017

The output of the compiler on all platforms should match and if it doesn't we have a bug to fix 😉, therefore truncating the version should be safe.

@axic
Copy link
Member

axic commented Jan 17, 2017

Wouldn't a solution be to have an option to the compiler, --metadata='0.4.8+commit.60cc1668.Linux.g++'

I'm strongly against adding an option to modify the version number from the outside.

@holiman
Copy link
Contributor

holiman commented Jan 17, 2017

but I think that opens the door for manipulation.

Hold on - we can't assume that this wont be manipulated, and shouldn't place any trust in that piece of data. It's just for aiding verification, but if it's going to be used for 'cheating' by skipping verification altogether, well, that sounds like a bad idea.

@axic
Copy link
Member

axic commented Jan 17, 2017

@holiman it is only used to look up the compiler to use for compilation. If it is manipulated, it defeats its purpose.

Note: the verification doesn't save you from knowing which Solidity versions are vulnerable.

@mtbitcoin
Copy link

mtbitcoin commented Jan 17, 2017

@ethernomad @axic The additional swarm hash does not break Etherscan's source code verification as I already updated it previously (1-2 weeks ago) to handle the different swarm hashes. So basically the swarm hash for contracts compiled with solc > 0.4.7 are not used as part of the comparison critieria.

For example see https://etherscan.io/address/0x9ae98746EB8a0aeEe5fF2b6B15875313a986f103#code which was successfully compiled and verified with 0.4.8

@mtbitcoin
Copy link

As for verifying contract code with 'includes' , does https://www.npmjs.com/package/solc (node) support command line verification for including source file imports?

@axic
Copy link
Member

axic commented Jan 17, 2017

@mtbitcoin the verification library is in progress and it may not be part of solc-js.

@mtbitcoin
Copy link

Would it be an option to ignore the metadata hash in the etherscan's verification procedure and also warn the users about this?

@chriseth the metadata hash is already being ignored in the verification procedure

@chriseth
Copy link
Contributor

@mtbitcoin perhaps we should take that offline, but multi-file inputs are supported by solc when used as a library. It is not supported by the commandline version because we want to move to a unified json input format soon that works in the same way for all commandline compilers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants