Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make mempool aware of MaxGas requirement #2360

Merged
merged 7 commits into from
Sep 12, 2018
Merged

Make mempool aware of MaxGas requirement #2360

merged 7 commits into from
Sep 12, 2018

Conversation

ValarDragon
Copy link
Contributor

@ValarDragon ValarDragon commented Sep 8, 2018 •
edited
Loading

Closes #2310

TODO: update spec

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant - no more instances of ReapMaxBytes AFAIK
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

@codecov-io
Copy link

codecov-io commented Sep 8, 2018 •
edited
Loading

Codecov Report

Merging #2360 into develop will increase coverage by 0.08%.
The diff coverage is 87.5%.

@@             Coverage Diff             @@
##           develop    #2360      +/-   ##
===========================================
+ Coverage       61%   61.09%   +0.08%     
===========================================
  Files          197      197              
  Lines        16294    16300       +6     
===========================================
+ Hits          9940     9958      +18     
+ Misses        5495     5482      -13     
- Partials       859      860       +1
Impacted Files Coverage Δ
state/services.go 38.46% <0%> (ø) ⬆️
abci/example/kvstore/kvstore.go 80.64% <0%> (ø) ⬆️
mempool/mempool.go 76.09% <100%> (+6.17%) ⬆️
consensus/state.go 77.19% <100%> (+0.02%) ⬆️
rpc/grpc/client_server.go 71.42% <0%> (-13.19%) ⬇️
p2p/peer.go 57.95% <0%> (-1.14%) ⬇️
consensus/reactor.go 73.55% <0%> (-0.81%) ⬇️
p2p/pex/addrbook.go 70.07% <0%> (+0.48%) ⬆️
blockchain/store.go 95.04% <0%> (+0.55%) ⬆️
p2p/pex/pex_reactor.go 74.66% <0%> (+0.66%) ⬆️
... and 3 more

melekes
melekes approved these changes Sep 10, 2018
View reviewed changes
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇 🏎 🍰

@melekes
Copy link
Contributor

melekes commented Sep 10, 2018

@melekes
Copy link
Contributor

melekes commented Sep 10, 2018

We should add this check:

Each DeliverTx returns an amount GasUsed. The total GasUsed by the block must be less than the BlockSize.MaxGas consensus param.

Also, would be great to see more tests.

@ValarDragon
Copy link
Contributor Author

Agreed for adding more tests.

Per the issue, I thought we put it on the app to require that gas used is less than gas wanted. We don't run through the block until h+1 so if we decided that the gas used was actually too high, what would we do? Just drop the last txs?

@ValarDragon
Copy link
Contributor Author

Added all tests I could think of. I set GasWanted=1 within kvstore's CheckTx to enable this. This seems reasonable to me, but if unreasonable, I can make a new proxy app which does something for GasWanted. This tests all cases which I could think of as valuable to test.

melekes
melekes approved these changes Sep 11, 2018
View reviewed changes
mempool/mempool_test.go Outdated
@@ -71,6 +71,54 @@ func checkTxs(t *testing.T, mempool *Mempool, count int) types.Txs {
return txs
}

func TestReapMaxGasMaxBytes(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestReapMaxBytesMaxGas

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@ValarDragon
Copy link
Contributor Author

Hrmm, looks like switching the gas from kv store to persistent kv store caused things to break. Not really sure why, since persistent kv store just wraps kv store. I think I'll switch it back to kv store regardless, since it feels kinda strange only the variant that writes to disk does something for gasWanted.

@ValarDragon
Copy link
Contributor Author

Oh actually this doesn't update the filter, to ensure no tx is greater than max gas. I think that should be done in a second PR though.

@ebuchman ebuchman mentioned this pull request Sep 12, 2018
Closed
@ebuchman ebuchman merged commit 1ea64fc into develop Sep 12, 2018
@ebuchman ebuchman deleted the dev/max_gas branch September 12, 2018 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melekes melekes melekes approved these changes

@ebuchman ebuchman Awaiting requested review from ebuchman

@xla xla Awaiting requested review from xla

@zramsay zramsay Awaiting requested review from zramsay

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ValarDragon @codecov-io @melekes @ebuchman