Introduce CommitmentTransactionInfo #742

devrandom · 2020-10-17T15:44:48Z

This is part of the revamp of the signer API (AKA "phase 2", also see #408) where we want to pass around the semantic transaction rather than the Bitcoin transaction.

CommitmentTransaction (the semantic transaction) maintains the per-commitment transaction fields needed to construct the associated bitcoin transactions (commitment, HTLC). This will allow a higher level of assurance that all relevant aspects of the transactions were checked for policy violations.

ChannelTransactionParameters replaces passing around of individual per-channel fields that are needed to construct Bitcoin transactions.

This change affects Channel, ChannelMonitor and ChannelKeys.

Steps for a future PR:

eliminate other uses of the bitcoin transaction as appropriate

codecov · 2020-10-17T18:04:21Z

Codecov Report

Merging #742 (9291a38) into main (74cd96f) will increase coverage by 0.06%.
The diff coverage is 95.89%.

@@            Coverage Diff             @@
##             main     #742      +/-   ##
==========================================
+ Coverage   91.23%   91.29%   +0.06%     
==========================================
  Files          37       37              
  Lines       22623    22791     +168     
==========================================
+ Hits        20639    20807     +168     
  Misses       1984     1984

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`90.35% <ø> (+2.89%)`	⬆️
lightning/src/chain/keysinterface.rs	`94.18% <92.68%> (-0.66%)`	⬇️
lightning/src/ln/channel.rs	`87.51% <94.44%> (+0.05%)`	⬆️
lightning/src/ln/chan_utils.rs	`97.14% <95.17%> (+1.48%)`	⬆️
lightning/src/chain/channelmonitor.rs	`95.58% <100.00%> (+0.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.93% <100.00%> (-0.25%)`	⬇️
lightning/src/ln/onchaintx.rs	`93.78% <100.00%> (ø)`
lightning/src/util/enforcing_trait_impls.rs	`100.00% <100.00%> (ø)`
... and 1 more

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 74cd96f...9291a38. Read the comment docs.

devrandom · 2020-10-18T14:49:57Z

sketched a replacement to HolderCommitmentTransaction

lightning/src/ln/chan_utils.rs

ariard

Thanks for working on this, there is a general point about binding to the broadcaster/countersignatory nomenclature for HolderCommitmentTxInfo//ChannelStaticInfo as these data structures are used to generate holder/counterparty transaction and the direction is caller-dependant.

Overall, I like the change to have a clean transaction store, independent of direction.

lightning/src/ln/chan_utils.rs

ariard · 2020-10-29T01:34:28Z

lightning/src/chain/channelmonitor.rs


 	destination_script: Script,
 	broadcasted_holder_revokable_script: Option<(Script, PublicKey, PublicKey)>,
 	counterparty_payment_script: Script,
 	shutdown_script: Script,

 	keys: ChanSigner,
+	counterparty_pubkeys: ChannelPublicKeys,


I don't follow this change at reviewing ec16cf2, is it consumed somewhere ?

Further, I even think we could drop any counterparty pubkeys by extending ChannelKeys with counterparty_pubkeys as they're stored their post channel acceptance. ChannelMonitor already have access to key signer, so it's not an issue calling at counterparty script generation.

It might be a low latency hit in case of external signer out-of-memory but onchain txns should be sufficiently limited to not care about it.

Note, we can't directly store the scripts as they're function of the per_commitment_point we only derive at counterparty commitment tx detection.

I don't follow this change at reviewing ec16cf2, is it consumed somewhere ?

I removed this field, my mistake.

Further, I even think we could drop any counterparty pubkeys by extending ChannelKeys with counterparty_pubkeys as they're stored their post channel acceptance. ChannelMonitor already have access to key signer, so it's not an issue calling at counterparty script generation.

@ksedgwic and I were thinking about that, but I'm a bit concerned about making the node dependent on the signer for transaction construction. I wonder if it may make it harder to perform some system-administration operations when the signer is unavailable for some reason. But otherwise, that's something that can be done in the followup PR.

Just fyi, you have access to counterparty keys in the following contexts:

To claim a revoked to_local output on counterparty tx, L1318 in src/chain/channelmonitor.rs

To claim a revoked HTLC output on counterparty tx, L1331 in src/chain/channelmonitor.rs

To claim a valid HTLC output on counterparty tx, L1470 in src/chain/channelmonitor.rs

To claim a revoked output on counterparty HTLC tx, L1502 in src/chain/channelmonitor.rs

All those cases assume your monitor is feeded with blocks (block_connected) but I guess in system-administration you will turn off chain watching to avoid a state update ? That said I agree it's hard to scope yet and orthogonal to this PR.

lightning/src/ln/channel.rs

lightning/src/ln/chan_utils.rs

ariard · 2020-10-29T02:08:25Z

lightning/src/ln/chan_utils.rs

+/// This class tracks the information needed to build a holder's commitment transaction and to actually
+/// build and sign.  It is used for holder transactions that we sign only when needed.
+#[derive(Clone)]
+pub struct HolderCommitmentTransactionInfo {


As a name suggestion what about HolderRawChannelTxn, what we want to convey is a) this is scoping both commitment and htlc txn b) this is "raw" in opposition to signer finalized.

