diff --git a/wallet/txauthor/author.go b/wallet/txauthor/author.go index 4daad618e7..b4f55a2b2d 100644 --- a/wallet/txauthor/author.go +++ b/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,12 +189,12 @@ 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 } @@ -203,7 +202,7 @@ func NewUnsignedTransaction(outputs []*wire.TxOut, feeRatePerKb btcutil.Amount, Tx: unsignedTransaction, PrevScripts: scripts, PrevInputValues: inputValues, - TotalInput: inputSelection.inputState.inputTotal, + TotalInput: inputState.inputTotal, ChangeIndex: changeIndex, }, nil diff --git a/wallet/txauthor/author_test.go b/wallet/txauthor/author_test.go index 0f8f921812..b4ad561b3d 100644 --- a/wallet/txauthor/author_test.go +++ b/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[:] } diff --git a/wallet/txauthor/txselection.go b/wallet/txauthor/txselection.go index 8e3ea85131..090bf826b4 100644 --- a/wallet/txauthor/txselection.go +++ b/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 }