Clean up ChannelKeys API #598

TheBlueMatt · 2020-04-20T05:10:15Z

Based on #597, this cleans up the ChannelKeys API a bunch, closing #567. I'm not convinced the internal interfaces are quite where they need to be, but its a bit better than it was, and we can improve it as we move all signing external.

ariard

Reviewed until 2c09663

ariard · 2020-04-23T06:16:28Z

lightning/src/ln/channelmonitor.rs

-	// Used to detect programming bug due to unsafe monitor update sequence { ChannelForceClosed, LatestLocalCommitmentTXInfo }
+	// This is set when the Channel[Manager] generated a ChannelMonitorUpdate which indicated the
+	// channel has been force-closed. After this is set, no further local commitment transaction
+	// updates may occur, and we panic!() if one is provided.


7e11701

Is this making an assumption we receive ordered ChannelMonitorUpdateStep ? Like what if ChannelManager sends LatestLocalCommitmentTXInfo,ChannelForceClosed but we receive ChannelForceClosed,LatestLocalCommitmentTXInfo ? It should be mostly for all Internet deployment but as we are also targeting support of non-internet stack, for people who want to connect through some poorly-designed hardware link layer, we should care.

Let's avoid to burn our brains on this right now, but stay aware of this.

We always make the assumption that we receive the steps in order. The ChannelMonitorUpdates have serial numbers and the documentation clearly says they have to be in-order, and the ChannelMonitorUpdateSteps are private.

Oh right, it's in ChannelMonitorUpdate

ariard · 2020-04-23T06:39:17Z

lightning/src/chain/keysinterface.rs

-	/// Create a signature for a local commitment transaction
+	/// Create a signature for a local commitment transaction. This will only ever be called with
+	/// the same local_commitment_tx (or a copy thereof), though there are currently no guarantees
+	/// that it will not be called multiple times.


I agree, I think that's good to say you may want to resign multiples times, even the same commitment transaction, like if you do a CPFP, you may want to verify every time the transaction chain back to the funding_outpoint, and therefore redoing the whole signing process.

ariard · 2020-04-23T06:48:22Z

lightning/src/ln/chan_utils.rs

 	///
 	/// Funding key is your key included in the 2-2 funding_outpoint lock. Should be provided
 	/// by your ChannelKeys.
 	/// Funding redeemscript is script locking funding_outpoint. This is the mutlsig script
 	/// between your own funding key and your counterparty's. Currently, this is provided in
 	/// ChannelKeys::sign_local_commitment() calls directly.
 	/// Channel value is amount locked in funding_outpoint.
-	pub fn add_local_sig<T: secp256k1::Signing>(&mut self, funding_key: &SecretKey, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
-		if self.has_local_sig() { return; }
+	pub fn get_local_sig<T: secp256k1::Signing>(&self, funding_key: &SecretKey, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) -> Signature {


2c09663

At this commit, can be pub(crate)

This is used by any implementer of ChannelKeys, so I don't think so? I mean we should probably drop it wholesale because just writing the two lines in any implementer of ChannelKeys is better, but...

lightning/src/ln/chan_utils.rs

lightning/src/ln/channelmonitor.rs

ariard · 2020-04-23T07:01:41Z

lightning/src/ln/channelmonitor.rs

@@ -422,6 +422,7 @@ pub(crate) enum InputMaterial {
 	},
 	Funding {
 		channel_value: u64,
+		funding_redeemscript: Script,
 	}


2c09663

We do store local_channel_pubkeys+remote_channel_pubkeys in InMemoryChannelKeys, can't you derive script there instead ?

See-also #606.

Hmm, we don't expose the remote_channel_pubkeys in ChannelKeys, nor do I really think we should. We could maybe move to caching that in the ChannelMonitor and passing it in here, but for now might as well just leave it as caching the script.

I pretty agree with other lines in #606, for now I think we can cache the script. I expect signer to verify remote keys, so why caching in ChannelMonitor?

Ehh, I was just thinking try to be less prescriptive on the signer, who knows the signer may be purely remote/hardware/etc, so no need to be too strict about what it provides.

ariard

My only concern by batch-siginng HTLC is to make it harder to use SIGHASH_SINGLE for them in the future.

ariard · 2020-04-24T05:50:41Z

lightning/src/ln/chan_utils.rs

+			if this_htlc.0.transaction_output_index.is_some() {
+				let htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw, local_csv, &this_htlc.0, &self.local_keys.a_delayed_payment_key, &self.local_keys.revocation_key);
+				assert_eq!(htlc_tx.input.len(), 1);
+				assert_eq!(htlc_tx.input[0].witness.len(), 0);


You can assert nSequence==0 and len(Output)=1

I mean I just left these in cause they were already there, hopefully we can scroll up 20 lines and read build_htlc_transaction to see that that is true :). Instead I'm gonna add a commit to drop these.

ariard · 2020-04-24T05:56:36Z

lightning/src/ln/chan_utils.rs

+
+		let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key);
+
+		htlc_tx.input[0].witness.push(Vec::new()); // First is the multisig dummy


