Add option to ignore EIP 170 for debugging purposes

[mikeseese]

I'm creating a real-time Solidity debugger runtime for an Augur bounty.

To do this sanely, compiler optimization needs to be turned off for the developer to be follow a logical stepping process; otherwise it's possible for the compiler to optimize things away and cause unexpected behavior from a debugging perspective.

Unfortunately, turning off optimization can mean very large compiled contracts. EIP 170 prevents contracts with more than 0x6000 bytes (or 24KB) from being created. The rationale for EIP 170 (as I interpreted it) was really to address scaling from a global perspective, however I can see it acceptable to allow users to ignore EIP 170, specifically for debugging purposes.

This PR adds the option for ignoring EIP 170.

[coveralls]

Coverage increased (+0.009%) to 85.462% when pulling ad20751 on seesemichaelj:ignore-eip-170 into 9bc99c3 on ethereumjs:master.

[holgerd77]

Hi Mike, I think this is a useful addition and would be supportive of it. Wonder if it would be an alternative to rename this explicitly after the use case (allowUnlimitedContractSize) - since no-one nows EIP-170, but I'm actually also undecided about this.

Can you add this as well to the constructor doc strings and add the addition manually also to the README?

[holgerd77]

@axic I already wondered a couple of times if we need an additional test setup testing inner library functionality or API functionality, since we can't test things like this atm with our current test setup with just running the external Ethereum tests. Not sure if it's worth the effort though (with eWASM on the horizon... 😄 )

[mikeseese]

I agree with renaming the option; I think allowUnlimitedContractSize is good. I think as long as the default is false and the readme/jsdoc explains that it should be noted that the user can have failures with deploying to a real network because of EIP 170. I'll add some commits later to address this.

[holgerd77]

We have relatively strong guidelines regarding not bloating up the commit history too much with back-and-forth commits, so preferably do this via rebase if you don't add but just change/revert something. If you haven't done rebases before, I recommend a local backup of the repository folder (can go wrong 😄 ).

[benjamincburns]

Wonder if it would be an alternative to rename this explicitly after the use case (allowUnlimitedContractSize) - since no-one nows EIP-170, but I'm actually also undecided about this.

Better yet, can we make this a Number instead of a boolean, and call it maxContractSize?

[mikeseese]

From a developer's perspective, having to figure out what the maxContractSize is could be tedious. The dev should only use this option when not optimizing compilation, and the dev really doesn't care what the max size is. From a dev's perspective, I'd just want to turn off the check altogether. If I was forced to pick a sized, I would probably just put something "arbitrarily large" until I didn't get errors anymore.

Devil's advocate can see why a Number would be better than a Boolean; it attempts to forces the dev to think about what they're doing, as well as give a fall back (i.e. "it can be bigger, but not too much bigger"). However, dev's shouldn't be using this feature in any sort of production-like capacity, which would make me go back to the "I guess I'll just pick a big number". Otherwise devs could set the number to be the size of one contract, compile, run tests, set size to next bigger contract, compile, run tests, get frustrated that they keep having to increase it so they make it an arbitrarily large number which defeats the purpose of making it a Number vs Boolean.

Thoughts?

[holgerd77]

Yes, I would also stick to the boolean version first proposed. The complexity added like described by @seesemichaelj would be counter-productive. If this is needed, it would be better to add this as a separate option. @benjamincburns do you have got a concrete use case for this/would like to have this integrated?

[axic]

I'd rather see the "ewasmfork" option: https://github.com/ethereum/cpp-ethereum/blob/develop/libethcore/EVMSchedule.h#L120 :)

[holgerd77]

Many space, very options. 🐙 😄

[benjamincburns]

I agree with @seesemichaelj’s assessment. My original motivation for the assessment was flexibility, but I agree that such flexibility in this case is a hinderance.

[tcoulter]

Thanks for submitting this @seesemichaelj. Seems like a good change. I definitely agree the name should be allowUnlimitedContractSize.

As a long term strategy for changes like this, I wonder if there's a way we can architect the VM such that we don't have to create lots of flags altering functionality (and injecting a host of conditionals). Instead, as an example, could we make an API for the data layer that lets you add a new contract to the network, sans limits? The limits being circumvented here are protocol level, and if we had the power to alter the data at will we can unlock a host of new opportunities beyond the scope of the original PR.

[holgerd77]

@tcoulter can't really follow how this actually would look like I have to admit.

[holgerd77]

@seesemichaelj Hi Mike, I think we have an agreement here now to

a) Use allowUnlimitedContractSize as naming
b) Not switch to a more flexible maxContractSize version

Can you prepare this (please replace the old commit and not do the changes on top) and also add this to the constructor doc strings and manually to the README API description?

That would be great! :-)

[mikeseese]

Will do! Sorry, I've been busy and this fell through the cracks.

On a side note, if you didn't know, GitHubs gives you a squash and merge option when pulling in PRs which will merge a single commit rather than all of them

[mikeseese]

@holgerd77 please see the updated commit. I wanted to provide a little more detail in the README, and not clutter up the constructor doc string. Let me know if you want me to change/reword it

[holgerd77]

This looks good now, thanks for re-submitting.