market: implement MarketTunnel #85

chappjc · 2019-11-20T15:17:43Z

This implements two pieces of the MarketTunnel interface:

MidGap
CoinLocked

server/market/market.go

chappjc · 2019-12-03T14:49:05Z

dex/order/order.go

+	// TODO!!! The orders must be updated to track the current backing coins,
+	// not just the original backing coins. This is critical for partially
+	// filled orders where each fill creates change that must then be tracked as
+	// the new backing coins.


This is a big TODO. We discussed this in matrix recently, but in summary, the Order needs to know the coins that are currently used to back the order. This can change because of partial fills.

Say an order is partially filled and it stays on the books. Now say the swap succeeds and the backing coins are spent. From this swap that partially filled the order, there was change. That change would need to be tracked now as it would be used to back the order with the new/reduced remaining amount.

I don't think we should try to track the change output with the coin locker. The changes in #91 left a TxID method to hopefully deal with change outputs. See the use of (MarketTunnel).TxMonitored by the OrderRouter. I was guessing that the "dex-monitored" change output can be checked by asking the Swapper whether it is currently tracking the transaction ID. The Swapper now saves the Coins involved in the swap, so it should be straightforward to implement.

Implementation of coin tracking through the locker would necessitate the user submitting the change coin information with the init and redeem route payloads, and probably some messy edge cases. Notably, right now, the exchange wallet interface from #72 and #92 don't even give the user information about the coin output. It's tracked internally.

So it seems the swapStatus.coins needs to be used in the Swapper's coin locker? I think it will be clear to you what needs to be done there when you get a look at the swap.go changes.

As it stands, I think this PR is going to fail to lock or double lock partial filled orders with several active matches.

The changes in #91 left a TxID method to hopefully deal with change outputs. See the use of (MarketTunnel).TxMonitored by the OrderRouter. I was guessing that the "dex-monitored" change output can be checked by asking the Swapper whether it is currently tracking the transaction ID. The Swapper now saves the Coins involved in the swap, so it should be straightforward to implement.

Is the following diff what you had in mind? https://github.com/decred/dcrdex/pull/95/files#diff-5f1d20749025f318cd1a8ba0915ebd37R418-R456

chappjc · 2019-12-03T14:50:49Z

server/coinlock/coinlocker.go

