Fix for change and tiny fee calculations (#112)

* with the change to tiny dust limits and 0.05 sat / byte tx fee rates, some of the rounding needed re-visted as off by one bugs were noticed when calculating change. This has been remedied by using floating point as much as possible and using an int conversion as late as possible to ensure accurate figures. THis has resulted in some tests that had questionable results now becoming more accurate also.

* adding nolint to test

* achieving the same results but without floats - rejig of change

* undo changes in tx.go

Co-authored-by: Jad Wahab <15110087+jadwahab@users.noreply.github.com>

.make/go.mk

@@ -42,7 +42,7 @@ install-go: ## Install the application (Using Native Go)
 
 lint: ## Run the golangci-lint application (install if not found)
 	@echo "downloading golangci-lint..."
-	@curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- v1.42.0
+	@curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- v1.45.2
 	@echo "running golangci-lint..."
 	@GOGC=20 ./bin/golangci-lint run
 

fees_test.go

@@ -617,6 +617,7 @@ func TestFeeQuote_MarshalUnmarshalJSON(t *testing.T) {
 	}
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
+			// nolint:errchkjson // it's a test, who cares
 			bb, _ := json.Marshal(test.quote)
 			var quote *FeeQuote
 			err := json.Unmarshal(bb, &quote)

tx_test.go

@@ -688,7 +688,7 @@ func Test_EstimateIsFeePaidEnough(t *testing.T) {
 					0, "76a914ff8c9344d4e76c0580420142f697e5fc2ce5c98e88ac", 1000,
 				))
 				assert.NoError(t, tx.AddOpReturnOutput([]byte("hellohello")))
-				assert.NoError(t, tx.AddP2PKHOutputFromAddress("mtestD3vRB7AoYWK2n6kLdZmAMLbLhDsLr", 894))
+				assert.NoError(t, tx.AddP2PKHOutputFromAddress("mtestD3vRB7AoYWK2n6kLdZmAMLbLhDsLr", 89))
 				assert.NoError(t, tx.FillAllInputs(context.Background(), &unlocker.Getter{PrivateKey: w.PrivKey}))
 				return tx
 			}(),
@@ -867,14 +867,14 @@ func Test_IsFeePaidEnough(t *testing.T) {
 				assert.NoError(t, tx.From("160f06232540dcb0e9b6db9b36a27f01da1e7e473989df67859742cf098d498f",
 					0, "76a914ff8c9344d4e76c0580420142f697e5fc2ce5c98e88ac", 1000))
 				assert.NoError(t, tx.AddOpReturnOutput([]byte("hellohello")))
-				assert.NoError(t, tx.AddP2PKHOutputFromAddress("mtestD3vRB7AoYWK2n6kLdZmAMLbLhDsLr", 894))
+				assert.NoError(t, tx.AddP2PKHOutputFromAddress("mtestD3vRB7AoYWK2n6kLdZmAMLbLhDsLr", 893))
 				err = tx.FillAllInputs(context.Background(), &unlocker.Getter{PrivateKey: w.PrivKey})
 				assert.Nil(t, err)
 				return tx
 			}(),
 			expSize: &bt.TxSize{
-				TotalBytes:     214,
-				TotalStdBytes:  201,
+				TotalBytes:     213,
+				TotalStdBytes:  200,
 				TotalDataBytes: 13,
 			},
 			isEnough: true,

txchange.go

