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.

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

chore: update deafult mempool size to match ttl and block size #2681

Merged
merged 2 commits into from
Oct 16, 2023

Conversation

evan-forbes
Copy link
Member

Overview

The default mempool size should match that of the default TTL and block size.

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@evan-forbes evan-forbes self-assigned this Oct 16, 2023
@celestia-bot celestia-bot requested a review from a team October 16, 2023 01:19
@codecov-commenter
Copy link

Codecov Report

Merging #2681 (ac1f9b8) into main (40385b9) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2681      +/-   ##
==========================================
- Coverage   21.11%   21.11%   -0.01%     
==========================================
  Files         138      138              
  Lines       15767    15768       +1     
==========================================
  Hits         3329     3329              
- Misses      12115    12116       +1     
  Partials      323      323              
Files Coverage Δ
app/default_overrides.go 15.00% <0.00%> (-0.11%) ⬇️

rootulp
rootulp previously approved these changes Oct 16, 2023
@@ -225,6 +225,7 @@ func DefaultConsensusConfig() *tmcfg.Config {
// version. This acts as a first line of DoS protection
upperBoundBytes := appconsts.DefaultSquareSizeUpperBound * appconsts.DefaultSquareSizeUpperBound * appconsts.ContinuationSparseShareContentSize
cfg.Mempool.MaxTxBytes = upperBoundBytes
cfg.Mempool.MaxTxsBytes = appconsts.DefaultMaxBytes * cfg.Mempool.TTLNumBlocks
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think this should probably be the upper bound not the governance upper bound. Reason is if we go from 2MB to 8MB then 10MB can only hold enough for a single block

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense aff3566

@cmwaters cmwaters added the backport:v1.x PR will be backported automatically to the v1.x branch upon merging label Oct 16, 2023
@celestia-bot celestia-bot requested a review from a team October 16, 2023 11:47
@evan-forbes evan-forbes enabled auto-merge (squash) October 16, 2023 11:47
@evan-forbes evan-forbes merged commit 2bf9999 into main Oct 16, 2023
31 checks passed
@evan-forbes evan-forbes deleted the evan/default-mempool-size branch October 16, 2023 14:45
mergify bot pushed a commit that referenced this pull request Oct 16, 2023
## Overview

The default mempool size should match that of the default TTL and block
size.

## Checklist

- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user facing features like CLI or
documentation updates
- [ ] Linked issues closed with keywords

(cherry picked from commit 2bf9999)
rootulp pushed a commit that referenced this pull request Oct 17, 2023
…ort #2681) (#2685)

This is an automatic backport of pull request #2681 done by
[Mergify](https://mergify.com).


---


<details>
<summary>Mergify commands and options</summary>

<br />

More conditions and actions can be found in the
[documentation](https://docs.mergify.com/).

You can also trigger Mergify actions by commenting on this pull request:

- `@Mergifyio refresh` will re-evaluate the rules
- `@Mergifyio rebase` will rebase this PR on its base branch
- `@Mergifyio update` will merge the base branch into this PR
- `@Mergifyio backport <destination>` will backport this PR on
`<destination>` branch

Additionally, on Mergify [dashboard](https://dashboard.mergify.com) you
can:

- look at your merge queues
- generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com
</details>

Co-authored-by: Evan Forbes <42654277+evan-forbes@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:v1.x PR will be backported automatically to the v1.x branch upon merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants