From 682aebdd53f474e6b946c36bd625ea4994d03dd5 Mon Sep 17 00:00:00 2001
From: Wilmer Paulino <wilmer.paulino@gmail.com>
Date: Wed, 15 May 2019 17:04:49 -0700
Subject: [PATCH] sweep: broadcast sweep transactions in descending fee rate
 order

In this commit, we address another issue that arose with the
introduction of the fee rate buckets. We'll use an example to explain
the problem space:

Let's say we have inputs A, B, and C within the same fee rate bucket. If
A's fee rate is bumped to a higher bucket, then it's currently possible
for the lower fee rate bucket to be swept first, which would produce an
invalid RBF transaction since we're removing an input from the original
without providing a higher fee. By the time we get to the higher fee
rate bucket, we broadcast a valid RBF transaction _only_ sweeping input
A, which would evict the transaction sweeping inputs B and C from the
mempool.

To prevent this eviction, we can simply broadcast the higher fee rate
sweep transactions first, to ensure we have valid RBF transactions.
---
 sweep/sweeper.go      | 14 ++++++++++++--
 sweep/sweeper_test.go | 31 +++++++++++++++----------------
 2 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/sweep/sweeper.go b/sweep/sweeper.go
index 3737ecb00..9c8e59c7b 100644
--- a/sweep/sweeper.go
+++ b/sweep/sweeper.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"math"
 	"math/rand"
+	"sort"
 	"sync"
 	"sync/atomic"
 	"time"
@@ -488,8 +489,17 @@ func (s *UtxoSweeper) collector(blockEpochs <-chan *chainntnfs.BlockEpoch,
 			s.timer = nil
 
 			// We'll attempt to cluster all of our inputs with
-			// similar fee rates.
-			for _, cluster := range s.clusterBySweepFeeRate() {
+			// similar fee rates. Before attempting to sweep them,
+			// we'll sort them in descending fee rate order. We do
+			// this to ensure any inputs which have had their fee
+			// rate bumped are broadcast first in order enforce the
+			// RBF policy.
+			inputClusters := s.clusterBySweepFeeRate()
+			sort.Slice(inputClusters, func(i, j int) bool {
+				return inputClusters[i].sweepFeeRate >
+					inputClusters[j].sweepFeeRate
+			})
+			for _, cluster := range inputClusters {
 				// Examine pending inputs and try to construct
 				// lists of inputs.
 				inputLists, err := s.getInputLists(
diff --git a/sweep/sweeper_test.go b/sweep/sweeper_test.go
index 08fd1034a..f7ec2173c 100644
--- a/sweep/sweeper_test.go
+++ b/sweep/sweeper_test.go
@@ -975,7 +975,8 @@ func TestGiveUp(t *testing.T) {
 }
 
 // TestDifferentFeePreferences ensures that the sweeper can have different
-// transactions for different fee preferences.
+// transactions for different fee preferences. These transactions should be
+// broadcast from highest to lowest fee rate.
 func TestDifferentFeePreferences(t *testing.T) {
 	ctx := createSweeperTestContext(t)
 
@@ -1009,27 +1010,25 @@ func TestDifferentFeePreferences(t *testing.T) {
 	}
 
 	// Start the sweeper's batch ticker, which should cause the sweep
-	// transactions to be broadcast.
+	// transactions to be broadcast in order of high to low fee preference.
 	ctx.tick()
 
-	ctx.receiveTx()
-	ctx.receiveTx()
+	// The first transaction broadcast should be the one spending the higher
+	// fee rate inputs.
+	sweepTx1 := ctx.receiveTx()
+	assertTxSweepsInputs(t, &sweepTx1, input1, input2)
+
+	// The second should be the one spending the lower fee rate inputs.
+	sweepTx2 := ctx.receiveTx()
+	assertTxSweepsInputs(t, &sweepTx2, input3)
 
 	// With the transactions broadcast, we'll mine a block to so that the
 	// result is delivered to each respective client.
 	ctx.backend.mine()
-
-	// We should expect to see a single transaction that sweeps the high fee
-	// preference inputs.
-	sweepTx1 := receiveSpendTx(t, resultChan1)
-	assertTxSweepsInputs(t, sweepTx1, input1, input2)
-	sweepTx2 := receiveSpendTx(t, resultChan2)
-	assertTxSweepsInputs(t, sweepTx2, input1, input2)
-
-	// We should expect to see a distinct transaction that sweeps the low
-	// fee preference inputs.
-	sweepTx3 := receiveSpendTx(t, resultChan3)
-	assertTxSweepsInputs(t, sweepTx3, input3)
+	resultChans := []chan Result{resultChan1, resultChan2, resultChan3}
+	for _, resultChan := range resultChans {
+		ctx.expectResult(resultChan, nil)
+	}
 
 	ctx.finish(1)
 }