Add util fn for creating a Transaction from spendable outputs #789

TheBlueMatt · 2021-02-04T00:58:30Z

Based on #786, This adds a utility method, KeysManager::spend_spendable_outputs,
which constructs a Transaction from a given set of
SpendableOutputDescriptors, deriving relevant keys as needed.

It also adds methods which can sign individual inputs where
channel-specific key derivation is required to
InMemoryChannelKeys, making it easy to sign transaction inputs
when a custom KeysInterface is used with InMemoryChannelKeys.

codecov · 2021-02-04T01:09:23Z

Codecov Report

Merging #789 (95d0fe9) into main (2088f4b) will increase coverage by 0.01%.
The diff coverage is 95.66%.

@@            Coverage Diff             @@
##             main     #789      +/-   ##
==========================================
+ Coverage   90.75%   90.77%   +0.01%     
==========================================
  Files          44       44              
  Lines       24075    24282     +207     
==========================================
+ Hits        21849    22041     +192     
- Misses       2226     2241      +15

Impacted Files	Coverage Δ
lightning/src/chain/channelmonitor.rs	`95.68% <66.66%> (ø)`
lightning/src/chain/keysinterface.rs	`93.36% <95.20%> (+0.80%)`	⬆️
lightning/src/util/transaction_utils.rs	`98.06% <96.29%> (-1.94%)`	⬇️
lightning/src/ln/chan_utils.rs	`97.34% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.95% <100.00%> (-0.03%)`	⬇️
lightning/src/util/macro_logger.rs	`88.33% <100.00%> (ø)`
lightning/src/util/test_utils.rs	`83.16% <100.00%> (-1.66%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2088f4b...95d0fe9. Read the comment docs.

lightning/src/chain/keysinterface.rs

devrandom · 2021-02-05T16:17:17Z

lightning/src/chain/keysinterface.rs

+/// Information about a spendable output to a P2WSH script. See
+/// SpendableOutputDescriptor::DynamicOutputP2WSH for more details on how to spend this.
+#[derive(Clone, Debug, PartialEq)]
+pub struct DynamicP2WSHOutputDescriptor {


Probably not for this PR, but maybe this can be DelayedPaymentFromHolderOutput and the one below can be StaticPaymentFromCounterpartyOutput?

Right, there are a few things in keysinterface that could use renaming, so maybe we can do it as a follow up - I'd like to rename ChanKeys to ChannelSigner (and a bunch of things downstream that use similar naming) as well.

Went ahead and did this since Val had a further naming comment.

lightning/src/chain/keysinterface.rs

devrandom · 2021-02-05T16:53:25Z

lightning/src/chain/keysinterface.rs

-	/// script_pubkey as it appears in the output.
+	/// An output to a script which was provided via KeysInterface directly, either from
+	/// `get_destination_script()` or `get_shutdown_pubkey()`, thus you should already know how to
+	/// spend it. No secret keys are provided as rust-lightning was never given any key.
 	/// These may include outputs from a transaction punishing our counterparty or claiming an HTLC
 	/// on-chain using the payment preimage or after it has timed out.
 	StaticOutput {


it would be good to have the derivation path here (and maybe channel ID - see comment nearby), even if it's not used, just so that the user / KeysManager implementation can keep track of which channel is being swept

Note this is really in #786. It would feel awkward to tie the scripts to a channel without moving the functions to get them to the ChannelSigner? I dont really want to do that, though, as the current setup is great in that the KeysInterface implementation is trivial and gives you direct control over all the entropy used and keys used directly (that aren't fixed by the lightning protocol), and the ChannelSigner implementation is all about the lightning protocol details.

I guess we could maybe pass the channel information to the functions in KeysInterface to get the fields as a middle ground, but ultimately a user probably would just want some kind of integer id to figure out what HD derivation path they used, not channel information, and I'm not really sure how to capture that easily.

Oh, this wasn't about the scripts at all. It's about this struct (StaticOutput), which is constructed in ChannelMonitor when sweeping a payment to us. I'm just saying it would be good for the KeysManager to know which channel is being swept for housekeeping / policy controls. ChannelMonitor has the derivation ID handy, so it's easy to populate such a field in this struct.

Right, it just felt awkward that we pass an id referencing the derivation id when the script/output itself isn't in any way related to a given derivation, the KeysManager returns them directly. Best would be to pass in the derivation info to the keysmanager when we get those scripts, I think, but we don't have it yet at that point, afair.

OK, there's no rush to get into this in this PR.

lightning/src/chain/keysinterface.rs

devrandom · 2021-02-08T15:41:45Z

lightning/src/chain/keysinterface.rs

+	/// The output which is referenced by the given outpoint
+	pub output: TxOut,
+	/// The revocation_pubkey used to derive witnessScript
+	pub revocation_pubkey: PublicKey,


isn't this available from ChannelKeys, so doesn't need to be replicated here?

No, this is the revocation point for the broadcast commitment transaction. I'll clarify the docs.

Ah, we can't re-derive the revocation_pubkey, because that would require the counterparty's revocation base, which we don't have because we rederived ChannelKeys from scratch and therefore it doesn't have the counterparty's pubkeys.

However, we could still make it work if we looked up the populated ChannelKeys instead of re-deriving them.

So this brings up the question - why are we using derive_channel_keys instead of looking up the existing ChannelKeys in some index?

Right. Depending on the specific KeysInterface implementation we could look up a globally-cached remote public keys object. However we should try to be as implementation-agnostic as possible, so in this case I'm not sure it's ideal. We could provide the raw data to re-derive in the event, but I'm not sure what that protects against - if they even is bogus no funds loss can occur, the client may just create an invalid transaction.

It's pretty minor, just felt a bit more "normalized" to provide just the minimum set of fields.

lightning/src/chain/keysinterface.rs

We previously counted 35 bytes for a length + public key, but in reality they are never larger than 34 bytes - 33 for the key and 1 for the push length.

Both SpendableOutputDescriptor::DynamicOutputP2WSH and SpendableOutputDescriptor::StaticOutputCounterpartyPayment are relevant only in the context of a given channel, making them candidates for being passed into helper functions in `InMemoryChannelKeys`. This moves them into their own structs so that they can later be used standalone.

TheBlueMatt · 2021-02-16T17:40:36Z

Rebased and squashed after dependent-PR merge with no other changes.

valentinewallace

Super useful utility! Seems like a big win for users.

valentinewallace · 2021-02-16T18:20:35Z

lightning/src/chain/keysinterface.rs

+/// Information about a spendable output to a P2WSH script. See
+/// SpendableOutputDescriptor::DynamicOutputP2WSH for more details on how to spend this.
+#[derive(Clone, Debug, PartialEq)]
+pub struct DynamicP2WSHOutputDescriptor {


nit: this could be renamed to be uniform to DynamicOutputP2WSH: i.e., rename to DynamicOutputP2WSHDescriptor (or vice versa)

Did something more akin to devrandom's suggestion above #789 (comment)

lightning/src/chain/keysinterface.rs

KeyManager::new() took a bitcoin::Network parameter which needs to be passed to the BIP 32 Extended Key constructor, but because we never write out the BIP 32 serialization, it isn't used. Instead, we just pass a dummy value into `ExtendedPrivKey`, dropping the unused argument to KeysManager::new().

Previously, test_dynamic_spendable_outputs_local_htlc_success_tx called connect_block with two identical transactions, which resulted in duplicate SpendableOutputs Events back-to-back. This is a test issue as such a block_connected call represents an invalid block.

This adds a utility method, `KeysManager::spend_spendable_outputs`, which constructs a Transaction from a given set of `SpendableOutputDescriptor`s, deriving relevant keys as needed. It also adds methods which can sign individual inputs where channel-specific key derivation is required to `InMemoryChannelKeys`, making it easy to sign transaction inputs when a custom `KeysInterface` is used with `InMemoryChannelKeys`.

Both Miron and Val suggested naming tweaks in their reviews, so clarifying things some would be good.

valentinewallace

New naming + typo fixes LGTM

devrandom reviewed Feb 5, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-02-chansigner-util-fns branch from 5adddfe to 29d6c13 Compare February 5, 2021 21:53

TheBlueMatt mentioned this pull request Feb 6, 2021

Expand documentation and fields in SpendableOutputDescriptors #786

Merged

TheBlueMatt force-pushed the 2021-02-chansigner-util-fns branch 2 times, most recently from bfbcd4e to 84bec9b Compare February 6, 2021 20:00

devrandom approved these changes Feb 8, 2021

View reviewed changes

TheBlueMatt added 2 commits February 16, 2021 12:40

[tests] Correct witness len calc in StaticOutputCounterpartyPayment

311555a

We previously counted 35 bytes for a length + public key, but in reality they are never larger than 34 bytes - 33 for the key and 1 for the push length.

TheBlueMatt force-pushed the 2021-02-chansigner-util-fns branch from e16b8dc to 25a17d9 Compare February 16, 2021 17:40

valentinewallace self-requested a review February 16, 2021 17:50

valentinewallace approved these changes Feb 16, 2021

View reviewed changes

TheBlueMatt added 8 commits February 16, 2021 15:58

f fix typos val caught

e7784a6

Add utility function to add a change output to a transaction

294cba4

Switch to calling new KeysManager output-spending fn in tests

72186db

Drop stale and cleanup TODO entries in keysinterface

0df01c1

Clarify docs on revocation_pubkey in DynamicP2WSHOutputDescriptor

8dbadd1

TheBlueMatt force-pushed the 2021-02-chansigner-util-fns branch from 25a17d9 to 8dbadd1 Compare February 16, 2021 21:32

Rename output descriptor types for some clarity

95d0fe9

Both Miron and Val suggested naming tweaks in their reviews, so clarifying things some would be good.

TheBlueMatt mentioned this pull request Feb 16, 2021

Rename ChannelKeys -> Sign and generic it consistently #799

Merged

valentinewallace approved these changes Feb 17, 2021

View reviewed changes

TheBlueMatt merged commit 414e622 into lightningdevkit:main Feb 18, 2021

Add util fn for creating a Transaction from spendable outputs #789

Add util fn for creating a Transaction from spendable outputs #789

Uh oh!

Conversation

TheBlueMatt commented Feb 4, 2021

Uh oh!

codecov bot commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devrandom Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devrandom Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Feb 16, 2021

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Feb 4, 2021 •

edited

Loading

devrandom Feb 5, 2021 •

edited

Loading

devrandom Feb 10, 2021 •

edited

Loading