New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add listunspent RPC call #1984

Merged
merged 1 commit into from Dec 12, 2018

Conversation

Projects
None yet
8 participants
@AdamISZ
Copy link
Contributor

AdamISZ commented Sep 27, 2018

Returns a brief json summary of each utxo found by calling
ListUnspentWitness in the wallet. The only argument is the
number of confirmations (0=include unconfirmed), acting as
a filter.

Arguments: minconfs maxconfs (so filter functions as in Core).

moli on IRC suggested this might be a desirable thing, which is certainly understandable, for debugging/checking. It currently shows (address, scriptpubkey, txid:n outpoint and amount in satoshis) in the json output.

Adding the number of confirmations seems a little non-trivial as that's available in the call to the underlying wallet.ListUnspent ListUnspentResponse object but is not returned in the WalletController interface (which exposes ListUnspentWitness which returns objects of type Utxo, which don't contain the number of confirmations). That could be changed but it didn't seem enough of a big deal to address here.

I'll probably add a brief test function.

@molxyz

This comment has been minimized.

Copy link

molxyz commented Sep 27, 2018

tACK!

This is awesome! I just compiled on a testnet node and here's the result: http://paste.ubuntu.com/p/HGPs3bTKHG/

Would love to have this for LND. Thanks, Adam. :)

@AdamISZ AdamISZ force-pushed the AdamISZ:listunspent branch 2 times, most recently from 0a2e3c9 to 9b364bb Sep 27, 2018

@AdamISZ

This comment has been minimized.

Copy link
Contributor

AdamISZ commented Sep 29, 2018

Added maxconfs as well (see alteration to note above), also tidied up/corrected a few minor things.

@AdamISZ AdamISZ force-pushed the AdamISZ:listunspent branch from a67e1ee to e922dc8 Oct 2, 2018

@AdamISZ

This comment has been minimized.

Copy link
Contributor

AdamISZ commented Oct 2, 2018

Squashed and rebased.

@Roasbeef Roasbeef requested a review from halseth Oct 8, 2018


/// The outpoint in format txid:n
string outpoint = 5 [json_name = "outpoint"];
}

This comment has been minimized.

@joostjager

joostjager Oct 10, 2018

Collaborator

Consider reusing the structured types enum AddressType and message ChannelPoint, to prevent string parsing client-side.

Just a remark, no other reviewing done.

This comment has been minimized.

@AdamISZ

AdamISZ Oct 10, 2018

Contributor

Thanks, that's helpful. I'll take a look.

This comment has been minimized.

@AdamISZ

AdamISZ Oct 14, 2018

Contributor

Fixed as per 60ae2af although I have minor questions about this, see comment below.

@halseth
Copy link
Collaborator

halseth left a comment

Thanks for the PR! Can imagine this being useful 👍

Name: "listunspent",
Category: "On-chain",
Usage: "List all utxos available for spending",
ArgsUsage: "minconfs maxconfs",

This comment has been minimized.

@halseth

halseth Oct 10, 2018

Collaborator

These should be listed under Flags as well. Take a look at other commands where this is done :)

This comment has been minimized.

@AdamISZ

AdamISZ Oct 14, 2018

Contributor

Done in 60ae2af, it should now be in line with other commands

)
args := ctx.Args()

if !(ctx.NArg() != 2 || ctx.NArg() != 1) || ctx.NumFlags() != 0 {

This comment has been minimized.

@halseth

halseth Oct 10, 2018

Collaborator

this can also be simplified with the change to Flags.

string address = 2 [json_name = "address"];

/// the value of the unspent coin in satoshis
int64 amount = 3 [json_name = "amount"];

This comment has been minimized.

@halseth

halseth Oct 10, 2018

Collaborator

should be named amount_sat

This comment has been minimized.

@AdamISZ

AdamISZ Oct 14, 2018

Contributor

Done in 60ae2af

/// The address
string address = 2 [json_name = "address"];

/// the value of the unspent coin in satoshis

This comment has been minimized.

@halseth

halseth Oct 10, 2018

Collaborator

nit: Start comments with capital letter.

This comment has been minimized.

@AdamISZ

AdamISZ Oct 14, 2018

Contributor

Done in 60ae2af

@@ -456,6 +460,83 @@ func determineFeePerKw(feeEstimator lnwallet.FeeEstimator, targetConf int32,
}
}

func (r *rpcServer) ListUnspent(ctx context.Context,

This comment has been minimized.

@halseth

halseth Oct 10, 2018

Collaborator

Missing godoc.

This comment has been minimized.

@AdamISZ

AdamISZ Oct 14, 2018

Contributor

Fixed in 60ae2af

if err != nil {
return nil, err
}
// We want the symmetric difference between the first set (utxos1)

This comment has been minimized.

@halseth

halseth Oct 10, 2018

Collaborator

A better solution would be to modify the result struct to inlcude the number of confirmations:

type Utxo struct {

This will also be useful other places, and makes sure we list unspents in one atomic db transaction! 😀

This comment has been minimized.

@AdamISZ

AdamISZ Oct 10, 2018

Contributor

Thanks, so, just to check I understood: I mentioned in the first comment in the thread that I wanted to, but wasn't able, to include the number of confs due to that Utxo struct not containing it; so you're suggesting here that I do change that result struct. I was a bit wary of messing with more "internal" code for this PR, but I guess there's no possibility of that causing an issue elsewhere.

This comment has been minimized.

@halseth

halseth Oct 10, 2018

Collaborator

Oh sorry, didn't see that comment 😛 But yeah, I think it should be fairly trivial and non-disruptive, as you have the number of confirmations available as part of the returned result here:

unspentOutputs, err := b.wallet.ListUnspent(minConfs, maxConfs, nil)

This comment has been minimized.

@halseth

halseth Oct 10, 2018

Collaborator

The only think remaining is to add the same information to the Utxo struct, correct 👍

This comment has been minimized.

@AdamISZ

AdamISZ Oct 14, 2018

Contributor

Fixed in 60ae2af

}

// This logs the txid:n string only, server side.
rpcsLog.Infof("[listunspent] generated utxos: %v", utxos)

This comment has been minimized.

@halseth

halseth Oct 10, 2018

Collaborator

This might be a bit spammy? Could make debug.

This comment has been minimized.

@AdamISZ

AdamISZ Oct 14, 2018

Contributor

Switched it to debug, no prob removing it if preferred.

utxos = utxos1
}

utxoresplist := []*lnrpc.Utxo{}

This comment has been minimized.

@halseth

halseth Oct 10, 2018

