-
Notifications
You must be signed in to change notification settings - Fork 113
fix: use PriorityMempool with signer extractor to prevent missing signers error in tx execution #245
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: use PriorityMempool with signer extractor to prevent missing signers error in tx execution #245
Changes from 4 commits
ff37499
ca7ea77
3afced4
4fea008
fcaefab
0cd62c7
e859184
0d1c13e
77df4d6
70583c4
5f02fcf
4030801
0cfbe9c
bc17d7a
143a5d6
54ca560
4c29afb
15dc8f9
42fb4ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,11 +34,12 @@ import ( | |
| "github.com/cosmos/cosmos-sdk/client/pruning" | ||
| "github.com/cosmos/cosmos-sdk/client/rpc" | ||
| "github.com/cosmos/cosmos-sdk/client/snapshot" | ||
| "github.com/cosmos/cosmos-sdk/server" | ||
| sdkserver "github.com/cosmos/cosmos-sdk/server" | ||
| servertypes "github.com/cosmos/cosmos-sdk/server/types" | ||
| simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| sdkmempool "github.com/cosmos/cosmos-sdk/types/mempool" | ||
| "github.com/cosmos/cosmos-sdk/types/mempool" | ||
| sdktestutil "github.com/cosmos/cosmos-sdk/types/module/testutil" | ||
| "github.com/cosmos/cosmos-sdk/types/tx/signing" | ||
| authcmd "github.com/cosmos/cosmos-sdk/x/auth/client/cli" | ||
|
|
@@ -315,10 +316,19 @@ func newApp( | |
|
|
||
| // Set up the required mempool and ABCI proposal handlers for Cosmos EVM | ||
| baseappOptions = append(baseappOptions, func(app *baseapp.BaseApp) { | ||
| mempool := sdkmempool.NoOpMempool{} | ||
| app.SetMempool(mempool) | ||
|
|
||
| handler := baseapp.NewDefaultProposalHandler(mempool, app) | ||
| var mpool mempool.Mempool | ||
| if maxTxs := cast.ToInt(appOpts.Get(server.FlagMempoolMaxTxs)); maxTxs >= 0 { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: Afaik, this PR is supposed to somewhat mitigate that. Is that not the case, or am I doing something wrong here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mind share the script? but we use different senders for priority test.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anything that batch submits transactions using Forge would successfully replicate. Example: https://gist.github.com/vladjdk/cef7503d2dfe4c44eb3dbb1183947ec8
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it, since need specify nonce from same sender to avoid out of order in priority mempool, I could try sender nonce mempool later.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yihuang based on the ADR it should order by nonce:
Based on a quick look of the code, it looks like it orders by sender and nonce as a secondary key: @mmsqe sounds good. Priority nonce, however, should function the same as a sender nonce with an added priority dimension.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| // Setup Mempool and Proposal Handlers | ||
| mpool = mempool.NewPriorityMempool(mempool.PriorityNonceMempoolConfig[int64]{ | ||
| TxPriority: mempool.NewDefaultTxPriority(), | ||
| SignerExtractor: evmd.NewEthSignerExtractionAdapter(mempool.NewDefaultSignerExtractionAdapter()), | ||
| MaxTx: maxTxs, | ||
| }) | ||
| } else { | ||
| mpool = mempool.NoOpMempool{} | ||
| } | ||
| app.SetMempool(mpool) | ||
| handler := baseapp.NewDefaultProposalHandler(mpool, app) | ||
| app.SetPrepareProposal(handler.PrepareProposalHandler()) | ||
| app.SetProcessProposal(handler.ProcessProposalHandler()) | ||
| }) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| package evmd | ||
|
|
||
| import ( | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| mempool "github.com/cosmos/cosmos-sdk/types/mempool" | ||
| authante "github.com/cosmos/cosmos-sdk/x/auth/ante" | ||
| evmtypes "github.com/cosmos/evm/x/vm/types" | ||
| ) | ||
|
|
||
| var _ mempool.SignerExtractionAdapter = EthSignerExtractionAdapter{} | ||
|
|
||
| // EthSignerExtractionAdapter is the default implementation of SignerExtractionAdapter. It extracts the signers | ||
| // from a cosmos-sdk tx via GetSignaturesV2. | ||
| type EthSignerExtractionAdapter struct { | ||
| fallback mempool.SignerExtractionAdapter | ||
| } | ||
|
|
||
| // NewEthSignerExtractionAdapter constructs a new EthSignerExtractionAdapter instance | ||
| func NewEthSignerExtractionAdapter(fallback mempool.SignerExtractionAdapter) EthSignerExtractionAdapter { | ||
| return EthSignerExtractionAdapter{fallback} | ||
| } | ||
|
|
||
| // GetSigners implements the Adapter interface | ||
| // NOTE: only the first item is used by the mempool | ||
| func (s EthSignerExtractionAdapter) GetSigners(tx sdk.Tx) ([]mempool.SignerData, error) { | ||
| if txWithExtensions, ok := tx.(authante.HasExtensionOptionsTx); ok { | ||
| opts := txWithExtensions.GetExtensionOptions() | ||
| if len(opts) > 0 && opts[0].GetTypeUrl() == "/cosmos.evm.vm.v1.ExtensionOptionsEthereumTx" { | ||
| for _, msg := range tx.GetMsgs() { | ||
| if ethMsg, ok := msg.(*evmtypes.MsgEthereumTx); ok { | ||
| return []mempool.SignerData{ | ||
| mempool.NewSignerData( | ||
| ethMsg.GetFrom(), | ||
| ethMsg.AsTransaction().Nonce(), | ||
| ), | ||
| }, nil | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return s.fallback.GetSigners(tx) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| package evmd_test | ||
|
|
||
| import ( | ||
| "math/big" | ||
| "testing" | ||
|
|
||
| codectypes "github.com/cosmos/cosmos-sdk/codec/types" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| mempool "github.com/cosmos/cosmos-sdk/types/mempool" | ||
| "github.com/cosmos/evm/evmd" | ||
| "github.com/cosmos/evm/x/vm/types" | ||
| "github.com/stretchr/testify/require" | ||
| protov2 "google.golang.org/protobuf/proto" | ||
| ) | ||
|
|
||
| type mockFallback struct { | ||
| called bool | ||
| } | ||
|
|
||
| func (m *mockFallback) GetSigners(tx sdk.Tx) ([]mempool.SignerData, error) { | ||
| m.called = true | ||
| return []mempool.SignerData{mempool.NewSignerData(sdk.AccAddress("fallback"), 1)}, nil | ||
| } | ||
|
|
||
| type mockHasExtOptions struct { | ||
| msg sdk.Msg | ||
| } | ||
|
|
||
| func (m *mockHasExtOptions) GetMsgs() []sdk.Msg { return []sdk.Msg{m.msg} } | ||
| func (m *mockHasExtOptions) GetMsgsV2() ([]protov2.Message, error) { | ||
| return []protov2.Message{}, nil | ||
| } | ||
| func (m *mockHasExtOptions) GetExtensionOptions() []*codectypes.Any { | ||
| return []*codectypes.Any{ | ||
| { | ||
| TypeUrl: "/cosmos.evm.vm.v1.ExtensionOptionsEthereumTx", | ||
| Value: []byte{}, | ||
| }, | ||
| } | ||
| } | ||
| func (m *mockHasExtOptions) GetNonCriticalExtensionOptions() []*codectypes.Any { return nil } | ||
|
|
||
| func TestGetSigners(t *testing.T) { | ||
| ethAddr := sdk.AccAddress("ethsigner") | ||
| evmTx := &types.EvmTxArgs{ | ||
| ChainID: big.NewInt(100), | ||
| Nonce: 1, | ||
| Amount: big.NewInt(10), | ||
| GasLimit: 100000, | ||
| GasPrice: big.NewInt(150), | ||
| GasFeeCap: big.NewInt(200), | ||
| } | ||
| ethMsg := types.NewTx(evmTx) | ||
| ethMsg.From = ethAddr.Bytes() | ||
| txWithEth := &mockHasExtOptions{ | ||
| msg: ethMsg, | ||
| } | ||
| fallback := &mockFallback{} | ||
| adapter := evmd.NewEthSignerExtractionAdapter(fallback) | ||
| signers, err := adapter.GetSigners(txWithEth) | ||
| require.NoError(t, err) | ||
| require.Equal(t, []mempool.SignerData{{Signer: signers[0].Signer, Sequence: signers[0].Sequence}}, signers) | ||
mmsqe marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| require.False(t, fallback.called) | ||
|
|
||
| fallback = &mockFallback{} | ||
| txWithEth = &mockHasExtOptions{} | ||
| adapter = evmd.NewEthSignerExtractionAdapter(fallback) | ||
| signers, err = adapter.GetSigners(txWithEth) | ||
| require.NoError(t, err) | ||
| fallbackSigners, _ := new(mockFallback).GetSigners(txWithEth) | ||
| require.Equal(t, fallbackSigners, signers) | ||
| require.True(t, fallback.called) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 ]; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about this again, and the audited version of the code will still have the string version of |
||
| // from is the bytes of ethereum signer address. This address value is checked | ||
| // against the address derived from the signature (V, R, S) using the | ||
| // secp256k1 elliptic curve | ||
| string from = 4; | ||
| bytes from = 5; | ||
| } | ||
|
|
||
| // LegacyTx is the transaction data of regular Ethereum transactions. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Vvaradinov We need signer recovery and extractor to make PriorityMempool work