-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: increase gas cost for submit proposal #9995
Conversation
Visit https://dashboard.github.orijtech.com?back=0&pr=9995&remote=false&repo=cosmos%2Fcosmos-sdk to see benchmark details. |
Codecov Report
@@ Coverage Diff @@
## master #9995 +/- ##
==========================================
- Coverage 63.67% 63.65% -0.02%
==========================================
Files 573 573
Lines 53807 53816 +9
==========================================
Hits 34259 34259
- Misses 17599 17608 +9
Partials 1949 1949
|
x/gov/keeper/msg_server.go
Outdated
return nil, err | ||
} | ||
|
||
ctx.BlockGasMeter().ConsumeGas(uint64(3*gasCostPerStorage*len(bytes)), "Submit proposal") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any thought on this gas value?
cc: @robert-zaremba
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did we come up with this value? Personally, i think a min self-deposit approach is better because we can then just tweak the param.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are automatically deleting it - so the motivation here is to pay for that operation. Deposit doesn't really pay for the operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@atheeshp could you link the issue next to the line to explain why we do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the initial deposit does @robert-zaremba
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deposit is returned. Here we want to consume a gas for the EndBlocker
operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think this is a clean approach, but I can't think of a more streamlined way atm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we want to consume a gas for the EndBlocker operations.
Shouldn't we then charge exactly what EndBlocker consumes? if I'm not mistaken, it would be gas_cost_per_READ * len(proposal_bytes * {the number of blocks where it gets read}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is what we are charging for: submit proposal, inactive queue operation, active queue operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pre approving. Please add a link (in a comment) to the issue in the line where we increase the gas consumption.
x/gov/keeper/msg_server.go
Outdated
return nil, err | ||
} | ||
|
||
ctx.BlockGasMeter().ConsumeGas(uint64(3*gasCostPerStorage*len(bytes)), "Submit proposal") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are automatically deleting it - so the motivation here is to pay for that operation. Deposit doesn't really pay for the operation.
x/gov/keeper/msg_server.go
Outdated
return nil, err | ||
} | ||
|
||
ctx.BlockGasMeter().ConsumeGas(uint64(3*gasCostPerStorage*len(bytes)), "Submit proposal") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@atheeshp could you link the issue next to the line to explain why we do it?
…sdk into atheesh/increase-gas-cost
This change is worth a changelog entry too |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but the charged gas seems arbitrary to me too. I'm proposing #9995 (comment)
Not blocking the PR because of this though.
@alexanderbez Are you okay with the current changes? If yes, we can merge this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
Description
Closes: #9683
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change