Remove signing from Channel #420

TheBlueMatt · 2019-12-13T21:28:04Z

Based on #419, this takes a few big leaps forward, removing all actual signing from Channel, and dumping it all on ChannelMonitor.

codecov · 2019-12-13T21:42:25Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f755ae5). Click here to learn what that means.
The diff coverage is 95.68%.

@@           Coverage Diff            @@
##             master    #420   +/-   ##
========================================
  Coverage          ?   87.7%           
========================================
  Files             ?      30           
  Lines             ?   16794           
  Branches          ?       0           
========================================
  Hits              ?   14730           
  Misses            ?    2064           
  Partials          ?       0

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	`80.56% <100%> (ø)`
lightning/src/ln/channel.rs	`84.76% <100%> (ø)`
lightning/src/ln/chan_utils.rs	`95.08% <89.69%> (ø)`
lightning/src/ln/channelmonitor.rs	`85.58% <96.8%> (ø)`
lightning/src/ln/functional_tests.rs	`96.07% <98.98%> (ø)`

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 f755ae5...e2e1628. Read the comment docs.

ariard · 2019-12-19T18:03:48Z

lightning/src/chain/keysinterface.rs

@@ -178,9 +184,9 @@ impl ChannelKeys for InMemoryChannelKeys {
 	fn htlc_base_key(&self) -> &SecretKey { &self.htlc_base_key }
 	fn commitment_seed(&self) -> &[u8; 32] { &self.commitment_seed }

-	fn sign_remote_commitment<T: secp256k1::Signing>(&self, channel_value_satoshis: u64, channel_funding_script: &Script, feerate_per_kw: u64, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], to_self_delay: u16, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()> {
+	fn sign_remote_commitment<T: secp256k1::Signing>(&self, channel_value_satoshis: u64, channel_funding_redeemscript: &Script, feerate_per_kw: u64, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], to_self_delay: u16, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()> {


Implemented with parameter as feerate_per_kw instead of asbolute_fee. I think we also conclude verbally than fee passed by caller can't be trusted by signer and should be recomputed locally.

Hmm, no? Indeed, the parameter name in the trait is wrong, its per_kw, not absolute, but we can't not provide it - its required to generate the commitment transaction itself. Indeed, we can't trust it, and either our peer or our own fee estimator can make us attach an absurd fee and spend all our money on fee, but until the protocol doesn't have an update_fee message, we can't do anything about this.

Ah you're right, thought it was there for transaction digest algo but in fact that's for second-stage htlc transactions fees, so yes right now we have to trust it.

ariard

Excited by this move! Some comments and checks can be a bit improved I think

lightning/src/ln/chan_utils.rs

ariard · 2019-12-24T15:22:55Z

lightning/src/ln/channelmonitor.rs

@@ -2232,10 +2229,21 @@ impl ChannelMonitor {
 		// HTLCs set may differ between last and previous local commitment txn, in case of one them hitting chain, ensure we cancel all HTLCs backward
 		let mut is_local_tx = false;

+		if let &mut Some(ref mut local_tx) = &mut self.current_local_signed_commitment_tx {


85b1a9e

Why sign current and prev local commitment txn in check_spend_local_transaction ? One of them has been already signed and broadcast onchain.

We dont sign both - both sign calls are in a if local_tx.txid == commitment_txid check. We probably dont need to sign them at all, but there's a number of such cases in channelmonitor that we can discuss cleaning up later, so this is more in line with existing behavior.

Yes I understand we sign either one of them, it was more about the new code branches in themselves. Okay let's talk about them in channelmonitor cleanup (even if it's likely we remove them long-term)

lightning/src/ln/chan_utils.rs

ariard · 2019-12-24T15:35:29Z

lightning/src/ln/chan_utils.rs

+	}
+
+	pub fn new_from_unsigned(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey) -> LocalCommitmentTransaction {
+		if tx.input.len() != 1 { panic!("Tried to store a commitment transaction that had input count != 1!"); }


85b1a9e

Can we extend the number of non contextual commitment transaction format checks ? Like checking locktime and nSequence by passing the obscured commitment number or verifying then every output is either P2WSH or P2WPKH ?

I mean this is still an internal function, so these are more to prevent invalid usage of the struct that indicates we've confused ourselves over where the state machine is. Being able to do more is something we should add to EnforcingChannelKeys, but that can come over time once we move the add_local_sig to that.

ariard · 2019-12-24T15:49:07Z

lightning/src/ln/chan_utils.rs

@@ -63,7 +63,9 @@ pub(super) fn derive_public_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>,
 	base_point.combine(&hashkey)
 }

-/// Derives a revocation key from its constituent parts
+/// Derives a revocation key from its constituent parts.
+/// Note that this is infallible iff we trust that at least one of the two input keys are randomly


9aadb7f

Comment is obscure, is this a mention to the case that you were speaking about with a revocation pubkey without a private key ?

Yes. Its a key assumption in being able to not derive the private key at commitment-check-time and instead do it at broadcast time.

ariard · 2019-12-24T15:50:31Z

lightning/src/ln/chan_utils.rs

+		assert!(preimage.is_none());
+	} else {
+		tx.input[0].witness.push(preimage.unwrap().0.to_vec());
+	}


9aadb7f

You can add assert!(tx.locktime == htlc.cltv_expiry) for !htlc.offered

Hmm, this appears to fail in 15 tests. I'll leave it for adding later.

Ah yeah it's also used to build remote htlc transaction so we can't rely on offered flag. All tests passed with preimage.is_none { assert!(tx.lock_time == htlc.ctlv_expiry)

We now have current-local-tx broadcast ability in channel monitors directly (for ChannelManager deserialization), so we can just use that instead of always having the Channel store signed ready-to-go copies of the latest local commitment transaction. This is further kinda nice since ChannelMonitor is live and can, eg broadcast HTLC-Success transactions immediately as they will be generated at broadcast time instead of in advance. Finally, this lets us clean up a tiny bit in Channel.

ariard · 2019-12-28T16:41:56Z

ACK e2e1628, comments can be addressed in further refactoring.

TheBlueMatt added this to the 0.0.11 milestone Dec 13, 2019

TheBlueMatt force-pushed the 2019-12-chan-ext-signer branch from 5657cb2 to c13a6bf Compare December 13, 2019 21:42

TheBlueMatt force-pushed the 2019-12-chan-ext-signer branch from c13a6bf to 9aadb7f Compare December 13, 2019 21:53

ariard reviewed Dec 19, 2019

View reviewed changes

ariard reviewed Dec 24, 2019

View reviewed changes

TheBlueMatt added 5 commits December 24, 2019 12:13

Fix parameter name incorrectly changed in 4ebe64f

eeac6c9

Set basic channel info in chanmon all at once, add a bit more info

200e898

Make ChannelMonitor sign local transactions (at broadcast time)

eb97a75

Sign local HTLC transactions at broadcast-time, instead of generate

e2e1628

TheBlueMatt force-pushed the 2019-12-chan-ext-signer branch from 9aadb7f to e2e1628 Compare December 24, 2019 17:19

TheBlueMatt merged commit 9310813 into lightningdevkit:master Dec 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove signing from Channel #420

Remove signing from Channel #420

TheBlueMatt commented Dec 13, 2019

codecov bot commented Dec 13, 2019 •

edited

Loading

ariard Dec 19, 2019

TheBlueMatt Dec 24, 2019

ariard Dec 28, 2019

ariard left a comment

ariard Dec 24, 2019

TheBlueMatt Dec 24, 2019

ariard Dec 28, 2019

ariard Dec 24, 2019

TheBlueMatt Dec 24, 2019

ariard Dec 24, 2019

TheBlueMatt Dec 24, 2019

ariard Dec 24, 2019

TheBlueMatt Dec 24, 2019

ariard Dec 28, 2019

ariard commented Dec 28, 2019

Remove signing from Channel #420

Remove signing from Channel #420

Conversation

TheBlueMatt commented Dec 13, 2019

codecov bot commented Dec 13, 2019 • edited Loading

Codecov Report

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

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

ariard commented Dec 28, 2019

codecov bot commented Dec 13, 2019 •

edited

Loading