multi: eliminate funding confirmation requirement by chappjc · Pull Request #499 · decred/dcrdex

chappjc · 2020-06-17T16:46:04Z

Implementation of spec change in #498

Note, this would replace PR #494

chappjc · 2020-06-17T17:03:39Z

 		return sum+toAtoms(unspent.rpc.Amount) >= dcr.reqFunds(value, size+unspent.input.Size(), nfo)
 	}
-	coins, _, _, err := dcr.fund(nfo.FundConf, enough)
+	coins, _, _, err := dcr.fund(0, enough)


The confirmation argument could be removed if this change sticks forever, but it doesn't hurt to remain. And I can see a client configuration that lets the user specify a min confirms for funding, but which is not required by the DEX and possibly not reflected by the balance.

chappjc · 2020-06-17T20:24:10Z

+		Trusted   float64  `json:"trusted"`
+		Untrusted float64  `json:"untrusted_pending"`
+		Immature  float64  `json:"immature"`
+		Used      *float64 `json:"used,omitempty"`


"trusted" : n, (numeric) trusted balance (outputs created by the wallet or confirmed outputs) "untrusted_pending" : n, (numeric) untrusted pending balance (outputs created by others that are in the mempool) "immature" : n, (numeric) balance from immature coinbase outputs "used" : n (numeric) (only present if avoid_reuse is set) balance from coins sent to addresses that were previously spent from (potentially privacy violating)

So "trusted" includes 0-conf outputs created by the wallet and confirmed outputs, while "untrusted_pending" can be thrown in the same bin as "immature" for the purposes of DEX wallet balance. This completely mitigates the potential attack, at least for BTC funded orders, described in #498 (comment)

Bumps our minNetworkVersion to 190000, and we lose litecoin which isn't there yet.

I was on 0.18. After building 0.20 (released a couple of weeks ago), I was seeing a Fee estimation failed. Fallbackfee is disabled error. I had to set -fallbackfee=0.00001 in the calls to bitcoind in the harness.

Any chance you're running Bitcoin with blocksonly? https://bitcoin.stackexchange.com/q/88621

I'm more than fine waiting for LTC.

Oh, bitcoin/bitcoin#16524

It seems that Bitcoin Core uses a combination of fees in past blocks and mempool history to estimate fees: https://github.com/bitcoin/bitcoin/blob/35ed88f187c9bc7eedc3a1cf12193e0fbf222057/src/policy/fees.h#L120-L125

So being fully syncd apparently is not enough to allow smart fee estimation to work. The node has to get "sufficient data" from mempool too to estimate fees, in the words of the -fallbackfee docs:

-fallbackfee=<amt> A fee rate (in BTC/kB) that will be used when fee estimation has insufficient data. 0 to entirely disable the fallbackfee feature. (default: 0.00)

I wish I knew what "sufficient data" was (some likely outdata description here), and that there was a way to query if it has this data before trying to make transactions.

But it's interesting that we would have hit this when going to mainnet if it were not for this 0.20 change that disabled the fallback fee for testnet and regnet. I think it's clear we don't want it making swap contract txns with a fallback fee though.

But it's interesting that we would have hit this when going to mainnet

Probably not though, since mainnet probably always has "sufficient data".

I'm more than fine waiting for LTC.

If you mean drop LTC support until they get up to 0.19, then I'm definitely not on board. Who knows when that'll be, and the old algorithm can be adapted in the meantime. I see a couple of options, both relying on a LegacyBalance function.

We could switch on RPC version, using LegacyBalance if < 0.19.

A new type, ltc.ExchangeWallet, with embeds btc.ExchangeWallet and does something like

type ExchangeWallet struct { btc.ExchangeWallet } func (w *ExchangeWallet) Balance() (*asset.Balance, error) { return w.LegacyBalance() }

If, on the other hand, you just meant that you can wait until I fix it in a follow up PR, then that's fine with me.

Trolling LTC for 0.19, not that they care: https://twitter.com/chappjc/status/1273405424419721217

But it's interesting that we would have hit this when going to mainnet

Probably not though, since mainnet probably always has "sufficient data".

