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

swapper resume from db #856

Merged
merged 9 commits into from
Dec 11, 2020
Merged

swapper resume from db #856

merged 9 commits into from
Dec 11, 2020

Conversation

chappjc
Copy link
Member

@chappjc chappjc commented Nov 25, 2020

This removes the swap state gob files, loading necessary from DB on Swapper startup.

server/db

Add SwapArchiver.ActiveSwaps (impl. by (*Archiver).ActiveSwaps) to retrieve full details of all active swaps across all markets. Swapper's constructor uses this to resume active swaps.

Remove all the swap state hash get/set functions.

The meta table and it's state_hash column still exist, but I think I'm going to entirely drop this table and follow up with a PR that re-adds the meta table with a versioning scheme.

server/swap

Rip out:

  • All of the swap state/gob stuff.
  • The orderSwapTracker, which was used to infer order "completion" for the cancellation rate computations. The market now has these order fill/settlement/success responsibilities. A swapDone callback is provided to the Swapper from the consumer (DEX manager) to do this.
  • The offBook argument of Negotiate, which was used with the orderSwapTracker junk.
  • The the live waiter tracking. Clients retry init/redeem requests now, so jumping through hoops to recreate live coin waiters on swapper startup is not necessary.

The Swapper constructor now loads all active matches from the DB and reconstructs all of the matchTrackers. This uses the new Backend.CoinConfTime method for the SwapConfirmTime, which is not stored in the DB. The match request time field of matchTracker is also not recorded, so this is guessed on resume as the epoch close time plus a minute, which at worst gives the maker a little extra breathing room when resuming in NewlyMatched status.

server/market

Assume the responsibility of deciding when an order is successfully "completed" as per the cancellation rate definition. This is facilitated by the SwapDone method that the DEX manager bridges with the Swapper.

The SwapDone method also performs the same unbooking that the Swapper previous requested via the unbookHook, now removed.

server/dex

Remove the unbook hook, replacing it with a swapDone dispatcher.

server/auth

Sign no longer returns an error since the secp256k1 changes recently eliminated the error return.

@chappjc chappjc marked this pull request as ready for review November 26, 2020 00:41
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.

Made a lot of orders and swaps, suspending and just closing the server at different steps, then comparing the orderbooks and matches returned by the admin console, all returns with expected values. Am able to continue trading. Matches completed while the market is down are updated when it comes back online. Can't find anything wrong. And wow is the webserver ui easy to use now. Holy crap. DEX is legit.

server/market/market.go Outdated Show resolved Hide resolved
server/market/market.go Outdated Show resolved Hide resolved
@chappjc
Copy link
Member Author

chappjc commented Dec 1, 2020

Just a comment that the second commit (0e5c64b) is only touching on a mostly-unrelated can of worms. That is, the init and redeem requests sent by the client are synchronous, blocking in finalize{Swap,Redeem}Action while they wait for a response from server. That needs to change. Also, this commit addresses an issue with pending requests and reconnects: I realized that when these requests are sent and the server starts a coinwaiter or whatever other processing, and then the connection is lost before getting a response, the signAndRequest call will never get a response, just waiting until timeout, which is unfortunately very long (broadcast timeout). This commit forcibly expires any request handlers on reconnect causing signAndRequest to return. @buck54321 We've waffled on the appropriate timeouts for these requests and I've steered toward just as long as allowable, but given (1) the blocking issue and (2) the fact that core will retry finalize{Swap,Redeem}Action, I think the timeouts for signAndRequest can be shorter, but I still don't know what the right timeout is. If we make these things asynchronous, it might be a moot issue though.
Finally, in finalize{Swap,Redeem}Action, I think the match.SetStatus and update of proof.{T,M}akerStatus should be done on broadcast (e.g. in swapMatchGroup), esp. since on the first call after bcast you could be sitting at signAndRequest without having stored the coin you just broadcasted, plus it's redundant for repeated calls. The only thing we need to update the match with after signAndRequest is auth.{InitSig,InitStamp} if we got a response.

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.

+803 −1,237

Nice. So much cleaner now and roles are much clearer.

server/asset/btc/btc.go Outdated Show resolved Hide resolved
server/asset/btc/btc.go Outdated Show resolved Hide resolved
server/market/market.go Outdated Show resolved Hide resolved
server/swap/swap.go Outdated Show resolved Hide resolved
Comment on lines 447 to 463
// Load the maker's order.LimitOrder and taker's order.Order.
taker, _, err := s.storage.Order(sd.MatchData.Taker, sd.Base, sd.Quote)
if err != nil {
log.Errorf("Failed to load taker order: %v", err)
continue
Copy link
Member

Choose a reason for hiding this comment

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

Noting that this is a departure from normal because the Order we'll be tracking in Swapper is not backed by the same underlying being tracked in Market. Is that okay? I think the only thing we'd need to worry about is if we were somehow relying on the Filled/Remaining quantities to remain accurate, and a quick scan seems to say that we are not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, good catch and it's part of why it's better that market is back in charge of order status fully again. I will certainly make a note about the caveat you identified and have a scan myself.

Copy link
Member Author

@chappjc chappjc Dec 10, 2020

Choose a reason for hiding this comment

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

Turned out that (*Market).SwapDone was calling Remaining(). Fortunately since it's in Market, we don't have to infer based on type/force/fill because we can just do Book.HaveOrder(oid), so that was an easy fix.

Probably safest would be not to have an order.Order as an argument, and instead pass a whole bunch of separate data (ID, type, force, base, quote, user), but we'd need to hack together an Order anyway to use the coin unlocking and storage functions and I think that's even more error prone. We're returning to the idea of a FilledOrder that adds the annoying Filled/Remaining stuff so that the order.Order type can be immutable.. Maybe in the future. It's a big change to do right.

server/swap/swap_test.go Outdated Show resolved Hide resolved
@chappjc chappjc added this to the 0.2.0 milestone Dec 9, 2020
@chappjc
Copy link
Member Author

chappjc commented Dec 10, 2020

I reverted the commit "client/comms: expire response handlers on reconnect" because it was only appropriate for reconnects where the server had restarted, not for when a reconnect was just a connection issue. This is because authenticated requests handlers (or their child goroutines) will respond via AuthManager.Send, which looks up the current comms.Link on which the response should be sent, which means client can still get responses to requests made on a replaced link.

So instead of expiring response handlers client-side, I just changed the request timeout used for the signAndRequest calls in finalize{Init,Redeem}Action to a timeout that is a fraction of broadcast timeout so that if the server restarts before responding to requests the clients don't wait until revoke for a response than can never come. Related issue: #863

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.

3 participants