From 2558cdbad100446a990d0fed24ce39ac986b1232 Mon Sep 17 00:00:00 2001 From: Rene Zander Date: Wed, 3 Jun 2026 06:23:23 +0000 Subject: [PATCH 1/2] fix(market): enforce OrderMaxBids with >= instead of > CreateBid compared the existing bid count against OrderMaxBids using `>`, which let one extra bid through and allowed OrderMaxBids+1 bids per order. BidCountForOrder counts Open, Active and Closed bids, so the count reaches OrderMaxBids exactly at the cap and the check must reject at that point. Switch the comparison to `>=` and add a regression test that seeds an order to its cap and asserts a further bid is rejected with "too many existing bids". Closes akash-network/support#413 Co-Authored-By: Claude Opus 4.8 (1M context) --- x/market/handler/handler_test.go | 39 ++++++++++++++++++++++++++++++++ x/market/handler/server.go | 2 +- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/x/market/handler/handler_test.go b/x/market/handler/handler_test.go index fb72a2603..5c8d27696 100644 --- a/x/market/handler/handler_test.go +++ b/x/market/handler/handler_test.go @@ -915,6 +915,45 @@ func TestCreateBidValid(t *testing.T) { require.True(t, found) } +// TestCreateBidExceedsOrderMaxBids is a regression test for the OrderMaxBids +// off-by-one (akash-network/support#413). The original check used `>`, which +// allowed OrderMaxBids+1 bids per order. With the cap set to 1 and one bid +// already present, a further bid must be rejected. +func TestCreateBidExceedsOrderMaxBids(t *testing.T) { + suite := setupTestSuite(t) + + order, gspec := suite.createOrder(testutil.Resources(t, testutil.WithDenom("uact"))) + + params, err := suite.MarketKeeper().GetParams(suite.Context()) + require.NoError(t, err) + params.OrderMaxBids = 1 + require.NoError(t, suite.MarketKeeper().SetParams(suite.Context(), params)) + + // Seed the single permitted bid directly via the keeper so the order is at + // its cap. The OrderMaxBids check runs before any provider/escrow lookup, so + // no provider registration or bank mocks are required here. + roffer := mvbeta.ResourceOfferFromRU(gspec.Resources) + _, err = suite.MarketKeeper().CreateBid(suite.Context(), mv1.MakeBidID(order.ID, testutil.AccAddress(t)), order.Price(), roffer, nil) + require.NoError(t, err) + require.Equal(t, uint32(1), suite.MarketKeeper().BidCountForOrder(suite.Context(), order.ID)) + + // With the cap reached, a further bid must be rejected. + msg := &mvbeta.MsgCreateBid{ + ID: mv1.MakeBidID(order.ID, testutil.AccAddress(t)), + Price: sdk.NewDecCoin(sdkutil.DenomUact, sdkmath.NewInt(1)), + Deposit: deposit.Deposit{ + Amount: mvbeta.DefaultBidMinDepositACT, + Sources: deposit.Sources{deposit.SourceBalance}, + }, + } + + res, err := suite.handler(suite.Context(), msg) + require.Nil(t, res) + require.Error(t, err) + require.ErrorIs(t, err, mv1.ErrInvalidBid) + require.Contains(t, err.Error(), "too many existing bids") +} + func TestCreateBidInvalidPrice(t *testing.T) { suite := setupTestSuite(t) suite.PrepareMocks(func(ts *state.TestSuite) { diff --git a/x/market/handler/server.go b/x/market/handler/server.go index 935b7dd78..5764d3b0f 100644 --- a/x/market/handler/server.go +++ b/x/market/handler/server.go @@ -42,7 +42,7 @@ func (ms msgServer) CreateBid(goCtx context.Context, msg *mvbeta.MsgCreateBid) ( return nil, fmt.Errorf("%w: minimum:%v received:%v", mv1.ErrInvalidDeposit, sdk.NewCoin(msg.Deposit.Amount.Denom, minDeposit), msg.Deposit) } - if ms.keepers.Market.BidCountForOrder(ctx, msg.ID.OrderID()) > params.OrderMaxBids { + if ms.keepers.Market.BidCountForOrder(ctx, msg.ID.OrderID()) >= params.OrderMaxBids { return nil, fmt.Errorf("%w: too many existing bids (%v)", mv1.ErrInvalidBid, params.OrderMaxBids) } From 020e849d41a2ee52cd572a924be3611ae7bf2947 Mon Sep 17 00:00:00 2001 From: Rene Zander Date: Thu, 11 Jun 2026 08:51:28 +0000 Subject: [PATCH 2/2] fix(market): bound bids per order with a monotonic bids-ever-created counter CreateBid enforced OrderMaxBids by scanning the order's open, active and closed bids on every call. Counting closed bids was the only backpressure on an order, so the count doubled as a lifetime cap; the scan also missed lost bids entirely and cost O(n) across three state indexes per bid. Replace the scan with a per-order monotonic counter (bids-ever-created), bumped on CreateBid and never decremented when a bid closes or loses. The cap check reads the counter in O(1), and total bid records per order stay bounded by OrderMaxBids regardless of create/close churn, which closes the state-inflation loop where a provider cycles CreateBid > CloseBid forever without ever hitting the cap. Orders that predate the counter are seeded lazily from a scan of all stored bids (including lost and closed), so the cap carries over an upgrade or genesis import unchanged and no store migration is needed. BidCountForOrder now returns only live (open or active) bids for consumers that care about current competition on an order. Follow-up to #2068, as discussed in https://github.com/akash-network/node/pull/2068#issuecomment-4676418653 Co-Authored-By: Claude Fable 5 --- x/market/handler/handler_test.go | 90 ++++++++++++++++++++++++++++++++ x/market/handler/server.go | 5 +- x/market/keeper/keeper.go | 53 ++++++++++++++++--- x/market/keeper/keys/key.go | 1 + 4 files changed, 141 insertions(+), 8 deletions(-) diff --git a/x/market/handler/handler_test.go b/x/market/handler/handler_test.go index 5c8d27696..27885d5eb 100644 --- a/x/market/handler/handler_test.go +++ b/x/market/handler/handler_test.go @@ -954,6 +954,96 @@ func TestCreateBidExceedsOrderMaxBids(t *testing.T) { require.Contains(t, err.Error(), "too many existing bids") } +// TestCreateBidCapSurvivesBidChurn verifies that the OrderMaxBids cap is +// enforced against the monotonic bids-ever-created counter rather than the +// live bid count. Closing a bid must not free up cap capacity, otherwise a +// provider could loop CreateBid/CloseBid to accumulate unbounded bid records. +func TestCreateBidCapSurvivesBidChurn(t *testing.T) { + suite := setupTestSuite(t) + + order, gspec := suite.createOrder(testutil.Resources(t, testutil.WithDenom("uact"))) + + params, err := suite.MarketKeeper().GetParams(suite.Context()) + require.NoError(t, err) + params.OrderMaxBids = 1 + require.NoError(t, suite.MarketKeeper().SetParams(suite.Context(), params)) + + roffer := mvbeta.ResourceOfferFromRU(gspec.Resources) + bid, err := suite.MarketKeeper().CreateBid(suite.Context(), mv1.MakeBidID(order.ID, testutil.AccAddress(t)), order.Price(), roffer, nil) + require.NoError(t, err) + + // Closing the bid drops the live count to zero... + require.NoError(t, suite.MarketKeeper().OnBidClosed(suite.Context(), bid)) + require.Equal(t, uint32(0), suite.MarketKeeper().BidCountForOrder(suite.Context(), order.ID)) + + // ...but the monotonic counter still holds the order at its cap. + require.Equal(t, uint32(1), suite.MarketKeeper().BidsEverCreatedForOrder(suite.Context(), order.ID)) + + msg := &mvbeta.MsgCreateBid{ + ID: mv1.MakeBidID(order.ID, testutil.AccAddress(t)), + Price: sdk.NewDecCoin(sdkutil.DenomUact, sdkmath.NewInt(1)), + Deposit: deposit.Deposit{ + Amount: mvbeta.DefaultBidMinDepositACT, + Sources: deposit.Sources{deposit.SourceBalance}, + }, + } + + res, err := suite.handler(suite.Context(), msg) + require.Nil(t, res) + require.Error(t, err) + require.ErrorIs(t, err, mv1.ErrInvalidBid) + require.Contains(t, err.Error(), "too many existing bids") +} + +// TestBidsEverCreatedLazySeedsFromStore covers orders that predate the +// bids-ever-created counter: with no counter record present the count is +// derived from all stored bids, including closed and lost ones, so the cap +// carries over an upgrade or genesis import unchanged. +func TestBidsEverCreatedLazySeedsFromStore(t *testing.T) { + suite := setupTestSuite(t) + + order, gspec := suite.createOrder(testutil.Resources(t, testutil.WithDenom("uact"))) + + params, err := suite.MarketKeeper().GetParams(suite.Context()) + require.NoError(t, err) + params.OrderMaxBids = 2 + require.NoError(t, suite.MarketKeeper().SetParams(suite.Context(), params)) + + // Seed bids directly in the store, the same way genesis import and + // pre-counter state do, so no counter record exists for the order. + roffer := mvbeta.ResourceOfferFromRU(gspec.Resources) + for _, bstate := range []mvbeta.Bid_State{mvbeta.BidClosed, mvbeta.BidLost} { + err := suite.MarketKeeper().SaveBid(suite.Context(), mvbeta.Bid{ + ID: mv1.MakeBidID(order.ID, testutil.AccAddress(t)), + State: bstate, + Price: order.Price(), + CreatedAt: suite.Context().BlockHeight(), + ResourcesOffer: roffer, + }) + require.NoError(t, err) + } + + // The live count excludes the seeded bids, but the lazily derived + // bids-ever-created count sees both and holds the order at its cap. + require.Equal(t, uint32(0), suite.MarketKeeper().BidCountForOrder(suite.Context(), order.ID)) + require.Equal(t, uint32(2), suite.MarketKeeper().BidsEverCreatedForOrder(suite.Context(), order.ID)) + + msg := &mvbeta.MsgCreateBid{ + ID: mv1.MakeBidID(order.ID, testutil.AccAddress(t)), + Price: sdk.NewDecCoin(sdkutil.DenomUact, sdkmath.NewInt(1)), + Deposit: deposit.Deposit{ + Amount: mvbeta.DefaultBidMinDepositACT, + Sources: deposit.Sources{deposit.SourceBalance}, + }, + } + + res, err := suite.handler(suite.Context(), msg) + require.Nil(t, res) + require.Error(t, err) + require.ErrorIs(t, err, mv1.ErrInvalidBid) + require.Contains(t, err.Error(), "too many existing bids") +} + func TestCreateBidInvalidPrice(t *testing.T) { suite := setupTestSuite(t) suite.PrepareMocks(func(ts *state.TestSuite) { diff --git a/x/market/handler/server.go b/x/market/handler/server.go index 5764d3b0f..b03e82c94 100644 --- a/x/market/handler/server.go +++ b/x/market/handler/server.go @@ -42,7 +42,10 @@ func (ms msgServer) CreateBid(goCtx context.Context, msg *mvbeta.MsgCreateBid) ( return nil, fmt.Errorf("%w: minimum:%v received:%v", mv1.ErrInvalidDeposit, sdk.NewCoin(msg.Deposit.Amount.Denom, minDeposit), msg.Deposit) } - if ms.keepers.Market.BidCountForOrder(ctx, msg.ID.OrderID()) >= params.OrderMaxBids { + // Cap on bids ever created, not live bids: closing a bid must not free up + // capacity, otherwise a provider can loop CreateBid/CloseBid to grow state + // without bound and crowd out other providers. + if ms.keepers.Market.BidsEverCreatedForOrder(ctx, msg.ID.OrderID()) >= params.OrderMaxBids { return nil, fmt.Errorf("%w: too many existing bids (%v)", mv1.ErrInvalidBid, params.OrderMaxBids) } diff --git a/x/market/keeper/keeper.go b/x/market/keeper/keeper.go index 33bba5118..a24fb97b1 100644 --- a/x/market/keeper/keeper.go +++ b/x/market/keeper/keeper.go @@ -1,6 +1,7 @@ package keeper import ( + "errors" "fmt" "time" @@ -44,6 +45,7 @@ type IKeeper interface { WithLeases(ctx sdk.Context, fn func(mv1.Lease) bool) WithOrdersForGroup(ctx sdk.Context, id dtypes.GroupID, state types.Order_State, fn func(types.Order) bool) BidCountForOrder(ctx sdk.Context, id mv1.OrderID) uint32 + BidsEverCreatedForOrder(ctx sdk.Context, id mv1.OrderID) uint32 GetParams(ctx sdk.Context) (types.Params, error) SetParams(ctx sdk.Context, params types.Params) error GetAuthority() string @@ -61,11 +63,12 @@ type Keeper struct { // This should be the x/gov module account. authority string - schema collections.Schema - bids *collections.IndexedMap[keys.BidPrimaryKey, types.Bid, BidIndexes] - orders *collections.IndexedMap[keys.OrderPrimaryKey, types.Order, OrderIndexes] - leases *collections.IndexedMap[keys.LeasePrimaryKey, mv1.Lease, LeaseIndexes] - Params collections.Item[types.Params] + schema collections.Schema + bids *collections.IndexedMap[keys.BidPrimaryKey, types.Bid, BidIndexes] + orders *collections.IndexedMap[keys.OrderPrimaryKey, types.Order, OrderIndexes] + leases *collections.IndexedMap[keys.LeasePrimaryKey, mv1.Lease, LeaseIndexes] + bidCounts collections.Map[keys.OrderPrimaryKey, uint32] + Params collections.Item[types.Params] } // NewKeeper creates and returns an instance for Market keeper @@ -80,6 +83,7 @@ func NewKeeper(cdc codec.BinaryCodec, skey *storetypes.KVStoreKey, ekeeper Escro bids := collections.NewIndexedMap(sb, collections.NewPrefix(keys.BidPrefixNew), "bids", keys.BidPrimaryKeyCodec, codec.CollValue[types.Bid](cdc), bidIndexes) orders := collections.NewIndexedMap(sb, collections.NewPrefix(keys.OrderPrefixNew), "orders", keys.OrderPrimaryKeyCodec, codec.CollValue[types.Order](cdc), orderIndexes) leases := collections.NewIndexedMap(sb, collections.NewPrefix(keys.LeasePrefixNew), "leases", keys.LeasePrimaryKeyCodec, codec.CollValue[mv1.Lease](cdc), leaseIndexes) + bidCounts := collections.NewMap(sb, collections.NewPrefix(keys.BidCountPrefix), "bid_counts", keys.OrderPrimaryKeyCodec, collections.Uint32Value) params := collections.NewItem(sb, keys.ParamsPrefix, "params", codec.CollValue[types.Params](cdc)) schema, err := sb.Build() @@ -96,6 +100,7 @@ func NewKeeper(cdc codec.BinaryCodec, skey *storetypes.KVStoreKey, ekeeper Escro bids: bids, orders: orders, leases: leases, + bidCounts: bidCounts, Params: params, } @@ -230,6 +235,15 @@ func (k Keeper) CreateBid(ctx sdk.Context, id mv1.BidID, price sdk.DecCoin, roff ReclamationWindow: reclaimWindow, } + // Bump the monotonic bids-ever-created counter for the order. It never + // decrements when bids close, so total bid records per order stay bounded + // by OrderMaxBids regardless of create/close churn. Read before storing + // the new bid: a lazy seed scans stored bids and must not see this one. + count := k.BidsEverCreatedForOrder(ctx, id.OrderID()) + if err := k.bidCounts.Set(ctx, keys.OrderIDToKey(id.OrderID()), count+1); err != nil { + return types.Bid{}, err + } + if err := k.bids.Set(ctx, pk, bid); err != nil { return types.Bid{}, err } @@ -559,15 +573,40 @@ func (k Keeper) WithBidsForOrder(ctx sdk.Context, id mv1.OrderID, state types.Bi } } +// BidCountForOrder returns the number of live (open or active) bids on an +// order. Closed and lost bids are excluded; use BidsEverCreatedForOrder for +// the monotonic count that backs the OrderMaxBids cap. func (k Keeper) BidCountForOrder(ctx sdk.Context, id mv1.OrderID) uint32 { + return k.bidCountForStates(ctx, id, []types.Bid_State{types.BidOpen, types.BidActive}) +} + +// BidsEverCreatedForOrder returns the monotonic count of bids ever created for +// an order. The counter is bumped on CreateBid and never decremented when a +// bid closes or loses, so it bounds the total number of bid records an order +// can accumulate. Orders that predate the counter are seeded lazily from a +// scan of all stored bids, which preserves the cap across the upgrade and +// after genesis import. +func (k Keeper) BidsEverCreatedForOrder(ctx sdk.Context, id mv1.OrderID) uint32 { + count, err := k.bidCounts.Get(ctx, keys.OrderIDToKey(id)) + if err == nil { + return count + } + if !errors.Is(err, collections.ErrNotFound) { + panic(fmt.Sprintf("BidsEverCreatedForOrder failed: %v", err)) + } + + return k.bidCountForStates(ctx, id, []types.Bid_State{types.BidOpen, types.BidActive, types.BidLost, types.BidClosed}) +} + +func (k Keeper) bidCountForStates(ctx sdk.Context, id mv1.OrderID, states []types.Bid_State) uint32 { orderPart := collections.Join4(id.Owner, id.DSeq, id.GSeq, id.OSeq) count := uint32(0) - for _, state := range []types.Bid_State{types.BidOpen, types.BidActive, types.BidClosed} { + for _, state := range states { refKey := collections.Join(orderPart, int32(state)) iter, err := k.bids.Indexes.OrderState.MatchExact(ctx, refKey) if err != nil { - panic(fmt.Sprintf("BidCountForOrder failed: %v", err)) + panic(fmt.Sprintf("bidCountForStates failed: %v", err)) } for ; iter.Valid(); iter.Next() { count++ diff --git a/x/market/keeper/keys/key.go b/x/market/keeper/keys/key.go index ffc47b00c..058a180a7 100644 --- a/x/market/keeper/keys/key.go +++ b/x/market/keeper/keys/key.go @@ -40,6 +40,7 @@ var ( BidIndexStatePrefix = []byte{0x12, 0x03} BidIndexProviderPrefix = []byte{0x12, 0x04} BidIndexOrderStatePrefix = []byte{0x12, 0x05} + BidCountPrefix = []byte{0x12, 0x06} BidStateOpenPrefix = []byte{BidStateOpenPrefixID} BidStateActivePrefix = []byte{BidStateActivePrefixID} BidStateLostPrefix = []byte{BidStateLostPrefixID}