Move gasEstimate into CompilerStack by axic · Pull Request #2114 · argotorg/solidity

axic · 2017-04-10T13:52:10Z

Depends on #2113.

axic · 2017-04-10T13:54:41Z

libsolidity/interface/CompilerStack.cpp

+Json::Value gasToJson(GasEstimator::GasConsumption const& _gas)
+{
+    if (_gas.isInfinite || _gas.value > std::numeric_limits<Json::LargestUInt>::max())
+	return Json::Value(Json::nullValue);


This is taken from the jsonCompiler now. Not sure if null or -1 is better (jsonCompiler will need to keep null for backwards compatibility).

I guess we go with -1 since the JSON IO documentation says that.

Hm, I actually think null fits better here - we can still change the documentation, can't we?

We can. I think -1 is nicer because then we are not at the mercy of jsoncpp whether it serialises null or not. With -1 it is clear the function is present, but we couldn't measure it.

Alternatively we could make it a string and then no truncation is required. Infinity is then either a string ("infinite" or "infinity") as well or a special value (such as -1?)

I'm not sure I understand the issue with serializing null. My idea behind null is that (of course depending on the type-safety of the language used after having parsed the json) you cannot really add null to a finite gas value, whereas adding -1 to a finite gas value yields an actual result.

On the other hand, gas values might be quite large - this is an estimation after all and we use bigints here. So perhaps decimal-encoded integers for finite values and the string infinite for unknown upper bounds would be the best solution here.

Ah I see we already do a range check here. Perhaps it is better to keep it here since users would probably not do it.

The problem I have is that null may or may not be serialised in the JSON hence we lose the information that a given function exists.

I am leaning towards using strings, that way both infinity and >53bits can be represented (though this is less of a real issue).

The jsonCompiler will use null for backwards compatibility, but the new JSON IO will have it as a string.

axic · 2017-04-10T13:54:47Z

libsolidity/interface/CompilerStack.cpp

+		}
+
+		if (contract.fallbackFunction())
+			externalFunctions[""] = gasToJson(GasEstimator::functionalEstimation(*items, ""));


The old code used INVALID as a signature here, though that is not needed, as an empty string will simply not trigger the calldata replacement code in functionalEstimation.

Please keep INVALID. The fallback function is called for an invalid signature. There is an optimization that jumps directly to the fallback if the input is empty, so I think it would take this optimization here and thus not calculate the upper bound correctly.

Added back with an explanation of the reason.

chriseth · 2017-04-10T14:18:33Z

libsolidity/interface/CompilerStack.cpp

+Json::Value gasToJson(GasEstimator::GasConsumption const& _gas)
+{
+    if (_gas.isInfinite || _gas.value > std::numeric_limits<Json::LargestUInt>::max())
+	return Json::Value(Json::nullValue);


Hm, I actually think null fits better here - we can still change the documentation, can't we?

chriseth · 2017-04-10T14:20:42Z

libsolidity/interface/CompilerStack.cpp

+		}
+
+		if (contract.fallbackFunction())
+			externalFunctions[""] = gasToJson(GasEstimator::functionalEstimation(*items, ""));


Please keep INVALID. The fallback function is called for an invalid signature. There is an optimization that jumps directly to the fallback if the input is empty, so I think it would take this optimization here and thus not calculate the upper bound correctly.

chriseth · 2017-04-10T14:23:24Z

libsolidity/interface/CompilerStack.cpp

+		Gas dataGas = bytecodeSize * eth::GasCosts::createDataGas;
+
+		Json::Value creation(Json::objectValue);
+		creation["dataCost"] = gasToJson(dataGas);


Perhaps it is better to name this codeDepositCost - we both were confused by it already ;-)

Will change, that matches the YP.

chriseth · 2017-04-12T07:16:41Z

solc/jsonCompiler.cpp

-	}
-	output["external"] = estimates["external"];
-	output["internal"] = estimates["internal"];
+	} else


else on the next line.

axic · 2017-04-12T11:15:44Z

@chriseth this version has the same output on the CLI and jsonCompiler as before, but internally (and in StandardCompiler) it represents the gas values as a string, including "infinite", to avoid information loss.

chriseth · 2017-04-12T14:27:49Z

libsolidity/interface/CompilerStack.cpp

+
+Json::Value gasToJson(GasEstimator::GasConsumption const& _gas)
+{
+    if (_gas.isInfinite)


Please use tabs instead of spaces.

chriseth · 2017-04-12T14:27:59Z

libsolidity/interface/CompilerStack.cpp

 	return ret;
 }
+
+namespace {


Brace to the next line.

chriseth · 2017-04-12T14:35:46Z

Made style changes myself, please merge if you are fine with it.

…ven if empty (backwards compat)

axic · 2017-04-13T01:19:00Z

@chriseth thanks I've folded it into the original commit if you don't mind

axic added the in progress label Apr 10, 2017

axic commented Apr 10, 2017

View reviewed changes

chriseth suggested changes Apr 10, 2017

View reviewed changes

axic force-pushed the compilerstack-gasestimate branch 3 times, most recently from d092cff to 243f1ba Compare April 11, 2017 09:00

axic mentioned this pull request Apr 11, 2017

Support "standardised" JSON compiler input/output #1639

Merged

13 tasks

chriseth reviewed Apr 12, 2017

View reviewed changes

axic force-pushed the compilerstack-gasestimate branch 4 times, most recently from bd7dd66 to d4f1f99 Compare April 12, 2017 11:11

chriseth reviewed Apr 12, 2017

View reviewed changes

libsolidity/interface/CompilerStack.cpp Outdated

return ret;

}

namespace {

Copy link

Contributor

chriseth Apr 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brace to the next line.

chriseth approved these changes Apr 12, 2017

View reviewed changes

axic added 6 commits April 13, 2017 02:17

Move gasEstimate into CompilerStack

e622110

Use new gasEstimate in jsonCompiler

d90fd43

Use new gasEstimate in CLI

328f2b0

Exclude fallback function from the internal functions in estimateGas

3fbb48b

The gasEstimates objects must always be present in the jsonCompiler e…

fe4fcca

…ven if empty (backwards compat)

Keep gas values as a string in CompilerStack::gasEstimate

54dcb0e

axic force-pushed the compilerstack-gasestimate branch from fac0285 to 54dcb0e Compare April 13, 2017 01:18

axic merged commit 138c952 into develop Apr 13, 2017

axic removed the in progress label Apr 13, 2017

axic deleted the compilerstack-gasestimate branch April 13, 2017 01:37

axic mentioned this pull request May 2, 2017

Rewrite jsonCompiler using StandardCompiler #2213

Merged

Conversation

axic commented Apr 10, 2017

Uh oh!

axic Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Apr 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Apr 12, 2017

Uh oh!

axic commented Apr 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

axic Apr 10, 2017 •

edited

Loading

axic Apr 10, 2017 •

edited

Loading