From 60b907a34f79ec00b6ce5b70460a4c4508cc62d6 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Thu, 25 Jun 2020 12:11:17 +0200 Subject: [PATCH 1/5] loopin: refactor htlc conf event loop --- loopin.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/loopin.go b/loopin.go index ab82c47..24a5337 100644 --- a/loopin.go +++ b/loopin.go @@ -406,20 +406,19 @@ func (s *loopInSwap) waitForHtlcConf(globalCtx context.Context) ( 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 +437,8 @@ func (s *loopInSwap) waitForHtlcConf(globalCtx context.Context) ( return nil, globalCtx.Err() } } + + return conf, nil } // publishOnChainHtlc checks whether there are still enough blocks left and if From 6b8fcc547cda08f826860151d75765b2fb7b8439 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Thu, 25 Jun 2020 13:14:39 +0200 Subject: [PATCH 2/5] loopin: rename to persistAndAnnounceState --- loopin.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/loopin.go b/loopin.go index 24a5337..2ee6fa9 100644 --- a/loopin.go +++ b/loopin.go @@ -333,7 +333,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 } @@ -376,7 +376,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 } @@ -452,7 +452,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. @@ -466,7 +466,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 } @@ -590,7 +590,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 } @@ -690,8 +690,9 @@ 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, From e33d2fb7628497fd9f774a23ebe280f6d96ea06b Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Thu, 25 Jun 2020 13:15:58 +0200 Subject: [PATCH 3/5] loopin: extract persistState --- loopin.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/loopin.go b/loopin.go index 2ee6fa9..bc0fcc3 100644 --- a/loopin.go +++ b/loopin.go @@ -694,19 +694,23 @@ func (s *loopInSwap) publishTimeoutTx(ctx context.Context, // update notification. func (s *loopInSwap) persistAndAnnounceState(ctx context.Context) error { // Update state in store. - err := s.store.UpdateLoopIn( + if err := s.persistState(); err != nil { + return err + } + + // Send out swap update + 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, }, ) - if err != nil { - return err - } - - // Send out swap update - return s.sendUpdate(ctx) } // setState updates the swap state and last update timestamp. From a3b7fa5977e054c9fb7b88126393bd15af1ed024 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Thu, 25 Jun 2020 13:55:27 +0200 Subject: [PATCH 4/5] loopin: fail swap when htlc amount is incorrect Previously the swap would get stuck in a state where it wouldn't ever progress because the server rejected the htlc. --- loopdb/swapstate.go | 7 +++++++ loopin.go | 7 +++++++ loopin_test.go | 15 +++++++++++++++ 3 files changed, 29 insertions(+) diff --git a/loopdb/swapstate.go b/loopdb/swapstate.go index 87c21f3..c2cddb2 100644 --- a/loopdb/swapstate.go +++ b/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" } diff --git a/loopin.go b/loopin.go index bc0fcc3..40495e2 100644 --- a/loopin.go +++ b/loopin.go @@ -363,6 +363,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 diff --git a/loopin_test.go b/loopin_test.go index 9765d76..51d52cf 100644 --- a/loopin_test.go +++ b/loopin_test.go @@ -209,6 +209,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 @@ -380,6 +394,7 @@ func testLoopInResume(t *testing.T, state loopdb.SwapState, expired bool) { htlcTx.AddTxOut(&wire.TxOut{ PkScript: htlc.PkScript, + Value: int64(contract.AmountRequested), }) } From f91422d1883679e9764ad3a14fc6a6294aea6636 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Thu, 25 Jun 2020 12:35:22 +0200 Subject: [PATCH 5/5] loopin: record htlx tx hash --- loopin.go | 67 +++++++++++++++++++++++++++++++++++++--------- loopin_test.go | 14 ++++++++++ store_mock_test.go | 8 +++++- 3 files changed, 76 insertions(+), 13 deletions(-) diff --git a/loopin.go b/loopin.go index 40495e2..4936501 100644 --- a/loopin.go +++ b/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 @@ -394,20 +397,35 @@ 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 @@ -445,6 +463,17 @@ func (s *loopInSwap) waitForHtlcConf(globalCtx context.Context) ( } } + // 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 } @@ -491,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 @@ -507,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) @@ -714,8 +756,9 @@ func (s *loopInSwap) persistState() error { return s.store.UpdateLoopIn( s.hash, s.lastUpdateTime, loopdb.SwapStateData{ - State: s.state, - Cost: s.cost, + State: s.state, + Cost: s.cost, + HtlcTxHash: s.htlcTxHash, }, ) } diff --git a/loopin_test.go b/loopin_test.go index 51d52cf..ceda3e1 100644 --- a/loopin_test.go +++ b/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 @@ -389,6 +398,11 @@ 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) diff --git a/store_mock_test.go b/store_mock_test.go index 9d932a8..77f36c3 100644 --- a/store_mock_test.go +++ b/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() {