Enforce signing counterparty commitment only after revocation

Author: devrandom

lightning/src/chain/keysinterface.rs

@@ -229,6 +229,11 @@ pub trait BaseSign {
 	//
 	// TODO: Document the things someone using this interface should enforce before signing.
 	fn sign_counterparty_commitment(&self, commitment_tx: &CommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()>;
+	/// Validate the counterparty's revocation.
+	///
+	/// This is required in order for the signer to make sure that the state has moved
+	/// forward and it is safe to sign the next counterparty commitment.
+	fn validate_counterparty_revocation(&self, idx: u64, secret: &SecretKey);
 
 	/// Create a signatures for a holder's commitment transaction and its claiming HTLC transactions.
 	/// This will only ever be called with a non-revoked commitment_tx.  This will be called with the
@@ -592,6 +597,9 @@ impl BaseSign for InMemorySigner {
 		Ok((commitment_sig, htlc_sigs))
 	}
 
+	fn validate_counterparty_revocation(&self, _idx: u64, _secret: &SecretKey) {
+	}
+
 	fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()> {
 		let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
 		let funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &self.counterparty_pubkeys().funding_pubkey);

lightning/src/ln/channel.rs

@@ -2615,6 +2615,12 @@ impl<Signer: Sign> Channel<Signer> {
 			*self.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = None;
 		}
 
+		let secret = SecretKey::from_slice(&msg.per_commitment_secret)
+			.map_err(|_| ChannelError::Close("Malformed commitment secret".to_owned()))?;
+		self.holder_signer.validate_counterparty_revocation(
+			self.cur_counterparty_commitment_transaction_number + 1,
+			&secret
+		);
 		self.commitment_secrets.provide_secret(self.cur_counterparty_commitment_transaction_number + 1, msg.per_commitment_secret)
 			.map_err(|_| ChannelError::Close("Previous secrets did not match new one".to_owned()))?;
 		self.latest_monitor_update_id += 1;

lightning/src/util/enforcing_trait_impls.rs

@@ -117,12 +117,21 @@ impl BaseSign for EnforcingSigner {
 			// These commitment numbers are backwards counting.  We expect either the same as the previously encountered,
 			// or the next one.
 			assert!(last_commitment_number == actual_commitment_number || last_commitment_number - 1 == actual_commitment_number, "{} doesn't come after {}", actual_commitment_number, last_commitment_number);
+			// Ensure that the counterparty doesn't get more than two broadcastable commitments -
+			// the last and the one we are trying to sign
+			assert!(actual_commitment_number >= state.last_counterparty_revoked_commitment - 2, "cannot sign a commitment if second to last wasn't revoked - signing {} revoked {}", actual_commitment_number, state.last_counterparty_revoked_commitment);
 			state.last_counterparty_commitment = Some(cmp::min(last_commitment_number, actual_commitment_number))
 		}
 
 		Ok(self.inner.sign_counterparty_commitment(commitment_tx, secp_ctx).unwrap())
 	}
 
+	fn validate_counterparty_revocation(&self, idx: u64, _secret: &SecretKey) {
+		let mut state = self.state.lock().unwrap();
+		assert!(idx == state.last_counterparty_revoked_commitment || idx == state.last_counterparty_revoked_commitment - 1, "expecting to validate the current or next counterparty revocation - trying {}, current {}", idx, state.last_counterparty_revoked_commitment);
+		state.last_counterparty_revoked_commitment = idx;
+	}
+
 	fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()> {
 		let trusted_tx = self.verify_holder_commitment_tx(commitment_tx, secp_ctx);
 		let commitment_txid = trusted_tx.txid();
@@ -225,6 +234,8 @@ impl EnforcingSigner {
 pub struct EnforcementState {
 	/// The last counterparty commitment number we signed, backwards counting
 	pub last_counterparty_commitment: Option<u64>,
+	/// The last counterparty commitment they revoked, backwards counting
+	pub last_counterparty_revoked_commitment: u64,
 	/// The last holder commitment number we revoked, backwards counting
 	pub last_holder_revoked_commitment: u64,
 	/// The last validated holder commitment number, backwards counting
@@ -236,6 +247,7 @@ impl EnforcementState {
 	pub fn new() -> Self {
 		EnforcementState {
 			last_counterparty_commitment: None,
+			last_counterparty_revoked_commitment: INITIAL_REVOKED_COMMITMENT_NUMBER,
 			last_holder_revoked_commitment: INITIAL_REVOKED_COMMITMENT_NUMBER,
 			last_holder_commitment: INITIAL_REVOKED_COMMITMENT_NUMBER,
 		}