[Bug Report] A sell order trades against bids priced below it (the price-cross check is applied only to the best bid)

[OPTIONPOOL]

Found while integrating maringantis/ordermatchingengine 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. While checking the matcher against the README's own examples, I noticed that once a sell has traded its best counterparty, it keeps trading down the book into bids priced below it, producing a fill where the two prices do not cross.

Pinned at current master (991cd8ad194b4313632ab8903c7c81391b26450d).

Environment

• Commit: 991cd8ad194b4313632ab8903c7c81391b26450d ("Release V1.0")
• Language/toolchain: C# / .NET. The repo's OrderMatchingEngine.sln targets .NET Framework 4.6.1; the matching logic is framework-independent, so I built OrderMatchingEngine/Program.cs as a console app and fed the orders on stdin.
• Build/run: compile OrderMatchingEngine/Program.cs (or msbuild OrderMatchingEngine.sln), then pipe the input below into the resulting executable. The TRADE lines on stdout are what to look at; the program finishes on a Console.ReadKey, so with stdin redirected it prints the trades and then throws on that final read (the output is flushed first). Running it interactively avoids the throw.

What happens

The README states the matching rule plainly: "if there's a price cross meaning someone is willing to buy at a price higher than or equal with the current selling price, these two orders are traded." When a sell is matched, the engine sorts the bids by price (highest first) and walks them. It correctly refuses to trade if the best bid is below the sell price, but after it has traded against at least one bid it stops checking the price cross, so it continues filling against lower bids — including bids priced strictly below the sell. The result is a trade between orders that do not cross.

Minimal reproduction

Input (a resting sell at 100, a bid at 100 that legitimately crosses, and a bid at 90 that does not):

SELL GFD 100 10 S1
BUY GFD 100 3 B1
BUY GFD 90 5 B2

Actual output:

TRADE B1 100 3 S1 100 3
TRADE B2 90 5 S1 100 5

Expected output (only the crossing pair trades; B2 at 90 cannot cross a sell at 100):

TRADE B1 100 3 S1 100 3

The second line trades a buy priced at 90 against a sell priced at 100 — visible right in the printed prices (90 ... 100), which do not cross.

Mechanism / root cause

In sellTrade (OrderMatchingEngine/Program.cs), the bids are ordered best-first and walked with an index counter:

var sortedDict = from entry in buyTable orderby entry.Value.order_price descending select entry;

int index = 0;
foreach (var num in sortedDict)
{
    ...
    if (currentSellOrderPrice > num.Value.order_price && index.Equals(0)) return;   // price-cross guard
    ...
    Console.WriteLine("TRADE {0} {1} {2} {3} {4} {5}", num.Key, num.Value.order_price,
        num_traded, currentSellOrderID, currentSellOrderPrice, num_traded);
    ...
    index++;
}

The price-cross guard is gated on index.Equals(0), so it only protects the first (highest-priced) bid. Once index has advanced past 0, the guard can never fire again, and the loop trades against every remaining bid unconditionally — even bids priced below the sell. In the repro, B1 (100) trades first and advances index to 1; B2 (90) then satisfies 100 > 90 but index.Equals(0) is now false, so the guard is skipped and B2 is filled at a non-crossing price.

Suggested fix

Because the bids are already sorted highest-price-first, the first bid that fails the cross test means every later bid also fails, so the early return is the right action at every iteration — it just shouldn't be limited to the first one. Dropping the && index.Equals(0) clause is sufficient:

-                if (currentSellOrderPrice > num.Value.order_price && index.Equals(0)) return;
+                if (currentSellOrderPrice > num.Value.order_price) return;

I applied this one change, rebuilt, and re-ran. The repro now prints only TRADE B1 100 3 S1 100 3, and the README's worked examples are unchanged (e.g. Example 5, where both bids legitimately cross, still prints both trades). The index variable is then unused and can be removed if you like, but leaving it is harmless.

---

This is just a time-stamped snapshot of one specific commit, offered back in case it's useful — not a comment on the project as a whole. The README's worked examples made this easy to pin down and check against; thanks for putting the code and those examples up.

Respectfully submitted.