Merge pull request #7762 from guggero/empty-resp

lnrpc: return meaningful response instead of empty one

chanbackup/recover.go

@@ -40,9 +40,11 @@ type PeerConnector interface {
 // the channel. In addition a LinkNode will be created for each new peer as
 // well, in order to expose the addressing information required to locate to
 // and connect to each peer in order to initiate the recovery protocol.
+// The number of channels that were successfully restored is returned.
 func Recover(backups []Single, restorer ChannelRestorer,
-	peerConnector PeerConnector) error {
+	peerConnector PeerConnector) (int, error) {
 
+	var numRestored int
 	for i, backup := range backups {
 		log.Infof("Restoring ChannelPoint(%v) to disk: ",
 			backup.FundingOutpoint)
@@ -57,9 +59,10 @@ func Recover(backups []Single, restorer ChannelRestorer,
 			continue
 		}
 		if err != nil {
-			return err
+			return numRestored, err
 		}
 
+		numRestored++
 		log.Infof("Attempting to connect to node=%x (addrs=%v) to "+
 			"restore ChannelPoint(%v)",
 			backup.RemoteNodePub.SerializeCompressed(),
@@ -70,7 +73,7 @@ func Recover(backups []Single, restorer ChannelRestorer,
 			backup.RemoteNodePub, backup.Addresses,
 		)
 		if err != nil {
-			return err
+			return numRestored, err
 		}
 
 		// TODO(roasbeef): to handle case where node has changed addrs,
@@ -80,24 +83,25 @@ func Recover(backups []Single, restorer ChannelRestorer,
 		//  * just to to fresh w/ call to node addrs and de-dup?
 	}
 
-	return nil
+	return numRestored, nil
 }
 
 // TODO(roasbeef): more specific keychain interface?
 
 // UnpackAndRecoverSingles is a one-shot method, that given a set of packed
 // single channel backups, will restore the channel state to a channel shell,
 // and also reach out to connect to any of the known node addresses for that
-// channel. It is assumes that after this method exists, if a connection we
-// able to be established, then then PeerConnector will continue to attempt to
-// re-establish a persistent connection in the background.
+// channel. It is assumes that after this method exists, if a connection was
+// established, then the PeerConnector will continue to attempt to re-establish
+// a persistent connection in the background. The number of channels that were
+// successfully restored is returned.
 func UnpackAndRecoverSingles(singles PackedSingles,
 	keyChain keychain.KeyRing, restorer ChannelRestorer,
-	peerConnector PeerConnector) error {
+	peerConnector PeerConnector) (int, error) {
 
 	chanBackups, err := singles.Unpack(keyChain)
 	if err != nil {
-		return err
+		return 0, err
 	}
 
 	return Recover(chanBackups, restorer, peerConnector)
@@ -106,16 +110,17 @@ func UnpackAndRecoverSingles(singles PackedSingles,
 // UnpackAndRecoverMulti is a one-shot method, that given a set of packed
 // multi-channel backups, will restore the channel states to channel shells,
 // and also reach out to connect to any of the known node addresses for that
-// channel. It is assumes that after this method exists, if a connection we
-// able to be established, then then PeerConnector will continue to attempt to
-// re-establish a persistent connection in the background.
+// channel. It is assumes that after this method exists, if a connection was
+// established, then the PeerConnector will continue to attempt to re-establish
+// a persistent connection in the background. The number of channels that were
+// successfully restored is returned.
 func UnpackAndRecoverMulti(packedMulti PackedMulti,
 	keyChain keychain.KeyRing, restorer ChannelRestorer,
-	peerConnector PeerConnector) error {
+	peerConnector PeerConnector) (int, error) {
 
 	chanBackups, err := packedMulti.Unpack(keyChain)
 	if err != nil {
-		return err
+		return 0, err
 	}
 
 	return Recover(chanBackups.StaticBackups, restorer, peerConnector)

chanbackup/recover_test.go

@@ -2,7 +2,7 @@ package chanbackup
 
 import (
 	"bytes"
-	"fmt"
+	"errors"
 	"net"
 	"testing"
 
@@ -11,6 +11,12 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+var (
+	errRestoreFail = errors.New("restore fail")
+
+	errConnectFail = errors.New("connect fail")
+)
+
 type mockChannelRestorer struct {
 	fail bool
 
@@ -19,7 +25,7 @@ type mockChannelRestorer struct {
 
 func (m *mockChannelRestorer) RestoreChansFromSingles(...Single) error {
 	if m.fail {
-		return fmt.Errorf("fail")
+		return errRestoreFail
 	}
 
 	m.callCount++
@@ -33,11 +39,11 @@ type mockPeerConnector struct {
 	callCount int
 }
 
-func (m *mockPeerConnector) ConnectPeer(node *btcec.PublicKey,
-	addrs []net.Addr) error {
+func (m *mockPeerConnector) ConnectPeer(_ *btcec.PublicKey,
+	_ []net.Addr) error {
 
 	if m.fail {
-		return fmt.Errorf("fail")
+		return errConnectFail
 	}
 
 	m.callCount++
@@ -59,16 +65,13 @@ func TestUnpackAndRecoverSingles(t *testing.T) {
 	var packedBackups PackedSingles
 	for i := 0; i < numSingles; i++ {
 		channel, err := genRandomOpenChannelShell()
-		if err != nil {
-			t.Fatalf("unable make channel: %v", err)
-		}
+		require.NoError(t, err)
 
 		single := NewSingle(channel, nil)
 
 		var b bytes.Buffer
-		if err := single.PackToWriter(&b, keyRing); err != nil {
-			t.Fatalf("unable to pack single: %v", err)
-		}
+		err = single.PackToWriter(&b, keyRing)
+		require.NoError(t, err)
 
 		backups = append(backups, single)
 		packedBackups = append(packedBackups, b.Bytes())
@@ -83,54 +86,47 @@ func TestUnpackAndRecoverSingles(t *testing.T) {
 	// If we make the channel restore fail, then the entire method should
 	// as well
 	chanRestorer.fail = true
-	err := UnpackAndRecoverSingles(
+	_, err := UnpackAndRecoverSingles(
 		packedBackups, keyRing, &chanRestorer, &peerConnector,
 	)
-	if err == nil {
-		t.Fatalf("restoration should have failed")
-	}
+	require.ErrorIs(t, err, errRestoreFail)
 
 	chanRestorer.fail = false
 
 	// If we make the peer connector fail, then the entire method should as
 	// well
 	peerConnector.fail = true
-	err = UnpackAndRecoverSingles(
+	_, err = UnpackAndRecoverSingles(
 		packedBackups, keyRing, &chanRestorer, &peerConnector,
 	)
-	if err == nil {
-		t.Fatalf("restoration should have failed")
-	}
+	require.ErrorIs(t, err, errConnectFail)
 
 	chanRestorer.callCount--
 	peerConnector.fail = false
 
 	// Next, we'll ensure that if all the interfaces function as expected,
 	// then the channels will properly be unpacked and restored.
-	err = UnpackAndRecoverSingles(
+	numRestored, err := UnpackAndRecoverSingles(
 		packedBackups, keyRing, &chanRestorer, &peerConnector,
 	)
-	require.NoError(t, err, "unable to recover chans")
+	require.NoError(t, err)
+	require.EqualValues(t, numSingles, numRestored)
 
 	// Both the restorer, and connector should have been called 10 times,
 	// once for each backup.
-	if chanRestorer.callCount != numSingles {
-		t.Fatalf("expected %v calls, instead got %v",
-			numSingles, chanRestorer.callCount)
-	}
-	if peerConnector.callCount != numSingles {
-		t.Fatalf("expected %v calls, instead got %v",
-			numSingles, peerConnector.callCount)
-	}
+	require.EqualValues(
+		t, numSingles, chanRestorer.callCount, "restorer call count",
+	)
+	require.EqualValues(
+		t, numSingles, peerConnector.callCount, "peer call count",
+	)
 
 	// If we modify the keyRing, then unpacking should fail.
 	keyRing.Fail = true
-	err = UnpackAndRecoverSingles(
+	_, err = UnpackAndRecoverSingles(
 		packedBackups, keyRing, &chanRestorer, &peerConnector,
 	)
-	if err == nil {
-		t.Fatalf("unpacking should have failed")
-	}
+	require.ErrorContains(t, err, "fail")
 
 	// TODO(roasbeef): verify proper call args
 }
@@ -148,9 +144,7 @@ func TestUnpackAndRecoverMulti(t *testing.T) {
 	backups := make([]Single, 0, numSingles)
 	for i := 0; i < numSingles; i++ {
 		channel, err := genRandomOpenChannelShell()
-		if err != nil {
-			t.Fatalf("unable make channel: %v", err)
-		}
+		require.NoError(t, err)
 
 		single := NewSingle(channel, nil)
 
@@ -162,9 +156,8 @@ func TestUnpackAndRecoverMulti(t *testing.T) {
 	}
 
 	var b bytes.Buffer
-	if err := multi.PackToWriter(&b, keyRing); err != nil {
-		t.Fatalf("unable to pack multi: %v", err)
-	}
+	err := multi.PackToWriter(&b, keyRing)
+	require.NoError(t, err)
 
 	// Next, we'll pack the set of singles into a packed multi, and also
 	// create the set of interfaces we need to carry out the remainder of
@@ -177,54 +170,47 @@ func TestUnpackAndRecoverMulti(t *testing.T) {
 	// If we make the channel restore fail, then the entire method should
 	// as well
 	chanRestorer.fail = true
-	err := UnpackAndRecoverMulti(
+	_, err = UnpackAndRecoverMulti(
 		packedMulti, keyRing, &chanRestorer, &peerConnector,
 	)
-	if err == nil {
-		t.Fatalf("restoration should have failed")
-	}
+	require.ErrorIs(t, err, errRestoreFail)
 
 	chanRestorer.fail = false
 
 	// If we make the peer connector fail, then the entire method should as
 	// well
 	peerConnector.fail = true
-	err = UnpackAndRecoverMulti(
+	_, err = UnpackAndRecoverMulti(
 		packedMulti, keyRing, &chanRestorer, &peerConnector,
 	)
-	if err == nil {
-		t.Fatalf("restoration should have failed")
-	}
+	require.ErrorIs(t, err, errConnectFail)
 
 	chanRestorer.callCount--
 	peerConnector.fail = false
 
 	// Next, we'll ensure that if all the interfaces function as expected,
 	// then the channels will properly be unpacked and restored.
-	err = UnpackAndRecoverMulti(
+	numRestored, err := UnpackAndRecoverMulti(
 		packedMulti, keyRing, &chanRestorer, &peerConnector,
 	)
-	require.NoError(t, err, "unable to recover chans")
+	require.NoError(t, err)
+	require.EqualValues(t, numSingles, numRestored)
 
 	// Both the restorer, and connector should have been called 10 times,
 	// once for each backup.
-	if chanRestorer.callCount != numSingles {
-		t.Fatalf("expected %v calls, instead got %v",
-			numSingles, chanRestorer.callCount)
-	}
-	if peerConnector.callCount != numSingles {
-		t.Fatalf("expected %v calls, instead got %v",
-			numSingles, peerConnector.callCount)
-	}
+	require.EqualValues(
+		t, numSingles, chanRestorer.callCount, "restorer call count",
+	)
+	require.EqualValues(
+		t, numSingles, peerConnector.callCount, "peer call count",
+	)
 
 	// If we modify the keyRing, then unpacking should fail.
 	keyRing.Fail = true
-	err = UnpackAndRecoverMulti(
+	_, err = UnpackAndRecoverMulti(
 		packedMulti, keyRing, &chanRestorer, &peerConnector,
 	)
-	if err == nil {
-		t.Fatalf("unpacking should have failed")
-	}
+	require.ErrorContains(t, err, "fail")
 
 	// TODO(roasbeef): verify proper call args
 }

channeldb/payment_control_test.go

@@ -435,9 +435,9 @@ func TestPaymentControlDeleteNonInFlight(t *testing.T) {
 	}
 
 	// Delete all failed payments.
-	if err := db.DeletePayments(true, false); err != nil {
-		t.Fatal(err)
-	}
+	numPayments, err := db.DeletePayments(true, false)
+	require.NoError(t, err)
+	require.EqualValues(t, 1, numPayments)
 
 	// This should leave the succeeded and in-flight payments.
 	dbPayments, err := db.FetchPayments()
@@ -471,9 +471,9 @@ func TestPaymentControlDeleteNonInFlight(t *testing.T) {
 	}
 
 	// Now delete all payments except in-flight.
-	if err := db.DeletePayments(false, false); err != nil {
-		t.Fatal(err)
-	}
+	numPayments, err = db.DeletePayments(false, false)
+	require.NoError(t, err)
+	require.EqualValues(t, 2, numPayments)
 
 	// This should leave the in-flight payment.
 	dbPayments, err = db.FetchPayments()
@@ -536,27 +536,35 @@ func TestPaymentControlDeletePayments(t *testing.T) {
 	assertPayments(t, db, payments)
 
 	// Delete HTLC attempts for failed payments only.
-	require.NoError(t, db.DeletePayments(true, true))
+	numPayments, err := db.DeletePayments(true, true)
+	require.NoError(t, err)
+	require.EqualValues(t, 0, numPayments)
 
 	// The failed payment is the only altered one.
 	payments[0].htlcs = 0
 	assertPayments(t, db, payments)
 
 	// Delete failed attempts for all payments.
-	require.NoError(t, db.DeletePayments(false, true))
+	numPayments, err = db.DeletePayments(false, true)
+	require.NoError(t, err)
+	require.EqualValues(t, 0, numPayments)
 
 	// The failed attempts should be deleted, except for the in-flight
 	// payment, that shouldn't be altered until it has completed.
 	payments[1].htlcs = 1
 	assertPayments(t, db, payments)
 
 	// Now delete all failed payments.
-	require.NoError(t, db.DeletePayments(true, false))
+	numPayments, err = db.DeletePayments(true, false)
+	require.NoError(t, err)
+	require.EqualValues(t, 1, numPayments)
 
 	assertPayments(t, db, payments[1:])
 
 	// Finally delete all completed payments.
-	require.NoError(t, db.DeletePayments(false, false))
+	numPayments, err = db.DeletePayments(false, false)
+	require.NoError(t, err)
+	require.EqualValues(t, 1, numPayments)
 
 	assertPayments(t, db, payments[2:])
 }