@@ -6,7 +6,7 @@ import (
 
 const (
 	// DustLimit is the current minimum txo output accepted by miners.
-	DustLimit = 136
+	DustLimit = 1
 )
 
 // ChangeToAddress calculates the amount of fees needed to cover the transaction
@@ -63,62 +63,43 @@ func (tx *Tx) change(f *FeeQuote, output *changeOutput) (uint64, bool, error) {
 	}
 
 	available := inputAmount - outputAmount
-	standardFees, err := f.Fee(FeeTypeStandard)
+	size, err := tx.EstimateSizeWithTypes()
 	if err != nil {
 		return 0, false, err
 	}
-
-	var txFees *TxFees
-	if txFees, err = tx.EstimateFeesPaid(f); err != nil {
+	stdFee, err := f.Fee(FeeTypeStandard)
+	if err != nil {
 		return 0, false, err
 	}
-	changeFee, canAdd := tx.canAddChange(txFees, standardFees)
-	if !canAdd {
+	dataFee, err := f.Fee(FeeTypeData)
+	if err != nil {
 		return 0, false, err
 	}
-	available -= txFees.TotalFeePaid
-	// if we want to add to a new output, set
-	// newOutput to true, this will add the calculated change
-	// into a new output.
+	varIntUpper := VarInt(tx.OutputCount()).UpperLimitInc()
+	if varIntUpper == -1 {
+		return 0, false, nil
+	}
+	changeOutputFee := varIntUpper
+	changeP2pkhByteLen := uint64(0)
 	if output != nil && output.newOutput {
-		available -= changeFee
-		tx.AddOutput(&Output{Satoshis: available, LockingScript: output.lockingScript})
+		changeP2pkhByteLen = uint64(8 + 1 + 25)
 	}
 
-	return available, true, nil
-}
+	sFees := (size.TotalStdBytes + changeP2pkhByteLen) * uint64(stdFee.MiningFee.Satoshis) / uint64(stdFee.MiningFee.Bytes)
+	dFees := size.TotalDataBytes * uint64(dataFee.MiningFee.Satoshis) / uint64(dataFee.MiningFee.Bytes)
+	txFees := sFees + dFees + uint64(changeOutputFee)
 
-// canAddChange will return true / false if the tx can have a change output
-// added.
-// Reasons this could be false are:
-// - hitting max output limit
-// - change would be below dust limit
-// - not enough funds for change
-// We also return the change output fee amount, if we can add change
-func (tx *Tx) canAddChange(txFees *TxFees, standardFees *Fee) (uint64, bool) {
-	varIntUpper := VarInt(uint64(tx.OutputCount())).UpperLimitInc()
-	if varIntUpper == -1 {
-		return 0, false // upper limit of Outputs in one tx reached
-	}
-	changeOutputFee := uint64(varIntUpper)
-	// 8 bytes for satoshi value +1 for varint length + 25 bytes for p2pkh script (e.g. 76a914cc...05388ac)
-	changeP2pkhByteLen := 8 + 1 + 25
-	changeOutputFee += uint64(changeP2pkhByteLen * standardFees.MiningFee.Satoshis / standardFees.MiningFee.Bytes)
-
-	inputAmount := tx.TotalInputSatoshis()
-	outputAmount := tx.TotalOutputSatoshis()
-	// shouldn't get this far, but if we do, there's no change to add
-	if inputAmount <= outputAmount {
-		return 0, false
-	}
-	available := inputAmount - outputAmount
 	// not enough to add change, no change to add
-	if available <= changeOutputFee+txFees.TotalFeePaid {
-		return 0, false
+	if available <= txFees || available-txFees <= DustLimit {
+		return 0, false, nil
 	}
-	// after fees the change would be lower than dust limit, don't add change
-	if available-changeOutputFee+txFees.TotalFeePaid <= DustLimit {
-		return 0, false
+
+	// if we want to add to a new output, set
+	// newOutput to true, this will add the calculated change
+	// into a new output
+	available -= txFees
+	if output != nil && output.newOutput {
+		tx.AddOutput(&Output{Satoshis: available, LockingScript: output.lockingScript})
 	}
-	return changeOutputFee, true
+	return available, true, nil
 }

txchange_test.go

@@ -158,16 +158,13 @@ func TestTx_Change(t *testing.T) {
 			"0100000001760595866e99c1ce920197844740f5598b34763878696371d41b3a7c0a65b0b7000000006a47304402206b5b0b6546dbaccab4cd9c5698eeab7883f79ddbd4cbc195d4458b48b7dba6460220297a4c4b145e644d23cebdd7593f407e8da9c5bb3c3219767207121d65658ae3412102c8803fdd437d902f08e3c2344cb33065c99d7c99982018ff9f7219c3dd352ff0ffffffff03f4010000000000001976a9147a1980655efbfec416b2b0c663a7b3ac0b6a25d288ac000000000000000011006a02686903686f770361726503796f7577010000000000001976a91484e50b300b009833b297dc671817c79b5459da1d88ac00000000",
 			tx.String(),
 		)
-
 		feePaid := tx.TotalInputSatoshis() - tx.TotalOutputSatoshis()
 		assert.Equal(t, uint64(125), feePaid)
 
 		txSize := len(tx.Bytes())
 		assert.Equal(t, 251, txSize)
 
 		feeRate := float64(feePaid) / float64(txSize)
-		// note, due to the integer maths uses, this doesn't equal exactly 0.5, the fee is 125.5
-		// however, this is rounded down giving us the below final figure.
 		// The node will also perform the same deterministic fee cal to arrive at the above 125 sats.
 		assert.Equal(t, 0.49800796812749004, feeRate)
 	})