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

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

Merged
merged 2 commits into from Jul 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions client/asset/dcr/config.go
Expand Up @@ -39,6 +39,7 @@ type walletConfig struct {
FeeRateLimit float64 `ini:"feeratelimit"`
RedeemConfTarget uint64 `ini:"redeemconftarget"`
ActivelyUsed bool `ini:"special:activelyUsed"` //injected by core
ApiFeeFallback bool `ini:"apifeefallback"`
}

type rpcConfig struct {
Expand Down
120 changes: 98 additions & 22 deletions client/asset/dcr/dcr.go
Expand Up @@ -9,9 +9,12 @@ import (
"crypto/sha256"
"encoding/binary"
"encoding/hex"
"encoding/json"
"errors"
"fmt"
"io"
"math"
"net/http"
"path/filepath"
"sort"
"strconv"
Expand Down Expand Up @@ -74,6 +77,12 @@ const (
// hierarchical deterministic key derivation for the internal branch of an
// account.
acctInternalBranch uint32 = 1

// externalApiUrl is the URL of the external API in case of fallback.
externalApiUrl = "https://explorer.dcrdata.org/insight/api"
Comment on lines +81 to +82
Copy link
Member

Choose a reason for hiding this comment

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

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

// testnetExternalApiUrl is the URL of the testnet external API in case of
// fallback.
testnetExternalApiUrl = "https://testnet.dcrdata.org/insight/api"
)

var (
Expand Down Expand Up @@ -126,6 +135,15 @@ var (
IsBoolean: true,
DefaultValue: false,
},
{
Key: "apifeefallback",
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 wallets " +
"that have recently been started.",
IsBoolean: true,
DefaultValue: false,
},
}

rpcOpts = []*asset.ConfigOption{
Expand Down Expand Up @@ -215,6 +233,7 @@ var (
swapFeeBumpKey = "swapfeebump"
splitKey = "swapsplit"
redeemFeeBumpFee = "redeemfeebump"
client http.Client
)

// outPoint is the hash and output index of a transaction output.
Expand Down Expand Up @@ -499,6 +518,8 @@ type ExchangeWallet struct {
feeRateLimit uint64
redeemConfTarget uint64
useSplitTx bool
apiFeeFallback bool
network dex.Network

tipMtx sync.RWMutex
currentTip *block
Expand Down Expand Up @@ -562,7 +583,7 @@ func NewWallet(cfg *asset.WalletConfig, logger dex.Logger, network dex.Network)
walletCfg.PrimaryAccount = defaultAcctName
}

dcr, err := unconnectedWallet(cfg, walletCfg, chainParams, logger)
dcr, err := unconnectedWallet(cfg, walletCfg, chainParams, logger, network)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -594,7 +615,7 @@ func NewWallet(cfg *asset.WalletConfig, logger dex.Logger, network dex.Network)

// unconnectedWallet returns an ExchangeWallet without a base wallet. The wallet
// should be set before use.
func unconnectedWallet(cfg *asset.WalletConfig, dcrCfg *walletConfig, chainParams *chaincfg.Params, logger dex.Logger) (*ExchangeWallet, error) {
func unconnectedWallet(cfg *asset.WalletConfig, dcrCfg *walletConfig, chainParams *chaincfg.Params, logger dex.Logger, network dex.Network) (*ExchangeWallet, error) {
// If set in the user config, the fallback fee will be in units of DCR/kB.
// Convert to atoms/B.
fallbackFeesPerByte := toAtoms(dcrCfg.FallbackFeeRate / 1000)
Expand Down Expand Up @@ -656,6 +677,8 @@ func unconnectedWallet(cfg *asset.WalletConfig, dcrCfg *walletConfig, chainParam
feeRateLimit: feesLimitPerByte,
redeemConfTarget: redeemConfTarget,
useSplitTx: dcrCfg.UseSplitTx,
apiFeeFallback: dcrCfg.ApiFeeFallback,
network: network,
}, nil
}

Expand Down Expand Up @@ -850,50 +873,103 @@ func (dcr *ExchangeWallet) Balance() (*asset.Balance, error) {

// FeeRate satisfies asset.FeeRater.
func (dcr *ExchangeWallet) FeeRate() uint64 {
if dcr.wallet.SpvMode() {
return 0 // EstimateSmartFeeRate needs dcrd passthrough
}
// Requesting a rate for 1 confirmation can return unreasonably high rates.
rate, err := dcr.feeRate(2)
const confTarget = 2
rate, err := dcr.feeRate(confTarget)
if err != nil {
dcr.log.Errorf("Failed to get fee rate: %v", err)
return 0
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) {
feeEstimator, is := dcr.wallet.(FeeRateEstimator)
if !is {
return 0, fmt.Errorf("fee rate estimation unavailable")
}
// estimatesmartfee 1 returns extremely high rates on DCR.
if confTarget < 2 {
confTarget = 2
}
estimatedFeeRate, err := feeEstimator.EstimateSmartFeeRate(dcr.ctx, int64(confTarget), chainjson.EstimateSmartFeeConservative)
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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

if err != nil {
dcr.log.Errorf("Failed to get fee rate from external API: %v", err)
return 0, err
}
// convert fee to atoms Per kB and error if it is greater than fee rate
// limit.
atomsPerKB, err := convertFeeToUint(ratePerKB)
if err != nil {
dcr.log.Errorf("Failed to convert fee to atoms: %v", err)
return 0, err
}
if atomsPerKB > dcr.feeRateLimit {
dcr.log.Errorf("Fee rate greater than fee rate limit: %v", atomsPerKB)
return 0, err
}
atomsPerKB, err := dcrutil.NewAmount(estimatedFeeRate) // atomsPerKB is 0 when err != nil
return atomsPerKB, nil
}

// convertFeeToUint converts a estimated feeRate from dcr/kB to atoms/kb.
func convertFeeToUint(estimatedFeeRate float64) (uint64, error) {
if estimatedFeeRate == 0 {
return 0, nil
}
atomsPerKB, err := dcrutil.NewAmount(estimatedFeeRate) // atomsPerkB is 0 when err != nil
if err != nil {
return 0, err
}
// Add 1 extra atom/byte, which is both extra conservative and prevents a
// zero value if the atoms/KB is less than 1000.
return 1 + uint64(atomsPerKB)/1000, nil // dcrPerKB * 1e8 / 1e3
// zero value if the atoms/kB is less than 1000.
return 1 + uint64(atomsPerKB)/1000, nil // dcrPerkB * 1e8 / 1e3
}

// externalFeeEstimator gets the fee rate from the external API
func externalFeeEstimator(ctx context.Context, net dex.Network, nb uint64) (float64, error) {
var url string
if net == dex.Testnet {
url = testnetExternalApiUrl
} else {
url = externalApiUrl
Comment on lines +941 to +942
Copy link
Member

@chappjc chappjc Jul 12, 2022

Choose a reason for hiding this comment

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

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).

}
url = url + "/utils/estimatefee?nbBlocks=" + strconv.FormatUint(nb, 10)
ctx, cancel := context.WithTimeout(ctx, 4*time.Second)
defer cancel()
r, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
chappjc marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return 0, err
}
httpResponse, err := client.Do(r)
if err != nil {
return 0, err
}
c := make(map[uint64]float64)
reader := io.LimitReader(httpResponse.Body, 1<<14)
err = json.NewDecoder(reader).Decode(&c)
httpResponse.Body.Close()
if err != nil {
return 0, err
}
estimatedFeeRate, ok := c[nb]
if !ok {
return 0, errors.New("no fee rate for requested number of blocks")
}
return estimatedFeeRate, nil
}

Copy link
Member

@chappjc chappjc Jul 15, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

// targetFeeRateWithFallback attempts to get a fresh fee rate for the target
// number of confirmations, but falls back to the suggestion or fallbackFeeRate
// via feeRateWithFallback.
func (dcr *ExchangeWallet) targetFeeRateWithFallback(confTarget, feeSuggestion uint64) uint64 {
// Fee estimation is not available in SPV mode.
if dcr.wallet.SpvMode() {
return dcr.feeRateWithFallback(feeSuggestion)
}

feeRate, err := dcr.feeRate(confTarget)
if err != nil {
dcr.log.Errorf("Failed to get fee rate: %v", err)
Expand Down
2 changes: 1 addition & 1 deletion client/asset/dcr/dcr_test.go
Expand Up @@ -141,7 +141,7 @@ func tNewWallet() (*ExchangeWallet, *tRPCClient, func(), error) {
}
walletCtx, shutdown := context.WithCancel(tCtx)

wallet, err := unconnectedWallet(walletCfg, &walletConfig{PrimaryAccount: tAcctName}, tChainParams, tLogger)
wallet, err := unconnectedWallet(walletCfg, &walletConfig{PrimaryAccount: tAcctName}, tChainParams, tLogger, dex.Simnet)
if err != nil {
shutdown()
return nil, nil, nil, err
Expand Down