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

fix: Evict txs from mempool exceeding size limits #550

Merged
merged 2 commits into from
Jun 25, 2024

Conversation

davidterpay
Copy link
Contributor

No description provided.

@davidterpay davidterpay added backport/v1.x.x Backport your PR to the v1.x.x release backport/v2.x.x Backport your PR to the v2.x.x release labels Jun 25, 2024
@davidterpay davidterpay marked this pull request as ready for review June 25, 2024 15:17
Copy link
Contributor

@nivasan1 nivasan1 left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -49,7 +49,6 @@ func (h *ProposalHandler) PrepareLaneHandler() base.PrepareLaneHandler {
}

cacheCtx, write := ctx.CacheContext()

Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove?

Copy link
Contributor

@Eric-Warehime Eric-Warehime left a comment

Choose a reason for hiding this comment

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

A test for this would be nice, and pretty easy to write if you just set max gas/bytes to small numbers right?

Copy link

codecov bot commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 0% with 20 lines in your changes missing coverage. Please review.

Project coverage is 43.59%. Comparing base (79d7ef7) to head (a465770).

Files Patch % Lines
block/base/proposals.go 0.00% 20 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #550      +/-   ##
==========================================
- Coverage   43.91%   43.59%   -0.32%     
==========================================
  Files          63       63              
  Lines        2792     2812      +20     
==========================================
  Hits         1226     1226              
- Misses       1439     1459      +20     
  Partials      127      127              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@davidterpay
Copy link
Contributor Author

davidterpay commented Jun 25, 2024

A test for this would be nice, and pretty easy to write if you just set max gas/bytes to small numbers right?

There are tests for this, just added an additional condition check to ensure the tx was removed from the mempool
cc: @Eric-Warehime

@Eric-Warehime
Copy link
Contributor

Screenshot 2024-06-25 at 8 22 24 AM
cc @davidterpay

@davidterpay
Copy link
Contributor Author

not sure why codecov is saying no coverage when the test case hits that exact line of code. check lanes/base/abci_test.go

cc: @Eric-Warehime

@davidterpay
Copy link
Contributor Author

Screenshot 2024-06-25 at 11 25 48 AM

@davidterpay davidterpay merged commit b8d9c22 into main Jun 25, 2024
9 of 12 checks passed
@davidterpay davidterpay deleted the terpay/check-size-and-limit-with-eviction branch June 25, 2024 15:58
mergify bot pushed a commit that referenced this pull request Jun 25, 2024
* init

* nit test case

(cherry picked from commit b8d9c22)
mergify bot pushed a commit that referenced this pull request Jun 25, 2024
* init

* nit test case

(cherry picked from commit b8d9c22)
davidterpay added a commit that referenced this pull request Jun 25, 2024
* init

* nit test case

(cherry picked from commit b8d9c22)

Co-authored-by: David Terpay <[email protected]>
davidterpay added a commit that referenced this pull request Jun 25, 2024
* init

* nit test case

(cherry picked from commit b8d9c22)

Co-authored-by: David Terpay <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v1.x.x Backport your PR to the v1.x.x release backport/v2.x.x Backport your PR to the v2.x.x release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants