Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multi: handle trade suspension messages. #269

Merged
merged 10 commits into from
Jun 15, 2020

Conversation

dnldd
Copy link
Member

@dnldd dnldd commented Apr 15, 2020

This adds the suspension handler to the client core.

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've got an issue. The spec for suspension is still based off of a simpler time when we had a single epoch duration for the entire DEX. That was changed a while back. The suspension route can no longer send the epoch. It should probably be changed to either a timestamp, or a mapping of market->epoch or market->timestamp.


How do we reset the suspension? The docs do say

Users should expect to lose connection during suspension.

but I don't know if that's a strong enough guarantee that we could then hook into handleReconnect to catch resumption of trade and perform the necessary actions.

dex/msgjson/types.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the server should send a market->epoch map in the suspension message. one message per market

But as far as this PR goes, I think we should put it on hold and first address the issue from the server side to be sure it is practical.

Comment on lines 1105 to 1106
return nil, fmt.Errorf("order placement suspended due to impending"+
" trade suspension for dex %s", dc.acct.url)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth saying when this suspension is going to happen. (suspension epoch * epoch duration (ms)) We'd need to get this from the market map, but as @buck54321 pointed out, there will need to be a suspension epoch for each market.

@chappjc chappjc mentioned this pull request Apr 20, 2020
@chappjc
Copy link
Member

chappjc commented Apr 27, 2020

I'll wrap up the server-side trade suspension PR in a day or two. After we're sure what info will come from the server we can resume this one. Thanks for hanging in there on this one.

@dnldd
Copy link
Member Author

dnldd commented Apr 27, 2020

No worries, dcrpool work has been keeping me busy.

@chappjc
Copy link
Member

chappjc commented May 15, 2020

msgjson.TradeSuspension message is implemented server side now. a47e2cd#diff-9a36b4e081b2c1cc9032524815e5d57c

dcrdex/dex/msgjson/types.go

Lines 710 to 717 in 73a3f78

// TradeSuspension is the SuspensionRoute notification payload. It is part of
// the orderbook subscription.
type TradeSuspension struct {
MarketID string `json:"marketid"`
FinalEpoch uint64 `json:"finalepoch"`
SuspendTime uint64 `json:"suspendtime"`
Persist bool `json:"persistbook"`
}

@chappjc
Copy link
Member

chappjc commented May 15, 2020

Also, unless all of the markets are suspended, I don't think the client should expect a lost connection.

@dnldd
Copy link
Member Author

dnldd commented May 15, 2020

noted, will be back at it this weekend.

@chappjc
Copy link
Member

chappjc commented May 18, 2020

To clarify the current market suspend operation from the point of view of the client:

  • The server operator commands a suspend at as soon as a time or asap
  • dcrdex returns a final epoch and suspend time, and notifies connected clients immediately of the upcoming trade suspension with a msgjson.TradeSuspension.
  • Final epoch is the last epoch. It will be completed.
  • Suspend time is the time at which the market is scheduled to begin rejecting orders for that market.
  • Final epoch [start, end), where end is suspend time.

Also, TradeResumption is not implemented.

I imagine the client will simply prevent submission of orders to suspended markets.

Note that the config message also had this data, so clients that were offline when TradeSuspension is sent still have this info.

@dnldd
Copy link
Member Author

dnldd commented May 18, 2020

Noted, thank you.

@dnldd dnldd force-pushed the handle_suspension_msg branch 2 times, most recently from fe0b31d to a253026 Compare May 20, 2020 02:01
@dnldd
Copy link
Member Author

dnldd commented May 20, 2020

Apologies, I edited the existing commit instead of adding a new one, will remember not to in the updates to come. @chappjc please provide some feedback when you can. Also let me know if a unified type that would hold the market suspension time and resumption time for a market would be preferable here, instead of using suspension and resumption message maps.

@chappjc
Copy link
Member

chappjc commented May 25, 2020

Will have to review to give suggestion on the client implementation, but what to do really depends on server capabilities. I just put up a draft of the swapper suspend at #406 and it's looking feasible for the server to resend certain messages and to resume coin waiters. Will have a look at this tomorrow.

client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
@dnldd dnldd force-pushed the handle_suspension_msg branch 2 times, most recently from ca109b0 to 5c58b22 Compare May 27, 2020 21:14
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/db/bolt/db_test.go Outdated Show resolved Hide resolved
Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few concerns about concurrency. There's lots of mutexes.

client/order/orderbook.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Show resolved Hide resolved
client/core/core.go Show resolved Hide resolved
Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple more.

client/core/core.go Outdated Show resolved Hide resolved
client/core/core_test.go Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patience on this one. I felt we needed to get a better idea of what server is going to do before going too far down the suspend/resume road on the client side. I think this all makes sense for suspend. Just a few comments and questions. Will move on to the resume PR #442

client/order/orderbook.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
@chappjc
Copy link
Member

chappjc commented Jun 7, 2020

I'm going to outline a plan for "connectivity robustness" in a new issue tomorrow because we need to look at the bigger picture to decide what needs to be done.

"connectivity robustness" will cover any disruption, either planned or unplanned:

  • Planned trade suspensions, which may include market config changes and which may involve connectivity loss if all the markets on a DEX are suspended and the operator need to stop the dex (the spec says to expect this!). This PR starts to deal with this, but: (1) we've not talked about what happens when a client connects after the initial trade suspension message was already sent - the config response could create a pending suspend, (2) we've tossed around the idea of a second TradeSuspension at the instant of suspend just in case the client failed to figure out that a suspend is scheduled, and (3) trade suspension may be followed by connectivity loss - then what?
  • Unplanned loss of connectivity
    • client shuts down, server still up (what does server do while client is gone, and what do they both do on reconnect? Client resends certain requests or acks, even if the client sent them before going down? specifically, what? Wait for server to resend certain responses if relevant for active swaps?)
    • server shuts down, client still up (what does client do when server is gone, and what do they both do on reconnect? e.g. I've been dealing with this a lot in terms of the swap state, book restore, etc., but there is more to discuss.)
    • both client and server are still alive, just lost connection (there is no software that has a "startup recovery sequence", just a reconnect plan. should the server recognize mass outage? In general, I believe the client should bear most the burden of resuming swaps and checking on their orders, but certain server actions are still required. SendWhenConnected and RequestWhenConnected in the swap state PR touch on this )

client/core/core.go Outdated Show resolved Hide resolved
client/core/core_test.go Outdated Show resolved Hide resolved
client/core/core_test.go Outdated Show resolved Hide resolved
client/core/core_test.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core_test.go Outdated Show resolved Hide resolved
client/core/types.go Outdated Show resolved Hide resolved
client/core/types.go Outdated Show resolved Hide resolved
client/order/bookside.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core_test.go Outdated Show resolved Hide resolved
client/core/core_test.go Outdated Show resolved Hide resolved
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still pondering if a final TradeSuspension message should also be sent from the server at the moment of market suspend, which would need handling client side of course, but I'm good with this as a start.

I would also like to call out the different scenarios mentioned in #269 (comment), namely what happens when a client connects when a suspension is already scheduled. Does the client create a pendingSuspension or flag a market as suspended based on the 'config' response? If not, it will need to.

@chappjc
Copy link
Member

chappjc commented Jun 15, 2020

Once @buck54321's reviews are addressed and he approves, please rebase/resolve/squash.

@chappjc chappjc merged commit 93971d5 into decred:master Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants