Skip to content

feat(mempool): proxy app mempool implementation#9

Open
traviolus wants to merge 5 commits intorelease/v0.50.xfrom
feat/proxy-mempool
Open

feat(mempool): proxy app mempool implementation#9
traviolus wants to merge 5 commits intorelease/v0.50.xfrom
feat/proxy-mempool

Conversation

@traviolus
Copy link

@traviolus traviolus commented Feb 5, 2026

Description

Closes: #XXXX


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

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • run make lint and make test
  • confirmed all CI checks have passed

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

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

Summary by CodeRabbit

Release Notes

  • New Features

    • Added application-level mempool with transaction queue management and per-sender capacity limits
    • Enabled event propagation between mempool and consensus layer for improved transaction handling
    • Implemented dual transaction validation paths for optimized processing workflows
  • Bug Fixes

    • Corrected sequence validation behavior in transaction check mode
  • Tests

    • Added comprehensive test coverage for mempool operations and validation flows

@coderabbitai
Copy link

coderabbitai bot commented Feb 5, 2026

Walkthrough

This change introduces a ProxyAppMempool implementation with per-sender transaction queuing, queue promotion logic, minimal ante handler for efficient CheckTx processing, and infrastructure for mempool event propagation between the app and CometBFT. It extends the Cosmos SDK app lifecycle with mempool event connectivity and dual ante-handler routing without modifying existing public signatures.

Changes

Cohort / File(s) Summary
Mempool Core Implementation
types/mempool/proxy_app_mempool.go, types/mempool/proxy_app_mempool_test.go
New ProxyAppMempool type with per-sender active/queued pools, nonce-based ordering, capacity enforcement, and CometBFT event emission. Includes Insert, Remove, Select, PromoteQueued, and CountTx methods. Comprehensive test suite covering insertion, removal, promotion, capacity limits, and event emission.
BaseApp Extensions
baseapp/abci.go, baseapp/baseapp.go
Added ConnectMempoolEvents and GetContextForSimulate public methods. Extended Commit flow to promote queued transactions via QueuePromoter interface. Added TxBytesContextKey support for mempool-aware transaction handling in check mode.
Server Startup Wiring
server/start.go, server/types/app.go
Introduced MempoolEventConnector interface and connectMempoolEvents helper function to wire CometBFT mempool event channel to the application during node startup.
Ante Handler Refactoring
x/auth/ante/minimal.go, x/auth/ante/minimal_test.go, x/auth/ante/sigverify.go, x/auth/ante/sigverify_test.go
New MinimalHandlerOptions struct and NewMinimalAnteHandler for CheckTx/ReCheckTx with simplified validation. NewDualAnteHandler routes minimal path during CheckTx and full path otherwise. SigVerificationDecorator now bypasses sequence check in CheckTx/ReCheckTx mode. Updated test expectations for sequence validation behavior.
Application Integration
simapp/app.go
Integrated ProxyAppMempool into BaseApp with MaxTxsPerSender=16 and MaxTotalTxs=500. Wired mempool to AccountKeeper. Configured app to use dual ante handler combining minimal and full paths.

Sequence Diagram(s)

sequenceDiagram
    actor Client
    participant CheckTxPath as App (CheckTx)
    participant MinimalHandler as Minimal AnteHandler
    participant ProxyMempool as ProxyMempool
    
    Client->>CheckTxPath: Send Transaction
    CheckTxPath->>MinimalHandler: Route via DualAnteHandler
    MinimalHandler->>MinimalHandler: Validate Account, Signature<br/>(Skip Sequence Check)
    MinimalHandler-->>CheckTxPath: Return Context
    CheckTxPath->>ProxyMempool: Insert Transaction
    ProxyMempool->>ProxyMempool: Route to Active/Queued<br/>based on Nonce
    ProxyMempool-->>CheckTxPath: Confirmation
    CheckTxPath-->>Client: CheckTx Response
