From ab9a662758eb2abc46fa9d5ac43a257dd78efd8a Mon Sep 17 00:00:00 2001 From: carla Date: Thu, 22 Apr 2021 09:28:32 +0200 Subject: [PATCH 1/3] loop/test: simplify preimage push test to be less dependent on height Our preimage push test previously relied on our dropping down to the default sweep conf target to mock a drop in chain fees. This makes our test dependent on height, which makes changes to our sweep logic regarding when we reveal our preimage break this test. In this commit that logic is replaced with simply locking our mock and updating fees on the fly. --- loopout_test.go | 32 ++++++++++---------------------- test/lnd_services_mock.go | 2 +- test/walletkit_mock.go | 19 ++++++++++++++++--- 3 files changed, 27 insertions(+), 26 deletions(-) diff --git a/loopout_test.go b/loopout_test.go index 0e72933..a8faaf8 100644 --- a/loopout_test.go +++ b/loopout_test.go @@ -414,11 +414,7 @@ func TestCustomSweepConfTarget(t *testing.T) { // not detected our settle) and settle the off chain htlc, indicating that the // server successfully settled using the preimage push. In this test, we need // to start with a fee rate that will be too high, then progress to an -// acceptable one. We do this by starting with a high confirmation target with -// a high fee, and setting the default confirmation fee (which our swap will -// drop down to if it is not confirming in time) to a lower fee. This is not -// intuitive (lower confs having lower fees), but it allows up to mock fee -// changes. +// acceptable one. func TestPreimagePush(t *testing.T) { defer test.Guard(t)() @@ -426,11 +422,8 @@ func TestPreimagePush(t *testing.T) { ctx := test.NewContext(t, lnd) server := newServerMock(lnd) - // Start with a high confirmation delta which will have a very high fee - // attached to it. testReq := *testRequest - testReq.SweepConfTarget = testLoopOutMinOnChainCltvDelta - - DefaultSweepConfTargetDelta - 1 + testReq.SweepConfTarget = 10 testReq.Expiry = ctx.Lnd.Height + testLoopOutMinOnChainCltvDelta // We set our mock fee estimate for our target sweep confs to be our @@ -442,11 +435,6 @@ func TestPreimagePush(t *testing.T) { ), ) - // We set the fee estimate for our default confirmation target very - // low, so that once we drop down to our default confs we will start - // trying to sweep the preimage. - ctx.Lnd.SetFeeEstimate(DefaultSweepConfTarget, 1) - cfg := newSwapConfig( &lnd.LndServices, newStoreMock(t), server, ) @@ -520,15 +508,15 @@ func TestPreimagePush(t *testing.T) { // preimage is not revealed, we also do not expect a preimage push. expiryChan <- testTime - // Now, we notify the height at which the client will start using the - // default confirmation target. This has the effect of lowering our fees - // so that the client still start sweeping. - defaultConfTargetHeight := ctx.Lnd.Height + testLoopOutMinOnChainCltvDelta - - DefaultSweepConfTargetDelta - blockEpochChan <- defaultConfTargetHeight + // Now we decrease our fees for the swap's confirmation target to less + // than the maximum miner fee. + ctx.Lnd.SetFeeEstimate(testReq.SweepConfTarget, chainfee.SatPerKWeight( + testReq.MaxMinerFee/2, + )) - // This time when we tick the expiry chan, our fees are lower than the - // swap max, so we expect it to prompt a sweep. + // Now when we report a new block and tick our expiry fee timer, and + // fees are acceptably low so we expect our sweep to be published. + blockEpochChan <- ctx.Lnd.Height + 2 expiryChan <- testTime // Expect a signing request for the HTLC success transaction. diff --git a/test/lnd_services_mock.go b/test/lnd_services_mock.go index 7e162a2..7d3466c 100644 --- a/test/lnd_services_mock.go +++ b/test/lnd_services_mock.go @@ -258,5 +258,5 @@ func (s *LndMockServices) DecodeInvoice(request string) (*zpay32.Invoice, func (s *LndMockServices) SetFeeEstimate(confTarget int32, feeEstimate chainfee.SatPerKWeight) { - s.WalletKit.(*mockWalletKit).feeEstimates[confTarget] = feeEstimate + s.WalletKit.(*mockWalletKit).setFeeEstimate(confTarget, feeEstimate) } diff --git a/test/walletkit_mock.go b/test/walletkit_mock.go index b961162..19738a8 100644 --- a/test/walletkit_mock.go +++ b/test/walletkit_mock.go @@ -3,6 +3,7 @@ package test import ( "context" "errors" + "sync" "time" "github.com/btcsuite/btcd/chaincfg" @@ -21,9 +22,11 @@ import ( var DefaultMockFee = chainfee.SatPerKWeight(10000) type mockWalletKit struct { - lnd *LndMockServices - keyIndex int32 - feeEstimates map[int32]chainfee.SatPerKWeight + lnd *LndMockServices + keyIndex int32 + + feeEstimateLock sync.Mutex + feeEstimates map[int32]chainfee.SatPerKWeight } var _ lndclient.WalletKitClient = (*mockWalletKit)(nil) @@ -118,9 +121,19 @@ func (m *mockWalletKit) SendOutputs(ctx context.Context, outputs []*wire.TxOut, return &tx, nil } +func (m *mockWalletKit) setFeeEstimate(confTarget int32, fee chainfee.SatPerKWeight) { + m.feeEstimateLock.Lock() + defer m.feeEstimateLock.Unlock() + + m.feeEstimates[confTarget] = fee +} + func (m *mockWalletKit) EstimateFee(ctx context.Context, confTarget int32) ( chainfee.SatPerKWeight, error) { + m.feeEstimateLock.Lock() + defer m.feeEstimateLock.Unlock() + if confTarget <= 1 { return 0, errors.New("conf target must be greater than 1") } From 8d4404a8fb5c48dfa6615418681408e3bc3eeb9c Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 22 Apr 2021 09:28:35 +0200 Subject: [PATCH 2/3] loopout: add test for checking preimage reveal after timeout --- loopout_test.go | 114 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/loopout_test.go b/loopout_test.go index a8faaf8..c3a189b 100644 --- a/loopout_test.go +++ b/loopout_test.go @@ -581,3 +581,117 @@ func TestPreimagePush(t *testing.T) { require.NoError(t, <-errChan) } + +// 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. +func TestExpiryBeforeReveal(t *testing.T) { + defer test.Guard(t)() + + lnd := test.NewMockLnd() + ctx := test.NewContext(t, lnd) + server := newServerMock(lnd) + + testReq := *testRequest + + // Set on-chain HTLC CLTV. + testReq.Expiry = ctx.Lnd.Height + testLoopOutMinOnChainCltvDelta + + // Set our fee estimate to higher than our max miner fee will allow. + lnd.SetFeeEstimate(testReq.SweepConfTarget, chainfee.SatPerKWeight( + testReq.MaxMinerFee*2, + )) + + // Setup the cfg using mock server and init a loop out request. + cfg := newSwapConfig( + &lnd.LndServices, newStoreMock(t), server, + ) + initResult, err := newLoopOutSwap( + context.Background(), cfg, ctx.Lnd.Height, &testReq, + ) + require.NoError(t, err) + swap := initResult.swap + + // Set up the required dependencies to execute the swap. + sweeper := &sweep.Sweeper{Lnd: &lnd.LndServices} + blockEpochChan := make(chan interface{}) + statusChan := make(chan SwapInfo) + expiryChan := make(chan time.Time) + timerFactory := func(_ time.Duration) <-chan time.Time { + return expiryChan + } + + errChan := make(chan error) + cancelCtx, cancel := context.WithCancel(context.Background()) + go func() { + err := swap.execute(cancelCtx, &executeConfig{ + statusChan: statusChan, + blockEpochChan: blockEpochChan, + timerFactory: timerFactory, + sweeper: sweeper, + }, ctx.Lnd.Height) + if err != nil { + log.Error(err) + } + errChan <- err + }() + + // The swap should be found in its initial state. + cfg.store.(*storeMock).assertLoopOutStored() + state := <-statusChan + require.Equal(t, loopdb.StateInitiated, state.State) + + // We'll then pay both the swap and prepay invoice, which should trigger + // the server to publish the on-chain HTLC. + signalSwapPaymentResult := ctx.AssertPaid(swapInvoiceDesc) + signalPrepaymentResult := ctx.AssertPaid(prepayInvoiceDesc) + + signalSwapPaymentResult(nil) + signalPrepaymentResult(nil) + + // Notify the confirmation notification for the HTLC. + ctx.AssertRegisterConf(false, defaultConfirmations) + + // Advance the block height to get the HTLC confirmed. + blockEpochChan <- ctx.Lnd.Height + 1 + + htlcTx := wire.NewMsgTx(2) + htlcTx.AddTxOut(&wire.TxOut{ + Value: int64(swap.AmountRequested), + PkScript: swap.htlc.PkScript, + }) + ctx.NotifyConf(htlcTx) + + // The client should then register for a spend of the HTLC and attempt + // to sweep it using the custom confirmation target. + ctx.AssertRegisterSpendNtfn(swap.htlc.PkScript) + + // Assert that we made a query to track our payment, as required for + // preimage push tracking. + ctx.AssertTrackPayment() + + // Tick the expiry channel. Because our max miner fee is too high, we + // won't attempt a sweep at this point. + expiryChan <- testTime + + // Now we decrease our conf target to less than our max miner fee. + lnd.SetFeeEstimate(testReq.SweepConfTarget, chainfee.SatPerKWeight( + testReq.MaxMinerFee/2, + )) + + // Advance the block height to the point where we would do timeout + // instead of pushing the preimage. + blockEpochChan <- lnd.Height + testReq.Expiry + + // 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. + expiryChan <- testTime + + // Expect a signing request for the HTLC success transaction. + <-ctx.Lnd.SignOutputRawChannel + + // We just cancel the swap now rather than testing to completion. + cancel() + require.Equal(t, context.Canceled, <-errChan) +} From 9db8bd5f2a5817218c5128d5e65af4391b7da7bf Mon Sep 17 00:00:00 2001 From: carla Date: Wed, 12 May 2021 14:09:28 +0200 Subject: [PATCH 3/3] loopout: do not reveal preimage if time to safe reveal has passed --- loopout.go | 42 +++++++++++++++++++++++++++++++++++++++--- loopout_test.go | 30 ++++++++++++++++-------------- 2 files changed, 55 insertions(+), 17 deletions(-) diff --git a/loopout.go b/loopout.go index b930354..a90365e 100644 --- a/loopout.go +++ b/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. diff --git a/loopout_test.go b/loopout_test.go index c3a189b..1efb9d6 100644 --- a/loopout_test.go +++ b/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) }