Change get_per_commitment_point to return result type

Includes simple changes to test util signers and tests, as well as
handling the error case for get_per_commitment_point in
HolderCommitmentPoint. This leaves a couple `.expect`s in places
that will be addressed in a separate PR for handling funding.

Author: alecchendev

lightning/src/ln/channel.rs

@@ -962,8 +962,12 @@ impl HolderCommitmentPoint {
 	{
 		HolderCommitmentPoint::Available {
 			transaction_number: INITIAL_COMMITMENT_NUMBER,
-			current: signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, secp_ctx),
-			next: signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, secp_ctx),
+			// TODO(async_signing): remove this expect with the Uninitialized variant
+			current: signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, secp_ctx)
+				.expect("Signer must be able to provide initial commitment point"),
+			// TODO(async_signing): remove this expect with the Uninitialized variant
+			next: signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, secp_ctx)
+				.expect("Signer must be able to provide second commitment point"),
 		}
 	}
 
@@ -1001,9 +1005,12 @@ impl HolderCommitmentPoint {
 		where SP::Target: SignerProvider, L::Target: Logger
 	{
 		if let HolderCommitmentPoint::PendingNext { transaction_number, current } = self {
-			let next = signer.as_ref().get_per_commitment_point(*transaction_number - 1, secp_ctx);
-			log_trace!(logger, "Retrieved next per-commitment point {}", *transaction_number - 1);
-			*self = HolderCommitmentPoint::Available { transaction_number: *transaction_number, current: *current, next };
+			if let Ok(next) = signer.as_ref().get_per_commitment_point(*transaction_number - 1, secp_ctx) {
+				log_trace!(logger, "Retrieved next per-commitment point {}", *transaction_number - 1);
+				*self = HolderCommitmentPoint::Available { transaction_number: *transaction_number, current: *current, next };
+			} else {
+				log_trace!(logger, "Next per-commitment point {} is pending", transaction_number);
+			}
 		}
 	}
 
@@ -5607,7 +5614,7 @@ impl<SP: Deref> Channel<SP> where
 
 		let our_commitment_transaction = INITIAL_COMMITMENT_NUMBER - self.context.holder_commitment_point.transaction_number() - 1;
 		if msg.next_remote_commitment_number > 0 {
-			let expected_point = self.context.holder_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1, &self.context.secp_ctx);
+			let expected_point = self.context.holder_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1, &self.context.secp_ctx).expect("TODO");
 			let given_secret = SecretKey::from_slice(&msg.your_last_per_commitment_secret)
 				.map_err(|_| ChannelError::close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?;
 			if expected_point != PublicKey::from_secret_key(&self.context.secp_ctx, &given_secret) {
@@ -9305,14 +9312,16 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
 			(Some(current), Some(next)) => HolderCommitmentPoint::Available {
 				transaction_number: cur_holder_commitment_transaction_number, current, next
 			},
-			(Some(current), _) => HolderCommitmentPoint::Available {
+			(Some(current), _) => HolderCommitmentPoint::PendingNext {
 				transaction_number: cur_holder_commitment_transaction_number, current,
-				next: holder_signer.get_per_commitment_point(cur_holder_commitment_transaction_number - 1, &secp_ctx),
 			},
-			(_, _) => HolderCommitmentPoint::Available {
-				transaction_number: cur_holder_commitment_transaction_number,
-				current: holder_signer.get_per_commitment_point(cur_holder_commitment_transaction_number, &secp_ctx),
-				next: holder_signer.get_per_commitment_point(cur_holder_commitment_transaction_number - 1, &secp_ctx),
+			(_, _) => {
+				// TODO(async_signing): remove this expect with the Uninitialized variant
+				let current = holder_signer.get_per_commitment_point(cur_holder_commitment_transaction_number, &secp_ctx)
+					.expect("Must be able to derive the current commitment point upon channel restoration");
+				HolderCommitmentPoint::PendingNext {
+					transaction_number: cur_holder_commitment_transaction_number, current,
+				}
 			},
 		};
 

lightning/src/ln/functional_tests.rs

@@ -741,7 +741,7 @@ fn test_update_fee_that_funder_cannot_afford() {
 		(pubkeys.revocation_basepoint, pubkeys.htlc_basepoint,
 		 pubkeys.funding_pubkey)
 	};
-	let (remote_delayed_payment_basepoint, remote_htlc_basepoint,remote_point, remote_funding) = {
+	let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point, remote_funding) = {
 		let per_peer_state = nodes[1].node.per_peer_state.read().unwrap();
 		let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap();
 		let remote_chan = chan_lock.channel_by_id.get(&chan.2).map(
@@ -750,7 +750,7 @@ fn test_update_fee_that_funder_cannot_afford() {
 		let chan_signer = remote_chan.get_signer();
 		let pubkeys = chan_signer.as_ref().pubkeys();
 		(pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint,
-		 chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx),
+		 chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap(),
 		 pubkeys.funding_pubkey)
 	};
 
@@ -1475,7 +1475,7 @@ fn test_fee_spike_violation_fails_htlc() {
 		let pubkeys = chan_signer.as_ref().pubkeys();
 		(pubkeys.revocation_basepoint, pubkeys.htlc_basepoint,
 		 chan_signer.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER),
-		 chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx),
+		 chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx).unwrap(),
 		 chan_signer.as_ref().pubkeys().funding_pubkey)
 	};
 	let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point, remote_funding) = {
@@ -1487,7 +1487,7 @@ fn test_fee_spike_violation_fails_htlc() {
 		let chan_signer = remote_chan.get_signer();
 		let pubkeys = chan_signer.as_ref().pubkeys();
 		(pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint,
-		 chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx),
+		 chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap(),
 		 chan_signer.as_ref().pubkeys().funding_pubkey)
 	};
 

