[Bug Report] `match_order`: an order at the exact touch price never crosses, and the opposite-side guard dereferences an empty book

[OPTIONPOOL]

Found while integrating cpp20-orderbook into an open matching-engine benchmark, the Matching Engine Performance Challenge — it cross-checks engines against the byte-identical consensus of other open-source engines. And thanks for sharing this — the C++20 concepts-plus-coroutine generator design in OrderBook is genuinely lovely to read, and accept_order yielding one OrderQuantity per fill is a really clean matching interface. While running it I hit two small issues that both trace back to a single guard in PriceLevelStack::match_order; passing them back with a reproduction and a one-line fix.

Pinned at current main (da4dfde). Header-only C++20, built with g++ -std=c++20 -fcoroutines -O2 -Iinclude (GCC 14); I drove it the same shape as src/main.cpp — build an OrderBook<MyOrder>, call accept_order, and drain the returned generator.

Both issues live in this guard (include/orderbook/pricelevelstack.hpp, PriceLevelStack::match_order):

auto it = m_levels.begin();

if (price_compare(*it, order))   // :195
{
    for (; it != m_levels.end(); ++it)
    {
        if (quantity_of(order) == quantity_filled)      break;   // fully filled
        else if (price_compare(order, *it))             break;   // :203  price no longer crosses
        // ... match this level ...
    }
    m_levels.erase(m_levels.begin(), it);
}

Bug 1 — an order exactly at the touch price never crosses (the book locks)

The outer guard uses a strict price compare, so a marketable order whose price equals the best opposite level is rejected before the loop even starts — e.g. a SELL limit at 100 against a resting BID at 100 yields zero trades, and the sell then rests, leaving the book locked (best bid 100 / best ask 100). The inner break test at :203 is already inclusive (it keeps crossing while prices are equal), so it's really just the outer gate that's a hair too strict.

Mechanism. price_compare is PriceLevelCompare<MySide> (:150), which for the bid side returns price_of(b) < price_of(a). With the outer guard price_compare(*it, order) (a = best level, b = incoming order), a SELL matching the bid side enters only when sell_price < best_bid — strictly — so sell_price == best_bid is excluded. The inner guard price_compare(order, *it) at :203 is the inclusive form (best_bid < sell_price to stop), so the loop body would happily cross at equal price; only the outer gate blocks it.

Bug 2 — the guard dereferences an empty opposite side

When the side being matched against is empty, m_levels.begin() == m_levels.end(), and *it reads a past-the-end iterator. do_accept_order (orderbook.hpp) calls match_side.match_order(order, …) unconditionally, so this is reached by the very first order of any session (it matches an empty book). The auto it = m_levels.begin(); … *it at the top of the guard presumes at least one level exists, which isn't guaranteed.

Minimal reproduction

struct MyOrder : scob::Order<> { int userid; };
scob::OrderBook<MyOrder> book;

// Bug 2 is exercised right here: this first order matches the empty ask side.
MyOrder bid{scob::Side::Buy, scob::OrderType::Limit, 100, 5, 1};
for (auto e = book.accept_order(bid); e;) e();          // rests bid @100

// Bug 1: a marketable sell exactly at the touch.
MyOrder sell{scob::Side::Sell, scob::OrderType::Limit, 100, 5, 2};
int fills = 0;
for (auto e = book.accept_order(sell); e;) { e(); ++fills; }

// observed: fills == 0, and book.bid() and book.ask() are BOTH non-empty (locked at 100)
// expected: fills == 1 (5 @100), book cleared

For Bug 2, the same program compiled with -D_GLIBCXX_DEBUG aborts on the first order with attempt to dereference a past-the-end iterator (the empty ask-side deque). On a normal build the read is undefined but happened not to fault in my runs, and no matching happens against the empty side regardless — the for loop starts at begin() == end(), so it never iterates and the erase(begin, begin) is a no-op — so the effect isn't always visible, but it is still reading past the end of an empty container.

Suggested fix (one guard change resolves both)

Guard on emptiness instead of on a strict price compare, and let the loop's own (already-inclusive) price_compare(order, *it) decide crossing:

auto it = m_levels.begin();

if (!m_levels.empty())          // was: if (price_compare(*it, order))
{
    for (; it != m_levels.end(); ++it)
    {
        if (quantity_of(order) == quantity_filled)  break;
        else if (price_compare(order, *it))         break;   // crosses at equal price, stops past it
        // ... unchanged ...
    }
    m_levels.erase(m_levels.begin(), it);
}

This short-circuits the empty side (fixes Bug 2) and lets an order at the exact touch cross via the inner test (fixes Bug 1), while a non-marketable order still stops immediately at :203 (the erase(begin, begin) is a no-op). The inner per-level FIFO match and the level erase are untouched. I verified it both ways: the repro above now fills 5 @100 and clears the book, and the -D_GLIBCXX_DEBUG build no longer trips on the first order.

---

This is just a time-stamped snapshot of one commit offered back in case it helps — happy to share the exact failing workload, and thanks again for a really enjoyable codebase to read.

Respectfully submitted.

[sadhbh-c0d3]

Thank you for finding this! It's always great to see people are using what is being built. You were right in both cases. I was thinking of using your proposed solution, but I just found that I can simply remove that check before loop altogether. Let me know of any further updates.

[OPTIONPOOL]

I'm glad that it was helpful! Thank you for the response.