client/asset: fix estimateSwap split rejection

The enough func in estimateSwap does not account for a split transaction at the start, so it is possible that funding for trySplit would actually choose more UTXOs. Actual order funding accounts for this. For this estimate, we will just not use a split tx if the split-adjusted required funds exceeds the total value of the UTXOs selected with this enough closure. The fix is to reject a split transaction if the output amount plus split tx fees is more than the sum of the amounts of the utxos selected by fund, rather than the total of the available utxos provided to fund initially. There is no point to calling fund again with a different enough func that accounts for the cost split tx because this indicates it would reqiure an additional UTXO, thus making the spit txn wasteful.
decred · Dec 29, 2022 · 5c511d4 · 5c511d4
1 parent 75e8aac
commit 5c511d4
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 93 deletions.
diff --git a/client/asset/btc/btc.go b/client/asset/btc/btc.go
@@ -1903,15 +1903,19 @@ func (btc *baseWallet) estimateSwap(lots, lotSize, feeSuggestion, maxFeeRate uin
 	bumpedMaxRate := maxFeeRate
 	bumpedNetRate := feeSuggestion
 	if feeBump > 1 {
-		bumpedMaxRate = uint64(math.Round(float64(bumpedMaxRate) * feeBump))
-		bumpedNetRate = uint64(math.Round(float64(bumpedNetRate) * feeBump))
+		bumpedMaxRate = uint64(math.Ceil(float64(bumpedMaxRate) * feeBump))
+		bumpedNetRate = uint64(math.Ceil(float64(bumpedNetRate) * feeBump))
 	}
 
 	val := lots * lotSize
-
+	// This enough func does not account for a split transaction at the start,
+	// so it is possible that funding for trySplit would actually choose more
+	// UTXOs. Actual order funding accounts for this. For this estimate, we will
+	// just not use a split tx if the split-adjusted required funds exceeds the
+	// total value of the UTXO selected with this enough closure.
 	enough := func(inputsSize, inputsVal uint64) bool {
 		reqFunds := calc.RequiredOrderFundsAlt(val, inputsSize, lots, btc.initTxSizeBase,
-			btc.initTxSize, bumpedMaxRate)
+			btc.initTxSize, bumpedMaxRate) // no +splitMaxFees so this is accurate without split
 		return inputsVal >= reqFunds
 	}
 
