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 the Unauthorized error of full node caused by mempool tx #8511

Closed

Conversation

AaronZgl
Copy link

@AaronZgl AaronZgl commented Feb 4, 2021

Description

closes: #8510

  1. If a tx passes anteHandler of a full node but fail to pass anteHandler of all validators cause the minGasPrice of the full node is less than all validators, the tx will not be included in any block and only exist in the memory pool of the full node and be rechecked after per block.
  2. At this point, if the user sends another tx to the full node, the new tx will get the unauthorized error because the correct sequence of user will be added in NewIncrementSequenceDecorator when the old tx was rechecked. So,the NewIncrementSequenceDecorator function should be skipped in recheck.
  3. If both NewIncrementSequenceDecorator and NewSigVerificationDecorator were skipped in recheck, the old tx would always exist in the memory pool. And if NewSigVerificationDecorator is not be skipped in recheck, the old tx would fail to verify signature and be removed when the sequence of user was updated.

So the NewIncrementSequenceDecorator function should be skipped in recheck but not NewSigVerificationDecorator.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Mar 22, 2021
@@ -196,10 +196,6 @@ func OnlyLegacyAminoSigners(sigData signing.SignatureData) bool {
}

func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) {
// no need to verify signatures on recheck tx
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct. We can and should bypass signature verification on rechecking.

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

Blocking until we get consensus on the correct behavior. @alexanderbez ?

@alexanderbez
Copy link
Contributor

The NewIncrementSequenceDecorator change is correct. In fact, the fact that it's not there is a regression. See the original PR: https://github.com/cosmos/cosmos-sdk/pull/5950/files.

The NewSigVerificationDecorator I think is NOT correct. One of the entire points of ReCheckTx is the fact that we can skip sig verification.

@aaronc
Copy link
Member

aaronc commented Mar 24, 2021

The NewIncrementSequenceDecorator change is correct. In fact, the fact that it's not there is a regression. See the original PR: https://github.com/cosmos/cosmos-sdk/pull/5950/files.

It was reverted in #6291. Do you have any context for that @alexanderbez ?

@github-actions github-actions bot removed the stale label Mar 25, 2021
@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label May 10, 2021
@alexanderbez
Copy link
Contributor

It was to allow multiple txs in the same block from a sender which has always been a point of confusion for me.

@github-actions github-actions bot removed the stale label May 11, 2021
@blushi blushi self-assigned this May 11, 2021
@tac0turtle
Copy link
Member

pinging for an update here

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants