[Bug Report] A sell crossing a resting bid fills at the aggressor's price, not the resting bid's price

[OPTIONPOOL]

Found while integrating 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. The engine is clean on the canonical workload (the benchmark adapter sidesteps it); this is a latent defect in the engine's own executeTrade that surfaces the moment a sell aggresses a resting bid, and reproduces with two orders driven straight through addOrder.

Pinned at current main (e378705); built C++17 (g++ -std=c++17 -O2), the matcher translation unit only (src/orderbook.cpp), OrderBook("FIFO") → addOrder synchronously on one thread.

OrderBook::executeTrade reports every fill at askOrder.price. That is the resting (maker) price only when the order already on the book is the ask — a buy lifting a resting offer. When a sell crosses a resting bid (a bid sitting above the incoming sell's limit), the maker is the bid, but the engine still prints the ask side's price, so the trade is reported at the incoming sell's limit instead of the resting bid's price. Under price-time priority the resting order should set the trade price (price improvement accrues to the aggressor), so this direction reports the wrong fill price. The fill quantity, the order ids, and the resulting book state are all correct — only the reported price is wrong, and only on the resting-bid side.

Mechanism. matchOrders() pairs the highest bid against the lowest ask and FIFO-matches their deque fronts while the book crosses (src/orderbook.cpp:61-95). Every fill funnels through the single trade site:

// src/orderbook.cpp:127-129
void OrderBook::executeTrade(Order& bidOrder, Order& askOrder, int quantity) {
    std::cout << "Trade executed: " << quantity << " @ " << askOrder.price
              << " (Bid ID: " << bidOrder.orderId
              << ", Ask ID: " << askOrder.orderId << ")" << std::endl;
}

The price is always askOrder.price. But which of bidOrder/askOrder is the resting maker depends on which side was just added — addOrder (:6) pushes the incoming order onto its level and then runs matchOrders(), so the aggressor is whichever order was just inserted. When that aggressor is the sell, the bid is the maker, and pricing at askOrder.price takes the aggressor's price rather than the maker's. The function has no way to know which order is resting, so it can't pick the right one as written.

Repro. Rest a BUY 5 @ 101, then send a crossing SELL 5 @ 100:

#include "orderbook.h"
#include <iostream>

int main() {
    OrderBook ob("FIFO");

    Order bid;                  // resting BUY 5 @ 101
    bid.type = Order::BUY;  bid.orderType = Order::LIMIT;
    bid.quantity = 5;       bid.price = 101.0;
    ob.addOrder(bid);           // no match yet

    Order ask;                  // incoming SELL 5 @ 100 — crosses (101 >= 100)
    ask.type = Order::SELL; ask.orderType = Order::LIMIT;
    ask.quantity = 5;       ask.price = 100.0;
    ob.addOrder(ask);           // matchOrders() -> executeTrade(...)
}

Output:

Trade executed: 5 @ 100 (Bid ID: 0, Ask ID: 1)

Expected @ 101 — the resting bid is the maker and sets the price. The mirror case is correct, which is what makes this easy to miss: rest a SELL 5 @ 100, then send a crossing BUY 5 @ 101, and the engine prints @ 100 (the resting ask) — right, because there askOrder.price happens to be the maker price.

Fix. Price the fill at the resting (maker) order. The robust way is to let executeTrade know which order is the aggressor; addOrder is the single entry point, so record the incoming order's id there and price at the other side:

--- a/src/orderbook.h
+++ b/src/orderbook.h
@@
     std::deque<Order> stopOrders; // Queue for stop orders
+    int aggressorId = -1; // id of the order currently being added (the taker)

--- a/src/orderbook.cpp
+++ b/src/orderbook.cpp
@@ void OrderBook::addOrder(Order order) {
 void OrderBook::addOrder(Order order) {
+    aggressorId = order.orderId;
     if (order.orderType == Order::STOP) {
@@ void OrderBook::executeTrade(Order& bidOrder, Order& askOrder, int quantity) {
-    std::cout << "Trade executed: " << quantity << " @ " << askOrder.price << " (Bid ID: " << bidOrder.orderId << ", Ask ID: " << askOrder.orderId << ")" << std::endl;
+    const Order& maker = (bidOrder.orderId == aggressorId) ? askOrder : bidOrder;
+    std::cout << "Trade executed: " << quantity << " @ " << maker.price << " (Bid ID: " << bidOrder.orderId << ", Ask ID: " << askOrder.orderId << ")" << std::endl;

With this applied, the repro above prints @ 101, and the resting-ask control still prints @ 100. (A lighter timestamp-only pick — price at the older order — also works, but Order::timestamp is millisecond wall-clock, so two orders in the same millisecond tie and would mis-price; the aggressor-id approach avoids that.) This only changes the reported trade price; matching order, fill quantities, counterparties, and book state are untouched.

Note on the std::cout-only fill emission (not the point of this report): fills are emitted only via std::cout inside executeTrade, with no callback or sink, so a host process can't observe them programmatically — integrating the engine meant adding a trade hook at this same site. That's an integration limitation rather than a matching bug; I mention it only because the patched line happens to be the same one.

---

This is a reproducible, time-stamped snapshot of e378705, offered back in case it's useful — not a verdict on the project. I didn't find an existing issue covering the trade price, and the line is unchanged on main. Happy to share the failing workload.

Respectfully submitted.