Merge pull request #3486 from TheBlueMatt/2024-12-async-sign

Remove the async_signing cfg flag

Author: TheBlueMatt

Cargo.toml

@@ -62,7 +62,6 @@ check-cfg = [
     "cfg(ldk_bench)",
     "cfg(ldk_test_vectors)",
     "cfg(taproot)",
-    "cfg(async_signing)",
     "cfg(require_route_graph_test)",
     "cfg(splicing)",
     "cfg(async_payments)",

ci/ci-tests.sh

@@ -166,8 +166,6 @@ fi
 echo -e "\n\nTest cfg-flag builds"
 RUSTFLAGS="--cfg=taproot" cargo test --verbose --color always -p lightning
 [ "$CI_MINIMIZE_DISK_USAGE" != "" ] && cargo clean
-RUSTFLAGS="--cfg=async_signing" cargo test --verbose --color always -p lightning
-[ "$CI_MINIMIZE_DISK_USAGE" != "" ] && cargo clean
 RUSTFLAGS="--cfg=splicing" cargo test --verbose --color always -p lightning
 [ "$CI_MINIMIZE_DISK_USAGE" != "" ] && cargo clean
 RUSTFLAGS="--cfg=async_payments" cargo test --verbose --color always -p lightning

lightning/src/ln/channel.rs

@@ -906,7 +906,6 @@ pub(super) struct MonitorRestoreUpdates {
 }
 
 /// The return value of `signer_maybe_unblocked`
-#[allow(unused)]
 pub(super) struct SignerResumeUpdates {
 	pub commitment_update: Option<msgs::CommitmentUpdate>,
 	pub revoke_and_ack: Option<msgs::RevokeAndACK>,
@@ -3960,13 +3959,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 			log_trace!(logger, "Counterparty commitment signature available for funding_signed message; clearing signer_pending_funding");
 			self.signer_pending_funding = false;
 		} else if signature.is_none() {
-			#[cfg(not(async_signing))] {
-				panic!("Failed to get signature for funding_signed");
-			}
-			#[cfg(async_signing)] {
-				log_trace!(logger, "Counterparty commitment signature not available for funding_signed message; setting signer_pending_funding");
-				self.signer_pending_funding = true;
-			}
+			log_trace!(logger, "Counterparty commitment signature not available for funding_signed message; setting signer_pending_funding");
+			self.signer_pending_funding = true;
 		}
 
 		signature.map(|(signature, _)| msgs::FundingSigned {
@@ -6117,7 +6111,6 @@ impl<SP: Deref> Channel<SP> where
 
 	/// Indicates that the signer may have some signatures for us, so we should retry if we're
 	/// blocked.
-	#[cfg(async_signing)]
 	pub fn signer_maybe_unblocked<L: Deref>(&mut self, logger: &L) -> SignerResumeUpdates where L::Target: Logger {
 		if !self.holder_commitment_point.is_available() {
 			log_trace!(logger, "Attempting to update holder per-commitment point...");
@@ -6234,21 +6227,16 @@ impl<SP: Deref> Channel<SP> where
 				&self.context.channel_id(), self.holder_commitment_point.transaction_number(),
 				self.holder_commitment_point.transaction_number() + 2);
 		}
-		#[cfg(not(async_signing))] {
-			panic!("Holder commitment point and per commitment secret must be available when generating revoke_and_ack");
-		}
-		#[cfg(async_signing)] {
-			// Technically if we're at HolderCommitmentPoint::PendingNext,
-			// we have a commitment point ready to send in an RAA, however we
-			// choose to wait since if we send RAA now, we could get another
-			// CS before we have any commitment point available. Blocking our
-			// RAA here is a convenient way to make sure that post-funding
-			// we're only ever waiting on one commitment point at a time.
-			log_trace!(logger, "Last revoke-and-ack pending in channel {} for sequence {} because the next per-commitment point is not available",
-				&self.context.channel_id(), self.holder_commitment_point.transaction_number());
-			self.context.signer_pending_revoke_and_ack = true;
-			None
-		}
+		// Technically if we're at HolderCommitmentPoint::PendingNext,
+		// we have a commitment point ready to send in an RAA, however we
+		// choose to wait since if we send RAA now, we could get another
+		// CS before we have any commitment point available. Blocking our
+		// RAA here is a convenient way to make sure that post-funding
+		// we're only ever waiting on one commitment point at a time.
+		log_trace!(logger, "Last revoke-and-ack pending in channel {} for sequence {} because the next per-commitment point is not available",
+			&self.context.channel_id(), self.holder_commitment_point.transaction_number());
+		self.context.signer_pending_revoke_and_ack = true;
+		None
 	}
 
 	/// Gets the last commitment update for immediate sending to our peer.