Can you add a reference to BIP147, underscores that's a MUST here and we should assert at the end than NULLDUMMY is still one

I don't think we need to assert that something is trivially true. But I added a comment noting the requirement, thanks!

lightning/src/ln/chan_utils.rs

ariard · 2020-04-24T06:19:41Z

lightning/src/ln/onchaintx.rs

@@ -771,11 +817,47 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
 		if let Some(ref local_commitment) = self.local_commitment {
 			if local_commitment.has_local_sig() { return Err(()) }
 		}
+		if self.local_htlc_sigs.is_some() || self.prev_local_htlc_sigs.is_some() {


You may extend comment above, if we do have htlc_sigs, that's an hint we have seen a local onchain, even if it's coming from us, we should deny access to commitment (it's say elsewhere also)

Isn't that what it implies with "local comitment transaction has been signed"? I'm a bit confused by your comment here.

Nevermind, I think it was to make the distinction between a "local commitment has been signed" and a "local commitment has been by-this-monitor". But if you see a local commitment onchain, even broadcast from another monitor, the "local" copy should have been signed & broadcast because trigger condition should be met too (aka block height).

We should care about "local" (not remote commitment) and "local"(aka on this watchtower).

ariard · 2020-04-24T06:32:05Z

lightning/src/ln/onchaintx.rs

 			}
 		}
