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

Support both rate limiter and pendingPropagation in mempool #1238

Merged
merged 6 commits into from
Jan 13, 2022
Merged

Conversation

goshawk-3
Copy link
Contributor

fixes #1237

@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2022

Codecov Report

Merging #1238 (ab6ad48) into master (828364b) will decrease coverage by 2.89%.
The diff coverage is 52.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1238      +/-   ##
==========================================
- Coverage   50.51%   47.62%   -2.90%     
==========================================
  Files         145      139       -6     
  Lines        7544     8508     +964     
==========================================
+ Hits         3811     4052     +241     
- Misses       3066     3870     +804     
+ Partials      667      586      -81     
Impacted Files Coverage Δ
pkg/config/consts.go 0.00% <ø> (ø)
pkg/config/genesis/config.go 0.00% <0.00%> (ø)
pkg/core/candidate/validator.go 64.00% <ø> (+6.85%) ⬆️
pkg/core/chain/legacy.go 0.00% <0.00%> (-28.58%) ⬇️
pkg/core/chain/mock.go 9.09% <ø> (+0.75%) ⬆️
...kg/core/consensus/blockgenerator/candidate/mock.go 0.00% <0.00%> (ø)
pkg/core/consensus/comms.go 0.00% <0.00%> (ø)
pkg/core/consensus/fixtures.go 0.00% <0.00%> (ø)
pkg/core/consensus/header/header.go 24.50% <0.00%> (+2.41%) ⬆️
pkg/core/consensus/publisher.go 0.00% <0.00%> (ø)
... and 183 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3916d3...ab6ad48. Read the comment docs.

Copy link
Member

@autholykos autholykos left a comment

Choose a reason for hiding this comment

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

Some minor changes requested:

  • If my understanding of the rate limiting is correct, we should probably allow an additional configuration parameter to tune transaction burst other than 1.
  • Context cancelation leakage should be assessed. It might be that the m.pendingPropagation channel would be garbage collected, but this needs to be double checked

pkg/core/mempool/mempool.go Outdated Show resolved Hide resolved
pkg/core/mempool/mempool.go Show resolved Hide resolved
pkg/core/mempool/mempool.go Show resolved Hide resolved
@goshawk-3 goshawk-3 requested a review from autholykos January 13, 2022 12:05
Copy link
Member

@autholykos autholykos left a comment

Choose a reason for hiding this comment

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

LGTM

@goshawk-3 goshawk-3 merged commit 8a0b983 into master Jan 13, 2022
@goshawk-3 goshawk-3 deleted the fix-1237 branch January 13, 2022 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement back pressure on mempool tx processing
3 participants