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 KeepInvalidTxsInCache config option #5813

Merged
merged 8 commits into from
Dec 21, 2020
Merged

mempool: introduce KeepInvalidTxsInCache config option #5813

merged 8 commits into from
Dec 21, 2020

Conversation

melekes
Copy link
Contributor

@melekes melekes commented Dec 21, 2020 •
edited
Loading

When set to true, an invalid transaction will be kept in the cache (this may help some applications to protect against spam).

NOTE: this is a temporary config option. The more correct solution would be to add a TTL to each transaction (i.e. CheckTx may return a TTL in ResponseCheckTx).

Closes: #5751

p4u and others added 2 commits December 21, 2020 10:12
@p4u @melekes
mempool: introduce CacheKeepCheckTxInvalid config option
5f36b33 
If set to true, an invalid transaction will be kept in cache in order to
avoid processing it again.

see #5751

Signed-off-by: p4u <pau@dabax.net>
@melekes
fix linter issue
5a977f4
@melekes melekes mentioned this pull request Dec 21, 2020
Closed
@melekes melekes changed the title mempool: introduce CacheKeepCheckTxInvalid config option mempool: introduce KeepInvalidTxsInCache config option Dec 21, 2020
@codecov
Copy link

codecov bot commented Dec 21, 2020 •
edited
Loading

Codecov Report

Merging #5813 (267ca3d) into master (bdf688d) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5813      +/-   ##
==========================================
+ Coverage   59.93%   60.01%   +0.07%     
==========================================
  Files         262      262              
  Lines       23704    23705       +1     
==========================================
+ Hits        14207    14226      +19     
+ Misses       7992     7973      -19     
- Partials     1505     1506       +1
Impacted Files Coverage Δ
config/config.go 78.31% <ø> (ø)
config/toml.go 60.86% <ø> (ø)
mempool/clist_mempool.go 81.29% <100.00%> (+0.06%) ⬆️
privval/signer_listener_endpoint.go 82.35% <0.00%> (-7.06%) ⬇️
privval/socket_listeners.go 78.72% <0.00%> (-4.26%) ⬇️
privval/secret_connection.go 73.71% <0.00%> (-2.58%) ⬇️
libs/clist/clist.go 67.25% <0.00%> (-1.76%) ⬇️
statesync/syncer.go 78.96% <0.00%> (-0.80%) ⬇️
consensus/reactor.go 77.35% <0.00%> (-0.13%) ⬇️
proxy/multi_app_conn.go 48.05% <0.00%> (ø)
... and 9 more

@melekes melekes self-assigned this Dec 21, 2020
@p4u
Copy link
Contributor

p4u commented Dec 21, 2020

@melekes what's the reason behind removing the cache transactions if the mempool is full? Wouldn't it be a better approach to keep them so invalid transactions won't occupy a slot that could be used by a valid transaction?

@melekes
Copy link
Contributor Author

melekes commented Dec 21, 2020 •
edited
Loading

what's the reason behind removing the cache transactions if the mempool is full?

If the mempool is full and a good tx comes in, it will be added to cache. But if, by the time ABCI application responds with CheckTx (Code: 0 - valid tx), the mempool becomes full, we want to remove tx from the cache so it can be readded. Otherwise, there's a "deadlock" of sort where a user got successful response (meaning CheckTx succeeded, but 1) it was not committed 2) he/she can't add it because cache already contains this tx).

erikgrinaker
erikgrinaker approved these changes Dec 21, 2020
View reviewed changes
Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

LGTM

@p4u
Copy link
Contributor

p4u commented Dec 21, 2020

what's the reason behind removing the cache transactions if the mempool is full?

If the mempool is full and a good tx comes in, it will be added to cache. But if, by the time ABCI application responds with CheckTx (Code: 0 - valid tx), the mempool becomes full, we want to remove tx from the cache so it can be readded. Otherwise, there's a "deadlock" of sort where a user got successful response (meaning CheckTx succeeded, but 1) it was not committed 2) he/she can't add it because cache already contains this tx).

Ok, makes sense.

@melekes melekes merged commit 6a056e0 into master Dec 21, 2020
@melekes melekes deleted the anton/5797-vocdoni-mempool branch December 21, 2020 11:25
@p4u
Copy link
Contributor

p4u commented Dec 21, 2020

Thanks for the work!

melekes added a commit that referenced this pull request Dec 21, 2020
@melekes
mempool: introduce KeepInvalidTxsInCache config option (#5813)
886857f 
When set to true, an invalid transaction will be kept in the cache (this may help some applications to protect against spam).

NOTE: this is a temporary config option. The more correct solution would be to add a TTL to each transaction (i.e. CheckTx may return a TTL in ResponseCheckTx).

Closes: #5751
melekes added a commit that referenced this pull request Dec 21, 2020
@melekes
mempool: introduce KeepInvalidTxsInCache config option (#5813)
dc90cf6 
When set to true, an invalid transaction will be kept in the cache (this may help some applications to protect against spam).

NOTE: this is a temporary config option. The more correct solution would be to add a TTL to each transaction (i.e. CheckTx may return a TTL in ResponseCheckTx).

Closes: #5751
cmwaters pushed a commit that referenced this pull request Jan 4, 2021
@melekes @cmwaters
mempool: introduce KeepInvalidTxsInCache config option (#5813)
522f087 
When set to true, an invalid transaction will be kept in the cache (this may help some applications to protect against spam).

NOTE: this is a temporary config option. The more correct solution would be to add a TTL to each transaction (i.e. CheckTx may return a TTL in ResponseCheckTx).

Closes: #5751
@brianatcrypto
Copy link

Does this fixes issue on cosmos/cosmos-sdk#4695?

@melekes
Copy link
Contributor Author

melekes commented Jan 8, 2021

not really since tx1 and tx2 are different (if I understood the issue you've linked correctly)

@brianatcrypto
Copy link

@melekes - all different transactions.

@cmwaters cmwaters mentioned this pull request Feb 22, 2022
Merged
This was referenced Apr 22, 2022
Merged
Merged
@hvanz hvanz mentioned this pull request Jan 26, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikgrinaker erikgrinaker erikgrinaker 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

@melekes melekes

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
4 participants
@melekes @p4u @brianatcrypto @erikgrinaker