From ea2b706174af45b7f213e50112a8c6b5b848a8dc Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 17 Jul 2018 09:13:05 +0200 Subject: [PATCH 01/15] lntest/node: make sure finalizing file is over before stop --- lntest/node.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lntest/node.go b/lntest/node.go index 883a248d..a33e0e29 100644 --- a/lntest/node.go +++ b/lntest/node.go @@ -335,9 +335,11 @@ func (hn *HarnessNode) start(lndError chan<- error) error { // Launch a new goroutine which that bubbles up any potential fatal // process errors to the goroutine running the tests. hn.processExit = make(chan struct{}) + hn.wg.Add(1) go func() { - err := hn.cmd.Wait() + defer hn.wg.Done() + err := hn.cmd.Wait() if err != nil { lndError <- errors.Errorf("%v\n%v\n", err, errb.String()) } From fdf3b407c9f55d4f55e928a65ababb1380cf2b92 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 17 Jul 2018 09:13:05 +0200 Subject: [PATCH 02/15] lnd_test: correct node names in comments, format file --- lnd_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lnd_test.go b/lnd_test.go index f3a34503..7e401ad7 100644 --- a/lnd_test.go +++ b/lnd_test.go @@ -1771,7 +1771,7 @@ func testChannelForceClosure(net *lntest.NetworkHarness, t *harnessTest) { t.Fatalf("htlc mismatch: %v", err) } - // As we'll be querying the state of Carol's channels frequently we'll + // As we'll be querying the state of Alice's channels frequently we'll // create a closure helper function for the purpose. getAliceChanInfo := func() (*lnrpc.Channel, error) { req := &lnrpc.ListChannelsRequest{} @@ -2253,7 +2253,7 @@ func testChannelForceClosure(net *lntest.NetworkHarness, t *harnessTest) { t.Fatalf("no user funds should be left in limbo after incubation") } - // At this point, Carol should now be aware of his new immediately + // At this point, Bob should now be aware of his new immediately // spendable on-chain balance, as it was Alice who broadcast the // commitment transaction. carolBalResp, err = net.Bob.WalletBalance(ctxb, carolBalReq) From bf06dc24c174c38b23e59d22907ac7b26b03e26e Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 17 Jul 2018 09:13:05 +0200 Subject: [PATCH 03/15] contractcourt/contract_resolvers: make waitForOutputResolution use confirmed spend --- contractcourt/contract_resolvers.go | 34 +++-------------------------- 1 file changed, 3 insertions(+), 31 deletions(-) diff --git a/contractcourt/contract_resolvers.go b/contractcourt/contract_resolvers.go index 0a0ec5e2..d863343b 100644 --- a/contractcourt/contract_resolvers.go +++ b/contractcourt/contract_resolvers.go @@ -169,45 +169,17 @@ func (h *htlcTimeoutResolver) Resolve() (ContractResolver, error) { // spent, and the spending transaction has been fully confirmed. waitForOutputResolution := func() error { // We first need to register to see when the HTLC output itself - // has been spent so we can wait for the spending transaction - // to confirm. + // has been spent by a confirmed transaction. spendNtfn, err := h.Notifier.RegisterSpendNtfn( &h.htlcResolution.ClaimOutpoint, - h.broadcastHeight, true, + h.broadcastHeight, false, ) if err != nil { return err } - var spendDetail *chainntnfs.SpendDetail select { - case s, ok := <-spendNtfn.Spend: - if !ok { - return fmt.Errorf("notifier quit") - } - - spendDetail = s - - case <-h.Quit: - return fmt.Errorf("quitting") - } - - // Now that the output has been spent, we'll also wait for the - // transaction to be confirmed before proceeding. - confNtfn, err := h.Notifier.RegisterConfirmationsNtfn( - spendDetail.SpenderTxHash, 1, - uint32(spendDetail.SpendingHeight-1), - ) - if err != nil { - return err - } - - log.Infof("%T(%v): waiting for spending (txid=%v) to be fully "+ - "confirmed", h, h.htlcResolution.ClaimOutpoint, - spendDetail.SpenderTxHash) - - select { - case _, ok := <-confNtfn.Confirmed: + case _, ok := <-spendNtfn.Spend: if !ok { return fmt.Errorf("notifier quit") } From b3e17aa67b6ca8d4054de25c8468289a10749271 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 17 Jul 2018 09:13:05 +0200 Subject: [PATCH 04/15] contractcourt/contract_resolvers: make second level htlcSuccessResolver wait for conf --- contractcourt/contract_resolvers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contractcourt/contract_resolvers.go b/contractcourt/contract_resolvers.go index d863343b..c1264650 100644 --- a/contractcourt/contract_resolvers.go +++ b/contractcourt/contract_resolvers.go @@ -580,7 +580,7 @@ func (h *htlcSuccessResolver) Resolve() (ContractResolver, error) { // To wrap this up, we'll wait until the second-level transaction has // been spent, then fully resolve the contract. spendNtfn, err := h.Notifier.RegisterSpendNtfn( - &h.htlcResolution.ClaimOutpoint, h.broadcastHeight, true, + &h.htlcResolution.ClaimOutpoint, h.broadcastHeight, false, ) if err != nil { return nil, err From 9fffe236965cebffa9f7923219e7486d6a67589b Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 17 Jul 2018 09:13:05 +0200 Subject: [PATCH 05/15] contractcourt/contract_resolvers: make htlcOutgoingContestResolver act on conf --- contractcourt/contract_resolvers.go | 2 +- lnd_test.go | 270 +++++++++++++++++++++------- 2 files changed, 209 insertions(+), 63 deletions(-) diff --git a/contractcourt/contract_resolvers.go b/contractcourt/contract_resolvers.go index c1264650..f4d418ca 100644 --- a/contractcourt/contract_resolvers.go +++ b/contractcourt/contract_resolvers.go @@ -792,7 +792,7 @@ func (h *htlcOutgoingContestResolver) Resolve() (ContractResolver, error) { // the remote party sweeps with the pre-image, we'll be notified. spendNtfn, err := h.Notifier.RegisterSpendNtfn( &outPointToWatch, - h.broadcastHeight, true, + h.broadcastHeight, false, ) if err != nil { return nil, err diff --git a/lnd_test.go b/lnd_test.go index 7e401ad7..706141fc 100644 --- a/lnd_test.go +++ b/lnd_test.go @@ -8276,7 +8276,8 @@ func testMultiHopHtlcLocalChainClaim(net *lntest.NetworkHarness, t *harnessTest) // At this point, Bob decides that he wants to exit the channel // immediately, so he force closes his commitment transaction. ctxt, _ := context.WithTimeout(ctxb, timeout) - closeChannelAndAssert(ctxt, t, net, net.Bob, aliceChanPoint, true) + bobForceClose := closeChannelAndAssert(ctxt, t, net, net.Bob, + aliceChanPoint, true) // We'll now mine enough blocks so Carol decides that she needs to go // on-chain to claim the HTLC as Bob has been inactive. @@ -8324,31 +8325,58 @@ func testMultiHopHtlcLocalChainClaim(net *lntest.NetworkHarness, t *harnessTest) // After the force close transacion is mined, Carol should broadcast // her second level HTLC transacion. Bob will braodcast a sweep tx to - // sweep his output in the channel with Carol. When Bob notices Carol's - // second level transaction in the mempool, he will extract the - // preimage and broadcast a second level tx to claim the HTLC in his - // (already closed) channel with Alice. - secondLevelHashes, err := waitForNTxsInMempool(net.Miner.Node, 3, + // sweep his output in the channel with Carol. He can do this + // immediately, as the output is not timelocked since Carol was the one + // force closing. + commitSpends, err := waitForNTxsInMempool(net.Miner.Node, 2, time.Second*20) if err != nil { t.Fatalf("transactions not found in mempool: %v", err) } - // Carol's second level transaction should be spending from - // the commitment transaction. - var secondLevelHash *chainhash.Hash - for _, txid := range secondLevelHashes { + // Both Carol's second level transaction and Bob's sweep should be + // spending from the commitment transaction. + for _, txid := range commitSpends { tx, err := net.Miner.Node.GetRawTransaction(txid) if err != nil { t.Fatalf("unable to get txn: %v", err) } - if tx.MsgTx().TxIn[0].PreviousOutPoint.Hash == *commitHash { - secondLevelHash = txid + if tx.MsgTx().TxIn[0].PreviousOutPoint.Hash != *commitHash { + t.Fatalf("tx did not spend from commitment tx") } } - if secondLevelHash == nil { - t.Fatalf("Carol's second level tx not found") + + // Mine a block to confirm the two transactions (+ the coinbase). + block = mineBlocks(t, net, 1)[0] + if len(block.Transactions) != 3 { + t.Fatalf("expected 3 transactions in block, got %v", + len(block.Transactions)) + } + for _, txid := range commitSpends { + assertTxInBlock(t, block, txid) + } + + // Keep track of the second level tx maturity. + carolSecondLevelCSV := defaultCSV + + // When Bob notices Carol's second level transaction in the block, he + // will extract the preimage and broadcast a second level tx to claim + // the HTLC in his (already closed) channel with Alice. + bobSecondLvlTx, err := waitForTxInMempool(net.Miner.Node, + time.Second*20) + if err != nil { + t.Fatalf("transactions not found in mempool: %v", err) + } + + // It should spend from the commitment in the channel with Alice. + tx, err := net.Miner.Node.GetRawTransaction(bobSecondLvlTx) + if err != nil { + t.Fatalf("unable to get txn: %v", err) + } + + if tx.MsgTx().TxIn[0].PreviousOutPoint.Hash != *bobForceClose { + t.Fatalf("tx did not spend from bob's force close tx") } // At this point, Bob should have broadcast his second layer success @@ -8388,41 +8416,63 @@ func testMultiHopHtlcLocalChainClaim(net *lntest.NetworkHarness, t *harnessTest) return false } } - return true }, time.Second*15) if err != nil { t.Fatalf("bob didn't hand off time-locked HTLC: %v", predErr) } - // We'll now mine a block which should confirm the two second layer - // transactions and the commit sweep. + // We'll now mine a block which should confirm Bob's second layer + // transaction. block = mineBlocks(t, net, 1)[0] - if len(block.Transactions) != 4 { - t.Fatalf("expected 4 transactions in block, got %v", + if len(block.Transactions) != 2 { + t.Fatalf("expected 2 transactions in block, got %v", len(block.Transactions)) } - assertTxInBlock(t, block, secondLevelHash) + assertTxInBlock(t, block, bobSecondLvlTx) - // If we then mine 4 additional blocks, Bob and Carol should sweep the - // outputs destined for them. - if _, err := net.Miner.Node.Generate(defaultCSV); err != nil { + // Keep track of Bob's second level maturity, and decrement our track + // of Carol's. + bobSecondLevelCSV := defaultCSV + carolSecondLevelCSV-- + + // If we then mine 3 additional blocks, Carol's second level tx should + // mature, and she can pull the funds from it with a sweep tx. + if _, err := net.Miner.Node.Generate(carolSecondLevelCSV); err != nil { t.Fatalf("unable to generate block: %v", err) } + bobSecondLevelCSV -= carolSecondLevelCSV - sweepTxs, err := waitForNTxsInMempool(net.Miner.Node, 2, time.Second*10) + carolSweep, err := waitForTxInMempool(net.Miner.Node, time.Second*10) if err != nil { - t.Fatalf("unable to find sweeping transactions: %v", err) + t.Fatalf("unable to find Carol's sweeping transaction: %v", err) } - // At this point, Bob should detect that he has no pending channels - // anymore, as this just resolved it by the confirmation of the sweep - // transaction we detected above. - block = mineBlocks(t, net, 1)[0] - for _, sweepTx := range sweepTxs { - assertTxInBlock(t, block, sweepTx) + // Mining one additional block, Bob's second level tx is mature, and he + // can sweep the output. + block = mineBlocks(t, net, bobSecondLevelCSV)[0] + assertTxInBlock(t, block, carolSweep) + + bobSweep, err := waitForTxInMempool(net.Miner.Node, time.Second*10) + if err != nil { + t.Fatalf("unable to find bob's sweeping transaction") } + // Make sure it spends from the second level tx. + tx, err = net.Miner.Node.GetRawTransaction(bobSweep) + if err != nil { + t.Fatalf("unable to get txn: %v", err) + } + if tx.MsgTx().TxIn[0].PreviousOutPoint.Hash != *bobSecondLvlTx { + t.Fatalf("tx did not spend from bob's second level tx") + } + + // When we mine one additional block, that will confirm Bob's sweep. + // Now Bob should have no pending channels anymore, as this just + // resolved it by the confirmation of the sweep transaction. + block = mineBlocks(t, net, 1)[0] + assertTxInBlock(t, block, bobSweep) + err = lntest.WaitPredicate(func() bool { pendingChanResp, err := net.Bob.PendingChannels( ctxb, pendingChansRequest, @@ -8437,7 +8487,54 @@ func testMultiHopHtlcLocalChainClaim(net *lntest.NetworkHarness, t *harnessTest) "but shouldn't: %v", spew.Sdump(pendingChanResp)) return false } + req := &lnrpc.ListChannelsRequest{} + chanInfo, err := net.Bob.ListChannels(ctxb, req) + if err != nil { + predErr = fmt.Errorf("unable to query for open "+ + "channels: %v", err) + return false + } + if len(chanInfo.Channels) != 0 { + predErr = fmt.Errorf("Bob should have no open "+ + "channels, instead he has %v", + len(chanInfo.Channels)) + return false + } + return true + }, time.Second*15) + if err != nil { + t.Fatalf(predErr.Error()) + } + // Also Carol should have no channels left (open nor pending). + err = lntest.WaitPredicate(func() bool { + pendingChanResp, err := carol.PendingChannels( + ctxb, pendingChansRequest, + ) + if err != nil { + predErr = fmt.Errorf("unable to query for pending "+ + "channels: %v", err) + return false + } + if len(pendingChanResp.PendingForceClosingChannels) != 0 { + predErr = fmt.Errorf("bob carol has pending channels "+ + "but shouldn't: %v", spew.Sdump(pendingChanResp)) + return false + } + + req := &lnrpc.ListChannelsRequest{} + chanInfo, err := carol.ListChannels(ctxb, req) + if err != nil { + predErr = fmt.Errorf("unable to query for open "+ + "channels: %v", err) + return false + } + if len(chanInfo.Channels) != 0 { + predErr = fmt.Errorf("carol should have no open "+ + "channels, instead she has %v", + len(chanInfo.Channels)) + return false + } return true }, time.Second*15) if err != nil { @@ -8506,7 +8603,8 @@ func testMultiHopHtlcRemoteChainClaim(net *lntest.NetworkHarness, t *harnessTest // immediately force close the channel by broadcast her commitment // transaction. ctxt, _ := context.WithTimeout(ctxb, timeout) - closeChannelAndAssert(ctxt, t, net, net.Alice, aliceChanPoint, true) + aliceForceClose := closeChannelAndAssert(ctxt, t, net, net.Alice, + aliceChanPoint, true) // We'll now mine enough blocks so Carol decides that she needs to go // on-chain to claim the HTLC as Bob has been inactive. @@ -8555,58 +8653,68 @@ func testMultiHopHtlcRemoteChainClaim(net *lntest.NetworkHarness, t *harnessTest // After the force close transacion is mined, Carol should broadcast // her second level HTLC transacion. Bob will braodcast a sweep tx to - // sweep his output in the channel with Carol. When Bob notices Carol's - // second level transaction in the mempool, he will extract the - // preimage and broadcast a second level tx to claim the HTLC in his - // (already closed) channel with Alice. - secondLevelHashes, err := waitForNTxsInMempool(net.Miner.Node, 3, + // sweep his output in the channel with Carol. He can do this + // immediately, as the output is not timelocked since Carol was the one + // force closing. + commitSpends, err := waitForNTxsInMempool(net.Miner.Node, 2, time.Second*20) if err != nil { t.Fatalf("transactions not found in mempool: %v", err) } - // Carol's second level transaction should be spending from - // the commitment transaction. - var secondLevelHash *chainhash.Hash - for _, txid := range secondLevelHashes { + // Both Carol's second level transaction and Bob's sweep should be + // spending from the commitment transaction. + for _, txid := range commitSpends { tx, err := net.Miner.Node.GetRawTransaction(txid) if err != nil { t.Fatalf("unable to get txn: %v", err) } - if tx.MsgTx().TxIn[0].PreviousOutPoint.Hash == *commitHash { - secondLevelHash = txid + if tx.MsgTx().TxIn[0].PreviousOutPoint.Hash != *commitHash { + t.Fatalf("tx did not spend from commitment tx") } } - if secondLevelHash == nil { - t.Fatalf("Carol's second level tx not found") - } - // We'll now mine a block which should confirm the two second layer - // transactions and the commit sweep. + // Mine a block to confirm the two transactions (+ coinbase). block = mineBlocks(t, net, 1)[0] - if len(block.Transactions) != 4 { - t.Fatalf("expected 4 transactions in block, got %v", + if len(block.Transactions) != 3 { + t.Fatalf("expected 3 transactions in block, got %v", len(block.Transactions)) } - assertTxInBlock(t, block, secondLevelHash) - - // If we then mine 4 additional blocks, Bob should pull the output - // destined for him. - if _, err := net.Miner.Node.Generate(defaultCSV); err != nil { - t.Fatalf("unable to generate block: %v", err) + for _, txid := range commitSpends { + assertTxInBlock(t, block, txid) } - _, err = waitForNTxsInMempool(net.Miner.Node, 1, time.Second*15) + // Keep track of the second level tx maturity. + carolSecondLevelCSV := defaultCSV + + // When Bob notices Carol's second level transaction in the block, he + // will extract the preimage and broadcast a sweep tx to directly claim + // the HTLC in his (already closed) channel with Alice. + bobHtlcSweep, err := waitForTxInMempool(net.Miner.Node, + time.Second*20) if err != nil { - t.Fatalf("unable to find bob's sweeping transaction: %v", err) + t.Fatalf("transactions not found in mempool: %v", err) } - // We'll now mine another block, this should confirm the sweep - // transaction that Bob broadcast in the prior stage. - if _, err := net.Miner.Node.Generate(1); err != nil { - t.Fatalf("unable to generate block: %v", err) + // It should spend from the commitment in the channel with Alice. + tx, err := net.Miner.Node.GetRawTransaction(bobHtlcSweep) + if err != nil { + t.Fatalf("unable to get txn: %v", err) } + if tx.MsgTx().TxIn[0].PreviousOutPoint.Hash != *aliceForceClose { + t.Fatalf("tx did not spend from alice's force close tx") + } + + // We'll now mine a block which should confirm Bob's HTLC sweep + // transaction. + block = mineBlocks(t, net, 1)[0] + if len(block.Transactions) != 2 { + t.Fatalf("expected 2 transactions in block, got %v", + len(block.Transactions)) + } + assertTxInBlock(t, block, bobHtlcSweep) + carolSecondLevelCSV-- // Now that the sweeping transaction has been confirmed, Bob should now // recognize that all contracts have been fully resolved, and show no @@ -8632,6 +8740,44 @@ func testMultiHopHtlcRemoteChainClaim(net *lntest.NetworkHarness, t *harnessTest if err != nil { t.Fatalf(predErr.Error()) } + + // If we then mine 3 additional blocks, Carol's second level tx will + // mature, and she should pull the funds. + if _, err := net.Miner.Node.Generate(carolSecondLevelCSV); err != nil { + t.Fatalf("unable to generate block: %v", err) + } + + carolSweep, err := waitForTxInMempool(net.Miner.Node, time.Second*10) + if err != nil { + t.Fatalf("unable to find Carol's sweeping transaction: %v", err) + } + + // When Carol's sweep gets confirmed, she should have no more pending + // channels. + block = mineBlocks(t, net, 1)[0] + assertTxInBlock(t, block, carolSweep) + + pendingChansRequest = &lnrpc.PendingChannelsRequest{} + err = lntest.WaitPredicate(func() bool { + pendingChanResp, err := carol.PendingChannels( + ctxb, pendingChansRequest, + ) + if err != nil { + predErr = fmt.Errorf("unable to query for pending "+ + "channels: %v", err) + return false + } + if len(pendingChanResp.PendingForceClosingChannels) != 0 { + predErr = fmt.Errorf("carol still has pending channels "+ + "but shouldn't: %v", spew.Sdump(pendingChanResp)) + return false + } + + return true + }, time.Second*15) + if err != nil { + t.Fatalf(predErr.Error()) + } } // testSwitchCircuitPersistence creates a multihop network to ensure the sender From 5bfa2f5912bf2a421294c224200acb971cd8d47f Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 17 Jul 2018 09:13:05 +0200 Subject: [PATCH 06/15] contractcourt/contract_resolvers: make commitSweepResolver use confirmed spend --- contractcourt/contract_resolvers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contractcourt/contract_resolvers.go b/contractcourt/contract_resolvers.go index f4d418ca..6b154619 100644 --- a/contractcourt/contract_resolvers.go +++ b/contractcourt/contract_resolvers.go @@ -1288,7 +1288,7 @@ func (c *commitSweepResolver) Resolve() (ContractResolver, error) { // until the commitment output has been spent. spendNtfn, err := c.Notifier.RegisterSpendNtfn( &c.commitResolution.SelfOutPoint, - c.broadcastHeight, true, + c.broadcastHeight, false, ) if err != nil { return nil, err From a16bb662a4bb968b2c8a68d75fe98859171a0bf1 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 17 Jul 2018 09:13:05 +0200 Subject: [PATCH 07/15] contractcourt/chain_arbitrator: make watchForChannelClose use confirmed spend --- contractcourt/chain_arbitrator.go | 37 +++++++------------------------ 1 file changed, 8 insertions(+), 29 deletions(-) diff --git a/contractcourt/chain_arbitrator.go b/contractcourt/chain_arbitrator.go index fb5f1c97..a3a48d55 100644 --- a/contractcourt/chain_arbitrator.go +++ b/contractcourt/chain_arbitrator.go @@ -497,13 +497,16 @@ func (c *ChainArbitrator) Stop() error { // NOTE: This must be launched as a goroutine. func (c *ChainArbitrator) watchForChannelClose(closeInfo *channeldb.ChannelCloseSummary) { spendNtfn, err := c.cfg.Notifier.RegisterSpendNtfn( - &closeInfo.ChanPoint, closeInfo.CloseHeight, true, + &closeInfo.ChanPoint, closeInfo.CloseHeight, false, ) if err != nil { log.Errorf("unable to register for spend: %v", err) return } + log.Infof("Waiting for ChannelPoint(%v) to be coop closed on chain", + closeInfo.ChanPoint) + var ( commitSpend *chainntnfs.SpendDetail ok bool @@ -517,35 +520,11 @@ func (c *ChainArbitrator) watchForChannelClose(closeInfo *channeldb.ChannelClose return } - confNtfn, err := c.cfg.Notifier.RegisterConfirmationsNtfn( - commitSpend.SpenderTxHash, 1, - uint32(commitSpend.SpendingHeight), - ) - if err != nil { - log.Errorf("unable to register for "+ - "conf: %v", err) - return - } + log.Infof("ChannelPoint(%v) is fully closed, at height: %v", + closeInfo.ChanPoint, commitSpend.SpendingHeight) - log.Infof("Waiting for txid=%v to close ChannelPoint(%v) on chain", - commitSpend.SpenderTxHash, closeInfo.ChanPoint) - - select { - case confInfo, ok := <-confNtfn.Confirmed: - if !ok { - return - } - - log.Infof("ChannelPoint(%v) is fully closed, at height: %v", - closeInfo.ChanPoint, confInfo.BlockHeight) - - err := c.resolveContract(closeInfo.ChanPoint, nil) - if err != nil { - log.Errorf("unable to resolve contract: %v", err) - } - - case <-c.quit: - return + if err := c.resolveContract(closeInfo.ChanPoint, nil); err != nil { + log.Errorf("unable to resolve contract: %v", err) } } From 8cff5eae6c82c50b62fabfc917bee445d6593fbd Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 17 Jul 2018 09:13:06 +0200 Subject: [PATCH 08/15] breacharbiter: make the second level check use confirmed spends --- breacharbiter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/breacharbiter.go b/breacharbiter.go index 27a0a81b..9e46dbea 100644 --- a/breacharbiter.go +++ b/breacharbiter.go @@ -357,7 +357,7 @@ func (b *breachArbiter) waitForSpendEvent(breachInfo *retributionInfo, var err error spendNtfn, err = b.cfg.Notifier.RegisterSpendNtfn( &breachedOutput.outpoint, - breachInfo.breachHeight, true, + breachInfo.breachHeight, false, ) if err != nil { brarLog.Errorf("unable to check for spentness "+ From 77f002069730d859356966f43f0178f2a14ae939 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 17 Jul 2018 09:13:06 +0200 Subject: [PATCH 09/15] lnd_test: account for justice tx broadcast failure --- lnd_test.go | 48 ++++++++++++++++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 12 deletions(-) diff --git a/lnd_test.go b/lnd_test.go index 706141fc..6c638cb6 100644 --- a/lnd_test.go +++ b/lnd_test.go @@ -5867,17 +5867,17 @@ func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness, // Query the mempool for Dave's justice transaction, this should be // broadcast as Carol's contract breaching transaction gets confirmed // above. Since Carol might have had the time to take some of the HTLC - // outputs to the second level before Alice broadcasts her justice tx, + // outputs to the second level before Dave broadcasts his justice tx, // we'll search through the mempool for a tx that matches the number of // expected inputs in the justice tx. - // TODO(halseth): change to deterministic check if/when only acting on - // confirmed second level spends? var predErr error var justiceTxid *chainhash.Hash - err = lntest.WaitPredicate(func() bool { + errNotFound := errors.New("justice tx not found") + findJusticeTx := func() (*chainhash.Hash, error) { mempool, err := net.Miner.Node.GetRawMempool() if err != nil { - t.Fatalf("unable to get mempool from miner: %v", err) + return nil, fmt.Errorf("unable to get mempool from "+ + "miner: %v", err) } for _, txid := range mempool { @@ -5885,22 +5885,46 @@ func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness, // of inputs. tx, err := net.Miner.Node.GetRawTransaction(txid) if err != nil { - predErr = fmt.Errorf("unable to query for "+ + return nil, fmt.Errorf("unable to query for "+ "txs: %v", err) - return false } exNumInputs := 2 + numInvoices if len(tx.MsgTx().TxIn) == exNumInputs { - justiceTxid = txid - return true + return txid, nil } + } + return nil, errNotFound + } + err = lntest.WaitPredicate(func() bool { + txid, err := findJusticeTx() + if err != nil { + predErr = err + return false } - predErr = fmt.Errorf("justice tx not found") - return false - }, time.Second*15) + justiceTxid = txid + return true + }, time.Second*10) + if err != nil && predErr == errNotFound { + // If Dave is unable to broadcast his justice tx on first + // attempt because of the second layer transactions, he will + // wait until the next block epoch before trying again. Because + // of this, we'll mine a block if we cannot find the justice tx + // immediately. + mineBlocks(t, net, 1) + err = lntest.WaitPredicate(func() bool { + txid, err := findJusticeTx() + if err != nil { + predErr = err + return false + } + + justiceTxid = txid + return true + }, time.Second*10) + } if err != nil { t.Fatalf(predErr.Error()) } From 44d7b84df03d147b327bdb3caa3e577bbf9b08e7 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 17 Jul 2018 09:13:06 +0200 Subject: [PATCH 10/15] chainntnfs: remove mempool option from RegisterSpendNtfn --- chainntnfs/bitcoindnotify/bitcoind.go | 15 +++++---------- chainntnfs/btcdnotify/btcd.go | 12 ++++-------- chainntnfs/interface.go | 10 ++++------ chainntnfs/neutrinonotify/neutrino.go | 2 +- 4 files changed, 14 insertions(+), 25 deletions(-) diff --git a/chainntnfs/bitcoindnotify/bitcoind.go b/chainntnfs/bitcoindnotify/bitcoind.go index b6362746..8e65bb32 100644 --- a/chainntnfs/bitcoindnotify/bitcoind.go +++ b/chainntnfs/bitcoindnotify/bitcoind.go @@ -7,7 +7,6 @@ import ( "sync/atomic" "time" - "github.com/lightningnetwork/lnd/chainntnfs" "github.com/btcsuite/btcd/btcjson" "github.com/btcsuite/btcd/chaincfg" "github.com/btcsuite/btcd/chaincfg/chainhash" @@ -16,6 +15,7 @@ import ( "github.com/btcsuite/btcutil" "github.com/btcsuite/btcwallet/chain" "github.com/btcsuite/btcwallet/wtxmgr" + "github.com/lightningnetwork/lnd/chainntnfs" ) const ( @@ -367,11 +367,9 @@ func (b *BitcoindNotifier) handleRelevantTx(tx chain.RelevantTx, bestHeight int3 // the spend within a block. rem := make(map[uint64]*spendNotification) for c, ntfn := range clients { - // If this is a mempool spend, - // and this client didn't want - // to be notified on mempool - // spends, store it for later. - if !confirmedSpend && !ntfn.mempool { + // If this is a mempool spend, store the client + // to wait for a confirmed spend. + if !confirmedSpend { rem[c] = ntfn continue } @@ -560,8 +558,6 @@ type spendNotification struct { spendID uint64 heightHint uint32 - - mempool bool } // spendCancel is a message sent to the BitcoindNotifier when a client wishes @@ -580,13 +576,12 @@ type spendCancel struct { // across the 'Spend' channel. The heightHint should represent the earliest // height in the chain where the transaction could have been spent in. func (b *BitcoindNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint, - heightHint uint32, mempool bool) (*chainntnfs.SpendEvent, error) { + heightHint uint32) (*chainntnfs.SpendEvent, error) { ntfn := &spendNotification{ targetOutpoint: outpoint, spendChan: make(chan *chainntnfs.SpendDetail, 1), spendID: atomic.AddUint64(&b.spendClientCounter, 1), - mempool: mempool, } select { diff --git a/chainntnfs/btcdnotify/btcd.go b/chainntnfs/btcdnotify/btcd.go index e594ce36..f5c68dc0 100644 --- a/chainntnfs/btcdnotify/btcd.go +++ b/chainntnfs/btcdnotify/btcd.go @@ -416,10 +416,9 @@ out: rem := make(map[uint64]*spendNotification) for c, ntfn := range clients { // If this is a mempool spend, - // and this client didn't want - // to be notified on mempool - // spends, store it for later. - if !confirmedSpend && !ntfn.mempool { + // store the client to wait for + // a confirmed spend. + if !confirmedSpend { rem[c] = ntfn continue } @@ -673,8 +672,6 @@ type spendNotification struct { spendID uint64 - mempool bool - heightHint uint32 } @@ -694,14 +691,13 @@ type spendCancel struct { // across the 'Spend' channel. The heightHint should represent the earliest // height in the chain where the transaction could have been spent in. func (b *BtcdNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint, - heightHint uint32, mempool bool) (*chainntnfs.SpendEvent, error) { + heightHint uint32) (*chainntnfs.SpendEvent, error) { ntfn := &spendNotification{ targetOutpoint: outpoint, spendChan: make(chan *chainntnfs.SpendDetail, 1), spendID: atomic.AddUint64(&b.spendClientCounter, 1), heightHint: heightHint, - mempool: mempool, } select { diff --git a/chainntnfs/interface.go b/chainntnfs/interface.go index 930980cf..57870f64 100644 --- a/chainntnfs/interface.go +++ b/chainntnfs/interface.go @@ -43,15 +43,13 @@ type ChainNotifier interface { // The heightHint denotes the earliest height in the blockchain in // which the target output could have been created. // - // NOTE: If mempool=true is set, then this notification should be - // triggered on a best-effort basis once the transaction is *seen* on - // the network. If mempool=false, it should only be triggered when the - // spending transaction receives a single confirmation. + // NOTE: The notification should only be triggered when the spending + // transaction receives a single confirmation. // // NOTE: Dispatching notifications to multiple clients subscribed to a // spend of the same outpoint MUST be supported. - RegisterSpendNtfn(outpoint *wire.OutPoint, heightHint uint32, - mempool bool) (*SpendEvent, error) + RegisterSpendNtfn(outpoint *wire.OutPoint, + heightHint uint32) (*SpendEvent, error) // RegisterBlockEpochNtfn registers an intent to be notified of each // new block connected to the tip of the main chain. The returned diff --git a/chainntnfs/neutrinonotify/neutrino.go b/chainntnfs/neutrinonotify/neutrino.go index 32d3108c..2ab2ad05 100644 --- a/chainntnfs/neutrinonotify/neutrino.go +++ b/chainntnfs/neutrinonotify/neutrino.go @@ -566,7 +566,7 @@ type spendCancel struct { // target outpoint has been detected, the details of the spending event will be // sent across the 'Spend' channel. func (n *NeutrinoNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint, - heightHint uint32, _ bool) (*chainntnfs.SpendEvent, error) { + heightHint uint32) (*chainntnfs.SpendEvent, error) { n.heightMtx.RLock() currentHeight := n.bestHeight From 02f7f29b976290493b6b0580cd43847f3241fede Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 17 Jul 2018 09:13:06 +0200 Subject: [PATCH 11/15] chainntnfs/interface_test: remove mempool spend tests --- chainntnfs/interface_test.go | 177 +++++++---------------------------- 1 file changed, 32 insertions(+), 145 deletions(-) diff --git a/chainntnfs/interface_test.go b/chainntnfs/interface_test.go index cee8c514..90f31a01 100644 --- a/chainntnfs/interface_test.go +++ b/chainntnfs/interface_test.go @@ -17,9 +17,6 @@ import ( "github.com/btcsuite/btcwallet/walletdb" "github.com/lightninglabs/neutrino" "github.com/lightningnetwork/lnd/chainntnfs" - "github.com/lightningnetwork/lnd/chainntnfs/bitcoindnotify" - "github.com/lightningnetwork/lnd/chainntnfs/btcdnotify" - "github.com/lightningnetwork/lnd/chainntnfs/neutrinonotify" "github.com/ltcsuite/ltcd/btcjson" "github.com/btcsuite/btcd/btcec" @@ -30,6 +27,18 @@ import ( "github.com/btcsuite/btcd/wire" "github.com/btcsuite/btcutil" + // Required to auto-register the bitcoind backed ChainNotifier + // implementation. + _ "github.com/lightningnetwork/lnd/chainntnfs/bitcoindnotify" + + // Required to auto-register the btcd backed ChainNotifier + // implementation. + _ "github.com/lightningnetwork/lnd/chainntnfs/btcdnotify" + + // Required to auto-register the neutrino backed ChainNotifier + // implementation. + _ "github.com/lightningnetwork/lnd/chainntnfs/neutrinonotify" + // Required to register the boltdb walletdb implementation. _ "github.com/btcsuite/btcwallet/walletdb/bdb" ) @@ -403,7 +412,7 @@ func testSpendNotification(miner *rpctest.Harness, spendClients := make([]*chainntnfs.SpendEvent, numClients) for i := 0; i < numClients; i++ { spentIntent, err := notifier.RegisterSpendNtfn(outpoint, - uint32(currentHeight), false) + uint32(currentHeight)) if err != nil { t.Fatalf("unable to register for spend ntfn: %v", err) } @@ -451,6 +460,22 @@ func testSpendNotification(miner *rpctest.Harness, case <-time.After(mempoolSpendTimeout): } + // Make sure registering a client after the tx is in the mempool still + // doesn't trigger a notification. + spentIntent, err := notifier.RegisterSpendNtfn(outpoint, + uint32(currentHeight)) + if err != nil { + t.Fatalf("unable to register for spend ntfn: %v", err) + } + + select { + case <-spentIntent.Spend: + t.Fatalf("did not expect to get notification before " + + "block was mined") + case <-time.After(mempoolSpendTimeout): + } + spendClients = append(spendClients, spentIntent) + // Now we mine a single block, which should include our spend. The // notification should also be sent off. if _, err := miner.Node.Generate(1); err != nil { @@ -475,139 +500,6 @@ func testSpendNotification(miner *rpctest.Harness, } } -func testSpendNotificationMempoolSpends(miner *rpctest.Harness, - notifier chainntnfs.ChainNotifier, t *testing.T) { - - // Skip this test for neutrino and bitcoind backends, as they currently - // don't support notifying about mempool spends. - switch notifier.(type) { - case *neutrinonotify.NeutrinoNotifier: - return - case *bitcoindnotify.BitcoindNotifier: - return - case *btcdnotify.BtcdNotifier: - // Go on to test this implementation. - default: - t.Fatalf("unknown notifier type: %T", notifier) - } - - // We first create a new output to our test target address. - outpoint, pkScript := createSpendableOutput(miner, t) - - _, currentHeight, err := miner.Node.GetBestBlock() - if err != nil { - t.Fatalf("unable to get current height: %v", err) - } - - // Now that we have a output index and the pkScript, register for a - // spentness notification for the newly created output with multiple - // clients in order to ensure the implementation can support - // multi-client spend notifications. - - // We first create a list of clients that will be notified on mempool - // spends. - const numClients = 5 - spendClientsMempool := make([]*chainntnfs.SpendEvent, numClients) - for i := 0; i < numClients; i++ { - spentIntent, err := notifier.RegisterSpendNtfn(outpoint, - uint32(currentHeight), true) - if err != nil { - t.Fatalf("unable to register for spend ntfn: %v", err) - } - - spendClientsMempool[i] = spentIntent - } - - // Next, create a new transaction spending that output. - spendingTx := createSpendTx(outpoint, pkScript, t) - - // Broadcast our spending transaction. - spenderSha, err := miner.Node.SendRawTransaction(spendingTx, true) - if err != nil { - t.Fatalf("unable to broadcast tx: %v", err) - } - - err = waitForMempoolTx(miner, spenderSha) - if err != nil { - t.Fatalf("tx not relayed to miner: %v", err) - } - - // Make sure the mempool spend clients are correctly notified. - for _, client := range spendClientsMempool { - select { - case ntfn, ok := <-client.Spend: - if !ok { - t.Fatalf("channel closed unexpectedly") - } - - checkNotificationFields(ntfn, outpoint, spenderSha, - currentHeight+1, t) - - case <-time.After(5 * time.Second): - t.Fatalf("did not receive notification") - } - } - - // Create new clients that register after the tx is in the mempool - // already, but should still be notified. - newSpendClientsMempool := make([]*chainntnfs.SpendEvent, numClients) - for i := 0; i < numClients; i++ { - spentIntent, err := notifier.RegisterSpendNtfn(outpoint, - uint32(currentHeight), true) - if err != nil { - t.Fatalf("unable to register for spend ntfn: %v", err) - } - - newSpendClientsMempool[i] = spentIntent - } - - // Make sure the new mempool spend clients are correctly notified. - for _, client := range newSpendClientsMempool { - select { - case ntfn, ok := <-client.Spend: - if !ok { - t.Fatalf("channel closed unexpectedly") - } - - checkNotificationFields(ntfn, outpoint, spenderSha, - currentHeight+1, t) - - case <-time.After(5 * time.Second): - t.Fatalf("did not receive notification") - } - } - - // Now we mine a single block, which should include our spend. The - // notification should not be sent off again. - if _, err := miner.Node.Generate(1); err != nil { - t.Fatalf("unable to generate single block: %v", err) - } - - // When a block is mined, the mempool notifications we registered should - // not be sent off again, and the channel should be closed. - for _, c := range spendClientsMempool { - select { - case _, ok := <-c.Spend: - if ok { - t.Fatalf("channel should have been closed") - } - case <-time.After(30 * time.Second): - t.Fatalf("expected clients to be closed.") - } - } - for _, c := range newSpendClientsMempool { - select { - case _, ok := <-c.Spend: - if ok { - t.Fatalf("channel should have been closed") - } - case <-time.After(30 * time.Second): - t.Fatalf("expected clients to be closed.") - } - } - -} - func testBlockEpochNotification(miner *rpctest.Harness, notifier chainntnfs.ChainNotifier, t *testing.T) { @@ -1062,14 +954,13 @@ func testSpendBeforeNtfnRegistration(miner *rpctest.Harness, // checkSpends registers two clients to be notified of a spend that has // already happened. The notifier should dispatch a spend notification - // immediately. We register one that also listen for mempool spends, - // both should be notified the same way, as the spend is already mined. + // immediately. checkSpends := func() { const numClients = 2 spendClients := make([]*chainntnfs.SpendEvent, numClients) for i := 0; i < numClients; i++ { spentIntent, err := notifier.RegisterSpendNtfn(outpoint, - uint32(currentHeight), i%2 == 0) + uint32(currentHeight)) if err != nil { t.Fatalf("unable to register for spend ntfn: %v", err) @@ -1149,7 +1040,7 @@ func testCancelSpendNtfn(node *rpctest.Harness, spendClients := make([]*chainntnfs.SpendEvent, numClients) for i := 0; i < numClients; i++ { spentIntent, err := notifier.RegisterSpendNtfn(outpoint, - uint32(currentHeight), true) + uint32(currentHeight)) if err != nil { t.Fatalf("unable to register for spend ntfn: %v", err) } @@ -1446,10 +1337,6 @@ var ntfnTests = []testCase{ name: "spend ntfn", test: testSpendNotification, }, - { - name: "spend ntfn mempool", - test: testSpendNotificationMempoolSpends, - }, { name: "block epoch", test: testBlockEpochNotification, From 57e829f47eab0fc3b6e96fe8fd82b3bf3e62a518 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 17 Jul 2018 09:13:06 +0200 Subject: [PATCH 12/15] multi: remove mempool bool from RegisterSpendNtfn --- breacharbiter.go | 2 +- contractcourt/chain_arbitrator.go | 2 +- contractcourt/chain_watcher.go | 2 +- contractcourt/contract_resolvers.go | 9 ++++----- 4 files changed, 7 insertions(+), 8 deletions(-) diff --git a/breacharbiter.go b/breacharbiter.go index 9e46dbea..a2933a36 100644 --- a/breacharbiter.go +++ b/breacharbiter.go @@ -357,7 +357,7 @@ func (b *breachArbiter) waitForSpendEvent(breachInfo *retributionInfo, var err error spendNtfn, err = b.cfg.Notifier.RegisterSpendNtfn( &breachedOutput.outpoint, - breachInfo.breachHeight, false, + breachInfo.breachHeight, ) if err != nil { brarLog.Errorf("unable to check for spentness "+ diff --git a/contractcourt/chain_arbitrator.go b/contractcourt/chain_arbitrator.go index a3a48d55..9ceb4a9e 100644 --- a/contractcourt/chain_arbitrator.go +++ b/contractcourt/chain_arbitrator.go @@ -497,7 +497,7 @@ func (c *ChainArbitrator) Stop() error { // NOTE: This must be launched as a goroutine. func (c *ChainArbitrator) watchForChannelClose(closeInfo *channeldb.ChannelCloseSummary) { spendNtfn, err := c.cfg.Notifier.RegisterSpendNtfn( - &closeInfo.ChanPoint, closeInfo.CloseHeight, false, + &closeInfo.ChanPoint, closeInfo.CloseHeight, ) if err != nil { log.Errorf("unable to register for spend: %v", err) diff --git a/contractcourt/chain_watcher.go b/contractcourt/chain_watcher.go index 1ab959cb..7cda8f85 100644 --- a/contractcourt/chain_watcher.go +++ b/contractcourt/chain_watcher.go @@ -173,7 +173,7 @@ func (c *chainWatcher) Start() error { } spendNtfn, err := c.cfg.notifier.RegisterSpendNtfn( - fundingOut, heightHint, false, + fundingOut, heightHint, ) if err != nil { return err diff --git a/contractcourt/contract_resolvers.go b/contractcourt/contract_resolvers.go index 6b154619..f9e09630 100644 --- a/contractcourt/contract_resolvers.go +++ b/contractcourt/contract_resolvers.go @@ -172,7 +172,7 @@ func (h *htlcTimeoutResolver) Resolve() (ContractResolver, error) { // has been spent by a confirmed transaction. spendNtfn, err := h.Notifier.RegisterSpendNtfn( &h.htlcResolution.ClaimOutpoint, - h.broadcastHeight, false, + h.broadcastHeight, ) if err != nil { return err @@ -580,7 +580,7 @@ func (h *htlcSuccessResolver) Resolve() (ContractResolver, error) { // To wrap this up, we'll wait until the second-level transaction has // been spent, then fully resolve the contract. spendNtfn, err := h.Notifier.RegisterSpendNtfn( - &h.htlcResolution.ClaimOutpoint, h.broadcastHeight, false, + &h.htlcResolution.ClaimOutpoint, h.broadcastHeight, ) if err != nil { return nil, err @@ -791,8 +791,7 @@ func (h *htlcOutgoingContestResolver) Resolve() (ContractResolver, error) { // First, we'll register for a spend notification for this output. If // the remote party sweeps with the pre-image, we'll be notified. spendNtfn, err := h.Notifier.RegisterSpendNtfn( - &outPointToWatch, - h.broadcastHeight, false, + &outPointToWatch, h.broadcastHeight, ) if err != nil { return nil, err @@ -1288,7 +1287,7 @@ func (c *commitSweepResolver) Resolve() (ContractResolver, error) { // until the commitment output has been spent. spendNtfn, err := c.Notifier.RegisterSpendNtfn( &c.commitResolution.SelfOutPoint, - c.broadcastHeight, false, + c.broadcastHeight, ) if err != nil { return nil, err From d7b2977e8c7a8718203f7fd754ffce07d9725f28 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 17 Jul 2018 09:13:06 +0200 Subject: [PATCH 13/15] multi test: remove mempool bool from RegisterSpendNtfn --- contractcourt/chain_watcher_test.go | 2 +- discovery/gossiper_test.go | 4 ++-- fundingmanager_test.go | 2 +- htlcswitch/mock.go | 2 +- mock.go | 4 ++-- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/contractcourt/chain_watcher_test.go b/contractcourt/chain_watcher_test.go index 981fdf2c..a99fcd31 100644 --- a/contractcourt/chain_watcher_test.go +++ b/contractcourt/chain_watcher_test.go @@ -36,7 +36,7 @@ func (m *mockNotifier) Stop() error { return nil } func (m *mockNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint, - heightHint uint32, _ bool) (*chainntnfs.SpendEvent, error) { + heightHint uint32) (*chainntnfs.SpendEvent, error) { return &chainntnfs.SpendEvent{ Spend: m.spendChan, Cancel: func() {}, diff --git a/discovery/gossiper_test.go b/discovery/gossiper_test.go index 9ecf69f5..b3880c08 100644 --- a/discovery/gossiper_test.go +++ b/discovery/gossiper_test.go @@ -263,8 +263,8 @@ func (m *mockNotifier) RegisterConfirmationsNtfn(txid *chainhash.Hash, return nil, nil } -func (m *mockNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint, _ uint32, - _ bool) (*chainntnfs.SpendEvent, error) { +func (m *mockNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint, + _ uint32) (*chainntnfs.SpendEvent, error) { return nil, nil } diff --git a/fundingmanager_test.go b/fundingmanager_test.go index 163b64aa..7550db4c 100644 --- a/fundingmanager_test.go +++ b/fundingmanager_test.go @@ -126,7 +126,7 @@ func (m *mockNotifier) Stop() error { } func (m *mockNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint, - heightHint uint32, _ bool) (*chainntnfs.SpendEvent, error) { + heightHint uint32) (*chainntnfs.SpendEvent, error) { return &chainntnfs.SpendEvent{ Spend: make(chan *chainntnfs.SpendDetail), Cancel: func() {}, diff --git a/htlcswitch/mock.go b/htlcswitch/mock.go index fb648ac2..b3bcbdb1 100644 --- a/htlcswitch/mock.go +++ b/htlcswitch/mock.go @@ -801,7 +801,7 @@ func (m *mockNotifier) Stop() error { } func (m *mockNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint, - heightHint uint32, mempool bool) (*chainntnfs.SpendEvent, error) { + heightHint uint32) (*chainntnfs.SpendEvent, error) { return &chainntnfs.SpendEvent{ Spend: make(chan *chainntnfs.SpendDetail), diff --git a/mock.go b/mock.go index 405eaea0..e7b41828 100644 --- a/mock.go +++ b/mock.go @@ -106,7 +106,7 @@ func (m *mockNotfier) Stop() error { return nil } func (m *mockNotfier) RegisterSpendNtfn(outpoint *wire.OutPoint, - heightHint uint32, _ bool) (*chainntnfs.SpendEvent, error) { + heightHint uint32) (*chainntnfs.SpendEvent, error) { return &chainntnfs.SpendEvent{ Spend: make(chan *chainntnfs.SpendDetail), Cancel: func() {}, @@ -131,7 +131,7 @@ func makeMockSpendNotifier() *mockSpendNotifier { } func (m *mockSpendNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint, - heightHint uint32, _ bool) (*chainntnfs.SpendEvent, error) { + heightHint uint32) (*chainntnfs.SpendEvent, error) { m.mtx.Lock() defer m.mtx.Unlock() From 3808105dcdf8e2bc8a3877c576b8b2e8a9605f50 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 17 Jul 2018 09:13:06 +0200 Subject: [PATCH 14/15] chainntnfs/btcdnotify: remove all mempool spend clients --- chainntnfs/btcdnotify/btcd.go | 65 +++++++++++------------------------ 1 file changed, 21 insertions(+), 44 deletions(-) diff --git a/chainntnfs/btcdnotify/btcd.go b/chainntnfs/btcdnotify/btcd.go index f5c68dc0..371be550 100644 --- a/chainntnfs/btcdnotify/btcd.go +++ b/chainntnfs/btcdnotify/btcd.go @@ -377,6 +377,14 @@ out: // rescan spends. It might get removed entirely in the future. case item := <-b.txUpdates.ChanOut(): newSpend := item.(*txUpdate) + + // We only care about notifying on confirmed spends, so + // in case this is a mempool spend, we can continue, + // and wait for the spend to appear in chain. + if newSpend.details == nil { + continue + } + spendingTx := newSpend.tx // First, check if this transaction spends an output @@ -397,56 +405,27 @@ out: SpendingTx: spendingTx.MsgTx(), SpenderInputIndex: uint32(i), } - // TODO(roasbeef): after change to - // loadfilter, only notify on block - // inclusion? + spendDetails.SpendingHeight = newSpend.details.Height - confirmedSpend := false - if newSpend.details != nil { - confirmedSpend = true - spendDetails.SpendingHeight = newSpend.details.Height - } else { - spendDetails.SpendingHeight = currentHeight + 1 - } - - // Keep spendNotifications that are - // waiting for a confirmation around. - // They will be notified when we find - // the spend within a block. - rem := make(map[uint64]*spendNotification) - for c, ntfn := range clients { - // If this is a mempool spend, - // store the client to wait for - // a confirmed spend. - if !confirmedSpend { - rem[c] = ntfn - continue - } - - confStr := "unconfirmed" - if confirmedSpend { - confStr = "confirmed" - } - - chainntnfs.Log.Infof("Dispatching %s "+ - "spend notification for "+ + for _, ntfn := range clients { + chainntnfs.Log.Infof("Dispatching "+ + "confirmed spend "+ + "notification for "+ "outpoint=%v at height %v", - confStr, ntfn.targetOutpoint, + ntfn.targetOutpoint, spendDetails.SpendingHeight) ntfn.spendChan <- spendDetails - // Close spendChan to ensure that any calls to Cancel will not - // block. This is safe to do since the channel is buffered, and the - // message can still be read by the receiver. + // Close spendChan to ensure + // that any calls to Cancel + // will not block. This is safe + // to do since the channel is + // buffered, and the message + // can still be read by the + // receiver. close(ntfn.spendChan) } delete(b.spendNotifications, prevOut) - - // If we had any clients left, add them - // back to the map. - if len(rem) > 0 { - b.spendNotifications[prevOut] = rem - } } } @@ -610,8 +589,6 @@ func (b *BtcdNotifier) handleBlockConnected(newBlock *filteredBlock) error { continue } - // TODO(roasbeef): many integration tests expect spend to be - // notified within the mempool. spendDetails := &chainntnfs.SpendDetail{ SpentOutPoint: &prevOut, SpenderTxHash: &txSha, From 181014c3632a6ec5fd3724a5242d4b0084a78131 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 17 Jul 2018 11:18:18 +0200 Subject: [PATCH 15/15] chainntnfs/bitcoindnotify: remove all mempool spend clients --- chainntnfs/bitcoindnotify/bitcoind.go | 57 ++++++++------------------- 1 file changed, 16 insertions(+), 41 deletions(-) diff --git a/chainntnfs/bitcoindnotify/bitcoind.go b/chainntnfs/bitcoindnotify/bitcoind.go index 8e65bb32..2139e1f6 100644 --- a/chainntnfs/bitcoindnotify/bitcoind.go +++ b/chainntnfs/bitcoindnotify/bitcoind.go @@ -331,6 +331,14 @@ out: // handleRelevantTx notifies any clients of a relevant transaction. func (b *BitcoindNotifier) handleRelevantTx(tx chain.RelevantTx, bestHeight int32) { msgTx := tx.TxRecord.MsgTx + + // We only care about notifying on confirmed spends, so in case this is + // a mempool spend, we can continue, and wait for the spend to appear + // in chain. + if tx.Block == nil { + return + } + // First, check if this transaction spends an output // that has an existing spend notification for it. for i, txIn := range msgTx.TxIn { @@ -349,55 +357,22 @@ func (b *BitcoindNotifier) handleRelevantTx(tx chain.RelevantTx, bestHeight int3 SpendingTx: &msgTx, SpenderInputIndex: uint32(i), } - // TODO(roasbeef): after change to - // loadfilter, only notify on block - // inclusion? + spendDetails.SpendingHeight = tx.Block.Height - confirmedSpend := false - if tx.Block != nil { - confirmedSpend = true - spendDetails.SpendingHeight = tx.Block.Height - } else { - spendDetails.SpendingHeight = bestHeight + 1 - } - - // Keep spendNotifications that are - // waiting for a confirmation around. - // They will be notified when we find - // the spend within a block. - rem := make(map[uint64]*spendNotification) - for c, ntfn := range clients { - // If this is a mempool spend, store the client - // to wait for a confirmed spend. - if !confirmedSpend { - rem[c] = ntfn - continue - } - - confStr := "unconfirmed" - if confirmedSpend { - confStr = "confirmed" - } - - chainntnfs.Log.Infof("Dispatching %s "+ - "spend notification for "+ - "outpoint=%v at height %v", - confStr, ntfn.targetOutpoint, + for _, ntfn := range clients { + chainntnfs.Log.Infof("Dispatching confirmed "+ + "spend notification for outpoint=%v "+ + "at height %v", ntfn.targetOutpoint, spendDetails.SpendingHeight) ntfn.spendChan <- spendDetails - // Close spendChan to ensure that any calls to Cancel will not - // block. This is safe to do since the channel is buffered, and the + // Close spendChan to ensure that any calls to + // Cancel will not block. This is safe to do + // since the channel is buffered, and the // message can still be read by the receiver. close(ntfn.spendChan) } delete(b.spendNotifications, prevOut) - - // If we had any clients left, add them - // back to the map. - if len(rem) > 0 { - b.spendNotifications[prevOut] = rem - } } } }