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

mempool: introduce CacheKeepCheckTxInvalid config option #5797

Closed
wants to merge 1 commit into from
Closed

mempool: introduce CacheKeepCheckTxInvalid config option #5797

wants to merge 1 commit into from

Conversation

p4u
Copy link
Contributor

@p4u p4u commented Dec 15, 2020

see #5751

Signed-off-by: p4u pau@dabax.net

Description

Closes: #5751

@codecov
Copy link

codecov bot commented Dec 15, 2020 •
edited

Codecov Report

Merging #5797 (70d7dda) into master (bdf688d) will decrease coverage by 0.01%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##           master    #5797      +/-   ##
==========================================
- Coverage   59.93%   59.91%   -0.02%     
==========================================
  Files         262      262              
  Lines       23704    23708       +4     
==========================================
- Hits        14207    14205       -2     
- Misses       7992     7998       +6     
  Partials     1505     1505
Impacted Files Coverage Δ
config/config.go 78.31% <ø> (ø)
config/toml.go 60.86% <ø> (ø)
mempool/clist_mempool.go 80.78% <55.55%> (-0.45%) ⬇️
consensus/reactor.go 75.72% <0.00%> (-1.77%) ⬇️
p2p/pex/pex_reactor.go 78.27% <0.00%> (-1.20%) ⬇️
statesync/syncer.go 78.96% <0.00%> (-0.80%) ⬇️
evidence/reactor.go 54.00% <0.00%> (ø)
proxy/multi_app_conn.go 48.05% <0.00%> (ø)
consensus/state.go 68.68% <0.00%> (+0.27%) ⬆️
privval/signer_endpoint.go 81.81% <0.00%> (+3.03%) ⬆️
... and 2 more

alexanderbez
alexanderbez reviewed Dec 15, 2020
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Was this the agreed upon approach?

config/toml.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
@p4u
Copy link
Contributor Author

p4u commented Dec 15, 2020

Was this the agreed upon approach?

I am not sure. The last comment from @marbar3778 was pointing on this direction, but I'd not say yet this is a common agreement.

melekes
melekes reviewed Dec 16, 2020
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.

nice! could you also update docs/tendermint-core/configuration.md file?

@p4u
Copy link
Contributor Author

p4u commented Dec 16, 2020

nice! could you also update docs/tendermint-core/configuration.md file?

Done!

I might be wrong but looks like all options from configuration.md should be modified to use - instead of _ as separator.

@cmwaters
Copy link
Contributor

I might be wrong but looks like all options from configuration.md should be modified to use - instead of _ as separator.

Yup we should update these

melekes
melekes approved these changes Dec 16, 2020
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 Dec 16, 2020

"This branch is out-of-date with the base branch"

@p4u could you please rebase this against the latest master?

@p4u
Copy link
Contributor Author

p4u commented Dec 16, 2020

"This branch is out-of-date with the base branch"

@p4u could you please rebase this against the latest master?

Done

@melekes
Copy link
Contributor

melekes commented Dec 17, 2020

sorry, I will need to ask you to rebase once again. alternatively, you can allow owners (us) to rebase your PR in github repo settings.

@p4u
Copy link
Contributor Author

p4u commented Dec 18, 2020

sorry, I will need to ask you to rebase once again. alternatively, you can allow owners (us) to rebase your PR in github repo settings.

NP.

Done! We'll need to squash the commits.

@melekes
Copy link
Contributor

melekes commented Dec 18, 2020

This branch cannot be rebased due to conflicts
Rebasing the commits of this branch on top of the base branch cannot be performed automatically due to conflicts encountered while reapplying the individual commits from the head branch.

weird.. I will fetch commits myself on Monday and create another PR. thanks anyway for trying

@p4u
mempool: introduce CacheKeepCheckTxInvalid config option
70d7dda 
If set to true, an invalid transaction will be kept in cache in order to
avoid processing it again.

see tendermint#5751

Signed-off-by: p4u <pau@dabax.net>
@p4u
Copy link
Contributor Author

p4u commented Dec 19, 2020

This branch cannot be rebased due to conflicts
Rebasing the commits of this branch on top of the base branch cannot be performed automatically due to conflicts encountered while reapplying the individual commits from the head branch.

weird.. I will fetch commits myself on Monday and create another PR. thanks anyway for trying

I rebuild manually the commit based on the last master tendermint branch.

Lets see if this can be directly merged now.

@melekes
Copy link
Contributor

melekes commented Dec 21, 2020

Closing in favor of #5813

@melekes melekes closed this Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikgrinaker erikgrinaker erikgrinaker left review comments

@alexanderbez alexanderbez alexanderbez left review comments

@melekes melekes melekes approved these changes

@ebuchman ebuchman Awaiting requested review from ebuchman

@tac0turtle tac0turtle Awaiting requested review from tac0turtle

@tessr tessr Awaiting requested review from tessr

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

Successfully merging this pull request may close these issues.

Transaction is removed from Mempool cache if CheckTX result != 0
5 participants
@p4u @cmwaters @melekes @erikgrinaker @alexanderbez