Improve readability in weight per gas calculation #2075
Conversation
|
I have contributed code to this repo, why does the CI still need a maintainer to approve 😕 |
tgmichel
added
A0-pleasereview
|
PR looks good to me but just to be sure I understand, this is a refactor of the original formula and the improvement comes from readability only? Or is there any other benefit? |
just for readability only, I think |
koushiro
changed the title
tgmichel
changed the title
|
We are in the process of reviewing #2072 , please if you don't mind waiting until it is merged to rebase. Thank you! |
|
@tgmichel PTAL again. |
|
@notlesh could you take a look this PR? |
| // subtract roughly the cost of a balance transfer from it (about 1/3 the cost) | ||
| // and some cost to account for per-byte-fee. | ||
| // TODO: we should use benchmarking's overhead feature to measure this | ||
| pub const EXTRINSIC_BASE_WEIGHT: Weight = Weight::from_ref_time(10000 * WEIGHT_PER_GAS); |
There was a problem hiding this comment.
Removing this constant means we have to use the 10_000 magic number elsewhere (e.g. in tests below). In addition, extracting this value out of BlockWeights isn't straightforward. I think we should leave it.
There was a problem hiding this comment.
added the extrinsic_base_weight function and moved it into moonbeam_runtime_common crate.
| /// Current approximation of the gas/s consumption considering | ||
| /// EVM execution over compiled WASM (on 4.4Ghz CPU). | ||
| /// Given the 500ms Weight, from which 75% only are used for transactions, | ||
| /// the total EVM execution gas limit is: GAS_PER_SECOND * 0.500 * 0.75 ~= 15_000_000. | ||
| pub const GAS_PER_SECOND: u64 = 40_000_000; | ||
|
|
||
| /// Approximate ratio of the amount of Weight per Gas. | ||
| /// u64 works for approximations because Weight is a very small unit compared to gas. | ||
| pub const WEIGHT_PER_GAS: u64 = WEIGHT_REF_TIME_PER_SECOND / GAS_PER_SECOND; |
There was a problem hiding this comment.
Similarly, I think these constants are valuable for readability.
There was a problem hiding this comment.
|
💢 |
What does it do?
Improve the calculation of weight per gas.
What important points reviewers should know?
Is there something left for follow-up PRs?
No
What alternative implementations were considered?
Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?
polkadot-evm/frontier#995
polkadot-evm/frontier#1000
What value does it bring to the blockchain users?