mempool: remove only valid (Code==0) txs on Update #3625
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
so evil proposers can't drop valid txs in Commit stage. Also remove invalid (Code!=0) txs from the cache so they can be resubmitted. Fixes #3322 @rickyyangz: In the end of commit stage, we will update mempool to remove all the txs in current block. // Update mempool. err = blockExec.mempool.Update( block.Height, block.Txs, TxPreCheck(state), TxPostCheck(state), ) Assum an account has 3 transactions in the mempool, the sequences are 100, 101 and 102 separately, So an evil proposal can only package the 101 and 102 transactions into its proposal block, and leave 100 still in mempool, then the two txs will be removed from all validators' mempool when commit. So the account lost the two valid txs. @ebuchman: In the longer term we may want to do something like #2639 so we can validate txs before we commit the block. But even in this case we'd only want to run the equivalent of CheckTx, which means the DeliverTx could still fail even if the CheckTx passes depending on how the app handles the ABCI Code semantics. So more work will be required around the ABCI code. See also #2185
Codecov Report
@@ Coverage Diff @@
## develop #3625 +/- ##
==========================================
- Coverage 63.31% 63.2% -0.12%
==========================================
Files 218 217 -1
Lines 18253 18165 -88
==========================================
- Hits 11557 11481 -76
+ Misses 5737 5717 -20
- Partials 959 967 +8
|
liamsi
reviewed
May 6, 2019
liamsi
reviewed
May 6, 2019
liamsi
reviewed
May 6, 2019
liamsi
reviewed
May 6, 2019
| // Block, proposed by evil proposer: | ||
| // 101 -> 102 | ||
| // Mempool (if you remove txs): | ||
| // 100 |
There was a problem hiding this comment.
Can we write this down as a test case (which fails without the changes)?
There was a problem hiding this comment.
TestMempoolUpdate will fail without the changes
There was a problem hiding this comment.
I understand but I wondered if we should have the above example in a test instead of what is there in TestMempoolUpdate. But NVM. TestMempoolUpdate is actually better as it is more minimalistic!
This was referenced May 7, 2019
4 tasks
ebuchman
pushed a commit
that referenced
this pull request
Jun 3, 2019
brapse
pushed a commit
to brapse/tendermint
that referenced
this pull request
Jun 5, 2019
* mempool: remove only valid (Code==0) txs on Update so evil proposers can't drop valid txs in Commit stage. Also remove invalid (Code!=0) txs from the cache so they can be resubmitted. Fixes tendermint#3322 @rickyyangz: In the end of commit stage, we will update mempool to remove all the txs in current block. // Update mempool. err = blockExec.mempool.Update( block.Height, block.Txs, TxPreCheck(state), TxPostCheck(state), ) Assum an account has 3 transactions in the mempool, the sequences are 100, 101 and 102 separately, So an evil proposal can only package the 101 and 102 transactions into its proposal block, and leave 100 still in mempool, then the two txs will be removed from all validators' mempool when commit. So the account lost the two valid txs. @ebuchman: In the longer term we may want to do something like #2639 so we can validate txs before we commit the block. But even in this case we'd only want to run the equivalent of CheckTx, which means the DeliverTx could still fail even if the CheckTx passes depending on how the app handles the ABCI Code semantics. So more work will be required around the ABCI code. See also tendermint#2185 * add changelog entry and tests * improve changelog message * reformat code
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
so evil proposers can't drop valid txs in Commit stage.
Also remove invalid (Code!=0) txs from the cache so they can be
resubmitted.
Fixes #3322
@rickyyangz:
In the end of commit stage, we will update mempool to remove all the txs
in current block.
Assum an account has 3 transactions in the mempool, the sequences are
100, 101 and 102 separately, So an evil proposal can only package the
101 and 102 transactions into its proposal block, and leave 100 still in
mempool, then the two txs will be removed from all validators' mempool
when commit. So the account lost the two valid txs.
@ebuchman:
In the longer term we may want to do something like #2639 so we can
validate txs before we commit the block. But even in this case we'd only
want to run the equivalent of CheckTx, which means the DeliverTx could
still fail even if the CheckTx passes depending on how the app handles
the ABCI Code semantics. So more work will be required around the ABCI
code. See also #2185