Loading
sequenceDiagram
    participant App as App (FinalizeBlock/Commit)
    participant Mempool as ProxyMempool
    participant QueuePromoter
    participant AccountKeeper
    participant CometBFT as CometBFT Mempool
    
    App->>App: Finalize Block
    App->>QueuePromoter: PromoteQueued(ctx)
    QueuePromoter->>Mempool: Check Queued Transactions
    Mempool->>AccountKeeper: Get Current Sequence
    AccountKeeper-->>Mempool: Sequence Data
    Mempool->>Mempool: Promote Contiguous<br/>Chains to Active
    Mempool->>CometBFT: Emit EventTxInserted/Removed
    CometBFT-->>Mempool: Event Acknowledged
    Mempool-->>App: Promotion Complete
    App->>App: Commit State
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Poem

🐰 A mempool springs to life, so queued and bright,
With nonces neat and transactions tight,
Minimal checks for CheckTx's race,
Full validators in proper place,
Events hop and dance with CometBFT's grace! 🌟

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 53.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'feat(mempool): proxy app mempool implementation' clearly and concisely summarizes the main change, which is the implementation of a proxy app mempool across multiple files. This directly aligns with the primary focus of the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/proxy-mempool

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/go.mod (1)

81-81: ⚠️ Potential issue | 🟠 Major

Upgrade jose2go to v1.7.0 or later to resolve DoS vulnerabilities (GO-2025-4123 / GHSA-9mj6-hxhv-w67j).

The current version v1.6.0 is vulnerable to denial-of-service attacks via crafted JWE tokens. Both advisories are fixed in v1.7.0 and later releases.

🤖 Fix all issues with AI agents
In `@go.mod`:
- Line 105: Update the vulnerable dependency github.com/golang/glog in go.mod
from v1.2.3 to v1.2.4 or later (or add an appropriate replace directive) to
remediate CVE-2024-45339; edit the go.mod entry for github.com/golang/glog and
run `go mod tidy` to refresh module metadata so the updated version is used.
- Line 93: The go.mod currently pins github.com/dvsekhvalnov/jose2go at v1.6.0
which has a DoS vuln; update the indirect dependency to v1.7.0 by running a
module update (e.g., use "go get github.com/dvsekhvalnov/jose2go@v1.7.0" or
identify and bump the parent module that pulls it in via "go mod graph" and
update that parent), then run "go mod tidy" to refresh go.sum and ensure the
transitive upgrade is recorded; finally run tests and any vendoring steps to
confirm nothing breaks.

In `@simapp/go.mod`:
- Around line 3-5: The CI fails because simapp/go.mod specifies go 1.23.6 (and
toolchain go1.24.5) while GitHub Actions workflows are pinned to go-version:
"1.21"; update all workflows under .github/workflows to use go-version >=
"1.23.6" (or at least "1.23") so the runner can build modules requiring Go
1.23.6, or alternatively change simapp/go.mod's `go` directive back to a version
compatible with the workflows (e.g., `go 1.21` or `go 1.22`)—search for
go-version entries in the workflows and either raise them to >= 1.23.6 or adjust
simapp/go.mod accordingly.
- Line 81: Update the dependency line for github.com/dvsekhvalnov/jose2go from
v1.6.0 to v1.7.0 in simapp/go.mod to pick up the DEFLATE decompression limit
that fixes CVE-2025-63811; after changing the version, run go mod tidy (and
regenerate go.sum) and run the test suite/build to ensure no breakages from the
upgrade.

In `@tests/go.mod`:
- Around line 3-5: The project uses an indirect dependency
github.com/dvsekhvalnov/jose2go v1.6.0 which is vulnerable to CVE-2025-63811;
update that module to v1.7.0 or newer (e.g., run `go get
github.com/dvsekhvalnov/jose2go@v1.7.0` or add a direct require/replace to bump
it), then run `go mod tidy` to update tests/go.mod and tests/go.sum so the
indirect reference is upgraded and the vulnerability is resolved.

