autopilot: modify interfaces to specify *exactly* how many chans to open

In this commit, we fix a regression introduced by a recent change which
would allow the agent to detect a channel as failed, and blacklist the
node, promising faster convergence with the ideal state of the
heuristic.

The source of this bug is that we would use the set of blacklisted
nodes in order to compute how many additional channels we should open.
If 10 failures happened, then we would think that we had opened up 10
channels, and stop much earlier than we actually should.

To fix this, while ensuring we don’t retry to failed peers, the
NeedMoreChans method will now return *how* anymore channels to open,
and the Select method will take in how many channels it should try to
open *exactly*.

autopilot/agent.go

@@ -362,7 +362,7 @@ func (a *Agent) controller(startingBalance btcutil.Amount) {
 			// consult our channel attachment heuristic to
 			// determine if we should open up any additional
 			// channels or modify existing channels.
-			availableFunds, needMore := a.cfg.Heuristic.NeedMoreChans(
+			availableFunds, numChans, needMore := a.cfg.Heuristic.NeedMoreChans(
 				totalChans, a.totalBalance,
 			)
 			if !needMore {
@@ -387,7 +387,7 @@ func (a *Agent) controller(startingBalance btcutil.Amount) {
 			// for us to use.
 			chanCandidates, err := a.cfg.Heuristic.Select(
 				a.cfg.Self, a.cfg.Graph, availableFunds,
-				nodesToSkip,
+				numChans, nodesToSkip,
 			)
 			if err != nil {
 				log.Errorf("Unable to select candidates for "+
@@ -419,6 +419,7 @@ func (a *Agent) controller(startingBalance btcutil.Amount) {
 				go func(directive AttachmentDirective) {
 					pub := directive.PeerKey
 					err := a.cfg.ChanController.OpenChannel(
+
 						directive.PeerKey,
 						directive.ChanAmt,
 						directive.Addrs,

autopilot/agent_test.go

@@ -8,7 +8,6 @@ import (
 	"time"
 
 	"errors"
-	"fmt"
 
 	"github.com/roasbeef/btcd/btcec"
 	"github.com/roasbeef/btcd/wire"
@@ -17,6 +16,7 @@ import (
 
 type moreChansResp struct {
 	needMore bool
+	numMore  uint32
 	amt      btcutil.Amount
 }
 
@@ -34,7 +34,7 @@ type mockHeuristic struct {
 }
 
 func (m *mockHeuristic) NeedMoreChans(chans []Channel,
-	balance btcutil.Amount) (btcutil.Amount, bool) {
+	balance btcutil.Amount) (btcutil.Amount, uint32, bool) {
 
 	if m.moreChanArgs != nil {
 		m.moreChanArgs <- moreChanArg{
@@ -45,7 +45,7 @@ func (m *mockHeuristic) NeedMoreChans(chans []Channel,
 	}
 
 	resp := <-m.moreChansResps
-	return resp.amt, resp.needMore
+	return resp.amt, resp.numMore, resp.needMore
 }
 
 type directiveArg struct {
@@ -56,8 +56,8 @@ type directiveArg struct {
 }
 
 func (m *mockHeuristic) Select(self *btcec.PublicKey, graph ChannelGraph,
-
+	amtToUse btcutil.Amount, numChans uint32,
-	amtToUse btcutil.Amount, skipChans map[NodeID]struct{}) ([]AttachmentDirective, error) {
+	skipChans map[NodeID]struct{}) ([]AttachmentDirective, error) {
 
 	if m.directiveArgs != nil {
 		m.directiveArgs <- directiveArg{
@@ -160,7 +160,7 @@ func TestAgentChannelOpenSignal(t *testing.T) {
 	wg.Add(1)
 	go func() {
 		select {
-		case heuristic.moreChansResps <- moreChansResp{false, 0}:
+		case heuristic.moreChansResps <- moreChansResp{false, 0, 0}:
 			wg.Done()
 			return
 		case <-time.After(time.Second * 10):
@@ -185,7 +185,7 @@ func TestAgentChannelOpenSignal(t *testing.T) {
 	wg.Add(1)
 	go func() {
 		select {
-		case heuristic.moreChansResps <- moreChansResp{false, 0}:
+		case heuristic.moreChansResps <- moreChansResp{false, 0, 0}:
 			// At this point, the local state of the agent should
 			// have also been updated to reflect that the LN node
 			// now has an additional channel with one BTC.
@@ -290,15 +290,14 @@ func TestAgentChannelFailureSignal(t *testing.T) {
 	// First ensure the agent will attempt to open a new channel. Return
 	// that we need more channels, and have 5BTC to use.
 	select {
-	case heuristic.moreChansResps <- moreChansResp{true, 5 * btcutil.SatoshiPerBitcoin}:
+	case heuristic.moreChansResps <- moreChansResp{true, 1, 5 * btcutil.SatoshiPerBitcoin}:
-		fmt.Println("Returning 5BTC from heuristic")
 	case <-time.After(time.Second * 10):
 		t.Fatal("heuristic wasn't queried in time")
 	}
 
 	// At this point, the agent should now be querying the heuristic to
-	// request attachment directives, return a fake so the agent will attempt
+	// request attachment directives, return a fake so the agent will
-	// to open a channel.
+	// attempt to open a channel.
 	var fakeDirective = AttachmentDirective{
 		PeerKey: self,
 		ChanAmt: btcutil.SatoshiPerBitcoin,
@@ -311,7 +310,6 @@ func TestAgentChannelFailureSignal(t *testing.T) {
 
 	select {
 	case heuristic.directiveResps <- []AttachmentDirective{fakeDirective}:
-		fmt.Println("Returning a node to connect to from heuristic")
 	case <-time.After(time.Second * 10):
 		t.Fatal("heuristic wasn't queried in time")
 	}
@@ -320,15 +318,13 @@ func TestAgentChannelFailureSignal(t *testing.T) {
 
 	// Now ensure that the controller loop is re-executed.
 	select {
-	case heuristic.moreChansResps <- moreChansResp{true, 5 * btcutil.SatoshiPerBitcoin}:
+	case heuristic.moreChansResps <- moreChansResp{true, 1, 5 * btcutil.SatoshiPerBitcoin}:
-		fmt.Println("Returning need more channels from heuristic")
 	case <-time.After(time.Second * 10):
 		t.Fatal("heuristic wasn't queried in time")
 	}
 
 	select {
 	case heuristic.directiveResps <- []AttachmentDirective{}:
-		fmt.Println("Returning an empty directives list")
 	case <-time.After(time.Second * 10):
 		t.Fatal("heuristic wasn't queried in time")
 	}
@@ -397,7 +393,7 @@ func TestAgentChannelCloseSignal(t *testing.T) {
 	wg.Add(1)
 	go func() {
 		select {
-		case heuristic.moreChansResps <- moreChansResp{false, 0}:
+		case heuristic.moreChansResps <- moreChansResp{false, 0, 0}:
 			wg.Done()
 			return
 		case <-time.After(time.Second * 10):
@@ -418,7 +414,7 @@ func TestAgentChannelCloseSignal(t *testing.T) {
 	wg.Add(1)
 	go func() {
 		select {
-		case heuristic.moreChansResps <- moreChansResp{false, 0}:
+		case heuristic.moreChansResps <- moreChansResp{false, 0, 0}:
 			// At this point, the local state of the agent should
 			// have also been updated to reflect that the LN node
 			// has no existing open channels.
@@ -509,7 +505,7 @@ func TestAgentBalanceUpdate(t *testing.T) {
 	wg.Add(1)
 	go func() {
 		select {
-		case heuristic.moreChansResps <- moreChansResp{false, 0}:
+		case heuristic.moreChansResps <- moreChansResp{false, 0, 0}:
 			wg.Done()
 			return
 		case <-time.After(time.Second * 10):
@@ -531,7 +527,7 @@ func TestAgentBalanceUpdate(t *testing.T) {
 	wg.Add(1)
 	go func() {
 		select {
-		case heuristic.moreChansResps <- moreChansResp{false, 0}:
+		case heuristic.moreChansResps <- moreChansResp{false, 0, 0}:
 			// At this point, the local state of the agent should
 			// have also been updated to reflect that the LN node
 			// now has an additional  5BTC available.
@@ -619,6 +615,8 @@ func TestAgentImmediateAttach(t *testing.T) {
 
 	var wg sync.WaitGroup
 
+	const numChans = 5
+
 	// The very first thing the agent should do is query the NeedMoreChans
 	// method on the passed heuristic. So we'll provide it with a response
 	// that will kick off the main loop.
@@ -629,7 +627,7 @@ func TestAgentImmediateAttach(t *testing.T) {
 		// We'll send over a response indicating that it should
 		// establish more channels, and give it a budget of 5 BTC to do
 		// so.
-		case heuristic.moreChansResps <- moreChansResp{true, 5 * btcutil.SatoshiPerBitcoin}:
+		case heuristic.moreChansResps <- moreChansResp{true, numChans, 5 * btcutil.SatoshiPerBitcoin}:
 			wg.Done()
 			return
 		case <-time.After(time.Second * 10):
@@ -644,7 +642,6 @@ func TestAgentImmediateAttach(t *testing.T) {
 	// At this point, the agent should now be querying the heuristic to
 	// requests attachment directives. We'll generate 5 mock directives so
 	// it can progress within its loop.
-	const numChans = 5
 	directives := make([]AttachmentDirective, numChans)
 	for i := 0; i < numChans; i++ {
 		directives[i] = AttachmentDirective{
@@ -762,7 +759,7 @@ func TestAgentPendingChannelState(t *testing.T) {
 		// We'll send over a response indicating that it should
 		// establish more channels, and give it a budget of 1 BTC to do
 		// so.
-		case heuristic.moreChansResps <- moreChansResp{true, btcutil.SatoshiPerBitcoin}:
+		case heuristic.moreChansResps <- moreChansResp{true, 1, btcutil.SatoshiPerBitcoin}:
 			wg.Done()
 			return
 		case <-time.After(time.Second * 10):
@@ -856,7 +853,7 @@ func TestAgentPendingChannelState(t *testing.T) {
 	// We'll send across a response indicating that it *does* need more
 	// channels.
 	select {
-	case heuristic.moreChansResps <- moreChansResp{true, btcutil.SatoshiPerBitcoin}:
+	case heuristic.moreChansResps <- moreChansResp{true, 1, btcutil.SatoshiPerBitcoin}:
 	case <-time.After(time.Second * 10):
 		t.Fatalf("need more chans wasn't queried in time")
 	}

autopilot/interface.go

@@ -111,17 +111,20 @@ type AttachmentHeuristic interface {
 	// opened within the channel graph. If the heuristic decides that we do
 	// indeed need more channels, then the second argument returned will
 	// represent the amount of additional funds to be used towards creating
-	// channels.
+	// channels. This method should also return the exact *number* of
-	//
+	// additional channels that are needed in order to converge towards our
-	// TODO(roasbeef): return number of chans? ensure doesn't go over
+	// ideal state.
-	NeedMoreChans(chans []Channel, balance btcutil.Amount) (btcutil.Amount, bool)
+	NeedMoreChans(chans []Channel, balance btcutil.Amount) (btcutil.Amount, uint32, bool)
 
 	// Select is a method that given the current state of the channel
 	// graph, a set of nodes to ignore, and an amount of available funds,
 	// should return a set of attachment directives which describe which
 	// additional channels should be opened within the graph to push the
-	// heuristic back towards its equilibrium state.
+	// heuristic back towards its equilibrium state. The numNewChans
-	Select(self *btcec.PublicKey, graph ChannelGraph, amtToUse btcutil.Amount,
+	// argument represents the additional number of channels that should be
+	// open.
+	Select(self *btcec.PublicKey, graph ChannelGraph,
+		amtToUse btcutil.Amount, numNewChans uint32,
 		skipNodes map[NodeID]struct{}) ([]AttachmentDirective, error)
 }
 

autopilot/prefattach.go

@@ -58,14 +58,19 @@ var _ AttachmentHeuristic = (*ConstrainedPrefAttachment)(nil)
 //
 // NOTE: This is a part of the AttachmentHeuristic interface.
 func (p *ConstrainedPrefAttachment) NeedMoreChans(channels []Channel,
-	funds btcutil.Amount) (btcutil.Amount, bool) {
+	funds btcutil.Amount) (btcutil.Amount, uint32, bool) {
 
 	// If we're already over our maximum allowed number of channels, then
 	// we'll instruct the controller not to create any more channels.
 	if len(channels) >= int(p.chanLimit) {
-		return 0, false
+		return 0, 0, false
 	}
 
+	// The number of additional channels that should be opened is the
+	// difference between the channel limit, and the number of channels we
+	// already have open.
+	numAdditionalChans := uint32(p.chanLimit) - uint32(len(channels))
+
 	// First, we'll tally up the total amount of funds that are currently
 	// present within the set of active channels.
 	var totalChanAllocation btcutil.Amount
@@ -86,14 +91,14 @@ func (p *ConstrainedPrefAttachment) NeedMoreChans(channels []Channel,
 	// of channels to attempt to open.
 	needMore := fundsFraction < p.threshold
 	if !needMore {
-		return 0, false
+		return 0, 0, false
 	}
 
 	// Now that we know we need more funds, we'll compute the amount of
 	// additional funds we should allocate towards channels.
 	targetAllocation := btcutil.Amount(float64(totalFunds) * p.threshold)
 	fundsAvailable := targetAllocation - totalChanAllocation
-	return fundsAvailable, true
+	return fundsAvailable, numAdditionalChans, true
 }
 
 // NodeID is a simple type that holds a EC public key serialized in compressed
@@ -137,7 +142,7 @@ func shuffleCandidates(candidates []Node) []Node {
 //
 // NOTE: This is a part of the AttachmentHeuristic interface.
 func (p *ConstrainedPrefAttachment) Select(self *btcec.PublicKey, g ChannelGraph,
-	fundsAvailable btcutil.Amount,
+	fundsAvailable btcutil.Amount, numNewChans uint32,
 	skipNodes map[NodeID]struct{}) ([]AttachmentDirective, error) {
 
 	// TODO(roasbeef): rename?
@@ -151,8 +156,7 @@ func (p *ConstrainedPrefAttachment) Select(self *btcec.PublicKey, g ChannelGraph
 	// We'll continue our attachment loop until we've exhausted the current
 	// amount of available funds.
 	visited := make(map[NodeID]struct{})
-	chanLimit := p.chanLimit - uint16(len(skipNodes))
+	for i := uint32(0); i < numNewChans; i++ {
-	for i := uint16(0); i < chanLimit; i++ {
 		// selectionSlice will be used to randomly select a node
 		// according to a power law distribution. For each connected
 		// edge, we'll add an instance of the node to this slice. Thus,

autopilot/prefattach_test.go

@@ -38,6 +38,7 @@ func TestConstrainedPrefAttachmentNeedMoreChan(t *testing.T) {
 
 		needMore     bool
 		amtAvailable btcutil.Amount
+		numMore      uint32
 	}{
 		// Many available funds, but already have too many active open
 		// channels.
@@ -59,6 +60,7 @@ func TestConstrainedPrefAttachmentNeedMoreChan(t *testing.T) {
 			btcutil.Amount(btcutil.SatoshiPerBitcoin * 10),
 			false,
 			0,
+			0,
 		},
 
 		// Ratio of funds in channels and total funds meets the
@@ -77,13 +79,15 @@ func TestConstrainedPrefAttachmentNeedMoreChan(t *testing.T) {
 			btcutil.Amount(btcutil.SatoshiPerBitcoin * 2),
 			false,
 			0,
+			0,
 		},
 
 		// Ratio of funds in channels and total funds is below the
 		// threshold. We have 10 BTC allocated amongst channels and
 		// funds, atm. We're targeting 50%, so 5 BTC should be
 		// allocated. Only 1 BTC is atm, so 4 BTC should be
-		// recommended.
+		// recommended. We should also request 2 more channels as the
+		// limit is 3.
 		{
 			[]Channel{
 				{
@@ -94,14 +98,16 @@ func TestConstrainedPrefAttachmentNeedMoreChan(t *testing.T) {
 			btcutil.Amount(btcutil.SatoshiPerBitcoin * 9),
 			true,
 			btcutil.Amount(btcutil.SatoshiPerBitcoin * 4),
+			2,
 		},
 
 		// Ratio of funds in channels and total funds is below the
 		// threshold. We have 14 BTC total amongst the wallet's
 		// balance, and our currently opened channels. Since we're
 		// targeting a 50% allocation, we should commit 7 BTC. The
-		// current channels commit 4 BTC, so we should expected 3 bTC
+		// current channels commit 4 BTC, so we should expected 3 BTC
-		// to be committed.
+		// to be committed. We should only request a single additional
+		// channel as the limit is 3.
 		{
 			[]Channel{
 				{
@@ -116,6 +122,7 @@ func TestConstrainedPrefAttachmentNeedMoreChan(t *testing.T) {
 			btcutil.Amount(btcutil.SatoshiPerBitcoin * 10),
 			true,
 			btcutil.Amount(btcutil.SatoshiPerBitcoin * 3),
+			1,
 		},
 
 		// Ratio of funds in channels and total funds is above the
@@ -134,6 +141,7 @@ func TestConstrainedPrefAttachmentNeedMoreChan(t *testing.T) {
 			btcutil.Amount(btcutil.SatoshiPerBitcoin),
 			false,
 			0,
+			0,
 		},
 	}
 
@@ -141,8 +149,9 @@ func TestConstrainedPrefAttachmentNeedMoreChan(t *testing.T) {
 		chanLimit, threshold)
 
 	for i, testCase := range testCases {
-		amtToAllocate, needMore := prefAttach.NeedMoreChans(testCase.channels,
+		amtToAllocate, numMore, needMore := prefAttach.NeedMoreChans(
-			testCase.walletAmt)
+			testCase.channels, testCase.walletAmt,
+		)
 
 		if amtToAllocate != testCase.amtAvailable {
 			t.Fatalf("test #%v: expected %v, got %v",
@@ -152,6 +161,10 @@ func TestConstrainedPrefAttachmentNeedMoreChan(t *testing.T) {
 			t.Fatalf("test #%v: expected %v, got %v",
 				i, testCase.needMore, needMore)
 		}
+		if numMore != testCase.numMore {
+			t.Fatalf("test #%v: expected %v, got %v",
+				i, testCase.numMore, numMore)
+		}
 	}
 }
 
@@ -247,7 +260,7 @@ func TestConstrainedPrefAttachmentSelectEmptyGraph(t *testing.T) {
 			// creation given the current state of the graph.
 			const walletFunds = btcutil.SatoshiPerBitcoin
 			directives, err := prefAttach.Select(self, graph,
-				walletFunds, skipNodes)
+				walletFunds, 5, skipNodes)
 			if err != nil {
 				t1.Fatalf("unable to select attachment "+
 					"directives: %v", err)
@@ -316,7 +329,7 @@ func TestConstrainedPrefAttachmentSelectTwoVertexes(t *testing.T) {
 			// creation given the current state of the graph.
 			const walletFunds = btcutil.SatoshiPerBitcoin * 10
 			directives, err := prefAttach.Select(self, graph,
-				walletFunds, skipNodes)
+				walletFunds, 2, skipNodes)
 			if err != nil {
 				t1.Fatalf("unable to select attachment directives: %v", err)
 			}
@@ -395,7 +408,7 @@ func TestConstrainedPrefAttachmentSelectInsufficientFunds(t *testing.T) {
 			// passing zero for the amount of wallet funds. This
 			// should return an empty slice of directives.
 			directives, err := prefAttach.Select(self, graph, 0,
-				skipNodes)
+				0, skipNodes)
 			if err != nil {
 				t1.Fatalf("unable to select attachment "+
 					"directives: %v", err)
@@ -503,7 +516,7 @@ func TestConstrainedPrefAttachmentSelectGreedyAllocation(t *testing.T) {
 			// allocate funds to channels.
 			const availableBalance = btcutil.SatoshiPerBitcoin * 2.5
 			directives, err := prefAttach.Select(self, graph,
-				availableBalance, skipNodes)
+				availableBalance, 5, skipNodes)
 			if err != nil {
 				t1.Fatalf("unable to select attachment "+
 					"directives: %v", err)
@@ -594,7 +607,7 @@ func TestConstrainedPrefAttachmentSelectSkipNodes(t *testing.T) {
 			// candidates.
 			const availableBalance = btcutil.SatoshiPerBitcoin * 2.5
 			directives, err := prefAttach.Select(self, graph,
-				availableBalance, skipNodes)
+				availableBalance, 2, skipNodes)
 			if err != nil {
 				t1.Fatalf("unable to select attachment "+
 					"directives: %v", err)
@@ -618,7 +631,7 @@ func TestConstrainedPrefAttachmentSelectSkipNodes(t *testing.T) {
 			// should get no new directives as both nodes has
 			// already been attached to.
 			directives, err = prefAttach.Select(self, graph,
-				availableBalance, skipNodes)
+				availableBalance, 2, skipNodes)
 			if err != nil {
 				t1.Fatalf("unable to select attachment "+
 					"directives: %v", err)