client: Add external api request to get fee rate in case of fallback

[vctt94]

Partially addresses #1618

I am adding a new wallet config field external fee rate estimate, which is defaulted to false right now.

And estimating the fee with externalFeeEstimator for spv and rpc wallets.

For rpc wallet it is only checked in case of dcrd failure.

[buck54321]

Config items are set by the user via a corresponding ConfigOpt in the WalletDefinition.ConfigOpts. I agree that this is a wallet-level configuration option, and this is the right place for this field, but setting the option should not involve core, asset, or db in any way. We already have the system in place to populate Config.

{
	Key:         "allowexternalapirequest",
	DisplayName: "External fee rate estimates",
	Description: "Allow fee rate estimation from a block explorer API. " +
		"This is useful as a fallback for SPV wallets and RPC wallet th" +
		"at have just recently been started.",
	IsBoolean: true,
},

[vctt94]

Removed changes on core, asset and db and left them only on as wallet option. Much simpler indeed.

[vctt94]

Thanks for the review chapp. Adressed your comments.

[chappjc]

Thanks for those last changes. Will test it out shortly. Just two more tweaks please.

[JoeGruffins]

Still WIP? You can toggle the status to WIP and back to open somewhere now. You weren't able to before so it's new.

[buck54321]

About half of this diff is just because you picked a long name here and in ExchangeWallet. Can we not find a shorter way to say this? How about ApiFeeFallback?

AllowExternalAPIRequest doesn't mention fees anyway.

[JoeGruffins]

Looks good to me, but does it close #1618 ? I'm assuming this issue is for all assets.

[JoeGruffins]

Testnet also has one. https://testnet.dcrdata.org/insight/api would help with testing.

[chappjc]

Looks good to me, but does it close #1618 ? I'm assuming this issue is for all assets.

It does not. It's of particular importance for BTC SPV and even DOGE and ZEC full node that can't figure out how to get fee rates.

[buck54321]

if dex.Testnet == 1 is always true. You'll need to work with the dex.Network passed to NewWallet somehow. You could inspect the *chaincfg.Params to figure out what network you're on, but using the dex.Network would be better.

[vctt94]

my bad. Now I am adding a new global var isTestnet, which is stored on NewWallet and check when calling the externalFeeEstimator

[buck54321]

No globals here, please. Let's pass it in to unconnectedWallet and save it as a field on the ExchangeWallet.

[chappjc]

See the call to asset.OpenWallet about 6 lines down, providing c.net as an input arg. Then see:

func NewWallet(cfg *asset.WalletConfig, logger dex.Logger, network dex.Network) (*ExchangeWallet, error)

There is no need to add Network to asset.WalletConfig because it is already provided directly to NewWallet.

[vctt94]

should I just pass network directly to unconnectedWallet() instead?

like:
dcr, err := unconnectedWallet(cfg, walletCfg, chainParams, logger, network)

[chappjc]

Yeah, or set dcr.network = network in NewWallet. I honestly don't care which, but unconnectedWallet is probably right.

Also, I don't think the ExchangeWallet fields should be exported.

[chappjc]

Note that for simnet it's gonna use mainnet's fee estimates. This isn't bad TBH as it allows some more fee rate variability when testing (maybe not with DCR, but in general it would).

[buck54321]

What do you think of this refactor to reduce duplication?

// FeeRate satisfies asset.FeeRater.
func (dcr *ExchangeWallet) FeeRate() uint64 {
	const confTarget = 2
	rate, err := dcr.feeRate(confTarget)
	if err != nil {
		dcr.log.Errorf("feeRate error: %v", err)
	}
	return rate
}

// FeeRate returns the current optimal fee rate in atoms / byte.
func (dcr *ExchangeWallet) feeRate(confTarget uint64) (uint64, error) {
	// estimatesmartfee 1 returns extremely high rates on DCR.
	if confTarget < 2 {
		confTarget = 2
	}
	if feeEstimator, is := dcr.wallet.(FeeRateEstimator); is {
		ratePerKB, err := feeEstimator.EstimateSmartFeeRate(dcr.ctx, int64(confTarget), chainjson.EstimateSmartFeeConservative)
		if err == nil {
			return convertFeeToUint(ratePerKB)
		} else {
			dcr.log.Errorf("Failed to get fee rate with estimate smart fee rate: %v", err)
		}
	}
	// Either SPV wallet or EstimateSmartFeeRate failed.
	if !dcr.apiFeeFallback {
		return 0, fmt.Errorf("fee rate estimation unavailable")
	}
	dcr.log.Debug("Retrieving fee rate from external API")
	ratePerKB, err := externalFeeEstimator(dcr.ctx, dcr.network, confTarget)
	if err != nil {
		dcr.log.Errorf("Failed to get fee rate from external API: %v", err)
		return 0, err
	}
	return convertFeeToUint(ratePerKB)
}

[vctt94]

refactored

[chappjc]

Took a closer look and I think we've neglected two things.

[chappjc]

I'm just realizing that we're not doing any sanity checking or thresholding on the result from the external source. They could be malicious or maybe they changed the API from DCR/kB -> atoms/kB (or whatever) without warning.
The external source might also say 0.

I think after calling externalFeeEstimator we need to both catch zero and filter the result of convertFeeToUint through feeRateWithFallback

[vctt94]

Yeah, you are right, it was returning 1, which would not go into feeRateWithFallback. Returning 0 now if it is 0.

I am not checking for a upper limit though, should I?

[chappjc]

I really think we should apply our upper limit to any rates we get from an external source.

[vctt94]

sounds reasonable. Checking if it is greater than dcr.feeRateLimit now

[chappjc]

A few lines down there's still a if dcr.wallet.SpvMode() { that skips calling feeRate where the external API would be called. Thus we're still falling back to the wallet's hard coded fee rate without asking the external source in a lot of cases.

[vctt94]

lol I completely missed it. Removed it now, thanks for catching.