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

Sending multiple transactions from an account provides "unauthorized: signature verification failed" #6287

Closed
ahmedaly113 opened this issue May 27, 2020 · 7 comments · Fixed by #6291
Labels

Comments

@ahmedaly113
Copy link
Contributor

ahmedaly113 commented May 27, 2020

We are using cosmos sdk v0.38.4.
Here's the script that is producing the issue

for i in 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
do
  echo "Running $i times"
  pylonscli tx send $(pylonscli keys show -a eugen --keyring-backend=test) cosmos13p8890funv54hflk82ju0zv47tspglpk373453 1000pylon --keyring-backend=test --generate-only > tx_send_$i.json
  pylonscli tx sign tx_send_$i.json --from $(pylonscli keys show -a eugen --keyring-backend=test) --offline --chain-id pylonschain --sequence $i --account-number 45 --keyring-backend test > signed_tx_send_$i.json
  pylonscli tx broadcast signed_tx_send_$i.json
done

Not all 15 transactions success in one block.
Also behavior is different for every run.
Sometimes first 5 transactions success and rest shows "unauthorized: signature verification failed" but sometime 1 transaction success and the rest shows "unauthorized: signature verification failed".
If I run the script again, more transactions success. And it usually needed to run this script 3-4 times to send 15 transactions.

The most strange thing is, it shows unauthorized issue when tons of transactions are sent, but if I broadcast the failed transaction with same signed file, it success in other block.

This is testing after #5950 PR.
Need more fix on this.
It was not happening on v0.35.0

For now, we are trying to broadcast transaction until it success but it's not a beautiful way.
Please ask me questions if you don't have same issue and I will help you to reproduce this issue,

@ahmedaly113
Copy link
Contributor Author

Also cosmos sdk is providing tons of issues that are not signature failed as signature failed issue.
I think this should be fixed.

@alexanderbez
Copy link
Contributor

Confirmed this is an issue on the latest Gaia master. I checked out v2.0.x of Gaia and was not able to replicate it there.

After adding some debugging lines, I noticed that the nonce gets "stuck" somehow and I'm not sure why...maybe something to do around new blocks being produced.

@alessio or @jgimeno do you have time to look into this?

You can use the following script: https://pastebin.com/eT4CwGsL


Also cosmos sdk is providing tons of issues that are not signature failed as signature failed issue.
I think this should be fixed.

This comment is unproductive and is not conducive to the betterment of the SDK.

@alexanderbez
Copy link
Contributor

Looks like the following fixes it:

diff --git a/x/auth/ante/sigverify.go b/x/auth/ante/sigverify.go
index 0b17b05bc..5974319ac 100644
--- a/x/auth/ante/sigverify.go
+++ b/x/auth/ante/sigverify.go
@@ -215,9 +215,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 RecheckTX since
-// CheckTx would already bump the sequence number.
+// Use the IncrementSequenceDecorator decorator to prevent replay attacks.
 //
 // NOTE: Since CheckTx and DeliverTx state are managed separately, subsequent and
 // sequential txs orginating from the same account cannot be handled correctly in
