server/asset/{btc,doge,ltc,bch}: median-fee based fee rate estimation backup #1597

buck54321 · 2022-04-22T19:15:05Z

Add methods for calculating the median fee of the most recent block(s).
Ensure that there is sufficient data by requiring M transactions in the
last N blocks. If not enough data, fallback to a "no-competition" fee
rate. Implement a cache to prevent repeated scans between blocks.
Some chains provide the getblockstats RPC. Others require manual
scanning.

chappjc · 2022-04-23T01:47:35Z

Very cool for a fallback if estimate(smart)fee is bombing.

Although honestly if I'm picking a mainnet BTC fee rate, I barely even look at the recent past blocks, instead looking at the state of mempool. Say the next block is shaping up like the following:

Even if that mined block 733097 was full or nearly full, I'd make my decision based primarily on the next block's worth of mempool on the left:

If mempool is barely used (like above), I'd pick a value that was similar to the previous block's median fee rate or lower if the median was inexplicably high. Could just be 1 (the min rate) but that is risky because the next block might take a while. So here's where I think the median of past blocks is helpful, although subject to manipulation if there aren't many txns in the last block(s).
If that chunk of mempool is full (more than one "next blocks" on the left) or almost full, then I'd look closer at the histogram of the fee rates in that section of mempool:

I'd probably pick something around 7 or 8 in the above case case. Regardless, in this second case, I would barely consider the last mined block because the fees in mempool could be going up rapidly, and if I picked based on the last block, I could be immediately several blocks deep in mempool.

Do you think there's a way to bring mempool into the calculus you've got so far?

buck54321 · 2022-04-23T09:50:39Z

I'm happy to work some mempool in, but the whole point here is that mempool is not developed enough for estimatesmartfee. I'll take a look.

chappjc · 2022-04-23T13:54:20Z

I'm happy to work some mempool in, but the whole point here is that mempool is not developed enough for estimatesmartfee. I'll take a look.

Ah, for e.g. with doge mempool is probably fairly empty.
Is the assumption that BTC isn't gonna hit this code? It only would if the node is new and hasn't observed several blocks being mined. (estimatesmartfee is based on past blocks too, but based on time for txns to go from mempool to blocks rather than just current mempool)

chappjc · 2022-04-23T17:58:12Z

Another example from electrum x server, which can target a depth in the memory pool, which is conceptually what I do mentally
https://github.com/spesmilo/electrum/blob/59c1d03f018026ac301c4e74facfc64da8ae4708/RELEASE-NOTES#L34-L46

At the end of the day, if mempool is fairly empty, one could even go with the min relay fee, possibly plus some extra if blocks have historically been fairly full (where your current code would be valuable, although potentially tricked to overpaying). If mempool is of significant size, I'd only look at mempool, almost completely disregarding past blocks.

buck54321 · 2022-04-26T14:11:13Z

As per offline discussion, using mempool is a little fraught.

I'm admittedly still trying to wrap my head around estimatesmartfee, which comes down to the TxConfirmStats::EstimateMedianVal method, called from either CBlockPolicyEstimator::estimateCombinedFee or CBlockPolicyEstimator::estimateConservativeFee. From what I can tell though, TxConfirmStats is a data cache that is maintained internally as txs are received, and it has to collect for a couple blocks so it knows how long txs have been sitting. That's not something we could reasonably emulate with an RPC connection.

I do think that we could get something out of a static analysis of mempool txs, but this raises lots of questions about the right way to interpolate. I also question whether we should trust a mempool that the node clearly doesn't, but I don't know the details of how mempool is populated on startup.

Anyway, this block-based method is meant as a backup that should just kinda work for a rough estimate on any chain.

chappjc · 2022-04-26T14:19:11Z

Sounds reasonable. It's a sound backup.

For ref, here's a link to my early experimentation with estimatesmartfee in its warm-up stages: #501 In short, it took a new bitcoind on mainnet observing 5 blocks getting mined before it declared estimatesmartfee to have "sufficient" data. I think it was because it has to observe (a) txns entering its mempool and (b) those same txns getting mined, and observing those times and rates.

chappjc

tACK.

Just needs a rebase to deal with conflicts from when I added the block deserializaer.
We can go with your bool name, but I'd prefer my anylist approach, but whatever really.
Rebase branch I'm evaluating: chappjc@abb0db6

server/asset/btc/rpcclient.go

chappjc · 2022-05-07T19:05:17Z

server/asset/btc/rpcclient.go

+					prevs = make(map[int]int64, 1)
+					prevOuts[prevOut.Hash] = prevs
+				}
+				prevs[int(prevOut.Index)] = 0 // placeholder


// value will be retrieved subsequently, after all prevouts for this funding txhash are recorded

chappjc · 2022-05-07T19:08:00Z

server/asset/btc/rpcclient.go

+		if sz == 0 {
+			return 0, fmt.Errorf("size 0 tx %s", tx.TxHash())
+		}
+		rates = append(rates, uint64(fees)/uint64(sz))


