yield_resume: fix resume per-byte cost #12192
Conversation
nagisa
requested review from
Longarithm and
saketh-are
and removed request for
Longarithm
nagisa
force-pushed
the
fix-yield-resume-fees
branch
from
9602268 to
b026913
Compare
| @@ -390,7 +390,7 @@ impl ExtCosts { | |||
| ExtCosts::yield_create_base => Parameter::WasmYieldCreateBase, | |||
| ExtCosts::yield_create_byte => Parameter::WasmYieldCreateByte, | |||
| ExtCosts::yield_resume_base => Parameter::WasmYieldResumeBase, | |||
| ExtCosts::yield_resume_byte => Parameter::WasmYieldResumeBase, | |||
| @@ -147,7 +147,7 @@ wasm_alt_bn128_g1_sum_element 5_000_000_000 | |||
| wasm_yield_create_base 153_411_779_276 | |||
| wasm_yield_create_byte 15_643_988 | |||
| wasm_yield_resume_base 1_195_627_285_210 | |||
| wasm_yield_resume_byte 47_683_715 | |||
| wasm_yield_resume_byte 1_195_627_285_210 | |||
There was a problem hiding this comment.
Is this right? Perhaps I don't understand what parameters.snap is supposed to do.
There was a problem hiding this comment.
So, this is a snapshot of the parameters that we have at the current stable protocol version. I hadn't updated the protocol version in the previous revision, but I have done so now so the snapshot has been updated.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #12192 +/- ##
==========================================
+ Coverage 71.57% 71.60% +0.02%
==========================================
Files 821 824 +3
Lines 165303 165513 +210
Branches 165303 165513 +210
==========================================
+ Hits 118314 118510 +196
- Misses 41852 41881 +29
+ Partials 5137 5122 -15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This was intended to match the cost of the per-byte of function call payloads, but an unfortunate two-letter typo led to it becoming the same as the base cost instead. This change requires a protocol version bump (to 73) but otherwise is fairly straightforward in that I just set the cost of the fee to the incorrect value back in 67 and corrected it in code and protocol version 73.
nagisa
force-pushed
the
fix-yield-resume-fees
branch
from
b026913 to
e304c94
Compare
| @@ -2,4 +2,4 @@ yield_resume: { old: false, new: true } | |||
| wasm_yield_create_base: { old: 300_000_000_000_000, new: 153_411_779_276 } | |||
| wasm_yield_create_byte: { old: 300_000_000_000_000, new: 15_643_988 } | |||
| wasm_yield_resume_base: { old: 300_000_000_000_000, new: 1_195_627_285_210 } | |||
| wasm_yield_resume_byte: { old: 300_000_000_000_000, new: 17_212_011 } | |||
| wasm_yield_resume_byte: { old: 300_000_000_000_000, new: 1_195_627_285_210 } | |||
There was a problem hiding this comment.
Why do you modify an old config? Even if unused it seems like a bad practice.
There was a problem hiding this comment.
IIUC we were processing this config file the wrong way because of the bug in core/parameters/src/cost.rs. Now that we process it correctly we need to change the old file to maintain correct replayability.
There was a problem hiding this comment.
As described. The fee for byte between 67 and 73 was factually 1.195Tg and this change reflects the reality. I could have added a new parameter altogether, but that would have required a change in 67 anyway (though it would have been entirely additive.)
There was a problem hiding this comment.
omg, of course, thanks for clarifying
This was intended to match the cost of the per-byte of function call payloads, but an unfortunate two-letter typo led to it becoming the same as the base cost instead.
This change requires a protocol version bump (to 73) but otherwise is fairly straightforward in that I just set the cost of the fee to the incorrect value back in 67 and corrected it in code and protocol version 73.