Fix reward payment tx fee calculation

[jholdstock]

User @sebitt27 reported that his pool reward payments were paying extremely large fees (~0.074 DCR/kB). The cause was several bugs in the tx construction code, specifically in the function applyTxFees.

This PR starts by refactoring the tests for applyTxFees such that the existing behaviour can be characterized, and then goes on to fix the issues. The impact of the fixes (in the last two commits) can be observed by following the changes to the expected output amounts in the tests.

Using this new code the fee rate of the transaction shared by @sebitt27 is reduced to 0.000106216 DCR/kB, very close to the ideal value of 0.0001 DCR/kB.

[davecgh]

This section is a significant improvement as it fixes the primary issue, but it's still not handling extra atoms leftover as the result of non-even divisions properly. Moreover, the rounding behavior of MulF64 could lead to underpayment of the transaction fee because it might round down which would lead to one less atom being subtracted than is necessary thereby effectively reducing the fee by 1.

To illustrate this, consider a total fee of 41 atoms, but with a proportional split of 37.5%, 32.5%, and 30%. Using the existing code here would produce:

Output 1: round(41 * .375) = 15
Output 2: round(41 * .325) = 13
Output 3: round(41 * .3) = 12

Now, if you add those up, 15+13+12 = 40, which is not the required 41 atoms. In other words, the tx would be rejected for not paying enough fees.

That is why the rounding behavior should be avoided completely in favor of taking the floor at all times and making up for the difference as necessary via applying the remaining additional atoms to random outputs. Alternatively, it could always take the ceiling, but that would result in overpayment in some scenarios, so I don't recommend it.

I believe the following rewritten code should handle everything properly, however, do note that the tests will need to be updated to ensure that the expected amount can either be the existing values or one less (because the fee might have been increased by one thus reducing the output amount by one):

func (pm *PaymentMgr) applyTxFees(inputs []chainjson.TransactionInput, outputs map[string]dcrutil.Amount,
	feeAddr stdaddr.Address) (dcrutil.Amount, error) {
	const funcName = "applyTxFees"
	if len(inputs) == 0 {
		desc := fmt.Sprintf("%s: cannot create a payout transaction "+
			"without a tx input", funcName)
		return 0, errs.PoolError(errs.TxIn, desc)
	}
	if len(outputs) == 0 {
		desc := fmt.Sprintf("%s: cannot create a payout transaction "+
			"without a tx output", funcName)
		return 0, errs.PoolError(errs.TxOut, desc)
	}

	estTxFee := estimateTxFee(len(inputs), len(outputs))

	// Determine the total output amount without the pool fee.
	var outputAmountSansPoolFee dcrutil.Amount
	for addr, v := range outputs {
		if addr == feeAddr.String() {
			continue
		}
		outputAmountSansPoolFee += v
	}

	// Determine the proportional transaction fees for each output other than
	// the pool fee.  Note that in order to avoid overpayment of the fee, each
	// proportional fee is calculated via a floor.  This means there will still
	// be a few atoms leftover to account for when the fee is not evenly
	// divisible by the number of outputs.
	var txFeesPaid dcrutil.Amount
	proportionalTxFees := make(map[string]dcrutil.Amount)
	for addr, v := range outputs {
		// Pool fee payments are excluded from tx fee deductions.
		if addr == feeAddr.String() {
			continue
		}

		ratio := float64(v) / float64(outputAmountSansPoolFee)
		outFee := dcrutil.Amount(math.Floor(float64(estTxFee) * ratio))
		proportionalTxFees[addr] = outFee
		txFeesPaid += outFee
	}

	// Apply the fees to the outputs while accounting for any extra atoms needed
	// to reach the required transaction fee by reducing random outputs
	// accordingly.
	remainingTxFeeAtoms := estTxFee - txFeesPaid
	for addr, fee := range proportionalTxFees {
		outputs[addr] -= fee
		if remainingTxFeeAtoms > 0 {
			outputs[addr]--
			remainingTxFeeAtoms--
		}
	}

	return estTxFee, nil

[davecgh]

Here is a diff that updates the tests per my comments and code:

diff --git a/pool/paymentmgr_test.go b/pool/paymentmgr_test.go
index f07f846..9a1706f 100644
--- a/pool/paymentmgr_test.go
+++ b/pool/paymentmgr_test.go
@@ -512,7 +512,7 @@ func testPaymentMgrApplyTxFees(t *testing.T) {
                        },
                        expectedOutputs: map[string]dcrutil.Amount{
                                feeAddr: dcrutil.Amount(50000000),
-                               "user1": dcrutil.Amount(299997853),
+                               "user1": dcrutil.Amount(299997854),
                                "user2": dcrutil.Amount(149998927),
                        },
                },
@@ -587,9 +587,11 @@ func testPaymentMgrApplyTxFees(t *testing.T) {
 
                        // Validate outputs have been modified as expected.
                        for addr, amt := range test.outputs {
-                               if amt != test.expectedOutputs[addr] {
-                                       t.Fatalf("expected payment for %s to be %v, got %v",
-                                               addr, test.expectedOutputs[addr], amt)
+                               wantAmt1 := test.expectedOutputs[addr]
+                               wantAmt2 := wantAmt1 - 1
+                               if amt != wantAmt1 && amt != wantAmt2 {
+                                       t.Fatalf("expected payment for %s to be %v or %v, got %v",
+                                               addr, wantAmt1, wantAmt2, amt)
                                }
                        }

[jholdstock]

Amazing, thanks Dave. I've replaced the last commit in the PR to include this code.