-
Notifications
You must be signed in to change notification settings - Fork 50
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
Clean up the handling of gas (use int64_t only) #332
Conversation
20440d1
to
afab377
Compare
@chfast I think this should be correct now (tests are failing though due to the edge cases I guess?). Can you have a look at the logic in this PR? |
93cdff3
to
1287376
Compare
Codecov Report
@@ Coverage Diff @@
## master #332 +/- ##
==========================================
+ Coverage 67.28% 67.31% +0.03%
==========================================
Files 5 5
Lines 868 875 +7
Branches 123 123
==========================================
+ Hits 584 589 +5
- Misses 255 257 +2
Partials 29 29 |
src/eei.cpp
Outdated
@@ -485,7 +487,7 @@ namespace hera { | |||
static_assert(GasSchedule::logTopic <= 65536, "Gas cost of logTopic could lead to overflow"); | |||
static_assert(GasSchedule::logData <= 65536, "Gas cost of logData could lead to overflow"); | |||
// Using uint64_t to force a type issue if the underlying API changes. | |||
takeInterfaceGas(GasSchedule::log + (GasSchedule::logTopic * numberOfTopics) + (GasSchedule::logData * static_cast<uint64_t>(length))); | |||
takeInterfaceGas(GasSchedule::log + (GasSchedule::logTopic * numberOfTopics) + (GasSchedule::logData * static_cast<int64_t>(length))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is safe cast, so you can use int64_t(length)
.
src/eei.cpp
Outdated
@@ -1012,7 +1018,7 @@ namespace hera { | |||
// Allow 16 bits here. | |||
static_assert(GasSchedule::copy <= 65536, "Gas cost of copy could lead to overflow"); | |||
// Using uint64_t to force a type issue if the underlying API changes. | |||
takeInterfaceGas(GasSchedule::copy * ((static_cast<uint64_t>(length) + 31) / 32)); | |||
takeInterfaceGas(GasSchedule::copy * ((static_cast<int64_t>(length) + 31) / 32)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cast is safe, could use int64_t(length)
.
@@ -1001,7 +1007,7 @@ namespace hera { | |||
/* | |||
* Utilities | |||
*/ | |||
void EthereumInterface::safeChargeDataCopy(uint32_t length, uint32_t baseCost) { | |||
void EthereumInterface::safeChargeDataCopy(uint32_t length, unsigned baseCost) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have you changed this?
From my little experience I recommend using int
or int64_t
for cost constants as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constants are currently defined as unsigned
, wanted to ensure it is the same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving conditionally, but waiting for two int64_t(length)
changes.
Part of #311.