f Matt's feedback

Author: tnull

lightning/src/chain/keysinterface.rs

@@ -58,14 +58,14 @@ use crate::util::invoice::construct_invoice_preimage;
 #[derive(Hash, Copy, Clone, PartialEq, Eq, Debug)]
 pub struct KeyMaterial(pub [u8; 32]);
 
-/// Information about a spendable output to a `P2WSH` script.
+/// Information about a spendable output to a P2WSH script.
 ///
 /// See [`SpendableOutputDescriptor::DelayedPaymentOutput`] for more details on how to spend this.
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct DelayedPaymentOutputDescriptor {
 	/// The outpoint which is spendable.
 	pub outpoint: OutPoint,
-	/// Per commitment point to derive `delayed_payment_key` by key holder
+	/// Per commitment point to derive the delayed payment key by key holder.
 	pub per_commitment_point: PublicKey,
 	/// The `nSequence` value which must be set in the spending input to satisfy the `OP_CSV` in
 	/// the witness_script.
@@ -151,40 +151,46 @@ pub enum SpendableOutputDescriptor {
 		/// The output which is referenced by the given outpoint.
 		output: TxOut,
 	},
-	/// An output to a `P2WSH` script which can be spent with a single signature after an `OP_CSV`
+	/// An output to a P2WSH script which can be spent with a single signature after an `OP_CSV`
 	/// delay.
 	///
 	/// The witness in the spending input should be:
 	/// ```bitcoin
 	/// <BIP 143 signature> <empty vector> (MINIMALIF standard rule) <provided witnessScript>
 	/// ```
 	///
-	/// Note that the `nSequence` field in the spending input must be set to `to_self_delay` (which
-	/// means the transaction is not broadcastable until at least `to_self_delay` blocks after the
-	/// outpoint confirms, see [BIP
-	/// 68](https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki)).
+	/// Note that the `nSequence` field in the spending input must be set to
+	/// [`DelayedPaymentOutputDescriptor::to_self_delay`] (which means the transaction is not
+	/// broadcastable until at least [`DelayedPaymentOutputDescriptor::to_self_delay`] blocks after
+	/// the outpoint confirms, see [BIP
+	/// 68](https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki)). Also note that LDK
+	/// won't generate a [`SpendableOutputDescriptor`] until the corresponding block height
+	/// is reached.
 	///
 	/// These are generally the result of a "revocable" output to us, spendable only by us unless
 	/// it is an output from an old state which we broadcast (which should never happen).
 	///
-	/// To derive the `delayed_payment` key which is used to sign this input, you must pass the
-	/// holder `delayed_payment_base_key` (i.e., the private key which corresponds to the
-	/// `delayed_payment_basepoint` in [`BaseSign::pubkeys`]) and the provided
-	/// `per_commitment_point` to [`chan_utils::derive_private_key`]. The public key can be
+	/// To derive the delayed payment key which is used to sign this input, you must pass the
+	/// holder [`InMemorySigner::delayed_payment_base_key`] (i.e., the private key which corresponds to the
+	/// [`ChannelPublicKeys::delayed_payment_basepoint`] in [`BaseSign::pubkeys`]) and the provided
+	/// [`DelayedPaymentOutputDescriptor::per_commitment_point`] to [`chan_utils::derive_private_key`]. The public key can be
 	/// generated without the secret key using [`chan_utils::derive_public_key`] and only the
-	/// `delayed_payment_basepoint` which appears in [`BaseSign::pubkeys`].
+	/// [`ChannelPublicKeys::delayed_payment_basepoint`] which appears in [`BaseSign::pubkeys`].
 	///
-	/// To derive the `revocation_pubkey` provided here (which is used in the witness script
-	/// generation), you must pass the counterparty `revocation_basepoint` (which appears in the
-	/// call to [`BaseSign::provide_channel_parameters`]) and the provided per_commitment point to
+	/// To derive the [`DelayedPaymentOutputDescriptor::revocation_pubkey`] provided here (which is
+	/// used in the witness script generation), you must pass the counterparty
+	/// [`ChannelPublicKeys::revocation_basepoint`] (which appears in the call to
+	/// [`BaseSign::provide_channel_parameters`]) and the provided
+	/// [`DelayedPaymentOutputDescriptor::per_commitment_point`] to
 	/// [`chan_utils::derive_public_revocation_key`].
 	///
 	/// The witness script which is hashed and included in the output `script_pubkey` may be
-	/// regenerated by passing the `revocation_pubkey` (derived as above), our `delayed_payment`
-	/// pubkey (derived as above), and the `to_self_delay` contained here to
+	/// regenerated by passing the [`DelayedPaymentOutputDescriptor::revocation_pubkey`] (derived
+	/// as explained above), our delayed payment pubkey (derived as explained above), and the
+	/// [`DelayedPaymentOutputDescriptor::to_self_delay`] contained here to
 	/// [`chan_utils::get_revokeable_redeemscript`].
 	DelayedPaymentOutput(DelayedPaymentOutputDescriptor),
-	/// An output to a `P2WPKH`, spendable exclusively by our payment key (i.e., the private key
+	/// An output to a P2WPKH, spendable exclusively by our payment key (i.e., the private key
 	/// which corresponds to the `payment_point` in [`BaseSign::pubkeys`]). The witness
 	/// in the spending input is, thus, simply:
 	/// ```bitcoin
@@ -209,24 +215,10 @@ impl_writeable_tlv_based_enum!(SpendableOutputDescriptor,
 /// A trait to sign Lightning channel transactions as described in
 /// [BOLT 3](https://github.com/lightning/bolts/blob/master/03-transactions.md).
 ///
-/// Signing services could be implemented on a hardware wallet. In this case,
-/// the current [`BaseSign`] would be a front-end on top of a communication
-/// channel connected to your secure device and lightning key material wouldn't
-/// reside on a hot server. Nevertheless, a this deployment would still need
-/// to trust the ChannelManager to avoid loss of funds as this latest component
-/// could ask to sign commitment transaction with HTLCs paying to attacker pubkeys.
-///
-/// A more secure iteration would be to use hashlock (or payment points) to pair
-/// invoice/incoming HTLCs with outgoing HTLCs to implement a no-trust-[`ChannelManager`]
-/// at the price of more state and computation on the hardware wallet side. In the future,
-/// we are looking forward to design such interface.
-///
-/// In any case, [`ChannelMonitor`] or fallback watchtowers are always going to be trusted
-/// to act, as liveness and breach reply correctness are always going to be hard requirements
-/// of LN security model, orthogonal of key management issues.
-///
-/// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor
-/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
+/// Signing services could be implemented on a hardware wallet and should implement signing
+/// 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.
 pub trait BaseSign {
 	/// Gets the per-commitment point for a specific commitment number
 	///
@@ -287,12 +279,15 @@ pub trait BaseSign {
 	/// forward and it is safe to sign the next counterparty commitment.
 	fn validate_counterparty_revocation(&self, idx: u64, secret: &SecretKey) -> Result<(), ()>;
 	/// Creates a signature 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
-	/// latest `commitment_tx` when we initiate a force-close.
-	/// This will be called with the previous latest `commitment_tx`, just to get claiming HTLC
-	/// signatures, if we are reacting to a [`ChannelMonitor`]
-	/// [replica](https://github.com/lightningdevkit/rust-lightning/blob/main/GLOSSARY.md#monitor-replicas)
-	/// that decided to broadcast before it had been updated to the latest `commitment_tx`.
+	///
+	/// This will be called
+	/// - with a non-revoked `commitment_tx`.
+	/// - with the latest `commitment_tx` when we initiate a force-close.
+	/// - with the previous `commitment_tx`, just to get claiming HTLC
+	///   signatures, if we are reacting to a [`ChannelMonitor`]
+	///   [replica](https://github.com/lightningdevkit/rust-lightning/blob/main/GLOSSARY.md#monitor-replicas)
+	///   that decided to broadcast before it had been updated to the latest `commitment_tx`.
+	///
 	/// This may be called multiple times for the same transaction.
 	///
 	/// An external signer implementation should check that the commitment has not been revoked.
@@ -305,10 +300,10 @@ pub trait BaseSign {
 	// TODO: Key derivation failure should panic rather than Err
 	fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction,
 		secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<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.
+	/// Same as [`sign_holder_commitment_and_htlcs`], 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_and_htlcs`] may
+	/// enforce that we only ever get called once.
 	#[cfg(any(test,feature = "unsafe_revoked_tx_signing"))]
 	fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction,
 		secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()>;
@@ -437,7 +432,7 @@ pub enum Recipient {
 
 /// A trait to describe an object which can get user secrets and key material.
 pub trait KeysInterface {
-	/// A type which implements Sign which will be returned by [`Self::derive_channel_signer`].
+	/// A type which implements [`Sign`] which will be returned by [`Self::derive_channel_signer`].
 	type Signer : Sign;
 	/// Get node secret key based on the provided [`Recipient`].
 	///