wallet: add dryrun arg to tx create, rolling back db if set

wallet/createtx.go

@@ -101,60 +101,77 @@ func (s secretSource) GetScript(addr btcutil.Address) ([]byte, error) {
 // UTXO set and minconf policy. An additional output may be added to return
 // change to the wallet.  An appropriate fee is included based on the wallet's
 // current relay fee.  The wallet must be unlocked to create the transaction.
+//
+// NOTE: The dryRun argument can be set true to create a tx that doesn't alter
+// 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, account uint32,
-	minconf int32, feeSatPerKb btcutil.Amount) (tx *txauthor.AuthoredTx, err error) {
+	minconf int32, feeSatPerKb btcutil.Amount, dryRun bool) (
+	tx *txauthor.AuthoredTx, err error) {
 
 	chainClient, err := w.requireChainClient()
 	if err != nil {
 		return nil, err
 	}
 
-	err = walletdb.Update(w.db, func(dbtx walletdb.ReadWriteTx) error {
-		addrmgrNs := dbtx.ReadWriteBucket(waddrmgrNamespaceKey)
+	dbtx, err := w.db.BeginReadWriteTx()
+	if err != nil {
+		return nil, err
+	}
+	defer dbtx.Rollback()
 
-		// Get current block's height and hash.
-		bs, err := chainClient.BlockStamp()
+	addrmgrNs := dbtx.ReadWriteBucket(waddrmgrNamespaceKey)
+
+	// Get current block's height and hash.
+	bs, err := chainClient.BlockStamp()
+	if err != nil {
+		return nil, err
+	}
+
+	eligible, err := w.findEligibleOutputs(dbtx, account, minconf, bs)
+	if err != nil {
+		return nil, err
+	}
+
+	inputSource := makeInputSource(eligible)
+	changeSource := func() ([]byte, error) {
+		// Derive the change output script.  As a hack to allow
+		// spending from the imported account, change addresses are
+		// created from account 0.
+		var changeAddr btcutil.Address
+		var err error
+		if account == waddrmgr.ImportedAddrAccount {
+			changeAddr, err = w.newChangeAddress(addrmgrNs, 0)
+		} else {
+			changeAddr, err = w.newChangeAddress(addrmgrNs, account)
+		}
 		if err != nil {
-			return err
+			return nil, err
 		}
+		return txscript.PayToAddrScript(changeAddr)
+	}
+	tx, err = txauthor.NewUnsignedTransaction(outputs, feeSatPerKb,
+		inputSource, changeSource)
+	if err != nil {
+		return nil, err
+	}
 
-		eligible, err := w.findEligibleOutputs(dbtx, account, minconf, bs)
-		if err != nil {
-			return err
-		}
+	// Randomize change position, if change exists, before signing.  This
+	// doesn't affect the serialize size, so the change amount will still
+	// be valid.
+	if tx.ChangeIndex >= 0 {
+		tx.RandomizeChangePosition()
+	}
 
-		inputSource := makeInputSource(eligible)
-		changeSource := func() ([]byte, error) {
-			// Derive the change output script.  As a hack to allow
-			// spending from the imported account, change addresses
-			// are created from account 0.
-			var changeAddr btcutil.Address
-			var err error
-			if account == waddrmgr.ImportedAddrAccount {
-				changeAddr, err = w.newChangeAddress(addrmgrNs, 0)
-			} else {
-				changeAddr, err = w.newChangeAddress(addrmgrNs, account)
-			}
-			if err != nil {
-				return nil, err
-			}
-			return txscript.PayToAddrScript(changeAddr)
-		}
-		tx, err = txauthor.NewUnsignedTransaction(outputs, feeSatPerKb,
-			inputSource, changeSource)
-		if err != nil {
-			return err
-		}
+	// If a dry run was requested, we return now before adding the input
+	// scripts, and don't commit the database transaction. The DB will be
+	// rolled back when this method returns to ensure the dry run didn't
+	// alter the DB in any way.
+	if dryRun {
+		return tx, nil
+	}
 
-		// Randomize change position, if change exists, before signing.
-		// This doesn't affect the serialize size, so the change amount
-		// will still be valid.
-		if tx.ChangeIndex >= 0 {
-			tx.RandomizeChangePosition()
-		}
-
-		return tx.AddAllInputScripts(secretSource{w.Manager, addrmgrNs})
-	})
+	err = tx.AddAllInputScripts(secretSource{w.Manager, addrmgrNs})
 	if err != nil {
 		return nil, err
 	}
@@ -164,6 +181,10 @@ func (w *Wallet) txToOutputs(outputs []*wire.TxOut, account uint32,
 		return nil, err
 	}
 
+	if err := dbtx.Commit(); err != nil {
+		return nil, err
+	}
+
 	if tx.ChangeIndex >= 0 && account == waddrmgr.ImportedAddrAccount {
 		changeAmount := btcutil.Amount(tx.Tx.TxOut[tx.ChangeIndex].Value)
 		log.Warnf("Spend from imported account produced change: moving"+

wallet/createtx_test.go

@@ -0,0 +1,204 @@
+// Copyright (c) 2018 The btcsuite developers
+// Use of this source code is governed by an ISC
+// license that can be found in the LICENSE file.
+
+package wallet
+
+import (
+	"bytes"
+	"io/ioutil"
+	"os"
+	"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/hdkeychain"
+	"github.com/btcsuite/btcwallet/waddrmgr"
+	"github.com/btcsuite/btcwallet/walletdb"
+	_ "github.com/btcsuite/btcwallet/walletdb/bdb"
+	"github.com/btcsuite/btcwallet/wtxmgr"
+)
+
+// TestTxToOutput checks that no new address is added to he database if we
+// request a dry run of the txToOutputs call. It also makes sure a subsequent
+// non-dry run call produces a similar transaction to the dry-run.
+func TestTxToOutputsDryRun(t *testing.T) {
+	// Set up a wallet.
+	dir, err := ioutil.TempDir("", "createtx_test")
+	if err != nil {
+		t.Fatalf("Failed to create db dir: %v", err)
+	}
+	defer os.RemoveAll(dir)
+
+	seed, err := hdkeychain.GenerateSeed(hdkeychain.MinSeedBytes)
+	if err != nil {
+		t.Fatalf("unable to create seed: %v", err)
+	}
+
+	pubPass := []byte("hello")
+	privPass := []byte("world")
+
+	loader := NewLoader(&chaincfg.TestNet3Params, dir, 250)
+	w, err := loader.CreateNewWallet(pubPass, privPass, seed, time.Now())
+	if err != nil {
+		t.Fatalf("unable to create wallet: %v", err)
+	}
+	chainClient := &mockChainClient{}
+	w.chainClient = chainClient
+	if err := w.Unlock(privPass, time.After(10*time.Minute)); err != nil {
+		t.Fatalf("unable to unlock wallet: %v", err)
+	}
+
+	// Create an address we can use to send some coins to.
+	addr, err := w.CurrentAddress(0, waddrmgr.KeyScopeBIP0044)
+	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 an output paying to the wallet's address to the database.
+	txOut := wire.NewTxOut(100000, p2shAddr)
+	incomingTx := &wire.MsgTx{
+		TxIn: []*wire.TxIn{
+			{},
+		},
+		TxOut: []*wire.TxOut{
+			txOut,
+		},
+	}
+
+	var b bytes.Buffer
+	if err := incomingTx.Serialize(&b); err != nil {
+		t.Fatalf("unable to serialize tx: %v", err)
+	}
+	txBytes := b.Bytes()
+
+	rec, err := wtxmgr.NewTxRecord(txBytes, time.Now())
+	if err != nil {
+		t.Fatalf("unable to create tx record: %v", err)
+	}
+
+	// The block meta will be inserted to tell the wallet this is a
+	// confirmed transaction.
+	blockHash, _ := chainhash.NewHashFromStr(
+		"00000000000000017188b968a371bab95aa43522665353b646e41865abae02a4")
+	block := &wtxmgr.BlockMeta{
+		Block: wtxmgr.Block{Hash: *blockHash, Height: 276425},
+		Time:  time.Unix(1387737310, 0),
+	}
+
+	if err := walletdb.Update(w.db, func(tx walletdb.ReadWriteTx) error {
+		ns := tx.ReadWriteBucket(wtxmgrNamespaceKey)
+		err = w.TxStore.InsertTx(ns, rec, block)
+		if err != nil {
+			return err
+		}
+		err = w.TxStore.AddCredit(ns, rec, block, 0, false)
+		if err != nil {
+			return err
+		}
+		return nil
+	}); err != nil {
+		t.Fatalf("failed inserting tx: %v", err)
+	}
+
+	// Now tell the wallet to create a transaction paying to the specified
+	// outputs.
+	txOuts := []*wire.TxOut{
+		{
+			PkScript: p2shAddr,
+			Value:    10000,
+		},
+		{
+			PkScript: p2shAddr,
+			Value:    20000,
+		},
+	}
+
+	// First do a few dry-runs, making sure the number of addresses in the
+	// database us not inflated.
+	dryRunTx, err := w.txToOutputs(txOuts, 0, 1, 1000, true)
+	if err != nil {
+		t.Fatalf("unable to author tx: %v", err)
+	}
+	change := dryRunTx.Tx.TxOut[dryRunTx.ChangeIndex]
+
+	addresses, err := w.AccountAddresses(0)
+	if err != nil {
+		t.Fatalf("unable to get addresses: %v", err)
+	}
+
+	if len(addresses) != 1 {
+		t.Fatalf("expected 1 address, found %v", len(addresses))
+	}
+
+	dryRunTx2, err := w.txToOutputs(txOuts, 0, 1, 1000, true)
+	if err != nil {
+		t.Fatalf("unable to author tx: %v", err)
+	}
+	change2 := dryRunTx2.Tx.TxOut[dryRunTx2.ChangeIndex]
+
+	addresses, err = w.AccountAddresses(0)
+	if err != nil {
+		t.Fatalf("unable to get addresses: %v", err)
+	}
+
+	if len(addresses) != 1 {
+		t.Fatalf("expected 1 address, found %v", len(addresses))
+	}
+
+	// The two dry-run TXs should be invalid, since they don't have
+	// signatures.
+	err = validateMsgTx(
+		dryRunTx.Tx, dryRunTx.PrevScripts, dryRunTx.PrevInputValues,
+	)
+	if err == nil {
+		t.Fatalf("Expected tx to be invalid")
+	}
+
+	err = validateMsgTx(
+		dryRunTx2.Tx, dryRunTx2.PrevScripts, dryRunTx2.PrevInputValues,
+	)
+	if err == nil {
+		t.Fatalf("Expected tx to be invalid")
+	}
+
+	// Now we do a proper, non-dry run. This should add a change address
+	// to the database.
+	tx, err := w.txToOutputs(txOuts, 0, 1, 1000, false)
+	if err != nil {
+		t.Fatalf("unable to author tx: %v", err)
+	}
+	change3 := tx.Tx.TxOut[tx.ChangeIndex]
+
+	addresses, err = w.AccountAddresses(0)
+	if err != nil {
+		t.Fatalf("unable to get addresses: %v", err)
+	}
+
+	if len(addresses) != 2 {
+		t.Fatalf("expected 2 addresses, found %v", len(addresses))
+	}
+
+	err = validateMsgTx(tx.Tx, tx.PrevScripts, tx.PrevInputValues)
+	if err != nil {
+		t.Fatalf("Expected tx to be valid: %v", err)
+	}
+
+	// Finally, we check that all the transaction were using the same
+	// change address.
+	if !bytes.Equal(change.PkScript, change2.PkScript) {
+		t.Fatalf("first dry-run using different change address " +
+			"than second")
+	}
+	if !bytes.Equal(change2.PkScript, change3.PkScript) {
+		t.Fatalf("dry-run using different change address " +
+			"than wet run")
+	}
+}

wallet/mock.go

@@ -0,0 +1,81 @@
+package wallet
+
+import (
+	"time"
+
+	"github.com/btcsuite/btcd/chaincfg/chainhash"
+	"github.com/btcsuite/btcd/wire"
+	"github.com/btcsuite/btcutil"
+	"github.com/btcsuite/btcwallet/chain"
+	"github.com/btcsuite/btcwallet/waddrmgr"
+)
+
+type mockChainClient struct {
+}
+
+var _ chain.Interface = (*mockChainClient)(nil)
+
+func (m *mockChainClient) Start() error {
+	return nil
+}
+
+func (m *mockChainClient) Stop() {
+}
+
+func (m *mockChainClient) WaitForShutdown() {}
+
+func (m *mockChainClient) GetBestBlock() (*chainhash.Hash, int32, error) {
+	return nil, 0, nil
+}
+
+func (m *mockChainClient) GetBlock(*chainhash.Hash) (*wire.MsgBlock, error) {
+	return nil, nil
+}
+
+func (m *mockChainClient) GetBlockHash(int64) (*chainhash.Hash, error) {
+	return nil, nil
+}
+
+func (m *mockChainClient) GetBlockHeader(*chainhash.Hash) (*wire.BlockHeader,
+	error) {
+	return nil, nil
+}
+
+func (m *mockChainClient) FilterBlocks(*chain.FilterBlocksRequest) (
+	*chain.FilterBlocksResponse, error) {
+	return nil, nil
+}
+
+func (m *mockChainClient) BlockStamp() (*waddrmgr.BlockStamp, error) {
+	return &waddrmgr.BlockStamp{
+		Height:    500000,
+		Hash:      chainhash.Hash{},
+		Timestamp: time.Unix(1234, 0),
+	}, nil
+}
+
+func (m *mockChainClient) SendRawTransaction(*wire.MsgTx, bool) (
+	*chainhash.Hash, error) {
+	return nil, nil
+}
+
+func (m *mockChainClient) Rescan(*chainhash.Hash, []btcutil.Address,
+	map[wire.OutPoint]btcutil.Address) error {
+	return nil
+}
+
+func (m *mockChainClient) NotifyReceived([]btcutil.Address) error {
+	return nil
+}
+
+func (m *mockChainClient) NotifyBlocks() error {
+	return nil
+}
+
+func (m *mockChainClient) Notifications() <-chan interface{} {
+	return nil
+}
+
+func (m *mockChainClient) BackEnd() string {
+	return "mock"
+}

wallet/wallet.go

@@ -1148,6 +1148,7 @@ type (
 		outputs     []*wire.TxOut
 		minconf     int32
 		feeSatPerKB btcutil.Amount
+		dryRun      bool
 		resp        chan createTxResponse
 	}
 	createTxResponse struct {
@@ -1178,7 +1179,7 @@ out:
 				continue
 			}
 			tx, err := w.txToOutputs(txr.outputs, txr.account,
-				txr.minconf, txr.feeSatPerKB)
+				txr.minconf, txr.feeSatPerKB, txr.dryRun)
 			heldUnlock.release()
 			txr.resp <- createTxResponse{tx, err}
 		case <-quit:
@@ -1189,19 +1190,24 @@ out:
 }
 
 // CreateSimpleTx creates a new signed transaction spending unspent P2PKH
-// outputs with at laest minconf confirmations spending to any number of
+// outputs with at least minconf confirmations spending to any number of
 // address/amount pairs.  Change and an appropriate transaction fee are
 // automatically included, if necessary.  All transaction creation through this
 // function is serialized to prevent the creation of many transactions which
 // spend the same outputs.
+//
+// NOTE: The dryRun argument can be set true to create a tx that doesn't alter
+// the database. A tx created with this set to true SHOULD NOT be broadcasted.
 func (w *Wallet) CreateSimpleTx(account uint32, outputs []*wire.TxOut,
-	minconf int32, satPerKb btcutil.Amount) (*txauthor.AuthoredTx, error) {
+	minconf int32, satPerKb btcutil.Amount, dryRun bool) (
+	*txauthor.AuthoredTx, error) {
 
 	req := createTxRequest{
 		account:     account,
 		outputs:     outputs,
 		minconf:     minconf,
 		feeSatPerKB: satPerKb,
+		dryRun:      dryRun,
 		resp:        make(chan createTxResponse),
 	}
 	w.createTxRequests <- req
@@ -3209,7 +3215,9 @@ func (w *Wallet) SendOutputs(outputs []*wire.TxOut, account uint32,
 	// transaction will be added to the database in order to ensure that we
 	// continue to re-broadcast the transaction upon restarts until it has
 	// been confirmed.
-	createdTx, err := w.CreateSimpleTx(account, outputs, minconf, satPerKb)
+	createdTx, err := w.CreateSimpleTx(
+		account, outputs, minconf, satPerKb, false,
+	)
 	if err != nil {
 		return nil, err
 	}