From 1f7f3a366c9e62cff5a5025724b5b508255a89d7 Mon Sep 17 00:00:00 2001 From: Alec Chen Date: Wed, 5 Jun 2024 17:40:38 -0500 Subject: [PATCH] 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. --- lightning/src/ln/channel.rs | 47 +++++++++++++++-------- lightning/src/ln/functional_tests.rs | 8 ++-- lightning/src/sign/mod.rs | 19 +++++++-- lightning/src/util/test_channel_signer.rs | 4 +- 4 files changed, 53 insertions(+), 25 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 6d72bf0eb..9a824b2d6 100644 --- a/lightning/src/ln/channel.rs +++ b/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); + } } } @@ -5622,7 +5629,9 @@ impl Channel 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: async signing is not yet supported for per commitment points upon channel reestablishment"); 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) { @@ -8230,10 +8239,12 @@ impl OutboundV2Channel where SP::Target: SignerProvider { let first_per_commitment_point = self.context.holder_signer.as_ref() .get_per_commitment_point(self.context.holder_commitment_point.transaction_number(), - &self.context.secp_ctx); + &self.context.secp_ctx) + .expect("TODO: async signing is not yet supported for commitment points in v2 channel establishment"); let second_per_commitment_point = self.context.holder_signer.as_ref() .get_per_commitment_point(self.context.holder_commitment_point.transaction_number() - 1, - &self.context.secp_ctx); + &self.context.secp_ctx) + .expect("TODO: async signing is not yet supported for commitment points in v2 channel establishment"); let keys = self.context.get_holder_pubkeys(); msgs::OpenChannelV2 { @@ -8380,9 +8391,11 @@ impl InboundV2Channel where SP::Target: SignerProvider { /// [`msgs::AcceptChannelV2`]: crate::ln::msgs::AcceptChannelV2 fn generate_accept_channel_v2_message(&self) -> msgs::AcceptChannelV2 { let first_per_commitment_point = self.context.holder_signer.as_ref().get_per_commitment_point( - self.context.holder_commitment_point.transaction_number(), &self.context.secp_ctx); + self.context.holder_commitment_point.transaction_number(), &self.context.secp_ctx) + .expect("TODO: async signing is not yet supported for commitment points in v2 channel establishment"); let second_per_commitment_point = self.context.holder_signer.as_ref().get_per_commitment_point( - self.context.holder_commitment_point.transaction_number() - 1, &self.context.secp_ctx); + self.context.holder_commitment_point.transaction_number() - 1, &self.context.secp_ctx) + .expect("TODO: async signing is not yet supported for commitment points in v2 channel establishment"); let keys = self.context.get_holder_pubkeys(); msgs::AcceptChannelV2 { @@ -9334,14 +9347,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, + } }, }; diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index e56fbf76c..6a0343fb5 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/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) }; diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs index 885b8840b..d684b4fe6 100644 --- a/lightning/src/sign/mod.rs +++ b/lightning/src/sign/mod.rs @@ -725,12 +725,23 @@ impl HTLCDescriptor { /// A trait to handle Lightning channel key material without concretizing the channel type or /// the signature mechanism. +/// +/// Several methods allow error types to be returned to support async signing. This feature +/// is not yet complete, and panics may occur in certain situations when returning errors +/// for these methods. 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) - -> PublicKey; + /// + /// If the signer returns `Err`, then the user is responsible for either force-closing the channel + /// or calling `ChannelManager::signer_unblocked` (this method is only available when the + /// `async_signing` cfg flag is enabled) once the signature is ready. + /// + // TODO: link to `signer_unblocked` once the cfg flag is removed + fn get_per_commitment_point( + &self, idx: u64, secp_ctx: &Secp256k1, + ) -> Result; /// Gets the commitment secret for a specific commitment number as part of the revocation process /// @@ -1343,11 +1354,11 @@ impl EntropySource for InMemorySigner { impl ChannelSigner for InMemorySigner { fn get_per_commitment_point( &self, idx: u64, secp_ctx: &Secp256k1, - ) -> PublicKey { + ) -> Result { 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] { diff --git a/lightning/src/util/test_channel_signer.rs b/lightning/src/util/test_channel_signer.rs index 28647982b..aa491b04b 100644 --- a/lightning/src/util/test_channel_signer.rs +++ b/lightning/src/util/test_channel_signer.rs @@ -164,7 +164,9 @@ impl TestChannelSigner { } impl ChannelSigner for TestChannelSigner { - fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1) -> PublicKey { + fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1) -> Result { + // TODO: implement a mask in EnforcementState to let you test signatures being + // unavailable self.inner.get_per_commitment_point(idx, secp_ctx) } -- 2.39.5