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

Some proposers create blocks with duplicated transactions #2846

Closed
danil-lashin opened this issue Nov 15, 2018 · 8 comments
Closed

Some proposers create blocks with duplicated transactions #2846

danil-lashin opened this issue Nov 15, 2018 · 8 comments
Labels
C:mempool Component: Mempool T:bug Type Bug (Confirmed)
Milestone

Comments

@danil-lashin
Copy link
Contributor

Tendermint version (use tendermint version or git rev-parse --verify HEAD if installed from source): v0.26.0

ABCI app (name for built-in, URL for self-written if it's publicly available): https://github.com/MinterTeam/minter-go-node

Environment:

  • OS (e.g. from /etc/os-release): Ubuntu 18.04.1 LTS
  • Install tools: -
  • Others: -

What happened:
Some proposers create blocks with duplicated transactions. Here is example of a block with 3 exact same transactions: https://pastebin.com/HzzK6myi

What you expected to happen:
Proposers should not duplicate transactions in blocks.

Have you tried the latest version: yes

How to reproduce it (as minimally and precisely as possible): idk :)

Logs (paste a small part showing an error (< 10 lines) or link a pastebin, gist, etc. containing more of the log file): no related logs

Config (you can paste only the changes you've made):
Mempool.CacheSize = 100000
Mempool.Recheck = true
Mempool.Size = 10000

Anything else we need to know:
This issue often appears on slow machines.

@ebuchman
Copy link
Contributor

Does your app include some kind of replay protection (eg. an increment sequence number per account, like in Ethereum)? Technically this is supposed to be the app's responsibility, but the mempool includes an additional heuristic (ie. the cache) to prevent it.

Might be helpful if you have some mempool logs around this time ?

@ebuchman ebuchman added T:bug Type Bug (Confirmed) C:mempool Component: Mempool labels Nov 15, 2018
@ebuchman ebuchman added this to the v1.0 milestone Nov 15, 2018
@danil-lashin
Copy link
Contributor Author

Does your app include some kind of replay protection?

Yes. Just like Ethereum we are using nonce in transactions, so only one of these 3 transactions was applied to state. Other 2 marked as non-valid by our app.

Mempool includes an additional heuristic (ie. the cache) to prevent it.

Indeed, and there is 2 strange things in this situation:

  1. Cache did not help
  2. Mempool's transaction re-checking method did not help too

Might be helpful if you have some mempool logs around this time ?

Sure, I'll collect them.

@ValarDragon
Copy link
Contributor

Yes. Just like Ethereum we are using nonce in transactions, so only one of these 3 transactions was applied to state. Other 2 marked as non-valid by our app.

Are these failing CheckTx, or just deliver Tx. If it only fails deliver and not check, the following may be happening:

Machine 1 hasn't seen tx
Machine 2 proposes block w/ tx, machine 1 receives and removes tx from pool
Machine 1 receives tx from peer who hasn't received block yet
Machine 1 proposes block with tx in it

What we can do to fix this is update the mempool cache for txs received in a block, which makes sense to me. I'll open a new issue to discuss that in particular. (No guarantees that is what the problem your facing is though)

melekes added a commit that referenced this issue Nov 19, 2018
We should add txs that come in from mempool.Update to the mempool's
cache, so that they never hit a potentially expensive check tx.

Originally posted by @ValarDragon in #2846
#2846 (comment)

Refs #2855
melekes added a commit that referenced this issue Nov 20, 2018
We should add txs that come in from mempool.Update to the mempool's
cache, so that they never hit a potentially expensive check tx.

Originally posted by @ValarDragon in #2846
#2846 (comment)

Refs #2855
melekes added a commit that referenced this issue Nov 20, 2018
We should add txs that come in from mempool.Update to the mempool's
cache, so that they never hit a potentially expensive check tx.

Originally posted by @ValarDragon in #2846
#2846 (comment)

Refs #2855
@melekes
Copy link
Contributor

melekes commented Nov 27, 2018

@danil-lashin did the issue go away after #2882?

@danil-lashin
Copy link
Contributor Author

@danil-lashin did the issue go away after #2882?

I have not tried it yet. AFAIU, this fix is not released? If not I will try to use develop branch if there were no breaking changes.

@melekes
Copy link
Contributor

melekes commented Nov 27, 2018

There will be a non-breaking release later today.

@danil-lashin
Copy link
Contributor Author

@melekes, seems like the latest version solved the issue, thanks!

I also have another related problem. Just like Ethereum, we are using nonce in transactions. When a user tries to send more than one transaction in the block this situation appears:

Consider 2 transactions with nonces 1 and 2.
The user sends the first transaction with nonce 1. It successfully passes CheckTx method. Then, before the first tx is included in a block, he sends the second transaction with nonce 2. CheckTx method fails because it expected tx with nonce 1.

And here is the worst scenario:
The user sends the first transaction with nonce 1. It successfully passes CheckTx method. Then, before the first tx is included in a block, he sends another transaction with the same nonce again. It successfully passes CheckTx method and it will be included in a block. So we can have a situation where we will have 1 valid tx in a block and 9999 failed txs. I suppose this is a security issue because the user will pay a fee only for one tx and will be able to send many. Of course, invalid transactions will not be applied to the state, but a malicious user can utilize this vulnerability to pass some data to another party using blockchain and increase the size of a chain.

Do you have an issue for that? Or I can open one?

@ebuchman
Copy link
Contributor

The user sends the first transaction with nonce 1. It successfully passes CheckTx method. Then, before the first tx is included in a block, he sends the second transaction with nonce 2. CheckTx method fails because it expected tx with nonce 1.

The application needs to maintain some CheckTx state for the mempool connection to be able to handle this. This is what we do in the SDK for instance, and just reset it with every Commit.

Unless I'm missing something, my understanding of what you've described is an application-level concern. See eg. https://tendermint.com/docs/spec/abci/apps.html#mempool-connection

seems like the latest version solved the issue, thanks!

Great, thanks. I'll close this issue for now. If you still think there's another problem, please open a new issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:mempool Component: Mempool T:bug Type Bug (Confirmed)
Projects
None yet
Development

No branches or pull requests

4 participants