Under non-error conditions you will always append to length numTxs, so I'd be tempted to do for i, tx := range txs and rates[i] = ... to spare modifying len in the slice header on each iteration, but nbd.

chappjc · 2022-05-07T19:16:22Z

server/asset/btc/btc.go

@@ -127,9 +130,19 @@ type Backend struct {
 	chainParams *chaincfg.Params
 	// A logger will be provided by the dex for this backend. All logging should
 	// use the provided logger.
-	log         dex.Logger
-	decodeAddr  dexbtc.AddressDecoder
-	estimateFee func(*RPCClient) (uint64, error)


❤️
I'm looking to make a similar change in the future to client where we have the awkward issue of a client/asset.Wallet having to be a RawRequester, something that stands out given it's RPC-oriented.

chappjc · 2022-05-07T19:18:43Z

server/asset/btc/btc.go

@@ -506,7 +555,7 @@ func (btc *Backend) InitTxSizeBase() uint32 {

 // FeeRate returns the current optimal fee rate in sat / byte.
 func (btc *Backend) FeeRate(_ context.Context) (uint64, error) {


Once we get around to updating (*RPCClient).call to accept a context we should pass it through estimateFee, esp. if we hit the "hard way" path. In fact, a ctx could still be quite helpful in those loops juts by doing if ctx.Err() != nil { return ctx.Err() } in case it's shutdown or canceled so it doesn't keep trying getrawtransactions e.g.

chappjc · 2022-05-07T19:20:06Z

server/asset/btc/btc.go

+	if err == nil && satsPerB > 0 {
+		return satsPerB, nil
+	} else if err != nil {
+		btc.log.Tracef("Estimatefee error for %s: %v", btc.name, err)


Just noticing that this could show <nil> for err if satsPerB were just zero. Could be confusing, but meh, it's a TRC.

chappjc · 2022-05-07T19:20:56Z

server/asset/btc/btc.go

+	feeCache struct {
+		sync.Mutex
+		fee  uint64
+		hash chainhash.Hash


Nice pattern.

chappjc · 2022-05-07T19:22:21Z

server/asset/btc/btc.go

+		satsPerB, err = btc.node.medianFeeRate()
+	}
+	if err != nil {
+		if errors.Is(err, ErrNoCompetition) {


unexport ErrNoCompetition?

chappjc · 2022-05-07T19:27:58Z

server/asset/btc/btc.go

+	// MaxFeeBlocks is the maximum number of blocks that can be evaluated for
+	// median fee calculations. If > 100 txs are not seen in the last
+	// MaxFeeBlocks, an ErrNoCompetition is returned. Default value is 5.


Makes sense, but the ErrNoCompetition seems to be internal, not really something of concern for consumer of BackendCloneConfig

chappjc · 2022-05-07T19:52:01Z

Oh, you could also update the hard way to use the msgblock, but it's fine as is.

Add methods for calculating the median fee of the most recent block(s). Ensure that there is sufficient data by requiring M transactions in the last N blocks. If not enough data, fallback to a "no-competition" fee rate. Implement a cache to prevent repeated scans between blocks. Some chains provide the getblockstats RPC. Others require manual scanning.

chappjc added this to the 0.5 milestone Apr 23, 2022

chappjc self-requested a review May 6, 2022 15:29

chappjc reviewed May 7, 2022

View reviewed changes

buck54321 added 2 commits May 21, 2022 20:52

review followup and other assorted touchups

b2379d8

buck54321 force-pushed the estimatefee branch from 89cd473 to b2379d8 Compare May 22, 2022 12:10

chappjc approved these changes May 22, 2022

View reviewed changes

chappjc merged commit 8f21170 into decred:master May 22, 2022

chappjc mentioned this pull request May 22, 2022

multi: add support for ZCash #1570

Merged

chappjc mentioned this pull request Nov 18, 2022

server/asset/doge: use feeConfs and reduce maxFeeBlocks #1965

Merged

		@@ -506,7 +555,7 @@ func (btc *Backend) InitTxSizeBase() uint32 {

		// FeeRate returns the current optimal fee rate in sat / byte.
		func (btc *Backend) FeeRate(_ context.Context) (uint64, error) {

server/asset/{btc,doge,ltc,bch}: median-fee based fee rate estimation backup #1597

server/asset/{btc,doge,ltc,bch}: median-fee based fee rate estimation backup #1597

Uh oh!

Conversation

buck54321 commented Apr 22, 2022

Uh oh!

chappjc commented Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buck54321 commented Apr 23, 2022

Uh oh!

chappjc commented Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chappjc commented Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buck54321 commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chappjc commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chappjc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chappjc commented May 7, 2022 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

chappjc commented Apr 23, 2022 •

edited

Loading

chappjc commented Apr 23, 2022 •

edited

Loading

chappjc commented Apr 23, 2022 •

edited

Loading

buck54321 commented Apr 26, 2022 •

edited

Loading

chappjc commented Apr 26, 2022 •

edited

Loading

chappjc left a comment •

edited

Loading

chappjc commented May 7, 2022 via email •

edited

Loading