From 1ebc3b82c8fa316e6c1f975ab0524eeeb4256148 Mon Sep 17 00:00:00 2001
From: yyforyongyu <yy2452@columbia.edu>
Date: Thu, 23 Sep 2021 15:31:53 +0800
Subject: [PATCH 01/10] tor: rename dial to dialProxy

This commit renames dial to be dialProxy to make the connections
clearer. It also cleans the column width and provides more verbose error
messages.
---
 tor/add_onion.go  | 31 +++++++++++++++++--------------
 tor/controller.go |  6 +++---
 tor/tor.go        | 18 ++++++++++--------
 3 files changed, 30 insertions(+), 25 deletions(-)

diff --git a/tor/add_onion.go b/tor/add_onion.go
index aca433ad4..17b83dba0 100644
--- a/tor/add_onion.go
+++ b/tor/add_onion.go
@@ -52,7 +52,9 @@ var _ OnionStore = (*OnionFile)(nil)
 
 // NewOnionFile creates a file-based implementation of the OnionStore interface
 // to store an onion service's private key.
-func NewOnionFile(privateKeyPath string, privateKeyPerm os.FileMode) *OnionFile {
+func NewOnionFile(privateKeyPath string,
+	privateKeyPerm os.FileMode) *OnionFile {
+
 	return &OnionFile{
 		privateKeyPath: privateKeyPath,
 		privateKeyPerm: privateKeyPerm,
@@ -64,8 +66,8 @@ func (f *OnionFile) StorePrivateKey(_ OnionType, privateKey []byte) error {
 	return ioutil.WriteFile(f.privateKeyPath, privateKey, f.privateKeyPerm)
 }
 
-// PrivateKey retrieves the private key from its expected path. If the file does
-// not exist, then ErrNoPrivateKey is returned.
+// PrivateKey retrieves the private key from its expected path. If the file
+// does not exist, then ErrNoPrivateKey is returned.
 func (f *OnionFile) PrivateKey(_ OnionType) ([]byte, error) {
 	if _, err := os.Stat(f.privateKeyPath); os.IsNotExist(err) {
 		return nil, ErrNoPrivateKey
@@ -78,8 +80,8 @@ func (f *OnionFile) DeletePrivateKey(_ OnionType) error {
 	return os.Remove(f.privateKeyPath)
 }
 
-// AddOnionConfig houses all of the required parameters in order to successfully
-// create a new onion service or restore an existing one.
+// AddOnionConfig houses all of the required parameters in order to
+// successfully create a new onion service or restore an existing one.
 type AddOnionConfig struct {
 	// Type denotes the type of the onion service that should be created.
 	Type OnionType
@@ -87,9 +89,9 @@ type AddOnionConfig struct {
 	// VirtualPort is the externally reachable port of the onion address.
 	VirtualPort int
 
-	// TargetPorts is the set of ports that the service will be listening on
-	// locally. The Tor server will use choose a random port from this set
-	// to forward the traffic from the virtual port.
+	// TargetPorts is the set of ports that the service will be listening
+	// on locally. The Tor server will use choose a random port from this
+	// set to forward the traffic from the virtual port.
 	//
 	// NOTE: If nil/empty, the virtual port will be used as the only target
 	// port.
@@ -116,10 +118,11 @@ func (c *Controller) AddOnion(cfg AddOnionConfig) (*OnionAddr, error) {
 		}
 	}
 
-	// We'll start off by checking if the store contains an existing private
-	// key. If it does not, then we should request the server to create a
-	// new onion service and return its private key. Otherwise, we'll
-	// request the server to recreate the onion server from our private key.
+	// We'll start off by checking if the store contains an existing
+	// private key. If it does not, then we should request the server to
+	// create a new onion service and return its private key. Otherwise,
+	// we'll request the server to recreate the onion server from our
+	// private key.
 	var keyParam string
 	switch cfg.Type {
 	case V2:
@@ -155,8 +158,8 @@ func (c *Controller) AddOnion(cfg AddOnionConfig) (*OnionAddr, error) {
 			portParam += fmt.Sprintf("Port=%d,%d ", cfg.VirtualPort,
 				targetPort)
 		} else {
-			portParam += fmt.Sprintf("Port=%d,%s:%d ", cfg.VirtualPort,
-				c.targetIPAddress, targetPort)
+			portParam += fmt.Sprintf("Port=%d,%s:%d ",
+				cfg.VirtualPort, c.targetIPAddress, targetPort)
 		}
 	}
 
diff --git a/tor/controller.go b/tor/controller.go
index f828b6397..5e24ae20d 100644
--- a/tor/controller.go
+++ b/tor/controller.go
@@ -114,9 +114,9 @@ func NewController(controlAddr string, targetIPAddress string,
 	}
 }
 
-// Start establishes and authenticates the connection between the controller and
-// a Tor server. Once done, the controller will be able to send commands and
-// expect responses.
+// Start establishes and authenticates the connection between the controller
+// and a Tor server. Once done, the controller will be able to send commands
+// and expect responses.
 func (c *Controller) Start() error {
 	if !atomic.CompareAndSwapInt32(&c.started, 0, 1) {
 		return nil
diff --git a/tor/tor.go b/tor/tor.go
index c373a250e..f2cb472e8 100644
--- a/tor/tor.go
+++ b/tor/tor.go
@@ -66,14 +66,15 @@ func (c *proxyConn) RemoteAddr() net.Addr {
 // around net.Conn in order to expose the actual remote address we're dialing,
 // rather than the proxy's address.
 func Dial(address, socksAddr string, streamIsolation bool,
-	skipProxyForClearNetTargets bool, timeout time.Duration) (net.Conn, error) {
+	skipProxyForClearNetTargets bool,
+	timeout time.Duration) (net.Conn, error) {
 
-	conn, err := dial(
+	conn, err := dialProxy(
 		address, socksAddr, streamIsolation,
 		skipProxyForClearNetTargets, timeout,
 	)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("dial proxy failed: %w", err)
 	}
 
 	// Now that the connection is established, we'll create our internal
@@ -90,7 +91,7 @@ func Dial(address, socksAddr string, streamIsolation bool,
 	}, nil
 }
 
-// dial establishes a connection to the address via the provided TOR SOCKS
+// dialProxy establishes a connection to the address via the provided TOR SOCKS
 // proxy. Only TCP traffic may be routed via Tor.
 //
 // streamIsolation determines if we should force stream isolation for this new
@@ -100,8 +101,9 @@ func Dial(address, socksAddr string, streamIsolation bool,
 // skipProxyForClearNetTargets argument allows the dialer to directly connect
 // to the provided address if it does not represent an union service, skipping
 // the SOCKS proxy.
-func dial(address, socksAddr string, streamIsolation bool,
-	skipProxyForClearNetTargets bool, timeout time.Duration) (net.Conn, error) {
+func dialProxy(address, socksAddr string, streamIsolation bool,
+	skipProxyForClearNetTargets bool,
+	timeout time.Duration) (net.Conn, error) {
 
 	// If we were requested to force stream isolation for this connection,
 	// we'll populate the authentication credentials with random data as
@@ -136,7 +138,7 @@ func dial(address, socksAddr string, streamIsolation bool,
 	// Establish the connection through Tor's SOCKS proxy.
 	dialer, err := proxy.SOCKS5("tcp", socksAddr, auth, clearDialer)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("establish sock proxy: %w", err)
 	}
 
 	return dialer.Dial("tcp", address)
@@ -163,7 +165,7 @@ func LookupSRV(service, proto, name, socksAddr,
 	timeout time.Duration) (string, []*net.SRV, error) {
 
 	// Connect to the DNS server we'll be using to query SRV records.
-	conn, err := dial(
+	conn, err := dialProxy(
 		dnsServer, socksAddr, streamIsolation,
 		skipProxyForClearNetTargets, timeout,
 	)

From a101a53eb40fe1806d81cb5093a17b743225f150 Mon Sep 17 00:00:00 2001
From: yyforyongyu <yy2452@columbia.edu>
Date: Thu, 23 Sep 2021 17:03:28 +0800
Subject: [PATCH 02/10] tor: patch unit tests for tor controller

---
 tor/add_onion.go      |  62 ++++++++++++----
 tor/add_onion_test.go | 161 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 209 insertions(+), 14 deletions(-)

diff --git a/tor/add_onion.go b/tor/add_onion.go
index 17b83dba0..602a2dab3 100644
--- a/tor/add_onion.go
+++ b/tor/add_onion.go
@@ -105,19 +105,9 @@ type AddOnionConfig struct {
 	Store OnionStore
 }
 
-// AddOnion creates an onion service and returns its onion address. Once
-// created, the new onion service will remain active until the connection
-// between the controller and the Tor server is closed.
-func (c *Controller) AddOnion(cfg AddOnionConfig) (*OnionAddr, error) {
-	// Before sending the request to create an onion service to the Tor
-	// server, we'll make sure that it supports V3 onion services if that
-	// was the type requested.
-	if cfg.Type == V3 {
-		if err := supportsV3(c.version); err != nil {
-			return nil, err
-		}
-	}
-
+// prepareKeyparam takes a config and prepares the key param to be used inside
+// ADD_ONION.
+func (c *Controller) prepareKeyparam(cfg AddOnionConfig) (string, error) {
 	// We'll start off by checking if the store contains an existing
 	// private key. If it does not, then we should request the server to
 	// create a new onion service and return its private key. Otherwise,
@@ -125,6 +115,7 @@ func (c *Controller) AddOnion(cfg AddOnionConfig) (*OnionAddr, error) {
 	// private key.
 	var keyParam string
 	switch cfg.Type {
+	// TODO(yy): drop support for v2.
 	case V2:
 		keyParam = "NEW:RSA1024"
 	case V3:
@@ -142,10 +133,22 @@ func (c *Controller) AddOnion(cfg AddOnionConfig) (*OnionAddr, error) {
 			keyParam = string(privateKey)
 
 		default:
-			return nil, err
+			return "", err
 		}
 	}
 
+	return keyParam, nil
+}
+
+// prepareAddOnion constructs a cmd command string based on the specified
+// config.
+func (c *Controller) prepareAddOnion(cfg AddOnionConfig) (string, error) {
+	// Create the keyParam.
+	keyParam, err := c.prepareKeyparam(cfg)
+	if err != nil {
+		return "", err
+	}
+
 	// Now, we'll create a mapping from the virtual port to each target
 	// port. If no target ports were specified, we'll use the virtual port
 	// to provide a one-to-one mapping.
@@ -174,6 +177,37 @@ func (c *Controller) AddOnion(cfg AddOnionConfig) (*OnionAddr, error) {
 	// Send the command to create the onion service to the Tor server and
 	// await its response.
 	cmd := fmt.Sprintf("ADD_ONION %s %s", keyParam, portParam)
+
+	return cmd, nil
+}
+
+// AddOnion creates an ephemeral onion service and returns its onion address.
+// Once created, the new onion service will remain active until either,
+//   - the onion service is removed via `DEL_ONION`.
+//   - the Tor daemon terminates.
+//   - the controller connection that originated the `ADD_ONION` is closed.
+// Each connection can only see its own ephemeral services. If a service needs
+// to survive beyond current controller connection, use the "Detach" flag when
+// creating new service via `ADD_ONION`.
+func (c *Controller) AddOnion(cfg AddOnionConfig) (*OnionAddr, error) {
+	// Before sending the request to create an onion service to the Tor
+	// server, we'll make sure that it supports V3 onion services if that
+	// was the type requested.
+	// TODO(yy): drop support for v2.
+	if cfg.Type == V3 {
+		if err := supportsV3(c.version); err != nil {
+			return nil, err
+		}
+	}
+
+	// Construct the cmd command.
+	cmd, err := c.prepareAddOnion(cfg)
+	if err != nil {
+		return nil, err
+	}
+
+	// Send the command to create the onion service to the Tor server and
+	// await its response.
 	_, reply, err := c.sendCommand(cmd)
 	if err != nil {
 		return nil, err
diff --git a/tor/add_onion_test.go b/tor/add_onion_test.go
index 9cd255d32..e7356cb07 100644
--- a/tor/add_onion_test.go
+++ b/tor/add_onion_test.go
@@ -2,9 +2,13 @@ package tor
 
 import (
 	"bytes"
+	"errors"
 	"io/ioutil"
 	"path/filepath"
 	"testing"
+
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 )
 
 // TestOnionFile tests that the OnionFile implementation of the OnionStore
@@ -49,3 +53,160 @@ func TestOnionFile(t *testing.T) {
 		t.Fatal("found deleted private key")
 	}
 }
+
+// TestPrepareKeyParam checks that the key param is created as expected.
+func TestPrepareKeyParam(t *testing.T) {
+	testKey := []byte("hide_me_plz")
+	dummyErr := errors.New("dummy")
+
+	// Create a dummy controller.
+	controller := NewController("", "", "")
+
+	// Test that a V3 keyParam is used.
+	cfg := AddOnionConfig{Type: V3}
+	keyParam, err := controller.prepareKeyparam(cfg)
+
+	require.Equal(t, "NEW:ED25519-V3", keyParam)
+	require.NoError(t, err)
+
+	// Create a mock store which returns the test private key.
+	store := &mockStore{}
+	store.On("PrivateKey", cfg.Type).Return(testKey, nil)
+
+	// Check that the test private is returned.
+	cfg = AddOnionConfig{Type: V3, Store: store}
+	keyParam, err = controller.prepareKeyparam(cfg)
+
+	require.Equal(t, string(testKey), keyParam)
+	require.NoError(t, err)
+	store.AssertExpectations(t)
+
+	// Create a mock store which returns ErrNoPrivateKey.
+	store = &mockStore{}
+	store.On("PrivateKey", cfg.Type).Return(nil, ErrNoPrivateKey)
+
+	// Check that the V3 keyParam is returned.
+	cfg = AddOnionConfig{Type: V3, Store: store}
+	keyParam, err = controller.prepareKeyparam(cfg)
+
+	require.Equal(t, "NEW:ED25519-V3", keyParam)
+	require.NoError(t, err)
+	store.AssertExpectations(t)
+
+	// Create a mock store which returns an dummy error.
+	store = &mockStore{}
+	store.On("PrivateKey", cfg.Type).Return(nil, dummyErr)
+
+	// Check that an error is returned.
+	cfg = AddOnionConfig{Type: V3, Store: store}
+	keyParam, err = controller.prepareKeyparam(cfg)
+
+	require.Empty(t, keyParam)
+	require.ErrorIs(t, dummyErr, err)
+	store.AssertExpectations(t)
+}
+
+// TestPrepareAddOnion checks that the cmd used to add onion service is created
+// as expected.
+func TestPrepareAddOnion(t *testing.T) {
+	t.Parallel()
+
+	// Create a mock store.
+	store := &mockStore{}
+	testKey := []byte("hide_me_plz")
+
+	testCases := []struct {
+		name            string
+		targetIPAddress string
+		cfg             AddOnionConfig
+		expectedCmd     string
+		expectedErr     error
+	}{
+		{
+			name:            "empty target IP and ports",
+			targetIPAddress: "",
+			cfg:             AddOnionConfig{VirtualPort: 9735},
+			expectedCmd:     "ADD_ONION NEW:RSA1024 Port=9735,9735 ",
+			expectedErr:     nil,
+		},
+		{
+			name:            "specified target IP and empty ports",
+			targetIPAddress: "127.0.0.1",
+			cfg:             AddOnionConfig{VirtualPort: 9735},
+			expectedCmd: "ADD_ONION NEW:RSA1024 " +
+				"Port=9735,127.0.0.1:9735 ",
+			expectedErr: nil,
+		},
+		{
+			name:            "specified target IP and ports",
+			targetIPAddress: "127.0.0.1",
+			cfg: AddOnionConfig{
+				VirtualPort: 9735,
+				TargetPorts: []int{18000, 18001},
+			},
+			expectedCmd: "ADD_ONION NEW:RSA1024 " +
+				"Port=9735,127.0.0.1:18000 " +
+				"Port=9735,127.0.0.1:18001 ",
+			expectedErr: nil,
+		},
+		{
+			name:            "specified private key from store",
+			targetIPAddress: "",
+			cfg: AddOnionConfig{
+				VirtualPort: 9735,
+				Store:       store,
+			},
+			expectedCmd: "ADD_ONION hide_me_plz " +
+				"Port=9735,9735 ",
+			expectedErr: nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+
+		if tc.cfg.Store != nil {
+			store.On("PrivateKey", tc.cfg.Type).Return(
+				testKey, tc.expectedErr,
+			)
+		}
+
+		controller := NewController("", tc.targetIPAddress, "")
+		t.Run(tc.name, func(t *testing.T) {
+			cmd, err := controller.prepareAddOnion(tc.cfg)
+			require.Equal(t, tc.expectedErr, err)
+			require.Equal(t, tc.expectedCmd, cmd)
+
+			// Check that the mocker is satisfied.
+			store.AssertExpectations(t)
+		})
+	}
+
+}
+
+// mockStore implements a mock of the interface OnionStore.
+type mockStore struct {
+	mock.Mock
+}
+
+// A compile-time constraint to ensure mockStore satisfies the OnionStore
+// interface.
+var _ OnionStore = (*mockStore)(nil)
+
+func (m *mockStore) StorePrivateKey(ot OnionType, key []byte) error {
+	args := m.Called(ot, key)
+	return args.Error(0)
+}
+
+func (m *mockStore) PrivateKey(ot OnionType) ([]byte, error) {
+	args := m.Called(ot)
+	if args.Get(0) == nil {
+		return nil, args.Error(1)
+	}
+	return args.Get(0).([]byte), args.Error(1)
+}
+
+func (m *mockStore) DeletePrivateKey(ot OnionType) error {
+	args := m.Called(ot)
+	return args.Error(0)
+}

From 0b80d4feaafaef76e7973df2fd1a4bb4759fb9ec Mon Sep 17 00:00:00 2001
From: yyforyongyu <yy2452@columbia.edu>
Date: Thu, 23 Sep 2021 18:34:51 +0800
Subject: [PATCH 03/10] tor: add logging to tor controller

---
 log.go            |  2 ++
 tor/controller.go |  9 +++++++++
 tor/log.go        | 26 ++++++++++++++++++++++++++
 3 files changed, 37 insertions(+)
 create mode 100644 tor/log.go

diff --git a/log.go b/log.go
index 61d3b3ca3..68b592bb4 100644
--- a/log.go
+++ b/log.go
@@ -40,6 +40,7 @@ import (
 	"github.com/lightningnetwork/lnd/rpcperms"
 	"github.com/lightningnetwork/lnd/signal"
 	"github.com/lightningnetwork/lnd/sweep"
+	"github.com/lightningnetwork/lnd/tor"
 	"github.com/lightningnetwork/lnd/watchtower"
 	"github.com/lightningnetwork/lnd/watchtower/wtclient"
 )
@@ -161,6 +162,7 @@ func SetupLoggers(root *build.RotatingLogWriter, interceptor signal.Interceptor)
 	AddSubLogger(root, funding.Subsystem, interceptor, funding.UseLogger)
 	AddSubLogger(root, cluster.Subsystem, interceptor, cluster.UseLogger)
 	AddSubLogger(root, rpcperms.Subsystem, interceptor, rpcperms.UseLogger)
+	AddSubLogger(root, tor.Subsystem, interceptor, tor.UseLogger)
 }
 
 // AddSubLogger is a helper method to conveniently create and register the
diff --git a/tor/controller.go b/tor/controller.go
index 5e24ae20d..9ec3f15a8 100644
--- a/tor/controller.go
+++ b/tor/controller.go
@@ -122,6 +122,8 @@ func (c *Controller) Start() error {
 		return nil
 	}
 
+	log.Info("Starting tor controller")
+
 	conn, err := textproto.Dial("tcp", c.controlAddr)
 	if err != nil {
 		return fmt.Errorf("unable to connect to Tor server: %v", err)
@@ -138,6 +140,8 @@ func (c *Controller) Stop() error {
 		return nil
 	}
 
+	log.Info("Stopping tor controller")
+
 	return c.conn.Close()
 }
 
@@ -151,7 +155,12 @@ func (c *Controller) sendCommand(command string) (int, string, error) {
 	// We'll use ReadResponse as it has built-in support for multi-line
 	// text protocol responses.
 	code, reply, err := c.conn.Reader.ReadResponse(success)
+	log.Tracef("sendCommand: %v got <code:%v>, <reply:%v>",
+		command, code, reply)
+
 	if err != nil {
+		log.Debugf("sendCommand:%s got err:%v, reply:%v",
+			command, err, reply)
 		return code, reply, err
 	}
 
diff --git a/tor/log.go b/tor/log.go
new file mode 100644
index 000000000..9fe5b8aab
--- /dev/null
+++ b/tor/log.go
@@ -0,0 +1,26 @@
+package tor
+
+import (
+	"github.com/btcsuite/btclog"
+)
+
+// Subsystem defines the logging code for this subsystem.
+const Subsystem = "TORC" // TORC as in Tor Controller.
+
+// log is a logger that is initialized with no output filters.  This
+// means the package will not perform any logging by default until the caller
+// requests it.
+var log = btclog.Disabled
+
+// DisableLog disables all library log output.  Logging output is disabled
+// by default until UseLogger is called.
+func DisableLog() {
+	UseLogger(btclog.Disabled)
+}
+
+// UseLogger uses a specified Logger to output package logging info.
+// This should be used in preference to SetLogWriter if the caller is also
+// using btclog.
+func UseLogger(logger btclog.Logger) {
+	log = logger
+}

From 90e6c70ea0d96123bc045fa332ad7449aaa6d36d Mon Sep 17 00:00:00 2001
From: yyforyongyu <yy2452@columbia.edu>
Date: Thu, 23 Sep 2021 18:37:30 +0800
Subject: [PATCH 04/10] tor: remove Onion Service upon shutdown

This commit adds a new method to call DEL_ONION when lnd is shutting
down. Tor controller will now be aware of the active serviceID and
removes the service upon shutdown.
---
 tor/{add_onion.go => cmd_onion.go}           | 47 ++++++++++++++++++++
 tor/{add_onion_test.go => cmd_onion_test.go} |  0
 tor/controller.go                            | 20 +++++++++
 3 files changed, 67 insertions(+)
 rename tor/{add_onion.go => cmd_onion.go} (83%)
 rename tor/{add_onion_test.go => cmd_onion_test.go} (100%)

diff --git a/tor/add_onion.go b/tor/cmd_onion.go
similarity index 83%
rename from tor/add_onion.go
rename to tor/cmd_onion.go
index 602a2dab3..1e952428e 100644
--- a/tor/add_onion.go
+++ b/tor/cmd_onion.go
@@ -244,6 +244,9 @@ func (c *Controller) AddOnion(cfg AddOnionConfig) (*OnionAddr, error) {
 		}
 	}
 
+	c.activeServiceID = serviceID
+	log.Debugf("serviceID:%s added to tor controller", serviceID)
+
 	// Finally, we'll return the onion address composed of the service ID,
 	// along with the onion suffix, and the port this onion service can be
 	// reached at externally.
@@ -252,3 +255,47 @@ func (c *Controller) AddOnion(cfg AddOnionConfig) (*OnionAddr, error) {
 		Port:         cfg.VirtualPort,
 	}, nil
 }
+
+// DelOnion tells the Tor daemon to remove an onion service, which satisfies
+// either,
+//   - the onion service was created on the same control connection as the
+//     "DEL_ONION" command.
+//   - the onion service was created using the "Detach" flag.
+func (c *Controller) DelOnion(serviceID string) error {
+	log.Debugf("removing serviceID:%s from tor controller", serviceID)
+
+	cmd := fmt.Sprintf("DEL_ONION %s", serviceID)
+
+	// Send the command to create the onion service to the Tor server and
+	// await its response.
+	code, _, err := c.sendCommand(cmd)
+
+	// Tor replies with "250 OK" on success, or a 512 if there are an
+	// invalid number of arguments, or a 552 if it doesn't recognize the
+	// ServiceID.
+	switch code {
+	// Replied 250 OK.
+	case success:
+		return nil
+
+	// Replied 512 for invalid arguments. This is most likely that the
+	// serviceID is not set(empty string).
+	case invalidNumOfArguments:
+		return fmt.Errorf("invalid arguments: %w", err)
+
+	// Replied 552, which means either,
+	//   - the serviceID is invalid.
+	//   - the onion service is not owned by the current control connection
+	//     and,
+	//   - the onion service is not a detached service.
+	// In either case, we will ignore the error and log a warning as there
+	// not much we can do from the controller side.
+	case serviceIDNotRecognized:
+		log.Warnf("removing serviceID:%v not found", serviceID)
+		return nil
+
+	default:
+		return fmt.Errorf("undefined response code: %v, err: %w",
+			code, err)
+	}
+}
diff --git a/tor/add_onion_test.go b/tor/cmd_onion_test.go
similarity index 100%
rename from tor/add_onion_test.go
rename to tor/cmd_onion_test.go
diff --git a/tor/controller.go b/tor/controller.go
index 9ec3f15a8..f2120a92c 100644
--- a/tor/controller.go
+++ b/tor/controller.go
@@ -20,6 +20,14 @@ const (
 	// request.
 	success = 250
 
+	// invalidNumOfArguments is the Tor Control response code representing
+	// there being an invalid number of arguments.
+	invalidNumOfArguments = 512
+
+	// serviceIDNotRecognized is the Tor Control response code representing
+	// the specified ServiceID is not recognized.
+	serviceIDNotRecognized = 552
+
 	// nonceLen is the length of a nonce generated by either the controller
 	// or the Tor server
 	nonceLen = 32
@@ -100,6 +108,9 @@ type Controller struct {
 	// to connect to the LND node.  This is required when the Tor server
 	// runs on another host, otherwise the service will not be reachable.
 	targetIPAddress string
+
+	// activeServiceID is the Onion ServiceID created by ADD_ONION.
+	activeServiceID string
 }
 
 // NewController returns a new Tor controller that will be able to interact with
@@ -142,6 +153,15 @@ func (c *Controller) Stop() error {
 
 	log.Info("Stopping tor controller")
 
+	// Remove the onion service.
+	if err := c.DelOnion(c.activeServiceID); err != nil {
+		log.Errorf("DEL_ONION got error: %v", err)
+		return err
+	}
+
+	// Reset service ID.
+	c.activeServiceID = ""
+
 	return c.conn.Close()
 }
 

From 7daffcbba8d23e278545830d13bf45c0f61cfc39 Mon Sep 17 00:00:00 2001
From: yyforyongyu <yy2452@columbia.edu>
Date: Fri, 24 Sep 2021 18:31:39 +0800
Subject: [PATCH 05/10] tor: add a new response reader for tor controller

This commit adds a new response reader which replaces the old
textproto.Reader.ReadResponse. The older reader cannot handle the case
when the reply from Tor server contains a data reply line, which uses
the symbol "+" to signal such a case.
---
 tor/controller.go      | 132 ++++++++++++++++++++++++--
 tor/controller_test.go | 204 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 329 insertions(+), 7 deletions(-)

diff --git a/tor/controller.go b/tor/controller.go
index f2120a92c..c401cb0bf 100644
--- a/tor/controller.go
+++ b/tor/controller.go
@@ -65,6 +65,10 @@ var (
 	// message from the controller.
 	controllerKey = []byte("Tor safe cookie authentication " +
 		"controller-to-server hash")
+
+	// errCodeNotMatch is used when an expected response code is not
+	// returned.
+	errCodeNotMatch = errors.New("unexpected code")
 )
 
 // Controller is an implementation of the Tor Control protocol. This is used in
@@ -168,16 +172,17 @@ func (c *Controller) Stop() error {
 // sendCommand sends a command to the Tor server and returns its response, as a
 // single space-delimited string, and code.
 func (c *Controller) sendCommand(command string) (int, string, error) {
-	if err := c.conn.Writer.PrintfLine(command); err != nil {
+	id, err := c.conn.Cmd(command)
+	if err != nil {
 		return 0, "", err
 	}
 
-	// We'll use ReadResponse as it has built-in support for multi-line
-	// text protocol responses.
-	code, reply, err := c.conn.Reader.ReadResponse(success)
-	log.Tracef("sendCommand: %v got <code:%v>, <reply:%v>",
-		command, code, reply)
+	// Make sure our reader only process the response returned from the
+	// above command.
+	c.conn.StartResponse(id)
+	defer c.conn.EndResponse(id)
 
+	code, reply, err := c.readResponse(success)
 	if err != nil {
 		log.Debugf("sendCommand:%s got err:%v, reply:%v",
 			command, err, reply)
@@ -187,6 +192,119 @@ func (c *Controller) sendCommand(command string) (int, string, error) {
 	return code, reply, nil
 }
 
+// readResponse reads the replies from Tor to the controller. The reply has the
+// following format,
+//
+//   Reply = SyncReply / AsyncReply
+//   SyncReply = *(MidReplyLine / DataReplyLine) EndReplyLine
+//   AsyncReply = *(MidReplyLine / DataReplyLine) EndReplyLine
+//
+//   MidReplyLine = StatusCode "-" ReplyLine
+//   DataReplyLine = StatusCode "+" ReplyLine CmdData
+//   EndReplyLine = StatusCode SP ReplyLine
+//   ReplyLine = [ReplyText] CRLF
+//   ReplyText = XXXX
+//   StatusCode = 3DIGIT
+//
+// Unless specified otherwise, multiple lines in a single reply from Tor daemon
+// to the controller are guaranteed to share the same status code. Read more on
+// this topic:
+//   https://gitweb.torproject.org/torspec.git/tree/control-spec.txt#n158
+//
+// NOTE: this code is influenced by https://github.com/Yawning/bulb.
+func (c *Controller) readResponse(expected int) (int, string, error) {
+	// Clean the buffer inside the conn. This is needed when we encountered
+	// an error while reading the response, the remaining lines need to be
+	// cleaned before next read.
+	defer func() {
+		if _, err := c.conn.R.Discard(c.conn.R.Buffered()); err != nil {
+			log.Errorf("clean read buffer failed: %v", err)
+		}
+	}()
+
+	reply, code := "", 0
+	hasMoreLines := true
+
+	for hasMoreLines {
+		line, err := c.conn.Reader.ReadLine()
+		if err != nil {
+			return 0, reply, err
+		}
+		log.Tracef("Reading line: %v", line)
+
+		// Line being shortter than 4 is not allowed.
+		if len(line) < 4 {
+			err = textproto.ProtocolError("short line: " + line)
+			return 0, reply, err
+		}
+
+		// Parse the status code.
+		code, err = strconv.Atoi(line[0:3])
+		if err != nil {
+			return code, reply, err
+		}
+
+		switch line[3] {
+		// EndReplyLine = StatusCode SP ReplyLine.
+		// Example: 250 OK
+		// This is the end of the response, so we mark hasMoreLines to
+		// be false to exit the loop.
+		case ' ':
+			reply += line[4:]
+			hasMoreLines = false
+
+		// MidReplyLine = StatusCode "-" ReplyLine.
+		// Example: 250-version=...
+		// This is a continued response, so we keep reading the next
+		// line.
+		case '-':
+			reply += line[4:]
+
+		// DataReplyLine = StatusCode "+" ReplyLine CmdData.
+		// Example: 250+config-text=
+		//	    line1
+		//	    line2
+		//          more lines...
+		//          .
+		// This is a data response, meaning the following multiple
+		// lines are the actual data, and a dot(.) in the end means the
+		// end of the data response. The response will be formatted as,
+		// 	key=line1,line2,...
+		// The above example will then be,
+		// 	config-text=line1,line2,...
+		case '+':
+			// Add the key(config-text=)
+			reply += line[4:]
+
+			// Add the values.
+			resp, err := c.conn.Reader.ReadDotLines()
+			if err != nil {
+				return code, reply, err
+			}
+			reply += strings.Join(resp, ",")
+
+		// Invalid line separator found.
+		default:
+			err = textproto.ProtocolError("invalid line: " + line)
+			return code, reply, err
+		}
+
+		// We check the code here so that the error message is parsed
+		// from the line.
+		if code != expected {
+			return code, reply, errCodeNotMatch
+		}
+
+		// Separate each line using "\n".
+		if hasMoreLines {
+			reply += "\n"
+		}
+	}
+
+	log.Tracef("Parsed reply: %v", reply)
+	return code, reply, nil
+}
+
 // parseTorReply parses the reply from the Tor server after receiving a command
 // from a controller. This will parse the relevant reply parameters into a map
 // of keys and values.
@@ -225,6 +343,8 @@ func (c *Controller) authenticate() error {
 		return err
 	}
 
+	log.Debugf("received protocol info: %v", protocolInfo)
+
 	// With the version retrieved, we'll cache it now in case it needs to be
 	// used later on.
 	c.version = protocolInfo.version()
diff --git a/tor/controller_test.go b/tor/controller_test.go
index b040a4f9d..00926a3d8 100644
--- a/tor/controller_test.go
+++ b/tor/controller_test.go
@@ -1,6 +1,12 @@
 package tor
 
-import "testing"
+import (
+	"net"
+	"net/textproto"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
 
 // TestParseTorVersion is a series of tests for different version strings that
 // check the correctness of determining whether they support creating v3 onion
@@ -74,3 +80,199 @@ func TestParseTorVersion(t *testing.T) {
 		}
 	}
 }
+
+// testProxy emulates a Tor daemon and contains the info used for the tor
+// controller to make connections.
+type testProxy struct {
+	// server is the proxy listener.
+	server net.Listener
+
+	// serverConn is the established connection from the server side.
+	serverConn net.Conn
+
+	// serverAddr is the tcp address the proxy is listening on.
+	serverAddr string
+
+	// clientConn is the established connection from the client side.
+	clientConn *textproto.Conn
+}
+
+// cleanUp is used after each test to properly close the ports/connections.
+func (tp *testProxy) cleanUp() {
+	// Don't bother cleanning if there's no a server created.
+	if tp.server == nil {
+		return
+	}
+
+	if err := tp.clientConn.Close(); err != nil {
+		log.Errorf("closing client conn got err: %v", err)
+	}
+	if err := tp.server.Close(); err != nil {
+		log.Errorf("closing proxy server got err: %v", err)
+	}
+}
+
+// createTestProxy creates a proxy server to listen on a random address,
+// creates a server and a client connection, and initializes a testProxy using
+// these params.
+func createTestProxy(t *testing.T) *testProxy {
+	// Set up the proxy to listen on given port.
+	//
+	// NOTE: we use a port 0 here to indicate we want a free port selected
+	// by the system.
+	proxy, err := net.Listen("tcp", ":0")
+	require.NoError(t, err, "failed to create proxy")
+
+	t.Logf("created proxy server to listen on address: %v", proxy.Addr())
+
+	// Accept the connection inside a goroutine.
+	serverChan := make(chan net.Conn, 1)
+	go func(result chan net.Conn) {
+		conn, err := proxy.Accept()
+		require.NoError(t, err, "failed to accept")
+
+		result <- conn
+	}(serverChan)
+
+	// Create the connection using tor controller.
+	client, err := textproto.Dial("tcp", proxy.Addr().String())
+	require.NoError(t, err, "failed to create connection")
+
+	tc := &testProxy{
+		server:     proxy,
+		serverConn: <-serverChan,
+		serverAddr: proxy.Addr().String(),
+		clientConn: client,
+	}
+
+	return tc
+}
+
+// TestReadResponse contructs a series of possible responses returned by Tor
+// and asserts the readResponse can handle them correctly.
+func TestReadResponse(t *testing.T) {
+	// Create mock server and client connection.
+	proxy := createTestProxy(t)
+	defer proxy.cleanUp()
+	server := proxy.serverConn
+
+	// Create a dummy tor controller.
+	c := &Controller{conn: proxy.clientConn}
+
+	testCase := []struct {
+		name       string
+		serverResp string
+
+		// expectedReply is the reply we expect the readResponse to
+		// return.
+		expectedReply string
+
+		// expectedCode is the code we expect the server to return.
+		expectedCode int
+
+		// returnedCode is the code we expect the readResponse to
+		// return.
+		returnedCode int
+
+		// expectedErr is the error we expect the readResponse to
+		// return.
+		expectedErr error
+	}{
+		{
+			// Test a simple response.
+			name:          "succeed on 250",
+			serverResp:    "250 OK\n",
+			expectedReply: "OK",
+			expectedCode:  250,
+			returnedCode:  250,
+			expectedErr:   nil,
+		},
+		{
+			// Test a mid reply(-) response.
+			name: "succeed on mid reply line",
+			serverResp: "250-field=value\n" +
+				"250 OK\n",
+			expectedReply: "field=value\nOK",
+			expectedCode:  250,
+			returnedCode:  250,
+			expectedErr:   nil,
+		},
+		{
+			// Test a data reply(+) response.
+			name: "succeed on data reply line",
+			serverResp: "250+field=\n" +
+				"line1\n" +
+				"line2\n" +
+				".\n" +
+				"250 OK\n",
+			expectedReply: "field=line1,line2\nOK",
+			expectedCode:  250,
+			returnedCode:  250,
+			expectedErr:   nil,
+		},
+		{
+			// Test a mixed reply response.
+			name: "succeed on mixed reply line",
+			serverResp: "250-field=value\n" +
+				"250+field=\n" +
+				"line1\n" +
+				"line2\n" +
+				".\n" +
+				"250 OK\n",
+			expectedReply: "field=value\nfield=line1,line2\nOK",
+			expectedCode:  250,
+			returnedCode:  250,
+			expectedErr:   nil,
+		},
+		{
+			// Test unexpected code.
+			name:          "fail on codes not matched",
+			serverResp:    "250 ERR\n",
+			expectedReply: "ERR",
+			expectedCode:  500,
+			returnedCode:  250,
+			expectedErr:   errCodeNotMatch,
+		},
+		{
+			// Test short response error.
+			name:          "fail on short response",
+			serverResp:    "123\n250 OK\n",
+			expectedReply: "",
+			expectedCode:  250,
+			returnedCode:  0,
+			expectedErr: textproto.ProtocolError(
+				"short line: 123"),
+		},
+		{
+			// Test short response error.
+			name:          "fail on invalid response",
+			serverResp:    "250?OK\n",
+			expectedReply: "",
+			expectedCode:  250,
+			returnedCode:  250,
+			expectedErr: textproto.ProtocolError(
+				"invalid line: 250?OK"),
+		},
+	}
+
+	for _, tc := range testCase {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+
+			// Let the server mocks a given response.
+			_, err := server.Write([]byte(tc.serverResp))
+			require.NoError(t, err, "server failed to write")
+
+			// Read the response and checks all expectations
+			// satisfied.
+			code, reply, err := c.readResponse(tc.expectedCode)
+			require.Equal(t, tc.expectedErr, err)
+			require.Equal(t, tc.returnedCode, code)
+			require.Equal(t, tc.expectedReply, reply)
+
+			// Check that the read buffer is cleaned.
+			require.Zero(t, c.conn.R.Buffered(),
+				"read buffer not empty")
+		})
+	}
+}

From 2800c4364e57240442fac1294ade3c6ff9794351 Mon Sep 17 00:00:00 2001
From: yyforyongyu <yy2452@columbia.edu>
Date: Sat, 25 Sep 2021 17:52:58 +0800
Subject: [PATCH 06/10] tor: add GETINFO method to check liveness of onion
 service

---
 tor/cmd_info.go      | 72 ++++++++++++++++++++++++++++++++++++
 tor/cmd_info_test.go | 87 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 159 insertions(+)
 create mode 100644 tor/cmd_info.go
 create mode 100644 tor/cmd_info_test.go

diff --git a/tor/cmd_info.go b/tor/cmd_info.go
new file mode 100644
index 000000000..ea6452e6c
--- /dev/null
+++ b/tor/cmd_info.go
@@ -0,0 +1,72 @@
+package tor
+
+import (
+	"errors"
+	"fmt"
+)
+
+var (
+	// ErrServiceNotCreated is used when we want to query info on an onion
+	// service while it's not been created yet.
+	ErrServiceNotCreated = errors.New("onion service hasn't been created")
+
+	// ErrServiceIDUnmatch is used when the serviceID the controller has
+	// doesn't match the serviceID the Tor daemon has.
+	ErrServiceIDUnmatch = errors.New("onion serviceIDs not match")
+
+	// ErrNoServiceFound is used when the Tor daemon replies no active
+	// onion services found for the current control connection while we
+	// expect one.
+	ErrNoServiceFound = errors.New("no active service found")
+)
+
+// CheckOnionService checks that the onion service created by the controller
+// is active. It queries the Tor daemon using the endpoint "onions/current" to
+// get the current onion service and checks that service ID matches the
+// activeServiceID.
+func (c *Controller) CheckOnionService() error {
+	// Check that we have a hidden service created.
+	if c.activeServiceID == "" {
+		return ErrServiceNotCreated
+	}
+
+	// Fetch the onion services that live in current control connection.
+	cmd := "GETINFO onions/current"
+	code, reply, err := c.sendCommand(cmd)
+
+	// Exit early if we got an error or Tor daemon didn't respond success.
+	// TODO(yy): unify the usage of err and code so we could rely on a
+	// single source to change our state.
+	if err != nil || code != success {
+		log.Debugf("query service:%v got err:%v, reply:%v",
+			c.activeServiceID, err, reply)
+
+		return fmt.Errorf("%w: %v", err, reply)
+	}
+
+	// Parse the reply, which should have the following format,
+	//      onions/current=serviceID
+	// After parsing, we get a map as,
+	// 	[onion/current: serviceID]
+	//
+	// NOTE: our current tor controller does NOT support multiple onion
+	// services to be created at the same time, thus we expect the reply to
+	// only contain one serviceID. If multiple serviceIDs are returned, we
+	// would expected the reply to have the following format,
+	//      onions/current=serviceID1, serviceID2, serviceID3,...
+	// Thus a new parser is need to parse that reply.
+	resp := parseTorReply(reply)
+	serviceID, ok := resp["onions/current"]
+	if !ok {
+		return ErrNoServiceFound
+	}
+
+	// Check that our active service is indeed the service acknowledged by
+	// Tor daemon.
+	if c.activeServiceID != serviceID {
+		return fmt.Errorf("%w: controller has: %v, Tor daemon has: %v",
+			ErrServiceIDUnmatch, c.activeServiceID, serviceID)
+	}
+
+	return nil
+}
diff --git a/tor/cmd_info_test.go b/tor/cmd_info_test.go
new file mode 100644
index 000000000..81e9d051d
--- /dev/null
+++ b/tor/cmd_info_test.go
@@ -0,0 +1,87 @@
+package tor
+
+import (
+	"errors"
+	"io"
+	"syscall"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestCheckOnionServiceFailOnServiceNotCreated(t *testing.T) {
+	t.Parallel()
+
+	// Create a dummy tor controller.
+	c := &Controller{}
+
+	// Check that CheckOnionService returns an error when the service
+	// hasn't been created.
+	require.Equal(t, ErrServiceNotCreated, c.CheckOnionService())
+}
+
+func TestCheckOnionServiceSucceed(t *testing.T) {
+	t.Parallel()
+
+	// Create mock server and client connection.
+	proxy := createTestProxy(t)
+	defer proxy.cleanUp()
+	server := proxy.serverConn
+
+	// Assign a fake service ID to the controller.
+	c := &Controller{conn: proxy.clientConn, activeServiceID: "fakeID"}
+
+	// Test a successful response.
+	serverResp := "250-onions/current=fakeID\n250 OK\n"
+
+	// Let the server mocks a given response.
+	_, err := server.Write([]byte(serverResp))
+	require.NoError(t, err, "server failed to write")
+
+	// For a successful response, we expect no error.
+	require.NoError(t, c.CheckOnionService())
+}
+
+func TestCheckOnionServiceFailOnServiceIDNotMatch(t *testing.T) {
+	t.Parallel()
+
+	// Create mock server and client connection.
+	proxy := createTestProxy(t)
+	defer proxy.cleanUp()
+	server := proxy.serverConn
+
+	// Assign a fake service ID to the controller.
+	c := &Controller{conn: proxy.clientConn, activeServiceID: "fakeID"}
+
+	// Mock a response with a different serviceID.
+	serverResp := "250-onions/current=unmatchedID\n250 OK\n"
+
+	// Let the server mocks a given response.
+	_, err := server.Write([]byte(serverResp))
+	require.NoError(t, err, "server failed to write")
+
+	// Check the error returned from GetServiceInfo is expected.
+	require.ErrorIs(t, c.CheckOnionService(), ErrServiceIDUnmatch)
+}
+
+func TestCheckOnionServiceFailOnClosedConnection(t *testing.T) {
+	t.Parallel()
+
+	// Create mock server and client connection.
+	proxy := createTestProxy(t)
+	defer proxy.cleanUp()
+	server := proxy.serverConn
+
+	// Assign a fake service ID to the controller.
+	c := &Controller{conn: proxy.clientConn, activeServiceID: "fakeID"}
+
+	// Close the connection from the server side.
+	require.NoError(t, server.Close(), "server failed to close conn")
+
+	// Check the error returned from GetServiceInfo is expected.
+	err := c.CheckOnionService()
+	eof := errors.Is(err, io.EOF)
+	reset := errors.Is(err, syscall.ECONNRESET)
+	require.Truef(t, eof || reset,
+		"must of EOF or RESET error, instead got: %v", err)
+}

From cbd22a7c741e2aa1e24f1d7108d81b138ac420fc Mon Sep 17 00:00:00 2001
From: yyforyongyu <yy2452@columbia.edu>
Date: Mon, 27 Sep 2021 19:24:19 +0800
Subject: [PATCH 07/10] tor: add method Reconnect to reset connection

A new method, Reconnect, is added to tor controller which can be used to
reset the current connection. This will be later used in healthcheck to
help us reset the connection to Tor Daemon.
---
 tor/controller.go      | 62 +++++++++++++++++++++++++++++++
 tor/controller_test.go | 84 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 146 insertions(+)

diff --git a/tor/controller.go b/tor/controller.go
index c401cb0bf..ad9262578 100644
--- a/tor/controller.go
+++ b/tor/controller.go
@@ -69,6 +69,14 @@ var (
 	// errCodeNotMatch is used when an expected response code is not
 	// returned.
 	errCodeNotMatch = errors.New("unexpected code")
+
+	// errTCNotStarted is used when we require the tor controller to be
+	// started while it's not.
+	errTCNotStarted = errors.New("tor controller must be started")
+
+	// errTCNotStarted is used when we require the tor controller to be
+	// not stopped while it is.
+	errTCStopped = errors.New("tor controller must not be stopped")
 )
 
 // Controller is an implementation of the Tor Control protocol. This is used in
@@ -169,6 +177,60 @@ func (c *Controller) Stop() error {
 	return c.conn.Close()
 }
 
+// Reconnect makes a new socket connection between the tor controller and
+// daemon. It will attempt to close the old connection, make a new connection
+// and authenticate, and finally reset the activeServiceID that the controller
+// is aware of.
+//
+// NOTE: Any old onion services will be removed once this function is called.
+// In the case of a Tor daemon restart, previously created onion services will
+// no longer be there. If the function is called without a Tor daemon restart,
+// because the control connection is reset, all the onion services belonging to
+// the old connection will be removed.
+func (c *Controller) Reconnect() error {
+	// Require the tor controller to be running when we want to reconnect.
+	// This means the started flag must be 1 and the stopped flag must be
+	// 0.
+	if c.started != 1 {
+		return errTCNotStarted
+	}
+	if c.stopped != 0 {
+		return errTCStopped
+	}
+
+	log.Info("Re-connectting tor controller")
+
+	// If we have an old connection, try to close it. We might receive an
+	// error if the connection has already been closed by Tor daemon(ie,
+	// daemon restarted), so we ignore the error here.
+	if c.conn != nil {
+		if err := c.conn.Close(); err != nil {
+			log.Debugf("closing old conn got err: %v", err)
+		}
+	}
+
+	// Make a new connection and authenticate.
+	conn, err := textproto.Dial("tcp", c.controlAddr)
+	if err != nil {
+		return fmt.Errorf("unable to connect to Tor server: %w", err)
+	}
+
+	c.conn = conn
+
+	// Authenticate the connection between the controller and Tor daemon.
+	if err := c.authenticate(); err != nil {
+		return err
+	}
+
+	// Reset the activeServiceID. This value would only be set if a
+	// previous onion service was created. Because the old connection has
+	// been closed at this point, the old onion service is no longer
+	// active.
+	c.activeServiceID = ""
+
+	return nil
+}
+
 // sendCommand sends a command to the Tor server and returns its response, as a
 // single space-delimited string, and code.
 func (c *Controller) sendCommand(command string) (int, string, error) {
diff --git a/tor/controller_test.go b/tor/controller_test.go
index 00926a3d8..8fab16e38 100644
--- a/tor/controller_test.go
+++ b/tor/controller_test.go
@@ -276,3 +276,87 @@ func TestReadResponse(t *testing.T) {
 		})
 	}
 }
+
+// TestReconnectTCMustBeRunning checks that the tor controller must be running
+// while calling Reconnect.
+func TestReconnectTCMustBeRunning(t *testing.T) {
+	// Create a dummy controller.
+	c := &Controller{}
+
+	// Reconnect should fail because the TC is not started.
+	require.Equal(t, errTCNotStarted, c.Reconnect())
+
+	// Set the started flag.
+	c.started = 1
+
+	// Set the stopped flag so the TC is stopped.
+	c.stopped = 1
+
+	// Reconnect should fail because the TC is stopped.
+	require.Equal(t, errTCStopped, c.Reconnect())
+}
+
+// TestReconnectSucceed tests a reconnection will succeed when the tor
+// controller is up and running.
+func TestReconnectSucceed(t *testing.T) {
+	// Create mock server and client connection.
+	proxy := createTestProxy(t)
+	defer proxy.cleanUp()
+
+	// Create a tor controller and mark the controller as started.
+	c := &Controller{
+		conn:        proxy.clientConn,
+		started:     1,
+		controlAddr: proxy.serverAddr,
+	}
+
+	// Accept the connection inside a goroutine. We will also write some
+	// data so that the reconnection can succeed. We will mock three writes
+	// and two reads inside our proxy server,
+	//   - write protocol info
+	//   - read auth info
+	//   - write auth challenge
+	//   - read auth challenge
+	//   - write OK
+	go func() {
+		// Accept the new connection.
+		server, err := proxy.server.Accept()
+		require.NoError(t, err, "failed to accept")
+
+		// Write the protocol info.
+		resp := "250-PROTOCOLINFO 1\n" +
+			"250-AUTH METHODS=NULL\n" +
+			"250 OK\n"
+		_, err = server.Write([]byte(resp))
+		require.NoErrorf(t, err, "failed to write protocol info")
+
+		// Read the auth info from the client.
+		buf := make([]byte, 65535)
+		_, err = server.Read(buf)
+		require.NoError(t, err)
+
+		// Write the auth challenge.
+		resp = "250 AUTHCHALLENGE SERVERHASH=fake\n"
+		_, err = server.Write([]byte(resp))
+		require.NoErrorf(t, err, "failed to write auth challenge")
+
+		// Read the auth challenge resp from the client.
+		_, err = server.Read(buf)
+		require.NoError(t, err)
+
+		// Write OK resp.
+		resp = "250 OK\n"
+		_, err = server.Write([]byte(resp))
+		require.NoErrorf(t, err, "failed to write response auth")
+	}()
+
+	// Reconnect should succeed.
+	require.NoError(t, c.Reconnect())
+
+	// Check that the old connection is closed.
+	_, err := proxy.clientConn.ReadLine()
+	require.Contains(t, err.Error(), "use of closed network connection")
+
+	// Check that the connection has been updated.
+	require.NotEqual(t, proxy.clientConn, c.conn)
+}

From ca13750b2c46247c12740a6e96351b5195fa3767 Mon Sep 17 00:00:00 2001
From: yyforyongyu <yy2452@columbia.edu>
Date: Mon, 27 Sep 2021 20:17:26 +0800
Subject: [PATCH 08/10] healthcheck: add CheckTorServiceStatus to monitor Tor
 connection

---
 healthcheck/tor_connection.go | 86 +++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 healthcheck/tor_connection.go

diff --git a/healthcheck/tor_connection.go b/healthcheck/tor_connection.go
new file mode 100644
index 000000000..13f90a8c8
--- /dev/null
+++ b/healthcheck/tor_connection.go
@@ -0,0 +1,86 @@
+package healthcheck
+
+import (
+	"errors"
+	"fmt"
+	"io"
+	"syscall"
+
+	"github.com/lightningnetwork/lnd/tor"
+)
+
+// CheckTorServiceStatus checks whether the onion service is reachable by
+// sending a GETINFO command to the Tor daemon using our tor controller.
+// We will get an EOF or a broken pipe error if the Tor daemon is
+// stopped/restarted as the previously created socket connection is no longer
+// open. In this case, we will attempt a restart on our tor controller. If the
+// tor daemon comes back, a new socket connection will then be created.
+func CheckTorServiceStatus(tc *tor.Controller,
+	createService func() error) error {
+
+	// Send a cmd using GETINFO onions/current and checks that our known
+	// onion serviceID can be found.
+	err := tc.CheckOnionService()
+	if err == nil {
+		return nil
+	}
+
+	log.Debugf("Checking tor service got: %v", err)
+
+	switch {
+	// We will get an EOF if the connection is lost. In this case, we will
+	// return an error and wait for the Tor daemon to come back. We won't
+	// attempt to make a new connection since we know Tor daemon is down.
+	case errors.Is(err, io.EOF), errors.Is(err, syscall.ECONNREFUSED):
+		return fmt.Errorf("Tor daemon connection lost, " +
+			"check if Tor is up and running")
+
+	// Once Tor daemon is down, we will get a broken pipe error when we use
+	// the existing connection to make a GETINFO request since that socket
+	// has now been closed. As Tor daemon might not be running yet, we will
+	// attempt to make a new connection till Tor daemon is back.
+	case errors.Is(err, syscall.EPIPE):
+		log.Warnf("Tor connection lost, attempting a tor controller " +
+			"re-connection...")
+
+		// If the restart fails, we will attempt again during our next
+		// healthcheck cycle.
+		return restartTorController(tc, createService)
+
+	// If this is not a connection layer error, such as
+	// ErrServiceNotCreated or ErrServiceIDUnmatch, there's little we can
+	// do but to report the error to the user.
+	default:
+		return err
+	}
+}
+
+// restartTorController attempts to make a new connection to the Tor daemon and
+// re-create the Hidden Service.
+func restartTorController(tc *tor.Controller,
+	createService func() error) error {
+
+	err := tc.Reconnect()
+
+	// If we get a connection refused error, it means Tor daemon might not
+	// be started.
+	if errors.Is(err, syscall.ECONNREFUSED) {
+		return fmt.Errorf("check if Tor daemon is running")
+	}
+
+	// Otherwise, we get an unexpected and return it.
+	if err != nil {
+		log.Errorf("Re-connectting tor got err: %v", err)
+		return err
+	}
+
+	// Recreate the Hidden Service.
+	if err := createService(); err != nil {
+		log.Errorf("Re-create service tor got err: %v", err)
+		return err
+	}
+
+	log.Info("Successfully restarted tor connection!")
+
+	return nil
+}

From f41f5c7fa60de56a14befe8b001d08a4875de747 Mon Sep 17 00:00:00 2001
From: yyforyongyu <yy2452@columbia.edu>
Date: Mon, 27 Sep 2021 20:40:41 +0800
Subject: [PATCH 09/10] multi: add tor connection healthcheck

This commit adds a new health check, tor connection, to our liveness
monitor. A monitor refactor is applied to the server creation such that
the scope of health check creation is managed within one function.
---
 config.go            | 15 ++++++++
 lncfg/healthcheck.go |  6 +++
 sample-lnd.conf      | 16 ++++++++
 server.go            | 90 ++++++++++++++++++++++++++++++--------------
 4 files changed, 98 insertions(+), 29 deletions(-)

diff --git a/config.go b/config.go
index 6044ae0c6..35fc24dfd 100644
--- a/config.go
+++ b/config.go
@@ -139,6 +139,15 @@ const (
 	defaultTLSBackoff  = time.Minute
 	defaultTLSAttempts = 0
 
+	// Set defaults for a health check which ensures that the tor
+	// connection is alive. Although this check is off by default (not all
+	// setups require it), we still set the other default values so that
+	// the health check can be easily enabled with sane defaults.
+	defaultTCInterval = time.Minute
+	defaultTCTimeout  = time.Second * 5
+	defaultTCBackoff  = time.Minute
+	defaultTCAttempts = 0
+
 	// defaultRemoteMaxHtlcs specifies the default limit for maximum
 	// concurrent HTLCs the remote party may add to commitment transactions.
 	// This value can be overridden with --default-remote-max-htlcs.
@@ -541,6 +550,12 @@ func DefaultConfig() Config {
 				Attempts: defaultTLSAttempts,
 				Backoff:  defaultTLSBackoff,
 			},
+			TorConnection: &lncfg.CheckConfig{
+				Interval: defaultTCInterval,
+				Timeout:  defaultTCTimeout,
+				Attempts: defaultTCAttempts,
+				Backoff:  defaultTCBackoff,
+			},
 		},
 		Gossip: &lncfg.Gossip{
 			MaxChannelUpdateBurst: discovery.DefaultMaxChannelUpdateBurst,
diff --git a/lncfg/healthcheck.go b/lncfg/healthcheck.go
index bee569b3a..2c2a77c99 100644
--- a/lncfg/healthcheck.go
+++ b/lncfg/healthcheck.go
@@ -28,6 +28,8 @@ type HealthCheckConfig struct {
 	DiskCheck *DiskCheckConfig `group:"diskspace" namespace:"diskspace"`
 
 	TLSCheck *CheckConfig `group:"tls" namespace:"tls"`
+
+	TorConnection *CheckConfig `group:"torconnection" namespace:"torconnection"`
 }
 
 // Validate checks the values configured for our health checks.
@@ -50,6 +52,10 @@ func (h *HealthCheckConfig) Validate() error {
 		return errors.New("disk required ratio must be in [0:1)")
 	}
 
+	if err := h.TorConnection.validate("tor connection"); err != nil {
+		return err
+	}
+
 	return nil
 }
 
diff --git a/sample-lnd.conf b/sample-lnd.conf
index ad0ea1dc7..cef33bab6 100644
--- a/sample-lnd.conf
+++ b/sample-lnd.conf
@@ -990,6 +990,22 @@ litecoin.node=ltcd
 ; This value must be >= 1m.
 ; healthcheck.tls.interval=1m
 
+; The number of times we should attempt to check our tor connection before
+; gracefully shutting down. Set this value to 0 to disable this health check.
+; healthcheck.torconnection.attempts=3
+
+; The amount of time we allow a call to our tor connection to take before we 
+; fail the attempt. This value must be >= 1s.
+; healthcheck.torconnection.timeout=10s
+
+; The amount of time we should backoff between failed attempts to check tor 
+; connection. This value must be >= 1s.
+; healthcheck.torconnection.backoff=30s
+
+; The amount of time we should wait between tor connection health checks. This
+; value must be >= 1m.
+; healthcheck.torconnection.interval=1m
+
 
 [signrpc]
 
diff --git a/server.go b/server.go
index f8fddfd3f..30dadc3de 100644
--- a/server.go
+++ b/server.go
@@ -1470,9 +1470,40 @@ func newServer(cfg *Config, listenAddrs []net.Addr,
 		})
 	}
 
-	// Create a set of health checks using our configured values. If a
-	// health check has been disabled by setting attempts to 0, our monitor
-	// will not run it.
+	// Create liveliness monitor.
+	s.createLivenessMonitor(cfg, cc)
+
+	// Create the connection manager which will be responsible for
+	// maintaining persistent outbound connections and also accepting new
+	// incoming connections
+	cmgr, err := connmgr.New(&connmgr.Config{
+		Listeners:      listeners,
+		OnAccept:       s.InboundPeerConnected,
+		RetryDuration:  time.Second * 5,
+		TargetOutbound: 100,
+		Dial: noiseDial(
+			nodeKeyECDH, s.cfg.net, s.cfg.ConnectionTimeout,
+		),
+		OnConnection: s.OutboundPeerConnected,
+	})
+	if err != nil {
+		return nil, err
+	}
+	s.connMgr = cmgr
+
+	return s, nil
+}
+
+// createLivenessMonitor creates a set of health checks using our configured
+// values and uses these checks to create a liveliness monitor. Available
+// health checks,
+//   - chainHealthCheck
+//   - diskCheck
+//   - tlsHealthCheck
+//   - torController, only created when tor is enabled.
+// If a health check has been disabled by setting attempts to 0, our monitor
+// will not run it.
+func (s *server) createLivenessMonitor(cfg *Config, cc *chainreg.ChainControl) {
 	chainHealthCheck := healthcheck.NewObservation(
 		"chain backend",
 		cc.HealthCheck,
@@ -1521,11 +1552,12 @@ func newServer(cfg *Config, listenAddrs []net.Addr,
 			// If the current time is passed the certificate's
 			// expiry time, then it is considered expired
 			if time.Now().After(parsedCert.NotAfter) {
-				return fmt.Errorf("TLS certificate is expired as of %v", parsedCert.NotAfter)
+				return fmt.Errorf("TLS certificate is "+
+					"expired as of %v", parsedCert.NotAfter)
 			}
 
-			// If the certificate is not outdated, no error needs to
-			// be returned
+			// If the certificate is not outdated, no error needs
+			// to be returned
 			return nil
 		},
 		cfg.HealthChecks.TLSCheck.Interval,
@@ -1534,36 +1566,36 @@ func newServer(cfg *Config, listenAddrs []net.Addr,
 		cfg.HealthChecks.TLSCheck.Attempts,
 	)
 
+	checks := []*healthcheck.Observation{
+		chainHealthCheck, diskCheck, tlsHealthCheck,
+	}
+
+	// If Tor is enabled, add the healthcheck for tor connection.
+	if s.torController != nil {
+		torConnectionCheck := healthcheck.NewObservation(
+			"tor connection",
+			func() error {
+				return healthcheck.CheckTorServiceStatus(
+					s.torController,
+					s.createNewHiddenService,
+				)
+			},
+			cfg.HealthChecks.TorConnection.Interval,
+			cfg.HealthChecks.TorConnection.Timeout,
+			cfg.HealthChecks.TorConnection.Backoff,
+			cfg.HealthChecks.TorConnection.Attempts,
+		)
+		checks = append(checks, torConnectionCheck)
+	}
+
 	// If we have not disabled all of our health checks, we create a
 	// liveliness monitor with our configured checks.
 	s.livelinessMonitor = healthcheck.NewMonitor(
 		&healthcheck.Config{
-			Checks: []*healthcheck.Observation{
-				chainHealthCheck, diskCheck, tlsHealthCheck,
-			},
+			Checks:   checks,
 			Shutdown: srvrLog.Criticalf,
 		},
 	)
-
-	// Create the connection manager which will be responsible for
-	// maintaining persistent outbound connections and also accepting new
-	// incoming connections
-	cmgr, err := connmgr.New(&connmgr.Config{
-		Listeners:      listeners,
-		OnAccept:       s.InboundPeerConnected,
-		RetryDuration:  time.Second * 5,
-		TargetOutbound: 100,
-		Dial: noiseDial(
-			nodeKeyECDH, s.cfg.net, s.cfg.ConnectionTimeout,
-		),
-		OnConnection: s.OutboundPeerConnected,
-	})
-	if err != nil {
-		return nil, err
-	}
-	s.connMgr = cmgr
-
-	return s, nil
 }
 
 // Started returns true if the server has been started, and false otherwise.

From 07a8b637151684996cc7d05a925f67cd54df90e5 Mon Sep 17 00:00:00 2001
From: yyforyongyu <yy2452@columbia.edu>
Date: Mon, 27 Sep 2021 21:41:17 +0800
Subject: [PATCH 10/10] docs: add release note for tor controller

---
 docs/release-notes/release-notes-0.14.0.md | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/docs/release-notes/release-notes-0.14.0.md b/docs/release-notes/release-notes-0.14.0.md
index 738aeaf08..7444dbba6 100644
--- a/docs/release-notes/release-notes-0.14.0.md
+++ b/docs/release-notes/release-notes-0.14.0.md
@@ -2,11 +2,32 @@
 
 ## Networking & Tor
 
+### Connectivity mode
+
 A new flag has been added to enable a hybrid tor connectivity mode, where tor
 is only used for onion address connections, and clearnet for everything else.
 This new behavior can be added using the `tor.skip-proxy-for-clearnet-targets`
 flag.
 
+### Onion service
+
+The Onion service created upon lnd startup is [now deleted during lnd shutdown
+using `DEL_ONION`](https://github.com/lightningnetwork/lnd/pull/5794). 
+
+### Tor connection
+
+A new health check, tor connection, [is added to lnd's liveness monitor upon
+startup](https://github.com/lightningnetwork/lnd/pull/5794). This check will
+ensure the liveness of the connection between the Tor daemon and lnd's tor
+controller. To enable it, please use the following flags,
+```
+healthcheck.torconnection.attempts=xxx
+healthcheck.torconnection.timeout=xxx
+healthcheck.torconnection.backoff=xxx
+healthcheck.torconnection.internal=xxx
+```
+Read more about the usage of the flags in the `sample-lnd.conf`.
+
 ## LN Peer-to-Peer Netowrk
 
 ### Bitcoin Blockheaders in Ping Messages