discovery+routing: cancel dependent jobs if parent validation fails

Previously, we would always allow dependent jobs to be processed,
regardless of the result of its parent job's validation. This isn't
correct, as a parent job contains actions necessary to successfully
process a dependent job. A prime example of this can be found within the
AuthenticatedGossiper, where an incoming channel announcement and update
are both processed, but if the channel announcement job fails to
complete, then the gossiper is unable to properly validate the update.
This commit aims to address this by preventing the dependent jobs to
run.

routing/router.go

@@ -967,7 +967,8 @@ func (r *ChannelRouter) networkHandler() {
 					update.msg,
 				)
 				if err != nil {
-					if err != ErrVBarrierShuttingDown {
+					if err != ErrVBarrierShuttingDown &&
+						err != ErrParentValidationFailed {
 						log.Warnf("unexpected error "+
 							"during validation "+
 							"barrier shutdown: %v",
@@ -985,7 +986,11 @@ func (r *ChannelRouter) networkHandler() {
 
 				// If this message had any dependencies, then
 				// we can now signal them to continue.
-				validationBarrier.SignalDependants(update.msg)
+				allowDependents := err == nil ||
+					IsError(err, ErrIgnored, ErrOutdated)
+				validationBarrier.SignalDependants(
+					update.msg, allowDependents,
+				)
 				if err != nil {
 					return
 				}

routing/validation_barrier.go

@@ -9,10 +9,28 @@ import (
 	"github.com/lightningnetwork/lnd/routing/route"
 )
 
-// ErrVBarrierShuttingDown signals that the barrier has been requested to
-// shutdown, and that the caller should not treat the wait condition as
-// fulfilled.
-var ErrVBarrierShuttingDown = errors.New("validation barrier shutting down")
+var (
+	// ErrVBarrierShuttingDown signals that the barrier has been requested
+	// to shutdown, and that the caller should not treat the wait condition
+	// as fulfilled.
+	ErrVBarrierShuttingDown = errors.New("validation barrier shutting down")
+
+	// ErrParentValidationFailed signals that the validation of a
+	// dependent's parent failed, so the dependent must not be processed.
+	ErrParentValidationFailed = errors.New("parent validation failed")
+)
+
+// validationSignals contains two signals which allows the ValidationBarrier to
+// communicate back to the caller whether a dependent should be processed or not
+// based on whether its parent was successfully validated. Only one of these
+// signals is to be used at a time.
+type validationSignals struct {
+	// allow is the signal used to allow a dependent to be processed.
+	allow chan struct{}
+
+	// deny is the signal used to prevent a dependent from being processed.
+	deny chan struct{}
+}
 
 // ValidationBarrier is a barrier used to ensure proper validation order while
 // concurrently validating new announcements for channel edges, and the
@@ -31,19 +49,19 @@ type ValidationBarrier struct {
 	// ChannelAnnouncement like validation job going on. Once the job has
 	// been completed, the channel will be closed unblocking any
 	// dependants.
-	chanAnnFinSignal map[lnwire.ShortChannelID]chan struct{}
+	chanAnnFinSignal map[lnwire.ShortChannelID]*validationSignals
 
 	// chanEdgeDependencies tracks any channel edge updates which should
 	// wait until the completion of the ChannelAnnouncement before
 	// proceeding. This is a dependency, as we can't validate the update
 	// before we validate the announcement which creates the channel
 	// itself.
-	chanEdgeDependencies map[lnwire.ShortChannelID]chan struct{}
+	chanEdgeDependencies map[lnwire.ShortChannelID]*validationSignals
 
 	// nodeAnnDependencies tracks any pending NodeAnnouncement validation
 	// jobs which should wait until the completion of the
 	// ChannelAnnouncement before proceeding.
-	nodeAnnDependencies map[route.Vertex]chan struct{}
+	nodeAnnDependencies map[route.Vertex]*validationSignals
 
 	quit chan struct{}
 	sync.Mutex
@@ -56,9 +74,9 @@ func NewValidationBarrier(numActiveReqs int,
 	quitChan chan struct{}) *ValidationBarrier {
 
 	v := &ValidationBarrier{
-		chanAnnFinSignal:     make(map[lnwire.ShortChannelID]chan struct{}),
-		chanEdgeDependencies: make(map[lnwire.ShortChannelID]chan struct{}),
-		nodeAnnDependencies:  make(map[route.Vertex]chan struct{}),
+		chanAnnFinSignal:     make(map[lnwire.ShortChannelID]*validationSignals),
+		chanEdgeDependencies: make(map[lnwire.ShortChannelID]*validationSignals),
+		nodeAnnDependencies:  make(map[route.Vertex]*validationSignals),
 		quit:                 quitChan,
 	}
 
@@ -107,24 +125,31 @@ func (v *ValidationBarrier) InitJobDependencies(job interface{}) {
 			// validate this announcement. All dependants will
 			// point to this same channel, so they'll be unblocked
 			// at the same time.
-			annFinCond := make(chan struct{})
-			v.chanAnnFinSignal[msg.ShortChannelID] = annFinCond
-			v.chanEdgeDependencies[msg.ShortChannelID] = annFinCond
+			signals := &validationSignals{
+				allow: make(chan struct{}),
+				deny:  make(chan struct{}),
+			}
 
-			v.nodeAnnDependencies[route.Vertex(msg.NodeID1)] = annFinCond
-			v.nodeAnnDependencies[route.Vertex(msg.NodeID2)] = annFinCond
+			v.chanAnnFinSignal[msg.ShortChannelID] = signals
+			v.chanEdgeDependencies[msg.ShortChannelID] = signals
+
+			v.nodeAnnDependencies[route.Vertex(msg.NodeID1)] = signals
+			v.nodeAnnDependencies[route.Vertex(msg.NodeID2)] = signals
 		}
 	case *channeldb.ChannelEdgeInfo:
 
 		shortID := lnwire.NewShortChanIDFromInt(msg.ChannelID)
 		if _, ok := v.chanAnnFinSignal[shortID]; !ok {
-			annFinCond := make(chan struct{})
+			signals := &validationSignals{
+				allow: make(chan struct{}),
+				deny:  make(chan struct{}),
+			}
 
-			v.chanAnnFinSignal[shortID] = annFinCond
-			v.chanEdgeDependencies[shortID] = annFinCond
+			v.chanAnnFinSignal[shortID] = signals
+			v.chanEdgeDependencies[shortID] = signals
 
-			v.nodeAnnDependencies[route.Vertex(msg.NodeKey1Bytes)] = annFinCond
-			v.nodeAnnDependencies[route.Vertex(msg.NodeKey2Bytes)] = annFinCond
+			v.nodeAnnDependencies[route.Vertex(msg.NodeKey1Bytes)] = signals
+			v.nodeAnnDependencies[route.Vertex(msg.NodeKey2Bytes)] = signals
 		}
 
 	// These other types don't have any dependants, so no further
@@ -162,8 +187,8 @@ func (v *ValidationBarrier) CompleteJob() {
 func (v *ValidationBarrier) WaitForDependants(job interface{}) error {
 
 	var (
-		signal chan struct{}
-		ok     bool
+		signals *validationSignals
+		ok      bool
 	)
 
 	v.Lock()
@@ -173,15 +198,15 @@ func (v *ValidationBarrier) WaitForDependants(job interface{}) error {
 	// completion of any active ChannelAnnouncement jobs related to them.
 	case *channeldb.ChannelEdgePolicy:
 		shortID := lnwire.NewShortChanIDFromInt(msg.ChannelID)
-		signal, ok = v.chanEdgeDependencies[shortID]
+		signals, ok = v.chanEdgeDependencies[shortID]
 	case *channeldb.LightningNode:
 		vertex := route.Vertex(msg.PubKeyBytes)
-		signal, ok = v.nodeAnnDependencies[vertex]
+		signals, ok = v.nodeAnnDependencies[vertex]
 	case *lnwire.ChannelUpdate:
-		signal, ok = v.chanEdgeDependencies[msg.ShortChannelID]
+		signals, ok = v.chanEdgeDependencies[msg.ShortChannelID]
 	case *lnwire.NodeAnnouncement:
 		vertex := route.Vertex(msg.NodeID)
-		signal, ok = v.nodeAnnDependencies[vertex]
+		signals, ok = v.nodeAnnDependencies[vertex]
 
 	// Other types of jobs can be executed immediately, so we'll just
 	// return directly.
@@ -204,7 +229,9 @@ func (v *ValidationBarrier) WaitForDependants(job interface{}) error {
 		select {
 		case <-v.quit:
 			return ErrVBarrierShuttingDown
-		case <-signal:
+		case <-signals.deny:
+			return ErrParentValidationFailed
+		case <-signals.allow:
 			return nil
 		}
 	}
@@ -212,10 +239,10 @@ func (v *ValidationBarrier) WaitForDependants(job interface{}) error {
 	return nil
 }
 
-// SignalDependants will signal any jobs that are dependent on this job that
+// SignalDependants will allow/deny any jobs that are dependent on this job that
 // they can continue execution. If the job doesn't have any dependants, then
 // this function sill exit immediately.
-func (v *ValidationBarrier) SignalDependants(job interface{}) {
+func (v *ValidationBarrier) SignalDependants(job interface{}, allow bool) {
 	v.Lock()
 	defer v.Unlock()
 
@@ -223,18 +250,26 @@ func (v *ValidationBarrier) SignalDependants(job interface{}) {
 
 	// If we've just finished executing a ChannelAnnouncement, then we'll
 	// close out the signal, and remove the signal from the map of active
-	// ones. This will allow any dependent jobs to continue execution.
+	// ones. This will allow/deny any dependent jobs to continue execution.
 	case *channeldb.ChannelEdgeInfo:
 		shortID := lnwire.NewShortChanIDFromInt(msg.ChannelID)
-		finSignal, ok := v.chanAnnFinSignal[shortID]
+		finSignals, ok := v.chanAnnFinSignal[shortID]
 		if ok {
-			close(finSignal)
+			if allow {
+				close(finSignals.allow)
+			} else {
+				close(finSignals.deny)
+			}
 			delete(v.chanAnnFinSignal, shortID)
 		}
 	case *lnwire.ChannelAnnouncement:
-		finSignal, ok := v.chanAnnFinSignal[msg.ShortChannelID]
+		finSignals, ok := v.chanAnnFinSignal[msg.ShortChannelID]
 		if ok {
-			close(finSignal)
+			if allow {
+				close(finSignals.allow)
+			} else {
+				close(finSignals.deny)
+			}
 			delete(v.chanAnnFinSignal, msg.ShortChannelID)
 		}
 

routing/validation_barrier_test.go

@@ -12,6 +12,8 @@ import (
 // TestValidationBarrierSemaphore checks basic properties of the validation
 // barrier's semaphore wrt. enqueuing/dequeuing.
 func TestValidationBarrierSemaphore(t *testing.T) {
+	t.Parallel()
+
 	const (
 		numTasks        = 8
 		numPendingTasks = 8
@@ -59,6 +61,8 @@ func TestValidationBarrierSemaphore(t *testing.T) {
 // TestValidationBarrierQuit checks that pending validation tasks will return an
 // error from WaitForDependants if the barrier's quit signal is canceled.
 func TestValidationBarrierQuit(t *testing.T) {
+	t.Parallel()
+
 	const (
 		numTasks = 8
 		timeout  = 50 * time.Millisecond
@@ -113,9 +117,14 @@ func TestValidationBarrierQuit(t *testing.T) {
 	// with the correct error.
 	for i := 0; i < numTasks; i++ {
 		switch {
-		// First half, signal completion and task semaphore
+		// Signal completion for the first half of tasks, but only allow
+		// dependents to be processed as well for the second quarter.
+		case i < numTasks/4:
+			barrier.SignalDependants(anns[i], false)
+			barrier.CompleteJob()
+
 		case i < numTasks/2:
-			barrier.SignalDependants(anns[i])
+			barrier.SignalDependants(anns[i], true)
 			barrier.CompleteJob()
 
 		// At midpoint, quit the validation barrier.
@@ -132,7 +141,10 @@ func TestValidationBarrierQuit(t *testing.T) {
 
 		switch {
 		// First half should return without failure.
-		case i < numTasks/2 && err != nil:
+		case i < numTasks/4 && err != routing.ErrParentValidationFailed:
+			t.Fatalf("unexpected failure while waiting: %v", err)
+
+		case i >= numTasks/4 && i < numTasks/2 && err != nil:
 			t.Fatalf("unexpected failure while waiting: %v", err)
 
 		// Last half should return the shutdown error.