Enforce signing counterparty commitment only after revocation

diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs
index ac81e5430da299b7f57ecde5711717388107d3d0..d912740a019a8a6bb215081c94bd084dde380dc0 100644 (file)
--- a/lightning/src/chain/keysinterface.rs
+++ b/lightning/src/chain/keysinterface.rs
@@ -230,6 +230,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);

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 2cd7e3c2ff9733678e4ce9ecda2187308c70094e..9725650f9a122a5408b8ab78e9f93cc2d420fb0b 100644 (file)
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -2742,8 +2742,10 @@ impl<Signer: Sign> Channel<Signer> {
                        return Err(ChannelError::Close("Peer sent revoke_and_ack after we'd started exchanging closing_signeds".to_owned()));
                }
 
+               let secret = secp_check!(SecretKey::from_slice(&msg.per_commitment_secret), "Peer provided an invalid per_commitment_secret".to_owned());
+
                if let Some(counterparty_prev_commitment_point) = self.counterparty_prev_commitment_point {
-                       if PublicKey::from_secret_key(&self.secp_ctx, &secp_check!(SecretKey::from_slice(&msg.per_commitment_secret), "Peer provided an invalid per_commitment_secret".to_owned())) != counterparty_prev_commitment_point {
+                       if PublicKey::from_secret_key(&self.secp_ctx, &secret) != counterparty_prev_commitment_point {
                                return Err(ChannelError::Close("Got a revoke commitment secret which didn't correspond to their current pubkey".to_owned()));
                        }
                }
@@ -2765,6 +2767,10 @@ impl<Signer: Sign> Channel<Signer> {
                        *self.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = None;
                }
 
+               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;

diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs
index 18ed0ea834c207425dacf211b735970e7ab397c3..19408e9820daae4a9eb9af08a56eecab713cf1d0 100644 (file)
--- a/lightning/src/util/enforcing_trait_impls.rs
+++ b/lightning/src/util/enforcing_trait_impls.rs
@@ -122,12 +122,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 = 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();
@@ -230,6 +239,8 @@ impl EnforcingSigner {
 pub struct EnforcementState {
        /// The last counterparty commitment number we signed, backwards counting
        pub last_counterparty_commitment: 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
@@ -241,6 +252,7 @@ impl EnforcementState {
        pub fn new() -> Self {
                EnforcementState {
                        last_counterparty_commitment: INITIAL_REVOKED_COMMITMENT_NUMBER,
+                       last_counterparty_revoked_commitment: INITIAL_REVOKED_COMMITMENT_NUMBER,
                        last_holder_revoked_commitment: INITIAL_REVOKED_COMMITMENT_NUMBER,
                        last_holder_commitment: INITIAL_REVOKED_COMMITMENT_NUMBER,
                }