Review fixes. Still left fee estimation

internal/rpchelp/helpdescs_en_US.go

@@ -55,7 +55,7 @@ var helpDescsEnUS = map[string]string{
 	"fundrawtransaction-options":     "The optional inputs to fund a raw transaction",
 
 	// FundRawTransactionOptions
-	"fundrawtransactionoptions-changeaddress": "Address the change will be send to (default=\"default\")",
+	"fundrawtransactionoptions-changeaddress": "The decred address to receive the change",
 	"fundrawtransactionoptions-feerate":       "Set a specific fee rate in DCR/kB",
 	"fundrawtransactionoptions-conf_target":   "Number of blocks to confirm the transaction",
 

rpc/legacyrpc/methods.go

@@ -528,7 +528,7 @@ func fundRawTransaction(s *Server, icmd interface{}) (interface{}, error) {
 	var changeAddr dcrutil.Address
 
 	if cmd.Options != nil {
-		// use provided fee per Kb if specified
+		// use provided fee per kb if specified
 		if cmd.Options.FeeRate != nil {
 			var err error
 			feePerKb, err = dcrutil.NewAmount(*cmd.Options.FeeRate)
@@ -540,8 +540,7 @@ func fundRawTransaction(s *Server, icmd interface{}) (interface{}, error) {
 		if cmd.Options.ConfTarget != nil {
 			requiredConfs = *cmd.Options.ConfTarget
 		}
-		// use provided change account if specified
-		// use default account otherwise
+		// use provided change address if specified
 		if cmd.Options.ChangeAddress != nil {
 			addr, err := decodeAddress(*cmd.Options.ChangeAddress, w.ChainParams())
 			if err != nil {

wallet/addresses.go

@@ -572,15 +572,11 @@ func (src *p2PKHChangeSource) ScriptSize() int {
 }
 
 type p2PKHAddrChangeSource struct {
-	persist       persistReturnedChildFunc
 	changeAddress dcrutil.Address
-	wallet        *Wallet
 }
 
 func (src *p2PKHAddrChangeSource) Script() ([]byte, uint16, error) {
-	var script []byte
-	var err error
-	script, err = txscript.PayToAddrScript(src.changeAddress)
+	script, err := txscript.PayToAddrScript(src.changeAddress)
 	if err != nil {
 		return nil, 0, err
 	}

wallet/createtx.go

@@ -160,6 +160,12 @@ func (w *Wallet) NewUnsignedTransaction(outputs []*wire.TxOut, relayFeePerKb dcr
 	return authoredTx, nil
 }
 
+// FundRawTransaction funds an unsigned raw transaction using unspent
+// account outputs.
+//
+// The changeAddress parameter is optional and can be nil.  When nil, and if a
+// change output should be added, an internal change address is created for the
+// account.
 func (w *Wallet) FundRawTransaction(tx wire.MsgTx, relayFeePerKb dcrutil.Amount, account uint32, minConf int32,
 	changeAddress dcrutil.Address) (*wire.MsgTx, error) {
 
@@ -176,23 +182,22 @@ func (w *Wallet) FundRawTransaction(tx wire.MsgTx, relayFeePerKb dcrutil.Amount,
 		sourceImpl := w.TxStore.MakeInputSource(txmgrNs, addrmgrNs, account,
 			minConf, tipHeight)
 
+		var changeSource interface {
+			txauthor.ChangeSource
+		}
 		if changeAddress == nil {
-			changeSource := p2PKHChangeSource{
+			changeSource = &p2PKHChangeSource{
 				persist: w.deferPersistReturnedChild(&changeSourceUpdates),
 				account: account,
 				wallet:  w,
 			}
-			fundedTx, err = txauthor.FundRawTransaction(tx, relayFeePerKb,
-				sourceImpl.SelectInputs, &changeSource)
 		} else {
-			changeSource := p2PKHAddrChangeSource{
-				persist:       w.deferPersistReturnedChild(&changeSourceUpdates),
+			changeSource = &p2PKHAddrChangeSource{
 				changeAddress: changeAddress,
-				wallet:        w,
 			}
-			fundedTx, err = txauthor.FundRawTransaction(tx, relayFeePerKb,
-				sourceImpl.SelectInputs, &changeSource)
 		}
+		fundedTx, err = txauthor.FundRawTransaction(&tx, relayFeePerKb,
+			sourceImpl.SelectInputs, changeSource)
 
 		return err
 	})

wallet/txauthor/author.go

@@ -63,7 +63,7 @@ type ChangeSource interface {
 }
 
 // NewUnsignedTransaction creates an unsigned transaction paying to one or more
-// non-change outputs.  An appropriate transaction fee is included based on the
+// non-change outputs. An appropriate transaction fee is included based on the
 // transaction size.
 //
 // Transaction inputs are chosen from repeated calls to fetchInputs with
@@ -77,7 +77,7 @@ type ChangeSource interface {
 // will be incorrect.
 //
 // If successful, the transaction, total input value spent, and all previous
-// output scripts are returned.  If the input source was unable to provide
+// output scripts are returned. If the input source was unable to provide
 // enough input value to pay for every output any any necessary fees, an
 // InputSourceError is returned.
 func NewUnsignedTransaction(outputs []*wire.TxOut, relayFeePerKb dcrutil.Amount,
@@ -154,17 +154,34 @@ func NewUnsignedTransaction(outputs []*wire.TxOut, relayFeePerKb dcrutil.Amount,
 	}
 }
 
-func FundRawTransaction(mtx wire.MsgTx, relayFeePerKb dcrutil.Amount,
+// FundRawTransaction funds an unsigned raw transaction paying to one or more
+// non-change outputs. An appropriate transaction fee is included based on the
+// transaction size.
+//
+// If any remaining output value can be returned to the wallet via a change
+// output without violating mempool dust rules, a P2PKH change output is
+// appended to the transaction outputs.  Since the change output may not be
+// necessary, fetchChange is called zero or one times to generate this script.
+// This function must return a P2PKH script or smaller, otherwise fee estimation
+// will be incorrect.
+//
+// If successful, the transaction, total input value spent, and all previous
+// output scripts are returned. If the input source was unable to provide
+// enough input value to pay for every output any necessary fees, an
+// error is returned.
+func FundRawTransaction(mtx *wire.MsgTx, relayFeePerKb dcrutil.Amount,
 	fetchInputs InputSource, fetchChange ChangeSource) (*wire.MsgTx, error) {
 
 	const op errors.Op = "txauthor.FundRawTransaction"
 
+	var targetAmount dcrutil.Amount
 	inputs := mtx.TxIn
 	outputs := mtx.TxOut
-	totalInTransaction := h.SumInputValues(inputs)
-	targetAmount := h.SumOutputValues(outputs) - totalInTransaction
-	if targetAmount <= 0 {
-		return &mtx, nil
+	if len(inputs) > 0 {
+		targetAmount = h.SumOutputValues(outputs) - h.SumInputValues(inputs)
+		if targetAmount <= 0 {
+			return mtx, nil
+		}
 	}
 	scriptSizes := []int{txsizes.RedeemP2PKHSigScriptSize}
 	for range mtx.TxIn {
@@ -189,17 +206,18 @@ func FundRawTransaction(mtx wire.MsgTx, relayFeePerKb dcrutil.Amount,
 			return nil, errors.E(op, errors.InsufficientBalance)
 		}
 
-		scriptSizes = append(scriptSizes, inputDetail.RedeemScriptSizes...)
+		txInputs := append(mtx.TxIn, inputDetail.Inputs...)
+		mtx.TxIn = txInputs
 
+		scriptSizes = append(scriptSizes, inputDetail.RedeemScriptSizes...)
 		maxSignedSize = txsizes.EstimateSerializeSize(scriptSizes, outputs, changeScriptSize)
 		maxRequiredFee := txrules.FeeForSerializeSize(relayFeePerKb, maxSignedSize)
+
 		remainingAmount := inputDetail.Amount - targetAmount
 		if remainingAmount < maxRequiredFee {
 			targetFee = maxRequiredFee
 			continue
 		}
-		txInputs := append(mtx.TxIn, inputDetail.Inputs...)
-		mtx.TxIn = txInputs
 
 		changeAmount := inputDetail.Amount - targetAmount - maxRequiredFee
 		if changeAmount != 0 && !txrules.IsDustAmount(changeAmount,
@@ -215,11 +233,9 @@ func FundRawTransaction(mtx wire.MsgTx, relayFeePerKb dcrutil.Amount,
 			}
 			l := len(outputs)
 			mtx.TxOut = append(outputs[:l:l], change)
-		} else {
-			maxSignedSize = txsizes.EstimateSerializeSize(scriptSizes,
-				mtx.TxOut, 0)
 		}
-		return &mtx, nil
+
+		return mtx, nil
 	}
 }