@@ -1921,7 +1925,7 @@ func (btc *baseWallet) estimateSwap(lots, lotSize, feeSuggestion, maxFeeRate uin
 	}
 
 	reqFunds := calc.RequiredOrderFundsAlt(val, uint64(inputsSize), lots,
-		btc.initTxSizeBase, btc.initTxSize, bumpedMaxRate)
+		btc.initTxSizeBase, btc.initTxSize, bumpedMaxRate) // same as in enough func
 	maxFees := reqFunds - val
 
 	estHighFunds := calc.RequiredOrderFundsAlt(val, uint64(inputsSize), lots,
@@ -1935,23 +1939,21 @@ func (btc *baseWallet) estimateSwap(lots, lotSize, feeSuggestion, maxFeeRate uin
 	} else {
 		estLowFunds += dexbtc.P2SHOutputSize * (lots - 1) * bumpedNetRate
 	}
-
 	estLowFees := estLowFunds - val
 
 	// Math for split transactions is a little different.
 	if trySplit {
-		_, extraMaxFees := btc.splitBaggageFees(bumpedMaxRate)
+		_, splitMaxFees := btc.splitBaggageFees(bumpedMaxRate)
 		_, splitFees := btc.splitBaggageFees(bumpedNetRate)
-		locked := val + maxFees + extraMaxFees
-
-		if avail >= reqFunds+extraMaxFees {
+		reqTotal := reqFunds + splitMaxFees // ~ rather than actually fund()ing again
+		if reqTotal <= sum {
 			return &asset.SwapEstimate{
 				Lots:               lots,
 				Value:              val,
-				MaxFees:            maxFees + extraMaxFees,
+				MaxFees:            maxFees + splitMaxFees,
 				RealisticBestCase:  estLowFees + splitFees,
 				RealisticWorstCase: estHighFees + splitFees,
-			}, true, locked, nil
+			}, true, reqFunds, nil // requires reqTotal, but locks reqFunds in the split output
 		}
 	}
 

diff --git a/client/asset/btc/btc_test.go b/client/asset/btc/btc_test.go
@@ -1305,7 +1305,7 @@ func testFundingCoins(t *testing.T, segwit bool, walletType string) {
 	ensureGood()
 }
 
-func checkMaxOrder(t *testing.T, wallet asset.Wallet, lots, swapVal, maxFees, estWorstCase, estBestCase, locked uint64) {
+func checkMaxOrder(t *testing.T, wallet asset.Wallet, lots, swapVal, maxFees, estWorstCase, estBestCase uint64) {
 	t.Helper()
 	maxOrder, err := wallet.MaxOrder(&asset.MaxOrderForm{
 		LotSize:       tLotSize,
@@ -1316,10 +1316,10 @@ func checkMaxOrder(t *testing.T, wallet asset.Wallet, lots, swapVal, maxFees, es
 	if err != nil {
 		t.Fatalf("MaxOrder error: %v", err)
 	}
-	checkSwapEstimate(t, maxOrder, lots, swapVal, maxFees, estWorstCase, estBestCase, locked)
+	checkSwapEstimate(t, maxOrder, lots, swapVal, maxFees, estWorstCase, estBestCase)
 }
 
-func checkSwapEstimate(t *testing.T, est *asset.SwapEstimate, lots, swapVal, maxFees, estWorstCase, estBestCase, locked uint64) {
+func checkSwapEstimate(t *testing.T, est *asset.SwapEstimate, lots, swapVal, maxFees, estWorstCase, estBestCase uint64) {
 	t.Helper()
 	if est.Lots != lots {
 		t.Fatalf("Estimate has wrong Lots. wanted %d, got %d", lots, est.Lots)
@@ -1344,11 +1344,6 @@ func TestFundEdges(t *testing.T) {
 	swapVal := uint64(1e7)
 	lots := swapVal / tLotSize
 
-	checkMax := func(lots, swapVal, maxFees, estWorstCase, estBestCase, locked uint64) {
-		t.Helper()
-		checkMaxOrder(t, wallet, lots, swapVal, maxFees, estWorstCase, estBestCase, locked)
-	}
-
 	// Base Fees
 	// fee_rate: 34 satoshi / vbyte (MaxFeeRate)
 	// swap_size: 225 bytes (InitTxSize)
@@ -1401,8 +1396,9 @@ func TestFundEdges(t *testing.T) {
 
 	var feeReduction uint64 = swapSize * tBTC.MaxFeeRate
 	estFeeReduction := swapSize * feeSuggestion
-	checkMax(lots-1, swapVal-tLotSize, backingFees-feeReduction, totalBytes*feeSuggestion-estFeeReduction,
-		(bestCaseBytes-swapOutputSize)*feeSuggestion, swapVal+backingFees-1)
+	checkMaxOrder(t, wallet, lots-1, swapVal-tLotSize, backingFees-feeReduction,
+		totalBytes*feeSuggestion-estFeeReduction,
+		(bestCaseBytes-swapOutputSize)*feeSuggestion)
 
 	_, _, err := wallet.FundOrder(ord)
 	if err == nil {
@@ -1412,7 +1408,8 @@ func TestFundEdges(t *testing.T) {
 	p2pkhUnspent.Amount = float64(swapVal+backingFees) / 1e8
 	node.listUnspent = unspents
 
-	checkMax(lots, swapVal, backingFees, totalBytes*feeSuggestion, bestCaseBytes*feeSuggestion, swapVal+backingFees)
+	checkMaxOrder(t, wallet, lots, swapVal, backingFees, totalBytes*feeSuggestion,
+		bestCaseBytes*feeSuggestion)
 
 	spendables, _, err := wallet.FundOrder(ord)
 	if err != nil {
@@ -1447,7 +1444,9 @@ func TestFundEdges(t *testing.T) {
 	p2pkhUnspent.Amount = float64(v) / 1e8
 	node.listUnspent = unspents
 
-	checkMax(lots, swapVal, backingFees, (totalBytes+splitTxBaggage)*feeSuggestion, (bestCaseBytes+splitTxBaggage)*feeSuggestion, v)
+	checkMaxOrder(t, wallet, lots, swapVal, backingFees,
+		(totalBytes+splitTxBaggage)*feeSuggestion,
+		(bestCaseBytes+splitTxBaggage)*feeSuggestion)
 
 	coins, _, err = wallet.FundOrder(ord)
 	if err != nil {
@@ -1569,11 +1568,6 @@ func TestFundEdgesSegwit(t *testing.T) {
 	swapVal := uint64(1e7)
 	lots := swapVal / tLotSize
 
-	checkMax := func(lots, swapVal, maxFees, estWorstCase, estBestCase, locked uint64) {
-		t.Helper()
-		checkMaxOrder(t, wallet, lots, swapVal, maxFees, estWorstCase, estBestCase, locked)
-	}
-
 	// Base Fees
 	// fee_rate: 34 satoshi / vbyte (MaxFeeRate)
 
@@ -1625,8 +1619,9 @@ func TestFundEdgesSegwit(t *testing.T) {
 
 	var feeReduction uint64 = swapSize * tBTC.MaxFeeRate
 	estFeeReduction := swapSize * feeSuggestion
-	checkMax(lots-1, swapVal-tLotSize, backingFees-feeReduction, totalBytes*feeSuggestion-estFeeReduction,
-		(bestCaseBytes-swapOutputSize)*feeSuggestion, swapVal+backingFees-1)
+	checkMaxOrder(t, wallet, lots-1, swapVal-tLotSize, backingFees-feeReduction,
+		totalBytes*feeSuggestion-estFeeReduction,
+		(bestCaseBytes-swapOutputSize)*feeSuggestion)
 
 	_, _, err := wallet.FundOrder(ord)
 	if err == nil {
@@ -1636,7 +1631,8 @@ func TestFundEdgesSegwit(t *testing.T) {
 	p2wpkhUnspent.Amount = float64(swapVal+backingFees) / 1e8
 	node.listUnspent = unspents
 
-	checkMax(lots, swapVal, backingFees, totalBytes*feeSuggestion, bestCaseBytes*feeSuggestion, swapVal+backingFees)
+	checkMaxOrder(t, wallet, lots, swapVal, backingFees, totalBytes*feeSuggestion,
+		bestCaseBytes*feeSuggestion)
 
 	spendables, _, err := wallet.FundOrder(ord)
 	if err != nil {
@@ -1668,7 +1664,9 @@ func TestFundEdgesSegwit(t *testing.T) {
 	p2wpkhUnspent.Amount = float64(v) / 1e8
 	node.listUnspent = unspents
 
-	checkMax(lots, swapVal, backingFees, (totalBytes+splitTxBaggageSegwit)*feeSuggestion, (bestCaseBytes+splitTxBaggageSegwit)*feeSuggestion, v)
+	checkMaxOrder(t, wallet, lots, swapVal, backingFees,
+		(totalBytes+splitTxBaggageSegwit)*feeSuggestion,
+		(bestCaseBytes+splitTxBaggageSegwit)*feeSuggestion)
 
 	coins, _, err = wallet.FundOrder(ord)
 	if err != nil {
@@ -2904,7 +2902,7 @@ func testPreSwap(t *testing.T, segwit bool, walletType string) {
 	maxFees := totalBytes * tBTC.MaxFeeRate
 	estHighFees := totalBytes * feeSuggestion
 	estLowFees := bestCaseBytes * feeSuggestion
-	checkSwapEstimate(t, preSwap.Estimate, lots, swapVal, maxFees, estHighFees, estLowFees, minReq)
+	checkSwapEstimate(t, preSwap.Estimate, lots, swapVal, maxFees, estHighFees, estLowFees)
 
 	// Too little funding is an error.
 	setFunds(minReq - 1)

diff --git a/client/asset/dcr/dcr.go b/client/asset/dcr/dcr.go
@@ -1185,18 +1185,23 @@ func (dcr *ExchangeWallet) estimateSwap(lots, lotSize, feeSuggestion, maxFeeRate
 	bumpedMaxRate := maxFeeRate
 	bumpedNetRate := feeSuggestion
 	if feeBump > 1 {
-		bumpedMaxRate = uint64(math.Round(float64(bumpedMaxRate) * feeBump))
-		bumpedNetRate = uint64(math.Round(float64(bumpedNetRate) * feeBump))
+		bumpedMaxRate = uint64(math.Ceil(float64(bumpedMaxRate) * feeBump))
+		bumpedNetRate = uint64(math.Ceil(float64(bumpedNetRate) * feeBump))
 	}
 
 	val := lots * lotSize
+	// The orderEnough func does not account for a split transaction at the
+	// start, so it is possible that funding for trySplit would actually choose
+	// more UTXOs. Actual order funding accounts for this. For this estimate, we
+	// will just not use a split tx if the split-adjusted required funds exceeds
+	// the total value of the UTXO selected with this enough closure.
 	sum, inputsSize, _, _, _, err := tryFund(utxos, orderEnough(val, lots, bumpedMaxRate))
 	if err != nil {
 		return nil, false, 0, err
 	}
 
 	reqFunds := calc.RequiredOrderFundsAlt(val, uint64(inputsSize), lots,
-		dexdcr.InitTxSizeBase, dexdcr.InitTxSize, bumpedMaxRate)
+		dexdcr.InitTxSizeBase, dexdcr.InitTxSize, bumpedMaxRate) // as in tryFund's enough func
 	maxFees := reqFunds - val
 
 	estHighFunds := calc.RequiredOrderFundsAlt(val, uint64(inputsSize), lots,
@@ -1210,17 +1215,17 @@ func (dcr *ExchangeWallet) estimateSwap(lots, lotSize, feeSuggestion, maxFeeRate
 
 	// Math for split transactions is a little different.
 	if trySplit {
-		extraFees := splitTxBaggage * bumpedMaxRate
+		splitMaxFees := splitTxBaggage * bumpedMaxRate
 		splitFees := splitTxBaggage * bumpedNetRate
-		if avail >= reqFunds+extraFees {
-			locked := val + maxFees + extraFees
+		reqTotal := reqFunds + splitMaxFees // ~ rather than actually fund()ing again
+		if reqTotal <= sum {
 			return &asset.SwapEstimate{
 				Lots:               lots,
 				Value:              val,
-				MaxFees:            maxFees + extraFees,
+				MaxFees:            maxFees + splitMaxFees,
 				RealisticBestCase:  estLowFees + splitFees,
 				RealisticWorstCase: estHighFees + splitFees,
-			}, true, locked, nil
+			}, true, reqFunds, nil // requires reqTotal, but locks reqFunds in the split output
 		}
 	}
 

diff --git a/client/asset/dcr/dcr_test.go b/client/asset/dcr/dcr_test.go
@@ -1101,16 +1101,16 @@ func TestFundingCoins(t *testing.T) {
 	ensureGood()
 }
 
-func checkMaxOrder(t *testing.T, wallet *ExchangeWallet, lots, swapVal, maxFees, estWorstCase, estBestCase, locked uint64) {
+func checkMaxOrder(t *testing.T, wallet *ExchangeWallet, lots, swapVal, maxFees, estWorstCase, estBestCase uint64) {
 	t.Helper()
 	_, maxOrder, err := wallet.maxOrder(tLotSize, feeSuggestion, tDCR.MaxFeeRate)
 	if err != nil {
 		t.Fatalf("MaxOrder error: %v", err)
 	}
-	checkSwapEstimate(t, maxOrder, lots, swapVal, maxFees, estWorstCase, estBestCase, locked)
+	checkSwapEstimate(t, maxOrder, lots, swapVal, maxFees, estWorstCase, estBestCase)
 }
 
-func checkSwapEstimate(t *testing.T, est *asset.SwapEstimate, lots, swapVal, maxFees, estWorstCase, estBestCase, locked uint64) {
+func checkSwapEstimate(t *testing.T, est *asset.SwapEstimate, lots, swapVal, maxFees, estWorstCase, estBestCase uint64) {
 	t.Helper()
 	if est.Lots != lots {
 		t.Fatalf("MaxOrder has wrong Lots. wanted %d, got %d", lots, est.Lots)
@@ -1136,10 +1136,6 @@ func TestFundEdges(t *testing.T) {
 	swapVal := uint64(1e8)
 	lots := swapVal / tLotSize
 
-	checkMax := func(lots, swapVal, maxFees, estWorstCase, estBestCase, locked uint64) {
-		checkMaxOrder(t, wallet, lots, swapVal, maxFees, estWorstCase, estBestCase, locked)
-	}
-
 	// Swap fees
 	//
 	// fee_rate: 24 atoms / byte (dex MaxFeeRate)
@@ -1183,8 +1179,10 @@ func TestFundEdges(t *testing.T) {
 
 	var feeReduction uint64 = swapSize * tDCR.MaxFeeRate
 	estFeeReduction := swapSize * feeSuggestion
-	checkMax(lots-1, swapVal-tLotSize, fees-feeReduction, totalBytes*feeSuggestion-estFeeReduction,
-		(bestCaseBytes-swapOutputSize)*feeSuggestion, swapVal+fees-1)
+	checkMaxOrder(t, wallet, lots-1, swapVal-tLotSize,
+		fees-feeReduction,                            // max fees
+		totalBytes*feeSuggestion-estFeeReduction,     // worst case
+		(bestCaseBytes-swapOutputSize)*feeSuggestion) // best case
 
 	_, _, err := wallet.FundOrder(ord)
 	if err == nil {
@@ -1194,7 +1192,8 @@ func TestFundEdges(t *testing.T) {
 	p2pkhUnspent.Amount = float64(swapVal+fees) / 1e8
 	node.unspent = []walletjson.ListUnspentResult{p2pkhUnspent}
 
-	checkMax(lots, swapVal, fees, totalBytes*feeSuggestion, bestCaseBytes*feeSuggestion, swapVal+fees)
+	checkMaxOrder(t, wallet, lots, swapVal, fees, totalBytes*feeSuggestion,
+		bestCaseBytes*feeSuggestion)
 
 	_, _, err = wallet.FundOrder(ord)
 	if err != nil {
@@ -1223,7 +1222,8 @@ func TestFundEdges(t *testing.T) {
 	v = swapVal + fees
 	node.unspent[0].Amount = float64(v) / 1e8
 
-	checkMax(lots, swapVal, fees, (totalBytes+splitTxBaggage)*feeSuggestion, (bestCaseBytes+splitTxBaggage)*feeSuggestion, v)
+	checkMaxOrder(t, wallet, lots, swapVal, fees, (totalBytes+splitTxBaggage)*feeSuggestion,
+		(bestCaseBytes+splitTxBaggage)*feeSuggestion)
 
 	coins, _, err = wallet.FundOrder(ord)
 	if err != nil {
@@ -1252,47 +1252,9 @@ func TestFundEdges(t *testing.T) {
 	if err != nil {
 		t.Fatalf("error fixing split tx: %v", err)
 	}
-
-	// TODO: test version mismatch
-
 	wallet.config().useSplitTx = false
 
-	// TODO: fix the p2sh test so that the redeem script is a p2pk pkScript or a
-	// multisig pkScript, not a p2pkh pkScript.
-
-	// P2SH pkScript size = 23 bytes
-	// P2PK pkScript (the redeem script) = 35 bytes
-	// P2SH redeem input size = overhead(58) + sigScript((1 + 73 + 1) + (1 + 33 + 1)) +
-	//   p2sh pkScript length(23) = 191 bytes vs 166 for P2PKH
-	// backing_fees: 191 bytes * fee_rate(24) = 4584 atoms
-	// total bytes: 2344 + 191 = 2535
-	// total: 56256 + 4584 = 60840 atoms
-	//     OR (2344 + 191) * 24 = 60840
-	// p2shRedeem, _ := hex.DecodeString("76a914" + "db1755408acd315baa75c18ebbe0e8eaddf64a97" + "88ac") // (p2pkh! pkScript) 1+1+1+20+1+1 =25 bytes
-	// scriptAddr := "DcsJEKNF3dQwcSozroei5FRPsbPEmMuWRaj"
-	// p2shScriptPubKey, _ := hex.DecodeString("a914" + "3ff6a24a50135f69be9ffed744443da08408fc1a" + "87") // 1 + 1 + 20 + 1 = 23 bytes
-	// fees = 2535 * tDCR.MaxFeeRate
-	// halfSwap := swapVal / 2
-	// p2shUnspent := walletjson.ListUnspentResult{
-	// 	TxID:          tTxID,
-	// 	Address:       scriptAddr,
-	// 	Amount:        float64(halfSwap) / 1e8,
-	// 	Confirmations: 10,
-	// 	ScriptPubKey:  hex.EncodeToString(p2shScriptPubKey),
-	// 	RedeemScript:  hex.EncodeToString(p2shRedeem),
-	// }
-	// p2pkhUnspent.Amount = float64(halfSwap+fees-1) / 1e8
-	// node.unspent = []walletjson.ListUnspentResult{p2pkhUnspent, p2shUnspent}
-	// _, err = wallet.FundOrder(swapVal, false, tDCR)
-	// if err == nil {
-	// 	t.Fatalf("no error when not enough funds in two utxos")
-	// }
-	// p2pkhUnspent.Amount = float64(halfSwap+fees) / 1e8
-	// node.unspent = []walletjson.ListUnspentResult{p2pkhUnspent, p2shUnspent}
-	// _, err = wallet.FundOrder(swapVal, false, tDCR)
-	// if err != nil {
-	// 	t.Fatalf("error when should be enough funding in two utxos: %v", err)
-	// }
+	// TODO: test version mismatch
 }
 
 func TestSwap(t *testing.T) {
@@ -2474,7 +2436,7 @@ func TestPreSwap(t *testing.T) {
 	maxFees := totalBytes * tDCR.MaxFeeRate
 	estHighFees := totalBytes * feeSuggestion
 	estLowFees := bestCaseBytes * feeSuggestion
-	checkSwapEstimate(t, preSwap.Estimate, lots, swapVal, maxFees, estHighFees, estLowFees, minReq)
+	checkSwapEstimate(t, preSwap.Estimate, lots, swapVal, maxFees, estHighFees, estLowFees)
 
 	// Too little funding is an error.
 	node.unspent[0].Amount = float64(minReq-1) / 1e8