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: bump the max tx bytes in the mempool config #2219

Merged
merged 4 commits into from
Aug 8, 2023

Conversation

cmwaters
Copy link
Contributor

@cmwaters cmwaters commented Aug 7, 2023

No description provided.

@MSevey MSevey requested a review from a team August 7, 2023 12:00
@cmwaters cmwaters self-assigned this Aug 7, 2023
rootulp
rootulp previously approved these changes Aug 7, 2023
Comment on lines +169 to +174
// Given that there is a stateful transaction size check in CheckTx,
// We set a loose upper bound on what we expect the transaction to
// be based on the upper bound size of the entire block for the given
// version. This acts as a first line of DoS protection
upperBoundBytes := appconsts.DefaultSquareSizeUpperBound * appconsts.DefaultSquareSizeUpperBound * appconsts.ContinuationSparseShareContentSize
cfg.Mempool.MaxTxBytes = upperBoundBytes
Copy link
Collaborator

@rootulp rootulp Aug 7, 2023

Choose a reason for hiding this comment

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

I think this change makes sense b/c otherwise when governance bumps the gov max square size param from 64 to 128, a transaction that occupies (roughly) more than half the new square size will be rejected by all validators with the default max tx bytes. Validators can accept by modifying their local MaxTxBytes in config.toml but it seems prescient to bump the default now.

[optional]

  1. Do we want to change it here?
    // set the mempool's MaxTxBytes to allow the testnode to accept a
    // transaction that fills the entire square. Any blob transaction larger
    // than the square size will still fail no matter what.
    tmCfg.Mempool.MaxTxBytes = appconsts.DefaultMaxBytes
  2. Do we want to extract an appconst const for it?

[discussion]

stateful transaction size check in CheckTx

The stateful check in CheckTx uses appconsts.ShareSize instead of appconsts.ContinuationSparseShareContentSize here:

// squareBytes returns the number of bytes in a square for the given squareSize.
func squareBytes(squareSize int) int {
	numShares := squareSize * squareSize
	return numShares * appconsts.ShareSize
}

Mostly because the upper bound max blob size doesn't need to be exact. Similarly, MaxTxBytes doesn't need to be exact and I think it's slightly cleaner to rely on appconsts.ShareSize which is much less likely to change than appconsts.ContinuationSparseShareContentSize which could change with a share version bump. So we may consider changing appconsts.DefaultMaxBytes and cfg.Mempool.MaxTxBytes to calculate based on appconsts.ShareSize but definitely optional.

Copy link
Member

Choose a reason for hiding this comment

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

I could see the benefit of switching to something simpler for readability. I'm also fine with using a more accurate value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to change it here?

Yeah that makes sense

Do we want to extract an appconst const for it?

I'm indifferent. If it were used in more places I'd probably be for it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like appconsts for this but definitely not blocking on it.

evan-forbes
evan-forbes previously approved these changes Aug 7, 2023
@cmwaters cmwaters dismissed stale reviews from evan-forbes and rootulp via b172c8b August 8, 2023 07:04
@MSevey MSevey requested a review from a team August 8, 2023 07:05
@codecov-commenter
Copy link

Codecov Report

Merging #2219 (a176976) into main (3dd1ba6) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2219      +/-   ##
==========================================
- Coverage   21.54%   21.53%   -0.01%     
==========================================
  Files         127      127              
  Lines       14329    14335       +6     
==========================================
  Hits         3087     3087              
- Misses      10939    10945       +6     
  Partials      303      303              
Files Changed Coverage Δ
app/default_overrides.go 16.51% <0.00%> (-0.97%) ⬇️

@evan-forbes evan-forbes added the backport:v1.x PR will be backported automatically to the v1.x branch upon merging label Aug 8, 2023
@evan-forbes
Copy link
Member

do we need to update this branch for the backport to work?

@cmwaters cmwaters enabled auto-merge (squash) August 8, 2023 16:00
@MSevey MSevey requested a review from a team August 8, 2023 16:00
@cmwaters cmwaters merged commit 4276080 into main Aug 8, 2023
26 checks passed
@cmwaters cmwaters deleted the cal/bump-mempl-max-bytes branch August 8, 2023 16:12
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