From c1bcd0d5ad6db276760bcff00781591fbe88c6b2 Mon Sep 17 00:00:00 2001 From: norwnd Date: Thu, 9 Mar 2023 18:55:50 +0300 Subject: [PATCH] Acting on TODOs from previous commit --- client/core/bookie.go | 18 +++++------------- client/orderbook/epochqueue.go | 2 +- client/orderbook/orderbook.go | 28 ++++++++++++++++++++++------ 3 files changed, 28 insertions(+), 20 deletions(-) diff --git a/client/core/bookie.go b/client/core/bookie.go index b914f7b78c..b3d7ec0442 100644 --- a/client/core/bookie.go +++ b/client/core/bookie.go @@ -1168,16 +1168,7 @@ func handleEpochReportMsg(_ *Core, dc *dexConnection, msg *msgjson.Message) erro if err != nil { return fmt.Errorf("epoch report note unmarshal error: %w", err) } - // Acquiring dc.booksMtx here in dc.bookie serves as a barrier for notifications that - // come after we sent new subscription request (e.i. notes that need to be applied - // on top of snapshot we get from new subscription request will wait on that mutex - // here). Note, we don't acquire dc.booksMtx for the whole handleBookOrderMsg - // duration; that leaves some room for notifications from previous subscription to - // come and apply concurrently with us applying snapshot we just got from new - // subscription request. - // TODO: I'm not sure at the moment whether is OK to execute this notification from - // old subscription while Resetting order book concurrently. Could this result into - // mixed OrderBook state (for orders, candles cache, or something else) ? + // Locking dc.booksMtx to just get the bookie, nothing more. book := dc.bookie(note.MarketID) if book == nil { return fmt.Errorf("no order book found with market id %q", note.MarketID) @@ -1205,9 +1196,10 @@ func handleEpochOrderMsg(_ *Core, dc *dexConnection, msg *msgjson.Message) error // duration; that leaves some room for notifications from previous subscription to // come and apply concurrently with us applying snapshot we just got from new // subscription request. - // TODO: I'm not sure at the moment whether is OK to execute this notification from - // old subscription while Resetting order book concurrently. Could this result into - // mixed OrderBook state (for orders, candles cache, or something else) ? + // TODO: for epoch_order notification this ^ is only necessary because we currently + // update bookie.OrderBook.seq here below (if we don't wait on this barrier then + // bookie.OrderBook.seq monotonicity might get corrupted. There is no reason to do + // it and we can stop it with https://github.com/norwnd/dcrdex/issues/8. book := dc.bookie(note.MarketID) if book == nil { return fmt.Errorf("no order book found with market id %q", note.MarketID) diff --git a/client/orderbook/epochqueue.go b/client/orderbook/epochqueue.go index 894f413546..d4b8234eac 100644 --- a/client/orderbook/epochqueue.go +++ b/client/orderbook/epochqueue.go @@ -52,7 +52,7 @@ func (eq *EpochQueue) Enqueue(note *msgjson.EpochOrderNote) error { // Ensure the provided epoch order is not already queued. _, ok := eq.orders[oid] if ok { - return fmt.Errorf("%s is already queued", oid) + return fmt.Errorf("%s is already queued", oid) // should never happen } var commitment order.Commitment diff --git a/client/orderbook/orderbook.go b/client/orderbook/orderbook.go index c111244844..80a58720c8 100644 --- a/client/orderbook/orderbook.go +++ b/client/orderbook/orderbook.go @@ -504,6 +504,7 @@ func (ob *OrderBook) Enqueue(note *msgjson.EpochOrderNote) error { if idx > ob.currentEpoch { ob.currentEpoch = idx } else { + // This should never happen. ob.log.Errorf("epoch order note received for epoch %d but current epoch is %d", idx, ob.currentEpoch) } } @@ -532,8 +533,15 @@ func (ob *OrderBook) ValidateMatchProof(note msgjson.MatchProofNote) error { if noteSize == 0 || firstProof { return nil, nil } - return nil, fmt.Errorf("epoch %d match proof note references %d orders, but local epoch queue is empty", - idx, noteSize) + // Due to asynchronous nature of client-server communication, the way it's + // currently implemented (epochQueues, proofedEpoch and currentEpoch can fall + // out of sync because these are changed in non-atomic manner with respect to + // each other) we can't differentiate between server sending invalid match + // proof note and client's view on epoch orders being out of sync (non-atomic). + // Hopefully we can change it in the future, for now we don't want to treat + // it as error, logging in case it's useful for debugging and moving on. + ob.log.Tracef("epoch %d match proof note references %d orders, but local epoch queue is empty", idx, noteSize) + return nil, nil } eq, err := extractEpochQueue() if eq == nil /* includes err != nil */ { @@ -548,10 +556,18 @@ func (ob *OrderBook) ValidateMatchProof(note msgjson.MatchProofNote) error { if firstProof && localSize < noteSize { return nil // we only saw part of the epoch } - // Since match_proof lags epoch close by up to preimage request timeout, - // this can still happen for multiple proofs after (re)connect. - return fmt.Errorf("epoch %d match proof note references %d orders, but local epoch queue has %d", - idx, noteSize, localSize) + // Due to asynchronous nature of client-server communication, the way it's + // currently implemented (epochQueues, proofedEpoch and currentEpoch can fall + // out of sync because these are changed in non-atomic manner with respect to + // each other) we can't differentiate between server sending invalid match + // proof note and client's view on epoch orders being out of sync (non-atomic). + // Hopefully we can change it in the future, for now we don't want to treat + // it as error, logging in case it's useful for debugging and moving on. + ob.log.Tracef( + "epoch %d match proof note references %d orders, but local epoch queue has %d", + idx, noteSize, localSize, + ) + return nil } if len(note.Preimages) == 0 { return nil