diff --git a/CHANGELOG.md b/CHANGELOG.md index af0680f97..0d84ebd47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -71,6 +71,7 @@ - [\#1050](https://github.com/cosmos/evm/pull/1050) Align precompile gas calculation with expected EVM gas semantics. - [\#1107](https://github.com/cosmos/evm/pull/1107) Skip StateDB commit error transactions during receipt conversion to prevent `invalid message index` errors in block RPCs. - [\#1216](https://github.com/cosmos/evm/pull/1216) Fix blocking on mempool event bus unsubscribe. +- [\#1227](https://github.com/cosmos/evm/issues/1227) Fix cosmos-pool proposal starvation under backlogs: carry the rechecked snapshot across heights, serve the carried snapshot when recheck lags, re-verify proposal txs not validated at the proposal base, and skip redundant sigverify on recheck. ## v0.6.0 diff --git a/evmd/app.go b/evmd/app.go index 204345c2e..50ebc644f 100644 --- a/evmd/app.go +++ b/evmd/app.go @@ -132,6 +132,13 @@ func init() { // manually update the power reduction by replacing micro (u) -> atto (a) evmos sdk.DefaultPowerReduction = utils.AttoPowerReduction + // Pin config-registry scope to skip per-call syscalls, see cosmos-sdk#26729 + if os.Getenv(sdk.EnvConfigScope) == "" { + if err := os.Setenv(sdk.EnvConfigScope, appName); err != nil { + panic(err) + } + } + defaultNodeHome = evmconfig.MustGetDefaultNodeHome() } diff --git a/evmd/mempool.go b/evmd/mempool.go index 55986ce2a..70b800c00 100644 --- a/evmd/mempool.go +++ b/evmd/mempool.go @@ -1,6 +1,8 @@ package evmd import ( + abci "github.com/cometbft/cometbft/abci/types" + evmmempool "github.com/cosmos/evm/mempool" "github.com/cosmos/evm/server" evmtypes "github.com/cosmos/evm/x/vm/types" @@ -53,10 +55,18 @@ func (app *EVMD) configureEVMMempool(appOpts servertypes.AppOptions, logger log. app.EVMMempool = mempool - // create ABCI handlers - prepareProposalHandler := baseapp. - NewDefaultProposalHandler(mempool, NewNoCheckProposalTxVerifier(app.BaseApp)). + // Re-run ante for any selected tx the mempool cannot prove was validated + // at the height this proposal builds on (see SnapshotVerifiedTxVerifier). + // The base comes from the ABCI request, not the notify-driven pin, which + // can lag a beat behind the last commit. + verifier := NewSnapshotVerifiedTxVerifier(app.BaseApp, mempool) + defaultProposalHandler := baseapp. + NewDefaultProposalHandler(mempool, verifier). PrepareProposalHandler() + prepareProposalHandler := func(ctx sdk.Context, req *abci.RequestPrepareProposal) (*abci.ResponsePrepareProposal, error) { + verifier.SetProposalBase(req.Height - 1) + return defaultProposalHandler(ctx, req) + } insertTxHandler := mempool.NewInsertTxHandler(app.TxDecode) reapTxsHandler := mempool.NewReapTxsHandler() diff --git a/evmd/tx_verifier.go b/evmd/tx_verifier.go index 224d3abe6..f221d2ebb 100644 --- a/evmd/tx_verifier.go +++ b/evmd/tx_verifier.go @@ -1,27 +1,44 @@ package evmd import ( + "sync/atomic" + + evmmempool "github.com/cosmos/evm/mempool" + "github.com/cosmos/cosmos-sdk/baseapp" sdk "github.com/cosmos/cosmos-sdk/types" ) -var _ baseapp.ProposalTxVerifier = &NoCheckProposalTxVerifier{} +var _ baseapp.ProposalTxVerifier = &SnapshotVerifiedTxVerifier{} -type NoCheckProposalTxVerifier struct { +// SnapshotVerifiedTxVerifier re-runs ante over a proposal candidate only when +// the mempool cannot show it was validated at the height the proposal builds +// on. The base is set per proposal from the ABCI request, so a lagging +// recheck pin fails closed into re-verification. +type SnapshotVerifiedTxVerifier struct { *baseapp.BaseApp + mempool *evmmempool.Mempool + + // proposalBase is the last committed height the in-flight proposal builds + // on (req.Height - 1), set by the prepare-proposal handler before txs are + // verified. Zero means unknown and re-verifies everything. + proposalBase atomic.Int64 +} + +func NewSnapshotVerifiedTxVerifier(b *baseapp.BaseApp, mempool *evmmempool.Mempool) *SnapshotVerifiedTxVerifier { + return &SnapshotVerifiedTxVerifier{BaseApp: b, mempool: mempool} } -func NewNoCheckProposalTxVerifier(b *baseapp.BaseApp) *NoCheckProposalTxVerifier { - return &NoCheckProposalTxVerifier{BaseApp: b} +// SetProposalBase records the height the next proposal builds on. +func (txv *SnapshotVerifiedTxVerifier) SetProposalBase(height int64) { + txv.proposalBase.Store(height) } -// PrepareProposalVerifyTx overrides the typical tx verification done in -// BaseApp's PrepareProposalHandler. The default PrepareProposalVerifyTx -// implementation encodes the tx to bytes, then calls runTx in 'checktx' mode, -// executing all antehandlers. -// -// We now override the implementation to only verify that the tx can be encoded -// to bytes, since we will guarantee that all txs selected are valid elsewhere. -func (txv *NoCheckProposalTxVerifier) PrepareProposalVerifyTx(tx sdk.Tx) ([]byte, error) { - return txv.TxEncode(tx) +// PrepareProposalVerifyTx encodes txs validated at the proposal's base height +// and defers to BaseApp's full ante verification for stale or unknown ones. +func (txv *SnapshotVerifiedTxVerifier) PrepareProposalVerifyTx(tx sdk.Tx) ([]byte, error) { + if base := txv.proposalBase.Load(); base > 0 && txv.mempool.ProposalTxValidatedAt(tx, uint64(base)) { + return txv.TxEncode(tx) + } + return txv.BaseApp.PrepareProposalVerifyTx(tx) } diff --git a/mempool/blockchain.go b/mempool/blockchain.go index 2c49042d5..98384ff78 100644 --- a/mempool/blockchain.go +++ b/mempool/blockchain.go @@ -47,6 +47,9 @@ type Blockchain struct { mu sync.RWMutex coinInfo atomic.Pointer[evmtypes.EvmCoinInfo] + // pinnedHeader caches the header for the current pin generation, setLatestContext invalidates it + pinnedHeader atomic.Pointer[types.Header] + testingCommitMu sync.RWMutex } @@ -259,6 +262,22 @@ func (b *Blockchain) setLatestContext(ctx sdk.Context) { b.mu.Lock() defer b.mu.Unlock() b.latestCtx = ctx + b.pinnedHeader.Store(nil) +} + +// PinnedHeader returns the current block header, cached per pin generation +// (headers only change at commit, and the pin refreshes right after). Use it +// on hot paths that tolerate pin-refresh granularity, CurrentBlock always +// rebuilds fresh. +func (b *Blockchain) PinnedHeader() *types.Header { + if h := b.pinnedHeader.Load(); h != nil { + return h + } + h := b.CurrentBlock() + if h != b.zeroHeader { + b.pinnedHeader.Store(h) + } + return h } // GetLatestContext returns the latest context as updated by the block, diff --git a/mempool/internal/heightsync/heightsync.go b/mempool/internal/heightsync/heightsync.go index 7e83b5fc1..5737aded1 100644 --- a/mempool/internal/heightsync/heightsync.go +++ b/mempool/internal/heightsync/heightsync.go @@ -116,6 +116,14 @@ type HeightSync[Store any] struct { // fields of the Store itself mu sync.RWMutex + // staleFallback makes GetStore return the current carried-forward Store + // (instead of nil) when it times out while still behind the target height. + // Entries in it were validated at a height <= target — possibly including + // txs a later block committed or invalidated — so consumers MUST re-verify + // anything that has to hold against latest state (evmd's PrepareProposal + // re-runs ante for entries not validated at the proposal base). + staleFallback bool + logger log.Logger } @@ -134,9 +142,26 @@ func New[Store any](startHeight *big.Int, reset func(logger log.Logger) *Store, return hs } +// WithStaleFallback enables the stale fallback (see the staleFallback field) +// and returns hs for chaining at construction. +func (hs *HeightSync[Store]) WithStaleFallback() *HeightSync[Store] { + hs.staleFallback = true + return hs +} + // StartNewHeight resets the HeightSync for a new height, overwriting the // previous Store with a fresh Store via the reset fn. func (hs *HeightSync[Store]) StartNewHeight(height *big.Int) { + hs.StartNewHeightFrom(height, nil) +} + +// StartNewHeightFrom starts a new height whose Store is derived from the +// previous height's Store via carry (nil carry means a fresh Store, as in +// StartNewHeight). Carrying lets a producer keep validated state across +// heights, so a pass cancelled before completion does not discard everything +// the previous height validated. carry runs while the HeightSync write lock is +// held and must not call back into the HeightSync. +func (hs *HeightSync[Store]) StartNewHeightFrom(height *big.Int, carry func(prev *Store) *Store) { hs.mu.Lock() defer hs.mu.Unlock() @@ -146,9 +171,13 @@ func (hs *HeightSync[Store]) StartNewHeight(height *big.Int) { panic(fmt.Errorf("height %s not ended before starting new height %s", hs.currentHeight.String(), height.String())) } - // create new Store for this height + // create the Store for this height, either fresh or carried forward hs.currentHeight = new(big.Int).Set(height) - hs.store = hs.reset(hs.logger) + if carry != nil { + hs.store = carry(hs.store) + } else { + hs.store = hs.reset(hs.logger) + } // close old channel and create new one to wake up consumers oldChan := hs.heightChanged @@ -192,6 +221,7 @@ func (hs *HeightSync[Store]) isHeightEnded() bool { // reached the target height, GetStore blocks until the height is reached or the // context expires. If the height is reached, GetStore waits for EndCurrentHeight // to be called (or for the context to expire) before returning. +// If HeightSync has already moved past the target height, GetStore returns nil. func (hs *HeightSync[Store]) GetStore(ctx context.Context, height *big.Int) *Store { genesis := big.NewInt(0) @@ -205,11 +235,18 @@ func (hs *HeightSync[Store]) GetStore(ctx context.Context, height *big.Int) *Sto cmp := hs.currentHeight.Cmp(height) - // should never see a situation where the HeightSync is ahead of - // the caller + // Heights can skip past the target when producer triggers coalesce + // under backlog. Target's Store is gone and future state must not be + // served, so return nil. if cmp > 0 { - defer hs.mu.RUnlock() // defer unlock since the panic will read - panic(fmt.Errorf("HeightSync.Get called for height %d, but current height is %d; cannot serve requests in the past", height, hs.currentHeight)) + current := hs.currentHeight.String() + hs.mu.RUnlock() + hs.logger.Warn( + "height sync moved past requested height, no store to serve", + "requested_height", height.String(), + "current_height", current, + ) + return nil } // if we're at the target height, wait for completion or timeout @@ -237,8 +274,7 @@ func (hs *HeightSync[Store]) GetStore(ctx context.Context, height *big.Int) *Sto } // current height is behind target, we cannot return the Store at the - // current height, so we must wait for the height to advance to the - // callers target height + // target height yet, so we wait for the height to advance. heightChangedChan := hs.heightChanged hs.mu.RUnlock() @@ -250,10 +286,24 @@ func (hs *HeightSync[Store]) GetStore(ctx context.Context, height *big.Int) *Sto // height changed, loop back to check if we've reached target continue case <-ctx.Done(): - // caller is done waiting, but we still do not have a Store at the - // correct height to return, return nil instead + // The caller is done waiting and the height sync never reached the + // target height. hsTimeout.Add(ctx, 1) - return nil + if !hs.staleFallback { + // caller opted out of stale results, return nil + return nil + } + // Rather than starve the caller (e.g. an empty block proposal), + // fall back to the carried-forward Store, under the staleFallback + // contract above. + hs.mu.RLock() + value := hs.store + // heights can skip past target while we waited, never serve future state + if hs.currentHeight.Cmp(height) > 0 { + value = nil + } + hs.mu.RUnlock() + return value } } } diff --git a/mempool/internal/heightsync/heightsync_test.go b/mempool/internal/heightsync/heightsync_test.go index ff747c70c..dc28b824f 100644 --- a/mempool/internal/heightsync/heightsync_test.go +++ b/mempool/internal/heightsync/heightsync_test.go @@ -156,7 +156,7 @@ func TestGetBehindByTwoHeights(t *testing.T) { }) } -func TestPanicOnOldHeight(t *testing.T) { +func TestNilOnOldHeight(t *testing.T) { hv := heightsync.New(big.NewInt(1), newTestValue, log.NewNopLogger()) hv.StartNewHeight(big.NewInt(1)) @@ -166,9 +166,7 @@ func TestPanicOnOldHeight(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) defer cancel() - require.Panics(t, func() { - hv.GetStore(ctx, big.NewInt(1)) - }) + require.Nil(t, hv.GetStore(ctx, big.NewInt(1))) } func TestStartNewHeightResetsValue(t *testing.T) { @@ -190,6 +188,87 @@ func TestStartNewHeightResetsValue(t *testing.T) { require.Empty(t, result.get()) } +func TestStartNewHeightFromCarriesStore(t *testing.T) { + hv := heightsync.New(big.NewInt(1), newTestValue, log.NewNopLogger()) + + hv.StartNewHeight(big.NewInt(1)) + hv.Do(func(s *testStore) { s.add("carried") }) + hv.EndCurrentHeight() + + // advance to height 2 carrying the previous store forward + hv.StartNewHeightFrom(big.NewInt(2), func(prev *testStore) *testStore { + return &testStore{items: prev.get()} + }) + hv.Do(func(s *testStore) { s.add("fresh") }) + hv.EndCurrentHeight() + + ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) + defer cancel() + + result := hv.GetStore(ctx, big.NewInt(2)) + require.NotNil(t, result) + require.Equal(t, []string{"carried", "fresh"}, result.get()) +} + +// With the stale-fallback option, GetStore returns the current store (at a +// height <= target) instead of nil when it times out behind the target height. +func TestStaleFallbackReturnsLastStore(t *testing.T) { + hv := heightsync.New(big.NewInt(1), newTestValue, log.NewNopLogger()).WithStaleFallback() + + hv.StartNewHeight(big.NewInt(1)) + hv.Do(func(s *testStore) { s.add("h1") }) + hv.EndCurrentHeight() + + // request height 2 but never advance to it: times out on the height-behind path + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + value := hv.GetStore(ctx, big.NewInt(2)) + require.NotNil(t, value) + require.Equal(t, []string{"h1"}, value.get()) +} + +// Without the option, the same height-behind timeout returns nil (the default). +func TestNoStaleFallbackReturnsNil(t *testing.T) { + hv := heightsync.New(big.NewInt(1), newTestValue, log.NewNopLogger()) + + hv.StartNewHeight(big.NewInt(1)) + hv.Do(func(s *testStore) { s.add("h1") }) + hv.EndCurrentHeight() + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + require.Nil(t, hv.GetStore(ctx, big.NewInt(2))) +} + +// The stale fallback must never serve a store from a height past the target: +// that is future state the target's proposal must not see, so GetStore +// returns nil instead. +func TestStaleFallbackDoesNotServePastTarget(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + hv := heightsync.New(big.NewInt(1), newTestValue, log.NewNopLogger()).WithStaleFallback() + + // park a getter for height 2 on the height-behind wait + getCtx, cancelGet := context.WithCancel(context.Background()) + defer cancelGet() + valueChan := make(chan *testStore) + go func() { + valueChan <- hv.GetStore(getCtx, big.NewInt(2)) + }() + time.Sleep(1 * time.Second) + + // Skip to height 3, cancelling the getter while carry holds the write + // lock: it wakes on the fallback path with the height past its target. + hv.StartNewHeightFrom(big.NewInt(3), func(prev *testStore) *testStore { + cancelGet() + return prev + }) + + require.Nil(t, <-valueChan, "fallback served a store from a height past the target") + }) +} + func TestConcurrentDo(t *testing.T) { hv := heightsync.New(big.NewInt(1), newTestValue, log.NewNopLogger()) diff --git a/mempool/iterator.go b/mempool/iterator.go index 392544878..cdd0f9c9c 100644 --- a/mempool/iterator.go +++ b/mempool/iterator.go @@ -294,7 +294,7 @@ func currentBaseFee(blockchain *Blockchain) *uint256.Int { return nil } - header := blockchain.CurrentBlock() + header := blockchain.PinnedHeader() if header == nil || header.BaseFee == nil { return nil } diff --git a/mempool/mempool.go b/mempool/mempool.go index 180127ab2..6941dcbce 100644 --- a/mempool/mempool.go +++ b/mempool/mempool.go @@ -200,11 +200,14 @@ func NewMempool( panic("tx pool should contain only legacypool") } + // Stale fallback: when the recheck loop falls behind consensus, serve the + // carried snapshot rather than an empty proposal; the proposal verifier + // re-runs ante for entries not validated at the proposal base. heightSync := heightsync.New( blockchain.CurrentBlock().Number, NewCosmosTxStore, logger.With("pool", "cosmos_recheck_mempool"), - ) + ).WithStaleFallback() reservationHandle := reservationTracker.NewHandle(cosmosReserverHandlerID, reserver.WithRefCounter()) @@ -336,7 +339,7 @@ func (m *Mempool) insert(tx sdk.Tx) (<-chan error, error) { ethTx := ethMsg.AsTransaction() // Reject txs below base fee up-front, which can never be included. - if baseFee := m.blockchain.CurrentBlock().BaseFee; baseFee != nil && ethTx.GasFeeCapIntCmp(baseFee) < 0 { + if baseFee := m.blockchain.PinnedHeader().BaseFee; baseFee != nil && ethTx.GasFeeCapIntCmp(baseFee) < 0 { return nil, sdkerrors.ErrInsufficientFee.Wrapf( "max fee per gas (%s) is lower than the base fee (%s)", ethTx.GasFeeCap(), baseFee, @@ -653,6 +656,19 @@ func (m *Mempool) cosmosIterator( return m.recheckCosmosPool.OrderedRecheckedTxs(ctx, height, bondDenom, baseFee) } +// ProposalTxValidatedAt reports whether the mempool's snapshot copy of tx was +// ante-validated at exactly base, the last committed height the proposal +// builds on. EVM txs always qualify (their snapshot has no stale fallback); +// carried or unknown cosmos txs report false and must be re-verified. Base is +// caller-supplied, so a lagging or dead notify path fails closed. +func (m *Mempool) ProposalTxValidatedAt(tx sdk.Tx, base uint64) bool { + if _, err := evmTxFromCosmosTx(tx); err == nil { + return true + } + height, ok := m.recheckCosmosPool.SnapshotValidatedAt(tx) + return ok && height == base +} + // TrackTx submits a tx to be tracked for its tx inclusion metrics. func (m *Mempool) TrackTx(hash common.Hash) error { return m.txTracker.Track(hash) diff --git a/mempool/recheck_pool.go b/mempool/recheck_pool.go index 5a38319a0..fd3ec540b 100644 --- a/mempool/recheck_pool.go +++ b/mempool/recheck_pool.go @@ -130,7 +130,7 @@ func NewRecheckMempool( blockchain, defaultCosmosPoolConfig, maxTxs, - onTransactionReplace(reapList, signerExtractor, reserver, logger), + onTransactionReplace(reapList, recheckedTxs, signerExtractor, reserver, logger), ) return &RecheckMempool{ @@ -329,7 +329,7 @@ func (m *RecheckMempool) TriggerRecheckSync(newHead *ethtypes.Header) { // RecheckedTxs returns the txs that have been rechecked for a height. The // RecheckMempool must be currently operating on this height (i.e. recheck has // been triggered on this height via TriggerRecheck). If height is in the past -// (TriggerRecheck has been called on height + 1), this will panic. If height +// (TriggerRecheck has been called on a later height), this returns nil. If height // is in the future, this will block until TriggerReset is called for height, // or the context times out. func (m *RecheckMempool) RecheckedTxs(ctx context.Context, height *big.Int) sdkmempool.Iterator { @@ -429,7 +429,11 @@ func (m *RecheckMempool) runRecheck(done chan struct{}, newHead *ethtypes.Header m.mu.Lock() defer m.mu.Unlock() - m.recheckedTxs.StartNewHeight(newHead.Number) + // Carry the validated set forward instead of resetting to an empty store, + // so a pass cancelled by the next block does not discard all progress and + // starve proposals. The pass prunes whatever became invalid; anything it + // has not re-validated keeps an old stamp and is re-verified at proposal. + m.recheckedTxs.StartNewHeightFrom(newHead.Number, (*CosmosTxStore).Clone) defer m.recheckedTxs.EndCurrentHeight() latestCtx, err := m.blockchain.GetLatestContext() @@ -439,6 +443,10 @@ func (m *RecheckMempool) runRecheck(done chan struct{}, newHead *ethtypes.Header } m.rechecker.Update(latestCtx, newHead) + // stamp the snapshot only once validation actually runs against this + // height's state (see CosmosTxStore.SetHeight) + m.recheckedTxs.Do(func(store *CosmosTxStore) { store.SetHeight(newHead.Number.Uint64()) }) + failedAtSequence := make(map[string]uint64) removeTxs := make([]sdk.Tx, 0) @@ -475,7 +483,10 @@ func (m *RecheckMempool) runRecheck(done chan struct{}, newHead *ethtypes.Header keepFuturesOnError := false if !invalidTx { ctx, write := m.rechecker.GetContext() - _, err := m.rechecker.RecheckCosmos(ctx, txn) + // Signatures were verified on insert and the bytes have not changed, + // so recheck mode lets sigverify skip the crypto, state-dependent + // checks (sequence, fees, balances) still run. + _, err := m.rechecker.RecheckCosmos(ctx.WithIsReCheckTx(true), txn) if err == nil { write() m.markTxRechecked(txn) @@ -498,6 +509,10 @@ func (m *RecheckMempool) runRecheck(done chan struct{}, newHead *ethtypes.Header } removeTxs = append(removeTxs, txn) + // Drop from the snapshot at detection: the removal loop below is + // skipped on cancellation. ExtMempool removal still waits for the + // loop (reservations, multi-signer identification). + m.markTxRemoved(txn) if keepFuturesOnError { iter = iter.Next() @@ -536,11 +551,23 @@ func (m *RecheckMempool) runRecheck(done chan struct{}, newHead *ethtypes.Header txsRemoved = len(removeTxs) } +// SnapshotValidatedAt reports the height the current snapshot's copy of txn +// was validated at, and false if the snapshot does not hold that exact tx. +func (m *RecheckMempool) SnapshotValidatedAt(txn sdk.Tx) (height uint64, ok bool) { + m.recheckedTxs.Do(func(store *CosmosTxStore) { height, ok = store.ValidatedAt(txn) }) + return height, ok +} + // markTxRechecked adds a tx into the height synced cosmos tx store. func (m *RecheckMempool) markTxRechecked(txn sdk.Tx) { m.recheckedTxs.Do(func(store *CosmosTxStore) { store.AddTx(txn) }) } +// markTxRemoved drops a tx from the height synced cosmos tx store. +func (m *RecheckMempool) markTxRemoved(txn sdk.Tx) { + m.recheckedTxs.Do(func(store *CosmosTxStore) { store.RemoveTx(txn) }) +} + // markTxInserted conservatively updates the current height snapshot for live inserts. // If the inserted tx replaces an existing tx, any other txs from the same sender with // a higher nonce is dropped and rebuilt by the next recheck. @@ -621,16 +648,21 @@ func cosmosPoolConfig( func onTransactionReplace( reapList *reaplist.ReapList, + recheckedTxs *heightsync.HeightSync[CosmosTxStore], signerExtractor sdkmempool.SignerExtractionAdapter, reserver *reserver.ReservationHandle, logger log.Logger, ) func(oldTx, newTx sdk.Tx) { - return func(oldTx, _ sdk.Tx) { + return func(oldTx, newTx sdk.Tx) { // tx is being replaced, we need to drop the tx that is going to be removed // from the reap list. we assume that the tx doing the replacing has // already been inserted into the reaplist via the insert. reapList.DropCosmosTx(oldTx) + // drop the replaced tx from the snapshot when its signer set differs + // from the replacement's (see CosmosTxStore.InvalidateReplaced) + recheckedTxs.Do(func(store *CosmosTxStore) { store.InvalidateReplaced(oldTx, newTx) }) + addrs, err := extractEVMAddresses(signerExtractor, oldTx) if err != nil { return diff --git a/mempool/recheck_pool_test.go b/mempool/recheck_pool_test.go index d57233455..12f0a4d75 100644 --- a/mempool/recheck_pool_test.go +++ b/mempool/recheck_pool_test.go @@ -19,6 +19,7 @@ import ( "google.golang.org/protobuf/proto" "github.com/cosmos/evm/crypto/ethsecp256k1" + "github.com/cosmos/evm/encoding" "github.com/cosmos/evm/mempool" "github.com/cosmos/evm/mempool/internal/heightsync" "github.com/cosmos/evm/mempool/internal/reaplist" @@ -30,6 +31,7 @@ import ( "cosmossdk.io/log/v2" sdkmath "cosmossdk.io/math" + "github.com/cosmos/cosmos-sdk/client" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" storetypes "github.com/cosmos/cosmos-sdk/store/v2/types" "github.com/cosmos/cosmos-sdk/testutil" @@ -726,6 +728,160 @@ func TestRecheckMempool_RecheckedTxs(t *testing.T) { } } +// A tx that fails recheck must leave the snapshot at detection time — the +// end-of-pass removal loop is skipped on cancellation. +func TestRecheckMempool_FailedTxDroppedBeforeRemovalLoop(t *testing.T) { + tracker := reserver.NewReservationTracker() + handle := tracker.NewHandle(1) + ctx := newRecheckTestContext() + bc := newTestBlockchain(t, ctx) + + const numTxs = 3 + + var failPass atomic.Bool + var calls atomic.Int32 + ready := make(chan struct{}) + gate := make(chan struct{}) + anteHandler := func(ctx sdk.Context, _ sdk.Tx, _ bool) (sdk.Context, error) { + if !failPass.Load() { + return ctx, nil + } + switch calls.Add(1) { + case 1: + // first rechecked tx fails + return ctx, errors.New("recheck failure") + case 2: + // second stalls the pass before its removal loop can run + ready <- struct{}{} + <-gate + } + return ctx, nil + } + + rc := newMockRechecker(ctx, anteHandler) + mp := mempool.NewRecheckMempool( + nil, 0, handle, rc, + newTestRecheckedTxs(), newTestReapList(), bc, log.NewNopLogger(), + ) + mp.Start(testHeader(0)) + defer mp.Close() + + for range numTxs { + key, _ := crypto.GenerateKey() + require.NoError(t, mp.Insert(ctx, newRecheckTestTx(t, key))) + } + mp.TriggerRecheckSync(testHeader(1)) + require.Len(t, collectIteratorTxs(mp.RecheckedTxs(context.Background(), big.NewInt(1))), numTxs) + + failPass.Store(true) + mp.TriggerRecheck(testHeader(2)) + <-ready // one tx has failed and the pass is stalled mid-iteration + + getCtx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + require.Len(t, collectIteratorTxs(mp.RecheckedTxs(getCtx, big.NewInt(2))), numTxs-1, + "failed tx must leave the snapshot before the removal loop runs") + + // let the stalled pass finish; remaining txs pass without signalling again + failPass.Store(false) + close(gate) +} + +// TestRecheckMempool_CarryForwardSurvivesCancellation verifies the fix for the +// cosmos-pool proposal starvation: a recheck pass carries the previous height's +// validated set forward, so a pass that is cancelled (or merely still running) +// before it validates anything does not present an empty snapshot to proposals. +// Before the fix, StartNewHeight reset the store to empty each height, so a +// pass that had not yet re-added txs exposed a zero-length snapshot. +func TestRecheckMempool_CarryForwardSurvivesCancellation(t *testing.T) { + tracker := reserver.NewReservationTracker() + handle := tracker.NewHandle(1) + ctx := newRecheckTestContext() + bc := newTestBlockchain(t, ctx) + + const numTxs = 5 + + var blockPass atomic.Bool + ready := make(chan struct{}) + gate := make(chan struct{}) + anteHandler := func(ctx sdk.Context, _ sdk.Tx, _ bool) (sdk.Context, error) { + if blockPass.Load() { + ready <- struct{}{} + <-gate + } + return ctx, nil + } + + rc := newMockRechecker(ctx, anteHandler) + mp := mempool.NewRecheckMempool( + nil, 0, handle, rc, + newTestRecheckedTxs(), newTestReapList(), bc, log.NewNopLogger(), + ) + mp.Start(testHeader(0)) + defer mp.Close() + + // Insert and validate a set of txs at height 1. + for range numTxs { + key, _ := crypto.GenerateKey() + require.NoError(t, mp.Insert(ctx, newRecheckTestTx(t, key))) + } + mp.TriggerRecheckSync(testHeader(1)) + require.Len(t, collectIteratorTxs(mp.RecheckedTxs(context.Background(), big.NewInt(1))), numTxs) + + // Start a height-2 pass that blocks on the very first ante call, before it + // has re-added any tx to the height-2 snapshot. + blockPass.Store(true) + mp.TriggerRecheck(testHeader(2)) + <-ready // the pass is now stalled having added nothing itself + + // The height-2 snapshot must already expose the carried-forward set. Use a + // short timeout since the (stalled) pass will not call EndCurrentHeight. + getCtx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + carried := collectIteratorTxs(mp.RecheckedTxs(getCtx, big.NewInt(2))) + require.Len(t, carried, numTxs, "carried-forward snapshot must not be empty mid-pass") + + // Let the stalled pass finish. blockPass is cleared before releasing the + // gate, so only the already-stalled ante was waiting: the remaining txs + // pass through without signalling ready again. + blockPass.Store(false) + close(gate) +} + +func newStartedRecheckMempool( + t *testing.T, + ctx sdk.Context, + cfg *sdkmempool.PriorityNonceMempoolConfig[sdkmath.Int], + ante sdk.AnteHandler, +) (*mempool.RecheckMempool, *heightsync.HeightSync[mempool.CosmosTxStore]) { + t.Helper() + + recheckedTxs := newTestRecheckedTxs() + mp := mempool.NewRecheckMempool( + cfg, 0, reserver.NewReservationTracker().NewHandle(1), newMockRechecker(ctx, ante), + recheckedTxs, newTestReapList(), newTestBlockchain(t, ctx), log.NewNopLogger(), + ) + mp.Start(testHeader(0)) + t.Cleanup(func() { + require.NoError(t, mp.Close()) + }) + return mp, recheckedTxs +} + +func setupEVMChainConfig(t *testing.T) client.TxConfig { + t.Helper() + + vmtypes.NewEVMConfigurator().ResetTestConfig() + require.NoError(t, vmtypes.SetChainConfig(vmtypes.DefaultChainConfig(constants.EighteenDecimalsChainID))) + require.NoError(t, vmtypes.NewEVMConfigurator(). + WithEVMCoinInfo(constants.ChainsCoinInfo[constants.EighteenDecimalsChainID]). + Configure()) + + encodingConfig := encoding.MakeConfig(constants.EighteenDecimalsChainID) + vmtypes.RegisterInterfaces(encodingConfig.InterfaceRegistry) + return encodingConfig.TxConfig +} + func TestRecheckMempool_RecheckedTxsBlocksUntilComplete(t *testing.T) { acc := newRecheckTestAccount(t) tracker := reserver.NewReservationTracker() @@ -788,12 +944,7 @@ func TestRecheckMempool_RecheckedTxsBlocksUntilComplete(t *testing.T) { } func TestRecheckMempool_RecheckerNoContextOnInsert(t *testing.T) { - // setup mocks for blockchain fetching latest block - vmtypes.NewEVMConfigurator().ResetTestConfig() - require.NoError(t, vmtypes.SetChainConfig(vmtypes.DefaultChainConfig(constants.EighteenDecimalsChainID))) - require.NoError(t, vmtypes.NewEVMConfigurator(). - WithEVMCoinInfo(constants.ChainsCoinInfo[constants.EighteenDecimalsChainID]). - Configure()) + setupEVMChainConfig(t) acc := newRecheckTestAccount(t) tracker := reserver.NewReservationTracker() @@ -826,7 +977,7 @@ func newRecheckTestTxWithNonce(t *testing.T, key *ecdsa.PrivateKey, nonce uint64 return &recheckTestTx{key: key, sequence: nonce} } -func newRecheckTestTxWithGasPrice(t *testing.T, key *ecdsa.PrivateKey, nonce uint64, gasPrice int64) sdk.Tx { +func newRecheckTestTxWithGasPrice(t *testing.T, key *ecdsa.PrivateKey, nonce uint64, gasPrice int64) *recheckTestTx { t.Helper() return &recheckTestTx{ key: key, @@ -1023,6 +1174,36 @@ func customReplacementConfig() *sdkmempool.PriorityNonceMempoolConfig[sdkmath.In } } +// A replaced tx with a different signer set than its replacement sits in a bucket +// InvalidateFrom(newTx) never visits, only replacement hook can drop it and txs stacked on its nonces. +func TestRecheckMempool_ReplacementWithDifferentSignerSetInvalidatesRechecked(t *testing.T) { + ctx := newRecheckTestContext() + mp, _ := newStartedRecheckMempool(t, ctx, customReplacementConfig(), noopAnteHandler) + + sender, err := crypto.GenerateKey() + require.NoError(t, err) + cosigner, err := crypto.GenerateKey() + require.NoError(t, err) + + // sender's nonce 5 co-signed by a second account, plus one stacked on it + coSigned := newCoSignedRecheckTestTx(t, sender, cosigner, 5, 0, 1) + dependent := newCoSignedRecheckTestTx(t, sender, cosigner, 6, 1, 1) + require.NoError(t, mp.Insert(ctx, coSigned)) + require.NoError(t, mp.Insert(ctx, dependent)) + require.Equal(t, []sdk.Tx{coSigned, dependent}, + collectIteratorTxs(mp.RecheckedTxs(context.Background(), big.NewInt(0)))) + + // replace at the same sender and nonce with a tx sender signs alone + replacement := newRecheckTestTxWithGasPrice(t, sender, 5, 2) + require.NoError(t, mp.Insert(ctx, replacement)) + + // Only the replacement may survive: the co-signed tx left the pool, and the + // dependent was validated on a nonce the replacement now owns. + rechecked := collectIteratorTxs(mp.RecheckedTxs(context.Background(), big.NewInt(0))) + require.Equal(t, []sdk.Tx{replacement}, rechecked, + "the replaced tx and its dependent must not linger in the snapshot") +} + func TestRecheckMempool_RecheckRebuildsSnapshotAfterReplacement(t *testing.T) { ctx := newRecheckTestContext() tracker := reserver.NewReservationTracker() @@ -1068,9 +1249,8 @@ func TestRecheckMempool_RecheckRebuildsSnapshotAfterReplacement(t *testing.T) { require.Equal(t, []sdk.Tx{tx3, replacement, tx5, tx6}, rechecked) } -// TestRecheckMempool_RecheckDropsFromReapList verifies that when a tx fails -// recheck and gets removed from the pool, it is also dropped from the reap -// list. Txs that pass recheck must remain reapable. +// To verify that when a tx fails recheck and gets removed from the pool, it is also dropped +// from reap list. Txs that pass recheck must remain reapable. func TestRecheckMempool_RecheckDropsFromReapList(t *testing.T) { ctx := newRecheckTestContext() tracker := reserver.NewReservationTracker() @@ -1158,18 +1338,30 @@ func TestRecheckMempool_ReplacementDropsFromReapList(t *testing.T) { require.Equal(t, expected, reaped[0]) } -// newRecheckTestTx creates a minimal sdk.Tx for unit testing RecheckMempool. func newRecheckTestTx(t *testing.T, key *ecdsa.PrivateKey) sdk.Tx { t.Helper() return &recheckTestTx{key: key} } -// recheckTestTx is a minimal sdk.Tx implementation for unit testing. type recheckTestTx struct { + key *ecdsa.PrivateKey + sequence uint64 + gas uint64 + fee sdk.Coins + cosigners []recheckTestSigner +} + +type recheckTestSigner struct { key *ecdsa.PrivateKey sequence uint64 - gas uint64 - fee sdk.Coins +} + +func (m *recheckTestTx) signers() []recheckTestSigner { + return append([]recheckTestSigner{{key: m.key, sequence: m.sequence}}, m.cosigners...) +} + +func recheckTestPubKey(key *ecdsa.PrivateKey) cryptotypes.PubKey { + return ðsecp256k1.PubKey{Key: crypto.CompressPubkey(&key.PublicKey)} } const recheckTestFeeDenom = "atest" @@ -1207,29 +1399,44 @@ func (m *recheckTestTx) FeeGranter() []byte { } func (m *recheckTestTx) GetSigners() ([][]byte, error) { - pubKeyBytes := crypto.CompressPubkey(&m.key.PublicKey) - pubKey := ðsecp256k1.PubKey{Key: pubKeyBytes} - return [][]byte{pubKey.Address().Bytes()}, nil + signers := make([][]byte, 0, len(m.cosigners)+1) + for _, s := range m.signers() { + signers = append(signers, recheckTestPubKey(s.key).Address().Bytes()) + } + return signers, nil } func (m *recheckTestTx) GetPubKeys() ([]cryptotypes.PubKey, error) { - pubKeyBytes := crypto.CompressPubkey(&m.key.PublicKey) - pubKey := ðsecp256k1.PubKey{Key: pubKeyBytes} - return []cryptotypes.PubKey{pubKey}, nil + pubKeys := make([]cryptotypes.PubKey, 0, len(m.cosigners)+1) + for _, s := range m.signers() { + pubKeys = append(pubKeys, recheckTestPubKey(s.key)) + } + return pubKeys, nil } func (m *recheckTestTx) GetSignaturesV2() ([]signingtypes.SignatureV2, error) { - pubKeyBytes := crypto.CompressPubkey(&m.key.PublicKey) - pubKey := ðsecp256k1.PubKey{Key: pubKeyBytes} - return []signingtypes.SignatureV2{ - { - PubKey: pubKey, - Sequence: m.sequence, - }, - }, nil + sigs := make([]signingtypes.SignatureV2, 0, len(m.cosigners)+1) + for _, s := range m.signers() { + sigs = append(sigs, signingtypes.SignatureV2{ + PubKey: recheckTestPubKey(s.key), + Sequence: s.sequence, + }) + } + return sigs, nil +} + +func newCoSignedRecheckTestTx( + t *testing.T, + key, cosigner *ecdsa.PrivateKey, + nonce, cosignerNonce uint64, + gasPrice int64, +) sdk.Tx { + t.Helper() + tx := newRecheckTestTxWithGasPrice(t, key, nonce, gasPrice) + tx.cosigners = []recheckTestSigner{{key: cosigner, sequence: cosignerNonce}} + return tx } -// recheckTestAccount holds test account data. type recheckTestAccount struct { key *ecdsa.PrivateKey address common.Address @@ -1253,12 +1460,10 @@ func noopAnteHandler(ctx sdk.Context, _ sdk.Tx, _ bool) (sdk.Context, error) { return ctx, nil } -// newTestRecheckedTxs creates a HeightSync[CosmosTxStore] for testing, starting at height 0. func newTestRecheckedTxs() *heightsync.HeightSync[mempool.CosmosTxStore] { return heightsync.New(big.NewInt(0), mempool.NewCosmosTxStore, log.NewNopLogger()) } -// collectIteratorTxs drains an sdkmempool.Iterator into a slice. func collectIteratorTxs(iter sdkmempool.Iterator) []sdk.Tx { var txs []sdk.Tx for iter != nil { diff --git a/mempool/tx_store.go b/mempool/tx_store.go index 07724ec63..3ebbd8be9 100644 --- a/mempool/tx_store.go +++ b/mempool/tx_store.go @@ -1,7 +1,8 @@ package mempool import ( - "fmt" + "maps" + "reflect" "slices" "strconv" "strings" @@ -16,8 +17,26 @@ import ( // CosmosTxStore is a set of cosmos transactions that can be added to or // removed from. type CosmosTxStore struct { - txs map[string]cosmosTxBucket - nextUnkeyed uint64 + txs map[string]cosmosTxBucket + nextUnkeyed uint64 + + // signerBuckets indexes signer -> keys of the buckets containing it, so + // shared-signer scans (InvalidateFrom, InvalidateReplaced) touch only + // matching buckets. Membership changes when a bucket is created or emptied. + signerBuckets map[string]map[string]struct{} + + // height is the chain height the snapshot is being (re)built for (see + // SetHeight). AddTx stamps entries with it; carried entries keep the stamp + // of the pass that validated them (see ValidatedAt). + height uint64 + + // byTx indexes stored pointer-typed txs to their validatedAt stamp, so + // ValidatedAt and AddTx re-adds skip signer extraction and key building. + // A same-nonce replacement is a different object, so it never vouches for + // the tx it replaced. Non-pointer txs are not indexed (interface map keys + // must be comparable) and take the slow paths. + byTx map[sdk.Tx]uint64 + logger log.Logger signerExtractor sdkmempool.SignerExtractionAdapter mu sync.RWMutex @@ -40,17 +59,93 @@ type cosmosTxWithMetadata struct { func NewCosmosTxStore(l log.Logger) *CosmosTxStore { return &CosmosTxStore{ txs: make(map[string]cosmosTxBucket), + signerBuckets: make(map[string]map[string]struct{}), + byTx: make(map[sdk.Tx]uint64), logger: l, signerExtractor: sdkmempool.NewDefaultSignerExtractionAdapter(), } } +// Clone returns a deep-enough copy of store for carrying the validated set forward +// into next height. The tx values are shared (immutable), but the +// bucket/index maps are copied so mutations on clone do not affect source. +func (s *CosmosTxStore) Clone() *CosmosTxStore { + s.mu.RLock() + defer s.mu.RUnlock() + + clone := &CosmosTxStore{ + txs: make(map[string]cosmosTxBucket, len(s.txs)), + signerBuckets: make(map[string]map[string]struct{}, len(s.signerBuckets)), + byTx: maps.Clone(s.byTx), + nextUnkeyed: s.nextUnkeyed, + height: s.height, + logger: s.logger, + signerExtractor: s.signerExtractor, + } + for signer, bucketKeys := range s.signerBuckets { + clone.signerBuckets[signer] = maps.Clone(bucketKeys) + } + for signerKey, bucket := range s.txs { + // Unkeyed txs are unremovable and get a fresh key on every AddTx, so a + // carried copy would duplicate once per pass; let each pass re-add them. + if signerKey == unkeyedSignerKey { + continue + } + clone.txs[signerKey] = cosmosTxBucket{ + txs: slices.Clone(bucket.txs), + signers: maps.Clone(bucket.signers), + } + } + return clone +} + +// SetHeight records the chain height the snapshot is being (re)built for. +// Each pass calls it once, after the rechecker context moves to that height's +// state, so AddTx stamps entries with the height their validation ran against. +func (s *CosmosTxStore) SetHeight(height uint64) { + s.mu.Lock() + defer s.mu.Unlock() + s.height = height +} + +// ValidatedAt returns the height the stored copy of tx was last validated at. +// The entry must be the same tx object — a same-nonce replacement does not +// vouch for the tx it replaced — so replaced or absent txs report false. +func (s *CosmosTxStore) ValidatedAt(tx sdk.Tx) (uint64, bool) { + if !isPointerTx(tx) { + return 0, false + } + s.mu.RLock() + defer s.mu.RUnlock() + height, ok := s.byTx[tx] + return height, ok +} + +// isPointerTx reports whether tx's dynamic type is a pointer. Only pointer +// txs enter byTx (the mempool pipeline shares one decoded object per tx, and +// interface map keys must be comparable); anything else takes the slow paths. +func isPointerTx(tx sdk.Tx) bool { + return reflect.ValueOf(tx).Kind() == reflect.Pointer +} + // AddTx adds a single tx to the store while constructing a validated snapshot. func (s *CosmosTxStore) AddTx(tx sdk.Tx) { s.mu.Lock() defer s.mu.Unlock() + indexable := isPointerTx(tx) + + // fast path: re-adding a known tx only refreshes its stamp — its bucket + // entry is already correct + if indexable { + if _, ok := s.byTx[tx]; ok { + s.byTx[tx] = s.height + return + } + } + storedTx := s.newCosmosTxWithMetadata(tx) + if storedTx.signerKey == "" { storedTx.signerKey = unkeyedSignerKey } @@ -58,21 +153,42 @@ func (s *CosmosTxStore) AddTx(tx sdk.Tx) { storedTx.txKey = s.newUnkeyedStoreKey() } + // unkeyed txs are unremovable, so they are never indexed + if storedTx.signerKey == unkeyedSignerKey { + indexable = false + } + + // bucket.txs is sorted by (nonceSum, txKey): overwrite an occupied slot — + // each recheck pass re-adds still-valid txs and the newest wins. bucket := s.txs[storedTx.signerKey] - for _, existing := range bucket.txs { - if existing.txKey == storedTx.txKey { - // this should never happen. panicking for safety - s.logger.Warn("attempted to add duplicate tx to CosmosTxStore", "key", storedTx.txKey) - return + i, found := slices.BinarySearchFunc(bucket.txs, storedTx, compareCosmosTxWithMetadata) + if found { + // a replacement occupies the replaced tx's slot; its stamp must not + // vouch for the tx it replaced + if old := bucket.txs[i].tx; isPointerTx(old) { + delete(s.byTx, old) } + bucket.txs[i] = storedTx + if indexable { + s.byTx[tx] = s.height + } + return } if bucket.signers == nil { bucket.signers = signerSetFromNonceMap(storedTx.nonceMap) + for signer := range bucket.signers { + if s.signerBuckets[signer] == nil { + s.signerBuckets[signer] = make(map[string]struct{}) + } + s.signerBuckets[signer][storedTx.signerKey] = struct{}{} + } } - bucket.txs = append(bucket.txs, storedTx) - slices.SortFunc(bucket.txs, compareCosmosTxWithMetadata) + bucket.txs = slices.Insert(bucket.txs, i, storedTx) s.txs[storedTx.signerKey] = bucket + if indexable { + s.byTx[tx] = s.height + } } // InvalidateFrom removes any stored tx that depends on the supplied tx's signer/nonces. @@ -99,30 +215,94 @@ func (s *CosmosTxStore) InvalidateFrom(tx sdk.Tx) int { return 0 } - removed := 0 - for signerKey, existingBucket := range s.txs { - if !bucketContainsAnySigner(existingBucket, storedTx.nonceMap) { - continue + return s.filterSignerBucketsLocked(storedTx.nonceMap, func(t cosmosTxWithMetadata) bool { + return invalidatesCosmosTx(t, storedTx.nonceMap) + }) +} + +// InvalidateReplaced removes a replaced tx (and txs validated on top of its +// nonces) when its signer set differs from the replacement's, which +// InvalidateFrom(newTx) cannot see in newTx's own bucket. Same-set +// replacements stay with InvalidateFrom. Returns the number of txs removed. +func (s *CosmosTxStore) InvalidateReplaced(oldTx, newTx sdk.Tx) int { + s.mu.Lock() + defer s.mu.Unlock() + + oldStored := s.newCosmosTxWithMetadata(oldTx) + newStored := s.newCosmosTxWithMetadata(newTx) + if oldStored.signerKey == "" || oldStored.txKey == "" || oldStored.signerKey == newStored.signerKey { + return 0 + } + + return s.filterSignerBucketsLocked(oldStored.nonceMap, func(t cosmosTxWithMetadata) bool { + return invalidatesCosmosTx(t, oldStored.nonceMap) + }) +} + +// RemoveTx removes a single tx from the store if present. It is the counterpart +// to AddTx used when a recheck pass drops a tx that became invalid: with a +// carried-forward store the tx would otherwise linger from the previous height. +// Returns true if a tx was removed. +func (s *CosmosTxStore) RemoveTx(tx sdk.Tx) bool { + s.mu.Lock() + defer s.mu.Unlock() + + storedTx := s.newCosmosTxWithMetadata(tx) + if storedTx.signerKey == "" || storedTx.txKey == "" { + // unkeyed txs are not addressable for targeted removal + return false + } + + removed := s.filterBucketLocked(storedTx.signerKey, s.txs[storedTx.signerKey], func(t cosmosTxWithMetadata) bool { + return t.txKey == storedTx.txKey + }) + return removed > 0 +} + +// filterBucketLocked removes every tx in the bucket at signerKey for which +// match returns true (dropping it from the byTx index too), deleting the +// bucket if it empties. Callers must hold s.mu. Returns the number of txs +// removed. +func (s *CosmosTxStore) filterBucketLocked(signerKey string, bucket cosmosTxBucket, match func(cosmosTxWithMetadata) bool) int { + next := slices.DeleteFunc(bucket.txs, func(t cosmosTxWithMetadata) bool { + if !match(t) { + return false } + if isPointerTx(t.tx) { + delete(s.byTx, t.tx) + } + return true + }) + removed := len(bucket.txs) - len(next) + if removed == 0 { + return 0 + } - next := existingBucket.txs[:0] - for _, existing := range existingBucket.txs { - if invalidatesCosmosTx(existing, storedTx.nonceMap) { - removed++ - continue + if len(next) == 0 { + delete(s.txs, signerKey) + for signer := range bucket.signers { + delete(s.signerBuckets[signer], signerKey) + if len(s.signerBuckets[signer]) == 0 { + delete(s.signerBuckets, signer) } - next = append(next, existing) } + return removed + } + bucket.txs = next + s.txs[signerKey] = bucket + return removed +} - clear(existingBucket.txs[len(next):]) - if len(next) == 0 { - delete(s.txs, signerKey) - continue +// filterSignerBucketsLocked removes every tx matching match from the buckets sharing a signer +// with nonceMap, via signer index. Callers must hold s.mu. Returns the number of txs removed. +func (s *CosmosTxStore) filterSignerBucketsLocked(nonceMap map[string]uint64, match func(cosmosTxWithMetadata) bool) int { + removed := 0 + for signer := range nonceMap { + // repeat visits of a multi-signer bucket match nothing + for signerKey := range s.signerBuckets[signer] { + removed += s.filterBucketLocked(signerKey, s.txs[signerKey], match) } - existingBucket.txs = next - s.txs[signerKey] = existingBucket } - return removed } @@ -155,13 +335,23 @@ func cosmosTxSignerSetKey(nonceMap map[string]uint64) string { return b.String() } +// txKeyZeroPad left-pads nonces to the width of MaxUint64 so the string +// ordering of keys matches numeric nonce ordering. +const txKeyZeroPad = "00000000000000000000" + func cosmosTxKey(nonceMap map[string]uint64) string { var b strings.Builder for i, k := range sortedSignerKeys(nonceMap) { if i > 0 { b.WriteByte('|') } - fmt.Fprintf(&b, "%s/%020d", k, nonceMap[k]) + // equivalent to fmt.Fprintf(&b, "%s/%020d", ...) without fmt's + // reflection; this runs for every tx added on every recheck pass + nonce := strconv.FormatUint(nonceMap[k], 10) + b.WriteString(k) + b.WriteByte('/') + b.WriteString(txKeyZeroPad[:len(txKeyZeroPad)-len(nonce)]) + b.WriteString(nonce) } return b.String() @@ -226,15 +416,6 @@ func signerSetFromNonceMap(nonceMap map[string]uint64) map[string]struct{} { return signers } -func bucketContainsAnySigner(bucket cosmosTxBucket, thresholds map[string]uint64) bool { - for signer := range thresholds { - if _, ok := bucket.signers[signer]; ok { - return true - } - } - return false -} - func compareCosmosTxWithMetadata(a, b cosmosTxWithMetadata) int { if a.nonceSum < b.nonceSum { return -1 diff --git a/mempool/tx_store_test.go b/mempool/tx_store_test.go index f76510dcd..48aeeff40 100644 --- a/mempool/tx_store_test.go +++ b/mempool/tx_store_test.go @@ -1,6 +1,7 @@ package mempool import ( + "slices" "testing" "github.com/ethereum/go-ethereum/crypto" @@ -252,6 +253,233 @@ func TestCosmosTxStoreIteratorSnapshotIsolation(t *testing.T) { require.Equal(t, 2, count) } +func newPubKeyBytes(t *testing.T) []byte { + t.Helper() + key, err := crypto.GenerateKey() + require.NoError(t, err) + return crypto.CompressPubkey(&key.PublicKey) +} + +// signerKeyOf returns the key the store indexes a signer under: its account +// address, not the pubkey bytes the mocks are constructed from. +func signerKeyOf(pubKeyBytes []byte) string { + return string((ðsecp256k1.PubKey{Key: pubKeyBytes}).Address().Bytes()) +} + +// requireSignerIndexConsistent rebuilds the signer index from the buckets and +// compares: a missing entry hides a bucket from shared-signer scans, a stale +// one leaks. +func requireSignerIndexConsistent(t *testing.T, store *CosmosTxStore) { + t.Helper() + + want := make(map[string]map[string]struct{}) + for signerKey, bucket := range store.txs { + for signer := range bucket.signers { + if want[signer] == nil { + want[signer] = make(map[string]struct{}) + } + want[signer][signerKey] = struct{}{} + } + } + require.Equal(t, want, store.signerBuckets) +} + +func TestCosmosTxStoreRemoveTx(t *testing.T) { + store := NewCosmosTxStore(log.NewNopLogger()) + + signer := newPubKeyBytes(t) + tx0 := newKeyedMockTxWithPubKey(signer, 0) + tx1 := newKeyedMockTxWithPubKey(signer, 1) + + store.AddTx(tx0) + store.AddTx(tx1) + require.Equal(t, 2, store.Len()) + + require.True(t, store.RemoveTx(tx0)) + require.Equal(t, 1, store.Len()) + + // removing again is a no-op + require.False(t, store.RemoveTx(tx0)) + require.Equal(t, 1, store.Len()) + + // the remaining tx is the one we did not remove + require.True(t, store.RemoveTx(tx1)) + require.Equal(t, 0, store.Len()) +} + +func TestCosmosTxStoreCloneIsIndependent(t *testing.T) { + store := NewCosmosTxStore(log.NewNopLogger()) + + signer := newPubKeyBytes(t) + tx0 := newKeyedMockTxWithPubKey(signer, 0) + store.AddTx(tx0) + store.AddTx(newKeyedMockTxWithPubKey(signer, 1)) + store.AddTx(newKeyedMockTxWithPubKey(signer, 2)) + // remove one entry so the clone starts from a mutated source + require.True(t, store.RemoveTx(tx0)) + require.Equal(t, 2, store.Len()) + + clone := store.Clone() + require.Equal(t, store.Len(), clone.Len()) + + // mutating the clone must not affect the source + clone.AddTx(newKeyedMockTxWithPubKey(signer, 3)) + require.Equal(t, 2, store.Len()) + require.Equal(t, 3, clone.Len()) + + // mutating the source must not affect the clone + require.True(t, store.RemoveTx(newKeyedMockTxWithPubKey(signer, 1))) + require.Equal(t, 1, store.Len()) + require.Equal(t, 3, clone.Len()) +} + +func TestCosmosTxStoreCloneDropsUnkeyed(t *testing.T) { + store := NewCosmosTxStore(log.NewNopLogger()) + + store.AddTx(newMockTx(1)) // no signers: stored under the unkeyed bucket + store.AddTx(newKeyedMockTxWithPubKey(newPubKeyBytes(t), 0)) + require.Equal(t, 2, store.Len()) + + clone := store.Clone() + require.Equal(t, 1, clone.Len(), "clone must not carry the unkeyed bucket") + + // the re-add a recheck pass would perform yields exactly one copy again + clone.AddTx(newMockTx(1)) + require.Equal(t, 2, clone.Len()) +} + +// The signer index must name every bucket a signer sits in and nothing more: +// it decides which buckets a shared-signer scan visits. +func TestCosmosTxStoreSignerIndexTracksBuckets(t *testing.T) { + store := NewCosmosTxStore(log.NewNopLogger()) + + pubA, pubB := newPubKeyBytes(t), newPubKeyBytes(t) + keyA, keyB := signerKeyOf(pubA), signerKeyOf(pubB) + + store.AddTx(newKeyedMockTxWithPubKey(pubA, 0)) + store.AddTx(newKeyedMockTxWithPubKey(pubA, 1)) + store.AddTx(newKeyedMockTxWithPubKey(pubB, 0)) + store.AddTx(newMultiKeyedMockTx([][]byte{pubA, pubB}, []uint64{9, 9})) + store.AddTx(newMockTx(1)) // unkeyed: no signers, so it earns no index entry + require.Equal(t, 5, store.Len()) + requireSignerIndexConsistent(t, store) + + // each signer sits in its own bucket and in the one they share + require.Len(t, store.signerBuckets[keyA], 2) + require.Len(t, store.signerBuckets[keyB], 2) + + // Invalidating from A's nonce 0 empties both of A's buckets: its own and + // the shared one. B's own bucket is a different signer set and survives. + require.Equal(t, 3, store.InvalidateFrom(newKeyedMockTxWithPubKey(pubA, 0))) + requireSignerIndexConsistent(t, store) + require.NotContains(t, store.signerBuckets, keyA, "A has no bucket left to scan") + require.Len(t, store.signerBuckets[keyB], 1) + + // the index must not outlive the last keyed tx + require.True(t, store.RemoveTx(newKeyedMockTxWithPubKey(pubB, 0))) + requireSignerIndexConsistent(t, store) + require.Empty(t, store.signerBuckets) + require.Equal(t, 1, store.Len(), "the unkeyed tx is untouched by signer scans") +} + +// A clone's per-signer bucket sets must be copies: sharing them would let a +// prune on one height's store reach back into the previous height's. +func TestCosmosTxStoreCloneSignerIndexIsIndependent(t *testing.T) { + store := NewCosmosTxStore(log.NewNopLogger()) + + pubA, pubB := newPubKeyBytes(t), newPubKeyBytes(t) + store.AddTx(newMultiKeyedMockTx([][]byte{pubA, pubB}, []uint64{0, 0})) + store.AddTx(newKeyedMockTxWithPubKey(pubA, 3)) + store.AddTx(newMockTx(1)) // unkeyed: dropped by Clone, and indexed by neither + + clone := store.Clone() + requireSignerIndexConsistent(t, clone) + + // emptying every bucket the clone has must leave the source's index whole + require.Equal(t, 2, clone.InvalidateFrom(newMultiKeyedMockTx([][]byte{pubA, pubB}, []uint64{0, 0}))) + require.Equal(t, 0, clone.Len()) + require.Empty(t, clone.signerBuckets) + requireSignerIndexConsistent(t, store) + require.Len(t, store.signerBuckets[signerKeyOf(pubA)], 2) + require.Len(t, store.signerBuckets[signerKeyOf(pubB)], 1) +} + +// Two txs of one signer set can share a nonce sum; only AddTx's txKey +// tie-break keeps the second from overwriting the first. +func TestCosmosTxStoreAddTxDistinguishesEqualNonceSums(t *testing.T) { + store := NewCosmosTxStore(log.NewNopLogger()) + + pubA, pubB := newPubKeyBytes(t), newPubKeyBytes(t) + first := newMultiKeyedMockTx([][]byte{pubA, pubB}, []uint64{5, 0}) + second := newMultiKeyedMockTx([][]byte{pubA, pubB}, []uint64{0, 5}) + + store.AddTx(first) + store.AddTx(second) + require.Equal(t, 2, store.Len(), "equal nonce sums must not collapse into one slot") + require.ElementsMatch(t, []sdk.Tx{first, second}, store.Txs()) + requireSignerIndexConsistent(t, store) + + // a recheck pass re-adds both: each overwrites its own slot, no duplicates + store.AddTx(first) + store.AddTx(second) + require.Equal(t, 2, store.Len()) + require.ElementsMatch(t, []sdk.Tx{first, second}, store.Txs()) +} + +// Unkeyed store keys compare as strings ("unkeyed/10" < "unkeyed/9"), and +// AddTx's binary search relies on inserts landing in that same order. +func TestCosmosTxStoreUnkeyedInsertsStaySortedPastTen(t *testing.T) { + store := NewCosmosTxStore(log.NewNopLogger()) + + const count = 12 + for i := range count { + store.AddTx(newMockTx(i)) + } + + require.Equal(t, count, store.Len(), "every unkeyed tx gets its own slot") + require.True(t, slices.IsSortedFunc(store.txs[unkeyedSignerKey].txs, compareCosmosTxWithMetadata)) + requireSignerIndexConsistent(t, store) +} + +// A replaced multi-signer tx lives in a bucket InvalidateFrom(newTx) cannot +// see; InvalidateReplaced drops it (and its dependents) by its own identity. +func TestCosmosTxStoreInvalidateReplaced(t *testing.T) { + store := NewCosmosTxStore(log.NewNopLogger()) + + signerA := newPubKeyBytes(t) + signerB := newPubKeyBytes(t) + oldTx := newMultiKeyedMockTx([][]byte{signerA, signerB}, []uint64{5, 0}) + dependent := newMultiKeyedMockTx([][]byte{signerA, signerB}, []uint64{6, 1}) + newTx := newKeyedMockTxWithPubKey(signerA, 5) + + store.AddTx(oldTx) + store.AddTx(dependent) + require.Equal(t, 2, store.Len()) + + // same signer set is a no-op: InvalidateFrom owns that case + require.Equal(t, 0, store.InvalidateReplaced(oldTx, oldTx)) + require.Equal(t, 2, store.Len()) + + // different signer set drops the old tx and anything atop its nonces + require.Equal(t, 2, store.InvalidateReplaced(oldTx, newTx)) + require.Equal(t, 0, store.Len()) +} + +func TestCosmosTxStoreAddOverwritesSlot(t *testing.T) { + store := NewCosmosTxStore(log.NewNopLogger()) + + signer := newPubKeyBytes(t) + store.AddTx(newFeeKeyedMockTxWithPubKey(signer, 0, 1)) + store.AddTx(newFeeKeyedMockTxWithPubKey(signer, 0, 5)) // same slot, higher fee + + require.Equal(t, 1, store.Len()) + txs := store.Txs() + require.Len(t, txs, 1) + feeTx, ok := txs[0].(sdk.FeeTx) + require.True(t, ok) + require.Equal(t, sdk.NewInt64Coin(feeKeyedMockTxDenom, 5*100_000), feeTx.GetFee()[0]) +} + func TestCosmosTxStoreOrdersBucketByNonceSum(t *testing.T) { store := NewCosmosTxStore(log.NewNopLogger()) @@ -400,3 +628,100 @@ func TestCosmosTxStoreInvalidateFromMultiSignerEvictsSingleSigner(t *testing.T) require.ElementsMatch(t, []sdk.Tx{bobTx3, eveTx9}, store.Txs()) } + +func TestCosmosTxStoreValidatedAtTracksHeights(t *testing.T) { + store := NewCosmosTxStore(log.NewNopLogger()) + + tx := newKeyedMockTx(t, 5) + store.SetHeight(7) + store.AddTx(tx) + + // stamped with the height the pass validated it at + height, ok := store.ValidatedAt(tx) + require.True(t, ok) + require.Equal(t, uint64(7), height) + + // a carried clone keeps the stamp of the pass that validated the entry... + clone := store.Clone() + clone.SetHeight(8) + height, ok = clone.ValidatedAt(tx) + require.True(t, ok) + require.Equal(t, uint64(7), height) + + // ...until its pass re-adds the tx, which re-stamps it + clone.AddTx(tx) + height, ok = clone.ValidatedAt(tx) + require.True(t, ok) + require.Equal(t, uint64(8), height) + + // the source store is unaffected by the clone's re-stamp + height, ok = store.ValidatedAt(tx) + require.True(t, ok) + require.Equal(t, uint64(7), height) + + // an absent tx does not resolve + _, ok = clone.ValidatedAt(newKeyedMockTx(t, 6)) + require.False(t, ok) +} + +// A same-signer same-nonce replacement occupies the replaced tx's slot; its +// stamp must not vouch for the tx it replaced. +func TestCosmosTxStoreValidatedAtRejectsReplacement(t *testing.T) { + store := NewCosmosTxStore(log.NewNopLogger()) + + key, err := crypto.GenerateKey() + require.NoError(t, err) + signer := crypto.CompressPubkey(&key.PublicKey) + + replaced := newKeyedMockTxWithPubKey(signer, 3) + replacement := newKeyedMockTxWithPubKey(signer, 3) + + store.SetHeight(7) + store.AddTx(replaced) + store.SetHeight(8) + store.AddTx(replacement) // same (signer, nonce): overwrites the slot + + height, ok := store.ValidatedAt(replacement) + require.True(t, ok) + require.Equal(t, uint64(8), height) + + _, ok = store.ValidatedAt(replaced) + require.False(t, ok, "a replacement's stamp must not vouch for the replaced tx") +} + +// Every removal path must drop a tx from the identity index, or a dead entry +// would keep vouching for it. +func TestCosmosTxStoreValidatedAtDroppedOnRemoval(t *testing.T) { + key, err := crypto.GenerateKey() + require.NoError(t, err) + signer := crypto.CompressPubkey(&key.PublicKey) + + newStore := func(txs ...sdk.Tx) *CosmosTxStore { + store := NewCosmosTxStore(log.NewNopLogger()) + store.SetHeight(7) + for _, tx := range txs { + store.AddTx(tx) + } + return store + } + assertDropped := func(store *CosmosTxStore, txs ...sdk.Tx) { + t.Helper() + for _, tx := range txs { + _, ok := store.ValidatedAt(tx) + require.False(t, ok) + } + } + + tx3 := newKeyedMockTxWithPubKey(signer, 3) + tx4 := newKeyedMockTxWithPubKey(signer, 4) + + // RemoveTx + store := newStore(tx3) + require.True(t, store.RemoveTx(tx3)) + assertDropped(store, tx3) + + // InvalidateFrom removes the tx and its dependents + store = newStore(tx3, tx4) + require.Equal(t, 2, store.InvalidateFrom(tx3)) + assertDropped(store, tx3, tx4) +} diff --git a/server/server_app_options.go b/server/server_app_options.go index 59157518a..eaec8c667 100644 --- a/server/server_app_options.go +++ b/server/server_app_options.go @@ -114,7 +114,9 @@ func GetBlockGasLimit(appOpts servertypes.AppOptions, logger log.Logger) uint64 maxGas := genDoc.ConsensusParams.Block.MaxGas if maxGas == -1 { - logger.Warn("genesis max_gas is unlimited (-1), using max int64 block gas limit") + logger.Warn("genesis max_gas is unlimited (-1), using max int64 block gas limit; " + + "with app-side mempool an unbounded block can reap a whole tx backlog " + + "and exceed timeout_propose — set a finite consensus block.max_gas") return math.MaxInt64 } if maxGas < -1 { diff --git a/tests/integration/mempool/test_mempool_integration_abci.go b/tests/integration/mempool/test_mempool_integration_abci.go index ef5354587..6555773bd 100644 --- a/tests/integration/mempool/test_mempool_integration_abci.go +++ b/tests/integration/mempool/test_mempool_integration_abci.go @@ -1096,3 +1096,52 @@ func (s *IntegrationTestSuite) TestMultiPoolInteractions() { }) } } + +// TestProposalStarvationWhenRecheckLagsHeight checks that a proposal one height +// ahead of last recheck still draws from the pool, the situation a backlog +// creates by cancelling every recheck pass. +func (s *IntegrationTestSuite) TestProposalStarvationWhenRecheckLagsHeight() { + const numTxs = 8 + + s.TearDownTest() + s.SetupTest() + + mpool := s.network.App.GetMempool() + kMp, ok := mpool.(*evmmempool.Mempool) + if !ok { + s.T().Skip("EVM mempool not configured") + } + + // A backlog of cosmos txs, one per signer so none depend on another. + txs := make([]sdk.Tx, 0, numTxs) + for i := range numTxs { + txs = append(txs, s.createCosmosSendTx(s.keyring.GetKey(i), big.NewInt(1000000000))) + } + s.Require().NoError(s.insertTxs(txs)) + + // One full recheck pass at the current head, validating against real state. + head := kMp.GetBlockchain().CurrentBlock() + kMp.RecheckCosmosTxs(head) + + _, err := s.network.FinalizeBlock() + s.Require().NoError(err) + + proposedTxCount := func(height int64) int { + res, err := s.network.App.PrepareProposal(&abci.RequestPrepareProposal{ + MaxTxBytes: 1_000_000, + Height: height, + }) + s.Require().NoError(err) + return len(res.Txs) + } + + // Control: the height the snapshot was built for proposes everything. + current := s.network.GetContext().BlockHeight() + s.Require().Equal(numTxs, proposedTxCount(current+1), + "in-sync proposal should carry the whole validated backlog") + + // One height further on, which no recheck pass has reached. Pre-fix this + // served nothing and block came out empty. + s.Require().Equal(numTxs, proposedTxCount(current+2), + "proposal starved: the pool holds a validated backlog but served nothing") +} diff --git a/tests/integration/mempool/test_perf.go b/tests/integration/mempool/test_perf.go new file mode 100644 index 000000000..5bbe4a24c --- /dev/null +++ b/tests/integration/mempool/test_perf.go @@ -0,0 +1,67 @@ +package mempool + +import ( + "math/big" + "time" + + abci "github.com/cometbft/cometbft/abci/types" + + evmmempool "github.com/cosmos/evm/mempool" + + sdk "github.com/cosmos/cosmos-sdk/types" +) + +// TestPerfRecheckAndProposal logs recheck-pass and steady-state +// PrepareProposal timings over 200 cosmos txs. +func (s *IntegrationTestSuite) TestPerfRecheckAndProposal() { + const ( + signers = 20 + noncesPerSigner = 10 + iters = 10 + gasLimit = 200000 + ) + + kMp, ok := s.network.App.GetMempool().(*evmmempool.Mempool) + if !ok { + s.T().Skip("EVM mempool not configured") + } + + txs := make([]sdk.Tx, 0, signers*noncesPerSigner) + for nonce := range noncesPerSigner { + for i := range signers { + txs = append(txs, s.createCosmosSendTxWithNonceAndGas( + s.keyring.GetKey(i), uint64(nonce), big.NewInt(1000), gasLimit, big.NewInt(1000000000), + )) + } + } + s.Require().NoError(s.insertTxs(txs)) + + bench := func(fn func()) time.Duration { + fn() // warm + start := time.Now() + for range iters { + fn() + } + return time.Since(start) / iters + } + + head := kMp.GetBlockchain().CurrentBlock() + perPass := bench(func() { kMp.RecheckCosmosTxs(head) }) + + _, err := s.network.FinalizeBlock() + s.Require().NoError(err) + + height := s.network.GetContext().BlockHeight() + 1 + perProposal := bench(func() { + res, err := s.network.App.PrepareProposal(&abci.RequestPrepareProposal{ + MaxTxBytes: 10_000_000, + Height: height, + }) + s.Require().NoError(err) + s.Require().Len(res.Txs, len(txs)) + }) + + perTx := time.Duration(len(txs)) + s.T().Logf("PERF txs=%d recheck_pass=%v (%v/tx) proposal=%v (%v/tx)", + len(txs), perPass, perPass/perTx, perProposal, perProposal/perTx) +}