watchtower+htlcswitch: update client tower logic to recognize safu commitments

In this commit, we update the tower+link logic to tag a commitment as the new (tweakless) format if it applies. In order to do this, the BackupTask method has gained an additional parameter to indicate the type of commitment that we're attempting to upload. This new tweakless bool is then threaded through all the way to back up task creation to ensure that we make the proper input.Input. Finally, we've added a new test case for each existing test case to test each case w/ and w/o the tweakless modifier.
2019-08-07 19:49:59 -07:00 · 2019-08-07 19:49:59 -07:00 · 4b65aea306
commit 4b65aea306
parent d22f2a1936
8 changed files with 219 additions and 162 deletions
--- a/htlcswitch/interfaces.go
+++ b/htlcswitch/interfaces.go
@ -175,6 +175,8 @@ type TowerClient interface {
 	// state. If the method returns nil, the backup is guaranteed to be
 	// successful unless the tower is unavailable and client is force quit,
 	// or the justice transaction would create dust outputs when trying to
-	// abide by the negotiated policy.
-	BackupState(*lnwire.ChannelID, *lnwallet.BreachRetribution) error
+	// abide by the negotiated policy. If the channel we're trying to back
+	// up doesn't have a tweak for the remote party's output, then
+	// isTweakless should be true.
+	BackupState(*lnwire.ChannelID, *lnwallet.BreachRetribution, bool) error
 }
