Skip to content

Commit 1f7f3a3

Browse files
alecchendevalecchendev
committed
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.
1 parent 1fa67d9 commit 1f7f3a3

File tree

4 files changed

+53
-25
lines changed

4 files changed

+53
-25
lines changed

‎lightning/src/ln/channel.rs

+31-16
Original file line numberDiff line numberDiff line change
@@ -962,8 +962,12 @@ impl HolderCommitmentPoint {
962962
{
963963
HolderCommitmentPoint::Available {
964964
transaction_number: INITIAL_COMMITMENT_NUMBER,
965-
current: signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, secp_ctx),
966-
next: signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, secp_ctx),
965+
// TODO(async_signing): remove this expect with the Uninitialized variant
966+
current: signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, secp_ctx)
967+
.expect("Signer must be able to provide initial commitment point"),
968+
// TODO(async_signing): remove this expect with the Uninitialized variant
969+
next: signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, secp_ctx)
970+
.expect("Signer must be able to provide second commitment point"),
967971
}
968972
}
969973

@@ -1001,9 +1005,12 @@ impl HolderCommitmentPoint {
10011005
where SP::Target: SignerProvider, L::Target: Logger
10021006
{
10031007
if let HolderCommitmentPoint::PendingNext { transaction_number, current } = self {
1004-
let next = signer.as_ref().get_per_commitment_point(*transaction_number - 1, secp_ctx);
1005-
log_trace!(logger, "Retrieved next per-commitment point {}", *transaction_number - 1);
1006-
*self = HolderCommitmentPoint::Available { transaction_number: *transaction_number, current: *current, next };
1008+
if let Ok(next) = signer.as_ref().get_per_commitment_point(*transaction_number - 1, secp_ctx) {
1009+
log_trace!(logger, "Retrieved next per-commitment point {}", *transaction_number - 1);
1010+
*self = HolderCommitmentPoint::Available { transaction_number: *transaction_number, current: *current, next };
1011+
} else {
1012+
log_trace!(logger, "Next per-commitment point {} is pending", transaction_number);
1013+
}
10071014
}
10081015
}
10091016

@@ -5622,7 +5629,9 @@ impl<SP: Deref> Channel<SP> where
56225629

56235630
let our_commitment_transaction = INITIAL_COMMITMENT_NUMBER - self.context.holder_commitment_point.transaction_number() - 1;
56245631
if msg.next_remote_commitment_number > 0 {
5625-
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);
5632+
let expected_point = self.context.holder_signer.as_ref()
5633+
.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1, &self.context.secp_ctx)
5634+
.expect("TODO: async signing is not yet supported for per commitment points upon channel reestablishment");
56265635
let given_secret = SecretKey::from_slice(&msg.your_last_per_commitment_secret)
56275636
.map_err(|_| ChannelError::close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?;
56285637
if expected_point != PublicKey::from_secret_key(&self.context.secp_ctx, &given_secret) {
@@ -8230,10 +8239,12 @@ impl<SP: Deref> OutboundV2Channel<SP> where SP::Target: SignerProvider {
82308239

82318240
let first_per_commitment_point = self.context.holder_signer.as_ref()
82328241
.get_per_commitment_point(self.context.holder_commitment_point.transaction_number(),
8233-
&self.context.secp_ctx);
8242+
&self.context.secp_ctx)
8243+
.expect("TODO: async signing is not yet supported for commitment points in v2 channel establishment");
82348244
let second_per_commitment_point = self.context.holder_signer.as_ref()
82358245
.get_per_commitment_point(self.context.holder_commitment_point.transaction_number() - 1,
8236-
&self.context.secp_ctx);
8246+
&self.context.secp_ctx)
8247+
.expect("TODO: async signing is not yet supported for commitment points in v2 channel establishment");
82378248
let keys = self.context.get_holder_pubkeys();
82388249

82398250
msgs::OpenChannelV2 {
@@ -8380,9 +8391,11 @@ impl<SP: Deref> InboundV2Channel<SP> where SP::Target: SignerProvider {
83808391
/// [`msgs::AcceptChannelV2`]: crate::ln::msgs::AcceptChannelV2
83818392
fn generate_accept_channel_v2_message(&self) -> msgs::AcceptChannelV2 {
83828393
let first_per_commitment_point = self.context.holder_signer.as_ref().get_per_commitment_point(
8383-
self.context.holder_commitment_point.transaction_number(), &self.context.secp_ctx);
8394+
self.context.holder_commitment_point.transaction_number(), &self.context.secp_ctx)
8395+
.expect("TODO: async signing is not yet supported for commitment points in v2 channel establishment");
83848396
let second_per_commitment_point = self.context.holder_signer.as_ref().get_per_commitment_point(
8385-
self.context.holder_commitment_point.transaction_number() - 1, &self.context.secp_ctx);
8397+
self.context.holder_commitment_point.transaction_number() - 1, &self.context.secp_ctx)
8398+
.expect("TODO: async signing is not yet supported for commitment points in v2 channel establishment");
83868399
let keys = self.context.get_holder_pubkeys();
83878400

83888401
msgs::AcceptChannelV2 {
@@ -9334,14 +9347,16 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
93349347
(Some(current), Some(next)) => HolderCommitmentPoint::Available {
93359348
transaction_number: cur_holder_commitment_transaction_number, current, next
93369349
},
9337-
(Some(current), _) => HolderCommitmentPoint::Available {
9350+
(Some(current), _) => HolderCommitmentPoint::PendingNext {
93389351
transaction_number: cur_holder_commitment_transaction_number, current,
9339-
next: holder_signer.get_per_commitment_point(cur_holder_commitment_transaction_number - 1, &secp_ctx),
93409352
},
9341-
(_, _) => HolderCommitmentPoint::Available {
9342-
transaction_number: cur_holder_commitment_transaction_number,
9343-
current: holder_signer.get_per_commitment_point(cur_holder_commitment_transaction_number, &secp_ctx),
9344-
next: holder_signer.get_per_commitment_point(cur_holder_commitment_transaction_number - 1, &secp_ctx),
9353+
(_, _) => {
9354+
// TODO(async_signing): remove this expect with the Uninitialized variant
9355+
let current = holder_signer.get_per_commitment_point(cur_holder_commitment_transaction_number, &secp_ctx)
9356+
.expect("Must be able to derive the current commitment point upon channel restoration");
9357+
HolderCommitmentPoint::PendingNext {
9358+
transaction_number: cur_holder_commitment_transaction_number, current,
9359+
}
93459360
},
93469361
};
93479362

‎lightning/src/ln/functional_tests.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -741,7 +741,7 @@ fn test_update_fee_that_funder_cannot_afford() {
741741
(pubkeys.revocation_basepoint, pubkeys.htlc_basepoint,
742742
pubkeys.funding_pubkey)
743743
};
744-
let (remote_delayed_payment_basepoint, remote_htlc_basepoint,remote_point, remote_funding) = {
744+
let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point, remote_funding) = {
745745
let per_peer_state = nodes[1].node.per_peer_state.read().unwrap();
746746
let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap();
747747
let remote_chan = chan_lock.channel_by_id.get(&chan.2).map(
@@ -750,7 +750,7 @@ fn test_update_fee_that_funder_cannot_afford() {
750750
let chan_signer = remote_chan.get_signer();
751751
let pubkeys = chan_signer.as_ref().pubkeys();
752752
(pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint,
753-
chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx),
753+
chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap(),
754754
pubkeys.funding_pubkey)
755755
};
756756

@@ -1475,7 +1475,7 @@ fn test_fee_spike_violation_fails_htlc() {
14751475
let pubkeys = chan_signer.as_ref().pubkeys();
14761476
(pubkeys.revocation_basepoint, pubkeys.htlc_basepoint,
14771477
chan_signer.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER),
1478-
chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx),
1478+
chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx).unwrap(),
14791479
chan_signer.as_ref().pubkeys().funding_pubkey)
14801480
};
14811481
let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point, remote_funding) = {
@@ -1487,7 +1487,7 @@ fn test_fee_spike_violation_fails_htlc() {
14871487
let chan_signer = remote_chan.get_signer();
14881488
let pubkeys = chan_signer.as_ref().pubkeys();
14891489
(pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint,
1490-
chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx),
1490+
chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap(),
14911491
chan_signer.as_ref().pubkeys().funding_pubkey)
14921492
};
14931493

‎lightning/src/sign/mod.rs

+15-4
Original file line numberDiff line numberDiff line change
@@ -725,12 +725,23 @@ impl HTLCDescriptor {
725725

726726
/// A trait to handle Lightning channel key material without concretizing the channel type or
727727
/// the signature mechanism.
728+
///
729+
/// Several methods allow error types to be returned to support async signing. This feature
730+
/// is not yet complete, and panics may occur in certain situations when returning errors
731+
/// for these methods.
728732
pub trait ChannelSigner {
729733
/// Gets the per-commitment point for a specific commitment number
730734
///
731735
/// Note that the commitment number starts at `(1 << 48) - 1` and counts backwards.
732-
fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1<secp256k1::All>)
733-
-> PublicKey;
736+
///
737+
/// If the signer returns `Err`, then the user is responsible for either force-closing the channel
738+
/// or calling `ChannelManager::signer_unblocked` (this method is only available when the
739+
/// `async_signing` cfg flag is enabled) once the signature is ready.
740+
///
741+
// TODO: link to `signer_unblocked` once the cfg flag is removed
742+
fn get_per_commitment_point(
743+
&self, idx: u64, secp_ctx: &Secp256k1<secp256k1::All>,
744+
) -> Result<PublicKey, ()>;
734745

735746
/// Gets the commitment secret for a specific commitment number as part of the revocation process
736747
///
@@ -1343,11 +1354,11 @@ impl EntropySource for InMemorySigner {
13431354
impl ChannelSigner for InMemorySigner {
13441355
fn get_per_commitment_point(
13451356
&self, idx: u64, secp_ctx: &Secp256k1<secp256k1::All>,
1346-
) -> PublicKey {
1357+
) -> Result<PublicKey, ()> {
13471358
let commitment_secret =
13481359
SecretKey::from_slice(&chan_utils::build_commitment_secret(&self.commitment_seed, idx))
13491360
.unwrap();
1350-
PublicKey::from_secret_key(secp_ctx, &commitment_secret)
1361+
Ok(PublicKey::from_secret_key(secp_ctx, &commitment_secret))
13511362
}
13521363

13531364
fn release_commitment_secret(&self, idx: u64) -> [u8; 32] {

‎lightning/src/util/test_channel_signer.rs

+3-1
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,9 @@ impl TestChannelSigner {
164164
}
165165

166166
impl ChannelSigner for TestChannelSigner {
167-
fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1<secp256k1::All>) -> PublicKey {
167+
fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<PublicKey, ()> {
168+
// TODO: implement a mask in EnforcementState to let you test signatures being
169+
// unavailable
168170
self.inner.get_per_commitment_point(idx, secp_ctx)
169171
}
170172

0 commit comments

Comments
 (0)