loopout: do not reveal preimage if time to safe reveal has passed

Author: carlaKC

loopout.go

@@ -438,6 +438,12 @@ func (s *loopOutSwap) executeSwap(globalCtx context.Context) error {
 		return err
 	}
 
+	// If spend details are nil, we resolved the swap without waiting for
+	// its spend, so we can exit.
+	if spendDetails == nil {
+		return nil
+	}
+
 	// Inspect witness stack to see if it is a success transaction. We
 	// don't just try to match with the hash of our sweep tx, because it
 	// may be swept by a different (fee) sweep tx from a previous run.
@@ -854,6 +860,14 @@ func (s *loopOutSwap) waitForHtlcSpendConfirmed(globalCtx context.Context,
 				return nil, err
 			}
 
+			// If the result of our spend func was that the swap
+			// has reached a final state, then we return nil spend
+			// details, because there is no further action required
+			// for this swap.
+			if s.state.Type() != loopdb.StateTypePending {
+				return nil, nil
+			}
+
 			// If our off chain payment is not yet complete, we
 			// try to push our preimage to the server.
 			if !paymentComplete {
@@ -889,7 +903,9 @@ func (s *loopOutSwap) pushPreimage(ctx context.Context) {
 
 // sweep tries to sweep the given htlc to a destination address. It takes into
 // account the max miner fee and marks the preimage as revealed when it
-// published the tx.
+// published the tx. If the preimage has not yet been revealed, and the time
+// during which we can safely reveal it has passed, the swap will be marked
+// as failed, and the function will return.
 //
 // TODO: Use lnd sweeper?
 func (s *loopOutSwap) sweep(ctx context.Context,
@@ -900,16 +916,36 @@ func (s *loopOutSwap) sweep(ctx context.Context,
 		return s.htlc.GenSuccessWitness(sig, s.Preimage)
 	}
 
+	remainingBlocks := s.CltvExpiry - s.height
+	blocksToLastReveal := remainingBlocks - MinLoopOutPreimageRevealDelta
+	preimageRevealed := s.state == loopdb.StatePreimageRevealed
+
+	// If we have not revealed our preimage, and we don't have time left
+	// to sweep the swap, we abandon the swap because we can no longer
+	// sweep the success path (without potentially having to compete with
+	// the server's timeout sweep), and we have not had any coins pulled
+	// off-chain.
+	if blocksToLastReveal <= 0 && !preimageRevealed {
+		s.log.Infof("Preimage can no longer be safely revealed: "+
+			"expires at: %v, current height: %v", s.CltvExpiry,
+			s.height)
+
+		s.state = loopdb.StateFailTimeout
+		return nil
+	}
+
 	// Calculate the transaction fee based on the confirmation target
 	// required to sweep the HTLC before the timeout. We'll use the
 	// confirmation target provided by the client unless we've come too
 	// close to the expiration height, in which case we'll use the default
 	// if it is better than what the client provided.
 	confTarget := s.SweepConfTarget
-	if s.CltvExpiry-s.height <= DefaultSweepConfTargetDelta &&
+	if remainingBlocks <= DefaultSweepConfTargetDelta &&
 		confTarget > DefaultSweepConfTarget {
+
 		confTarget = DefaultSweepConfTarget
 	}
+
 	fee, err := s.sweeper.GetSweepFee(
 		ctx, s.htlc.AddSuccessToEstimator, s.DestAddr, confTarget,
 	)
@@ -922,7 +958,7 @@ func (s *loopOutSwap) sweep(ctx context.Context,
 		s.log.Warnf("Required fee %v exceeds max miner fee of %v",
 			fee, s.MaxMinerFee)
 
-		if s.state == loopdb.StatePreimageRevealed {
+		if preimageRevealed {
 			// The currently required fee exceeds the max, but we
 			// already revealed the preimage. The best we can do now
 			// is to republish with the max fee.

loopout_test.go

@@ -583,8 +583,8 @@ func TestPreimagePush(t *testing.T) {
 }
 
 // TestExpiryBeforeReveal tests the case where the on-chain HTLC expires before
-// we have revealed our preimage. This test demonstrates that the client will
-// erroneously reveal the preimage even though we're nearing our timeout.
+// we have revealed our preimage, demonstrating that we do not reveal our
+// preimage once we've reached our expiry height.
 func TestExpiryBeforeReveal(t *testing.T) {
 	defer test.Guard(t)()
 
@@ -622,9 +622,8 @@ func TestExpiryBeforeReveal(t *testing.T) {
 	}
 
 	errChan := make(chan error)
-	cancelCtx, cancel := context.WithCancel(context.Background())
 	go func() {
-		err := swap.execute(cancelCtx, &executeConfig{
+		err := swap.execute(context.Background(), &executeConfig{
 			statusChan:     statusChan,
 			blockEpochChan: blockEpochChan,
 			timerFactory:   timerFactory,
@@ -653,7 +652,8 @@ func TestExpiryBeforeReveal(t *testing.T) {
 	ctx.AssertRegisterConf(false, defaultConfirmations)
 
 	// Advance the block height to get the HTLC confirmed.
-	blockEpochChan <- ctx.Lnd.Height + 1
+	height := ctx.Lnd.Height + 1
+	blockEpochChan <- height
 
 	htlcTx := wire.NewMsgTx(2)
 	htlcTx.AddTxOut(&wire.TxOut{
@@ -681,17 +681,19 @@ func TestExpiryBeforeReveal(t *testing.T) {
 
 	// Advance the block height to the point where we would do timeout
 	// instead of pushing the preimage.
-	blockEpochChan <- lnd.Height + testReq.Expiry
+	blockEpochChan <- testReq.Expiry + height
 
-	// Tick our expiry chan again, this time we expect the swap to
-	// publish our sweep timeout, despite expiry having passed, and the
-	// potential for a race with the server.
+	// Tick our expiry channel again to trigger another sweep attempt.
 	expiryChan <- testTime
 
-	// Expect a signing request for the HTLC success transaction.
-	<-ctx.Lnd.SignOutputRawChannel
+	// We should see our swap marked as failed.
+	cfg.store.(*storeMock).assertLoopOutState(
+		loopdb.StateFailTimeout,
+	)
+	status := <-statusChan
+	require.Equal(
+		t, status.State, loopdb.StateFailTimeout,
+	)
 
-	// We just cancel the swap now rather than testing to completion.
-	cancel()
-	require.Equal(t, context.Canceled, <-errChan)
+	require.Nil(t, <-errChan)
 }