lightning/src/sign/mod.rs

@@ -729,8 +729,12 @@ pub trait ChannelSigner {
 	/// Gets the per-commitment point for a specific commitment number
 	///
 	/// Note that the commitment number starts at `(1 << 48) - 1` and counts backwards.
-	fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1<secp256k1::All>)
-		-> PublicKey;
+	///
+	/// If the signer returns `Err`, then the user is responsible for either force-closing the channel
+	/// or retrying once the signature is ready.
+	fn get_per_commitment_point(
+		&self, idx: u64, secp_ctx: &Secp256k1<secp256k1::All>,
+	) -> Result<PublicKey, ()>;
 
 	/// Gets the commitment secret for a specific commitment number as part of the revocation process
 	///
@@ -1343,11 +1347,11 @@ impl EntropySource for InMemorySigner {
 impl ChannelSigner for InMemorySigner {
 	fn get_per_commitment_point(
 		&self, idx: u64, secp_ctx: &Secp256k1<secp256k1::All>,
-	) -> PublicKey {
+	) -> Result<PublicKey, ()> {
 		let commitment_secret =
 			SecretKey::from_slice(&chan_utils::build_commitment_secret(&self.commitment_seed, idx))
 				.unwrap();
-		PublicKey::from_secret_key(secp_ctx, &commitment_secret)
+		Ok(PublicKey::from_secret_key(secp_ctx, &commitment_secret))
 	}
 
 	fn release_commitment_secret(&self, idx: u64) -> [u8; 32] {

lightning/src/util/test_channel_signer.rs

@@ -166,7 +166,9 @@ impl TestChannelSigner {
 }
 
 impl ChannelSigner for TestChannelSigner {
-	fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1<secp256k1::All>) -> PublicKey {
+	fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<PublicKey, ()> {
+		// TODO: implement a mask in EnforcementState to let you test signatures being
+		// unavailable
 		self.inner.get_per_commitment_point(idx, secp_ctx)
 	}