Reestimate deterministic gas #646
Conversation
wojtek-coreum
requested review from
dzmitryhil,
vertex451,
miladz68 and
ysv
and removed request for
a team
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @miladz68, @vertex451, and @ysv)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @vertex451, @wojtek-coreum, and @ysv)
x/deterministicgas/config.go line 147 at r1 (raw file):
[]sdk.Msg{ // auth &authtypes.MsgUpdateParams{}, // This is non-deterministic because all the goc proposals are non-deterministic anyway
replace all goc with gov in the comments.
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, @vertex451, and @ysv)
x/deterministicgas/config.go line 147 at r1 (raw file):
Previously, miladz68 (milad) wrote…
replace all
gocwithgovin the comments.
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @vertex451, @wojtek-coreum, and @ysv)
x/deterministicgas/spec/README.md line 74 at r4 (raw file):
| `/cosmos.bank.v1beta1.MsgMultiSend` | [special case](#special-cases) | | `/cosmos.bank.v1beta1.MsgSend` | [special case](#special-cases) | | `/coreum.asset.ft.v1.MsgBurn` | 34960 |
I think round thiese numbers to the closest factor of 1000 .
There was a problem hiding this comment.
Reviewed 1 of 2 files at r3, 1 of 1 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @miladz68, @vertex451, and @wojtek-coreum)
integration-tests/modules/bank_test.go line 820 at r5 (raw file):
// Spreadsheet example might be found here: https://docs.google.com/spreadsheets/d/1qoKa8udTPYdS_-ofJ8xNbnZFDh-gqGb4n0_OgcTHUOA/edit?usp=sharing // Keep in mind that to estimate the gas you need to move bank send message to nondeterministic section inside deterministic gas config. func TestBankSendEstimation(t *testing.T) {
I don't think it makes sense to run this test under normal circumstances in our CI & integration tests
Maybe it makes sense to put it into different file and use another //go:build tag for it ?
x/deterministicgas/config.go line 123 at r5 (raw file):
MsgToMsgURL(&govtypesv1.MsgVote{}): constantGasFunc(7000 * 0.88), MsgToMsgURL(&govtypesv1.MsgVoteWeighted{}): constantGasFunc(9000 * 0.72), // FIXME (v47-deterministic): We must add integration test executing this message to have data to analyze
will you do close all these todos in scope of this task ? (should be separate PR from this one I think)
x/deterministicgas/config.go line 155 at r5 (raw file):
[]sdk.Msg{ // auth &authtypes.MsgUpdateParams{}, // This is non-deterministic because all the gov proposals are non-deterministic anyway
nit: one comment on top is enought for all of them instead of adding this // This is non-deterministic because all the gov proposals are non-deterministic anyway to each msg
I think it makes sene to group them
x/deterministicgas/README.md line 6 at r5 (raw file):
To do it: 1. Go to our Grafana dashboard
but first you run integration tests ?
Or you use production data ?
x/deterministicgas/README.md line 8 at r5 (raw file):
1. Go to our Grafana dashboard 2. Take values for deterministic gas factors reported there 3. Recalculate the deterministic gas by multiplying it by the minimum value taken from the metric.
so you mean that you take min for a specific msg and multiply it by current value present in deterministic gas ?
x/deterministicgas/spec/README.md line 74 at r4 (raw file):
Previously, miladz68 (milad) wrote…
I think round thiese numbers to the closest factor of 1000 .
I agree
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @miladz68, @vertex451, and @ysv)
x/deterministicgas/config.go line 123 at r5 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
will you do close all these todos in scope of this task ? (should be separate PR from this one I think)
No, they will be a separate PR
x/deterministicgas/config.go line 155 at r5 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
nit: one comment on top is enought for all of them instead of adding this
// This is non-deterministic because all the gov proposals are non-deterministic anywayto each msgI think it makes sene to group them
I disagree, not all those messages are named like MsgUpdateParams, so it's not necessarily obvious that they are executed by gov only
x/deterministicgas/README.md line 8 at r5 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
so you mean that you take min for a specific msg and multiply it by current value present in deterministic gas ?
yes
There was a problem hiding this comment.
Reviewable status: 3 of 4 files reviewed, 6 unresolved discussions (waiting on @miladz68, @vertex451, and @ysv)
x/deterministicgas/README.md line 6 at r5 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
but first you run integration tests ?
Or you use production data ?
clarified
There was a problem hiding this comment.
Reviewed 1 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @vertex451 and @ysv)
There was a problem hiding this comment.
Reviewable status: 2 of 4 files reviewed, 6 unresolved discussions (waiting on @miladz68, @vertex451, and @ysv)
x/deterministicgas/spec/README.md line 74 at r4 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
I agree
Done.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r8, 1 of 1 files at r9, 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @vertex451 and @wojtek-coreum)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @vertex451, and @wojtek-coreum)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @vertex451, and @wojtek-coreum)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @vertex451, @wojtek-coreum, and @ysv)
x/deterministicgas/spec/README.md line 35 at r8 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
Lets discuss after daily
I don't think we have another option except increasing it but I want to make sure we don't brake everything
OK, let's discuss.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @vertex451, @wojtek-coreum, and @ysv)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @vertex451 and @wojtek-coreum)
# Conflicts: # integration-tests/modules/authz_test.go
There was a problem hiding this comment.
Reviewed 5 of 5 files at r12, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @vertex451 and @wojtek-coreum)
There was a problem hiding this comment.
Reviewable status: 8 of 10 files reviewed, 1 unresolved discussion (waiting on @vertex451 and @ysv)
integration-tests/modules/bank_test.go line 820 at r5 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
I don't think it makes sense to run this test under normal circumstances in our CI & integration tests
Maybe it makes sense to put it into different file and use another
//go:buildtag for it ?
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r13, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewed 4 of 5 files at r12, 2 of 2 files at r13, 2 of 2 files at r14, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewed 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @vertex451 and @wojtek-coreum)
x/deterministicgas/spec/README.md line 105 at r15 (raw file):
| `/cosmos.gov.v1beta1.MsgVote` | 6000 | | `/cosmos.gov.v1beta1.MsgVoteWeighted` | 9000 | | `/cosmos.nft.v1beta1.MsgSend` | 25000 |
cosmos and coreum nft send should be identical I guess. why do we have 25k and 16k for them which are different ?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68 and @vertex451)
x/deterministicgas/spec/README.md line 105 at r15 (raw file):
Previously, miladz68 (milad) wrote…
cosmos and coreum nft send should be identical I guess. why do we have 25k and 16k for them which are different ?
ah, yes, because this PR updated the values for old module but you introduced new module in master.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r16, 1 of 1 files at r17, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewed 2 of 2 files at r16, 1 of 1 files at r17, all commit messages.
Reviewable status:
Description
Here are the metrics after reestimating:
Some transactions can't be estimated because have no integration tests using them, we should add:
Also we should discuss how we approach these messages:
Reviewers checklist:
Authors checklist
This change is