-		None
+		if self.prev_local_commitment.is_some() {


Hmmm is this correct ? If both local_commitment and prev_local_commitment are present, you're going to return a HTLC-transaction for a previous local commtiment while you may have onchain latest local commitment and so return an unspendable transaction ?

In both cases we don't do anything uness the outpoint points to the expected txid, so it should be fine?

Right we check by txid, everything is safe!

lightning/src/ln/chan_utils.rs

TheBlueMatt · 2020-04-24T19:28:48Z

My only concern by batch-siginng HTLC is to make it harder to use SIGHASH_SINGLE for them in the future.s

Hmm, I hope not, the function could just return a Vec of Transactions or so instead. The API is deliberately somewhat restrictive in that you definitely only ever actually want to sign all the HTLCs at once, so the API matches the safety requirement.

As channel_value last usage was for computing feerate but as this one is static per-commitment and will always-be following specification, we remove it.

In e46e183 we began tracking whether a local commitment transaction had been signed and broadcast in OnchainTxHandler, refusing to update the local commitment transaction state in the ChannelMonitor on that basis. This is fine, except that it doesn't make a lot of sense to store the full local transaction state in OnchainTxHandler - we should be providing it the unsigned local transaction at the time we wish to broadcast and no more (just like we do all other transaction data).

This cleans up sign_local_commitment somewhat by returning a Result<Signaure, ()> over the local commitment transaction instead of modifying the struct which was passed in. This is the first step in making LocalCommitmentTransaction a completely pub struct, using it just to communicate enough information to the user to allow them to construct a signaure instead of having it contain a bunch of logic. This should make it much easier to implement a custom ChannelKeys by disconnecting the local commitment transaction signing from our own datastructures.

1107ab0 introduced an API to have a ChannelKeys implementer sign HTLC transactions by calling into the LocalCommitmentTransaction object, which would then store the tx. This API was incredibly awkward, both because it required an external signer trust our own internal interfaces, but also because it didn't allow for any inspection of what was about to be signed. Further, it signed the HTLC transactions one-by-one in a somewhat inefficient way, and there isn't a clear way to resolve this (as the which-HTLC parameter has to refer to something in between the HTLC's arbitrary index, and its index in the commitment tx, which has "holes" for the non-HTLC outputs and skips some HTLCs). We replace it with a new function in ChannelKeys which allows us to sign all HTLCs in a given commitment transaction (which allows for a bit more effeciency on the signers' part, as well as sidesteps the which-HTLC issue). This may also simplify the signer implementation as we will always want to sign all HTLCs spending a given commitment transaction at once anyway. We also de-mut the LocalCommitmentTransaction passed to the ChanKeys, instead opting to make LocalCommitmentTransaction const and avoid storing any new HTLC-related data in it.

Instead of adding signatures to LocalCommitmentTransactions, we instead leave them unsigned and use them to construct signed Transactions when we want them. This cleans up the guts of LocalCommitmentTransaction enough that we can, and do, expose its state to the world, allowing external signers to have a basic awareness of what they're signing.

We should never be exposing our own TODOs to the world.

We don't need to assert that transaction structure is what we expect when the transaction is created by a function twenty lines up in the same file.

codecov · 2020-04-25T01:34:12Z

Codecov Report

Merging #598 into master will increase coverage by 0.02%.
The diff coverage is 96.42%.

@@            Coverage Diff             @@
##           master     #598      +/-   ##
==========================================
+ Coverage   91.05%   91.07%   +0.02%     
==========================================
  Files          34       34              
  Lines       20359    20447      +88     
==========================================
+ Hits        18538    18623      +85     
- Misses       1821     1824       +3

Impacted Files	Coverage Δ
lightning/src/ln/onchaintx.rs	`94.86% <93.69%> (-0.26%)`	⬇️
lightning/src/ln/channelmonitor.rs	`95.50% <95.34%> (-0.01%)`	⬇️
lightning/src/chain/keysinterface.rs	`96.98% <100.00%> (-0.07%)`	⬇️
lightning/src/ln/chan_utils.rs	`97.19% <100.00%> (+0.44%)`	⬆️
lightning/src/ln/channel.rs	`86.41% <100.00%> (-0.01%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`97.02% <0.00%> (-0.02%)`	⬇️
lightning/src/util/ser_macros.rs	`97.27% <0.00%> (+0.68%)`	⬆️

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 4dc0dd1...03316cd. Read the comment docs.

ariard · 2020-04-25T05:43:46Z

ACK 03316cd

TheBlueMatt added this to the 0.0.11 milestone Apr 20, 2020

ariard reviewed Apr 23, 2020

View reviewed changes

TheBlueMatt force-pushed the 2020-04-559-cleanups branch 3 times, most recently from cc7bc39 to 6191a40 Compare April 23, 2020 21:31

ariard reviewed Apr 24, 2020

View reviewed changes

TheBlueMatt force-pushed the 2020-04-559-cleanups branch from 6191a40 to fff4b23 Compare April 24, 2020 20:03

Antoine Riard and others added 7 commits April 24, 2020 18:51

Dry-up InputMaterial::Funding

359b3d5

As channel_value last usage was for computing feerate but as this one is static per-commitment and will always-be following specification, we remove it.

Remove TODOs from documentation in keysinterface

92c0698

We should never be exposing our own TODOs to the world.

Remove trivial assertions in LocalCommitmentTransaction fns

03316cd

We don't need to assert that transaction structure is what we expect when the transaction is created by a function twenty lines up in the same file.

TheBlueMatt force-pushed the 2020-04-559-cleanups branch from fff4b23 to 03316cd Compare April 25, 2020 01:24

TheBlueMatt mentioned this pull request Apr 25, 2020

0.0.11 #608

Merged

TheBlueMatt merged commit 8fb50f2 into lightningdevkit:master Apr 25, 2020

valentinewallace mentioned this pull request May 7, 2020

Improve ChannelKeys TODOs #567

Closed


		let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key);

		htlc_tx.input[0].witness.push(Vec::new()); // First is the multisig dummy

Clean up ChannelKeys API #598

Clean up ChannelKeys API #598

Uh oh!

Conversation

TheBlueMatt commented Apr 20, 2020

Uh oh!

ariard 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!

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!

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!

ariard 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!

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!

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!

Uh oh!

TheBlueMatt commented Apr 24, 2020

Uh oh!

codecov bot commented Apr 25, 2020

Codecov Report

Uh oh!

ariard commented Apr 25, 2020

Uh oh!

Uh oh!