fixup! txauthor: new input selection logic

wallet/txauthor/author.go

@@ -88,15 +88,19 @@ type ChangeSource struct {
 // the dust limit of the bitcoin network.
 //
 // If successful, the transaction, total input value spent, and all previous
-// output scripts are returned.  If the inputs were unable to provide
+// output scripts are returned. If the inputs were unable to provide
 // enough value to pay for every output and any necessary fees, an
 // InputSourceError is returned.
 //
 // BUGS: Fee estimation is off when redeeming inputs from a uncompressed P2PKH.
-// Because one cannot evaluate whether an uncrompressed or compressed public
-// key was used in the P2PKH before hashing the actual public effectively
-// knowing the detailed signature of such an input. Because umcompressed keys
-// are rare the worst case estimate for a P2PKH is a compressed public key.
+// Because one cannot evaluate whether an uncompressed or compressed public
+// key was used in the P2PKH output before knowing the ScriptSig.
+// The output of an P2PKH is the HASH160 of the public key. This public key
+// is only provided when signing the transaction (public key is part of the
+// ScriptSig) and can be compressed or uncompressed.
+// We use the compressed format to estimate P2PKH inputs because uncompressed
+// public keys are very rare which would lead to overpaying fees most of the
+// time.
 func NewUnsignedTransaction(outputs []*wire.TxOut, feeRatePerKb btcutil.Amount,
 	inputs []wtxmgr.Credit, selectionStrategy InputSelectionStrategy,
 	changeSource *ChangeSource) (*AuthoredTx, error) {
@@ -108,43 +112,38 @@ func NewUnsignedTransaction(outputs []*wire.TxOut, feeRatePerKb btcutil.Amount,
 
 	targetAmount := SumOutputValues(outputs)
 
-	inputSelection := txSelector{
-		inputState: &inputState{
-			feeRatePerKb:      feeRatePerKb,
-			targetAmount:      targetAmount,
-			outputs:           outputs,
-			selectionStrategy: selectionStrategy,
-			changeOutpoint: wire.TxOut{
-				PkScript: changeScript,
-			},
+	inputState := inputState{
+		feeRatePerKb:      feeRatePerKb,
+		targetAmount:      targetAmount,
+		outputs:           outputs,
+		selectionStrategy: selectionStrategy,
+		changeOutpoint: wire.TxOut{
+			PkScript: changeScript,
 		},
 	}
 
 	switch selectionStrategy {
-
 	case PositiveYieldingSelection, RandomSelection:
-
 		// We look through all our inputs and add an input until
 		// the amount is enough to pay the target amount and the
 		// transaction fees.
 		for _, input := range inputs {
-			if !inputSelection.inputState.enoughInput() {
+			if !inputState.enoughInput() {
 				// In case adding a new input fails in the
 				// positive yielding strategy we fail quickly
 				// because all follow up inputs will be negative
 				// yielding too.
-				if !inputSelection.add(input) &&
+				if !inputState.add(input) &&
 					selectionStrategy == PositiveYieldingSelection {
 
 					return nil, txSelectionError{
-						targetAmount: inputSelection.inputState.targetAmount,
-						txFee:        inputSelection.inputState.txFee,
-						availableAmt: inputSelection.inputState.inputTotal,
+						targetAmount: inputState.targetAmount,
+						txFee:        inputState.txFee,
+						availableAmt: inputState.inputTotal,
 					}
 				}
 				continue
 			}
-
 			// We stop considering inputs when the input amount
 			// is enough to fund the transaction.
 			break
@@ -154,26 +153,26 @@ func NewUnsignedTransaction(outputs []*wire.TxOut, feeRatePerKb btcutil.Amount,
 		// In case of a constant selection all inputs are added
 		// although they might be negative yielding so we do not
 		// check for the return value.
-		inputSelection.add(inputs...)
+		inputState.add(inputs...)
 	}
 
 	// This check is needed to make sure our input amount suffice
 	// after considering all eligable inputs.
-	if !inputSelection.inputState.enoughInput() {
+	if !inputState.enoughInput() {
 		return nil, txSelectionError{
-			targetAmount: inputSelection.inputState.targetAmount,
-			txFee:        inputSelection.inputState.txFee,
-			availableAmt: inputSelection.inputState.inputTotal,
+			targetAmount: inputState.targetAmount,
+			txFee:        inputState.txFee,
+			availableAmt: inputState.inputTotal,
 		}
 	}
 
 	// We need the inputs in the right format.
-	numberInputs := len(inputSelection.inputState.inputs)
+	numberInputs := len(inputState.inputs)
 	txIn := make([]*wire.TxIn, 0, numberInputs)
 	inputValues := make([]btcutil.Amount, 0, numberInputs)
 	scripts := make([][]byte, 0, numberInputs)
 
-	for _, input := range inputSelection.inputState.inputs {
+	for _, input := range inputState.inputs {
 		txIn = append(txIn, wire.NewTxIn(&input.OutPoint, nil, nil))
 		inputValues = append(inputValues, input.Amount)
 		scripts = append(scripts, input.PkScript)
@@ -190,20 +189,20 @@ func NewUnsignedTransaction(outputs []*wire.TxOut, feeRatePerKb btcutil.Amount,
 	// input state is above dust, and add the changeoutput to the
 	// transaction.
 	changeIndex := -1
-	if !txrules.IsDustOutput(&inputSelection.inputState.changeOutpoint,
+	if !txrules.IsDustOutput(&inputState.changeOutpoint,
 		txrules.DefaultRelayFeePerKb) {
 
 		l := len(outputs)
 		unsignedTransaction.TxOut = append(outputs[:l:l],
-			&inputSelection.inputState.changeOutpoint)
+			&inputState.changeOutpoint)
 		changeIndex = l
 	}
 
 	return &AuthoredTx{
 		Tx:              unsignedTransaction,
 		PrevScripts:     scripts,
 		PrevInputValues: inputValues,
-		TotalInput:      inputSelection.inputState.inputTotal,
+		TotalInput:      inputState.inputTotal,
 		ChangeIndex:     changeIndex,
 	}, nil
 

wallet/txauthor/author_test.go

@@ -31,7 +31,6 @@ type testOutput struct {
 // createOutput creates outputs of a transaction depending on their
 // output script.
 func createOutput(testOutputs ...testOutput) []*wire.TxOut {
-
 	outputs := make([]*wire.TxOut, 0, len(testOutputs))
 	var outScript []byte
 
@@ -58,7 +57,6 @@ func createOutput(testOutputs ...testOutput) []*wire.TxOut {
 
 // createCredit creates the unspent outputs for the transaction in the right format.
 func createCredit(txIn ...testOutput) []wtxmgr.Credit {
-
 	credits := make([]wtxmgr.Credit, len(txIn))
 
 	var (
@@ -93,6 +91,7 @@ func createCredit(txIn ...testOutput) []wtxmgr.Credit {
 
 		case p2npkh:
 			pkScript = zeroScriptPush[:]
+
 		case p2tr:
 			pkScript = zeroV1KeyPush[:]
 		}

wallet/txauthor/txselection.go

@@ -104,7 +104,6 @@ type inputState struct {
 // virutalSizeEstimate is the (worst case) tx size with the current set of
 // inputs. It takes a parameter whether to add a change output or not.
 func (t *inputState) virutalSizeEstimate(change bool) int {
-
 	// We count the types of inputs, which we'll use to estimate
 	// the vsize of the transaction.
 	var (
@@ -139,8 +138,8 @@ func (t *inputState) virutalSizeEstimate(change bool) int {
 			p2pkh, p2tr, p2wpkh, nested, t.outputs, 0,
 		)
 	}
-	return maxSignedSize
 
+	return maxSignedSize
 }
 
 // enoughInput returns true if we've accumulated enough inputs to pay the fees
@@ -187,7 +186,13 @@ func (t *inputState) clone() inputState {
 		inputs:            make([]wtxmgr.Credit, len(t.inputs)),
 	}
 
-	copy(s.outputs, t.outputs)
+	// we deepcopy outputs otherwise changing the clone would lead to
+	// changing the initial state of the outputs.
+	for idx, out := range t.outputs {
+		cpy := *out
+		s.outputs[idx] = &cpy
+	}
+
 	copy(s.inputs, t.inputs)
 
 	return s
@@ -215,7 +220,6 @@ func (t *inputState) totalOutput() btcutil.Amount {
 // decreases the tx output value after paying fees (depending on the
 // input selection strategy).
 func (t *inputState) addToState(inputs ...wtxmgr.Credit) *inputState {
-
 	// Clone the current set state.
 	tempInputState := t.clone()
 
@@ -253,20 +257,14 @@ func (t *inputState) addToState(inputs ...wtxmgr.Credit) *inputState {
 	return &tempInputState
 }
 
-// txSelector is needed to swap the input states if the
-// addToState function is successful.
-type txSelector struct {
-	inputState *inputState
-}
-
-// add tries to add the input to the current inputState.
-func (t *txSelector) add(input ...wtxmgr.Credit) bool {
-	newState := t.inputState.addToState(input...)
+func (t *inputState) add(inputs ...wtxmgr.Credit) bool {
+	newState := t.addToState(inputs...)
 	if newState == nil {
 		return false
 	}
 
-	t.inputState = newState
-
+	// We copy the contents of the new state to our main inputState.
+	// just copying the pointer would lead to information loss.
+	*t = newState.clone()
 	return true
 }