lnrpc: Add destination output information (pkScript, output index, amount and if output belongs to the node)

[bjarnemagnussen]

Fixes #2835.

This PR adds destination output information to the TransactionDetail structure returned by both RPC GetTransactions and SubscribeTransactions.

The additional fields include the output script (PkScript), amount (Amount), index (OutputIndex) and a bool (IsOurAddress) to denote if the output belongs to the node's internal wallet or not.

Since all of this information is already available when constructing the TransactionDetail, this information can just be added as it can be useful in many applications and depending on the use case may also solve issue #4826.

Note

The output index does not simply correspond to the position in the DestOutput array, as unrecognized outputs are skipped:

lnd/lnwallet/btcwallet/btcwallet.go

Lines 927 to 935 in 756a0ab

	for _, txOut := range wireTx.TxOut {
	_, outAddresses, _, err := txscript.ExtractPkScriptAddrs(
	txOut.PkScript, chainParams,
	)
	if err != nil {
	// Skip any unsupported addresses to prevent
	// other transactions from not being returned.
	continue
	}

[guggero]

Great PR, thank you! This additional info should be very useful.

I think we should merge the new info with the address internally and deprecate the address field in the RPC. But other than that looks great to me!

[guggero]

The output index does not simply correspond to the position in the DestOutput array, as unrecognized outputs are skipped:

Perhaps we could even optimize the output here. If we can't parse the address, we set the address to nil (and empty string on the RPC) but still include the other details. That way we could get rid of that continue statement you linked.

[bjarnemagnussen]

The output index does not simply correspond to the position in the DestOutput array, as unrecognized outputs are skipped:

Perhaps we could even optimize the output here. If we can't parse the address, we set the address to nil (and empty string on the RPC) but still include the other details. That way we could get rid of that continue statement you linked.

I agree and think this makes a lot of sense. My only reservation is in the way that the address is calculated. As it currently is, the returned address for a single output script is a slice of potential multiple addresses. Normally the slice will just contain a single valid address that corresponds to exactly the output script. However, in the extremely rare case of a MultiSig script NOT encoded via a P2SH, the returned slice of addresses will simply contain a list of P2PKH addresses from the multisig script (since the direct multisig script does not have an address representation).

I have therefore added some logic to make sure in this case we return an empty address.

[bjarnemagnussen]

I have squashed the new changes:

• Remove DestAddresses from the lnwallet.TransactionDetail structure as it is otherwise redundantly available via OutputDetails and just re-populated to the RPC for backwards compatibility.
• Introduced another field to OutputDetail called OutputType that will be of OutputScriptType to denote the kind of output script. However, I am not sure if alternatively defining it as a string and using the String() method on a ScriptClass is the more pragmatic solution? (See the definition here)

[guggero]

Looks good, thanks for adding the new enum. Just one comment about the enum values and a few nits, other than that I think this is ready.

[guggero]

Thank you for the super quick updates, LGTM 🎉

[lightninglabs-deploy]

@ellemouton: review reminder

[ellemouton]

left some comments. Also, needs a rebase :)

[ellemouton]

in the first commit, the OutputDetail struct has an array of addresses instead of just 1. I assumed that is for a multisig script or something? in anycase, if we do a slice there, then this should also be be repeated. otherwise, the struct should be changed to just have 1 addr too.

[bjarnemagnussen]

The reason to use a slice for it was to deprecate DestAddresses but still expose it on the RPC for backwards compatibility. However, we can also re-introduce the DestAddresses directly to the interface and have the OutputDetail only contain the single destination address if available?

[ellemouton]

yeah so following up on my comment from before, what about the other addresses here?

[bjarnemagnussen]

The DestAddresses field is a bit weird, because if an output is not re-presentable by an address (e.g. due to NonStandard type or the MultiSigType), then the indices in the DestAddresses do not correspond to the outputs.

The reason that MultiSigType returns a slice of addresses, is because this is the case where the multisig script is directly put inside the scriptPubKey, hence not behind a hash. Therefore it cannot be represented by an address. It is questionable if a slice of addresses for the pubkeys referred to in the multisig script is helpful though. :)

[ellemouton]

ok cool, chatted to oli about this and he agrees with you 👍

[bjarnemagnussen]

Thanks @ellemouton for your comments! I have rebased this PR. Also I have tried to clarify the weird behaviour of the legacy DestAddresses field in a comment and am open to handle it differently.

[ellemouton]

tACK 🚀

i think it needs another rebase @bjarnemagnussen

[bjarnemagnussen]

Thanks @ellemouton! I have rebased and added release notes!