From 900574b7d50638cf027e42d1fb98d1924285dc1e Mon Sep 17 00:00:00 2001 From: Jonathan Chappelow Date: Wed, 7 Dec 2022 15:59:30 -0600 Subject: [PATCH] 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. --- client/asset/btc/btc.go | 26 ++++++++------- client/asset/btc/btc_test.go | 42 +++++++++++------------ client/asset/dcr/dcr.go | 21 +++++++----- client/asset/dcr/dcr_test.go | 64 ++++++++---------------------------- 4 files changed, 60 insertions(+), 93 deletions(-) diff --git a/client/asset/btc/btc.go b/client/asset/btc/btc.go index a2ce396eec..2b3ab2d0d3 100644 --- a/client/asset/btc/btc.go +++ b/client/asset/btc/btc.go @@ -1702,14 +1702,18 @@ func (btc *baseWallet) estimateSwap(lots, lotSize, feeSuggestion uint64, utxos [ bumpedMaxRate := nfo.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, nfo.SwapSizeBase, nfo.SwapSize, bumpedMaxRate) + reqFunds := calc.RequiredOrderFundsAlt(val, inputsSize, lots, nfo.SwapSizeBase, nfo.SwapSize, bumpedMaxRate) // no +splitMaxFees so this is accurate without split return inputsVal >= reqFunds } @@ -1718,7 +1722,7 @@ func (btc *baseWallet) estimateSwap(lots, lotSize, feeSuggestion uint64, utxos [ return nil, false, 0, fmt.Errorf("error funding swap value %s: %w", amount(val), err) } - reqFunds := calc.RequiredOrderFundsAlt(val, uint64(inputsSize), lots, nfo.SwapSizeBase, nfo.SwapSize, bumpedMaxRate) + reqFunds := calc.RequiredOrderFundsAlt(val, uint64(inputsSize), lots, nfo.SwapSizeBase, nfo.SwapSize, bumpedMaxRate) // same as in enough func maxFees := reqFunds - val estHighFunds := calc.RequiredOrderFundsAlt(val, uint64(inputsSize), lots, nfo.SwapSizeBase, nfo.SwapSize, bumpedNetRate) @@ -1730,23 +1734,21 @@ func (btc *baseWallet) estimateSwap(lots, lotSize, feeSuggestion uint64, utxos [ } 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 index aeceeb305f..86b9105f29 100644 --- a/client/asset/btc/btc_test.go +++ b/client/asset/btc/btc_test.go @@ -1292,7 +1292,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, @@ -1302,10 +1302,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) @@ -1330,11 +1330,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) @@ -1386,8 +1381,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 { @@ -1397,7 +1393,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 { @@ -1432,7 +1429,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 { @@ -1554,11 +1553,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) @@ -1609,8 +1603,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 { @@ -1620,7 +1615,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 { @@ -1652,7 +1648,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 { @@ -2775,7 +2773,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 index aa6c0807e9..9c4cde82ae 100644 --- a/client/asset/dcr/dcr.go +++ b/client/asset/dcr/dcr.go @@ -1164,17 +1164,22 @@ func (dcr *ExchangeWallet) estimateSwap(lots, lotSize, feeSuggestion uint64, utx bumpedMaxRate := nfo.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 := dcr.tryFund(utxos, orderEnough(val, lots, bumpedMaxRate, nfo)) if err != nil { return nil, false, 0, err } - reqFunds := calc.RequiredOrderFundsAlt(val, uint64(inputsSize), lots, nfo.SwapSizeBase, nfo.SwapSize, bumpedMaxRate) + reqFunds := calc.RequiredOrderFundsAlt(val, uint64(inputsSize), lots, nfo.SwapSizeBase, nfo.SwapSize, bumpedMaxRate) // as in tryFund's enough func maxFees := reqFunds - val estHighFunds := calc.RequiredOrderFundsAlt(val, uint64(inputsSize), lots, nfo.SwapSizeBase, nfo.SwapSize, bumpedNetRate) @@ -1186,17 +1191,17 @@ func (dcr *ExchangeWallet) estimateSwap(lots, lotSize, feeSuggestion uint64, utx // 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 index 0a9aaef268..ff7c309cd5 100644 --- a/client/asset/dcr/dcr_test.go +++ b/client/asset/dcr/dcr_test.go @@ -1109,16 +1109,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) 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) @@ -1146,10 +1146,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) @@ -1192,8 +1188,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 { @@ -1203,7 +1201,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 { @@ -1232,7 +1231,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 { @@ -1261,47 +1261,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) { @@ -2508,7 +2470,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