Update gas for storage occupation #3510
Conversation
… integration tests
grarco
added
gas
breaking:consensus
|
Let's wait for the CI to come back online before reviewing this cause there's a change we might have a few tests failing due to gas |
brentstone
force-pushed
the
grarco/adjust-gas-for-payload
branch
from
569d43e to
96d685d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3510 +/- ##
==========================================
- Coverage 53.48% 53.46% -0.02%
==========================================
Files 320 320
Lines 110000 109964 -36
==========================================
- Hits 58832 58792 -40
- Misses 51168 51172 +4 ☔ View full report in Codecov by Sentry. |
Ok I've fixed the broken tests, this PR is now ready for review |
| // free-floating but it's tied to the other costs | ||
| const STORAGE_OCCUPATION_GAS_PER_BYTE: u64 = | ||
| 100_000 + PHYSICAL_STORAGE_LATENCY_PER_BYTE; | ||
| PHYSICAL_STORAGE_LATENCY_PER_BYTE * (1 + 10); |
There was a problem hiding this comment.
why did you write the argument as (1 + 10)? Do each of these numbers account for some different effect?
There was a problem hiding this comment.
They do: the 1 represent the actual cost of storage latency, the 10 component instead is just an arbitrary multiplier that I use to compute an actual storage occupation gas cost based on the storage latency. They are not really tied but we cannot leave the storage occupation gas cost free-floating because we'd end up with the same issue this Pr tries to solve. So we need a way to tie that cost to some other gas cost that is actually based on a benchmark and the storage latency is the closest one
There was a problem hiding this comment.
cool would you mind describing this a bit in a comment / docstring?
There was a problem hiding this comment.
It's described right above
There was a problem hiding this comment.
Ok I think I was having trouble understanding the comment at first and was seeking clarity, but I think it's fine now. Thanks.
brentstone
left a comment
brentstone
left a comment
There was a problem hiding this comment.
LGTM, modulo response to the (1 + 10) comment
* grarco/adjust-gas-for-payload: Removes redundant `new_from_sub_limit` for `TxGasMeter` Increases gas limit for wasm tests Increases gas limit for unit tests Changelog #3510 Increases gas cost for storage and binds it to the other costs
Describe your changes
Increases the gas cost component for storage occupation and ties this cost the other ones so that it's not free-floating anymore (which could cause a divergence if the other costs are changed).
Removes the redundant
new_from_sub_limitconstructor forTxGasMeter.Indicate on which release or other PRs this topic is based on
#3428 (diff for review: https://github.com/anoma/namada/pull/3510/files/209da0c6ad9cd456a5a6634ec25cc45408e0b72c..33a6cc9437692ccb1ef24e58f2536ac2dfdeb8f3)
Checklist before merging to
draft