[EC-302] Adding maximum gasLimit validation #301
Conversation
|
Could we add a unit test for this? |
| val gasLimitDiffLimit = parentHeader.gasLimit / GasLimitBoundDivisor | ||
| if(gasLimitDiff < gasLimitDiffLimit && blockHeader.gasLimit >= MinGasLimit) Right(blockHeader) | ||
| else Left(HeaderGasLimitError) | ||
| if(blockHeader.gasLimit > MaxGasLimit) |
| else { | ||
| val gasLimitDiff = (blockHeader.gasLimit - parentHeader.gasLimit).abs | ||
| val gasLimitDiffLimit = parentHeader.gasLimit / GasLimitBoundDivisor | ||
| if(gasLimitDiff < gasLimitDiffLimit && blockHeader.gasLimit >= MinGasLimit) Right(blockHeader) |
There was a problem hiding this comment.
It would be more concise to just extend this conditional with MaxGasLimit
There was a problem hiding this comment.
I did not want to calculate unnecessary gasLimitDiff and gasLimitDiffLimit, but this is probably such small thing that for me both versions are ok
|
Code LGTM! |
|
It is implemented definitely in geth and pytheruem. Also under ticket ( ethereum/EIPs#106 ) it seems that java and c++ client also have it. |
There was a problem hiding this comment.
Nico raises a very good point. This is about ETC vs ETH. The geth code you've referred to is for ETH. We need to find out whether ETC clients also implement this. For geth try this repo: https://github.com/ethereumproject/go-ethereum
(I'm not requesting changes, I'm requesting more investigation; can't seem to be able to dismiss own review anymore)
|
After research added a possibility to configure eip106 block number similar way to parity. If the |
| @@ -280,6 +281,8 @@ object BlockchainConfig { | |||
| } | |||
|
|
|||
| override val monetaryPolicyConfig = MonetaryPolicyConfig(blockchainConfig.getConfig("monetary-policy")) | |||
|
|
|||
| override val eip106BlockNumber: BigInt = Try(blockchainConfig.getString("eip106-block-number")).map(BigInt(_)).getOrElse(Long.MaxValue) | |||
There was a problem hiding this comment.
I'd rather avoid optional settings in the config. Can we make this required and use high values for disabling? Just like other clients do.
There was a problem hiding this comment.
Actually parity uses optional values https://github.com/paritytech/parity/blob/246b5282e546333c086b2e725de024fdfab0b8d9/json/src/spec/ethash.rs#L101, but ok that way we will be consistent with our other configuration values.
|
Please copy the new setting (commented out) with doc to |
29b92ea to
6739aea
Compare
…106_MaxGasLimitValidation
Validating that block header gas limit is lower than 2**63-1