--- a/htlcswitch/link.go
+++ b/htlcswitch/link.go
@ -954,7 +954,7 @@ func (l *channelLink) htlcManager() {

 			l.fail(
 				LinkFailureError{
-					code:       ErrSyncError,
+					code:       ErrRecoveryError,
 					ForceClose: false,
 				},
 				"unable to synchronize channel "+
@ -1844,8 +1844,13 @@ func (l *channelLink) handleUpstreamMsg(msg lnwire.Message) {
 				return
 			}

+			chanType := l.channel.State().ChanType
+			isTweakless := chanType == channeldb.SingleFunderTweakless
+
 			chanID := l.ChanID()
-			err = l.cfg.TowerClient.BackupState(&chanID, breachInfo)
+			err = l.cfg.TowerClient.BackupState(
+				&chanID, breachInfo, isTweakless,
+			)
 			if err != nil {
 				l.fail(LinkFailureError{code: ErrInternalError},
 					"unable to queue breach backup: %v", err)
--- a/htlcswitch/linkfailure.go
+++ b/htlcswitch/linkfailure.go
@ -34,6 +34,11 @@ const (
 	// ErrInvalidRevocation indicates that the remote peer send us an
 	// invalid revocation message.
 	ErrInvalidRevocation
+
+	// ErrRecoveryError the channel was unable to be resumed, we need the
+	// remote party to force close the channel out on chain now as a
+	// result.
+	ErrRecoveryError
 )

 // LinkFailureError encapsulates an error that will make us fail the current
@ -74,6 +79,8 @@ func (e LinkFailureError) Error() string {
 		return "invalid commitment"
 	case ErrInvalidRevocation:
 		return "invalid revocation"
+	case ErrRecoveryError:
+		return "unable to resume channel, recovery required"
 	default:
 		return "unknown error"
 	}
--- a/watchtower/lookout/justice_descriptor_test.go
+++ b/watchtower/lookout/justice_descriptor_test.go
@ -254,7 +254,7 @@ func testJusticeDescriptor(t *testing.T, blobType blob.Type) {
 	// DER-encoded signature under the to-remote pubkey. The sighash flag is
 	// also present, so we trim it.
 	toRemoteWitness, err := input.CommitSpendNoDelay(
-		signer, toRemoteSignDesc, justiceTxn,
+		signer, toRemoteSignDesc, justiceTxn, false,
 	)
 	if err != nil {
 		t.Fatalf("unable to sign to-remote input: %v", err)
--- a/watchtower/wtclient/backup_task.go
+++ b/watchtower/wtclient/backup_task.go
@ -54,7 +54,7 @@ type backupTask struct {
 // variables.
 func newBackupTask(chanID *lnwire.ChannelID,
 	breachInfo *lnwallet.BreachRetribution,
-	sweepPkScript []byte) *backupTask {
+	sweepPkScript []byte, isTweakless bool) *backupTask {

 	// Parse the non-dust outputs from the breach transaction,
 	// simultaneously computing the total amount contained in the inputs
@ -85,12 +85,18 @@ func newBackupTask(chanID *lnwire.ChannelID,
 		totalAmt += breachInfo.RemoteOutputSignDesc.Output.Value
 	}
 	if breachInfo.LocalOutputSignDesc != nil {
+		witnessType := input.CommitmentNoDelay
+		if isTweakless {
+			witnessType = input.CommitSpendNoDelayTweakless
+		}
+
 		toRemoteInput = input.NewBaseInput(
 			&breachInfo.LocalOutpoint,
-			input.CommitmentNoDelay,
+			witnessType,
 			breachInfo.LocalOutputSignDesc,
 			0,
 		)
+
 		totalAmt += breachInfo.LocalOutputSignDesc.Output.Value
 	}

@ -271,6 +277,8 @@ func (t *backupTask) craftSessionPayload(
 		case input.CommitmentRevoke:
 			copy(justiceKit.CommitToLocalSig[:], signature[:])

+		case input.CommitSpendNoDelayTweakless:
+			fallthrough
 		case input.CommitmentNoDelay:
 			copy(justiceKit.CommitToRemoteSig[:], signature[:])
 		}
--- a/watchtower/wtclient/backup_task_internal_test.go
+++ b/watchtower/wtclient/backup_task_internal_test.go
@ -73,6 +73,7 @@ type backupTaskTest struct {
 	bindErr          error
 	expSweepScript   []byte
 	signer           input.Signer
+	tweakless        bool
 }

 // genTaskTest creates a instance of a backupTaskTest using the passed
@ -89,7 +90,8 @@ func genTaskTest(
 	rewardScript []byte,
 	expSweepAmt int64,
 	expRewardAmt int64,
-	bindErr error) backupTaskTest {
+	bindErr error,
+	tweakless bool) backupTaskTest {

 	// Parse the key pairs for all keys used in the test.
 	revSK, revPK := btcec.PrivKeyFromBytes(
@ -188,9 +190,15 @@ func genTaskTest(
 			Hash:  txid,
 			Index: index,
 		}
+
+		witnessType := input.CommitmentNoDelay
+		if tweakless {
+			witnessType = input.CommitSpendNoDelayTweakless
+		}
+
 		toRemoteInput = input.NewBaseInput(
 			&breachInfo.LocalOutpoint,
-			input.CommitmentNoDelay,
+			witnessType,
 			breachInfo.LocalOutputSignDesc,
 			0,
 		)
@ -218,6 +226,7 @@ func genTaskTest(
 		bindErr:        bindErr,
 		expSweepScript: makeAddrSlice(22),
 		signer:         signer,
+		tweakless:      tweakless,
 	}
 }

@ -233,153 +242,6 @@ var (
 	addrScript, _ = txscript.PayToAddrScript(addr)
 )

-var backupTaskTests = []backupTaskTest{
-	genTaskTest(
-		"commit no-reward, both outputs",
-		100,                    // stateNum
-		200000,                 // toLocalAmt
-		100000,                 // toRemoteAmt
-		blobTypeCommitNoReward, // blobType
-		1000,                   // sweepFeeRate
-		nil,                    // rewardScript
-		299241,                 // expSweepAmt
-		0,                      // expRewardAmt
-		nil,                    // bindErr
-	),
-	genTaskTest(
-		"commit no-reward, to-local output only",
-		1000,                   // stateNum
-		200000,                 // toLocalAmt
-		0,                      // toRemoteAmt
-		blobTypeCommitNoReward, // blobType
-		1000,                   // sweepFeeRate
-		nil,                    // rewardScript
-		199514,                 // expSweepAmt
-		0,                      // expRewardAmt
-		nil,                    // bindErr
-	),
-	genTaskTest(
-		"commit no-reward, to-remote output only",
-		1,                      // stateNum
-		0,                      // toLocalAmt
-		100000,                 // toRemoteAmt
-		blobTypeCommitNoReward, // blobType
-		1000,                   // sweepFeeRate
-		nil,                    // rewardScript
-		99561,                  // expSweepAmt
-		0,                      // expRewardAmt
-		nil,                    // bindErr
-	),
-	genTaskTest(
-		"commit no-reward, to-remote output only, creates dust",
-		1,                       // stateNum
-		0,                       // toLocalAmt
-		100000,                  // toRemoteAmt
-		blobTypeCommitNoReward,  // blobType
-		227500,                  // sweepFeeRate
-		nil,                     // rewardScript
-		0,                       // expSweepAmt
-		0,                       // expRewardAmt
-		wtpolicy.ErrCreatesDust, // bindErr
-	),
-	genTaskTest(
-		"commit no-reward, no outputs, fee rate exceeds inputs",
-		300,                          // stateNum
-		0,                            // toLocalAmt
-		0,                            // toRemoteAmt
-		blobTypeCommitNoReward,       // blobType
-		1000,                         // sweepFeeRate
-		nil,                          // rewardScript
-		0,                            // expSweepAmt
-		0,                            // expRewardAmt
-		wtpolicy.ErrFeeExceedsInputs, // bindErr
-	),
-	genTaskTest(
-		"commit no-reward, no outputs, fee rate of 0 creates dust",
-		300,                     // stateNum
-		0,                       // toLocalAmt
-		0,                       // toRemoteAmt
-		blobTypeCommitNoReward,  // blobType
-		0,                       // sweepFeeRate
-		nil,                     // rewardScript
-		0,                       // expSweepAmt
-		0,                       // expRewardAmt
-		wtpolicy.ErrCreatesDust, // bindErr
-	),
-	genTaskTest(
-		"commit reward, both outputs",
-		100,                  // stateNum
-		200000,               // toLocalAmt
-		100000,               // toRemoteAmt
-		blobTypeCommitReward, // blobType
-		1000,                 // sweepFeeRate
-		addrScript,           // rewardScript
-		296117,               // expSweepAmt
-		3000,                 // expRewardAmt
-		nil,                  // bindErr
-	),
-	genTaskTest(
-		"commit reward, to-local output only",
-		1000,                 // stateNum
-		200000,               // toLocalAmt
-		0,                    // toRemoteAmt
-		blobTypeCommitReward, // blobType
-		1000,                 // sweepFeeRate
-		addrScript,           // rewardScript
-		197390,               // expSweepAmt
-		2000,                 // expRewardAmt
-		nil,                  // bindErr
-	),
-	genTaskTest(
-		"commit reward, to-remote output only",
-		1,                    // stateNum
-		0,                    // toLocalAmt
-		100000,               // toRemoteAmt
-		blobTypeCommitReward, // blobType
-		1000,                 // sweepFeeRate
-		addrScript,           // rewardScript
-		98437,                // expSweepAmt
-		1000,                 // expRewardAmt
-		nil,                  // bindErr
-	),
-	genTaskTest(
-		"commit reward, to-remote output only, creates dust",
-		1,                       // stateNum
-		0,                       // toLocalAmt
-		100000,                  // toRemoteAmt
-		blobTypeCommitReward,    // blobType
-		175000,                  // sweepFeeRate
-		addrScript,              // rewardScript
-		0,                       // expSweepAmt
-		0,                       // expRewardAmt
-		wtpolicy.ErrCreatesDust, // bindErr
-	),
-	genTaskTest(
-		"commit reward, no outputs, fee rate exceeds inputs",
-		300,                          // stateNum
-		0,                            // toLocalAmt
-		0,                            // toRemoteAmt
-		blobTypeCommitReward,         // blobType
-		1000,                         // sweepFeeRate
-		addrScript,                   // rewardScript
-		0,                            // expSweepAmt
-		0,                            // expRewardAmt
-		wtpolicy.ErrFeeExceedsInputs, // bindErr
-	),
-	genTaskTest(
-		"commit reward, no outputs, fee rate of 0 creates dust",
-		300,                     // stateNum
-		0,                       // toLocalAmt
-		0,                       // toRemoteAmt
-		blobTypeCommitReward,    // blobType
-		0,                       // sweepFeeRate
-		addrScript,              // rewardScript
-		0,                       // expSweepAmt
-		0,                       // expRewardAmt
-		wtpolicy.ErrCreatesDust, // bindErr
-	),
-}
-
 // TestBackupTaskBind tests the initialization and binding of a backupTask to a
 // ClientSession. After a successful bind, all parameters of the justice
 // transaction should be solidified, so we assert there correctness. In an
@ -390,8 +252,174 @@ var backupTaskTests = []backupTaskTest{
 func TestBackupTask(t *testing.T) {
 	t.Parallel()

+	var backupTaskTests []backupTaskTest
+	for _, tweakless := range []bool{true, false} {
+		backupTaskTests = append(backupTaskTests, []backupTaskTest{
+			genTaskTest(
+				"commit no-reward, both outputs",
+				100,                    // stateNum
+				200000,                 // toLocalAmt
+				100000,                 // toRemoteAmt
+				blobTypeCommitNoReward, // blobType
+				1000,                   // sweepFeeRate
+				nil,                    // rewardScript
+				299241,                 // expSweepAmt
+				0,                      // expRewardAmt
+				nil,                    // bindErr
+				tweakless,
+			),
+			genTaskTest(
+				"commit no-reward, to-local output only",
+				1000,                   // stateNum
+				200000,                 // toLocalAmt
+				0,                      // toRemoteAmt
+				blobTypeCommitNoReward, // blobType
+				1000,                   // sweepFeeRate
+				nil,                    // rewardScript
+				199514,                 // expSweepAmt
+				0,                      // expRewardAmt
+				nil,                    // bindErr
+				tweakless,
+			),
+			genTaskTest(
+				"commit no-reward, to-remote output only",
+				1,                      // stateNum
+				0,                      // toLocalAmt
+				100000,                 // toRemoteAmt
+				blobTypeCommitNoReward, // blobType
+				1000,                   // sweepFeeRate
+				nil,                    // rewardScript
+				99561,                  // expSweepAmt
+				0,                      // expRewardAmt
+				nil,                    // bindErr
+				tweakless,
+			),
+			genTaskTest(
+				"commit no-reward, to-remote output only, creates dust",
+				1,                       // stateNum
+				0,                       // toLocalAmt
+				100000,                  // toRemoteAmt
+				blobTypeCommitNoReward,  // blobType
+				227500,                  // sweepFeeRate
+				nil,                     // rewardScript
+				0,                       // expSweepAmt
+				0,                       // expRewardAmt
+				wtpolicy.ErrCreatesDust, // bindErr
+				tweakless,
+			),
+			genTaskTest(
+				"commit no-reward, no outputs, fee rate exceeds inputs",
+				300,                          // stateNum
+				0,                            // toLocalAmt
+				0,                            // toRemoteAmt
+				blobTypeCommitNoReward,       // blobType
+				1000,                         // sweepFeeRate
+				nil,                          // rewardScript
+				0,                            // expSweepAmt
+				0,                            // expRewardAmt
+				wtpolicy.ErrFeeExceedsInputs, // bindErr
+				tweakless,
+			),
+			genTaskTest(
+				"commit no-reward, no outputs, fee rate of 0 creates dust",
+				300,                     // stateNum
+				0,                       // toLocalAmt
+				0,                       // toRemoteAmt
+				blobTypeCommitNoReward,  // blobType
+				0,                       // sweepFeeRate
+				nil,                     // rewardScript
+				0,                       // expSweepAmt
+				0,                       // expRewardAmt
+				wtpolicy.ErrCreatesDust, // bindErr
+				tweakless,
+			),
+			genTaskTest(
+				"commit reward, both outputs",
+				100,                  // stateNum
+				200000,               // toLocalAmt
+				100000,               // toRemoteAmt
+				blobTypeCommitReward, // blobType
+				1000,                 // sweepFeeRate
+				addrScript,           // rewardScript
+				296117,               // expSweepAmt
+				3000,                 // expRewardAmt
+				nil,                  // bindErr
+				tweakless,
+			),
+			genTaskTest(
+				"commit reward, to-local output only",
+				1000,                 // stateNum
+				200000,               // toLocalAmt
+				0,                    // toRemoteAmt
+				blobTypeCommitReward, // blobType
+				1000,                 // sweepFeeRate
+				addrScript,           // rewardScript
+				197390,               // expSweepAmt
+				2000,                 // expRewardAmt
+				nil,                  // bindErr
+				tweakless,
+			),
+			genTaskTest(
+				"commit reward, to-remote output only",
+				1,                    // stateNum
+				0,                    // toLocalAmt
+				100000,               // toRemoteAmt
+				blobTypeCommitReward, // blobType
+				1000,                 // sweepFeeRate
+				addrScript,           // rewardScript
+				98437,                // expSweepAmt
+				1000,                 // expRewardAmt
+				nil,                  // bindErr
+				tweakless,
+			),
+			genTaskTest(
+				"commit reward, to-remote output only, creates dust",
+				1,                       // stateNum
+				0,                       // toLocalAmt
+				100000,                  // toRemoteAmt
+				blobTypeCommitReward,    // blobType
+				175000,                  // sweepFeeRate
+				addrScript,              // rewardScript
+				0,                       // expSweepAmt
+				0,                       // expRewardAmt
+				wtpolicy.ErrCreatesDust, // bindErr
+				tweakless,
+			),
+			genTaskTest(
+				"commit reward, no outputs, fee rate exceeds inputs",
+				300,                          // stateNum
+				0,                            // toLocalAmt
+				0,                            // toRemoteAmt
+				blobTypeCommitReward,         // blobType
+				1000,                         // sweepFeeRate
+				addrScript,                   // rewardScript
+				0,                            // expSweepAmt
+				0,                            // expRewardAmt
+				wtpolicy.ErrFeeExceedsInputs, // bindErr
+				tweakless,
+			),
+			genTaskTest(
+				"commit reward, no outputs, fee rate of 0 creates dust",
+				300,                     // stateNum
+				0,                       // toLocalAmt
+				0,                       // toRemoteAmt
+				blobTypeCommitReward,    // blobType
+				0,                       // sweepFeeRate
+				addrScript,              // rewardScript
+				0,                       // expSweepAmt
+				0,                       // expRewardAmt
+				wtpolicy.ErrCreatesDust, // bindErr
+				tweakless,
+			),
+		}...)
+	}
+
 	for _, test := range backupTaskTests {
+		test := test
+
 		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
 			testBackupTask(t, test)
 		})
 	}
@ -399,7 +427,10 @@ func TestBackupTask(t *testing.T) {

 func testBackupTask(t *testing.T, test backupTaskTest) {
 	// Create a new backupTask from the channel id and breach info.
-	task := newBackupTask(&test.chanID, test.breachInfo, test.expSweepScript)
+	task := newBackupTask(
+		&test.chanID, test.breachInfo, test.expSweepScript,
+		test.tweakless,
+	)

 	// Assert that all parameters set during initialization are properly
 	// populated.
--- a/watchtower/wtclient/client.go
+++ b/watchtower/wtclient/client.go
@ -90,8 +90,10 @@ type Client interface {
 	// state. If the method returns nil, the backup is guaranteed to be
 	// successful unless the client is force quit, or the justice
 	// transaction would create dust outputs when trying to abide by the
-	// negotiated policy.
-	BackupState(*lnwire.ChannelID, *lnwallet.BreachRetribution) error
+	// negotiated policy. If the channel we're trying to back up doesn't
+	// have a tweak for the remote party's output, then isTweakless should
+	// be true.
+	BackupState(*lnwire.ChannelID, *lnwallet.BreachRetribution, bool) error

 	// Start initializes the watchtower client, allowing it process requests
 	// to backup revoked channel states.
@ -564,7 +566,7 @@ func (c *TowerClient) RegisterChannel(chanID lnwire.ChannelID) error {
 //  - breached outputs contain too little value to sweep at the target sweep fee
 //    rate.
 func (c *TowerClient) BackupState(chanID *lnwire.ChannelID,
-	breachInfo *lnwallet.BreachRetribution) error {
+	breachInfo *lnwallet.BreachRetribution, isTweakless bool) error {

 	// Retrieve the cached sweep pkscript used for this channel.
 	c.backupMu.Lock()
@ -589,7 +591,9 @@ func (c *TowerClient) BackupState(chanID *lnwire.ChannelID,
 	c.chanCommitHeights[*chanID] = breachInfo.RevokedStateNum
 	c.backupMu.Unlock()

-	task := newBackupTask(chanID, breachInfo, summary.SweepPkScript)
+	task := newBackupTask(
+		chanID, breachInfo, summary.SweepPkScript, isTweakless,
+	)

 	return c.pipeline.QueueBackupTask(task)
 }
--- a/watchtower/wtclient/client_test.go
+++ b/watchtower/wtclient/client_test.go
@ -628,7 +628,7 @@ func (h *testHarness) backupState(id, i uint64, expErr error) {
 	_, retribution := h.channel(id).getState(i)

 	chanID := chanIDFromInt(id)
-	err := h.client.BackupState(&chanID, retribution)
+	err := h.client.BackupState(&chanID, retribution, false)
 	if err != expErr {
 		h.t.Fatalf("back error mismatch, want: %v, got: %v",
 			expErr, err)