Merge pull request #240 from joostjager/loopin-record-htlc

loopin: record htlx tx hash

loopdb/swapstate.go

@@ -60,6 +60,10 @@ const (
 	// StateInvoiceSettled means that the swap invoice has been paid by the
 	// server.
 	StateInvoiceSettled SwapState = 9
+
+	// StateFailIncorrectHtlcAmt indicates that the amount of an externally
+	// published loop in htlc didn't match the swap amount.
+	StateFailIncorrectHtlcAmt SwapState = 10
 )
 
 // SwapStateType defines the types of swap states that exist. Every swap state
@@ -127,6 +131,9 @@ func (s SwapState) String() string {
 	case StateInvoiceSettled:
 		return "InvoiceSettled"
 
+	case StateFailIncorrectHtlcAmt:
+		return "IncorrectHtlcAmt"
+
 	default:
 		return "Unknown"
 	}

loopin.go

@@ -9,17 +9,16 @@ import (
 
 	"github.com/btcsuite/btcutil"
 
-	"github.com/lightningnetwork/lnd/chainntnfs"
-	"github.com/lightningnetwork/lnd/channeldb"
-	"github.com/lightningnetwork/lnd/lnwire"
-
+	"github.com/btcsuite/btcd/chaincfg/chainhash"
 	"github.com/btcsuite/btcd/wire"
-	"github.com/lightningnetwork/lnd/lnrpc/invoicesrpc"
-
 	"github.com/lightninglabs/lndclient"
 	"github.com/lightninglabs/loop/loopdb"
 	"github.com/lightninglabs/loop/swap"
+	"github.com/lightningnetwork/lnd/chainntnfs"
+	"github.com/lightningnetwork/lnd/channeldb"
+	"github.com/lightningnetwork/lnd/lnrpc/invoicesrpc"
 	"github.com/lightningnetwork/lnd/lntypes"
+	"github.com/lightningnetwork/lnd/lnwire"
 )
 
 var (
@@ -58,6 +57,9 @@ type loopInSwap struct {
 
 	htlcNP2WSH *swap.Htlc
 
+	// htlcTxHash is the confirmed htlc tx id.
+	htlcTxHash *chainhash.Hash
+
 	timeoutAddr btcutil.Address
 }
 
@@ -209,6 +211,7 @@ func resumeLoopInSwap(reqContext context.Context, cfg *swapConfig,
 	} else {
 		swap.state = lastUpdate.State
 		swap.lastUpdateTime = lastUpdate.Time
+		swap.htlcTxHash = lastUpdate.HtlcTxHash
 	}
 
 	return swap, nil
@@ -333,7 +336,7 @@ func (s *loopInSwap) executeSwap(globalCtx context.Context) error {
 			// HtlcPublished state directly and wait for
 			// confirmation.
 			s.setState(loopdb.StateHtlcPublished)
-			err = s.persistState(globalCtx)
+			err = s.persistAndAnnounceState(globalCtx)
 			if err != nil {
 				return err
 			}
@@ -363,6 +366,13 @@ func (s *loopInSwap) executeSwap(globalCtx context.Context) error {
 		return err
 	}
 
+	// Verify that the confirmed (external) htlc value matches the swap
+	// amount. Otherwise fail the swap immediately.
+	if htlcValue != s.LoopInContract.AmountRequested {
+		s.setState(loopdb.StateFailIncorrectHtlcAmt)
+		return s.persistAndAnnounceState(globalCtx)
+	}
+
 	// TODO: Add miner fee of htlc tx to swap cost balance.
 
 	// The server is expected to see the htlc on-chain and knowing that it
@@ -376,7 +386,7 @@ func (s *loopInSwap) executeSwap(globalCtx context.Context) error {
 	}
 
 	// Persist swap outcome.
-	if err := s.persistState(globalCtx); err != nil {
+	if err := s.persistAndAnnounceState(globalCtx); err != nil {
 		return err
 	}
 
@@ -387,39 +397,53 @@ func (s *loopInSwap) executeSwap(globalCtx context.Context) error {
 func (s *loopInSwap) waitForHtlcConf(globalCtx context.Context) (
 	*chainntnfs.TxConfirmation, error) {
 
+	// Register for confirmation of the htlc. It is essential to specify not
+	// just the pk script, because an attacker may publish the same htlc
+	// with a lower value and we don't want to follow through with that tx.
+	// In the unlikely event that our call to SendOutputs crashes and we
+	// restart, htlcTxHash will be nil at this point. Then only register
+	// with PkScript and accept the risk that the call triggers on a
+	// different htlc outpoint.
+	s.log.Infof("Register for htlc conf (hh=%v, txid=%v)",
+		s.InitiationHeight, s.htlcTxHash)
+
+	if s.htlcTxHash == nil {
+		s.log.Warnf("No htlc tx hash available, registering with " +
+			"just the pkscript")
+	}
+
 	ctx, cancel := context.WithCancel(globalCtx)
 	defer cancel()
 
 	notifier := s.lnd.ChainNotifier
 
 	confChanP2WSH, confErrP2WSH, err := notifier.RegisterConfirmationsNtfn(
-		ctx, nil, s.htlcP2WSH.PkScript, 1, s.InitiationHeight,
+		ctx, s.htlcTxHash, s.htlcP2WSH.PkScript, 1, s.InitiationHeight,
 	)
 	if err != nil {
 		return nil, err
 	}
 
 	confChanNP2WSH, confErrNP2WSH, err := notifier.RegisterConfirmationsNtfn(
-		ctx, nil, s.htlcNP2WSH.PkScript, 1, s.InitiationHeight,
+		ctx, s.htlcTxHash, s.htlcNP2WSH.PkScript, 1, s.InitiationHeight,
 	)
 	if err != nil {
 		return nil, err
 	}
 
-	for {
+	var conf *chainntnfs.TxConfirmation
+	for conf == nil {
 		select {
 
 		// P2WSH htlc confirmed.
-		case conf := <-confChanP2WSH:
+		case conf = <-confChanP2WSH:
 			s.htlc = s.htlcP2WSH
 			s.log.Infof("P2WSH htlc confirmed")
-			return conf, nil
 
 		// NP2WSH htlc confirmed.
-		case conf := <-confChanNP2WSH:
+		case conf = <-confChanNP2WSH:
 			s.htlc = s.htlcNP2WSH
 			s.log.Infof("NP2WSH htlc confirmed")
-			return conf, nil
 
 		// Conf ntfn error.
 		case err := <-confErrP2WSH:
@@ -438,6 +462,19 @@ func (s *loopInSwap) waitForHtlcConf(globalCtx context.Context) (
 			return nil, globalCtx.Err()
 		}
 	}
+
+	// Store htlc tx hash for accounting purposes. Usually this call is a
+	// no-op because the htlc tx hash was already known. Exceptions are:
+	//
+	// - Old pending swaps that were initiated before we persisted the htlc
+	//   tx hash directly after publish.
+	//
+	// - Swaps that experienced a crash during their call to SendOutputs. In
+	//   that case, we weren't able to record the tx hash.
+	txHash := conf.Tx.TxHash()
+	s.htlcTxHash = &txHash
+
+	return conf, nil
 }
 
 // publishOnChainHtlc checks whether there are still enough blocks left and if
@@ -451,7 +488,7 @@ func (s *loopInSwap) publishOnChainHtlc(ctx context.Context) (bool, error) {
 	// Verify whether it still makes sense to publish the htlc.
 	if blocksRemaining < MinLoopInPublishDelta {
 		s.setState(loopdb.StateFailTimeout)
-		return false, s.persistState(ctx)
+		return false, s.persistAndAnnounceState(ctx)
 	}
 
 	// Get fee estimate from lnd.
@@ -465,7 +502,7 @@ func (s *loopInSwap) publishOnChainHtlc(ctx context.Context) (bool, error) {
 	// Transition to state HtlcPublished before calling SendOutputs to
 	// prevent us from ever paying multiple times after a crash.
 	s.setState(loopdb.StateHtlcPublished)
-	err = s.persistState(ctx)
+	err = s.persistAndAnnounceState(ctx)
 	if err != nil {
 		return false, err
 	}
@@ -483,7 +520,20 @@ func (s *loopInSwap) publishOnChainHtlc(ctx context.Context) (bool, error) {
 	if err != nil {
 		return false, fmt.Errorf("send outputs: %v", err)
 	}
-	s.log.Infof("Published on chain HTLC tx %v", tx.TxHash())
+	txHash := tx.TxHash()
+	s.log.Infof("Published on chain HTLC tx %v", txHash)
+
+	// Persist the htlc hash so that after a restart we are still waiting
+	// for our own htlc. We don't need to announce to clients, because the
+	// state remains unchanged.
+	//
+	// TODO(joostjager): Store tx hash before calling SendOutputs. This is
+	// not yet possible with the current lnd api.
+	s.htlcTxHash = &txHash
+	s.lastUpdateTime = time.Now()
+	if err := s.persistState(); err != nil {
+		return false, fmt.Errorf("persist htlc tx: %v", err)
+	}
 
 	return true, nil
 
@@ -499,7 +549,7 @@ func (s *loopInSwap) waitForSwapComplete(ctx context.Context,
 	rpcCtx, cancel := context.WithCancel(ctx)
 	defer cancel()
 	spendChan, spendErr, err := s.lnd.ChainNotifier.RegisterSpendNtfn(
-		rpcCtx, nil, s.htlc.PkScript, s.InitiationHeight,
+		rpcCtx, htlcOutpoint, s.htlc.PkScript, s.InitiationHeight,
 	)
 	if err != nil {
 		return fmt.Errorf("register spend ntfn: %v", err)
@@ -589,7 +639,7 @@ func (s *loopInSwap) waitForSwapComplete(ctx context.Context,
 				// accounting data.
 				if s.state == loopdb.StateHtlcPublished {
 					s.setState(loopdb.StateInvoiceSettled)
-					err := s.persistState(ctx)
+					err := s.persistAndAnnounceState(ctx)
 					if err != nil {
 						return err
 					}
@@ -689,17 +739,11 @@ func (s *loopInSwap) publishTimeoutTx(ctx context.Context,
 	return nil
 }
 
-// persistState updates the swap state and sends out an update notification.
-func (s *loopInSwap) persistState(ctx context.Context) error {
+// persistAndAnnounceState updates the swap state on disk and sends out an
+// update notification.
+func (s *loopInSwap) persistAndAnnounceState(ctx context.Context) error {
 	// Update state in store.
-	err := s.store.UpdateLoopIn(
-		s.hash, s.lastUpdateTime,
-		loopdb.SwapStateData{
-			State: s.state,
-			Cost:  s.cost,
-		},
-	)
-	if err != nil {
+	if err := s.persistState(); err != nil {
 		return err
 	}
 
@@ -707,6 +751,18 @@ func (s *loopInSwap) persistState(ctx context.Context) error {
 	return s.sendUpdate(ctx)
 }
 
+// persistState updates the swap state on disk.
+func (s *loopInSwap) persistState() error {
+	return s.store.UpdateLoopIn(
+		s.hash, s.lastUpdateTime,
+		loopdb.SwapStateData{
+			State:      s.state,
+			Cost:       s.cost,
+			HtlcTxHash: s.htlcTxHash,
+		},
+	)
+}
+
 // setState updates the swap state and last update timestamp.
 func (s *loopInSwap) setState(state loopdb.SwapState) {
 	s.lastUpdateTime = time.Now()

loopin_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/lightninglabs/loop/test"
 	"github.com/lightningnetwork/lnd/chainntnfs"
 	"github.com/lightningnetwork/lnd/channeldb"
+	"github.com/stretchr/testify/require"
 
 	"github.com/btcsuite/btcd/wire"
 	"github.com/btcsuite/btcutil"
@@ -61,6 +62,10 @@ func TestLoopInSuccess(t *testing.T) {
 	// Expect htlc to be published.
 	htlcTx := <-ctx.lnd.SendOutputsChannel
 
+	// Expect the same state to be written again with the htlc tx hash.
+	state := ctx.store.assertLoopInState(loopdb.StateHtlcPublished)
+	require.NotNil(t, state.HtlcTxHash)
+
 	// Expect register for htlc conf.
 	<-ctx.lnd.RegisterConfChannel
 	<-ctx.lnd.RegisterConfChannel
@@ -182,6 +187,10 @@ func testLoopInTimeout(t *testing.T,
 	if externalValue == 0 {
 		// Expect htlc to be published.
 		htlcTx = <-ctx.lnd.SendOutputsChannel
+
+		// Expect the same state to be written again with the htlc tx hash.
+		state := ctx.store.assertLoopInState(loopdb.StateHtlcPublished)
+		require.NotNil(t, state.HtlcTxHash)
 	} else {
 		// Create an external htlc publish tx.
 		var pkScript []byte
@@ -209,6 +218,20 @@ func testLoopInTimeout(t *testing.T,
 		Tx: &htlcTx,
 	}
 
+	// Assert that the swap is failed in case of an invalid amount.
+	invalidAmt := externalValue != 0 && externalValue != int64(req.Amount)
+	if invalidAmt {
+		ctx.assertState(loopdb.StateFailIncorrectHtlcAmt)
+		ctx.store.assertLoopInState(loopdb.StateFailIncorrectHtlcAmt)
+
+		err = <-errChan
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		return
+	}
+
 	// Client starts listening for spend of htlc.
 	<-ctx.lnd.RegisterSpendChannel
 
@@ -375,11 +398,17 @@ func testLoopInResume(t *testing.T, state loopdb.SwapState, expired bool) {
 
 		// Expect htlc to be published.
 		htlcTx = <-ctx.lnd.SendOutputsChannel
+
+		// Expect the same state to be written again with the htlc tx
+		// hash.
+		state := ctx.store.assertLoopInState(loopdb.StateHtlcPublished)
+		require.NotNil(t, state.HtlcTxHash)
 	} else {
 		ctx.assertState(loopdb.StateHtlcPublished)
 
 		htlcTx.AddTxOut(&wire.TxOut{
 			PkScript: htlc.PkScript,
+			Value:    int64(contract.AmountRequested),
 		})
 	}
 

store_mock_test.go

@@ -215,13 +215,19 @@ func (s *storeMock) assertLoopInStored() {
 	<-s.loopInStoreChan
 }
 
-func (s *storeMock) assertLoopInState(expectedState loopdb.SwapState) {
+// assertLoopInState asserts that a specified state transition is persisted to
+// disk.
+func (s *storeMock) assertLoopInState(
+	expectedState loopdb.SwapState) loopdb.SwapStateData {
+
 	s.t.Helper()
 
 	state := <-s.loopInUpdateChan
 	if state.State != expectedState {
 		s.t.Fatalf("expected state %v, got %v", expectedState, state)
 	}
+
+	return state
 }
 
 func (s *storeMock) assertStorePreimageReveal() {