Charge gas for EEI methods #86

jakelang · 2018-01-16T15:16:36Z

Fixes #74.

axic · 2018-01-16T16:41:14Z

src/eei.h

@@ -108,8 +108,18 @@ struct EthereumInterface : ShellExternalInterface {
 };

 struct GasSchedule {
+  static constexpr unsigned sLoad = 50;


Should be called storageLoad

axic · 2018-01-16T16:57:50Z

src/eei.h

+  static constexpr unsigned create = 32000;
+  static constexpr unsigned call = 40;
+  static constexpr unsigned memory = 3;
+  static constexpr unsigned copy = 3;


What are these two used for?

They are used in codecopy and calldatacopy, according to outdated yellowpaper semantics. Will fix this

Can you name perhaps have codeMemory, codeCopy and callDataMemory, callDataCopy?

yeah, I was using the fee schedule names from EVMJIT and the yellowpaper but this makes more sense i guess.

EDIT: It does not make sense to do this because gas costs are tiered. going to use EVMJIT style naming.

chfast

In general, you subtract gas before execution an instruction.

chfast · 2018-01-18T12:26:18Z

src/eei.cpp

@@ -209,6 +221,8 @@ namespace HeraVM {
      vector<uint8_t> input(msg.input, msg.input + msg.input_size);
      storeMemory(input, dataOffset, resultOffset, length);

+      takeGas(GasSchedule::stepGas2 + GasSchedule::copy * (length / 32));


@axic What do you think about charging for memory per byte, instead of per "word"?
@jakelang I believe length / 32 should be rounded up, not down.

chfast · 2018-01-18T12:26:36Z

src/eei.cpp

@@ -241,12 +259,16 @@ namespace HeraVM {

      storeMemory(code, codeOffset, resultOffset, length);

+      takeGas(GasSchedule::stepGas2 + GasSchedule::copy * (length / 32));


chfast · 2018-01-18T12:27:53Z

src/eei.cpp

@@ -347,6 +382,8 @@ namespace HeraVM {

      context->fn_table->log(context, &msg.address, data.data(), length, topics, numberOfTopics);

+      takeGas(GasSchedule::log + (length * GasSchedule::logData) + (GasSchedule::logTopic * numberOfTopics));


How do you prevent overflows?

I don't see a solution to this that isn't messy.
Maybe a helper function would be better for this.

chfast · 2018-01-18T12:32:27Z

src/eei.cpp

+      if (import->base == Name("call") && !context->fn_table->account_exists(context, &call_message.address))
+        takeGas(GasSchedule::callnewacct);
+
+      takeGas(GasSchedule::call);


This should go first, before other checks.

Also, the gas should be taken before the actual call.

You also have to take gas of the gas argument, and put back the amount left after the call.

chfast · 2018-01-18T12:32:53Z

src/eei.cpp

@@ -601,6 +661,9 @@ namespace HeraVM {
      if (create_result.release)
        create_result.release(&create_result);

+      takeGas(GasSchedule::create);


Must go before the CREATE.

chfast · 2018-01-18T12:34:08Z

src/eei.h

+  static constexpr unsigned stepGas4 = 8;
+  static constexpr unsigned stepGas5 = 10;
+  static constexpr unsigned stepGas6 = 20;
+  static constexpr unsigned stepGas7 = 0;


This was here:
https://github.com/ethereum/evmjit/blob/develop/libevmjit/JIT.h#L71

It is unneeded, so I am getting rid of it

axic · 2018-01-18T17:25:48Z

@chfast do you want to review the costs and work on this to finish it?

jakelang · 2018-01-22T01:03:02Z

@chfast I have made appropriate changes. I am also considering writing helpers for arithmetic overflow checking in gas charges.

axic · 2018-01-22T01:10:35Z

src/eei.cpp

@@ -403,6 +438,7 @@ namespace HeraVM {
        GasSchedule::storageStoreChange
      );

+      context->fn_table->get_storage(&current, context, &msg.address, &path);


This must happen before takeGas otherwise it will charge invalidly.

axic · 2018-01-22T01:12:54Z

src/eei.cpp

@@ -550,6 +597,9 @@ namespace HeraVM {
      if (call_result.release)
        call_result.release(&call_result);

+      /* Return unspent gas */
+      result.gasLeft += call_result.gas_left;


For this to work, the entire passed gas must be used via takeGas before doing an EVM-C call.

This is done by just performing:
takeGas(gas);
before the call execution, correct?

axic · 2018-01-24T23:29:45Z

Please rebase, it is in conflict.

axic · 2018-01-24T23:31:15Z

src/eei.cpp

@@ -470,20 +516,24 @@ namespace HeraVM {
      uint32_t valueOffset;
      uint32_t dataOffset;
      uint32_t dataLength;
+      uint32_t resultOffset;
+      uint32_t resultLength;


These shouldn't need to be added back.

axic · 2018-01-24T23:31:22Z

src/eei.cpp


      heraAssert((msg.flags & ~EVM_STATIC) == 0, "Unknown flags not supported.");

      evm_message call_message;
      call_message.address = loadUint160(addressOffset);
      call_message.flags = msg.flags;
      call_message.code_hash = {};
-      call_message.gas = gas - (gas / 64);
+      call_message.gas = gas;


Please don't remove this.

axic · 2018-01-24T23:33:01Z

src/eei.h

+  static constexpr unsigned blockhash = 800;
+  static constexpr unsigned balance = 400;
+  static constexpr unsigned stepGas0 = 0;
+  static constexpr unsigned stepGas1 = 2;


I think I'd prefer having named gas fields for each of the interface method as opposed to YellowPaper-esque stepGas*

any recommendations for a naming scheme?

Probably the name of the interface method?

Lots of these are used for multiple interface methods.

I'd suggest to with the YP in that case: Appendix G of https://ethereum.github.io/yellowpaper/paper.pdf

chfast · 2018-01-26T13:42:17Z

src/eei.cpp

+
+      takeGas(create_message.gas);
+      takeGas(GasSchedule::create);
+      takeGas(GasSchedule::callnewacct);


This is not needed here. However, changing GasSchedule::create to be equal GasSchedule::call + GasSchedule::callnewacct would make sense, @axic ?

chfast · 2018-01-26T13:44:34Z

src/eei.cpp

@@ -598,6 +661,9 @@ namespace HeraVM {
      if (create_result.release)
        create_result.release(&create_result);

+      /* Return unspent gas */
+      result.gasLeft += create_result.gas_left;


I'm not sure this is correct. This was not subtruck for CREATE case.

axic · 2018-02-04T03:32:34Z

src/eei.cpp

@@ -516,7 +564,7 @@ namespace HeraVM {
        addressOffset << " " <<
        valueOffset << " " <<
        dataOffset << " " <<
-        dataLength << dec << "\n";
+        dataLength << " " << dec << "\n";


I think this is not needed. dec as misleading it is (perhaps should used explicit std::dec here), is switching to decimal display mode for numbers

This change is leftover from an old version of Hera. Will rebase against new master

axic · 2018-02-04T03:35:01Z

src/eei.cpp

+        takeGas(GasSchedule::callnewacct);
+      if (!isZeroUint256(call_message.value))
+        takeGas(GasSchedule::valuetransfer);
+      takeGas(gas);


I think this actually should use call_message.gas because the all-but-1/64th rule applies to this too.

axic · 2018-02-04T03:35:30Z

src/eei.h

+  static constexpr unsigned copy = 3;
+  static constexpr unsigned blockhash = 800;
+  static constexpr unsigned balance = 400;
+  static constexpr unsigned stepGas1 = 2;


Can you please use the names from the Yellowpapyer? veryLow, low, etc.

Some of the gas charges in the are unused in EEI. Should they be retained in the gas schedule for the sake of yellowpaper compliance and potential later use, or trimmed off?

Just trimmed off.

axic · 2018-02-04T03:36:39Z

src/eei.cpp

@@ -239,6 +254,9 @@ namespace HeraVM {

      HERA_DEBUG << "codeCopy " << hex << resultOffset << " " << codeOffset << " " << length << dec << "\n";

+      heraAssert(ffs(GasSchedule::copy) + (ffs(length) - 5) <= 64, "Gas charge overflow");
+      heraAssert(numeric_limits<uint64_t>::max() - GasSchedule::stepGas2 >= GasSchedule::copy * (length / 32), "Gas charge overflow");
+      takeGas((uint64_t)ceil(GasSchedule::stepGas2 + GasSchedule::copy * (length / 32)));


This one uses ceil, but the one in externalCodeCopy doesn't. Any reason for that? Also shouldn't this instead just use (length + 31) / 32 to avoid the use of ceil?

It also may make sense creating a helper along the lines of takeWordCopyGas(baseCost, costPerWord, length)

I will implement helpers in a separate PR if there is a significant need for it.

axic · 2018-02-04T03:38:31Z

src/eei.cpp

@@ -206,6 +218,7 @@ namespace HeraVM {

      HERA_DEBUG << "callDataCopy " << hex << resultOffset << " " << dataOffset << " " << length << dec << "\n";

+      takeGas(GasSchedule::stepGas2 + GasSchedule::copy * (length / 32));


The rules applied in codeCopy and externalCodeCopy are not really followed here?

axic · 2018-02-07T00:12:33Z

src/eei.cpp

@@ -262,8 +285,10 @@ namespace HeraVM {
      const uint8_t *code;
      size_t code_size = context->fn_table->get_code(&code, context, &address);

+      heraAssert(ffs(GasSchedule::copy) + (ffs(length) - 5) <= 64, "Gas charge overflow");
+      heraAssert(numeric_limits<uint64_t>::max() - GasSchedule::extcode >= GasSchedule::copy * (length / 32), "Gas charge overflow");


The assertions don't use the same rounding for length?

axic · 2018-02-14T19:11:52Z

@jakelang I hoped that @chfast could review the values but I'll do it tonight so we can stop dragging this PR forever. Will just fix issues as we go along.

axic · 2018-02-15T00:50:46Z

src/eei.cpp

@@ -206,6 +218,10 @@ namespace HeraVM {

      HERA_DEBUG << "callDataCopy " << hex << resultOffset << " " << dataOffset << " " << length << dec << "\n";

+      heraAssert(ffs(GasSchedule::copy) + (ffs(length) - 5) <= 64, "Gas charge overflow");
+      heraAssert(numeric_limits<uint64_t>::max() - GasSchedule::verylow >= GasSchedule::copy * ((static_cast<uint64_t>(length) + 31) / 32), "Gas charge overflow");
+      takeGas(GasSchedule::verylow + GasSchedule::copy * ((static_cast<uint64_t>(length) + 31) / 32));


I really think we should have a helper for this since there 3 copies now.

Could open an issue for this and do it in a separate PR, possibly a refactoring pass.
Might as well get it working now.

chfast

Looks good, just one C++ suggestion for future: if you widen a type e.g. from size_t to uint64_t, you can just do uint64_t(length). It is always safe. static_cast suggests you are discarding some information.

Use YellowPaper-style gas fee names

axic added the in progress label Jan 16, 2018

jakelang mentioned this pull request Jan 16, 2018

Charge gas for each interface method properly #74

Closed

jakelang force-pushed the eei-gas-charge branch from 504054e to 13f69a5 Compare January 16, 2018 15:36

jakelang requested a review from axic January 16, 2018 15:51

axic reviewed Jan 16, 2018

View reviewed changes

jakelang force-pushed the eei-gas-charge branch 7 times, most recently from de060bf to 6cc69b2 Compare January 17, 2018 22:06

axic changed the title ~~[WIP] Charge gas for EEI methods~~ Charge gas for EEI methods Jan 18, 2018

axic force-pushed the eei-gas-charge branch from 6cc69b2 to 4852472 Compare January 18, 2018 02:46

axic requested a review from chfast January 18, 2018 02:46

chfast requested changes Jan 18, 2018

View reviewed changes

jakelang force-pushed the eei-gas-charge branch from 4852472 to 77fd2f4 Compare January 22, 2018 00:36

axic reviewed Jan 22, 2018

View reviewed changes

jakelang force-pushed the eei-gas-charge branch 5 times, most recently from 7f6b298 to 0f983f0 Compare January 24, 2018 23:28

axic reviewed Jan 24, 2018

View reviewed changes

jakelang force-pushed the eei-gas-charge branch 5 times, most recently from c94412a to 6d3f9ad Compare January 25, 2018 00:52

chfast reviewed Jan 26, 2018

View reviewed changes

jakelang force-pushed the eei-gas-charge branch 2 times, most recently from 05209c1 to 848ec17 Compare January 30, 2018 22:23

axic reviewed Feb 4, 2018

View reviewed changes

jakelang force-pushed the eei-gas-charge branch 2 times, most recently from 8663aee to 026de4b Compare February 4, 2018 20:41

axic reviewed Feb 7, 2018

View reviewed changes

jakelang force-pushed the eei-gas-charge branch from 41630c6 to ae62b74 Compare February 7, 2018 00:46

jakelang force-pushed the eei-gas-charge branch from 6eb808c to a7229de Compare February 14, 2018 01:52

jakelang force-pushed the eei-gas-charge branch from a7229de to ce54bad Compare February 15, 2018 00:39

axic reviewed Feb 15, 2018

View reviewed changes

chfast approved these changes Feb 20, 2018

View reviewed changes

jakelang force-pushed the eei-gas-charge branch 2 times, most recently from 5c9bf12 to 13145fb Compare February 21, 2018 19:37

axic force-pushed the eei-gas-charge branch from 13145fb to cfc6518 Compare March 5, 2018 21:37

EEI: gas charges

e2a6838

Use YellowPaper-style gas fee names

axic force-pushed the eei-gas-charge branch from cfc6518 to e2a6838 Compare March 5, 2018 21:43

axic merged commit cc11a88 into master Mar 5, 2018

axic deleted the eei-gas-charge branch March 5, 2018 22:17

		@@ -241,12 +259,16 @@ namespace HeraVM {

		storeMemory(code, codeOffset, resultOffset, length);

		takeGas(GasSchedule::stepGas2 + GasSchedule::copy * (length / 32));

		@@ -347,6 +382,8 @@ namespace HeraVM {

		context->fn_table->log(context, &msg.address, data.data(), length, topics, numberOfTopics);

		takeGas(GasSchedule::log + (length * GasSchedule::logData) + (GasSchedule::logTopic * numberOfTopics));

		@@ -206,6 +218,7 @@ namespace HeraVM {

		HERA_DEBUG << "callDataCopy " << hex << resultOffset << " " << dataOffset << " " << length << dec << "\n";

		takeGas(GasSchedule::stepGas2 + GasSchedule::copy * (length / 32));

Charge gas for EEI methods #86

Charge gas for EEI methods #86

Conversation

jakelang commented Jan 16, 2018 • edited by axic Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelang Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

chfast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Jan 18, 2018

jakelang commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelang Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelang Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast left a comment

Choose a reason for hiding this comment

jakelang commented Jan 16, 2018 •

edited by axic

Loading

jakelang Jan 17, 2018 •

edited

Loading

jakelang Feb 4, 2018 •

edited

Loading

jakelang Feb 4, 2018 •

edited

Loading