gas_limit for blocks, mined by Parity will be divisible by 37 #4154
Conversation
svyatonik
added
A0-pleasereview 🤓
svyatonik
changed the title
|
haven't checked the logic - might be worth pulling out the determinant logic into a separate function that takes min, max, target and determinant and returns the closest value |
|
I just wanted to decrease number of U256 operations, as in two first if branches there's no need to check that result is in [lower_limit; upper_limit]. I agree that separate function which will round-up result will be more convenient && understandable, but it will cost in extra-ops. So - if you insist, I'll make an extra func, if not - let it be as it is now. |
|
premature optimisation is the root of all evil :-) i don't think it's on a hotpath; feel free to benchmark the two approaches for block import but i expect any difference will not be significant. if it is, maybe do the range check inline and abstract the other clause out into a function with |
|
Added separate |
|
probably best to leave as is and respect them. |
gavofyork
added
A8-looksgood 🦄
| // Try to round gas_limit a bit so that: | ||
| // 1) it will still be in desired range | ||
| // 2) it will be a nearest (with tendency to increase) multiplier of PARITY_GAS_LIMIT_DETERMINANT | ||
| fn round_block_gas_limit(&self, gas_limit: U256, lower_limit: U256, upper_limit: U256) -> U256 { |
There was a problem hiding this comment.
nit: why take self? seems to be unused here.
|
Changes Unknown when pulling e7a142f on block_identification into ** on master**. |
| } | ||
| let total_lower_limit = max(lower_limit, gas_floor_target); | ||
| let total_upper_limit = min(upper_limit, gas_ceil_target); | ||
| let gas_limit = max(gas_floor_target, min(total_upper_limit, |
There was a problem hiding this comment.
I think it would make more sense to have max(total_upper_limit, .. (I know it wasn't like this in previous implementation, but still it would make sense)
There was a problem hiding this comment.
If I change this to max(total_upper_limit, .. it will always stick to the upper_limit:
max(upper_limit, lower_limit + something) = uper_limit
So this definitely changes the logic significantly and in strange way :)
Maybe you mean max(total_lower_limit, ... - I see that this is logical. However:
max(N, N + M), where N && M are U256 are always larger than N (let's omit overflows)
Previously it wasn't there, because there were no total_lower_limit && it is an extra-op, but now everything is known && I could easily change this, if this is what you meant. Just ping me
* gas_limit for new blocks will divide evenly by 13 * increased PARITY_GAS_LIMIT_DETERMINANT to 37 * separate method for marking mined block * debug_asserts(gas_limit within protocol range) * round_block_gas_limit method is now static * made round_block_gas_limit free-function * multiplier->multiple [ci skip]
* gas_limit for new blocks will divide evenly by 13 * increased PARITY_GAS_LIMIT_DETERMINANT to 37 * separate method for marking mined block * debug_asserts(gas_limit within protocol range) * round_block_gas_limit method is now static * made round_block_gas_limit free-function * multiplier->multiple [ci skip]
* gas_limit for new blocks will divide evenly by 13 * increased PARITY_GAS_LIMIT_DETERMINANT to 37 * separate method for marking mined block * debug_asserts(gas_limit within protocol range) * round_block_gas_limit method is now static * made round_block_gas_limit free-function * multiplier->multiple
…#4176) * gas_limit for new blocks will divide evenly by 13 * increased PARITY_GAS_LIMIT_DETERMINANT to 37 * separate method for marking mined block * debug_asserts(gas_limit within protocol range) * round_block_gas_limit method is now static * made round_block_gas_limit free-function * multiplier->multiple
* gas_limit for new blocks will divide evenly by 13 * increased PARITY_GAS_LIMIT_DETERMINANT to 37 * separate method for marking mined block * debug_asserts(gas_limit within protocol range) * round_block_gas_limit method is now static * made round_block_gas_limit free-function * multiplier->multiple
…#4179) * gas_limit for new blocks will divide evenly by 13 * increased PARITY_GAS_LIMIT_DETERMINANT to 37 * separate method for marking mined block * debug_asserts(gas_limit within protocol range) * round_block_gas_limit method is now static * made round_block_gas_limit free-function * multiplier->multiple
…#4176) * gas_limit for new blocks will divide evenly by 13 * increased PARITY_GAS_LIMIT_DETERMINANT to 37 * separate method for marking mined block * debug_asserts(gas_limit within protocol range) * round_block_gas_limit method is now static * made round_block_gas_limit free-function * multiplier renamed to multiple
closes [todo: backport then close] #4129