@@ -6319,16 +6307,11 @@ impl<SP: Deref> Channel<SP> where
 			}
 			update
 		} else {
-			#[cfg(not(async_signing))] {
-				panic!("Failed to get signature for new commitment state");
-			}
-			#[cfg(async_signing)] {
-				if !self.context.signer_pending_commitment_update {
-					log_trace!(logger, "Commitment update awaiting signer: setting signer_pending_commitment_update");
-					self.context.signer_pending_commitment_update = true;
-				}
-				return Err(());
+			if !self.context.signer_pending_commitment_update {
+				log_trace!(logger, "Commitment update awaiting signer: setting signer_pending_commitment_update");
+				self.context.signer_pending_commitment_update = true;
 			}
+			return Err(());
 		};
 		Ok(msgs::CommitmentUpdate {
 			update_add_htlcs, update_fulfill_htlcs, update_fail_htlcs, update_fail_malformed_htlcs, update_fee,
@@ -8366,13 +8349,8 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
 			log_trace!(logger, "Counterparty commitment signature ready for funding_created message: clearing signer_pending_funding");
 			self.context.signer_pending_funding = false;
 		} else if signature.is_none() {
-			#[cfg(not(async_signing))] {
-				panic!("Failed to get signature for new funding creation");
-			}
-			#[cfg(async_signing)] {
-				log_trace!(logger, "funding_created awaiting signer; setting signer_pending_funding");
-				self.context.signer_pending_funding = true;
-			}
+			log_trace!(logger, "funding_created awaiting signer; setting signer_pending_funding");
+			self.context.signer_pending_funding = true;
 		};
 
 		signature.map(|signature| msgs::FundingCreated {
@@ -8471,14 +8449,9 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
 				holder_commitment_point.current_point()
 			},
 			_ => {
-				#[cfg(not(async_signing))] {
-					panic!("Failed getting commitment point for open_channel message");
-				}
-				#[cfg(async_signing)] {
-					log_trace!(_logger, "Unable to generate open_channel message, waiting for commitment point");
-					self.signer_pending_open_channel = true;
-					return None;
-				}
+				log_trace!(_logger, "Unable to generate open_channel message, waiting for commitment point");
+				self.signer_pending_open_channel = true;
+				return None;
 			}
 		};
 		let keys = self.context.get_holder_pubkeys();
@@ -8566,7 +8539,6 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
 
 	/// Indicates that the signer may have some signatures for us, so we should retry if we're
 	/// blocked.
-	#[cfg(async_signing)]
 	pub fn signer_maybe_unblocked<L: Deref>(
 		&mut self, chain_hash: ChainHash, logger: &L
 	) -> (Option<msgs::OpenChannel>, Option<msgs::FundingCreated>) where L::Target: Logger {
@@ -8727,14 +8699,9 @@ impl<SP: Deref> InboundV1Channel<SP> where SP::Target: SignerProvider {
 				holder_commitment_point.current_point()
 			},
 			_ => {
-				#[cfg(not(async_signing))] {
-					panic!("Failed getting commitment point for accept_channel message");
-				}
-				#[cfg(async_signing)] {
-					log_trace!(_logger, "Unable to generate accept_channel message, waiting for commitment point");
-					self.signer_pending_accept_channel = true;
-					return None;
-				}
+				log_trace!(_logger, "Unable to generate accept_channel message, waiting for commitment point");
+				self.signer_pending_accept_channel = true;
+				return None;
 			}
 		};
 		let keys = self.context.get_holder_pubkeys();
