fix(market): enforce OrderMaxBids with >= instead of >#2068
fix(market): enforce OrderMaxBids with >= instead of >#2068renezander030 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughCreateBid's bid-count check was changed from ChangesBid Maximum Boundary Condition Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
chalabi2
left a comment
There was a problem hiding this comment.
LGTM
Interested to hear your take on excluding closed bids from the bid count without causing DOS issues or allowing close records to inflate state.
edce4db to
7246b6c
Compare
|
Thanks! I left that out of scope here since this PR is just the off-by-one, but it's worth digging into. Closed bids count today because So closed bids can't just be excluded, you'd need to replace the cap that count was providing. One clean option is a monotonic per-order counter (bids-ever-created), bumped on I can spin that into a follow-up PR if the direction sounds right. |
Yeah that would be great, your implementation idea sounds similar to what I had in mind so go for it. |
|
@renezander030 can you sign your commits? |
7246b6c to
2558cdb
Compare
|
Commits are signed now. Follow-up PR with the monotonic counter is up: #2071. |
|
Marked as stale; will be closed in five days. |
|
Friendly ping — this one's approved and still relevant. Anything blocking the merge, or is it waiting on a release window? Happy to help if needed. |
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) <noreply@anthropic.com>
2558cdb to
c5c1fc8
Compare
…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 akash-network#2068, as discussed in akash-network#2068 (comment) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Hi, and thanks for all the work on the node.
This fixes the OrderMaxBids off-by-one reported in akash-network/support#413.
CreateBidinx/market/handler/server.gocompared the existing bid count againstOrderMaxBidswith>:BidCountForOrdercounts Open, Active and Closed bids, so the count reachesOrderMaxBidsexactly when the cap is hit. With>, the check only fired atOrderMaxBids + 1, so one extra bid was always allowed. This switches the comparison to>=.Test
Added
TestCreateBidExceedsOrderMaxBids: it sets the cap to 1, seeds one bid via the keeper, then asserts a second bid through the handler is rejected with "too many existing bids". It fails on the old>(the second bid falls through to an "unknown provider" error) and passes with>=.go test ./x/market/handler/andgo vet ./x/market/handler/pass, and gofmt is clean.Note
This changes consensus behaviour (a bid that previously succeeded now fails), so it belongs in a coordinated upgrade. I saw the code freeze for the upcoming network upgrade, so feel free to queue it for whenever that lands. Happy to adjust if you would rather have
BidCountForOrderexclude Closed bids, which the issue flags as a possible follow-up.