From 4b65aea306eaba0a42232593840b072bb41a0bde Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 7 Aug 2019 19:49:59 -0700 Subject: [PATCH] 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. --- htlcswitch/interfaces.go | 6 +- htlcswitch/link.go | 9 +- htlcswitch/linkfailure.go | 7 + watchtower/lookout/justice_descriptor_test.go | 2 +- watchtower/wtclient/backup_task.go | 12 +- .../wtclient/backup_task_internal_test.go | 331 ++++++++++-------- watchtower/wtclient/client.go | 12 +- watchtower/wtclient/client_test.go | 2 +- 8 files changed, 219 insertions(+), 162 deletions(-) diff --git a/htlcswitch/interfaces.go b/htlcswitch/interfaces.go index d4bcc0a8..5e162795 100644 --- 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 } diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 351a7623..0539bde1 100644 --- 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) diff --git a/htlcswitch/linkfailure.go b/htlcswitch/linkfailure.go index 2f44e434..c806c4b2 100644 --- 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" } diff --git a/watchtower/lookout/justice_descriptor_test.go b/watchtower/lookout/justice_descriptor_test.go index 1f880c23..7fd93c8c 100644 --- 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) diff --git a/watchtower/wtclient/backup_task.go b/watchtower/wtclient/backup_task.go index 9f284b98..abdabe2f 100644 --- 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[:]) } diff --git a/watchtower/wtclient/backup_task_internal_test.go b/watchtower/wtclient/backup_task_internal_test.go index 5bc0dbb4..cff6f2b8 100644 --- 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. diff --git a/watchtower/wtclient/client.go b/watchtower/wtclient/client.go index 795f1798..76aa2a4b 100644 --- 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) } diff --git a/watchtower/wtclient/client_test.go b/watchtower/wtclient/client_test.go index e9019ad9..bd16ac06 100644 --- 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)