From 887ff442db593bb63965cea2c6938bbd5f84309c Mon Sep 17 00:00:00 2001 From: carla Date: Fri, 13 Nov 2020 13:59:52 +0200 Subject: [PATCH] multi: fix verbose output for listsweeps and check all against wallet Previously, the verbose output of listsweeps would fail if we did not find some sweeps in our wallet's listtransactions output. This could be the case for sweeps that were rbf-ed, so the endpoint would fail. This commit also updates the listsweeps endpoint to always check against the wallet, so that we do not return these discarded sweeps that never confirmed. --- lnrpc/walletrpc/walletkit.pb.go | 3 +- lnrpc/walletrpc/walletkit.proto | 3 +- lnrpc/walletrpc/walletkit.swagger.json | 2 +- lnrpc/walletrpc/walletkit_server.go | 58 ++++++++++----------- lntest/itest/lnd_test.go | 72 +++++++++++++++++--------- 5 files changed, 81 insertions(+), 57 deletions(-) diff --git a/lnrpc/walletrpc/walletkit.pb.go b/lnrpc/walletrpc/walletkit.pb.go index 24c3eda4..794500c6 100644 --- a/lnrpc/walletrpc/walletkit.pb.go +++ b/lnrpc/walletrpc/walletkit.pb.go @@ -1117,7 +1117,8 @@ var xxx_messageInfo_BumpFeeResponse proto.InternalMessageInfo type ListSweepsRequest struct { // //Retrieve the full sweep transaction details. If false, only the sweep txids - //will be returned. + //will be returned. Note that some sweeps that LND publishes will have been + //replaced-by-fee, so will not be included in this output. Verbose bool `protobuf:"varint,1,opt,name=verbose,proto3" json:"verbose,omitempty"` XXX_NoUnkeyedLiteral struct{} `json:"-"` XXX_unrecognized []byte `json:"-"` diff --git a/lnrpc/walletrpc/walletkit.proto b/lnrpc/walletrpc/walletkit.proto index 09eb8982..a22d030d 100644 --- a/lnrpc/walletrpc/walletkit.proto +++ b/lnrpc/walletrpc/walletkit.proto @@ -464,7 +464,8 @@ message BumpFeeResponse { message ListSweepsRequest { /* Retrieve the full sweep transaction details. If false, only the sweep txids - will be returned. + will be returned. Note that some sweeps that LND publishes will have been + replaced-by-fee, so will not be included in this output. */ bool verbose = 1; } diff --git a/lnrpc/walletrpc/walletkit.swagger.json b/lnrpc/walletrpc/walletkit.swagger.json index 2a37b98e..5a574435 100644 --- a/lnrpc/walletrpc/walletkit.swagger.json +++ b/lnrpc/walletrpc/walletkit.swagger.json @@ -299,7 +299,7 @@ "parameters": [ { "name": "verbose", - "description": "Retrieve the full sweep transaction details. If false, only the sweep txids\nwill be returned.", + "description": "Retrieve the full sweep transaction details. If false, only the sweep txids\nwill be returned. Note that some sweeps that LND publishes will have been\nreplaced-by-fee, so will not be included in this output.", "in": "query", "required": false, "type": "boolean", diff --git a/lnrpc/walletrpc/walletkit_server.go b/lnrpc/walletrpc/walletkit_server.go index 90b89f5e..04fafc25 100644 --- a/lnrpc/walletrpc/walletkit_server.go +++ b/lnrpc/walletrpc/walletkit_server.go @@ -782,29 +782,14 @@ func (w *WalletKit) ListSweeps(ctx context.Context, } sweepTxns := make(map[string]bool) - - txids := make([]string, len(sweeps)) - for i, sweep := range sweeps { + for _, sweep := range sweeps { sweepTxns[sweep.String()] = true - txids[i] = sweep.String() } - // If the caller does not want verbose output, just return the set of - // sweep txids. - if !in.Verbose { - txidResp := &ListSweepsResponse_TransactionIDs{ - TransactionIds: txids, - } - - return &ListSweepsResponse{ - Sweeps: &ListSweepsResponse_TransactionIds{ - TransactionIds: txidResp, - }, - }, nil - } - - // If the caller does want full transaction lookups, query our wallet - // for all transactions, including unconfirmed transactions. + // Some of our sweeps could have been replaced by fee, or dropped out + // of the mempool. Here, we lookup our wallet transactions so that we + // can match our list of sweeps against the list of transactions that + // the wallet is still tracking. transactions, err := w.cfg.Wallet.ListTransactionDetails( 0, btcwallet.UnconfirmedHeight, ) @@ -812,26 +797,41 @@ func (w *WalletKit) ListSweeps(ctx context.Context, return nil, err } - var sweepTxDetails []*lnwallet.TransactionDetail + var ( + txids []string + txDetails []*lnwallet.TransactionDetail + ) + for _, tx := range transactions { _, ok := sweepTxns[tx.Hash.String()] if !ok { continue } - sweepTxDetails = append(sweepTxDetails, tx) + // Add the txid or full tx details depending on whether we want + // verbose output or not. + if in.Verbose { + txDetails = append(txDetails, tx) + } else { + txids = append(txids, tx.Hash.String()) + } } - // Fail if we have not retrieved all of our sweep transactions from the - // wallet. - if len(sweepTxDetails) != len(txids) { - return nil, fmt.Errorf("not all sweeps found by list "+ - "transactions: %v, %v", len(sweepTxDetails), len(txids)) + if in.Verbose { + return &ListSweepsResponse{ + Sweeps: &ListSweepsResponse_TransactionDetails{ + TransactionDetails: lnrpc.RPCTransactionDetails( + txDetails, + ), + }, + }, nil } return &ListSweepsResponse{ - Sweeps: &ListSweepsResponse_TransactionDetails{ - TransactionDetails: lnrpc.RPCTransactionDetails(transactions), + Sweeps: &ListSweepsResponse_TransactionIds{ + TransactionIds: &ListSweepsResponse_TransactionIDs{ + TransactionIds: txids, + }, }, }, nil } diff --git a/lntest/itest/lnd_test.go b/lntest/itest/lnd_test.go index 18b1cfca..93afb8be 100644 --- a/lntest/itest/lnd_test.go +++ b/lntest/itest/lnd_test.go @@ -3975,11 +3975,8 @@ func channelForceClosureTest(net *lntest.NetworkHarness, t *harnessTest, } // Check that we can find the commitment sweep in our set of known - // sweeps. - err = findSweep(ctxb, alice, sweepingTXID) - if err != nil { - t.Fatalf("csv sweep not found: %v", err) - } + // sweeps, using the simple transaction id ListSweeps output. + assertSweepFound(ctxb, t.t, alice, sweepingTXID.String(), false) // Restart Alice to ensure that she resumes watching the finalized // commitment sweep txid. @@ -4368,11 +4365,9 @@ func channelForceClosureTest(net *lntest.NetworkHarness, t *harnessTest, } } - // Check that we can find the htlc sweep in our set of sweeps. - err = findSweep(ctxb, alice, htlcSweepTx.Hash()) - if err != nil { - t.Fatalf("htlc sweep not found: %v", err) - } + // Check that we can find the htlc sweep in our set of sweeps using + // the verbose output of the listsweeps output. + assertSweepFound(ctxb, t.t, alice, htlcSweepTx.Hash().String(), true) // The following restart checks to ensure that the nursery store is // storing the txid of the previously broadcast htlc sweep txn, and that @@ -4571,33 +4566,60 @@ func assertReports(ctxb context.Context, t *harnessTest, } } -// findSweep looks up a sweep in a nodes list of broadcast sweeps. -func findSweep(ctx context.Context, node *lntest.HarnessNode, - sweep *chainhash.Hash) error { +// assertSweepFound looks up a sweep in a nodes list of broadcast sweeps. +func assertSweepFound(ctx context.Context, t *testing.T, node *lntest.HarnessNode, + sweep string, verbose bool) { // List all sweeps that alice's node had broadcast. ctx, _ = context.WithTimeout(ctx, defaultTimeout) sweepResp, err := node.WalletKitClient.ListSweeps( ctx, &walletrpc.ListSweepsRequest{ - Verbose: false, - }) - if err != nil { - return fmt.Errorf("list sweeps error: %v", err) + Verbose: verbose, + }, + ) + require.NoError(t, err) + + var found bool + if verbose { + found = findSweepInDetails(t, sweep, sweepResp) + } else { + found = findSweepInTxids(t, sweep, sweepResp) } - sweepTxIDs, ok := sweepResp.Sweeps.(*walletrpc.ListSweepsResponse_TransactionIds) - if !ok { - return errors.New("expected sweep txids in response") - } + require.True(t, found, "sweep: %v not found", sweep) +} + +func findSweepInTxids(t *testing.T, sweepTxid string, + sweepResp *walletrpc.ListSweepsResponse) bool { + + sweepTxIDs := sweepResp.GetTransactionIds() + require.NotNil(t, sweepTxIDs, "expected transaction ids") + require.Nil(t, sweepResp.GetTransactionDetails()) // Check that the sweep tx we have just produced is present. - for _, tx := range sweepTxIDs.TransactionIds.TransactionIds { - if tx == sweep.String() { - return nil + for _, tx := range sweepTxIDs.TransactionIds { + if tx == sweepTxid { + return true } } - return fmt.Errorf("sweep: %v not found", sweep.String()) + return false +} + +func findSweepInDetails(t *testing.T, sweepTxid string, + sweepResp *walletrpc.ListSweepsResponse) bool { + + sweepDetails := sweepResp.GetTransactionDetails() + require.NotNil(t, sweepDetails, "expected transaction details") + require.Nil(t, sweepResp.GetTransactionIds()) + + for _, tx := range sweepDetails.Transactions { + if tx.TxHash == sweepTxid { + return true + } + } + + return false } // assertAmountSent generates a closure which queries listchannels for sndr and