@@ -234,11 +232,6 @@ func NewIncrementSequenceDecorator(ak AccountKeeper) IncrementSequenceDecorator
 }
 
 func (isd IncrementSequenceDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) {
-       // no need to increment sequence on RecheckTx
-       if ctx.IsReCheckTx() && !simulate {
-               return next(ctx, tx, simulate)
-       }
-
        sigTx, ok := tx.(SigVerifiableTx)
        if !ok {
                return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type")

@alexanderbez
Copy link
Contributor

Please make these changes locally @ahmedaly113 and confirm.

@ahmedaly113
Copy link
Contributor Author

Let me check, actually it's my sleeping time :)

@ahmedaly113
Copy link
Contributor Author

It works if I remove IsRecheckTx() && !simulate if statement

@ahmedaly113
Copy link
Contributor Author

ahmedaly113 commented May 27, 2020

@alexanderbez

Running 0 times
{
  "height": "0",
  "txhash": "B34DF353BFAD378183BE5194CE51FD99EE32E61A48A2ABA308D699E46A1F476E",
  "raw_log": "[]"
}
Running 1 times
{
  "height": "0",
  "txhash": "50CAA3878F42F9B21F41634800B2F233584BC71AD1C7DCF201FE5C4958BF1CD7",
  "raw_log": "[]"
}
Running 2 times
{
  "height": "0",
  "txhash": "E07AB0DF7F153FEE7F98C64BDC39ADC3F9F8DB862DA774E1275111960E6E9D74",
  "raw_log": "[]"
}
Running 3 times
{
  "height": "0",
  "txhash": "14E0AED27523A93D595119E99A8E9A2ECB019921A0A58AF2108DBFB78E2E21EF",
  "raw_log": "[]"
}
Running 4 times
{
  "height": "0",
  "txhash": "338AD3B9163874DA70610514A9693500B735C70AC75B31078263BF6538607135",
  "raw_log": "[]"
}
Running 5 times
{
  "height": "0",
  "txhash": "B5BB90EB2E677C5DF73C3BDB714B09A17BF3DA3019C476B764A8B52F9C2BDE70",
  "raw_log": "[]"
}
Running 6 times
{
  "height": "0",
  "txhash": "71025092D000874F09AEE10F1F0976E9F410993B54D4CDC6F2259A14AA4F3E24",
  "raw_log": "[]"
}
Running 7 times
{
  "height": "0",
  "txhash": "F158E249FE1D77331D3A32B6D57656A9EEE84AC56845A8DE14F727425A222A4B",
  "raw_log": "[]"
}
Running 8 times
{
  "height": "0",
  "txhash": "9FA92A037D2A2DC39FF63DC7EE455226C95032291E097C3A60682561A4683CD3",
  "raw_log": "[]"
}
Running 9 times
{
  "height": "0",
  "txhash": "D9ABC42867399F74F7176643B9CC5C6C12CF7782F132DCA3C5EFD5C674BA86EE",
  "raw_log": "[]"
}
Running 10 times
{
  "height": "0",
  "txhash": "6CF85171F674183B45A2E3443A1A3C4A840A4095624171194A93FA6F33210D06",
  "raw_log": "[]"
}
Running 11 times
{
  "height": "0",
  "txhash": "F6DC53A301986CE437826925FBC50E164D3CD84FA9ABFBA70E98DBED2C1F3611",
  "raw_log": "[]"
}
Running 12 times
{
  "height": "0",
  "txhash": "E068128E4A99922B279290317DC0621A550D4BB94B3CBC10D23B5C8356F06279",
  "raw_log": "[]"
}
Running 13 times
{
  "height": "0",
  "txhash": "610082A4E9D0CB5C00A3BB93A781C78E182C43709CDA0BF8C32EA8F9D5002A21",
  "raw_log": "[]"
}
Running 14 times
{
  "height": "0",
  "txhash": "128F32631743F9FCCDA5FE39CBF75FC97EAF1AF27808530C31CE073ACE284B7C",
  "raw_log": "[]"
}
Running 15 times
{
  "height": "0",
  "txhash": "662C38773E6E2E5266F29237AB2ADC4D1C434DF96783B117BEFA0939C3D00A99",
  "raw_log": "[]"
}

ahmedaly113 added a commit to ahmedaly113/cosmos-sdk that referenced this issue May 27, 2020
ahmedaly113 added a commit to ahmedaly113/cosmos-sdk that referenced this issue May 27, 2020
@mergify mergify bot closed this as completed in #6291 May 28, 2020
mergify bot pushed a commit that referenced this issue May 28, 2020
…quence stuck (#6291)

* fix #6287 sending multiple transactions from an account make nonce sequence stuck

* add change log

* fix ante unit test after #6287 fix

* Update x/auth/ante/sigverify.go

Co-authored-by: Alessio Treglia <[email protected]>
Co-authored-by: Alessio Treglia <[email protected]>
Co-authored-by: Alexander Bezobchuk <[email protected]>
alessio pushed a commit that referenced this issue Jul 14, 2020
Raumo0 pushed a commit to mapofzones/cosmos-sdk that referenced this issue Feb 13, 2022
* v0.38.4-RC1

* add release date

* client/keys/parse: honor config changes (cosmos#6340)

`keys parse` uses the global configuration before
before client applications have had a chance to
apply their settings.

This change adds a `GetSealedConfig()` helper
that waits for the config to be sealed before
returning it.

Fixes: cosmos#5091
Origin: cosmos@4e328d7
Author: Adam Bozanich <[email protected]>
Reviewed-by: Alessio Treglia <[email protected]>

* Merge PR cosmos#6505: Backport v0.38.5: IAVL Bump (RC) & Pruning Refactor

* Fix cl

* Fix cl

* Merge PR cosmos#6551: Ethanfrey/fix trace flag

* Merge PR cosmos#6552: Add sender info to bank transfer event

* Merge PR cosmos#6581: v0.38.5-RC1

* Add release date

* deps: bump IAVL to 0.14

* cl++

* Merge PR cosmos#6618: backport 0.39.0 (launchpad): cherry pick cosmos#5839

* launchpad: bump tendermint to v0.33.6 (cosmos#6673)

* launchpad: bump tendermint to v0.33.6

* cha-cha-cha

* fix types.ChainAnteDecorators() panic (cosmos#5742) (cosmos#6669)

ChainAnteDecorators() panics when no arguments are supplied.
This change its behaviour and the function now returns a nil
AnteHandler in case no AnteDecorator instances are supplied.

Closes: cosmos#5741

* launchpad: register MsgFundCommunityPool to distribution codec (cosmos#6675)

Closes: cosmos#6210

* client: backport IBC additions (cosmos#6682)

* launchpad: backport cliCtx.QueryABCI

* add prove flag

* Save account for multi sending (cosmos#6674)

Include changes from PR cosmos#6283

Co-authored-by: Federico Kunze <[email protected]>
Co-authored-by: Alessio Treglia <[email protected]>

* baseapp: fix sender events accumulation (cosmos#6683)

closes: cosmos#6306
original PR: cosmos#6307

* run go mod tidy

* add changelog line re: issue that was not included in v0.38.5

* Fix changelog

* add release notes

* update CODEOWNERS as per STABLE_RELEASES.md

Launchpad's Stable Release Managers team's current composition as
approved by @okwme (ICF):

* @alessio
* @clevinson
* @ethanfrey

* Add milestone's URL

* Revert "update CODEOWNERS as per STABLE_RELEASES.md"

This reverts commit f384592.

* fix typo

* add example patch

* launchpad: backport account sequence stuck (cosmos#6721)

Launchpad fix for cosmos#6287

* update release notes

* make explicit that the regression is fixed in 0.39

* update release notes

* Update CHANGELOG.md

* update release notes

@ethanfrey

* remove pruning info from changelog

They have been moved to NEWS.md

* Update NEWS.md

Co-authored-by: Alexander Bezobchuk <[email protected]>

* Mege PR cosmos#6749: auth: remove custom JSON marshaling

* mv NEWS.md -> RELEASE_NOTES.md

Self-explanatory naming convention.

* Update changelog

* add reference to gaia software upgrade

* x/staking: add call to iterator Close() method (cosmos#6794)

Original pull request: cosmos#6791

* [ci] fix linter (cosmos#6795)

* Merge PR cosmos#6842: Remove custom acc JSON marshl

* Backport cosmos#5671: Add a Flag for CORS (cosmos#6853)

This adds the --unsafe-cors flag functionality.

From: cosmos#5671
Co-authored-by: Marko <[email protected]>

* Merge PR cosmos#6855: Allow passing through Content-Type (needed for POST)

* Merge PR cosmos#6861: Revert "Backport 0.39.1: Remove Custom JSON Marshaling for Accounts"

* Merge PR cosmos#6869: Backport 0.39.1: Launchpad Migration cosmos#6829

* Update changelog and release notes

* Update x/auth/legacy/v0_39/types.go

* Update x/auth/legacy/v0_38/types.go

* Merge PR cosmos#6938: Fix v0.39 migrate test

* Merge PR cosmos#6937: Bump Tendermint to v0.33.7

* Update x/auth/legacy/v0_39/types.go

Co-authored-by: Alexander Bezobchuk <[email protected]>

* update RELEASE_NOTES.md

* update changelog

* test case with real genesis data (cosmos#6995)

* incorporate Ethan Frey's suggestion

* make format

* Update go mod

* Remove some ci files

* Modify data for test

Co-authored-by: Alexander Bezobchuk <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Alessio Treglia <[email protected]>
Co-authored-by: Ethan Frey <[email protected]>
Co-authored-by: Federico Kunze <[email protected]>
Co-authored-by: Jonathan Gimeno <[email protected]>
Co-authored-by: Marko <[email protected]>
Raumo0 pushed a commit to mapofzones/cosmos-sdk that referenced this issue Feb 13, 2022
* v0.38.4-RC1

* add release date

* client/keys/parse: honor config changes (cosmos#6340)

`keys parse` uses the global configuration before
before client applications have had a chance to
apply their settings.

This change adds a `GetSealedConfig()` helper
that waits for the config to be sealed before
returning it.

Fixes: cosmos#5091
Origin: cosmos@4e328d7
Author: Adam Bozanich <[email protected]>
Reviewed-by: Alessio Treglia <[email protected]>

* Merge PR cosmos#6505: Backport v0.38.5: IAVL Bump (RC) & Pruning Refactor

* Fix cl

* Fix cl

* Merge PR cosmos#6551: Ethanfrey/fix trace flag

* Merge PR cosmos#6552: Add sender info to bank transfer event

* Merge PR cosmos#6581: v0.38.5-RC1

* Add release date

* deps: bump IAVL to 0.14

* cl++

* Merge PR cosmos#6618: backport 0.39.0 (launchpad): cherry pick cosmos#5839

* launchpad: bump tendermint to v0.33.6 (cosmos#6673)

* launchpad: bump tendermint to v0.33.6

* cha-cha-cha

* fix types.ChainAnteDecorators() panic (cosmos#5742) (cosmos#6669)

ChainAnteDecorators() panics when no arguments are supplied.
This change its behaviour and the function now returns a nil
AnteHandler in case no AnteDecorator instances are supplied.

Closes: cosmos#5741

* launchpad: register MsgFundCommunityPool to distribution codec (cosmos#6675)

Closes: cosmos#6210

* client: backport IBC additions (cosmos#6682)

* launchpad: backport cliCtx.QueryABCI

* add prove flag

* Save account for multi sending (cosmos#6674)

Include changes from PR cosmos#6283

Co-authored-by: Federico Kunze <[email protected]>
Co-authored-by: Alessio Treglia <[email protected]>

* baseapp: fix sender events accumulation (cosmos#6683)

closes: cosmos#6306
original PR: cosmos#6307

* run go mod tidy

* add changelog line re: issue that was not included in v0.38.5

* Fix changelog

* add release notes

* update CODEOWNERS as per STABLE_RELEASES.md

Launchpad's Stable Release Managers team's current composition as
approved by @okwme (ICF):

* @alessio
* @clevinson
* @ethanfrey

* Add milestone's URL

* Revert "update CODEOWNERS as per STABLE_RELEASES.md"

This reverts commit f384592.

* fix typo

* add example patch

* launchpad: backport account sequence stuck (cosmos#6721)

Launchpad fix for cosmos#6287

* update release notes

* make explicit that the regression is fixed in 0.39

* update release notes

* Update CHANGELOG.md

* update release notes

@ethanfrey

* remove pruning info from changelog

They have been moved to NEWS.md

* Update NEWS.md

Co-authored-by: Alexander Bezobchuk <[email protected]>

* Mege PR cosmos#6749: auth: remove custom JSON marshaling

* mv NEWS.md -> RELEASE_NOTES.md

Self-explanatory naming convention.

* Update changelog

* add reference to gaia software upgrade

* x/staking: add call to iterator Close() method (cosmos#6794)

Original pull request: cosmos#6791

* [ci] fix linter (cosmos#6795)

* Merge PR cosmos#6842: Remove custom acc JSON marshl

* Backport cosmos#5671: Add a Flag for CORS (cosmos#6853)

This adds the --unsafe-cors flag functionality.

From: cosmos#5671
Co-authored-by: Marko <[email protected]>

* Merge PR cosmos#6855: Allow passing through Content-Type (needed for POST)

* Merge PR cosmos#6861: Revert "Backport 0.39.1: Remove Custom JSON Marshaling for Accounts"

* Merge PR cosmos#6869: Backport 0.39.1: Launchpad Migration cosmos#6829

* Update changelog and release notes

* Update x/auth/legacy/v0_39/types.go

* Update x/auth/legacy/v0_38/types.go

* Merge PR cosmos#6938: Fix v0.39 migrate test

* Merge PR cosmos#6937: Bump Tendermint to v0.33.7

* Update x/auth/legacy/v0_39/types.go

Co-authored-by: Alexander Bezobchuk <[email protected]>

* update RELEASE_NOTES.md

* update changelog

* test case with real genesis data (cosmos#6995)

* incorporate Ethan Frey's suggestion

* make format

* Update go mod

* Remove some ci files

* Modify data for test

* API server

* Port telemetry

* Initial metrics

* Fix measure

* Merge PR cosmos#6761: telemetry: use UTC() in wrappers

* Remove file

* Make format

* Revert changes to client

* Revert changes to client/lcd/root

* Revert renames in client

* Fix int64 panics

* Revert "Revert renames in client"

This reverts commit cc0a95d14c3212fa1c49f93789dd94e167427a57.

* Revert "Revert changes to client/lcd/root"

This reverts commit e3bb87bbacae13676c3a1f86f8d441d576b1f2ba.

* Revert "Revert changes to client"

This reverts commit 332cacba3242e503bcc4bffc3f6b25c1906efca4.

Co-authored-by: Alexander Bezobchuk <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Alessio Treglia <[email protected]>
Co-authored-by: Ethan Frey <[email protected]>
Co-authored-by: Federico Kunze <[email protected]>
Co-authored-by: Jonathan Gimeno <[email protected]>
Co-authored-by: Marko <[email protected]>
Co-authored-by: Adam Bozanich <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants