multi: fee rate estimate feed #1000

buck54321 · 2021-03-04T21:05:28Z

Add the most recent fee rate estimate to 1) The orderbook response, 2) the epoch_report notification, and 3) a custom fee_rate endpoint. There are two major reason for this.

SPV clients will have no way to estimate fees. They can use the suggested fee rate with some basic sanity checks.
This finally closes a small vulnerability regarding fee rates on zero-conf funding coins. Previously, we handled this only client side by ensuring a reasonably high fee rate. But since the server's latest fee rate is available at the client now, we can more justifiably reject low-fee zero-conf coin-funded orders. Right now, the filter is set to >= 90% of the most recently procured rate estimate.

Order estimates should also be more accurate now.

server/market/market.go

client/orderbook/orderbook.go

server/market/orderrouter.go

chappjc · 2021-03-15T14:23:53Z

client/core/trade.go

@@ -1599,7 +1599,8 @@ func (c *Core) redeemMatchGroup(t *trackedTrade, matches []*matchTracker, errs *
 	// Send the transaction.
 	redeemWallet, redeemAsset := t.wallets.toWallet, t.wallets.toAsset // this is our redeem
 	coinIDs, outCoin, fees, err := redeemWallet.Redeem(&asset.RedeemForm{
-		Redemptions: redemptions,
+		Redemptions:   redemptions,
+		FeeSuggestion: c.feeSuggestion(t.dc, t.dc.bookie(t.mktID), redeemWallet.AssetID, !t.Trade().Sell),


The feeSuggestion call can make a request to the server. That's a problem in redeemMatchGroup. We split out the actual init and redeem requests to make them asynchronous (sendInitAsync and sendRedeemAsync) because of the trouble this can cause, but ideally we'd have no requests at all for this. All of the automated (tick or block based) actions need to be snappy and not have the potential to block. Is there any way to avoid this here, either changing feeSuggestion or ensuring there is already a value ready somewhere for this?

chappjc · 2021-03-15T14:35:07Z

server/market/orderrouter.go

+		return nil
+	}
+	lastKnownFeeRate := r.feeSource.LastFeeRate(fundingAsset.ID)
+	feeMinimum := uint64(math.Round(float64(lastKnownFeeRate) * 0.9))


0.9 seems pretty reasonable. Let's make this a named constant and describe.

Also what do you think about a minimum difference allowed? e.g. say the last was 15, then feeMinimum is 14 (round(15*0.9 = 13.5)), and it makes me wonder if that's too tight tolerance toward the smaller end.
Obviously we don't want things slowed down, but blocking incoming orders would suck.

Or if all client eventually update would there theoretically be no difference if they all use the fee suggestion from one of the various sources?

client/asset/btc/btc.go

client/asset/dcr/dcr.go

client/orderbook/orderbook.go

server/market/bookrouter.go

client/core/trade.go

chappjc · 2021-03-30T00:15:26Z

When you rebase, you are welcome to squash too. A couple conflict notes: context was removed from data api constructor (was unused), and handleFeeRate can switch to using msg.Unmarshal as per the changes in #1014. The one conflict with the stopBook closure can go back to one line too if you like.

client/asset/interface.go

chappjc · 2021-03-30T18:25:27Z

client/orderbook/orderbook.go

+// copy creates a copy. Note that the OrderID is not a deep copy.
+func (o *Order) copy() *Order {
+	return &(*o)


Since order.OrderID is an array, it would be a deep copy too.

Oh, yeah. Nice.

Comment still says not a deep copy

That's alright. I realized this wasn't making a copy of anything actually. Go was simplifying it to just returning the same pointer: a71b9d6
Working on some tweaks for this.

chappjc · 2021-03-30T18:27:12Z

client/orderbook/orderbook.go


-	// Clear all orders, if any.
-	ob.orders = make(map[order.OrderID]*Order)


Funny, we must have been (unintentionally?) avoiding a race with a higher level synchronization mechanism in the caller.

chappjc · 2021-03-30T18:28:39Z

client/orderbook/orderbook.go

+	ob.ordersMtx.Lock()
+	ord, found := ob.orders[oid]
+	if found {
+		newOrder := ord.copy()
+		newOrder.Quantity = note.Remaining
+		ob.orders[oid] = newOrder
 	}
-
-	ord = ob.buys.UpdateRemaining(oid, note.Remaining)
-	if ord != nil {
-		return nil
+	ob.ordersMtx.Unlock()


I'm a little unsure about the need to replace the order in the map with a fresh Order instance, but also the need to do anything with the orders map at all since the two booksides refer to the map entries (pointers).
Did you observe a bug or did this just seem like a needed change?

Was an attempt at fixing a race condition. We can't update the field, since a caller might be reading, e.g.

dcrdex/client/core/bookie.go

Line 373 in 9bf1a3e

Qty: float64(o.Quantity) / conversionFactor,

But I do think I messed something up here. Lemme get into it.

Curious that the race detector hasn't hit it before. Maybe because when that code is called in bookie.go we've always been locking booky.mtx like in (*dexConnection).syncBook?

chappjc · 2021-03-30T18:29:31Z

client/orderbook/orderbook.go

+	ob.ordersMtx.Lock()
 	order, ok := ob.orders[oid]
+	ob.ordersMtx.Unlock()


Hmm, another surprising one. OrderBook use patterns must have been preventing concurrent access to the map.

I do believe that's the case, but not behavior I wanted to rely upon.

chappjc · 2021-03-30T18:45:50Z

server/market/bookrouter.go

+func (r *BookRouter) LastFeeRate(assetID uint32) uint64 {
+	return atomic.LoadUint64(r.feeRateCache[assetID])
+}


I had some thoughts about the roles of BookRouter, Market, and OrderRouter with respect to the fee rate fetching, caching, and retrieval that I shared in matrix. The following is my high level understanding of how the pieces currently fit and what is responsible for what:

The assets implement market.FeeFetcher, which is a FeeRate() (uint64, error) method. These are provided to Markets, who use the fetchers at match time to get fee rates per asset for all the match sets.
These base/quote fee rates make their way to the outgoing BookRouter via an epoch_report signal. The BookRouter stores these rates when it gets them.
BookRouter also implements LastFeeRate(assetID uint32) uint64 (this commented line), and OrderRouter uses BookRouter as a FeeSource interface for this method. The OrderRouter uses this interface to ensure zero conf funded orders have high enough fee rate on funding txn.
So BookRouter with it's fee rate cache is the fee source for both itself and OrderRouter, while Market uses the assets directly for fee rates.

One question I had was what fee rates are known/cached before the first epoch_report? Also, how can we populate the book router's fee rate cache earlier, ideally construction time?

A general suggestion I had was to reduce and clarify the roles of these three server/market subsystems with respect to fee rate fetching/caching. Presently the flow of fee rates is from RPC fetch in Market on epoch close -> fee rate cache in BookRouter via epoch_report -> retrieve from cache in OrderRouter via BookRouter's LastFeeRate method. This design makes BookRouter implement the cache, while Market actually does retrieval. So I'm contemplating if a FeeSource can be some other type implemented outside of the market package, like something in server/dex that can do the RPC fetch and cache management without troubling the server/market types. Also, server/dex.DEX has the backends so it can hit FeeRate() to prepopulate the fee rate caches before even firing up the market subsystems.

Part of the appeal of a type that handles fee fetching and caching is consolidating these responsibilities that are now shared between the three market subsystems, and a fetch can automatically update the cache directly. Both OrderRouter and BookRouter can get the same FeeSource, and calls from Market to this type's FetchFeeRate(assetID uint32) method (indirectly via wrapper for the relevant assetID) could update the cache.

chappjc · 2021-03-31T20:21:19Z

server/swap/swap.go

+			match.FeeRateBase = matchSet.FeeRateBase
+			match.FeeRateQuote = matchSet.FeeRateQuote


I think matchSet.Matches() -> dex/order.newMatch has this job now, so redundant I believe.

chappjc · 2021-03-31T21:30:27Z

server/dex/feemgr.go

+func (f *feeFetcher) FeeRate() uint64 {
+	r, err := f.Backend.FeeRate()
+	if err != nil {
+		log.Errorf("Error retrieving fee rate for %s: %v", f.Symbol, err)


should this return 0 instead of going on to atomic.StoreUint64?

There was a little chat in matrix about this. In short, no. If the backend returns an error, we would want to pass that on to the client as a zero, indicating no estimate is available. Returning any other value would require the server to reason about either the validity of outdated estimates or the meaning of particular errors, which is undesireable, imo.

client/core/trade.go

server/dex/feemgr.go

chappjc

Looks good and tests well. I had a handful of other questions and suggestions, but I'm happy to merge this regardless.

Getting client to hit the fee_rate route was a bit tricky too, needing to use dexcctl trade with the browser closed and unsubscribed from the book. Does that seem right?

I'm also still a bit confused by the OrderBook stuff and I'm wondering if this is gonna resolve #835

Add the most recent fee rate estimate to 1) The orderbook response, 2) the epoch_report notification, and 3) a custom fee_rate endpoint. There are two major reason for this. 1. SPV clients will have no way to estimate fees. They can use the suggested fee rate with some basic sanity checks. 2. This finally closes a small vulnerability regarding fee rates on zero-conf funding coins. Previously, we handled this only client side by ensuring a reasonably high fee rate. But since the server's latest fee rate is available at the client now, we can more justifiably reject low-fee zero-conf coin-funded orders. Right now, the filter is set to >= 90% of the most recently procured rate estimate. Order estimates should also be more accurate now.

chappjc reviewed Mar 4, 2021

View reviewed changes

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

chappjc reviewed Mar 4, 2021

View reviewed changes

client/orderbook/orderbook.go Outdated Show resolved Hide resolved

chappjc reviewed Mar 4, 2021

View reviewed changes

server/market/orderrouter.go Show resolved Hide resolved

chappjc added this to the 0.2 milestone Mar 6, 2021

buck54321 force-pushed the rate-feed branch 2 times, most recently from 68073fa to dcd8f50 Compare March 10, 2021 22:00

chappjc reviewed Mar 15, 2021

View reviewed changes

JoeGruffins approved these changes Mar 16, 2021

View reviewed changes

client/asset/btc/btc.go Outdated Show resolved Hide resolved

client/asset/dcr/dcr.go Outdated Show resolved Hide resolved

client/orderbook/orderbook.go Outdated Show resolved Hide resolved

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

chappjc reviewed Mar 29, 2021

View reviewed changes

client/core/trade.go Outdated Show resolved Hide resolved

buck54321 force-pushed the rate-feed branch from 0154b39 to 17437b8 Compare March 30, 2021 14:15

chappjc reviewed Mar 30, 2021

View reviewed changes

buck54321 force-pushed the rate-feed branch 3 times, most recently from 567a16b to b01f2ca Compare March 31, 2021 14:28

buck54321 mentioned this pull request Mar 31, 2021

eth: coin ID uses and encodings (discussion) #1021

Closed

chappjc reviewed Mar 31, 2021

View reviewed changes

client/core/trade.go Outdated Show resolved Hide resolved

chappjc reviewed Mar 31, 2021

View reviewed changes

server/dex/feemgr.go Show resolved Hide resolved

chappjc approved these changes Mar 31, 2021

View reviewed changes

chappjc mentioned this pull request Mar 31, 2021

client/asset: lower limit on redeem txn fee rate #801

Closed

buck54321 force-pushed the rate-feed branch from 35ca743 to a14a2f0 Compare March 31, 2021 23:42

chappjc merged commit 79a1cb0 into decred:master Mar 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: fee rate estimate feed #1000

multi: fee rate estimate feed #1000

buck54321 commented Mar 4, 2021 •

edited

Loading

chappjc Mar 15, 2021

chappjc Mar 15, 2021 •

edited

Loading

chappjc commented Mar 30, 2021 •

edited

Loading

chappjc Mar 30, 2021

buck54321 Mar 30, 2021

chappjc Mar 31, 2021

buck54321 Apr 1, 2021

chappjc Apr 1, 2021

chappjc Mar 30, 2021

chappjc Mar 30, 2021

buck54321 Mar 30, 2021

chappjc Mar 30, 2021 •

edited

Loading

chappjc Mar 30, 2021

buck54321 Mar 31, 2021

chappjc Mar 30, 2021 •

edited

Loading

chappjc Mar 31, 2021

chappjc Mar 31, 2021 •

edited

Loading

buck54321 Mar 31, 2021 •

edited

Loading

chappjc left a comment •

edited

Loading


		// Clear all orders, if any.
		ob.orders = make(map[order.OrderID]*Order)

		match.FeeRateBase = matchSet.FeeRateBase
		match.FeeRateQuote = matchSet.FeeRateQuote

multi: fee rate estimate feed #1000

multi: fee rate estimate feed #1000

Conversation

buck54321 commented Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

chappjc Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

chappjc commented Mar 30, 2021 • 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

Choose a reason for hiding this comment

chappjc Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

buck54321 Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

chappjc left a comment • edited Loading

Choose a reason for hiding this comment

buck54321 commented Mar 4, 2021 •

edited

Loading

chappjc Mar 15, 2021 •

edited

Loading

chappjc commented Mar 30, 2021 •

edited

Loading

chappjc Mar 30, 2021 •

edited

Loading

chappjc Mar 30, 2021 •

edited

Loading

chappjc Mar 31, 2021 •

edited

Loading

buck54321 Mar 31, 2021 •

edited

Loading

chappjc left a comment •

edited

Loading