From 4d2665ee3a6f3b5275d7c47a7f0f144f0ed55129 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Mon, 29 Mar 2021 16:36:41 +0200 Subject: [PATCH 1/2] wallet: add random coin selection --- rpc/legacyrpc/methods.go | 5 +- wallet/createtx.go | 53 +++++++++++++++-- wallet/createtx_test.go | 121 +++++++++++++++++++++++++++++++++++++-- wallet/psbt.go | 5 +- wallet/psbt_test.go | 1 + wallet/rand.go | 11 ++++ wallet/txsizes/size.go | 25 ++++++++ wallet/wallet.go | 55 ++++++++++++------ 8 files changed, 243 insertions(+), 33 deletions(-) create mode 100644 wallet/rand.go diff --git a/rpc/legacyrpc/methods.go b/rpc/legacyrpc/methods.go index 0eff722..0ed1f34 100644 --- a/rpc/legacyrpc/methods.go +++ b/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 diff --git a/wallet/createtx.go b/wallet/createtx.go index 900be64..240f57c 100644 --- a/wallet/createtx.go +++ b/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 diff --git a/wallet/createtx_test.go b/wallet/createtx_test.go index a3e915c..cdbecb3 100644 --- a/wallet/createtx_test.go +++ b/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) +} diff --git a/wallet/psbt.go b/wallet/psbt.go index a0617d8..603e1fe 100644 --- a/wallet/psbt.go +++ b/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", diff --git a/wallet/psbt_test.go b/wallet/psbt_test.go index 5ef2b3a..0df18da 100644 --- a/wallet/psbt_test.go +++ b/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 diff --git a/wallet/rand.go b/wallet/rand.go new file mode 100644 index 0000000..4711fa1 --- /dev/null +++ b/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()) +} diff --git a/wallet/txsizes/size.go b/wallet/txsizes/size.go index 4b47284..1522e37 100644 --- a/wallet/txsizes/size.go +++ b/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 +} diff --git a/wallet/wallet.go b/wallet/wallet.go index 7e85a89..2456faa 100644 --- a/wallet/wallet.go +++ b/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 From 0efc499b8cfc3e9b931541a2718cfaa66b239f33 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Wed, 19 May 2021 09:11:13 +0200 Subject: [PATCH 2/2] build: extend unit-race timeout --- .travis.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index b6aced7..583e3b3 100644 --- a/.travis.yml +++ b/.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