Collaborator

nit: just create the ListUnspentResponse here, and append directly to it.

This comment has been minimized.

@AdamISZ

AdamISZ Oct 14, 2018

Contributor

Done 60ae2af

}
utxoresp.Address = outAddresses[0].String()
if utxo.AddressType == 0 {
utxoresp.AddressType = "p2wkh"

This comment has been minimized.

@halseth

halseth Oct 10, 2018

Collaborator

as @joostjager mentioned, here we should be using the already defined enums.

This comment has been minimized.

@AdamISZ

AdamISZ Oct 14, 2018

Contributor

Done, but have Qs about this, see comment below

@halseth halseth added this to the 0.5.2 milestone Oct 10, 2018

@AdamISZ

This comment has been minimized.

Copy link
Contributor

AdamISZ commented Oct 14, 2018

So a couple of (very) minor technical issues that may or may not be a problem:

(1) The message type ChannelPoint in the rpc is now reused for the Outpoint field in the lnrpc.Utxo message type. However this is a bit silly naming-wise since of course our spendable utxos are not channel points. As previously, just trying to avoid spreading code changes all over the place if it's not needed.

(2) For the AddressType I factored out the enum so it was reusable (i.e. not restricted to the NewAddressRequest message), which required a couple of very minor edits. I presume that's OK/desirable

(3) There is an issue with casting the number of confirmations from int64 to int32 here, but as per comment it seemed it was OK ... note the origin of the mismatch is that what is returned from ListUnspent in the btcd codebase here is an int64, so that's what I've stored in the Utxo struct that ListUnspentWitness returns.

@AdamISZ AdamISZ force-pushed the AdamISZ:listunspent branch 2 times, most recently from 4ec611b to ffce491 Oct 14, 2018

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Oct 17, 2018

The message type ChannelPoint in the rpc is now reused for the Outpoint field in the lnrpc.Utxo message type. However this is a bit silly naming-wise since of course our spendable utxos are not channel points. As previously, just trying to avoid spreading code changes all over the place if it's not needed.

Yes, I agree that it should probably be renamed to OutPoint. Also, one could argue if the oneof is justifiable. It makes the interface wider while decoding hex bytes could easily by done by the caller too.

@wpaulino
Copy link
Collaborator

wpaulino left a comment

Thanks for the PR! This has been a much requested feature. Most of my comments are pretty minor, with the most major thing being using the internal ListUnspent call within the wallet to directly retrieve maxConfs outputs.

var listUnspentCommand = cli.Command{
Name: "listunspent",
Category: "On-chain",
Usage: "List utxos available for spending",

This comment has been minimized.

@wpaulino

wpaulino Oct 27, 2018

Collaborator

Nit: end with period like the other commands.

This comment has been minimized.

@AdamISZ

AdamISZ Oct 31, 2018

Contributor

Done

func listUnspent(ctx *cli.Context) error {
var (
MinConfirms int64
MaxConfirms int64

This comment has been minimized.

@wpaulino

wpaulino Oct 27, 2018

Collaborator

Style nit: doesn't seem like these need their first letter to be capitalized?

This comment has been minimized.

@AdamISZ

AdamISZ Oct 31, 2018

Contributor

Done

case args.Present():
MinConfirms, err = strconv.ParseInt(args.First(), 10, 64)
if err != nil {
cli.ShowCommandHelp(ctx, "listunspent")

This comment has been minimized.

@wpaulino

wpaulino Oct 27, 2018

Collaborator

👍

*/
rpc ListUnspent (ListUnspentRequest) returns (ListUnspentResponse) {
option (google.api.http) = {
get: "/v1/txouts"

This comment has been minimized.

@wpaulino

wpaulino Oct 27, 2018

Collaborator

Thoughts on using /v1/utxos instead?

This comment has been minimized.

@AdamISZ

AdamISZ Oct 31, 2018

Contributor

Sure; done

@@ -647,6 +657,29 @@ service Lightning {
};
}

message Utxo {

This comment has been minimized.

@wpaulino

wpaulino Oct 27, 2018

Collaborator

Nit: extra newline.

This comment has been minimized.

@AdamISZ

AdamISZ Oct 31, 2018

Contributor

Done

in *lnrpc.ListUnspentRequest) (*lnrpc.ListUnspentResponse, error) {
minConfs := in.Minconfs
maxConfs := in.Maxconfs
if minConfs > maxConfs {

This comment has been minimized.

@wpaulino

wpaulino Oct 27, 2018

Collaborator

Should also check for negative minConfs here for users of the RPC.

This comment has been minimized.

@AdamISZ

AdamISZ Oct 31, 2018

Contributor

Done

return nil, fmt.Errorf("Max confirmations must be >= min " +
"confirmations")
}
usingMax := maxConfs != math.MaxInt32

This comment has been minimized.

@wpaulino

wpaulino Oct 27, 2018

Collaborator

Nit: add newline above.

This comment has been minimized.

@AdamISZ

AdamISZ Oct 31, 2018

Contributor

Done

rpcsLog.Infof("[listunspent] min=%v", minConfs)
}

utxos1, err := r.server.cc.wallet.ListUnspentWitness(minConfs)

This comment has been minimized.

@wpaulino

wpaulino Oct 27, 2018

Collaborator

Rather than having all of this logic to determine which satisfy maxConfs, it'd be better to do this at the wallet level. If you take a look at ListUnspentWitness, you'll see that the inner ListUnspent call already allows you to specify maxConfs, so we should use that instead.

This comment has been minimized.

@AdamISZ

AdamISZ Oct 27, 2018

Contributor

Many thanks for the review overall, I'll address it, but on this point: I thought that the idea was that we should be using the WalletController interface as specified in lnwallet/interface.go ... or perhaps more generally, not relying on underlying features of a wallet implementation in case that gets swap-out-able in future. Perhaps I misinterpreted.

You could reasonably ask why the existing ListUnspentWitness with only a minconfs parameter is all that's exposed in that interface I guess.

This comment has been minimized.

@wpaulino

wpaulino Oct 27, 2018

Collaborator

I would say that’s just the way things were done initially. I think extending the interface method with the additional parameter is a perfectly reasonable change to have though.

This comment has been minimized.

@AdamISZ

AdamISZ Oct 27, 2018

Contributor

Right, extending the interface makes more sense, but: If I change the existing one (ListUnspentWitness) I'd have to modify all calls to it, wouldn't I? It seems a bit invasive but happy to do it if that's what's desired.

This comment has been minimized.

@wpaulino

wpaulino Oct 27, 2018

Collaborator

Indeed. Definitely warrants its own PR.

This comment has been minimized.

@AdamISZ

AdamISZ Oct 27, 2018

Contributor

OK so I can PR that and make this a dependency then, sound good? (I do agree it's more logical!)

This comment has been minimized.

@wpaulino

wpaulino Oct 27, 2018

Collaborator

Yup!

This comment has been minimized.

@AdamISZ

AdamISZ Oct 28, 2018

Contributor

See #2112 ; I'll wait on that before updating here, in case for some reason people decide against it.

This comment has been minimized.

@AdamISZ

AdamISZ Oct 31, 2018

Contributor

Done (#2112 merged)

addrType = lnrpc.AddressType_WITNESS_PUBKEY_HASH
case lnwallet.NestedWitnessPubKey:
addrType = lnrpc.AddressType_NESTED_PUBKEY_HASH
}

This comment has been minimized.

@wpaulino

wpaulino Oct 27, 2018

Collaborator

We should also handle UnknownAddressType and at least log it (debug).

This comment has been minimized.

@AdamISZ

AdamISZ Oct 31, 2018

Contributor

Done (it is logged on server but not returned; don't see how it can happen for now anyway)

This comment has been minimized.

@halseth

halseth Dec 5, 2018

Collaborator

Maybe warn level?

return nil, err
}
if len(outAddresses) != 1 {
return nil, fmt.Errorf("An output was unexpectedly multisig")

This comment has been minimized.

@wpaulino

wpaulino Oct 27, 2018

Collaborator

Nit: errors returned should not be capitalized.

This comment has been minimized.

@AdamISZ

AdamISZ Oct 31, 2018

Contributor

Done

@wpaulino

This comment has been minimized.

Copy link
Collaborator

wpaulino commented Oct 27, 2018

Needs a proto rebase (this happens quite often)!

(1) The message type ChannelPoint in the rpc is now reused for the Outpoint field in the lnrpc.Utxo message type. However this is a bit silly naming-wise since of course our spendable utxos are not channel points. As previously, just trying to avoid spreading code changes all over the place if it's not needed.

It doesn't seem like type aliases are supported, so I think it's probably better to just rename it or define a new OutPoint message to prevent the rename change.

(2) For the AddressType I factored out the enum so it was reusable (i.e. not restricted to the NewAddressRequest message), which required a couple of very minor edits. I presume that's OK/desirable

Indeed 👍

(3) There is an issue with casting the number of confirmations from int64 to int32 here, but as per comment it seemed it was OK ... note the origin of the mismatch is that what is returned from ListUnspent in the btcd codebase here is an int64, so that's what I've stored in the Utxo struct that ListUnspentWitness returns.

Yeah, I'd say just keep things as they are now. Modifying this to be int64 would probably be a somewhat large diff.

AdamISZ added a commit to AdamISZ/lnd that referenced this pull request Oct 28, 2018

Add a maxconfirms argument to ListUnspentWitness
This change was inspired by lightningnetwork#1984 - the underlying call to
ListUnspent supports a (min, max) range so it makes sense that
the WalletController interface can also support this; a
default no-maximum can be expressed using a MaxInt32 value.

@AdamISZ AdamISZ force-pushed the AdamISZ:listunspent branch 4 times, most recently from c12ce61 to d0ba4a7 Oct 31, 2018

/**
`AddressType` has to be one of:
- `p2wkh`: Pay to witness key hash (`WITNESS_PUBKEY_HASH` = 0)
- `np2wkh`: Pay to nested witness key hash (`NESTED_PUBKEY_HASH` = 1)
*/
message NewAddressRequest {
enum AddressType {
enum AddressType {

This comment has been minimized.

@halseth

halseth Nov 1, 2018

Collaborator

Will this be a breaking RPC change?

This comment has been minimized.

@AdamISZ

AdamISZ Nov 1, 2018

Contributor

I don't think so. It tests fine on the lncli command line; I also just tested it over grpc using a python client and it seemed to work fine. (it = NewAddress request)

This comment has been minimized.

@halseth

halseth Dec 5, 2018

Collaborator

It will break usages of NewAddressRequests for projects importing lnrpc, but I think that's fine. It is only a simple name change.

@halseth

This comment has been minimized.

Copy link
Collaborator

halseth commented Nov 6, 2018

Needs a rebase!

@AdamISZ AdamISZ force-pushed the AdamISZ:listunspent branch from d0ba4a7 to 89e2107 Nov 7, 2018

@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Dec 4, 2018

After one final rebase (hopefully!?) that also squashes the commits, I think this is ready to land!

@AdamISZ AdamISZ force-pushed the AdamISZ:listunspent branch from 89e2107 to 76fbe5c Dec 4, 2018

@AdamISZ

This comment has been minimized.

Copy link
Contributor

AdamISZ commented Dec 4, 2018

Rebased again. I removed the created change to go.sum; I actually don't know what it is, so let me know if that was the wrong thing to do.

treygriffith added a commit to sparkswap/lnd that referenced this pull request Dec 5, 2018

Upgrade base to 0.5.1-beta (#21)
* server: ensure we update our node ann with new tor addrs in initTorController

Fixes lightningnetwork#1939.

* server: re-work initial node ann creation to use LightnignNode.NodeAnnouncement

* lnd_test: add onion addresses to testNodeAnnouncement

In this commit, we include onion addresses in testNodeAnnouncement to
ensure they properly propagate throughout the network.

* cnct/contract_resolvers: reliably publish commit sweep

* cnct/contract_resolvers: reliably publish htlc success sweep

* cnct/contract_resolvers: ignore duplicate publication error

* cnct/contract_resolvers: propagate checkpoint failures

* channeldb/graph: add method to determine if a node is public

In this commit, we add a method to the ChannelGraph struct that
determines whether a node is seen as public based on graph's source
node's point of view.

* routing/router: extend ChannelGraphSource interface with IsPublicNode
method

* discovery: ensure we only broadcast NodeAnnouncements of public nodes

In this commit, we modify the gossiper to no longer broadcast
NodeAnnouncements of nodes who intend to remain private. We do this to
prevent leaking their information to the greater network.

* discovery/gossiper_test: modify TestProcessAnnouncement to process node
ann last

In this commit, we modify TestProcessAnnouncement to process the node
announcement last. We do this due to the recent change in the gossiper
where we'll only forward node announcements of nodes who intend to
advertised themselves within the network.

This change was needed in order to allow the node announcement to be
broadcast to the greater network, as otherwise the gossiper would assume
the node intends to stay private due to not having any advertised edges.

* make: ensure make fmt is run with -s flag

Linting will fail if code is not formatted with the -s flag, so make
sure we always use it when formatting.

* lnd_test: process all node/edge updates in graph_top_itest

This commit modifies the graph topology test to
properly count channel updates and node
announcments in the event that they are batched
into a single topology update. The prior logic
made the assumption that they were always in
distinct topology updates, so this method should
be more general and robust.

* fundingmanager: send NodeAnnouncement to unadvertised channel counterparty

In this commit, we modify the funding manager to send our
NodeAnnouncement to our channel counterparty in the event of an
unadvertised channel. We do this to ensure that our counterparty learns
about some information about us that may aid them in one way or another
(e.g., addresses to reconnect, features supported, etc.).

* chan_series: filter out nodes who intend to remain private

In this commit, we filter out nodes who intend to remain private. We do
this to prevent leaking information about them by forwarding their
NodeAnnouncements.

* rpc: ensure we don't leak unadvertised nodes within invoice routing hints

In this commit, we ensure that we don't include routing hints for
unadvertised nodes at the time of invoice creation. Otherwise, this
would lead us to leak these unadvertised nodes to anyone who can get
their hands on the invoice being created. To prevent this, we'll now
look at the network graph and ensure that the node in unadvertised if
all of their edges are unadvertised and only extend to us.

* lnd_test: update testInvoiceRoutingHints to account

In this commit, we open an additional channel between Bob and Carol to
ensure that Bob gets selected as the only routing hint. Previously Bob
would get selected, but with the recent changes, it would no longer
happen due to him not having any advertised edges.

* watchtower/wtdb/breach_hint: adds BreachHint, txid prefix

* watchtower/wtdb/session_id: adds SessoinID, client pubkey

* watchtower/wtdb/session_info: adds SessionInfo

* watchtower/wtdb/session_state_update: adds session state

* watchtower/wtdb/mock: adds MockDB for debug build

* watchtower/server/server: server skeleton

* watchtower/server/log: adds wt SRVR sublogger

* watchtower/server/interface: adds Server, Peer and DB

* watchtower/server/mock: create MockPeer for debug build

* watchtower/server/server_test: add test vectors for server behavior

* Revert "make: ensure make fmt is run with -s flag"

This reverts commit e8003af.

* build: update to latest neutrino commit

In this commit, we update to the latest version of neutrino. This
version has an important bug fixes which fixes an issue where we
wouldn't detect a duplicate incoming cfheader message, and panic due to
trying to write the same header twice.

* fundingmanager: identify tx on funding broadcast error

* Fix incorrect hash key in gRPC Ruby documentation

* Show how macaroon interceptor can work with streaming gRPC in Ruby doc

* chainntnfs/txconfnotifier_test: use tcn instead of txConfNotifier

* chainntnfs/txconfnotifier_test: update height hint cache test

* chainntnfs/txnotifier_test: update nil spend details to restore tests

* chainntnfs/txconfnotifier: isolate scanning ntfns

* chainntnfs/bitcoind+btcd+neutrino: pass nil conf details

* chainntnfs/txconfnotifier: query conf hint in Register

* chainntnfs/txconfnotifier: add rescanStates

* chainntnfs/bitcoind+btcd+neutrino: let tcn query for height hint

* chainntnfs/txconfnotifier_test: update to use multi-value Register

* chainntnfs/txconfnotifier: add HistoricalConfDispatch struct

* chainntnfs/txconfnotifier: add PkScript to ConfNtfn

* chainntnfs/txconfnotifier: return HistoricalConfDispatch from Register

* chainntnfs/txconfnotifier: return HistoricalConfDispatch from Register

* chainntnfs/bitcoind: use HistoricalConfDispatch in ntfn registry

* chainntnfs/btcd: use HistoricalConfDispatch in ntfn registry

* chainntnfs/neutrino: use HistoricalConfDispatch in ntfn registry

* chainntnfs/txconfnotifier: split out ntfn dispatch into helper

* chainntnfs/txconfnotifier: remove clientID from UpdateConfDetails signature

* chainntnfs/txconfnotifier: set confset details at tip

This commit ensures that a confSet's details
are assigned in the confNotifications index
after discovering the transaction at tip. The
recent changes allow a later notification to
be dispatched on registration if an earlier one
has already discovered the confirmation details.

Before this change, it was observed that a later
registration would attempt an immediate delivery,
but fail to do so because the confset's details
were nil. This commit remedies that dispatch path,
allowing the integration tests to pass again.

* chainntnfs/txconfnotifier_test: remove clientID argument...

to UpdateConfDetails

* chainntnfs/txconnotifier: add debug logs, log errs/warnings, godocs

* chainntnfs/tx_notifier: mark rescan as complete for transactions confirmed at tip

In this commit, we mark the rescan status for a transaction as complete
if we happen to detect it has confirmed within a new block that extends
the chain. We do this as otherwise, it's possible for us to not
immediately dispatch the notification upon a subsequent registration due
to the rescan state machine.

* chainntnfs/tx_notifier: consume reorg notification for transactions on block inclusion

In this commit, we'll attempt to consume a reorg notification for a
transaction that was previously reorged out of the chain upon block
inclusion to ensure that it is not lingering due to a client not
handling it the first time.

* chainntnfs/tx_notifier: remove cached conf details on reorg

In this commit, we address a small bug where it's possible to deliver a
confirmation notification with stale confirmation details upon
registration. This can happen if a transaction has confirmed but was
reorged out of the chain later on, and a subsequent notification is
registered.

* chainntnfs/tx_notifier: extract conf reorg dispatch into method

* chainntnfs/txconfnotifier: remove ntfn details, bound conf depth

Removes details field from conf notifications, in favor
of using the details on the confSet. We also bound the
requested conf depth to the reorg saftey limit, as the
behavior of state tracking within the notifier is
undefined otherwise.

* chainntnfs/neutrinonotify/neutrino: fix debug logs

* Add a maxconfirms argument to ListUnspentWitness

This change was inspired by lightningnetwork#1984 - the underlying call to
ListUnspent supports a (min, max) range so it makes sense that
the WalletController interface can also support this; a
default no-maximum can be expressed using a MaxInt32 value.

* server: properly set node pubkey within initTorController

* lnrpc: extract hop unmarshall code

* docs: update INSTALL.md 

- Provides clarity to the sample `lnd.conf` to reduce friction on a first-time mainnet set up of lnd (avoiding the `loadConfig: debug-htlc mode cannot be used on bitcoin mainnet` error)

- Removes `debughtlc` from the sample `lnd.conf` since it is not something most people would use anymore
- Adds link to `sample-lnd.conf` with proper line-wrapping

* watchtower/blob/justice_kit_test: use test.Run for sub tests

* watchtower/blob/justice_kit: add variable length sweep addr

This commit fixes an oversight in the previous
design of the watchtower blob, by introducing
a length byte for sweep addresses. The previous
format supposed that addresses would be padded
to 42 bytes, but had no indication of the
address's actual length.

To rememdy this, we introduce a single byte
indicating the actual size of the address,
such that the padding can be removed upon
decoding.

* watchtower/blob/justice_kit_test: add sweep addr tests

Adds vectors to the justice kit tests to
ensure variable length sweep addresses are
properly encoded/decoded.

* chainntnfs/interface_test: stop UnsafeStart notifiers within test

In this commit, we modify the set of tests that start the different
backend notifiers with UnsafeStart to stop them within the tests
themselves. This prevents us from running into a panic when attempting
to run the package-level tests with a filter (using test.run).

* chainntnfs/height_hint_cache: prevent db transactions with no updates

In this commit, we modify our height hint cache to no longer start a
database transaction if no outpoints/txids are provided to update the
height hints for.

* chainntnfs: rename txconfnotifier.go -> txnotifier.go

* chainntnfs: rename TxConfNotifier -> TxNotifier

* chainntnfs/txnotifier: rename Register -> RegisterConf

* chainntnfs/txnotifier: rename hintCache -> confirmHintCache

* chainntnfs: extend SpendEvent with reorg channel

In this commit, we add a new channel within the SpendEvent struct that
will be sent upon whenever the spending transaction of the registered
outpoint gets reorged out of the chain. This will pave the road for
successfully handling a funding transaction getting reorged out of the
chain among other things.

* chainntnfs/txnotifier: add fields/structs to track spend notifications

In this commit, we introduce the required fields for the TxNotifier to
properly carry its duties in notifying its registered clients about the
spend of an outpoint. These are not yet used, but will be throughout the
some of the following commits.

* chainntnfs/txnotifier: allow registration of spend notifications

In this commit, we introduce the registration logic for spend
notifications to the TxNotifier. Most of this logic was taken from the
different existing ChainNotifier implementations, however, it features
some useful additions in order to make the ChainNotifier a bit more robust.

Some of these additions include the following:

  1. RegisterSpend will now return a HistoricalSpendDispatch struct,
  which includes the details for successfully determining if an outpoint
  was spent in the past. A HistoricalSpendDispatch will only be returned
  upon the first registration of an outpoint. This is done as,
  previously, if multiple clients registered for the same outpoint, then
  multiple historical rescans would also be dispatched, incurring a toll
  on the backend itself.

  2. UpdateSpendDetails will now be used to determine when a historical
  rescan has completed, no matter if a spending transaction was found or
  not. This is needed in order to responsibly update the spend hints for
  outpoints at tip, otherwise we'd attempt to update them even though we
  haven't yet determined if they have been spent or not. This will
  dispatch notifications to all currently registered clients for the
  same outpoint. In the event that another client registers later on,
  then the spending details are cached in memory in order to prevent
  further historical rescans.

* chainntnfs/txnotifier: watch for spends at tip

In this commit, we add support to allow the TxNotifier to properly
determine whether a new block extending the chain contains a transaction
that spends a registered outpoint. In the event that it does, spend
notifications will be dispatched to all active registered clients for
such outpoint.

* chainntnfs/txnotifier: detect reorgs for spending transactions of registered outpoints

In this commit, we introduce support to the TxNotifier to detect
spending transactions of registered outpoints being reorged out of the
chain. In the event that a reorg does occur, we'll consume the Spend
notification if it hasn't been consumed yet, and dispatch a Reorg
notification instead.

* chainntnfs/txnotifier: correctly update confirm/spend hints on chain updates

In this commit, we address an issue w.r.t. updating the confirm hints
for transactions and spend hints for outpoints on chain updates.
Previously, upon a block being disconnected, we'd attempt to commit a
new height hint for all outstanding confirmation notifications. This is
not correct because we'll end up modifying the height hint for things
that have confirmed at a previous height than the one being
disconnected. This would cause issues on restart when attempting a
historical dispatch, as we would start scanning at a height above which
the transaction actually confirmed in.

This has been addressed by only updating the hints for outstanding
notifications that are still unconfirmed/unspent and for notifications
that were confirmed/spent within the block being connected/disconnected.

* chainntnfs/txnotifier_test: extend tests to handle spend notifications

* chainntnfs/bitcoindnotify: handle spend notification registration w/ TxNotifier

In this commit, we modify the logic within RegisterSpendNtfn for the
BitcoindNotifier to account for the recent changes made to the
TxNotifier. Since it is now able to handle spend notification
registration and dispatch, we can bypass all the current logic within
the BitcoindNotifier and interact directly with the TxNotifier instead.

The most notable changes include the following:

  1. We'll only attempt a historical rescan if the TxNotifier tells us
  so.

  2. We'll dispatch the historical rescan within the main goroutine to
  prevent WaitGroup panics, due to the asynchronous nature of the
  notifier.

* chainntnfs/bitcoindnotify: remove old spend notification handling logic

In this commit, we remove the old spend notification logic within the
BitcoindNotifier as it's been phased out by the TxNotifier.

* chainntnfs/btcdnotify: handle spend notification registration w/ TxNotifier

In this commit, we modify the logic within RegisterSpendNtfn for the
BtcdNotifier to account for the recent changes made to the TxNotifier.
Since it is now able to handle spend notification registration and
dispatch, we can bypass all the current logic within the
BtcdNotifier and interact directly with the TxNotifier instead.

The most notable change is that now we'll only attempt a historical
rescan if the TxNotifier tells us so.

* chainntnfs/btcdnotify: remove old spend notification handling logic

In this commit, we remove the old spend notification logic within the
BtcdNotifier as it's been phased out by the TxNotifier.

* chainntnfs/neutrinonotify: handle spend notification registration w/ TxNotifier

In this commit, we modify the logic within RegisterSpendNtfn for the
NeutrinoNotifier to account for the recent changes made to the
TxNotifier. Since it is now able to handle spend notification
registration and dispatch, we can bypass all the current logic within
the NeutrinoNotifier and interact directly with the TxNotifier instead.

The most notable change is that now we'll only attempt a historical
rescan if the TxNotifier tells us so.

* chainntnfs/neutrinonotify: remove old spend notification handling logic

In this commit, we remove the old spend notification logic within the
NeutrinoNotifier as it's been phased out by the TxNotifier.

* chainntnfs/interface_test: add spend reorg test

* chainntnfs/txnotifier: move rescanState & confNtfnSet decl to top

* chainntnfs/txnotifier: commit height hint after rescan is complete

In this commit, we'll now commit the current height of the TxNotifier as
the height hint for an outpoint/transaction after a rescan has been
completed and has determined that the outpoint is unspent/transaction is
unconfirmed. We do this to prevent another potentially long rescan if
the daemon is restarted before a new block comes in (which is when the
hints will be updated again).

* chainntnfs/neutrinonotify: make filter update synchronous

In this commit, we modify the notifier to handle filter updates
synchronously. We do this to prevent race conditions between new block
notifications and filter updates. Otherwise, it's possible for a new
block to come in that should match our filter, but doesn't due to the
filter being updated after.

We also modify their order so that the filter is updated first. We do
this so we can immediately start watching for the event at tip while the
rescan is ongoing.

* build: bump Go versions, use '.x' to always get latest patch versions

* Revert "chainntnfs/height_hint_cache: add disable flag to hint cache"

This reverts commit 7df9ae0.

* Revert "chainntnfs/height_hint_cache_test: add tests for disabled cache"

This reverts commit 45a2c9a.

* Revert "chainntnfs/interface_test: run tests w/ disabled cache"

This reverts commit 12761a4.

* Revert "lnwallet/interface_test: run tests with disabled hint cache"

This reverts commit 70ba311.

* Revert "chainregistry: disable height hint cache"

This reverts commit 0e29a45.

* Revert "chainntnfs/bitcoindnotify: disable height hints in testing"

This reverts commit ab28db5.

* Revert "chainntnfs/btcdnotify: disable height hint cache in testing"

This reverts commit 98e7c96.

* chainntnfs/txnotifer: prevent dispatching notifications within ConnectTip

In this commit, we modify the TxNotifier's ConnectTip method to no
longer dispatch notifications to any clients who had a request fulfilled
within the height connected. Instead, it will queue the notifications
for dispatch and we add a new method NotifyHeight, which will actually
dispatch them. We do this to allow the users of the TxNotifier to be
more flexible when dispatching notifications.

* chainntnfs: dispatch conf/spend notifications after blocks

In this commit, we alter the different ChainNotifier implementations to
dispatch confirmation and spend notifications after blocks. We do this
to ensure the external consistency of our registered clients.

* watchtower/blob/justice_kit: return DER signatures

This commit fixes an issue with the witness stack
construction for to-local and to-remote inputs,
that would cause the justice kit to return
signatures as fixed-size, 64-byte signatures.
The correct behavior is to return DER-encoded
signatures so that they will properly verify on
the network, since the consensus rules won't
be able to understand the fixed-size variant.

* watchtower/blob/justice_kit: use randomized 192-bit nonce

This commit modifies the blob encryption scheme to
use chacha20-poly1305 with a randomized 192-bit nonce.
The previous approach used a deterministic nonce scheme,
which is being replaced to simplify the requirements of
a correct implementation.  As a result, each payload
gains an addtional 24-bytes prepended to the ciphertext.

* watchtower/blob/justice_kit_test: remove external nonce

The nonce is now passed in as the prefix to the
ciphertext, and is generated randomly in calls
to Encrypt.

* watchtower/wtdb/session_info: compute rewards outputs

* watchtower/wtdb/mock: adds lookout-related mock functions

* watchtower/lookout/log: adds lookout subsystem logger

* watchtower/lookout/interface: adds primary lookout ifaces

* watchtower/lookout/justice_descriptor: adds justice txn creation

* watchtower/lookout/punisher: adds Punisher craft+bcast justice txn

* watchtower/lookout/lookout: adds Lookout

* watchtower/lookout/mock: adds mock backend

* watchtower/lookout/lookout_test: adds simple lookout tests

* watchtower/lookout/justice_descriptor_test: add create txn test

* lnrpc: accept pubkey in hop message

By passing a pubkey into SendToRoute, it becomes unnecessary for lnd to
query the channel graph to retrieve the hop pubkey. This allows routes
over private channels that are not present in the graph.

* config: add option to disable incoming push amounts in OpenChannel

This is useful for merchant-side prevention of accidental pushes
during channel opening.

* fundingmanager test: add test for 'rejectpush' option

* peer: remove quit chan from AddMsg signature

* discovery/gossiper: bypass main event loop for queries

This commit restructures the delivery of gossip
query related messages, such that they are delivered
directly to the gossip syncers. Gossip query rate
limiting was introduced in lightningnetwork#1824 on a per-peer basis.
However, since all gossip query messages were being
delivered in the main event loop, the end result is
that one rate-limited peer could stall all other
peers.

In addition, since no other peers would be able to
submit gossip-related messages through the blocked
event loop, the back pressure would eventually rate
limit the read handlers of all peers as well.
The end result would be lengthy delays in reading
messages related to htlc forwarding.

The fix is to lift the delivery of gossip query
messages outside of the main event loop. With
this change, the rate limiting backpressure is
delivered only to the intended peer.

* discovery: pass peer quit signal to ProcessQueryMsg

This commit passes the peer's quit signal to the
gossipSyncer when attempt to hand off gossip query
messages. This allows a rate-limited peer's read
handler to break out immediately, which would
otherwise remain stuck until the rate-limited
gossip syncer pulled the message.

* Move pewpew diff log into debug log

* lnd: update copyright notice

* rpcserver: show inactive channels in GetInfo

* add num_inactive_channels field to docs

* lntest: define and export various test constants

* lnd_test: define global test timeouts

In preparation for the added propagation delay by separating the miner
and the chain backend, we increase several timeouts throughout the test,
and extract them into constants that can easily be altered.

* lnd_test: remove log disabling hack

* lntest/node: remove extraneous externalip

* lntest: make DBPath aware of active net

* lnd_test: mine reorged out funding tx

* cnct: fix log line

* cnct: add comment on expiry calculation

* utxonursery: remove unused TxOut serialization funcs

* utxonursery: use StaticFeeEstimator instead of mock in test

* fix two comment typos in signer.go

* lnrpc: modify AbandonChannel REST endpoint

In this commit, we modify the AbandonChannel REST endpoint to avoid
conflicting with the CloseChannel's. Otherwise, if a debug build of lnd
is being used, there's no way of closing channels through the REST API
as it's been overwritten by AbandonChannel.

* Improved color validation - now with fixes and a table driven test

* lnd_test: add waitForChannelPendingForceClose

This commit introduces a new utility method
waitForChannelPendingForceClose, that is used to ensure a force closed
channel has been recognized by the UTXO nursery, and is ready to be
swept as soon as it matures.

The commit also utilizes this method to properly wait before mining
blocks in certain tests, as it makes sure that the UTXO nursery will
react properly to the new blocks.

* lnwallet: update to new SendOutputs signature

* discovery+routing: add FetchLightningNode to ChannelGraphSource interface

* discovery/gossiper: add trace log when skipping unadvertised node ann

* discovery/gossiper: send node anns when constructing full chan proof

In this commit, we allow the gossiper to also broadcast the
corresponding node announcements, if we know of them, of a channel when
constructing its full proof. We do this to ensure peers (other than our
remote peer) receive all the relevant announcements for a channel.

The tests changes were made to ensure the new behavior introduced works
as intended. Previously, the node announcements for each test channel
announcement were not processed, so they never existed from the
gossiper's point of view.

This also addresses an existing flake in the integration test
`testNodeAnnouncement`. This problem arose due to the node announcement
being sent before the connection between Dave (node announcement sender)
and Alice (node announcement receiver) was initiated and the full
channel proof was constructed.

* Fix peer handshake timeout error message

Use proper format verb for handshakeTimeout

* removes duplicated default message on queryroutes command

* Add EXPOSE directive for 9735 (p2p) & 10009 (rpc)

See EXPOSE additional directive line 32

* lnwallet/btcwallet: check output is under our control in FetchInputInfo

In this commit, we add an additional check to btcwallet's FetchInputInfo
method to ensure the output is actually under control of the wallet.
Previously, the wallet would assume the output was under its control if
the txid of the output was found within the wallet. This is not a safe
assumption to make however, because if we happened to be the sender of
this transaction, it would be found within the wallet but it's not
actually under our control. To fix this, we explicitly check that there
exists an address in our wallet for this output.

* lnwallet/interface_test: add test to detect confirmation of change output spend tx

In this commit, we add a new test to the existing set of wallet tests to
ensure we can properly detect the confirmation of transactions that
spend our change outputs. We do this as a measure to prevent future
regressions from happening where the wallet doesn't request its backend
to be notified of when an on-chain transaction pays to a change address,
like with the recently discovered SendOutputs bug.

As is, this test will not pass until we update the btcwallet dependency
in the next commit.

* build: update btcwallet dependency to address SendOutputs bug

In this commit, we update our btcwallet dependency to include the latest
changes that aim to address the recently discovered SendOutputs bug.

This commit will also allow the lnwallet test case added in the
previous commit to succeed.

* lnwallet/btcwallet: add lightning addr scope before wallet start

In this commit, we add the lightning address scope before the wallet
starts to prevent a race condition between the wallet syncing and adding
the scope itself. This became more apparent with the recent btcwallet
fixes, as several database transactions now occur between the wallet
being started and it syncing.

* lnwallet/btcwallet: check wallet rescan is complete within IsSynced

In this commit, we add an additional check to btcwallet's IsSynced
method to ensure that it is not currently undergoing a rescan. We do
this to block upon starting the server and all other dependent
subsystems until the rescan is complete.

* build: update to latest version of btcwallet

In this commit, we update to the latest version of btcwallet. This
version includes a bug fix which ensures that the wallet birthday sanity
check only executes once and not each time the deamon is restarted.

* build: bump version to 0.5.1

* build: update btcwallet dep to fix birthday block sanity check infinite loop

In this commit, we update our btcwallet dependency that includes a fix
to address an issue that would cause users to be stuck in an infinite
loop by fetching the same candidate birthday block due to its height not
being updated if the sanity check was attempting to fix an estimate in
the future.

* lnwallet/btcwallet: return best header timestamp while wallet is rescanning.

One way applications built on top of lnd can estimate sync percentage is
through comparing the current time to the best known timestamp of the
lnd wallet's sync state. Therefore, we should always return this
information even if the the wallet is not synced.

* lnd_test: remove unused math/rand rependency

* lnd_test: define createPayReqs helper method

* lnd_test: define helper getChanInfo

* lnd test: refactor testDataLossProtection

This extracts part of the test into a new helper method timeTravel,
which can be used to easily reset a node back to a state where channel
state is lost.

* peer: correct variable name, print error

* channeldb/legacy_serialization: add deserializeCloseChannelSummaryV6

This commit adds a new file legacy_serialization.go, where a copy of the
current deserializeCloseChannelSummary is made, called
deserializeCloseChannelSummaryV6.

The rationale is to keep old deserialization code around to be used
during migration, as it is hard maintaining compatibility with the old
format while changing the code in use.

* channeldb/channel: write boolean to indicate presence of ChannelCloseSummary fields

* channeldb/migrations: migrate ChannelCloseSummary

* channeldb/channel: add LastChanSync field to CloseChannelSummary

This commit adds an optional field LastChanSyncMsg to the
CloseChannelSummary, which will be used to save the ChannelReestablish
message for the channel at the point of channel close.

* lnwallet+link: make ChanSyncMsg take channel state as arg

This lets us get the channel reestablish message without creating the LightningChannel struct first.

* contractcourt/chain_watcher: add channel sync message to CloseChannelSummary

* channeldb/db: define FetchClosedChannelForID

FetchClosedChannelForID is used to find the channel close summary given
only a channel ID.

* channeldb/db_test: add TestFetchClosedChannelForID

* peer: define resendChanSyncMsg

This method is used to fetch channel sync messages for closed channels
from the db, and respond to the peer.

* chain_watcher: poll for commit point in case of failure

We pool the database for the channel commit point with an exponential
backoff. This is meant to handle the case where we are in process of
handling a channel sync, and the case where we detect a channel close
and must wait for the peer to come online to start channel sync before
we can proceed.

* lnd test: add offline scenario to testDataLossProtection

This adds the scenario where a channel is closed while the node is
offline, the node loses state and comes back online. In this case the
node should attempt to resync the channel, and the peer should resend a
channel sync message for the closed channel, such that the node can
retrieve its funds.

* htlcswitch/link: remove handled TODO

* channeldb/migrations_test: add TestMigrateOptionalChannelCloseSummaryFields

* lnwallet/channel: remove startingHeight from commitmentChain

Field is not being used.

* rpc: properly set output index in OpenChannelSync resp

Fixes lightningnetwork#2219

* lnd_test: use global channelOpenTimeout when opening channels

* lnd_test: use global channelCloseTimeout when closing channels

* lnd_test: use global minerMempoolTimeout when waiting for miner to see TXs

* lnd_test: use global defaultTimeout instead of test specific timeouts

* lnd_test: make test use globale defaultCSV

* lntest/harness: retry ConnectPeer of chain backend still syncing

* Docs: code_contribution_guidelines change link

* fix merge

* rebuild Gopkg.lock with updated dep

* remove hanging files from merge

* remove duplicated settleindex reference from merge

* remove duplicated edgePolicyWithSource definition from merge

* remove unused chainntnfs dep from merge

* remove duplicated logRotator from merge

* remove duplications from merge

* remove whitespace change from merge

* remove whitespace change from merge

* remove whitespace change from merge

* add missed parameter from merge

* update link test for changes

* bad reference in link test

* configure extpreimage client when creating a link

* use new return values from AddInvoice

* upgrade ltcd

* don't use fee estimator on litecoin regtest

* update invoice tests

* run fmt

* linter fixes
@cfromknecht

This comment has been minimized.

Copy link
Collaborator

cfromknecht commented Dec 5, 2018

tACK, just tested on my node; thanks a bunch @AdamISZ!

I have one minor cosmetic comment, which I think has an easy fix. Since we use the ChannelPoint proto object to serialize the outpoints, the json fields show "funding_txid_str":

            "outpoint": {
                "funding_txid_str": "b616b49afa2b724a71469e798c9dbff24f11f3bfddd38a3f3ef356e6a388926a",
                "output_index": 0
            },

I think it might be worth adding an OutPoint type to lnrpc and using it here. (IMO arguably ChanPoint should just use this as well, but that's out of scope). Idt this is a blocker on the PR though, as it would be an easy fix in a follow up and since we're in between releases it should be fine if the final API changes slightly.

Appreciate all the hard work on this feature. I know a lot people have requested it, so thanks again :)

@AdamISZ

This comment has been minimized.

Copy link
Contributor

AdamISZ commented Dec 5, 2018

On funding_txid_str: doh, good point :)

So I can just squash this and let you fix that after the merge; I'll do that later today unless someone wants something else. Cheers.

@halseth
Copy link
Collaborator

halseth left a comment

Commits need squashing :)

"confirmations")
}

usingMax := maxConfs != math.MaxInt32

This comment has been minimized.

@halseth

halseth Dec 5, 2018

Collaborator

nit: can jsut do this comparison in the if statement itself, since variable is not used elsewhere.

This comment has been minimized.

@AdamISZ

AdamISZ Dec 5, 2018

Contributor

Done


usingMax := maxConfs != math.MaxInt32
if usingMax {
rpcsLog.Infof("[listunspent] min=%v max=%v", minConfs, maxConfs)

This comment has been minimized.

@halseth

halseth Dec 5, 2018

Collaborator

Maybe we can just restrict logging to the final log statement?

maxStr := ""
if ... {
    maxStr = " max="+maxConf
}
rpcsLog.Infof("[listunspent] min=%v%v generated utxos: %v", maxStr, utxos)

This comment has been minimized.

@AdamISZ

AdamISZ Dec 5, 2018

Contributor

Done

addrType = lnrpc.AddressType_WITNESS_PUBKEY_HASH
case lnwallet.NestedWitnessPubKey:
addrType = lnrpc.AddressType_NESTED_PUBKEY_HASH
}

This comment has been minimized.

@halseth

halseth Dec 5, 2018

Collaborator

Maybe warn level?

},
OutputIndex: utxo.OutPoint.Index,
}
utxoresp := lnrpc.Utxo{

This comment has been minimized.

@halseth

halseth Dec 5, 2018

Collaborator

naming nit: utxoResp

This comment has been minimized.

@AdamISZ

AdamISZ Dec 5, 2018

Contributor

Done

Outpoint: outpoint,
Confirmations: utxo.Confirmations,
}
_, outAddresses, _, err := txscript.ExtractPkScriptAddrs(

This comment has been minimized.

@halseth

halseth Dec 5, 2018

Collaborator

add some newlines between statements here

This comment has been minimized.

@AdamISZ

AdamISZ Dec 5, 2018

Contributor

Done

@AdamISZ AdamISZ force-pushed the AdamISZ:listunspent branch from 76fbe5c to bba3369 Dec 5, 2018

@AdamISZ

This comment has been minimized.

Copy link
Contributor

AdamISZ commented Dec 5, 2018

Squashed + addressed nits, cheers.

@halseth
Copy link
Collaborator

halseth left a comment

I think this is ready to land after a rebase! 👍

type isKeyDescriptor_Key interface {
isKeyDescriptor_Key()
}
type isKeyDescriptor_Key interface{ isKeyDescriptor_Key() }

This comment has been minimized.

@halseth

halseth Dec 11, 2018

Collaborator

I see this is conflicting with master now. You should rebase and regenerate protos.

return nil, err
}

var respList = lnrpc.ListUnspentResponse{Utxos: []*lnrpc.Utxo{}}

This comment has been minimized.

@halseth

halseth Dec 11, 2018

Collaborator

style nit:

resp := &lnrpc.ListUnspentResponse{}
...
resp.Utxos = append(resp.Utxos, &utxoResp)
...
return resp, nil

This comment has been minimized.

@AdamISZ

AdamISZ Dec 11, 2018

Contributor

OK; changed to that syntax.

Add listunspent RPC call
Returns a brief json summary of each utxo found by calling
ListUnspentWitness in the wallet. The two arguments are the
minimum and maximum number of conrfirmations (0=include
unconfirmed)

@AdamISZ AdamISZ force-pushed the AdamISZ:listunspent branch from bba3369 to 9bb2a26 Dec 11, 2018

@AdamISZ

This comment has been minimized.

Copy link
Contributor

AdamISZ commented Dec 11, 2018

Rebased, squashed and style nit addressed, cheers.

@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 🌋

@Roasbeef Roasbeef merged commit 2352918 into lightningnetwork:master Dec 12, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on listunspent at 55.754%
Details

taketa added a commit to LightningPeach/lnd that referenced this pull request Dec 27, 2018

Add a maxconfirms argument to ListUnspentWitness
This change was inspired by lightningnetwork#1984 - the underlying call to
ListUnspent supports a (min, max) range so it makes sense that
the WalletController interface can also support this; a
default no-maximum can be expressed using a MaxInt32 value.
@alexbosworth

This comment has been minimized.

Copy link
Contributor

alexbosworth commented Jan 3, 2019

May be an issue with this method when listing unspents after a seed restore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment