Skip to content

Commit 256d913

Browse files
almk-devaljo242
andauthored
fix(ci): solve gas meter race condition in integration tests (#732)
* create new context * add teardown * add multistore to test setup so that cachecontext can be used * add explicit subpool resets * add longer timeout * add mutex control * lint * update changelog * update comments * add separated builds * rename attr --------- Co-authored-by: Alex | Cosmos Labs <[email protected]>
1 parent 91519e4 commit 256d913

File tree

12 files changed

+152
-23
lines changed

12 files changed

+152
-23
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
- [\#730](https://github.com/cosmos/evm/pull/730) Fix panic if evm mempool not used.
4848
- [\#733](https://github.com/cosmos/evm/pull/733) Avoid rejecting tx with unsupported extension option for ExtensionOptionDynamicFeeTx.
4949
- [\#736](https://github.com/cosmos/evm/pull/736) Add InitEvmCoinInfo upgrade to avoid panic when denom is not registered.
50+
- [\#732](https://github.com/cosmos/evm/pull/732) Fix gas meter race condition in integration tests
5051

5152
### IMPROVEMENTS
5253

Makefile

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ test-race: run-tests
156156

157157
test-evmd: ARGS=-timeout=15m
158158
test-evmd:
159-
@cd evmd && go test -race -tags=test -mod=readonly $(ARGS) $(EXTRA_ARGS) $(PACKAGES_EVMD)
159+
@cd evmd && go test -count=1 -race -tags=test -mod=readonly $(ARGS) $(EXTRA_ARGS) $(PACKAGES_EVMD)
160160

161161
test-unit-cover: ARGS=-timeout=15m -coverprofile=coverage.txt -covermode=atomic
162162
test-unit-cover: TEST_PACKAGES=$(PACKAGES_UNIT)
@@ -182,9 +182,9 @@ test-all:
182182

183183
run-tests:
184184
ifneq (,$(shell which tparse 2>/dev/null))
185-
go test -race -tags=test -mod=readonly -json $(ARGS) $(EXTRA_ARGS) $(TEST_PACKAGES) | tparse
185+
go test -count=1 -race -tags=test -mod=readonly -json $(ARGS) $(EXTRA_ARGS) $(TEST_PACKAGES) | tparse
186186
else
187-
go test -race -tags=test -mod=readonly $(ARGS) $(EXTRA_ARGS) $(TEST_PACKAGES)
187+
go test -count=1 -race -tags=test -mod=readonly $(ARGS) $(EXTRA_ARGS) $(TEST_PACKAGES)
188188
endif
189189

190190
# Use the old Apple linker to workaround broken xcode - https://github.com/golang/go/issues/65169

mempool/blockchain.go

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@ type Blockchain struct {
4444
previousHeaderHash common.Hash
4545
latestCtx sdk.Context
4646
mu sync.RWMutex
47+
48+
testingCommitMu sync.RWMutex
4749
}
4850

4951
// NewBlockchain creates a new Blockchain instance that bridges Cosmos SDK state with Ethereum mempools.
@@ -211,13 +213,34 @@ func (b *Blockchain) StateAt(hash common.Hash) (vm.StateDB, error) {
211213
return nil, fmt.Errorf("failed to get latest context for StateAt: %w", err)
212214
}
213215

214-
appHash := ctx.BlockHeader().AppHash
215-
stateDB := statedb.New(ctx, b.vmKeeper, statedb.NewEmptyTxConfig())
216+
// Create a cache context to isolate the StateDB from concurrent commits.
217+
// This prevents race conditions when the background txpool reorg goroutine
218+
// reads state while the main thread is committing new blocks.
219+
cacheCtx, _ := ctx.CacheContext()
220+
// Use an infinite gas meter to avoid tracking gas for read-only state queries
221+
cacheCtx = cacheCtx.WithGasMeter(sdktypes.NewInfiniteGasMeter())
222+
223+
appHash := cacheCtx.BlockHeader().AppHash
224+
stateDB := statedb.New(cacheCtx, b.vmKeeper, statedb.NewEmptyTxConfig())
216225

217226
b.logger.Debug("StateDB created successfully", "app_hash", common.Hash(appHash).Hex())
218227
return stateDB, nil
219228
}
220229

230+
// BeginCommit acquires an exclusive lock to prevent mempool state reads during Commit.
231+
// This avoids data races in the underlying storage (e.g., IAVL) when tests run with -race.
232+
func (b *Blockchain) BeginCommit() { b.testingCommitMu.Lock() }
233+
234+
// EndCommit releases the exclusive lock acquired by BeginCommit.
235+
func (b *Blockchain) EndCommit() { b.testingCommitMu.Unlock() }
236+
237+
// BeginRead acquires a shared lock for background readers (e.g., txpool reorg).
238+
// This enables optional coordination with Commit without importing the type.
239+
func (b *Blockchain) BeginRead() { b.testingCommitMu.RLock() }
240+
241+
// EndRead releases the shared read lock acquired by BeginRead.
242+
func (b *Blockchain) EndRead() { b.testingCommitMu.RUnlock() }
243+
221244
func (b *Blockchain) getPreviousHeaderHash() common.Hash {
222245
b.mu.RLock()
223246
defer b.mu.RUnlock()

mempool/blockchain_test.go

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,18 @@ import (
2121
"cosmossdk.io/log"
2222
storetypes "cosmossdk.io/store/types"
2323

24+
"github.com/cosmos/cosmos-sdk/testutil"
2425
sdk "github.com/cosmos/cosmos-sdk/types"
2526
)
2627

27-
// createMockContext creates a basic mock context for testing
28+
// createMockContext creates a basic mock context for testing with multistore
2829
func createMockContext() sdk.Context {
29-
return sdk.Context{}.
30+
// crate ctx with multistore so that CacheContext() can be used
31+
storeKey := storetypes.NewKVStoreKey("test")
32+
transientKey := storetypes.NewTransientStoreKey("transient_test")
33+
ctx := testutil.DefaultContext(storeKey, transientKey)
34+
35+
return ctx.
3036
WithBlockTime(time.Now()).
3137
WithBlockHeader(cmtproto.Header{AppHash: []byte("00000000000000000000000000000000")}).
3238
WithBlockHeight(1)
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
//go:build !race
2+
3+
package legacypool
4+
5+
// beginCommitRead is a no-op in non-race builds to avoid unnecessary overhead.
6+
func beginCommitRead(_ any) func() { return func() {} }
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
//go:build race
2+
3+
package legacypool
4+
5+
// commitAwareChain is implemented by chains that expose optional commit RW locks.
6+
// These are used to coordinate background readers with Commit in tests/race builds.
7+
type commitAwareChain interface {
8+
BeginRead()
9+
EndRead()
10+
}
11+
12+
// beginCommitRead acquires a shared read lock when available and returns a
13+
// function that releases it. In race/test builds this coordinates with Commit
14+
// to avoid read/write races in storage. In non-race builds this is a no-op.
15+
func beginCommitRead(chain any) func() {
16+
if ca, ok := chain.(commitAwareChain); ok {
17+
ca.BeginRead()
18+
return func() { ca.EndRead() }
19+
}
20+
return func() {}
21+
}

mempool/txpool/legacypool/legacypool.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1273,6 +1273,9 @@ func (pool *LegacyPool) runReorg(done chan struct{}, reset *txpoolResetRequest,
12731273
defer close(done)
12741274

12751275
var promoteAddrs []common.Address
1276+
// Optionally acquire a shared read lock to coordinate with Commit in tests.
1277+
unlock := beginCommitRead(any(pool.chain))
1278+
defer unlock()
12761279
if dirtyAccounts != nil && reset == nil {
12771280
// Only dirty accounts need to be promoted, unless we're resetting.
12781281
// For resets, all addresses in the tx queue will be promoted and

tests/integration/mempool/test_helpers.go

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"encoding/hex"
55
"fmt"
66
"math/big"
7-
"time"
87

98
abci "github.com/cometbft/cometbft/abci/types"
109
"github.com/cometbft/cometbft/crypto/tmhash"
@@ -205,17 +204,20 @@ func (s *IntegrationTestSuite) notifyNewBlockToMempool() {
205204
// Access the underlying blockchain interface from the EVM mempool
206205
if evmMempoolCast, ok := evmMempool.(*evmmempool.ExperimentalEVMMempool); ok {
207206
blockchain := evmMempoolCast.GetBlockchain()
207+
txPool := evmMempoolCast.GetTxPool()
208208

209-
// Trigger a new block notification
210-
// This sends a ChainHeadEvent that the mempool subscribes to.
211-
// The TxPool's event loop receives this and calls Reset() for each subpool,
212-
// which naturally removes committed transactions via demoteUnexecutables().
209+
// Get the current and new block headers for reset
210+
oldHead := blockchain.CurrentBlock()
211+
212+
// Trigger a new block notification to update the blockchain state
213213
blockchain.NotifyNewBlock()
214+
newHead := blockchain.CurrentBlock()
214215

215-
// The ChainHeadEvent is processed asynchronously, so we need to wait a bit
216-
// for the event to be processed and the reset to complete.
217-
// In integration tests, this might need a small delay to ensure the event
218-
// is processed before we check the mempool state.
219-
time.Sleep(100 * time.Millisecond)
216+
// Directly call Reset on each subpool to ensure synchronous completion
217+
// This prevents race conditions by waiting for the reset to complete
218+
// before continuing with test assertions
219+
for _, subpool := range txPool.Subpools {
220+
subpool.Reset(oldHead, newHead)
221+
}
220222
}
221223
}

tests/integration/mempool/test_mempool_integration.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,8 @@ func (s *IntegrationTestSuite) TestMempoolInsert() {
134134

135135
for _, tc := range testCases {
136136
s.Run(tc.name, func() {
137+
// Clean up previous test's resources before resetting
138+
s.TearDownTest()
137139
// Reset test setup to ensure clean state
138140
s.SetupTest()
139141

@@ -220,6 +222,8 @@ func (s *IntegrationTestSuite) TestMempoolRemove() {
220222

221223
for _, tc := range testCases {
222224
s.Run(tc.name, func() {
225+
// Clean up previous test's resources before resetting
226+
s.TearDownTest()
223227
// Reset test setup to ensure clean state
224228
s.SetupTest()
225229

@@ -304,6 +308,8 @@ func (s *IntegrationTestSuite) TestMempoolSelect() {
304308

305309
for _, tc := range testCases {
306310
s.Run(tc.name, func() {
311+
// Clean up previous test's resources before resetting
312+
s.TearDownTest()
307313
// Reset test setup to ensure clean state
308314
s.SetupTest()
309315

@@ -419,6 +425,8 @@ func (s *IntegrationTestSuite) TestMempoolIterator() {
419425

420426
for _, tc := range testCases {
421427
s.Run(tc.name, func() {
428+
// Clean up previous test's resources before resetting
429+
s.TearDownTest()
422430
// Reset test setup to ensure clean state
423431
s.SetupTest()
424432

@@ -805,6 +813,8 @@ func (s *IntegrationTestSuite) TestTransactionOrdering() {
805813

806814
for _, tc := range testCases {
807815
s.Run(tc.name, func() {
816+
// Clean up previous test's resources before resetting
817+
s.TearDownTest()
808818
// Reset test setup to ensure clean state
809819
s.SetupTest()
810820

tests/integration/mempool/test_mempool_integration_abci.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,8 @@ func (s *IntegrationTestSuite) TestTransactionOrderingWithABCIMethodCalls() {
166166

167167
for _, tc := range testCases {
168168
s.Run(tc.name, func() {
169+
// Clean up previous test's resources before resetting
170+
s.TearDownTest()
169171
// Reset test setup to ensure clean state
170172
s.SetupTest()
171173

@@ -372,6 +374,8 @@ func (s *IntegrationTestSuite) TestNonceGappedEVMTransactionsWithABCIMethodCalls
372374

373375
for _, tc := range testCases {
374376
s.Run(tc.name, func() {
377+
// Clean up previous test's resources before resetting
378+
s.TearDownTest()
375379
s.SetupTest()
376380

377381
txs, expTxHashes := tc.setupTxs()
@@ -479,6 +483,8 @@ func (s *IntegrationTestSuite) TestCheckTxHandlerForCommittedAndLowerNonceTxs()
479483

480484
for _, tc := range testCases {
481485
s.Run(tc.name, func() {
486+
// Clean up previous test's resources before resetting
487+
s.TearDownTest()
482488
// Reset test setup to ensure clean state
483489
s.SetupTest()
484490

0 commit comments

Comments
 (0)