Well it seems that this can mean a warm up period for your node to get mempool data to gauge congestion, hence why blocksonly=1 disables fee estimation. The following Reddit responses are vague and uninformative, but generally corroborated by other sources: https://www.reddit.com/r/Bitcoin/comments/dnw5h3/why_this_warning_fee_estimation_is_currently_not/

If you mean drop LTC support until they get up to 0.19, then I'm definitely not on board

That is actually what I meant, but we can work around it in the way you suggested in a follow up PR. I just don't see a pressing need for LTC support in DEX MVP as we'll have our hands full supporting DEX on the DCR-BTC pair for months, and I expect LTC is feeling a bit of pressure to catch up with BTC (they rolled the LTC 0.18 release exactly 8 days after BTC's 0.20 release, out of embarrassment I suspect).

There does seem to be a 0.20 branch of litecoin in their dev's fork. I've made an issue for us to evaluate it: #508

chappjc · 2020-06-17T20:27:27Z

 type rpcClient interface {
 	SendRawTransaction(tx *wire.MsgTx, allowHighFees bool) (*chainhash.Hash, error)
 	GetTxOut(txHash *chainhash.Hash, index uint32, mempool bool) (*chainjson.GetTxOutResult, error)
+	GetBalanceMinConf(account string, minConfirms int) (*walletjson.GetBalanceResult, error)


getbalance ("account" minconf=1) Calculates and returns the balance of all accounts. Arguments: 1. account (string, optional) The account name to query the balance for, or "*" to consider all accounts (default="*") 2. minconf (numeric, optional, default=1) Minimum number of block confirmations required before an unspent output's value is included in the balance Result: { "balances": [{ (array of object) Balances for all accounts. "accountname": "value", (string) Name of account. "immaturecoinbaserewards": n.nnn, (numeric) Immature Coinbase reward coins. "immaturestakegeneration": n.nnn, (numeric) Number of immature stake coins. "lockedbytickets": n.nnn, (numeric) Coins locked by tickets. "spendable": n.nnn, (numeric) Spendable number of coins. "total": n.nnn, (numeric) Total amount of coins. "unconfirmed": n.nnn, (numeric) Unconfirmed number of coins. "votingauthority": n.nnn, (numeric) Coins for voting authority. },...], "blockhash": "value", (string) Block hash. "totalimmaturecoinbaserewards": n.nnn, (numeric) Total number of immature coinbase reward coins. "totalimmaturestakegeneration": n.nnn, (numeric) Total number of immature stake coins. "totallockedbytickets": n.nnn, (numeric) Total number of coins locked by tickets. "totalspendable": n.nnn, (numeric) Total number of spendable number of coins. "cumulativetotal": n.nnn, (numeric) Total number of coins. "totalunconfirmed": n.nnn, (numeric) Total number of unconfirmed coins. "totalvotingauthority": n.nnn, (numeric) Total number of coins for voting authority. }

For DCR, calling this with minconf=0, we consider "spendable" as the Available balance, and "immaturecoinbaserewards" + "totalimmaturestakegeneration" + "totallockedbytickets" as immature, although the totallockedbytickets is debatable since it is not just immature (it needs to be spent by a vote or revoke before it can come back to the wallet). But for the purposes of the DEX wallet, it may make sense to account for it as immature rather than ignore it.

chappjc · 2020-06-17T20:30:06Z

+	confs int64
+	// TODO: consider including isDexChange bool for consumer


confs is not used by the caller (fund), but I'm thinking one of the simple mitigations for DCR would be to pick the confirmed ones first, as stated in the TODO in that function. Similarly, I've made the comment in this struct definition about possibly including a isDexChange flag and have retained that function, but not used it.

chappjc · 2020-06-17T20:30:46Z

-}
-
 // lockedAtoms is the total value of locked outputs, as locked with LockUnspent.
+// TODO: handle multiple accounts!


It occurred to me that Balance.Locked amount is going to include locked coins from all accounts, which is misleading.

chappjc · 2020-06-18T01:48:47Z

I'm reviewing #497 so we can get that in first

chappjc · 2020-06-18T15:22:55Z

+	// First try with confs>0.
+	ok, err := tryUTXOs(1)
+	if err != nil {
+		return nil, 0, 0, err
+	}
+	// Fallback to allowing 0-conf outputs.
+	if !ok {
+		ok, err = tryUTXOs(0)


The least we can do until we have safe/trusted support with dcrwallet: decred/dcrwallet#1769

buck54321

I would prefer to drop the extraneous return values, but this is otherwise ready. The fallbackfee harness updates are done in #497, and LTC will be temporarily busted.

chappjc · 2020-06-19T00:00:58Z

I'll need to squash this down to make the rebase easier, but I cleaned up the return values in 224ae4d.

dex/{,msgjson}: eliminate FundConf from Asset structs client: eliminate FundConf client: eliminiate BalanceSet and per-dex balances asset backend no longer have to do listunspent with a dex change check. btc: Use getbalances with trusted as Balance.Available, and untrusted_pending+immature as Balance.Immature. dcr: Use GetBalanceMinConf with immaturecoinbaserewards+ immaturestakegeneration as Balance.Immature. Include lockedbytickets in Balance.Locked along with locked utxos. bump minNetworkVersions getbalances required bitcoin core 0.20 litcoin does not support it yet, but bump to 0.18.1, which was released 2020/06/11. btc: filter btc outputs by "safe" flag dcr: try confirmed utxos first remove fundConf from sample-markets.json

chappjc mentioned this pull request Jun 17, 2020

spec: remove funding confirmations requirement #498

Merged

chappjc marked this pull request as ready for review June 17, 2020 16:58

chappjc changed the title ~~eliminate funding confirmation requirements~~ multi: eliminate funding confirmation requirement Jun 17, 2020

chappjc commented Jun 17, 2020

View reviewed changes

buck54321 reviewed Jun 17, 2020

View reviewed changes

Comment thread client/asset/btc/btc.go Outdated

chappjc commented Jun 17, 2020

View reviewed changes

buck54321 reviewed Jun 17, 2020

View reviewed changes

Comment thread client/asset/btc/btc.go Outdated

Comment thread client/core/core.go Outdated

Comment thread client/asset/dcr/dcr.go Outdated

buck54321 reviewed Jun 18, 2020

View reviewed changes

Comment thread client/asset/btc/btc.go Outdated

chappjc commented Jun 18, 2020

View reviewed changes

chappjc added server server components client labels Jun 18, 2020

buck54321 approved these changes Jun 18, 2020

View reviewed changes

Comment thread client/asset/btc/btc.go Outdated

Comment thread client/asset/dcr/dcr.go Outdated

chappjc force-pushed the no-fundconf branch from 224ae4d to 082e766 Compare June 19, 2020 00:17

chappjc merged commit 0f7d4fd into decred:master Jun 19, 2020

chappjc deleted the no-fundconf branch June 19, 2020 00:22

This was referenced Jun 19, 2020

swap: fix missing nil redemption check #494

Closed

server/swap: panic: nil pointer dereference in match.takerStatus.redemption.TxID() #490

Closed

This was referenced Jun 26, 2020

client/asset: change handling is insufficient #409

Closed

reimplement TxMonitored #141

Closed

itswisdomagain mentioned this pull request Aug 13, 2020

client/asset: correct available and locked balance calculations #548

Merged

		confs int64
		// TODO: consider including isDexChange bool for consumer

Conversation

chappjc commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chappjc Jun 17, 2020 • 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!

Uh oh!

chappjc Jun 17, 2020 • 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!

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 Jun 17, 2020 • 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!

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 Jun 17, 2020 • 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!

chappjc Jun 17, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chappjc commented Jun 18, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buck54321 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chappjc commented Jun 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chappjc commented Jun 17, 2020 •

edited

Loading

chappjc Jun 17, 2020 •

edited

Loading

chappjc Jun 17, 2020 •

edited

Loading

chappjc Jun 17, 2020 •

edited

Loading

chappjc Jun 17, 2020 •

edited

Loading

chappjc Jun 17, 2020 •

edited

Loading