app/evm_mempool.go (1)

29-35: Consider logging when global mempool is already set.

When GetGlobalEVMMempool() is not nil, the code silently skips setting the global mempool. This could lead to a subtle mismatch where the app's EVMMempool differs from the global one. Consider adding a debug log or warning when this condition occurs.

 	// Set the global mempool for RPC access
 	if evmmempool.GetGlobalEVMMempool() == nil {
 		if err := evmmempool.SetGlobalEVMMempool(evmMempool); err != nil {
 			panic(err)
 		}
+	} else {
+		logger.Debug("global EVM mempool already set, skipping")
 	}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

app/evm_mempool.go (1)

31-35: Potential TOCTOU race on global mempool initialization.

The check-then-set pattern between GetGlobalEVMMempool() and SetGlobalEVMMempool() is not atomic. If SetupEVMMempool is called concurrently (unlikely but possible in testing scenarios), a race condition could occur. Since this is initialization-time code typically called once, the practical risk is low.

Consider if the evmmempool package provides an atomic "set if nil" operation. If not and concurrent initialization is a concern, you could add synchronization or rely on SetGlobalEVMMempool to handle the "already set" case gracefully:

-	if evmmempool.GetGlobalEVMMempool() == nil {
-		if err := evmmempool.SetGlobalEVMMempool(evmMempool); err != nil {
-			panic(err)
-		}
+	if err := evmmempool.SetGlobalEVMMempool(evmMempool); err != nil {
+		// Only panic if not already set
+		if evmmempool.GetGlobalEVMMempool() == nil {
+			panic(err)
+		}
 	}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

app/evm_mempool.go (1)

47-97: Genesis-based block gas limit handling is consistent and fixes prior edge cases.

The new GetBlockGasLimit logic:

• Uses defaultBlockGasLimit for all configuration/genesis read failures and for invalid max_gas < -1.
• Correctly maps max_gas == -1 to math.MaxUint64 for “unlimited” and logs that explicitly.
• Safely casts non-negative max_gas to uint64 after validation.

This is consistent, avoids silent “zero gas” behavior, and matches the documented semantics. Only very minor nit (optional): you could log the actual defaultBlockGasLimit value in the error paths for easier observability.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

app/evm_mempool.go (1)

58-58: Fix function name in comment.

The comment says "getBlockGasLimit" but the actual function is "fetchBlockGasLimit".

Apply this diff to fix the typo:

-// getBlockGasLimit reads the genesis json file using AppGenesisFromFile
+// fetchBlockGasLimit reads the genesis json file using AppGenesisFromFile
 // to extract the consensus block gas limit before InitChain is called.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

app/evm_mempool.go (2)

20-45: EVMMempool wiring looks solid; consider surfacing final gas limit for observability.

The SetupEVMMempool path (AnteHandler wiring, global mempool registration, CheckTx, and PrepareProposal) looks consistent and self-contained. As a minor improvement, consider logging the final resolved BlockGasLimit once here (in addition to the genesis debug log) so operators can see exactly what limit the mempool is using at startup, especially when the default fallback is applied.

---

59-72: Clarify helper comment and logs vs sentinel behavior.

Two small nits here:

• The comment above fetchBlockGasLimit still refers to getBlockGasLimit, which can be confusing when scanning the file.
• The error logs say “using default block gas limit” but the function actually returns 0 as a sentinel, with the actual default applied only in getBlockGasLimit; if this helper is ever reused directly, those messages could mislead. Consider either returning defaultBlockGasLimit here or adjusting the log text (e.g., “returning 0; caller will fall back to default block gas limit”) to reflect the real behavior.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

app/evm_mempool.go (2)

14-17: Clarify appOpts usage vs. block gas limit configuration.

SetupEVMMempool accepts appOpts but doesn’t use it, and EVMMempoolConfig is only populated with AnteHandler. Given the PR description about taking the block gas limit from genesis, it’s a bit hard to see from this function where that value is actually injected into the EVM mempool now.

If the block gas limit is configured elsewhere (e.g., inside EVMKeeper or another helper that this function indirectly uses), it might be worth adding a brief comment here indicating that appOpts is intentionally unused and that block gas wiring happens in <component>. If instead the intent was to drive mempool config from appOpts (e.g., via a BlockGasLimit field on EVMMempoolConfig), consider either wiring it here or removing appOpts from the signature to avoid confusion.

If you intend to keep appOpts for future configuration, you could also document that with a short comment, or wire it through to wherever the genesis block gas limit is currently read, so the PR behavior matches its description.

---

23-27: Consider logging when the global EVMMempool is already set.

Right now, if GetGlobalEVMMempool() is non-nil, SetupEVMMempool silently skips SetGlobalEVMMempool but still replaces app.EVMMempool. In multi-app or multi-test-process scenarios this can make it tricky to see that the global and per-app mempools differ.

Consider adding an else branch with a debug/warn log explaining that the global EVM mempool was already initialized and won’t be replaced, so operators and test writers can more easily diagnose any surprising behavior.

Example sketch:

	if evmmempool.GetGlobalEVMMempool() == nil {
		if err := evmmempool.SetGlobalEVMMempool(evmMempool); err != nil {
			panic(err)
		}
+	} else {
+		logger.Debug("global EVM mempool already initialized; skipping SetGlobalEVMMempool")
	}

Please double-check SetGlobalEVMMempool’s semantics in your github.com/cosmos/evm version to ensure that logging (without attempting to reset the global) is the right behavior for repeated initializations.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

app/evm_mempool.go (1)

46-46: Track the TODO for removing temporary workaround.

The comment indicates this function is a temporary workaround until the EVM module provides a fix.

Would you like me to create a tracking issue for removing this workaround once the upstream EVM fix is available?

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

app/evm_mempool.go (1)

48-71: broadcastEVMTransactions implementation is sound; optional improvements only.

• For cosmos/evm v0.4.x, MsgEthereumTx.FromEthereumTx(tx *ethtypes.Transaction) does not return an error, so calling it without error handling is correct and the earlier review concern about a missing error check does not apply. (pkg.go.dev)
• The error handling for SetMsgs, tx encoding, BroadcastTxSync, and non‑zero response codes is clear and should make mempool broadcast failures easy to diagnose.
• If you ever want more resilience, you could log and continue on per‑tx failures instead of aborting the whole batch on the first error, but that’s a design choice and not required for this bugfix.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

app/evm_mempool.go (1)

50-51: Add error handling for FromEthereumTx call.

The return value from msg.FromEthereumTx(ethTx) is ignored. If this method returns an error (e.g., due to invalid transaction data or signature issues), the msg could be in an invalid state, causing silent failures in subsequent operations like SetMsgs, encoding, or broadcasting.

Apply this diff to add error handling:

 	for _, ethTx := range ethTxs {
 		msg := &evmtypes.MsgEthereumTx{}
-		msg.FromEthereumTx(ethTx)
+		if err := msg.FromEthereumTx(ethTx); err != nil {
+			return fmt.Errorf("failed to convert Ethereum tx %s: %w", ethTx.Hash().Hex(), err)
+		}

app/evm_mempool.go (1)

47-47: TODO indicates temporary workaround—consider tracking.

The comment suggests this function should be removed once the EVM module provides a fix. Consider creating a tracking issue to ensure this technical debt is addressed when the upstream fix is available.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

app/evm_mempool.go (2)

48-74: Solid broadcast flow; consider minor efficiency and diagnostics tweaks.

The conversion → build → encode → BroadcastTxSync pipeline and error handling look good. Two optional nits:

• Compute the Ethereum signer once outside the loop to avoid redundant work.
• Include the tx hash in all error messages (build/encode) to ease debugging when processing batches.

For example:

-func (app *KiichainApp) broadcastEVMTransactions(ethTxs []*ethtypes.Transaction) error {
-	for _, ethTx := range ethTxs {
-		msg := &evmtypes.MsgEthereumTx{}
-		ethSigner := ethtypes.LatestSigner(evmtypes.GetEthChainConfig())
+func (app *KiichainApp) broadcastEVMTransactions(ethTxs []*ethtypes.Transaction) error {
+	ethSigner := ethtypes.LatestSigner(evmtypes.GetEthChainConfig())
+	for _, ethTx := range ethTxs {
+		msg := &evmtypes.MsgEthereumTx{}
@@
-		cosmosTx, err := msg.BuildTx(app.clientCtx.TxConfig.NewTxBuilder(), "akii")
+		cosmosTx, err := msg.BuildTx(app.clientCtx.TxConfig.NewTxBuilder(), "akii")
 		if err != nil {
-			return fmt.Errorf("failed to build cosmos tx: %w", err)
+			return fmt.Errorf("failed to build cosmos tx for %s: %w", ethTx.Hash().Hex(), err)
 		}
@@
-		txBytes, err := app.clientCtx.TxConfig.TxEncoder()(cosmosTx)
+		txBytes, err := app.clientCtx.TxConfig.TxEncoder()(cosmosTx)
 		if err != nil {
-			return fmt.Errorf("failed to encode transaction: %w", err)
+			return fmt.Errorf("failed to encode transaction %s: %w", ethTx.Hash().Hex(), err)
 		}

These are non-blocking improvements; current behavior is fine functionally.

If you want to double-check behavior, you can add a small test that feeds multiple ethTxs and asserts that failures are surfaced with the expected hash context.

---

19-42: Use blank identifier for unused appOpts parameter.

The appOpts parameter is not used in this function. While Go allows unused function parameters (they don't cause compilation errors), it's good practice to mark intentionally unused parameters with the blank identifier _ to improve code clarity and satisfy linters. Apply this change if you don't plan to use appOpts for configuration in the near future:

-func (app *KiichainApp) SetupEVMMempool(appOpts servertypes.AppOptions, logger log.Logger) {
+func (app *KiichainApp) SetupEVMMempool(_ servertypes.AppOptions, logger log.Logger) {

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

tests/e2e/e2e_test.go (1)

166-167: Clarify and track the disabled mempool EVM test

The inline comment is easy to overlook. It would help future maintainers if this explicitly reads as a TODO and points to the underlying bug/PR so it’s obvious when and how to re‑enable.

For example:

-	// Disabled until mempool bug is fixed
-	// s.testMempoolEVM(jsonRPC)
+	// TODO: Re-enable once the EVM mempool bug is fixed (see PR #175).
+	// s.testMempoolEVM(jsonRPC)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

tests/e2e/e2e_evm_test.go (1)

106-152: Prefer a skipping test over fully commented-out code

Instead of commenting out the entire testMempoolEVM function, consider keeping the test and immediately calling s.T().Skip("disabled while EVM mempool is off due to bug, see PR #175"). That way:

• The test remains visible in go test output as “skipped” rather than silently disappearing.
• The code continues to compile against evolving APIs, reducing bit-rot and easing future re‑enablement.

Example:

-// Disabled until EVM mempool is enabled again
-// // testMempoolEVM Tests EVM's mempool
-// func (s *IntegrationTestSuite) testMempoolEVM(jsonRPC string) {
-// 	...
-// }
+// testMempoolEVM Tests EVM's mempool (currently disabled until EVM mempool is enabled again).
+func (s *IntegrationTestSuite) testMempoolEVM(jsonRPC string) {
+	s.T().Skip("disabled while EVM mempool is off due to bug, see PR #175")
+	// TODO: restore original mempool test logic when EVM mempool is re-enabled.
+}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro