market,dex: trade suspension #287
Conversation
|
Took a cursory look. I'll just drop some random observations here.
|
The idea of this suspend is either a temporary trading halt with no market config changes, or a graceful stop of the market. Changing market settings would involve re-reading markets.json file and the complications you mentioned. I'm not shooting to address that here, but I don't want to make that work harder either.
The order book is already in the main DB. I'm working in another branch now to reload the book.
Hadn't considered the connect modification. The suspend notification would come though, yes. |
|
In addition to the Considering the possibility for changed market or asset parameters, a reasonable way to handle that is for the client to request a new config after the market resumes. Alternatively, the config response could be further complicated with multiple entries for the same market but with different start epochs. That doesn't sound nice though. Another complication I see is changing asset parameters that affect all markets using that asset. |
|
So I don't want to dwell on this too much since Swapper resume without dropping active swaps has to get done right away, and it's a big job. This PR mainly allows the market to shutdown without dropping queued closed epochs prior to handing off to negotiation, and doing it at a specified time. To wrap this PR up, a persist bool option needs to be added that will revoke/unbook all booked orders. The PR as it stands guarantees that there will be no orders in epoch status. Future work should cover market config reload and on-demand resume. |
|
Added the persist book option, with necessary book, db, and coinlock functionality. But keeping the PR as draft while I remove the resume option from the Suspend function. I believe resume is something best handled by the dex manager by tracking running and suspended market subsystems and manually (re)launching them via their Run goroutines. |
|
There is no longer a resume time because it over-complicated the Suspend function and Market's epoch cycling. Resume can be implemented by restarting Market.Run, and although this is done in the test and works, there is no resume function added here. The admin server has |
server/market/bookrouter.go
Outdated
| // Presently this kills the book router for this market. | ||
| // Depending on how resume is handled, the order feed may remain | ||
| // opened, allowing this router to wait for resume. | ||
| r.sendNote(route, subs, note) | ||
| break out |
There was a problem hiding this comment.
Some open questions about how resume will operate. We don't define a resume ntfn message, just startepoch in config, which is probably not sufficient for clients that never (re)connect. What would we like the client to do if they are subscribed to market X and it suspends? If persist=true, poll for the market status? I think the answer is a TradeResumption notification type, which this PR defines to aid in planning but does not use.
Oh, so presently there's only the TradeSuspension message when the market actually suspends, not when it's scheduled, that should change, although resending the TradeSuspension at suspend time couldn't hurt. The connect response also does not say anything yet. Some missing pieces here... |
|
@buck54321 After hacking on the comms part more, I think that communications regarding market status (running, suspend time, start/resume time) are better handled by the The The On the other hand, the order book subscription can include live changes to market status, e.g. In short, I think it's best to include market status in the |
|
To make the |
|
That all seems to make sense. There is one case that I'm uncertain about, though. If a client has an active order on some market which will be shutting down, but they are not currently subscribed to the order book, they should probably still get a suspend notification. |
I suspect the right approach to the trade suspend notification is to send at the time it is scheduled and when the suspend actually happens, but my thinking on the above is that if they have an active order and they are connected, they will be subscribed to that market's orderbook. If they are not connected when the suspend is scheduled, the config route provides the suspend schedule... although now that I say this, the scheduled suspend would seem to be appropriate to the first orderbook response when the user subscribes to the market feed. Starting a chat in Matrix about this now... Back in draft while I sort out the comms changes. |
chappjc
force-pushed
the
suspend
branch
3 times, most recently
from
828fe3f
to
b281c7d
Compare
|
As per issue #354, these changes send a I need to retest everything, and add some test coverage for Also, the exact structure of the |
| // ResumptionRoute is the DEX-originating request-type message informing the | ||
| // client of an upcoming trade resumption. This is part of the | ||
| // subscription-based orderbook notification feed. | ||
| ResumptionRoute = "resumption" |
There was a problem hiding this comment.
Unused. Here for planning.
| @@ -669,6 +690,7 @@ type OrderBook struct { | |||
| MarketID string `json:"marketid"` | |||
| Seq uint64 `json:"seq"` | |||
| Epoch uint64 `json:"epoch"` | |||
| // MarketStatus `json:"status"`// maybe | |||
There was a problem hiding this comment.
Also here for consideration, but it should be redundant with 'config' response data and the TradeSuspension + TradeResumption ntfns.
| StartEpoch uint64 `json:"startepoch"` | ||
| FinalEpoch uint64 `json:"finalepoch,omitempty"` | ||
| Persist *bool `json:"persistbook,omitempty"` // nil and omitted when finalepoch is omitted |
There was a problem hiding this comment.
I initially had a field ActiveEpoch uint64 , but due to the extra subsystem communication work I described this is not implemented.json:"epoch"
server/market/market.go
Outdated
| m.coinLockerBase.UnlockAll() | ||
| m.coinLockerQuote.UnlockAll() |
There was a problem hiding this comment.
I just realized that this is not correct when there are other markets with a shared asset since these book locks are really shared asset coin lockers for just this reason. Need to selectively unlock a slice of orders...
There was a problem hiding this comment.
Addressed this in 19a9e77 by returning purged order IDs, and explicitly unlocking the coins for just those orders, not all coins for asset managed by the coin locker.
| // Signal to the book router of the suspend: | ||
| notifyChan <- &updateSignal{ | ||
| action: suspendAction, | ||
| data: sigDataSuspend{ | ||
| finalEpoch: m.suspendEpochIdx, | ||
| stopTime: epochCloseTime, | ||
| persistBook: m.persistBook, | ||
| }, | ||
| } |
There was a problem hiding this comment.
This may not be necessary at all. It all depends on if the book router should stay running when the feeding market suspends, and how resume is implemented. My current thinking is that the DEX manager would handle the resume process, restarting the subsustems as it sees fit (noting that the book router's subscriptions would still persist).
| } | ||
| } | ||
|
|
||
| func (r *OrderRouter) Suspend(asSoonAs time.Time, persistBooks bool) map[string]*SuspendEpoch { |
There was a problem hiding this comment.
Not used presently, but there is likely a need for a "suspend all" with a time rather than immediate sigint shutdown.
| // Spare some resources if the market is closed now. Any orders that make it | ||
| // through to a closed market will receive a similar error from SubmitOrder. | ||
| if !tunnel.Running() { | ||
| return msgjson.NewError(msgjson.MarketNotRunningError, "market closed to new orders") | ||
| } |
There was a problem hiding this comment.
In the present design, the order router commanded the suspension and thus can track the status of each market, at least nominally, so this will likely not query the MarketTunnel in the future. However, handleLimit and the other order handlers should catch orders submitted to suspended markets asap to save resources rather than sending them all the way to SubmitOrder to be bounced.
|
Updated and tested |
|
The only remaining known issue here is the excessive coin unlocking mentioned in #287 (comment). This PR has been a bear, but I ironed out a number of related bugs and design issues in later commits, so it's worth it. Clicking ready for review, again, despite the open coin unlock issue. |
| type TradeResumption struct { | ||
| MarketID string `json:"marketid"` | ||
| StartEpoch uint64 `json:"startepoch"` | ||
| EpochLen uint64 `json:"epochlen,omitempty"` // maybe just ConfigChange bool `json:"configchange"` |
There was a problem hiding this comment.
Presumably, ConfigChange = true would trigger a new config request from every client. It would be nice if we could send only the necessary market and asset info right here instead. Nothing to change now though.
| // Set the orderRouter field now since the main loop below receives on it, | ||
| // even though SubmitOrderAsync disallows sends on orderRouter when the | ||
| // market is not running. | ||
| m.orderRouter = make(chan *orderUpdateSignal, 32) // implicitly guarded by m.runMtx since Market is not running yet |
There was a problem hiding this comment.
Since you're not restarting the Market anymore, I think this can move back to the constructor.
There was a problem hiding this comment.
The setup now supports starting the market again with Run/Start with the same Market instance. In TestMarket_Suspend I did this as a proof of concept: https://github.com/decred/dcrdex/pull/287/files#diff-92957aac6dd698f2d846e3b99ed7281dR465-R499 Possibly this will not work out in resume, but a subsystem manager in DEX that can simple restart the same market instance is the idea.
| switch u.action { | ||
| case newEpochAction: | ||
| switch sigData := u.data.(type) { |
There was a problem hiding this comment.
Is it correct to say that updateSignal.action is just used for logging now?
There was a problem hiding this comment.
It is, and debugging I suppose. Really the type switch is all that's needed.
Refactor epochPump (into epump.go) for suspend support.
Add Market.Suspend and Market.Running.
When persistBook is false, in-memory and persistent books are purged.
Add the PurgeBook method to Market to do this. PurgeBook calls the new
OrderArchiver.FlushBook method, clears the in-memory book, and unlocks
all coins. TODO: unlock just the book orders, not possible epoch orders.
Run shutdown sequence is now meticulous.
Use internal notifyChan with lifetime of Run, and async subscriber
broadcasts via the closure goroutine inside Run.
db: add OrderArchiver.FlushBook
Add the FlushBook method to the pg.Archiver type. It changes all orders
for a market that have status booked to status revoked, and it creates
and stores the server-generated pseudocancel orders. It does this all in
a single db transaction. This is similar to calling RevokeOrder
individually on all booked orders.
The new PurgeBook query template is added to do the bulk book-revoked
status change for a market.
coinlock: Add UnlockAll to CoinLocker interface.
book: add Clear method to Book
market: rework bookUpdateSignal -> updateAction+sigData
Remove bookUpdateSignal, which combined data used for many different
signals, and create updateSignal with action and data fields. Receivers
will type cast data into one of several sigData types that contain only
the data that is relevant to the action at hand.
msgjson, market: add TradeSuspension message
(*BookRouter).runBook now sends msgjson.TradeSuspension for
msgjson.SuspensionRoute when the market sends a sigDataSuspend
on the order feed. (*Market).Run sends this signal when cycleEpoch hits
the target suspend epoch index.
market,dex: SuspendMarket methods for OrderRouter and DEX mgr
Add (*OrderRouter).SuspendMarket, called from the DEX manager in the new
server/dex.(*DEX) SuspendMarket method.
server/admin: suspend and market info routes
msgjson: suspend data in config
server/dex: on suspend update config resp and send TradeSuspension
market: Add Status type and (*Market).Status
dex: add MarketStatus and MarketStatuses methods for admin API
admin: add /markets
The markets route summarizes configured markets. e.g.
$ dexadm markets
{
"btc_ltc": {
"running": true,
"epochlen": 6000,
"activeepoch": 264872312,
"startepoch": 264872312
},
"dcr_btc": {
"running": true,
"epochlen": 7000,
"activeepoch": 227033410,
"startepoch": 227033410
}
}
$ dexadm market/dcr_btc/suspend
{
"market": "dcr_btc",
"finalepoch": 227033424,
"supendtime": "2020-05-11T21:52:55Z"
}
$ dexadm markets{
"btc_ltc": {
"running": true,
"epochlen": 6000,
"activeepoch": 264872328,
"startepoch": 264872328
},
"dcr_btc": {
"running": true,
"epochlen": 7000,
"activeepoch": 227033424,
"startepoch": 227033424,
"finalepoch": 227033424,
"persistbook": true
}
}
$ dexadm markets
{
"btc_ltc": {
"running": true,
"epochlen": 6000,
"activeepoch": 264872329,
"startepoch": 264872329
},
"dcr_btc": {
"running": false,
"epochlen": 7000,
"activeepoch": 0,
"startepoch": 0,
"finalepoch": 227033424,
"persistbook": true
}
}
|
Just a manual squash. No changes since review. |
This deals with trade suspension on a per-market basis, and makes Market shutdown more courteous. This does not deal with
Marketresume, or cleanSwappershutdown or resume, which is going to be a significant task in itself.Notable areas of work:
epochPumpand(*Market).Runwork), with optional book purge and coin unlock (the book and coinlock work).TradeSuspensionmessage, and send it to clients fromserver/dex.DEXto subscribed clients viacomms/ServeronMarketsuspend command.Technically, a suspend involves:
Market'sRungoroutine after the final scheduled epoch.Marketscan startRunagain, and this is done in tests with the sameMarketinstance, but a resume command to do so is note implemented in this work.(*Market).Runbegins shutting down, it always allows the closed epoch processing pipeline (preimage collection and hand off of ordersSwapper) to complete/drain so no submitted orders are lost.OrderRouter, which submits the requests to the targetMarketand gets the suspend request result (final epoch and suspend time). It also has access to a new(*Market).Runningmethod to short circuit the new order processing rather than have the order rejected downstream by theMarket. Proxying all suspend requests through theOrderRouterputs it in a position to track running and suspended markets, and markets with pending suspension.Note that this also improves the regular dcrdex shutdown in response to a CTRL+C (and cancellation of all Contexts), in that a
Market'sRungoroutine will always allow the closed epoch processing pipeline to completely drain and no received orders are just dropped. dcrdex process interrupt is unlike a suspend in that it does not wait for the current epoch to close, but the closed epochs always complete processing. Better coordination of subsystem shutdown by theDEXmanager is still needed however.Major next steps:
Swapperneeds a clean shutdown/resume solution too, which I believe will be difficult or complex to do with the pg DB, so I'm considering options to dump tracked trades, swap statuses, etc. and everythingSwapperneeds to resume into a gob file or similar.