+func (ac *AssetCoinLocker) LockCoins(orderCoins map[order.OrderID][]CoinID) {
+	ac.coinMtx.Lock()
+	for oid, coins := range orderCoins {
+		ac.lockedCoinsByOrder[oid] = coins


Note that this would need to be an append if the set of coins tracked by a given order changes.

chappjc · 2019-12-03T14:52:45Z

server/coinlock/coinlocker.go

+}
+
+// UnlockOrderCoins unlocks any coins backing order.
+func (ac *AssetCoinLocker) UnlockOrderCoins(oid order.OrderID) {


Unlocking by order ID may prove to be inappropriate for the same reasons mentioned before, at least for the Swapper's CoinLocker. Unlocking by CoinID may be needed.

May prove to be a nice feature, but not a priority, IMO. Locking only the coins that have been spent is possible with no changes to the current backend interface though.

chappjc · 2019-12-03T14:54:04Z

server/coinlock/coinlocker.go

+type DEXCoinLocker struct {
+	masterLocks map[uint32]*MasterCoinLocker
+}
+
+func NewDEXCoinLocker(assets []uint32) *DEXCoinLocker {
+	masterLocks := make(map[uint32]*MasterCoinLocker, len(assets))
+	for _, asset := range assets {
+		masterLocks[asset] = NewMasterCoinLocker()
+	}
+
+	return &DEXCoinLocker{masterLocks}
+}
+
+func (c *DEXCoinLocker) CoinLocked(asset uint32, coin string) bool {
+	locker := c.masterLocks[asset]
+	if locker == nil {
+		panic(fmt.Sprintf("unknown asset %d", asset))
+	}
+
+	return locker.CoinLocked(CoinID(coin))
+}
+
+func (c *DEXCoinLocker) OrderCoinsLocked(asset uint32, oid order.OrderID) []CoinID {
+	locker := c.masterLocks[asset]
+	if locker == nil {
+		panic(fmt.Sprintf("unknown asset %d", asset))
+	}
+
+	return locker.OrderCoinsLocked(oid)
+}


These may or may not be used by the highest level DEX component, hence not commented, but kept here to facilitate the next steps.

chappjc · 2019-12-03T14:57:30Z

server/matcher/match.go

 				makers = matchSet.Makers
 			} else if o.Force == order.ImmediateTiF {
 				// There was no match and TiF is Immediate. Fail.
 				failed = append(failed, q)
 				break
 			}

+			// Either matched or standing unmatched => passed.
+			passed = append(passed, q)


It was a bug that passed only applies to matched orders, but not newly-booked/standing unbooked orders.

As of right now, it doesn't look like we are using the passed group anywhere. Do you anticipate that we will need it?

I haven't had to use it. Will remove.

Actually, going to remove in #95 since it touches Matcher too.

dex/order/order.go

chappjc · 2019-12-03T15:23:18Z

There is the massive TODO regarding the potentially changing CoinID set for an order as it is partially filled, but I think that is slightly beyond the scope of this PR.

The main thing in this PR is the coinlocker types that provide a shared underlying coin locking system between book and swapper (and markets).

buck54321

The coin locker API and the scope of the coin handling were a little larger than I expected, but it seems to be tuned in to the DEX's needs. Just a couple things that need attention, but otherwise looks good here.

server/book/book.go

buck54321 · 2019-12-03T17:51:04Z

server/matcher/match.go

 				makers = matchSet.Makers
 			} else if o.Force == order.ImmediateTiF {
 				// There was no match and TiF is Immediate. Fail.
 				failed = append(failed, q)
 				break
 			}

+			// Either matched or standing unmatched => passed.
+			passed = append(passed, q)


As of right now, it doesn't look like we are using the passed group anywhere. Do you anticipate that we will need it?

server/db/driver/pg/internal/orders.go

server/market/market.go

buck54321 · 2019-12-03T23:59:39Z

server/swap/swap.go

+type LockableAsset struct {
+	*asset.Asset
+	coinlock.CoinLocker // should be *coinlock.AssetCoinLocker
+}


This is gonna get interesting. #72 and #92 move most of asset.Asset to the dex module, and creates a BackedAsset in the server/asset module. It'll all need to come together somehow.

I'm also considering proposing to rename DEXAsset to AssetBackend or similar, which I think is more descriptive.

buck54321 · 2019-12-04T00:21:15Z

server/market/market.go

@@ -327,6 +425,20 @@ func (m *Market) processOrder(rec *orderRecord, epoch *EpochQueue, errChan chan<
 		return
 	}

+	// Ensure that the received order does not use locked coins.
+	lockedCoins := m.coinsLocked(ord)


These coins are being checked in both the order router and the market. Is that the intention?

Since the epoch processing loop in Market.runEpochs runs concurrently with the order router handlers registered with the AuthManager, and coins aren't locked until runEpochs does Market.processOrder, there needs to be a check somewhere in the synchronous processing pipeline of runEpochs/processOrder. I think we could eliminate the CoinLocked call in (*OrderRouter).checkPrefixTrade, but the early extra check seemed fine.

buck54321 · 2019-12-04T00:23:34Z

server/market/orderrouter.go

 	// storage, since active orders and active matches are tracked there.
-	OutpointLocked([]byte) bool
+	CoinLocked(coinID order.CoinID, assetID uint32) bool


You could also pass the DEXCoinLocker directly to the order book router, if that makes things easier.

OK either way. But see what you make of my comment regarding the extra lock check in #85 (comment)

buck54321 · 2019-12-04T00:32:24Z

server/market/market.go

