fix: use PriorityMempool with signer extractor to prevent missing signers error in tx execution #245

mmsqe · 2025-06-24T15:52:40Z

Description

to avoid tx must have at least one signer error when set positive mempool.max-txs
upstream the fix of repeated tx sender recovery is wastful
upstream the decouple of sender address verification from execution
for test info

Closes: #217

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...

tackled an existing issue or discussed with a team member
left instructions on how to review the changes
targeted the main branch

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...

added a relevant changelog entry to the Unreleased section in CHANGELOG.md
confirmed all author checklist items have been addressed
confirmed that this PR does not change production code
reviewed content
tested instructions (if applicable)
confirmed all CI checks have passed

add test

mmsqe · 2025-06-30T01:31:10Z

ante/evm/04_validate.go

 	txData evmtypes.TxData,
 	from sdktypes.AccAddress,
 ) error {
-	if from != nil {


@Vvaradinov We need signer recovery and extractor to make PriorityMempool work

vladjdk

My understanding of this PR is that we're moving the VRS signer extraction from the antehandler to the JSON-RPC. I think that's a good idea overall. I have a few small nits regarding the changes in that domain.

However, when it came to using the priority mempool, I couldn't actually get it to work properly. I'm still getting the same errors that I would get using the default mempool.

I don't mind using the priority nonce as an interim to this proposal, because the mempool doesn't hold transactions, and only sorts them, so there's no risk of p2p layer spam.

However, since it seems to behave the same way as the noop mempool with batched transactions, I don't really see a difference in setting it over noop in this case.

To be clear, I agree with the rest of the PR.

ante/evm/05_signature_verification.go

evmd/signer_test.go

vladjdk · 2025-07-04T16:39:22Z

proto/cosmos/evm/vm/v1/tx.proto

      [ (gogoproto.moretags) = "rlp:\"-\"", (amino.dont_omitempty) = true ];
-  // from is the ethereum signer address in hex format. This address value is
-  // checked against the address derived from the signature (V, R, S) using the
+  string deprecated_from = 4 [ deprecated = true ];


For V1, we're going to be using new protos, with the assumption that the first version is a fresh start. Let's just keep using this field and remove the deprecated tag.

I thought about this again, and the audited version of the code will still have the string version of from. Looks like we're going to still have to support this/deprecate it since this PR won't yet be in effect for the audited release. Apologies for my mistake here!

vladjdk · 2025-07-04T16:50:00Z

x/vm/types/msg.go

+}
+
+// GetSenderLegacy fallbacks to old behavior if From is empty, should be used by json-rpc
+func (msg *MsgEthereumTx) GetSenderLegacy(chainID *big.Int) (common.Address, error) {


Could we rename this from GetSenderLegacy to GetSenderFromSignature? Legacy implies that this is an old method that shouldn't be used. As far as I understand, it injects the sender bytes into the msg.From and should always be used by the JSON-RPC before it hits the antehandler.

vladjdk · 2025-07-04T16:50:34Z

x/vm/types/msg.go

+
+// GetSender convert the From field to common.Address
+// From should always be set, which is validated in ValidateBasic
+func (msg *MsgEthereumTx) GetSender() common.Address {


Like the below comment, let's rename this to GetSenderFromMsg, which will make it clear that msg.From cannot be nil

vladjdk · 2025-07-04T16:52:28Z

tests/integration/x/vm/state_transition_benchmark.go

 	}

-	m, err := msg.AsMessage(msgSigner, baseFee)
+	m, err := msg.AsMessage(baseFee)


Rename msg to signedMsg here

vladjdk · 2025-07-04T17:33:30Z

evmd/cmd/evmd/cmd/root.go

-
-		handler := baseapp.NewDefaultProposalHandler(mempool, app)
+		var mpool mempool.Mempool
+		if maxTxs := cast.ToInt(appOpts.Get(server.FlagMempoolMaxTxs)); maxTxs >= 0 {


I'm testing this with a Foundry script that uses transaction batching on this branch, and it seems like this is still failing to process out-of-order nonces.

Getting these errors:

- server returned an error response: error code -32000: invalid nonce; got 24, expected 23: invalid sequence: invalid sequence

Afaik, this PR is supposed to somewhat mitigate that. Is that not the case, or am I doing something wrong here?

Do you mind share the script? but we use different senders for priority test.

Anything that batch submits transactions using Forge would successfully replicate. Example: https://gist.github.com/vladjdk/cef7503d2dfe4c44eb3dbb1183947ec8

Got it, since need specify nonce from same sender to avoid out of order in priority mempool, I could try sender nonce mempool later.

PriorityNonceMempool don't handle out of order nonce, it only handles priority.

@yihuang based on the ADR it should order by nonce:

The follow rules are strictly observed while iterating the mempool to select transactions:

For a given sender their txs must be selected in nonce order.

A transaction with a higher priority is always selected before a transaction with a lower priority except when to do so would violate sender-nonce order.

Based on a quick look of the code, it looks like it orders by sender and nonce as a secondary key:
https://github.com/cosmos/cosmos-sdk/blob/main/types/mempool/priority_nonce.go#L241-L276.

@mmsqe sounds good. Priority nonce, however, should function the same as a sender nonce with an added priority dimension.

@vladjdk

the problem is check-tx state, if the txs comes in the order of nonces, the check-tx logic can run on the same check-tx state. but if out of order, the check-tx logic will fail and they won't get into mempool in the first place.

Vvaradinov · 2025-07-08T12:25:30Z

For the purpose we are using PriorityNonceMempool the signer extraction part works fine with the changes from this PR. I do still see the same issues as @vladjdk mentioned but it would be good to have this as interim supported since we are using this mempool over no-op and would prefer it to keep Babylon specific transactions prioritized over anything else.

vladjdk

Few more comments to resolve.

aljo242 · 2025-07-21T16:07:26Z

@vladjdk to revist

vladjdk

Lgtm. thanks!

vladjdk · 2025-07-24T21:09:19Z

Will merge when you fix the lints, tests and fix the merge conflict.

skosito · 2025-07-28T20:53:55Z

@mmsqe sorry for ping, i am curious is this issue that I opened in cosmos-sdk valid? cosmos/cosmos-sdk#24848

it seemed once i checked that this extractor is not used in proposal handler, so my question is does priority nonce mempool work like this?

EDIT: it seems like it is fixed here https://github.com/cosmos/cosmos-sdk/pull/25012/files

…e priority nonce mempool (#1430) This PR updates the evm to the latest commit hash that included the priority nonce mempool cosmos/evm#245. This adds the mandatory signer extractor to the mempool configuration. Additional changes in our testnet command, cosmos evm version and other configurations are needed.

…e priority nonce mempool (#1430) This PR updates the evm to the latest commit hash that included the priority nonce mempool cosmos/evm#245. This adds the mandatory signer extractor to the mempool configuration. Additional changes in our testnet command, cosmos evm version and other configurations are needed. (cherry picked from commit dac5bad)

…e priority nonce mempool (backport #1430) (#1575) This PR updates the evm to the latest commit hash that included the priority nonce mempool cosmos/evm#245. This adds the mandatory signer extractor to the mempool configuration. Additional changes in our testnet command, cosmos evm version and other configurations are needed. <hr>This is an automatic backport of pull request #1430 done by [Mergify](https://mergify.com). Co-authored-by: Vladislav Varadinov <[email protected]>

…ners error in tx execution (cosmos#245) * Problem: diff mempool max-txs trigger apphash mismatch add test * upstream repeated tx sender recovery is wastful cosmos#227 * sender address verification is not decoupled from execution cosmos#358 * fix main lint * Apply suggestions from code review * Problem: EVMChainID is not aligned in tests * lint * fix test * align cfg * eth signer not accurate in state machine cosmos#470 * keep deprecated * Update config.go --------- Co-authored-by: Alex | Interchain Labs <[email protected]> Co-authored-by: Vlad J <[email protected]>

Problem: diff mempool max-txs trigger apphash mismatch

ff37499

add test

mmsqe force-pushed the set_mempool branch from 9cf39b3 to ff37499 Compare June 24, 2025 16:31

mmsqe changed the title ~~Problem: diff mempool max-txs trigger apphash mismatch~~ fix: use PriorityMempool with signer extractor to prevent missing signers error in tx execution Jun 25, 2025

mmsqe marked this pull request as ready for review June 25, 2025 00:22

mmsqe added 3 commits June 27, 2025 18:17

upstream repeated tx sender recovery is wastful cosmos#227

ca7ea77

sender address verification is not decoupled from execution cosmos#358

3afced4

Merge remote-tracking branch 'origin/main' into set_mempool

4fea008

mmsqe commented Jun 30, 2025

View reviewed changes

vladjdk requested changes Jul 4, 2025

View reviewed changes

mmsqe added 3 commits July 5, 2025 09:33

Merge remote-tracking branch 'origin/main' into set_mempool

fcaefab

fix main lint

0cd62c7

Apply suggestions from code review

e859184

vladjdk approved these changes Jul 14, 2025

View reviewed changes

vladjdk requested changes Jul 14, 2025

View reviewed changes

mmsqe and others added 10 commits July 15, 2025 10:23

Problem: EVMChainID is not aligned in tests

0d1c13e

Merge remote-tracking branch 'origin/main' into fix_test

77df4d6

lint

70583c4

fix test

5f02fcf

Merge remote-tracking branch 'origin/main' into set_mempool

4030801

Merge remote-tracking branch 'origin/fix_test' into set_mempool

0cfbe9c

align cfg

bc17d7a

eth signer not accurate in state machine cosmos#470

143a5d6

keep deprecated

54ca560

Merge branch 'main' into set_mempool

4c29afb

vladjdk approved these changes Jul 24, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into set_mempool

15dc8f9

mmsqe requested a review from a team as a code owner July 25, 2025 01:34

Update config.go

42fb4ad

vladjdk added this pull request to the merge queue Jul 25, 2025

Merged via the queue into cosmos:main with commit 2934281 Jul 25, 2025
17 of 19 checks passed

Vvaradinov mentioned this pull request Jul 28, 2025

fix(evm): Bump cosmos evm to 0.41 and add eth signer extractor for the priority nonce mempool babylonlabs-io/babylon#1430

Merged

Vvaradinov mentioned this pull request Jul 29, 2025

evm: PriorityNonceMempool integration with ETH signer extractor babylonlabs-io/babylon#1440

Closed

mergify bot mentioned this pull request Aug 21, 2025

fix(evm): Bump cosmos evm to 0.41 and add eth signer extractor for the priority nonce mempool (backport #1430) babylonlabs-io/babylon#1575

Merged

fix: use PriorityMempool with signer extractor to prevent missing signers error in tx execution #245

fix: use PriorityMempool with signer extractor to prevent missing signers error in tx execution #245

Uh oh!

Conversation

mmsqe commented Jun 24, 2025 • edited by aljo242 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Author Checklist

Reviewers Checklist

Uh oh!

mmsqe Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladjdk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladjdk Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yihuang Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vvaradinov commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vladjdk left a comment

Choose a reason for hiding this comment

Uh oh!

aljo242 commented Jul 21, 2025

Uh oh!

vladjdk left a comment

Choose a reason for hiding this comment

Uh oh!

vladjdk commented Jul 24, 2025

Uh oh!

Uh oh!

skosito commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mmsqe commented Jun 24, 2025 •

edited by aljo242

Loading

mmsqe Jun 30, 2025 •

edited

Loading

vladjdk Jul 15, 2025 •

edited

Loading

yihuang Jul 15, 2025 •

edited

Loading

Vvaradinov commented Jul 8, 2025 •

edited

Loading

skosito commented Jul 28, 2025 •

edited

Loading