Return an error if we fail to advance our commitment number

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index ccba85c60c100e72a15c7d1e73a9c5cc5a371f94..3f7d01bb891be96e028b20bf00f6df8850b2da40 100644 (file)
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -1008,7 +1008,9 @@ impl HolderCommitmentPoint {
        }
 
        /// If we are not pending the next commitment point, this method advances the commitment number
-       /// and requests the next commitment point from the signer.
+       /// and requests the next commitment point from the signer. Returns `Ok` if we were at
+       /// `Available` and were able to advance our commitment number (even if we are still pending
+       /// the next commitment point).
        ///
        /// If our signer is not ready to provide the next commitment point, we will
        /// only advance to `PendingNext`, and should be tried again later in `signer_unblocked`
@@ -1020,7 +1022,9 @@ impl HolderCommitmentPoint {
        /// This method is used for the following transitions:
        /// - `Available` -> `PendingNext`
        /// - `Available` -> `PendingNext` -> `Available` (in one fell swoop)
-       pub fn advance<SP: Deref, L: Deref>(&mut self, signer: &ChannelSignerType<SP>, secp_ctx: &Secp256k1<secp256k1::All>, logger: &L)
+       pub fn advance<SP: Deref, L: Deref>(
+               &mut self, signer: &ChannelSignerType<SP>, secp_ctx: &Secp256k1<secp256k1::All>, logger: &L
+       ) -> Result<(), ()>
                where SP::Target: SignerProvider, L::Target: Logger
        {
                if let HolderCommitmentPoint::Available { transaction_number, next, .. } = self {
@@ -1029,7 +1033,9 @@ impl HolderCommitmentPoint {
                                current: *next,
                        };
                        self.try_resolve_pending(signer, secp_ctx, logger);
+                       return Ok(());
                }
+               Err(())
        }
 }
 
@@ -4618,7 +4624,18 @@ impl<SP: Deref> Channel<SP> where
                        channel_id: Some(self.context.channel_id()),
                };
 
-               self.context.holder_commitment_point.advance(&self.context.holder_signer, &self.context.secp_ctx, logger);
+               if self.context.holder_commitment_point.advance(&self.context.holder_signer, &self.context.secp_ctx, logger).is_err() {
+                       // We only fail to advance our commitment point/number if we're currently
+                       // waiting for our signer to unblock and provide a commitment point.
+                       // During post-funding channel operation, we only advance our point upon
+                       // receiving a commitment_signed, and our counterparty cannot send us
+                       // another commitment signed until we've provided a new commitment point
+                       // in revoke_and_ack, which requires unblocking our signer and completing
+                       // the advance to the next point. This should be unreachable since
+                       // a new commitment_signed should fail at our signature checks above.
+                       debug_assert!(false, "We should be ready to advance our commitment point by the time we receive commitment_signed");
+                       return Err(ChannelError::close("Failed to advance our commitment point".to_owned()));
+               }
                self.context.expecting_peer_commitment_signed = false;
                // Note that if we need_commitment & !AwaitingRemoteRevoke we'll call
                // build_commitment_no_status_check() next which will reset this to RAAFirst.
@@ -7799,7 +7816,14 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
                } else {
                        self.context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new());
                }
-               self.context.holder_commitment_point.advance(&self.context.holder_signer, &self.context.secp_ctx, logger);
+               if self.context.holder_commitment_point.advance(&self.context.holder_signer, &self.context.secp_ctx, logger).is_err() {
+                       // We only fail to advance our commitment point/number if we're currently
+                       // waiting for our signer to unblock and provide a commitment point.
+                       // We cannot send open_channel before this has occurred, so if we
+                       // err here by the time we receive funding_signed, something has gone wrong.
+                       debug_assert!(false, "We should be ready to advance our commitment point by the time we receive funding_signed");
+                       return Err((self, ChannelError::close("Failed to advance holder commitment point".to_owned())));
+               }
                self.context.cur_counterparty_commitment_transaction_number -= 1;
 
                log_info!(logger, "Received funding_signed from peer for channel {}", &self.context.channel_id());
@@ -8066,7 +8090,14 @@ impl<SP: Deref> InboundV1Channel<SP> where SP::Target: SignerProvider {
                self.context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new());
                self.context.channel_id = ChannelId::v1_from_funding_outpoint(funding_txo);
                self.context.cur_counterparty_commitment_transaction_number -= 1;
-               self.context.holder_commitment_point.advance(&self.context.holder_signer, &self.context.secp_ctx, logger);
+               if self.context.holder_commitment_point.advance(&self.context.holder_signer, &self.context.secp_ctx, logger).is_err() {
+                       // We only fail to advance our commitment point/number if we're currently
+                       // waiting for our signer to unblock and provide a commitment point.
+                       // We cannot send accept_channel before this has occurred, so if we
+                       // err here by the time we receive funding_created, something has gone wrong.
+                       debug_assert!(false, "We should be ready to advance our commitment point by the time we receive funding_created");
+                       return Err((self, ChannelError::close("Failed to advance holder commitment point".to_owned())));
+               }
 
                let (counterparty_initial_commitment_tx, funding_signed) = self.context.get_funding_signed_msg(logger);