+// CoinLocked checks if a coin is locked. The asset is specified since we should
+// not assume that a CoinID for one asset cannot be made to match another
+// asset's CoinID.
+func (m *Market) CoinLocked(asset uint32, coin coinlock.CoinID) bool {


I made a comment in the order router too, but if it's easier to pass the DEXCoinLocker to the order router, this method could be dropped from MarketTunnel.

chappjc · 2019-12-05T22:19:32Z

A book bugfix is in progress. Do not re-review yet.

chappjc · 2019-12-05T23:39:36Z

Bug fix in #97 is required to be merged first.

chappjc · 2019-12-06T17:56:48Z

PR #97 with the book bug fix is merged and this is rebased. Tests working now. Also added the tests for ActiveOrderCoins, and changed it so it is not limited to standing limit orders but any active orders even those with epoch status (although that doesn't make a lot of sense on startup when the epoch is empty). The details of shutdown and start up will need to be considered more carefully in the future before this detail is resolved.

chappjc · 2019-12-06T18:17:40Z

server/coinlock/coinlocker.go

+
+var _ (CoinLocker) = (*swapLocker)(nil)
+
+type coinIDKey string


This is kinda icky, so I'm open to other ideas for a map key, although since there's no restriction on CoinID length in bytes, I'm at a loss for ideas... except perhaps a hash of the slice, which would then be a fixed length.

I'm okay with strings. I know they take a performance hit in map look ups, but I'm guessing it's not significant enough to warrant the complexity of extra IDs.

chappjc · 2019-12-06T19:43:16Z

server/db/driver/pg/orders.go

+
+		// Deep copy the coin ID (a slice) since the backing buffer may be
+		// reused.
+		bc := make([]byte, cLen)
+		copy(bc, b[1:cLen+1])
+		c = append(c, bc)
+


This was subtle and hard to catch, but the lib/pq driver did hit it and the backing buffers were reused.

buck54321

Rebase is going to run into the LockedAsset/BackedAsset thing, but this is otherwise good to go.

buck54321 · 2019-12-10T23:22:22Z

server/market/market.go

+		// Since Swapper.Negotiate is called asynchronously, we must lock coins
+		// with the Swapper first. The swapper will unlock the coins.
+		m.swapper.LockOrdersCoins(swapOrders)


I'm not sure I follow. Why can't the Swapper lock the coins asynchronously?

I believe what could happen is a new order could arrive that is backed by coins that should be locked by the swapper, but since execution of go m.swapper.Negotiate is at the mercy by the Go scheduler, the new order may get accepted by the m.coinsLocked(ord) check in processOrder if the locking happens inside Negotiate.

market: Add and test MidGap, improve tests. market: CoinLocker, and Market.CoinLocked swap: LockableAsset in coins map market: update Market tests with CoinLocked, ActiveOrderCoins, and Swapper config changes with LockableAsset set cancel order ServerTime in match tests CI: Add structcheck and replace gofmt with goimports. matcher: add doneOK output doneOK = passed - booked Fix passed not including unmatched standing orders. swap: LockOrdersCoins separates orders by locked asset coins internally. Add unlockOrderCoins to unlock any order.Order's coins. Unlock coins in Swapper.revoke and in processBlock for completed matches.

…ng limit

SelectActiveOrderCoinIDs -> SelectOrderCoinIDs Restrict SelectOrderCoinIDs to market and limit orders (no cancel).

chappjc commented Nov 21, 2019

View reviewed changes

server/market/market.go Outdated Show resolved Hide resolved

chappjc force-pushed the markettunnel branch 3 times, most recently from 68fdf22 to da167e8 Compare November 26, 2019 19:48

buck54321 mentioned this pull request Nov 27, 2019

server: add missing server timestamp in tests #90

Merged

chappjc force-pushed the markettunnel branch from 2fb0eb8 to 79236f6 Compare November 27, 2019 14:41

buck54321 reviewed Dec 1, 2019

View reviewed changes

server/market/market.go Outdated Show resolved Hide resolved

chappjc force-pushed the markettunnel branch from 824cbe5 to 1e5234a Compare December 3, 2019 14:34

chappjc commented Dec 3, 2019

View reviewed changes

dex/order/order.go Show resolved Hide resolved

chappjc marked this pull request as ready for review December 3, 2019 15:23

buck54321 mentioned this pull request Dec 3, 2019

swap,market: add TxMonitored #95

Merged

buck54321 requested changes Dec 4, 2019

View reviewed changes

chappjc force-pushed the markettunnel branch from 56d7a7a to 6ff141f Compare December 5, 2019 22:19

chappjc force-pushed the markettunnel branch from 6ff141f to c29fb2a Compare December 6, 2019 15:28

chappjc commented Dec 6, 2019

View reviewed changes

buck54321 approved these changes Dec 10, 2019

View reviewed changes

chappjc and others added 4 commits December 11, 2019 14:11

market: MidGap handles nil best orders

8ebb94c

pg: rows.Err after rows.Next

e15ab2c

pg: ActiveOrderCoins includes any active order coins, not just standi…

430468e

…ng limit

chappjc added 2 commits December 11, 2019 14:11

limit book mtx

081f05f

pg: fix shallow copy and avoid buffer reuse in (*dbCoins).Scan

e94de6c

SelectActiveOrderCoinIDs -> SelectOrderCoinIDs Restrict SelectOrderCoinIDs to market and limit orders (no cancel).

chappjc force-pushed the markettunnel branch from d298d52 to e94de6c Compare December 11, 2019 20:11

chappjc merged commit cc08b9b into decred:master Dec 11, 2019

chappjc deleted the markettunnel branch December 11, 2019 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

market: implement MarketTunnel #85

market: implement MarketTunnel #85

chappjc commented Nov 20, 2019 •

edited

Loading

chappjc Dec 3, 2019

buck54321 Dec 3, 2019

chappjc Dec 3, 2019

chappjc Dec 3, 2019 •

edited

Loading

chappjc Dec 3, 2019

chappjc Dec 3, 2019

chappjc Dec 3, 2019 •

edited

Loading

buck54321 Dec 3, 2019

chappjc Dec 3, 2019

chappjc Dec 3, 2019

buck54321 Dec 3, 2019

chappjc Dec 5, 2019

chappjc Dec 6, 2019

chappjc commented Dec 3, 2019

buck54321 left a comment

buck54321 Dec 3, 2019

buck54321 Dec 3, 2019

buck54321 Dec 4, 2019

chappjc Dec 5, 2019 •

edited

Loading

buck54321 Dec 4, 2019

chappjc Dec 5, 2019

buck54321 Dec 4, 2019

chappjc commented Dec 5, 2019

chappjc commented Dec 5, 2019 •

edited

Loading

chappjc commented Dec 6, 2019

chappjc Dec 6, 2019 •

edited

Loading

buck54321 Dec 10, 2019

chappjc Dec 6, 2019

buck54321 left a comment

buck54321 Dec 10, 2019

chappjc Dec 11, 2019 •

edited

Loading


		var _ (CoinLocker) = (*swapLocker)(nil)

		type coinIDKey string

market: implement MarketTunnel #85

market: implement MarketTunnel #85

Conversation

chappjc commented Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc commented Dec 3, 2019

buck54321 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc commented Dec 5, 2019

chappjc commented Dec 5, 2019 • edited Loading

chappjc commented Dec 6, 2019

chappjc Dec 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buck54321 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Dec 11, 2019 • edited Loading

Choose a reason for hiding this comment

chappjc commented Nov 20, 2019 •

edited

Loading

chappjc Dec 3, 2019 •

edited

Loading

chappjc Dec 3, 2019 •

edited

Loading

chappjc Dec 5, 2019 •

edited

Loading

chappjc commented Dec 5, 2019 •

edited

Loading

chappjc Dec 6, 2019 •

edited

Loading

chappjc Dec 11, 2019 •

edited

Loading