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

allow multiple transactions from the same account on the same block #5905

Closed
wants to merge 4 commits into from
Closed

allow multiple transactions from the same account on the same block #5905

wants to merge 4 commits into from

Conversation

cryptophonic
Copy link

Closes: #XXX

Description

Removes code that was preventing multiple transactions from being submitted from the same account in a single block, even if the transactions had correctly incremented sequence numbers and were submitted in the proper order.


For contributor use:

  • 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

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Apr 1, 2020

Codecov Report

Merging #5905 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5905   +/-   ##
=======================================
  Coverage   57.06%   57.06%           
=======================================
  Files         342      342           
  Lines       20056    20056           
=======================================
  Hits        11444    11444           
  Misses       7767     7767           
  Partials      845      845
Impacted Files Coverage Δ
x/auth/ante/sigverify.go 82.22% <100%> (ø) ⬆️

@@ -219,10 +219,7 @@ func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simul
// IncrementSequenceDecorator handles incrementing sequences of all signers.
// Use the IncrementSequenceDecorator decorator to prevent replay attacks. Note,
// there is no need to execute IncrementSequenceDecorator on CheckTx or RecheckTX
// since it is merely updating the nonce. As a result, this has the side effect
Copy link
Contributor

Choose a reason for hiding this comment

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

I would undo this comment removal. The idea is still the same. BaseApp maintains two different states -- check and deliver state. Sending sequential txs for the same block can still be unreliable.

Copy link
Contributor

Choose a reason for hiding this comment

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

bump @mjackson001

x/auth/ante/sigverify.go Show resolved Hide resolved
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

@mjackson001 can you add an entry on the CHANGELOG.md under BugFixes?

@fedekunze fedekunze added the T:Bug label Apr 1, 2020
Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

utACK

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.

ACK pending changelog entry @mjackson001 :)

@@ -96,6 +96,7 @@ to now accept a `codec.JSONMarshaler` for modular serialization of genesis state
when method receivers are offline/multisig keys.
* (x/auth) [\#5892](https://github.com/cosmos/cosmos-sdk/pull/5892) Add `RegisterKeyTypeCodec` to register new
types (eg. keys) to the `auth` module internal amino codec.
* (x/auth) Allow more than one Tx per account per block
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* (x/auth) Allow more than one Tx per account per block
* (x/auth) [\#5905](https://github.com/cosmos/cosmos-sdk/pull/5905) Fix `IncrementSequenceDecorator` to use is `IsReCheckTx` instead of `IsCheckTx` to allow account sequence incrementing.

@@ -219,10 +219,7 @@ func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simul
// IncrementSequenceDecorator handles incrementing sequences of all signers.
// Use the IncrementSequenceDecorator decorator to prevent replay attacks. Note,
// there is no need to execute IncrementSequenceDecorator on CheckTx or RecheckTX
// since it is merely updating the nonce. As a result, this has the side effect
Copy link
Contributor

Choose a reason for hiding this comment

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

bump @mjackson001

@alexanderbez alexanderbez mentioned this pull request Apr 7, 2020
11 tasks
@alexanderbez
Copy link
Contributor

replaced by #5950

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

Successfully merging this pull request may close these issues.

4 participants