@@ -8837,7 +8804,6 @@ impl<SP: Deref> InboundV1Channel<SP> where SP::Target: SignerProvider {
 
 	/// Indicates that the signer may have some signatures for us, so we should retry if we're
 	/// blocked.
-	#[allow(unused)]
 	pub fn signer_maybe_unblocked<L: Deref>(
 		&mut self, logger: &L
 	) -> Option<msgs::AcceptChannel> where L::Target: Logger {

lightning/src/ln/channelmanager.rs

@@ -9470,7 +9470,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 	/// attempted in every channel, or in the specifically provided channel.
 	///
 	/// [`ChannelSigner`]: crate::sign::ChannelSigner
-	#[cfg(async_signing)]
 	pub fn signer_unblocked(&self, channel_opt: Option<(PublicKey, ChannelId)>) {
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
 

lightning/src/ln/mod.rs

@@ -85,7 +85,7 @@ mod monitor_tests;
 #[cfg(test)]
 #[allow(unused_mut)]
 mod shutdown_tests;
-#[cfg(all(test, async_signing))]
+#[cfg(test)]
 #[allow(unused_mut)]
 mod async_signer_tests;
 #[cfg(test)]

lightning/src/sign/ecdsa.rs

@@ -24,11 +24,18 @@ use crate::sign::{ChannelSigner, HTLCDescriptor};
 /// policies in order to be secure. Please refer to the [VLS Policy
 /// Controls](https://gitlab.com/lightning-signer/validating-lightning-signer/-/blob/main/docs/policy-controls.md)
 /// for an example of such policies.
+///
+/// Like [`ChannelSigner`], many of the methods allow errors to be returned to support async
+/// signing. In such cases, the signing operation can be replayed by calling
+/// [`ChannelManager::signer_unblocked`] or [`ChainMonitor::signer_unblocked`] (see individual
+/// method documentation for which method should be called) once the result is ready, at which
+/// point the channel operation will resume.
+///
+/// [`ChannelManager::signer_unblocked`]: crate::ln::channelmanager::ChannelManager::signer_unblocked
+/// [`ChainMonitor::signer_unblocked`]: crate::chain::chainmonitor::ChainMonitor::signer_unblocked
 pub trait EcdsaChannelSigner: ChannelSigner {
 	/// Create a signature for a counterparty's commitment transaction and associated HTLC transactions.
 	///
-	/// Note that if signing fails or is rejected, the channel will be force-closed.
-	///
 	/// Policy checks should be implemented in this function, including checking the amount
 	/// sent to us and checking the HTLCs.
 	///
@@ -39,8 +46,12 @@ pub trait EcdsaChannelSigner: ChannelSigner {
 	///
 	/// Note that all the relevant preimages will be provided, but there may also be additional
 	/// irrelevant or duplicate preimages.
-	//
-	// TODO: Document the things someone using this interface should enforce before signing.
+	///
+	/// An `Err` can be returned to signal that the signer is unavailable/cannot produce a valid
+	/// signature and should be retried later. Once the signer is ready to provide a signature after
+	/// previously returning an `Err`, [`ChannelManager::signer_unblocked`] must be called.
+	///
+	/// [`ChannelManager::signer_unblocked`]: crate::ln::channelmanager::ChannelManager::signer_unblocked
 	fn sign_counterparty_commitment(
 		&self, commitment_tx: &CommitmentTransaction, inbound_htlc_preimages: Vec<PaymentPreimage>,
 		outbound_htlc_preimages: Vec<PaymentPreimage>, secp_ctx: &Secp256k1<secp256k1::All>,
@@ -58,18 +69,19 @@ pub trait EcdsaChannelSigner: ChannelSigner {
 	/// An `Err` can be returned to signal that the signer is unavailable/cannot produce a valid
 	/// signature and should be retried later. Once the signer is ready to provide a signature after
 	/// previously returning an `Err`, [`ChannelMonitor::signer_unblocked`] must be called on its
-	/// monitor.
+	/// monitor or [`ChainMonitor::signer_unblocked`] called to attempt unblocking all monitors.
 	///
 	/// [`ChannelMonitor::signer_unblocked`]: crate::chain::channelmonitor::ChannelMonitor::signer_unblocked
-	//
-	// TODO: Document the things someone using this interface should enforce before signing.
+	/// [`ChainMonitor::signer_unblocked`]: crate::chain::chainmonitor::ChainMonitor::signer_unblocked
 	fn sign_holder_commitment(
 		&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>,
 	) -> Result<Signature, ()>;
 	/// Same as [`sign_holder_commitment`], but exists only for tests to get access to holder
 	/// commitment transactions which will be broadcasted later, after the channel has moved on to a
 	/// newer state. Thus, needs its own method as [`sign_holder_commitment`] may enforce that we
 	/// only ever get called once.
+	///
+	/// This method is *not* async as it is intended only for testing purposes.
 	#[cfg(any(test, feature = "unsafe_revoked_tx_signing"))]
 	fn unsafe_sign_holder_commitment(
 		&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>,
@@ -92,9 +104,10 @@ pub trait EcdsaChannelSigner: ChannelSigner {
 	/// An `Err` can be returned to signal that the signer is unavailable/cannot produce a valid
 	/// signature and should be retried later. Once the signer is ready to provide a signature after
 	/// previously returning an `Err`, [`ChannelMonitor::signer_unblocked`] must be called on its
-	/// monitor.
+	/// monitor or [`ChainMonitor::signer_unblocked`] called to attempt unblocking all monitors.
 	///
 	/// [`ChannelMonitor::signer_unblocked`]: crate::chain::channelmonitor::ChannelMonitor::signer_unblocked
+	/// [`ChainMonitor::signer_unblocked`]: crate::chain::chainmonitor::ChainMonitor::signer_unblocked
 	fn sign_justice_revoked_output(
 		&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey,
 		secp_ctx: &Secp256k1<secp256k1::All>,
@@ -121,9 +134,10 @@ pub trait EcdsaChannelSigner: ChannelSigner {
 	/// An `Err` can be returned to signal that the signer is unavailable/cannot produce a valid
 	/// signature and should be retried later. Once the signer is ready to provide a signature after
 	/// previously returning an `Err`, [`ChannelMonitor::signer_unblocked`] must be called on its
-	/// monitor.
+	/// monitor or [`ChainMonitor::signer_unblocked`] called to attempt unblocking all monitors.
 	///
 	/// [`ChannelMonitor::signer_unblocked`]: crate::chain::channelmonitor::ChannelMonitor::signer_unblocked
+	/// [`ChainMonitor::signer_unblocked`]: crate::chain::chainmonitor::ChainMonitor::signer_unblocked
 	fn sign_justice_revoked_htlc(
 		&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey,
 		htlc: &HTLCOutputInCommitment, secp_ctx: &Secp256k1<secp256k1::All>,
@@ -139,11 +153,12 @@ pub trait EcdsaChannelSigner: ChannelSigner {
 	/// An `Err` can be returned to signal that the signer is unavailable/cannot produce a valid
 	/// signature and should be retried later. Once the signer is ready to provide a signature after
 	/// previously returning an `Err`, [`ChannelMonitor::signer_unblocked`] must be called on its
-	/// monitor.
+	/// monitor or [`ChainMonitor::signer_unblocked`] called to attempt unblocking all monitors.
 	///
 	/// [`EcdsaSighashType::All`]: bitcoin::sighash::EcdsaSighashType::All
 	/// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor
 	/// [`ChannelMonitor::signer_unblocked`]: crate::chain::channelmonitor::ChannelMonitor::signer_unblocked
+	/// [`ChainMonitor::signer_unblocked`]: crate::chain::chainmonitor::ChainMonitor::signer_unblocked
 	fn sign_holder_htlc_transaction(
 		&self, htlc_tx: &Transaction, input: usize, htlc_descriptor: &HTLCDescriptor,
 		secp_ctx: &Secp256k1<secp256k1::All>,
@@ -169,9 +184,10 @@ pub trait EcdsaChannelSigner: ChannelSigner {
 	/// An `Err` can be returned to signal that the signer is unavailable/cannot produce a valid
 	/// signature and should be retried later. Once the signer is ready to provide a signature after
 	/// previously returning an `Err`, [`ChannelMonitor::signer_unblocked`] must be called on its
-	/// monitor.
+	/// monitor or [`ChainMonitor::signer_unblocked`] called to attempt unblocking all monitors.
 	///
 	/// [`ChannelMonitor::signer_unblocked`]: crate::chain::channelmonitor::ChannelMonitor::signer_unblocked
+	/// [`ChainMonitor::signer_unblocked`]: crate::chain::chainmonitor::ChainMonitor::signer_unblocked
 	fn sign_counterparty_htlc_transaction(
 		&self, htlc_tx: &Transaction, input: usize, amount: u64, per_commitment_point: &PublicKey,
 		htlc: &HTLCOutputInCommitment, secp_ctx: &Secp256k1<secp256k1::All>,
@@ -180,6 +196,12 @@ pub trait EcdsaChannelSigner: ChannelSigner {
 	///
 	/// Note that, due to rounding, there may be one "missing" satoshi, and either party may have
 	/// chosen to forgo their output as dust.
+	///
+	/// An `Err` can be returned to signal that the signer is unavailable/cannot produce a valid
+	/// signature and should be retried later. Once the signer is ready to provide a signature after
+	/// previously returning an `Err`, [`ChannelManager::signer_unblocked`] must be called.
+	///
+	/// [`ChannelManager::signer_unblocked`]: crate::ln::channelmanager::ChannelManager::signer_unblocked
 	fn sign_closing_transaction(
 		&self, closing_tx: &ClosingTransaction, secp_ctx: &Secp256k1<secp256k1::All>,
 	) -> Result<Signature, ()>;
@@ -189,9 +211,10 @@ pub trait EcdsaChannelSigner: ChannelSigner {
 	/// An `Err` can be returned to signal that the signer is unavailable/cannot produce a valid
 	/// signature and should be retried later. Once the signer is ready to provide a signature after
 	/// previously returning an `Err`, [`ChannelMonitor::signer_unblocked`] must be called on its
-	/// monitor.
+	/// monitor or [`ChainMonitor::signer_unblocked`] called to attempt unblocking all monitors.
 	///
 	/// [`ChannelMonitor::signer_unblocked`]: crate::chain::channelmonitor::ChannelMonitor::signer_unblocked
+	/// [`ChainMonitor::signer_unblocked`]: crate::chain::chainmonitor::ChainMonitor::signer_unblocked
 	fn sign_holder_anchor_input(
 		&self, anchor_tx: &Transaction, input: usize, secp_ctx: &Secp256k1<secp256k1::All>,
 	) -> Result<Signature, ()>;
@@ -201,9 +224,9 @@ pub trait EcdsaChannelSigner: ChannelSigner {
 	/// Channel announcements also require a signature from each node's network key. Our node
 	/// signature is computed through [`NodeSigner::sign_gossip_message`].
 	///
-	/// Note that if this fails or is rejected, the channel will not be publicly announced and
-	/// our counterparty may (though likely will not) close the channel on us for violating the
-	/// protocol.
+	/// This method is *not* asynchronous. If an `Err` is returned, the channel will not be
+	/// publicly announced and our counterparty may (though likely will not) close the channel on
+	/// us for violating the protocol.
 	///
 	/// [`NodeSigner::sign_gossip_message`]: crate::sign::NodeSigner::sign_gossip_message
 	fn sign_channel_announcement_with_funding_key(
@@ -219,6 +242,9 @@ pub trait EcdsaChannelSigner: ChannelSigner {
 	///    spending the previous funding transaction's output
 	///
 	/// `input_value`: The value of the previous funding transaction output.
+	///
+	/// This method is *not* asynchronous. If an `Err` is returned, the channel will be immediately
+	/// closed.
 	fn sign_splicing_funding_input(
 		&self, tx: &Transaction, input_index: usize, input_value: u64,
 		secp_ctx: &Secp256k1<secp256k1::All>,