diff --git a/x/market/handler/handler_test.go b/x/market/handler/handler_test.go index fb72a2603..27885d5eb 100644 --- a/x/market/handler/handler_test.go +++ b/x/market/handler/handler_test.go @@ -915,6 +915,135 @@ 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") +} + +// 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 935b7dd78..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}