From f71cc951fd3f22c429687e1d28b7f51cab35c47b Mon Sep 17 00:00:00 2001
From: Conner Fromknecht <conner@lightning.engineering>
Date: Fri, 10 Apr 2020 16:01:21 -0700
Subject: [PATCH] channeldb/channel: fix HasChanStatus for ChanStatusDefault

This commit resovles a lingering issue w/in the codebase wrt how the
ChannelStatus flags are defined. Currently ChannelStatus is improperly
used to define a bit field and the individual flags themselves. As a
result, HasChanStatus accepts queries on particular status (combinations
of flags) and individual flags themselves.

This is an issue because the way HasChanStatus computes whether the
channel has a particular status assumes the provided inputs are all
flags (or at least combinations of flags).

However, ChanStatusDefault is simply the absence of any other flag.
Hence, HasChanStatus will always return true when querying for
ChanStatusDefault because status&0 == 0 is always true.

Longer term we should should consider splitting these definitions into
flags and particular states, and change the way construct or operate on
them, but for now I've just special-cased this one value. Fortunately,
we don't query HasChannelStatus w/ ChanStatusDefault anywhere in the
codebase so we dodge a bullet here, but it'd be nice to have some
greater assurances moving forward.
---
 channeldb/channel.go      |  6 ++++
 channeldb/channel_test.go | 59 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/channeldb/channel.go b/channeldb/channel.go
index 21468b7f3..e2e6658dd 100644
--- a/channeldb/channel.go
+++ b/channeldb/channel.go
@@ -711,6 +711,12 @@ func (c *OpenChannel) HasChanStatus(status ChannelStatus) bool {
 }
 
 func (c *OpenChannel) hasChanStatus(status ChannelStatus) bool {
+	// Special case ChanStatusDefualt since it isn't actually flag, but a
+	// particular combination (or lack-there-of) of flags.
+	if status == ChanStatusDefault {
+		return c.chanStatus == ChanStatusDefault
+	}
+
 	return c.chanStatus&status == status
 }
 
diff --git a/channeldb/channel_test.go b/channeldb/channel_test.go
index eb068485e..6b1e0ab8b 100644
--- a/channeldb/channel_test.go
+++ b/channeldb/channel_test.go
@@ -1581,3 +1581,62 @@ func TestBalanceAtHeight(t *testing.T) {
 		})
 	}
 }
+
+// TestHasChanStatus asserts the behavior of HasChanStatus by checking the
+// behavior of various status flags in addition to the special case of
+// ChanStatusDefault which is treated like a flag in the code base even though
+// it isn't.
+func TestHasChanStatus(t *testing.T) {
+	tests := []struct {
+		name   string
+		status ChannelStatus
+		expHas map[ChannelStatus]bool
+	}{
+		{
+			name:   "default",
+			status: ChanStatusDefault,
+			expHas: map[ChannelStatus]bool{
+				ChanStatusDefault: true,
+				ChanStatusBorked:  false,
+			},
+		},
+		{
+			name:   "single flag",
+			status: ChanStatusBorked,
+			expHas: map[ChannelStatus]bool{
+				ChanStatusDefault: false,
+				ChanStatusBorked:  true,
+			},
+		},
+		{
+			name:   "multiple flags",
+			status: ChanStatusBorked | ChanStatusLocalDataLoss,
+			expHas: map[ChannelStatus]bool{
+				ChanStatusDefault:       false,
+				ChanStatusBorked:        true,
+				ChanStatusLocalDataLoss: true,
+			},
+		},
+	}
+
+	for _, test := range tests {
+		test := test
+
+		t.Run(test.name, func(t *testing.T) {
+			c := &OpenChannel{
+				chanStatus: test.status,
+			}
+
+			for status, expHas := range test.expHas {
+				has := c.HasChanStatus(status)
+				if has == expHas {
+					continue
+				}
+
+				t.Fatalf("expected chan status to "+
+					"have %s? %t, got: %t",
+					status, expHas, has)
+			}
+		})
+	}
+}