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

fix: Make rechecking a tx check the sequence number #12060

Merged
merged 1 commit into from
May 26, 2022

Conversation

ValarDragon
Copy link
Contributor

@ValarDragon ValarDragon commented May 26, 2022

Upstream of my change to Osmosis that makes recheck still check sequence numbers.

This has been incorporated into Terra and Secret Network.

osmosis-labs#58

The antehandler check updated (despite its name) is responsible for checking two things:

  1. Sequence Numbers
  2. Signatures

Previously on recheck, it skipped the entire check, and therefore skipped both of its responsibilities.
Now it only skips the signature verification component, which was the purpose of recheck. (To save the heavy comptutional burden)

Every node with a default mempool, runs this ante handler check in "recheck" mode, on every single tx in their mempool upon each block being committed. Any tx with an invalidated sequence number will then get evicted from the mempool, ensuring that bad sequence numbers no longer enter in blocks produced by nodes with the default mempool.

You only really run into this as a noticeable problem once two conditions are satisfied:

  1. At least one validator runs a modified mempool (Sentinel does this)
  2. There are bots on the network, creating many txs from the same addr

which is the case for Osmosis, Terra, Secret, Evmos


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@ValarDragon ValarDragon added backport/0.45.x backport/0.46.x PR scheduled for inclusion in the v0.46's next stable release labels May 26, 2022
@ValarDragon ValarDragon requested a review from a team as a code owner May 26, 2022 20:54
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.

❤️

@alexanderbez alexanderbez merged commit 8eaff8f into main May 26, 2022
@alexanderbez alexanderbez deleted the dev/upstream_checktx_fix branch May 26, 2022 21:09
mergify bot pushed a commit that referenced this pull request May 26, 2022
mergify bot pushed a commit that referenced this pull request May 26, 2022
@robert-zaremba
Copy link
Collaborator

This is to clean the mempool from the transactions with the same sequence number, right?

@WhiteNoise1989
Copy link

@ValarDragon Does this mean that all user transactions that were not included in a block, will get evicted from mempool?

@WhiteNoise1989
Copy link

For example -

Account 'A' with expected sequence 's'. User sends a tx with sequence 's'. CheckTx runs, and passes(assuming all other checks were passed). CheckTx bumps account sequence to s+1. For some reason tx was not included in block. At this point account seq is s+1, but tx seq remains s. Now at end of block(since tx was not included in block), RecheckTx runs on above tx(with above pr this will check the account sequence), and fail bcz of account seq mismatch. Tx is evicted from mempool.

@tac0turtle
Copy link
Member

This is to clean the mempool from the transactions with the same sequence number, right?

correct

tac0turtle pushed a commit that referenced this pull request May 27, 2022
(cherry picked from commit 8eaff8f)

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
alexanderbez pushed a commit that referenced this pull request May 27, 2022
(cherry picked from commit 8eaff8f)

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
@ValarDragon
Copy link
Contributor Author

@ValarDragon Does this mean that all user transactions that were not included in a block, will get evicted from mempool?

No, the Tendermint mempool maintains an ordering s.t. a tx with sequence number s + 1 must come after a tx w/ sequence number s (or last sequence number on chain was s)

Therefore if tx w/ seq num s gets in a block, then s+1 will remain in the mempool. If competing txs w/ sequence number s, s+1, s+2 get on-chain, the tx with w/ seq s+1 in your mempool will get removed on the recheck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/0.46.x PR scheduled for inclusion in the v0.46's next stable release C:x/auth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants