Merge pull request #740 from bottlepay/random-coins

wallet: add random coin selection

.travis.yml

@@ -9,5 +9,8 @@ script:
   - export GO111MODULE=on
   - make fmt
   - make lint
-  - make unit-race
+
+  # Extend unit test timeout with travis_wait
+  - travis_wait make unit-race
+
   - make unit-cover

rpc/legacyrpc/methods.go

@@ -1360,7 +1360,10 @@ func sendPairs(w *wallet.Wallet, amounts map[string]btcutil.Amount,
 	if err != nil {
 		return "", err
 	}
-	tx, err := w.SendOutputs(outputs, &keyScope, account, minconf, feeSatPerKb, "")
+	tx, err := w.SendOutputs(
+		outputs, &keyScope, account, minconf, feeSatPerKb,
+		wallet.CoinSelectionLargest, "",
+	)
 	if err != nil {
 		if err == txrules.ErrAmountNegative {
 			return "", ErrNeedPositiveAmount

wallet/createtx.go

@@ -7,6 +7,7 @@ package wallet
 
 import (
 	"fmt"
+	"math/rand"
 	"sort"
 
 	"github.com/btcsuite/btcd/btcec"
@@ -29,10 +30,6 @@ func (s byAmount) Less(i, j int) bool { return s[i].Amount < s[j].Amount }
 func (s byAmount) Swap(i, j int)      { s[i], s[j] = s[j], s[i] }
 
 func makeInputSource(eligible []wtxmgr.Credit) txauthor.InputSource {
-	// Pick largest outputs first.  This is only done for compatibility with
-	// previous tx creation code, not because it's a good idea.
-	sort.Sort(sort.Reverse(byAmount(eligible)))
-
 	// Current inputs and their total value.  These are closed over by the
 	// returned input source and reused across multiple calls.
 	currentTotal := btcutil.Amount(0)
@@ -110,7 +107,8 @@ func (s secretSource) GetScript(addr btcutil.Address) ([]byte, error) {
 // the database. A tx created with this set to true will intentionally have no
 // input scripts added and SHOULD NOT be broadcasted.
 func (w *Wallet) txToOutputs(outputs []*wire.TxOut, keyScope *waddrmgr.KeyScope,
-	account uint32, minconf int32, feeSatPerKb btcutil.Amount, dryRun bool) (
+	account uint32, minconf int32, feeSatPerKb btcutil.Amount,
+	coinSelectionStrategy CoinSelectionStrategy, dryRun bool) (
 	tx *txauthor.AuthoredTx, err error) {
 
 	chainClient, err := w.requireChainClient()
@@ -144,7 +142,38 @@ func (w *Wallet) txToOutputs(outputs []*wire.TxOut, keyScope *waddrmgr.KeyScope,
 		return nil, err
 	}
 
-	inputSource := makeInputSource(eligible)
+	var inputSource txauthor.InputSource
+
+	switch coinSelectionStrategy {
+	// Pick largest outputs first.
+	case CoinSelectionLargest:
+		sort.Sort(sort.Reverse(byAmount(eligible)))
+		inputSource = makeInputSource(eligible)
+
+	// Select coins at random. This prevents the creation of ever smaller
+	// utxos over time that may never become economical to spend.
+	case CoinSelectionRandom:
+		// Skip inputs that do not raise the total transaction output
+		// value at the requested fee rate.
+		var positivelyYielding []wtxmgr.Credit
+		for _, output := range eligible {
+			output := output
+
+			if !inputYieldsPositively(&output, feeSatPerKb) {
+				continue
+			}
+
+			positivelyYielding = append(positivelyYielding, output)
+		}
+
+		rand.Shuffle(len(positivelyYielding), func(i, j int) {
+			positivelyYielding[i], positivelyYielding[j] =
+				positivelyYielding[j], positivelyYielding[i]
+		})
+
+		inputSource = makeInputSource(positivelyYielding)
+	}
+
 	tx, err = txauthor.NewUnsignedTransaction(
 		outputs, feeSatPerKb, inputSource, changeSource,
 	)
@@ -291,6 +320,18 @@ func (w *Wallet) findEligibleOutputs(dbtx walletdb.ReadTx,
 	return eligible, nil
 }
 
+// inputYieldsPositively returns a boolean indicating whether this input yields
+// positively if added to a transaction. This determination is based on the
+// best-case added virtual size. For edge cases this function can return true
+// while the input is yielding slightly negative as part of the final
+// transaction.
+func inputYieldsPositively(credit *wtxmgr.Credit, feeRatePerKb btcutil.Amount) bool {
+	inputSize := txsizes.GetMinInputVirtualSize(credit.PkScript)
+	inputFee := feeRatePerKb * btcutil.Amount(inputSize) / 1000
+
+	return inputFee < credit.Amount
+}
+
 // addrMgrWithChangeSource returns the address manager bucket and a change
 // source that returns change addresses from said address manager. The change
 // addresses will come from the specified key scope and account, unless a key

wallet/createtx_test.go

@@ -9,13 +9,17 @@ import (
 	"testing"
 	"time"
 
+	"github.com/btcsuite/btcd/chaincfg"
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
 	"github.com/btcsuite/btcd/txscript"
 	"github.com/btcsuite/btcd/wire"
+	"github.com/btcsuite/btcutil"
 	"github.com/btcsuite/btcwallet/waddrmgr"
+	"github.com/btcsuite/btcwallet/wallet/txauthor"
 	"github.com/btcsuite/btcwallet/walletdb"
 	_ "github.com/btcsuite/btcwallet/walletdb/bdb"
 	"github.com/btcsuite/btcwallet/wtxmgr"
+	"github.com/stretchr/testify/require"
 )
 
 var (
@@ -71,7 +75,9 @@ func TestTxToOutputsDryRun(t *testing.T) {
 
 	// First do a few dry-runs, making sure the number of addresses in the
 	// database us not inflated.
-	dryRunTx, err := w.txToOutputs(txOuts, nil, 0, 1, 1000, true)
+	dryRunTx, err := w.txToOutputs(
+		txOuts, nil, 0, 1, 1000, CoinSelectionLargest, true,
+	)
 	if err != nil {
 		t.Fatalf("unable to author tx: %v", err)
 	}
@@ -86,7 +92,9 @@ func TestTxToOutputsDryRun(t *testing.T) {
 		t.Fatalf("expected 1 address, found %v", len(addresses))
 	}
 
-	dryRunTx2, err := w.txToOutputs(txOuts, nil, 0, 1, 1000, true)
+	dryRunTx2, err := w.txToOutputs(
+		txOuts, nil, 0, 1, 1000, CoinSelectionLargest, true,
+	)
 	if err != nil {
 		t.Fatalf("unable to author tx: %v", err)
 	}
@@ -119,7 +127,9 @@ func TestTxToOutputsDryRun(t *testing.T) {
 
 	// Now we do a proper, non-dry run. This should add a change address
 	// to the database.
-	tx, err := w.txToOutputs(txOuts, nil, 0, 1, 1000, false)
+	tx, err := w.txToOutputs(
+		txOuts, nil, 0, 1, 1000, CoinSelectionLargest, false,
+	)
 	if err != nil {
 		t.Fatalf("unable to author tx: %v", err)
 	}
@@ -181,12 +191,111 @@ func addUtxo(t *testing.T, w *Wallet, incomingTx *wire.MsgTx) {
 		if err != nil {
 			return err
 		}
-		err = w.TxStore.AddCredit(ns, rec, block, 0, false)
-		if err != nil {
-			return err
+		// Add all tx outputs as credits.
+		for i := 0; i < len(incomingTx.TxOut); i++ {
+			err = w.TxStore.AddCredit(
+				ns, rec, block, uint32(i), false,
+			)
+			if err != nil {
+				return err
+			}
 		}
 		return nil
 	}); err != nil {
 		t.Fatalf("failed inserting tx: %v", err)
 	}
 }
+
+// TestInputYield verifies the functioning of the inputYieldsPositively.
+func TestInputYield(t *testing.T) {
+	addr, _ := btcutil.DecodeAddress("bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4", &chaincfg.MainNetParams)
+	pkScript, err := txscript.PayToAddrScript(addr)
+	require.NoError(t, err)
+
+	credit := &wtxmgr.Credit{
+		Amount:   1000,
+		PkScript: pkScript,
+	}
+
+	// At 10 sat/b this input is yielding positively.
+	require.True(t, inputYieldsPositively(credit, 10000))
+
+	// At 20 sat/b this input is yielding negatively.
+	require.False(t, inputYieldsPositively(credit, 20000))
+}
+
+// TestTxToOutputsRandom tests random coin selection.
+func TestTxToOutputsRandom(t *testing.T) {
+	w, cleanup := testWallet(t)
+	defer cleanup()
+
+	// Create an address we can use to send some coins to.
+	keyScope := waddrmgr.KeyScopeBIP0049Plus
+	addr, err := w.CurrentAddress(0, keyScope)
+	if err != nil {
+		t.Fatalf("unable to get current address: %v", addr)
+	}
+	p2shAddr, err := txscript.PayToAddrScript(addr)
+	if err != nil {
+		t.Fatalf("unable to convert wallet address to p2sh: %v", err)
+	}
+
+	// Add a set of utxos to the wallet.
+	incomingTx := &wire.MsgTx{
+		TxIn: []*wire.TxIn{
+			{},
+		},
+		TxOut: []*wire.TxOut{},
+	}
+	for amt := int64(5000); amt <= 125000; amt += 10000 {
+		incomingTx.AddTxOut(wire.NewTxOut(amt, p2shAddr))
+	}
+
+	addUtxo(t, w, incomingTx)
+
+	// Now tell the wallet to create a transaction paying to the specified
+	// outputs.
+	txOuts := []*wire.TxOut{
+		{
+			PkScript: p2shAddr,
+			Value:    50000,
+		},
+		{
+			PkScript: p2shAddr,
+			Value:    100000,
+		},
+	}
+
+	const (
+		feeSatPerKb   = 100000
+		maxIterations = 100
+	)
+
+	createTx := func() *txauthor.AuthoredTx {
+		tx, err := w.txToOutputs(
+			txOuts, nil, 0, 1, feeSatPerKb, CoinSelectionRandom, true,
+		)
+		require.NoError(t, err)
+		return tx
+	}
+
+	firstTx := createTx()
+	var isRandom bool
+	for iteration := 0; iteration < maxIterations; iteration++ {
+		tx := createTx()
+
+		// Check to see if we are getting a total input value.
+		// We consider this proof that the randomization works.
+		if tx.TotalInput != firstTx.TotalInput {
+			isRandom = true
+		}
+
+		// At the used fee rate of 100 sat/b, the 5000 sat input is
+		// negatively yielding. We don't expect it to ever be selected.
+		for _, inputValue := range tx.PrevInputValues {
+			require.NotEqual(t, inputValue, btcutil.Amount(5000))
+		}
+	}
+
+	require.True(t, isRandom)
+}

wallet/psbt.go

@@ -41,7 +41,8 @@ import (
 // selected/validated inputs by this method. It is in the caller's
 // responsibility to lock the inputs before handing the partial transaction out.
 func (w *Wallet) FundPsbt(packet *psbt.Packet, keyScope *waddrmgr.KeyScope,
-	account uint32, feeSatPerKB btcutil.Amount) (int32, error) {
+	account uint32, feeSatPerKB btcutil.Amount,
+	coinSelectionStrategy CoinSelectionStrategy) (int32, error) {
 
 	// Make sure the packet is well formed. We only require there to be at
 	// least one output but not necessarily any inputs.
@@ -133,7 +134,7 @@ func (w *Wallet) FundPsbt(packet *psbt.Packet, keyScope *waddrmgr.KeyScope,
 		// change address creation.
 		tx, err = w.CreateSimpleTx(
 			keyScope, account, packet.UnsignedTx.TxOut, 1,
-			feeSatPerKB, false,
+			feeSatPerKB, coinSelectionStrategy, false,
 		)
 		if err != nil {
 			return 0, fmt.Errorf("error creating funding TX: %v",

wallet/psbt_test.go

@@ -220,6 +220,7 @@ func TestFundPsbt(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			changeIndex, err := w.FundPsbt(
 				tc.packet, nil, 0, tc.feeRateSatPerKB,
+				CoinSelectionLargest,
 			)
 
 			// In any case, unlock the UTXO before continuing, we

wallet/rand.go

@@ -0,0 +1,11 @@
+package wallet
+
+import (
+	"math/rand"
+	"time"
+)
+
+// init initializes the random generator.
+func init() {
+	rand.Seed(time.Now().Unix())
+}

wallet/txsizes/size.go

@@ -6,6 +6,7 @@ package txsizes
 
 import (
 	"github.com/btcsuite/btcd/blockchain"
+	"github.com/btcsuite/btcd/txscript"
 	"github.com/btcsuite/btcd/wire"
 )
 
@@ -200,3 +201,27 @@ func EstimateVirtualSize(numP2PKHIns, numP2WPKHIns, numNestedP2WPKHIns int,
 	// always rounded up.
 	return baseSize + (witnessWeight+3)/blockchain.WitnessScaleFactor
 }
+
+// GetMinInputVirtualSize returns the minimum number of vbytes that this input
+// adds to a transaction.
+func GetMinInputVirtualSize(pkScript []byte) int {
+	var baseSize, witnessWeight int
+	switch {
+	// If this is a p2sh output, we assume this is a
+	// nested P2WKH.
+	case txscript.IsPayToScriptHash(pkScript):
+		baseSize = RedeemNestedP2WPKHInputSize
+		witnessWeight = RedeemP2WPKHInputWitnessWeight
+
+	case txscript.IsPayToWitnessPubKeyHash(pkScript):
+		baseSize = RedeemP2WPKHInputSize
+		witnessWeight = RedeemP2WPKHInputWitnessWeight
+
+	default:
+		baseSize = RedeemP2PKHInputSize
+	}
+
+	return baseSize +
+		(witnessWeight+blockchain.WitnessScaleFactor-1)/
+			blockchain.WitnessScaleFactor
+}

wallet/wallet.go

@@ -77,6 +77,19 @@ var (
 	wtxmgrNamespaceKey   = []byte("wtxmgr")
 )
 
+type CoinSelectionStrategy int
+
+const (
+	// CoinSelectionLargest always picks the largest available utxo to add
+	// to the transaction next.
+	CoinSelectionLargest CoinSelectionStrategy = iota
+
+	// CoinSelectionRandom randomly selects the next utxo to add to the
+	// transaction. This strategy prevents the creation of ever smaller
+	// utxos over time.
+	CoinSelectionRandom
+)
+
 // Wallet is a structure containing all the components for a
 // complete wallet.  It contains the Armory-style key store
 // addresses and keys),
@@ -1125,13 +1138,14 @@ func logFilterBlocksResp(block wtxmgr.BlockMeta,
 
 type (
 	createTxRequest struct {
-		keyScope    *waddrmgr.KeyScope
-		account     uint32
-		outputs     []*wire.TxOut
-		minconf     int32
-		feeSatPerKB btcutil.Amount
-		dryRun      bool
-		resp        chan createTxResponse
+		keyScope              *waddrmgr.KeyScope
+		account               uint32
+		outputs               []*wire.TxOut
+		minconf               int32
+		feeSatPerKB           btcutil.Amount
+		coinSelectionStrategy CoinSelectionStrategy
+		dryRun                bool
+		resp                  chan createTxResponse
 	}
 	createTxResponse struct {
 		tx  *txauthor.AuthoredTx
@@ -1162,7 +1176,8 @@ out:
 			}
 			tx, err := w.txToOutputs(
 				txr.outputs, txr.keyScope, txr.account,
-				txr.minconf, txr.feeSatPerKB, txr.dryRun,
+				txr.minconf, txr.feeSatPerKB,
+				txr.coinSelectionStrategy, txr.dryRun,
 			)
 			heldUnlock.release()
 			txr.resp <- createTxResponse{tx, err}
@@ -1188,16 +1203,18 @@ out:
 // the database. A tx created with this set to true SHOULD NOT be broadcasted.
 func (w *Wallet) CreateSimpleTx(keyScope *waddrmgr.KeyScope, account uint32,
 	outputs []*wire.TxOut, minconf int32, satPerKb btcutil.Amount,
-	dryRun bool) (*txauthor.AuthoredTx, error) {
+	coinSelectionStrategy CoinSelectionStrategy, dryRun bool) (
+	*txauthor.AuthoredTx, error) {
 
 	req := createTxRequest{
-		keyScope:    keyScope,
-		account:     account,
-		outputs:     outputs,
-		minconf:     minconf,
-		feeSatPerKB: satPerKb,
-		dryRun:      dryRun,
-		resp:        make(chan createTxResponse),
+		keyScope:              keyScope,
+		account:               account,
+		outputs:               outputs,
+		minconf:               minconf,
+		feeSatPerKB:           satPerKb,
+		coinSelectionStrategy: coinSelectionStrategy,
+		dryRun:                dryRun,
+		resp:                  make(chan createTxResponse),
 	}
 	w.createTxRequests <- req
 	resp := <-req.resp
@@ -3156,7 +3173,8 @@ func (w *Wallet) TotalReceivedForAddr(addr btcutil.Address, minConf int32) (btcu
 // returns the transaction upon success.
 func (w *Wallet) SendOutputs(outputs []*wire.TxOut, keyScope *waddrmgr.KeyScope,
 	account uint32, minconf int32, satPerKb btcutil.Amount,
-	label string) (*wire.MsgTx, error) {
+	coinSelectionStrategy CoinSelectionStrategy, label string) (
+	*wire.MsgTx, error) {
 
 	// Ensure the outputs to be created adhere to the network's consensus
 	// rules.
@@ -3174,7 +3192,8 @@ func (w *Wallet) SendOutputs(outputs []*wire.TxOut, keyScope *waddrmgr.KeyScope,
 	// continue to re-broadcast the transaction upon restarts until it has
 	// been confirmed.
 	createdTx, err := w.CreateSimpleTx(
-		keyScope, account, outputs, minconf, satPerKb, false,
+		keyScope, account, outputs, minconf, satPerKb,
+		coinSelectionStrategy, false,
 	)
 	if err != nil {
 		return nil, err