I renamed this to HolderCommitmentTransaction (and renamed the old one to something else). I feel that "Raw" isn't that descriptive.

Let me know what you think.

I like dropping the Info on new names.

I would like to convey better the different commitment package states : channel_parameters (step 1) --> channel_parameters + CommitmentTransaction (dynamic) (step 2) --> channel_parameters + CommitmentTransaction + counterparty_sigs (step 3) --> channel_parameters + CommitmentTransaction + counterparty_sigs + holder_sigs (step 4).

Note that you have a bifurcation at (step 2) if it's for counterparty, you will produce the holder sigs and send them through commitment_signed. Otherwise, if it's for holder, you will pass the state to our chain monitor.

That's the gross graph I hope we move towards to make it clearer for future reviewers, but not need to stick to it rn.

lightning/src/ln/chan_utils.rs

ariard

Thanks for update, I think this comment still needs resolution, afterwards SGTM.

lightning/src/chain/channelmonitor.rs

lightning/src/ln/channel.rs

devrandom · 2020-11-06T14:29:03Z

I reworded the commits and added a rename as follows:

ChannelStaticInfo -> ChannelTransactionParameters
HolderCommitmentTransaction -> HolderCommitmentTransactionForSigning (to be removed later)
CommitmentTransactionInfo -> CommitmentTransaction
HolderCommitmentTransactionInfo -> HolderCommitmentTransaction

Curious how people feel about the naming.

lightning/src/ln/chan_utils.rs

lightning/src/ln/channel.rs

ariard

See the small nits on new DirectedChannelTransactionParameters, otherwise SGTM

TheBlueMatt

Generally looks good, thanks! I think there's some opportunities for cleanup and optimization yet, but the structure seems good.

lightning/src/ln/chan_utils.rs

lightning/src/chain/channelmonitor.rs

lightning/src/ln/chan_utils.rs

TheBlueMatt · 2020-11-10T17:43:14Z

lightning/src/ln/chan_utils.rs