In `@types/mempool/proxy_app_mempool.go`:
- Around line 266-294: SelectBy currently holds m.mu.RLock while executing the
callback causing deadlocks if the callback calls Insert/Remove; change SelectBy
to snapshot the active txs like Select does (use m.sortedActiveSenders and copy
per-sender tx slices or build a [][]sdk.Tx/[]sdk.Tx for interleaved iteration),
release the lock, then iterate and invoke the callback without any lock held (or
simply delegate to the existing Select implementation by converting the required
arguments and calling Select), ensuring you remove the RLock/defer RUnlock and
no shared m.senderActive state is accessed while callbacks run.
- Around line 304-323: The Remove method currently deletes a tx from active or
queued pools but does not emit the promised EventTxRemoved; update
ProxyAppMempool.Remove to call m.pushEvent(cmtmempool.EventTxRemoved,
txEntry.txBytes) after a successful removal from either removeFromActive or
removeFromQueued (ensure you retrieve the removed txEntry/txBytes from those
helpers or adjust them to return the txEntry so you can push the event),
preserving the existing lock/unlock behavior and returning nil after emitting
the event.

In `@x/auth/ante/minimal.go`:
- Around line 47-55: NewDualAnteHandler should guard against nil inputs to avoid
panics: check if either minimal or full (the sdk.AnteHandler arguments) is nil
and return an sdk.AnteHandler that immediately returns a clear error (e.g.,
wrapped sdkerrors or fmt.Errorf) instead of calling a nil function;
alternatively perform the nil-check at NewDualAnteHandler construction time and
return a handler that returns an error describing which handler is nil. Ensure
the returned handler still routes to minimal for ctx.IsCheckTx()/IsReCheckTx()
and to full otherwise, but first validate the handlers and surface a descriptive
error if one is nil.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In `@types/mempool/proxy_app_mempool.go`:
- Around line 467-481: The removeFromActive function can leave a sender's active
nonces non-contiguous when a middle nonce is removed; modify removeFromActive
(and/or add a helper like rebuildActiveChain or demoteTrailingActive) so that
after sq.remove(nonce) it scans sq.txs (or the senderActive sequence structure)
for any higher nonces that now lack their predecessor and moves those trailing
txs back into the queued pool (senderQueue) or otherwise marks them inactive,
updating m.activeCount for each demoted tx and deleting m.senderActive[sender]
if no active txs remain; ensure the logic uses the same sender sequence types
(sq, senderActive, senderQueue, sq.txs, sq.remove) so the active chain remains
contiguous.
- Around line 151-158: The code currently may emit events with empty txBytes
when ctx.Value(TxBytesContextKey{}) yields no bytes and m.txEncoder is nil or
returns empty; update the logic in the function that reads txBytes (referencing
TxBytesContextKey{}, m.txEncoder and txBytes) to validate after the encoding
attempt and return a non-nil error (e.g., fmt.Errorf("empty tx bytes")) if
len(txBytes) == 0, so we never proceed to emit events or process a transaction
with empty bytes.
- Around line 406-425: extractSenderNonce currently calls GetSignaturesV2() then
uses sigs[0].PubKey.Address(), which can panic when PubKey is nil; instead call
sigTx.GetSigners() to obtain the sender address safely (use the first signer),
keep GetSignaturesV2() only to read the Sequence for nonce, validate both
returned slices are non-empty, and return the signer address
(string(signers[0])) and nonce (sigs[0].Sequence); also apply the same change to
the signer extraction code referenced in signer_extraction_adapter.go (replace
direct PubKey.Address() use with GetSigners()).

