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

@@ -24,10 +24,6 @@ func ValidateMsg(
 	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

@@ -46,10 +46,11 @@ message MsgEthereumTx {
  // hash of the transaction in hex format
  string hash = 3
      [ (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

 	msg, baseFee, err := newEthMsgTx(nonce, address, krSigner, ethSigner, txType, data, accessList)
 	if err != nil {
 		return nil, err
 	}

-	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

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 #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 #470

143a5d6

keep deprecated

54ca560

Merge branch 'main' into set_mempool

4c29afb

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

Are you sure you want to change the base?

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!

Uh oh!

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