+	broadcaster_is_holder: bool,
+}
+
+impl<'a> DirectedChannelTransactionParameters<'a> {


Since this basically just exists to switch between different fields in ChannelTransactionParameters can it be replaced with a few functions in impl ChannelTransactionParameters that take either a direction bool or a CommitmentTransaction (or functions in CommitmentTransaction that take ChannelTransactionParameters instead?)? It just seems like quite a bit of code for a few if is_outbound { a } else { b } blocks.

This was changed in 66949ce after discussion with @ariard. It seems a bit cleaner API this way, but I'm neutral.

Note that the direction is not about outboundness (who's the initiator) but about who's the broadcaster.

Note if you do this and consume directly ChannelTransactionParameters in CommitmentTransaction builder you will re-introduce directionality about code path which are exercised for building commitments for both sides, which is confusing.

Your direction bool is likely to be something like holder_is_broadcaster ?

TheBlueMatt · 2020-11-10T18:28:28Z

lightning/src/ln/chan_utils.rs

+	/// The backwards-counting commitment number
+	pub commitment_number: u64,
+	/// The value to be sent to the broadcaster
+	pub to_broadcaster_value_sat: u64,


Shouldn't the explicit values be replaced with the set of dust HTLCs so that an external signer can at least see the dust HTLCs, even if they aren't included?

the only interesting policy on dust HTLCs that I came up with is a limit on their aggregate value, which can be enforced without knowing their details. any other policies you can think of?

Hmm, maybe, I was thinking also that its better to force the external signer to re-check the values against dust instead of letting them accidentally accept a commitment transaction that burns their entire value to dust unreasonably - just in general its more explicit and we communicate the data needed to build the transaction, not the conclusions the non-trusted signer made from that data.

I still don't see how the signer would use this information. Here are my thoughts:

the info in the dust HTLCOutputInCommitment cannot be verified by the signer. It could have been invented by a compromised node.

as opposed to the non-dust HTLCs, the info is not going on-chain and is not signed by the counterparty, so doesn't describe any recourse we have

So it seems like the only thing the signer can do with this is sum the individual values and see everything adds up, but that doesn't seem to buy us any additional security, since we already know the result.

The one policy I can see related to this is a limit on the sum of dust HTLCs, which can be computed from the existing fields already.

Am I missing something?

I was thinking that a signer may use information about dust HTLCs in one channel to correlate with non-dust HTLCs in another channel (with a different dust limit) that represent forwarding of the same payment. Or possibly correlate HTLCs in the inbound/outbound commitment transactions in the same channel, since IIRC both could have different dust limits.

I admit I don't in any way have a coherent threat model for how using this data would be helpful, only that it seems strange to not pass it to the signer.

TheBlueMatt · 2020-11-10T18:36:15Z

lightning/src/ln/chan_utils.rs

+	}
+
+	fn build_outputs<T: secp256k1::Signing + secp256k1::Verification>(&self, channel_parameters: &DirectedChannelTransactionParameters, secp_ctx: &Secp256k1<T>) -> Result<Vec<(TxOut, (Script, Option<HTLCOutputInCommitment>))>, ()> {
+		let htlcs = self.htlcs.iter().map(|h| (h.clone(), ())).collect();


This seems pretty redundant - are there callers where we really need clones of HTLCOutputInCommitment or can we just let clients access the already-newly-pub htlcs field in the struct?

this was mostly because I didn't want to duplicate do_build_outputs for the two cases: initial sort of HTLCSource aux data, and later building of the transaction which doesn't need aux data. I cleaned this up in 3c35f8d and b2ce5e8 - let me know what you think.

lightning/src/ln/chan_utils.rs

lightning/src/chain/keysinterface.rs

ariard

Maybe squash commits for final review ?

lightning/src/ln/chan_utils.rs

devrandom · 2020-12-29T19:16:36Z

Squashed.

TheBlueMatt

Basically happy with this. A few comments, but they're all simple doc/API/one-liners, so should be trivial. Need one more ACK, but I'm happy.

lightning/src/chain/keysinterface.rs

TheBlueMatt · 2020-12-29T21:21:11Z

lightning/src/chain/keysinterface.rs

+	/// Set the counterparty static channel data, including basepoints,
+	/// counterparty_selected/holder_selected_contest_delay and funding outpoint.
+	/// This is done as soon as the funding outpoint is known.  Since these are static channel data,
+	/// they MUST NOT be allowed to change to different values once set.


Maybe note in the docs that channel_parameters.is_populated() will be true. In a future change it may make sense to have a PendingOutboundChannel object that gets converted to a Channel when we receive our counterparty data to remove a ton of the Optional stuff in Channel, including makeing counterparty_parameters in here non-Optional

The phrasing here is awkward - its not that is_populated() MUST be true (implying some kind of requirement on the user), but that it WILL be true (as it is a requirement for our own internal code, thus an API guarantee provided to the user).

The ChannelKeys API is also used internally in an external signer. i.e. it's called directly by user code.

lightning/src/chain/keysinterface.rs

lightning/src/ln/chan_utils.rs

TheBlueMatt · 2020-12-29T23:25:03Z

lightning/src/ln/chan_utils.rs

+	// - initial sorting of outputs / HTLCs in the constructor, in which case T is auxiliary data the
+	//   caller needs to have sorted together with the HTLCs so it can keep track of the output index
+	// - building of a bitcoin transaction during a verify() call, in which case T is just ()
+	fn internal_build_outputs<T>(keys: &TxCreationKeys, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters) -> Result<(Vec<TxOut>, Vec<HTLCOutputInCommitment>, Vec<Script>), ()> {


nit: this could probably use an #[inline]

Does this actually do anything? isn't the compiler smart enough to inline if it's appropriate? also, isn't the gain pretty minimal, given everything that this function does internally?

Probably nothing, indeed. The function is probably big enough that it may not qualify for normal inlining, but also does work which certain callers don't care about (and the callers are generally tiny functions) - making it a decent candidate for "LLVM, you should try inlining here, even if you think this function is big enough that you wouldn't otherwise". That said, a bunch of that extra work is probably complicated enough that LLVM wouldn't be able to figure out how to skip it, so...whatever.

Got it. Well, do you think LLVM knows to skip populating a vec if the inlining makes it obvious the vec isn't used?

If the contained object didn't have a Drop and you gave it enough CPU budget...maybe? I don't think this is worth discussing in this much depth either way, its just one of those things I always figure doing as the "utility function that is big but only called from tiny exposed functions" pattern can be pessimal for the inliner if code locality isn't critical to the specific use-case :).

lightning/src/ln/chan_utils.rs

lightning/src/ln/channel.rs

lightning/src/ln/chan_utils.rs

TheBlueMatt · 2020-12-30T18:41:44Z

Needs another ACK, but I'm happy modulo resolution of #742 (comment)

CommitmentTransaction maintains the per-commitment transaction fields needed to construct the associated bitcoin transactions (commitment, HTLC). It replaces passing around of Bitcoin transactions. The ChannelKeys API is modified accordingly. By regenerating the transaction when implementing a validating external signer, this allows a higher level of assurance that all relevant aspects of the transactions were checked for policy violations. ChannelTransactionParameters replaces passing around of individual per-channel fields that are needed to construct Bitcoin transactions. Eliminate ChannelStaticData in favor of ChannelTransactionParameters. Use counterparty txid instead of tx in channelmonitor update.

…tion

ariard

Code Review ACK 9291a38, comments are nits.

ariard · 2021-01-03T23:56:30Z

lightning/src/chain/keysinterface.rs

 #[derive(Clone)]
 /// A simple implementation of ChannelKeys that just keeps the private keys in memory.
+///
+/// This implementation performs no policy checks and is insufficient by itself as


Would be cool to describe what kind of policy checks can be implemented for a custom signer. Maybe you can link your already-written set of policy controls examples (https://gitlab.com/lightning-signer/docs/-/blob/master/policy-controls.md) somewhere :) ?

ariard · 2021-01-04T04:34:58Z

lightning/src/chain/keysinterface.rs

+	/// This is done as soon as the funding outpoint is known.  Since these are static channel data,
+	/// they MUST NOT be allowed to change to different values once set.
+	///
+	/// channel_parameters.is_populated() MUST be true.


"...otherwise it will panic".

There's no guarantees of this - there are no requirements around panics placed on the clients of the API.

ariard · 2021-01-04T04:37:15Z

lightning/src/ln/onchaintx.rs

 			}
 		}
 	}

-	//TODO: getting lastest holder transactions should be infaillible and result in us "force-closing the channel", but we may
+	fn extract_holder_sigs(holder_commitment: &HolderCommitmentTransaction, sigs: Vec<Signature>) -> Vec<Option<(usize, Signature)>> {


Could be called "extract_indexed_htlc_sigs"? It will be clearer.

TheBlueMatt · 2021-01-04T17:11:06Z

<ariard> BlueMatt: merge it, they're really super-nits

devrandom changed the title ~~Introduce CommitmentTransactionInfo~~ Draft: Introduce CommitmentTransactionInfo Oct 17, 2020

devrandom force-pushed the tx-phase2 branch from 31037e2 to a016204 Compare October 19, 2020 10:04

devrandom commented Oct 19, 2020

View reviewed changes

lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved

devrandom commented Oct 19, 2020

View reviewed changes

lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved

devrandom force-pushed the tx-phase2 branch 4 times, most recently from ec93084 to c669158 Compare October 27, 2020 14:52

ariard reviewed Oct 29, 2020

View reviewed changes

lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved

devrandom force-pushed the tx-phase2 branch 3 times, most recently from 0a18bd3 to 12f90c5 Compare November 2, 2020 16:24

devrandom changed the title ~~Draft: Introduce CommitmentTransactionInfo~~ Introduce CommitmentTransactionInfo Nov 2, 2020

devrandom marked this pull request as ready for review November 2, 2020 16:35

devrandom force-pushed the tx-phase2 branch from 12f90c5 to 8aec70a Compare November 2, 2020 21:08

ariard reviewed Nov 4, 2020

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

devrandom force-pushed the tx-phase2 branch 2 times, most recently from 8bed28b to 2c2ad88 Compare November 5, 2020 13:48

devrandom commented Nov 5, 2020

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

devrandom force-pushed the tx-phase2 branch from 2c2ad88 to 4e1b980 Compare November 6, 2020 14:24

devrandom force-pushed the tx-phase2 branch from 4e1b980 to 78c1b4a Compare November 9, 2020 11:06

ariard reviewed Nov 10, 2020

View reviewed changes

lightning/src/ln/chan_utils.rs Show resolved Hide resolved

lightning/src/ln/chan_utils.rs Show resolved Hide resolved

lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

ariard reviewed Nov 10, 2020

View reviewed changes

devrandom force-pushed the tx-phase2 branch from 4f15846 to 7d4f3d5 Compare November 10, 2020 16:17

TheBlueMatt reviewed Nov 10, 2020

View reviewed changes

ariard added the High Review Priority label Nov 16, 2020

devrandom force-pushed the tx-phase2 branch 2 times, most recently from 5c91c1b to d09ea33 Compare December 18, 2020 18:29

Unwrap unneeded macros

a294a3f

devrandom force-pushed the tx-phase2 branch 5 times, most recently from bc0fba0 to 49a2fc6 Compare December 20, 2020 01:29

jkczyz reviewed Dec 22, 2020

View reviewed changes

lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved

lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved

devrandom force-pushed the tx-phase2 branch from 49a2fc6 to 8b6dbe6 Compare December 22, 2020 06:11

devrandom mentioned this pull request Dec 27, 2020

Draft: phase-2 signer API #723

Closed

TheBlueMatt reviewed Dec 28, 2020

View reviewed changes

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

ariard reviewed Dec 29, 2020

View reviewed changes

lightning/src/ln/chan_utils.rs Show resolved Hide resolved

devrandom force-pushed the tx-phase2 branch from 4b6cc0b to 834ede5 Compare December 29, 2020 19:12

TheBlueMatt reviewed Dec 30, 2020

View reviewed changes

devrandom force-pushed the tx-phase2 branch 3 times, most recently from f39873d to 4ef6a3f Compare December 30, 2020 18:20

TheBlueMatt reviewed Dec 30, 2020

View reviewed changes

lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved

devrandom force-pushed the tx-phase2 branch from 4ef6a3f to 2e4e9a5 Compare December 30, 2020 18:27

TheBlueMatt approved these changes Dec 30, 2020

View reviewed changes

devrandom added 2 commits December 30, 2020 13:40

Use new get_commitment_transaction_number_obscure_factor utility func…

9291a38

…tion

devrandom force-pushed the tx-phase2 branch from b8544dd to 9291a38 Compare December 30, 2020 21:40

ariard reviewed Jan 4, 2021

View reviewed changes

TheBlueMatt merged commit 9c9c881 into lightningdevkit:main Jan 4, 2021

devrandom mentioned this pull request Jan 5, 2021

Fold sign_holder_commitment_htlc_transactions into sign_holder_commitment #770

Merged

Introduce CommitmentTransactionInfo #742

Introduce CommitmentTransactionInfo #742

Conversation

devrandom commented Oct 17, 2020 • edited Loading

codecov bot commented Oct 17, 2020 • edited Loading

Codecov Report

devrandom commented Oct 18, 2020

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

devrandom commented Nov 6, 2020

ariard left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devrandom Nov 11, 2020 • edited Loading

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

devrandom commented Dec 29, 2020

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devrandom Dec 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Dec 30, 2020 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt commented Dec 30, 2020

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jan 4, 2021

devrandom commented Oct 17, 2020 •

edited

Loading

codecov bot commented Oct 17, 2020 •

edited

Loading

devrandom Nov 11, 2020 •

edited

Loading

devrandom Dec 30, 2020 •

edited

Loading

TheBlueMatt Dec 30, 2020 •

edited

Loading