Comment on lines +151 to +158
// optimistically get raw tx bytes from context, should be set by baseapp
txBytes, _ := ctx.Value(TxBytesContextKey{}).([]byte)
if len(txBytes) == 0 && m.txEncoder != nil {
txBytes, err = m.txEncoder(tx)
if err != nil {
return fmt.Errorf("failed to encode tx: %w", err)
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Guard against empty txBytes to avoid invalid CometBFT event keys.
If the context lacks bytes and txEncoder is nil (or returns empty), events will be emitted with empty txs, causing key collisions and proxy-mempool desync. Consider returning an error when tx bytes are still empty after encoding attempts.

✅ Suggested guard
	txBytes, _ := ctx.Value(TxBytesContextKey{}).([]byte)
	if len(txBytes) == 0 && m.txEncoder != nil {
		txBytes, err = m.txEncoder(tx)
		if err != nil {
			return fmt.Errorf("failed to encode tx: %w", err)
		}
	}
+	if len(txBytes) == 0 {
+		return fmt.Errorf("tx bytes unavailable: set TxBytesContextKey or txEncoder")
+	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// optimistically get raw tx bytes from context, should be set by baseapp
txBytes, _ := ctx.Value(TxBytesContextKey{}).([]byte)
if len(txBytes) == 0 && m.txEncoder != nil {
txBytes, err = m.txEncoder(tx)
if err != nil {
return fmt.Errorf("failed to encode tx: %w", err)
}
}
// optimistically get raw tx bytes from context, should be set by baseapp
txBytes, _ := ctx.Value(TxBytesContextKey{}).([]byte)
if len(txBytes) == 0 && m.txEncoder != nil {
txBytes, err = m.txEncoder(tx)
if err != nil {
return fmt.Errorf("failed to encode tx: %w", err)
}
}
if len(txBytes) == 0 {
return fmt.Errorf("tx bytes unavailable: set TxBytesContextKey or txEncoder")
}
🤖 Prompt for AI Agents
In `@types/mempool/proxy_app_mempool.go` around lines 151 - 158, The code
currently may emit events with empty txBytes when ctx.Value(TxBytesContextKey{})
yields no bytes and m.txEncoder is nil or returns empty; update the logic in the
function that reads txBytes (referencing TxBytesContextKey{}, m.txEncoder and
txBytes) to validate after the encoding attempt and return a non-nil error
(e.g., fmt.Errorf("empty tx bytes")) if len(txBytes) == 0, so we never proceed
to emit events or process a transaction with empty bytes.

Comment on lines +406 to +425
// extractSenderNonce extracts the sender address and nonce from a transaction's first signature.
func extractSenderNonce(tx sdk.Tx) (sender string, nonce uint64, err error) {
sigTx, ok := tx.(signing.SigVerifiableTx)
if !ok {
return "", 0, fmt.Errorf("tx of type %T does not implement SigVerifiableTx", tx)
}

sigs, err := sigTx.GetSignaturesV2()
if err != nil {
return "", 0, err
}
if len(sigs) == 0 {
return "", 0, fmt.Errorf("tx must have at least one signature")
}

sender = string(sigs[0].PubKey.Address().Bytes())
nonce = sigs[0].Sequence

return sender, nonce, nil
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the file structure and find related code
find . -type f -name "*.go" | grep -E "(mempool|signature)" | head -20

Repository: initia-labs/cosmos-sdk

Length of output: 656


🏁 Script executed:

# Search for SignatureV2 definition
rg "type SignatureV2" --type go

Repository: initia-labs/cosmos-sdk

Length of output: 121


🏁 Script executed:

# Search for GetSignaturesV2 implementation
rg "func.*GetSignaturesV2" --type go -A 5

Repository: initia-labs/cosmos-sdk

Length of output: 2305


🏁 Script executed:

# Search for GetSigners implementation
rg "func.*GetSigners" --type go -A 5

Repository: initia-labs/cosmos-sdk

Length of output: 11235


🏁 Script executed:

# Look for PubKey nil checks in the codebase
rg "PubKey.*nil" --type go -B 2 -A 2

Repository: initia-labs/cosmos-sdk

Length of output: 50378


🌐 Web query:

Cosmos SDK SignatureV2 PubKey nil when pubkey already in context GetSigners vs PubKey.Address

💡 Result:

In the Cosmos SDK, signing.SignatureV2.PubKey being nil is normal and simply means the public key was not included in the tx’s signature/signer info. SigVerifiableTx.GetPubKeys() explicitly returns nil entries when the pubkey is not present in the tx data. [1]

That’s why you can see this mismatch:

  • GetSigners() returns signer addresses (derived from the Msg.GetSigners() / required signer addresses), even if no pubkey is included in the tx.
  • sig.PubKey in GetSignaturesV2() can be nil, because it reflects what was serialized into the tx, not what the node might already know.

So what should you use?

  • If you need “who must sign”, use tx.GetSigners() (addresses).
  • If you need the pubkey for verification / address derivation, you generally must fetch it from state (account) using the signer address:
    • The SDK’s tx processing does exactly this: it gets the account by signer address and uses acc.GetPubKey(), and if missing (e.g., simulation), it falls back to a placeholder pubkey. [2]

Why PubKey.Address() vs signer address can differ here

PubKey.Address() can only be computed if you actually have a pubkey. Accounts/addresses are derived from pubkeys conceptually, but the tx does not have to carry the pubkey every time. [3]

Practical implication

  • For accounts that already have a pubkey on-chain, txs often omit the pubkey to save bytes → SignatureV2.PubKey == nil is expected. [1]
  • For the first tx from a brand-new account (pubkey not on-chain yet), the tx must include the pubkey (otherwise the chain can’t verify it).

Sources: Cosmos SDK tx/pubkey semantics [1], BaseApp signature simulation/account pubkey lookup behavior [2], account/pubkey/address relationship [3].

[1] https://docs.cosmos.network/sdk/v0.47/learn/advanced/transactions
[2] https://cosmos-docs.mintlify.app/sdk/v0.53/learn/advanced/baseapp
[3] https://evm.cosmos.network/docs/sdk/v0.50/learn/beginner/accounts


🏁 Script executed:

# Look at the SignatureV2 definition
cat -n types/tx/signing/signature.go | head -50

Repository: initia-labs/cosmos-sdk

Length of output: 2070


🏁 Script executed:

# Get the full signature.go file to understand PubKey
cat types/tx/signing/signature.go

Repository: initia-labs/cosmos-sdk

Length of output: 3378


🏁 Script executed:

# Check if Address() method on PubKey has nil checks
rg "func.*Address\(\)" --type go -A 3 crypto/types/

Repository: initia-labs/cosmos-sdk

Length of output: 48


🏁 Script executed:

# Look at the actual extractSenderNonce function and its context
cat -n types/mempool/proxy_app_mempool.go | sed -n '400,430p'

Repository: initia-labs/cosmos-sdk

Length of output: 1165


🏁 Script executed:

# Check if there's any test that covers nil PubKey case
rg "extractSenderNonce" --type go -B 2 -A 10

Repository: initia-labs/cosmos-sdk

Length of output: 2485


🏁 Script executed:

# Check the SigVerifiableTx interface definition
rg "type SigVerifiableTx interface" --type go -A 20

Repository: initia-labs/cosmos-sdk

Length of output: 1175


🏁 Script executed:

# Check what GetSigners returns
rg "GetSigners\(\)" --type go -B 2 -A 3 | grep -A 5 "interface\|returns\|error" | head -40

Repository: initia-labs/cosmos-sdk

Length of output: 2287


🏁 Script executed:

# Look at the actual imports in proxy_app_mempool.go
head -30 types/mempool/proxy_app_mempool.go

Repository: initia-labs/cosmos-sdk

Length of output: 683


🏁 Script executed:

# Check if there are any usages of GetSigners in similar contexts
rg "GetSigners\(\)" types/mempool/ --type go -B 3 -A 5

Repository: initia-labs/cosmos-sdk

Length of output: 1214


🏁 Script executed:

# Check the signer_extraction_adapter.go which seems to handle GetSigners
cat -n types/mempool/signer_extraction_adapter.go | head -60

Repository: initia-labs/cosmos-sdk

Length of output: 2287


🏁 Script executed:

# Check for any nil checks on PubKey in the codebase
rg "PubKey != nil" types/mempool/ --type go -B 2 -A 2

Repository: initia-labs/cosmos-sdk

Length of output: 48


🏁 Script executed:

# Look for other uses of GetSigners in mempool context
rg "GetSigners\(\)" types/mempool/ --type go -B 3 -A 5

Repository: initia-labs/cosmos-sdk

Length of output: 1214


🏁 Script executed:

# See the full GetSigners implementation in signer_extraction_adapter.go
cat -n types/mempool/signer_extraction_adapter.go | sed -n '46,80p'

Repository: initia-labs/cosmos-sdk

Length of output: 746


🏁 Script executed:

# Check if there are test cases that demonstrate nil PubKey scenarios
rg "PubKey.*nil" types/mempool/proxy_app_mempool_test.go --type go -B 3 -A 3

Repository: initia-labs/cosmos-sdk

Length of output: 572


🏁 Script executed:

# Look at the full test implementation to understand what fixtures are being used
cat -n types/mempool/proxy_app_mempool_test.go | sed -n '1,100p'

Repository: initia-labs/cosmos-sdk

Length of output: 3805


🏁 Script executed:

# Check if there are any tests that specifically test nil PubKey scenarios
rg "nil.*PubKey\|PubKey.*nil" types/mempool/ --type go -B 3 -A 3

Repository: initia-labs/cosmos-sdk

Length of output: 48


🏁 Script executed:

# Search for how the code handles accounts with nil PubKey
rg "GetPubKey" types/mempool/ --type go -B 2 -A 2

Repository: initia-labs/cosmos-sdk

Length of output: 1417


🏁 Script executed:

# Check the priority_nonce_test.go to see how it tests with real tx structures
grep -n "extractSenderNonce\|GetSignaturesV2" types/mempool/priority_nonce_test.go

Repository: initia-labs/cosmos-sdk

Length of output: 193


🏁 Script executed:

# Let me check the exact definition of testPubKey to understand the test scenario
rg "type testPubKey" types/mempool/ --type go -A 10

Repository: initia-labs/cosmos-sdk

Length of output: 715


🏁 Script executed:

# Check if there's a nil check anywhere for PubKey before calling Address()
rg "PubKey.*!= nil\|if.*PubKey" types/mempool/proxy_app_mempool.go --type go

Repository: initia-labs/cosmos-sdk

Length of output: 48


🏁 Script executed:

# Let me check if there's an actual test that would fail with nil PubKey
rg "GetSignaturesV2" types/mempool/proxy_app_mempool_test.go --type go -B 5 -A 10

Repository: initia-labs/cosmos-sdk

Length of output: 497


🏁 Script executed:

# Check what the testTx implementation returns for GetSignaturesV2
cat -n types/mempool/mempool_test.go | sed -n '40,60p'

Repository: initia-labs/cosmos-sdk

Length of output: 819


🏁 Script executed:

# See the full GetSignaturesV2 implementation for testTx
cat -n types/mempool/mempool_test.go | sed -n '60,75p'

Repository: initia-labs/cosmos-sdk

Length of output: 551


🏁 Script executed:

# Check if the signer_extraction_adapter.go which has similar code has any nil checks
cat -n types/mempool/signer_extraction_adapter.go

Repository: initia-labs/cosmos-sdk

Length of output: 2417


Handle nil PubKey in SignatureV2 to prevent panic.

GetSignaturesV2() returns signatures with PubKey == nil for transactions from accounts that already have a public key on-chain (to save bytes). Calling Address() on a nil PubKey will panic. Use GetSigners() instead to extract the sender address, which returns signer addresses regardless of whether the pubkey is present in the transaction.

Suggested fix
func extractSenderNonce(tx sdk.Tx) (sender string, nonce uint64, err error) {
	sigTx, ok := tx.(signing.SigVerifiableTx)
	if !ok {
		return "", 0, fmt.Errorf("tx of type %T does not implement SigVerifiableTx", tx)
	}

+	signers, err := sigTx.GetSigners()
+	if err != nil {
+		return "", 0, err
+	}
+	if len(signers) == 0 {
+		return "", 0, fmt.Errorf("tx must have at least one signer")
+	}

	sigs, err := sigTx.GetSignaturesV2()
	if err != nil {
		return "", 0, err
	}
	if len(sigs) == 0 {
		return "", 0, fmt.Errorf("tx must have at least one signature")
	}

-	sender = string(sigs[0].PubKey.Address().Bytes())
+	sender = string(signers[0])
	nonce = sigs[0].Sequence

	return sender, nonce, nil
}

Note: Similar vulnerable code exists in signer_extraction_adapter.go at lines 60-61 that should also be fixed.

🤖 Prompt for AI Agents
In `@types/mempool/proxy_app_mempool.go` around lines 406 - 425,
extractSenderNonce currently calls GetSignaturesV2() then uses
sigs[0].PubKey.Address(), which can panic when PubKey is nil; instead call
sigTx.GetSigners() to obtain the sender address safely (use the first signer),
keep GetSignaturesV2() only to read the Sequence for nonce, validate both
returned slices are non-empty, and return the signer address
(string(signers[0])) and nonce (sigs[0].Sequence); also apply the same change to
the signer extraction code referenced in signer_extraction_adapter.go (replace
direct PubKey.Address() use with GetSigners()).

Comment on lines +467 to +481
// removeFromActive removes a tx by sender+nonce from the active pool. Returns true if found.
func (m *ProxyAppMempool) removeFromActive(sender string, nonce uint64) bool {
sq, ok := m.senderActive[sender]
if !ok {
return false
}
if sq.remove(nonce) {
m.activeCount--
if len(sq.txs) == 0 {
delete(m.senderActive, sender)
}
return true
}
return false
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Removing an active tx can leave nonces non‑contiguous.
If a middle nonce is removed, higher nonces remain active and may be selected even though their predecessors are missing. Consider demoting the trailing active txs back to the queued pool or rebuilding the active chain to keep it contiguous.

🤖 Prompt for AI Agents
In `@types/mempool/proxy_app_mempool.go` around lines 467 - 481, The
removeFromActive function can leave a sender's active nonces non-contiguous when
a middle nonce is removed; modify removeFromActive (and/or add a helper like
rebuildActiveChain or demoteTrailingActive) so that after sq.remove(nonce) it
scans sq.txs (or the senderActive sequence structure) for any higher nonces that
now lack their predecessor and moves those trailing txs back into the queued
pool (senderQueue) or otherwise marks them inactive, updating m.activeCount for
each demoted tx and deleting m.senderActive[sender] if no active txs remain;
ensure the logic uses the same sender sequence types (sq, senderActive,
senderQueue, sq.txs, sq.remove) so the active chain remains contiguous.

Copy link
Member

@beer-1 beer-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can implement this thing to initia instead of cosmos-sdk side, so the main thing we want to do is calling PromoteQueued at then end of commit, and I think we can do this at ListenCommit in initia side. wdyt?

}

if promoter, ok := app.mempool.(mempool.QueuePromoter); ok {
promoter.PromoteQueued(app.checkState.Context())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems I missed this part, but lately I fixed to use app.getState() function to get state to prevent race condition
#5

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so good to merge release/v0.50.x and fix to use getState function instead

switch mode {
case execModeCheck:
if err := app.mempool.Insert(mempoolCtx, tx); err != nil {
insertCtx := context.WithValue(mempoolCtx, mempool.TxBytesContextKey{}, txBytes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
insertCtx := context.WithValue(mempoolCtx, mempool.TxBytesContextKey{}, txBytes)
insertCtx := mempoolCtx.WithValue(mempool.TxBytesContextKey{}, txBytes)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and I can see getContextForTx return context with txBytes already, so probably this is unnecessary?

	ctx := modeState.Context().
		WithTxBytes(txBytes).
		WithGasMeter(storetypes.NewInfiniteGasMeter())

NewValidateSigCountDecorator(opts.AccountKeeper),
NewSigGasConsumeDecorator(opts.AccountKeeper, opts.SigGasConsumer),
NewSigVerificationDecorator(opts.AccountKeeper, opts.SignModeHandler),
// no DeductFeeDecorator, fees deducted during PrepareProposal/FinalizeBlock
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I think we still need to check balance > fee

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants