-
Notifications
You must be signed in to change notification settings - Fork 2.2k
EIP150 version1 gasPrice change proposal #3344
Conversation
@@ -543,6 +544,7 @@ void VM::interpretCases() | |||
|
|||
CASE_BEGIN(BALANCE) | |||
{ | |||
m_runGas += toUint64(m_schedule->balanceGas); |
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.
It previously was charged ~20 gas. Are you sure it will not be now 20 + balanceGas?
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.
I am not.
should we null it in gas prices list?
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.
You should change the schedule itself, not just add something to rungas here manually.
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.
Change it to how it is done for e.g. sha3, i.e. leave it like it is here and change the tier to specialTier.
@@ -84,6 +84,9 @@ StringHashMap Ethash::jsInfo(BlockHeader const& _bi) const | |||
|
|||
EVMSchedule const& Ethash::evmSchedule(EnvInfo const& _envInfo) const | |||
{ | |||
if (_envInfo.number() >= chainParams().u256Param("devconforkBlock")) |
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.
Please call it EIP150
@@ -543,6 +544,7 @@ void VM::interpretCases() | |||
|
|||
CASE_BEGIN(BALANCE) | |||
{ | |||
m_runGas += toUint64(m_schedule->balanceGas); |
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.
You should change the schedule itself, not just add something to rungas here manually.
BALANCE EXTCODECOPY EXTCODESIZE
@@ -84,6 +84,9 @@ StringHashMap Ethash::jsInfo(BlockHeader const& _bi) const | |||
|
|||
EVMSchedule const& Ethash::evmSchedule(EnvInfo const& _envInfo) const | |||
{ | |||
if (_envInfo.number() >= chainParams().u256Param("devconforkBlock")) | |||
return DevconSchedule; | |||
else |
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.
Style
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.
fixed with 7a39c97
@@ -159,6 +160,7 @@ R"E( | |||
"accountStartNonce": "0x00", | |||
"frontierCompatibilityModeLimit": "0x05", | |||
"daoHardforkBlock": "0x08", | |||
"devconforkBlock": "0x0a", |
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.
Is there enough space between 0x08
and 0x0a
to perform all necessary tests?
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.
i think it is. 08 block is just a daohardfork rules.
from 5 to 10 is homestead
}; | ||
|
||
static const EVMSchedule DefaultSchedule = EVMSchedule(); | ||
static EVMSchedule EVMScheduleDevcon() |
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.
Please call it EVMScheduleEIP150
static EVMSchedule EVMScheduleDevcon() | ||
{ | ||
//EIP150 | ||
EVMSchedule m_schedule = DefaultSchedule; |
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.
Shouldn't this be a modified HomesteadSchedule?
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.
it should. really nearly missed that out.
@@ -195,7 +195,7 @@ static const std::map<Instruction, InstructionInfo> c_instructionInfo = | |||
{ Instruction::SIGNEXTEND, { "SIGNEXTEND", 0, 2, 1, false, LowTier } }, | |||
{ Instruction::SHA3, { "SHA3", 0, 2, 1, false, SpecialTier } }, | |||
{ Instruction::ADDRESS, { "ADDRESS", 0, 0, 1, false, BaseTier } }, | |||
{ Instruction::BALANCE, { "BALANCE", 0, 1, 1, false, ExtTier } }, | |||
{ Instruction::BALANCE, { "BALANCE", 0, 1, 1, false, ZeroTier } }, |
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.
It should be SpecialTier (also the others)
} | ||
}, | ||
"pre" : { | ||
"d4fe7bc31cedb7bfb8a345f31e668033056b2728" : { "balance" : "1000000", "nonce" : "0", "code" : "", "storage": {} }, |
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.
Is this the attack contract? If yes, I don't think this is a good test. What we need to test here is gas prices, not resource usage.
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 a contract for dao transition. have to carry it with test for the transition rules
@@ -543,6 +544,7 @@ void VM::interpretCases() | |||
|
|||
CASE_BEGIN(BALANCE) | |||
{ | |||
m_runGas += toUint64(m_schedule->balanceGas); |
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.
Change it to how it is done for e.g. sha3, i.e. leave it like it is here and change the tier to specialTier.
Implements http://github.com/ethereum/EIPs/issues/150