Skip to content

Commit ed84b02

Browse files
TheBlueMattTheBlueMatt
authored
Merge pull request #655 from lightning-signer/per-commitment
ChannelKeys - separate commitment revocation from getting the per-commitment point
2 parents 50df4cf + b19d447 commit ed84b02

File tree

5 files changed

+55
-36
lines changed

5 files changed

+55
-36
lines changed

‎lightning/src/chain/keysinterface.rs

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -195,9 +195,20 @@ impl Readable for SpendableOutputDescriptor {
195195
// TODO: We should remove Clone by instead requesting a new ChannelKeys copy when we create
196196
// ChannelMonitors instead of expecting to clone the one out of the Channel into the monitors.
197197
pub trait ChannelKeys : Send+Clone {
198-
/// Gets the commitment seed for a specific commitment number
199-
/// Note that the commitment number starts at (1 << 48) - 1 and counts backwards
200-
fn commitment_secret(&self, idx: u64) -> [u8; 32];
198+
/// Gets the per-commitment point for a specific commitment number
199+
///
200+
/// Note that the commitment number starts at (1 << 48) - 1 and counts backwards.
201+
fn get_per_commitment_point<T: secp256k1::Signing + secp256k1::Verification>(&self, idx: u64, secp_ctx: &Secp256k1<T>) -> PublicKey;
202+
/// Gets the commitment secret for a specific commitment number as part of the revocation process
203+
///
204+
/// An external signer implementation should error here if the commitment was already signed
205+
/// and should refuse to sign it in the future.
206+
///
207+
/// May be called more than once for the same index.
208+
///
209+
/// Note that the commitment number starts at (1 << 48) - 1 and counts backwards.
210+
/// TODO: return a Result so we can signal a validation error
211+
fn release_commitment_secret(&self, idx: u64) -> [u8; 32];
201212
/// Gets the local channel public keys and basepoints
202213
fn pubkeys(&self) -> &ChannelPublicKeys;
203214
/// Gets arbitrary identifiers describing the set of keys which are provided back to you in
@@ -217,6 +228,7 @@ pub trait ChannelKeys : Send+Clone {
217228
/// Create a signature for a local commitment transaction. This will only ever be called with
218229
/// the same local_commitment_tx (or a copy thereof), though there are currently no guarantees
219230
/// that it will not be called multiple times.
231+
/// An external signer implementation should check that the commitment has not been revoked.
220232
//
221233
// TODO: Document the things someone using this interface should enforce before signing.
222234
// TODO: Add more input vars to enable better checking (preferably removing commitment_tx and
@@ -405,7 +417,12 @@ impl InMemoryChannelKeys {
405417
}
406418

407419
impl ChannelKeys for InMemoryChannelKeys {
408-
fn commitment_secret(&self, idx: u64) -> [u8; 32] {
420+
fn get_per_commitment_point<T: secp256k1::Signing + secp256k1::Verification>(&self, idx: u64, secp_ctx: &Secp256k1<T>) -> PublicKey {
421+
let commitment_secret = SecretKey::from_slice(&chan_utils::build_commitment_secret(&self.commitment_seed, idx)).unwrap();
422+
PublicKey::from_secret_key(secp_ctx, &commitment_secret)
423+
}
424+
425+
fn release_commitment_secret(&self, idx: u64) -> [u8; 32] {
409426
chan_utils::build_commitment_secret(&self.commitment_seed, idx)
410427
}
411428

‎lightning/src/ln/chan_utils.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -497,8 +497,8 @@ pub fn build_htlc_transaction(prev_hash: &Txid, feerate_per_kw: u32, to_self_del
497497

498498
#[derive(Clone)]
499499
/// We use this to track local commitment transactions and put off signing them until we are ready
500-
/// to broadcast. Eventually this will require a signer which is possibly external, but for now we
501-
/// just pass in the SecretKeys required.
500+
/// to broadcast. This class can be used inside a signer implementation to generate a signature
501+
/// given the relevant secret key.
502502
pub struct LocalCommitmentTransaction {
503503
// TODO: We should migrate away from providing the transaction, instead providing enough to
504504
// allow the ChannelKeys to construct it from scratch. Luckily we already have HTLC data here,

‎lightning/src/ln/channel.rs

Lines changed: 17 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -785,13 +785,6 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
785785
Ok(chan)
786786
}
787787

788-
// Utilities to derive keys:
789-
790-
fn build_local_commitment_secret(&self, idx: u64) -> SecretKey {
791-
let res = self.local_keys.commitment_secret(idx);
792-
SecretKey::from_slice(&res).unwrap()
793-
}
794-
795788
// Utilities to build transactions:
796789

797790
fn get_commitment_transaction_number_obscure_factor(&self) -> u64 {
@@ -1123,7 +1116,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
11231116
/// The result is a transaction which we can revoke ownership of (ie a "local" transaction)
11241117
/// TODO Some magic rust shit to compile-time check this?
11251118
fn build_local_transaction_keys(&self, commitment_number: u64) -> Result<TxCreationKeys, ChannelError> {
1126-
let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &self.build_local_commitment_secret(commitment_number));
1119+
let per_commitment_point = self.local_keys.get_per_commitment_point(commitment_number, &self.secp_ctx);
11271120
let delayed_payment_base = &self.local_keys.pubkeys().delayed_payment_basepoint;
11281121
let htlc_basepoint = &self.local_keys.pubkeys().htlc_basepoint;
11291122
let their_pubkeys = self.their_pubkeys.as_ref().unwrap();
@@ -2028,8 +2021,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
20282021
}
20292022
}
20302023

2031-
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &self.build_local_commitment_secret(self.cur_local_commitment_transaction_number - 1));
2032-
let per_commitment_secret = self.local_keys.commitment_secret(self.cur_local_commitment_transaction_number + 1);
2024+
let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number - 1, &self.secp_ctx);
2025+
let per_commitment_secret = self.local_keys.release_commitment_secret(self.cur_local_commitment_transaction_number + 1);
20332026

20342027
// Update state now that we've passed all the can-fail calls...
20352028
let mut need_our_commitment = false;
@@ -2614,8 +2607,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
26142607
let funding_locked = if self.monitor_pending_funding_locked {
26152608
assert!(!self.channel_outbound, "Funding transaction broadcast without FundingBroadcastSafe!");
26162609
self.monitor_pending_funding_locked = false;
2617-
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
2618-
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
2610+
let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
26192611
Some(msgs::FundingLocked {
26202612
channel_id: self.channel_id(),
26212613
next_per_commitment_point: next_per_commitment_point,
@@ -2667,8 +2659,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
26672659
}
26682660

26692661
fn get_last_revoke_and_ack(&self) -> msgs::RevokeAndACK {
2670-
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &self.build_local_commitment_secret(self.cur_local_commitment_transaction_number));
2671-
let per_commitment_secret = self.local_keys.commitment_secret(self.cur_local_commitment_transaction_number + 2);
2662+
let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
2663+
let per_commitment_secret = self.local_keys.release_commitment_secret(self.cur_local_commitment_transaction_number + 2);
26722664
msgs::RevokeAndACK {
26732665
channel_id: self.channel_id,
26742666
per_commitment_secret,
@@ -2751,7 +2743,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
27512743
if msg.next_remote_commitment_number > 0 {
27522744
match msg.data_loss_protect {
27532745
OptionalField::Present(ref data_loss) => {
2754-
if self.local_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1) != data_loss.your_last_per_commitment_secret {
2746+
let expected_point = self.local_keys.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1, &self.secp_ctx);
2747+
let given_secret = SecretKey::from_slice(&data_loss.your_last_per_commitment_secret)
2748+
.map_err(|_| ChannelError::Close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?;
2749+
if expected_point != PublicKey::from_secret_key(&self.secp_ctx, &given_secret) {
27552750
return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided".to_owned()));
27562751
}
27572752
if msg.next_remote_commitment_number > INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number {
@@ -2787,8 +2782,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
27872782
}
27882783

27892784
// We have OurFundingLocked set!
2790-
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
2791-
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
2785+
let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
27922786
return Ok((Some(msgs::FundingLocked {
27932787
channel_id: self.channel_id(),
27942788
next_per_commitment_point: next_per_commitment_point,
@@ -2818,8 +2812,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
28182812

28192813
let resend_funding_locked = if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number == 1 {
28202814
// We should never have to worry about MonitorUpdateFailed resending FundingLocked
2821-
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
2822-
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
2815+
let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
28232816
Some(msgs::FundingLocked {
28242817
channel_id: self.channel_id(),
28252818
next_per_commitment_point: next_per_commitment_point,
@@ -3405,8 +3398,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
34053398
//a protocol oversight, but I assume I'm just missing something.
34063399
if need_commitment_update {
34073400
if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 {
3408-
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
3409-
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
3401+
let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
34103402
return Ok((Some(msgs::FundingLocked {
34113403
channel_id: self.channel_id,
34123404
next_per_commitment_point: next_per_commitment_point,
@@ -3457,7 +3449,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
34573449
panic!("Tried to send an open_channel for a channel that has already advanced");
34583450
}
34593451

3460-
let local_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
3452+
let first_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
34613453
let local_keys = self.local_keys.pubkeys();
34623454

34633455
msgs::OpenChannel {
@@ -3477,7 +3469,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
34773469
payment_point: local_keys.payment_point,
34783470
delayed_payment_basepoint: local_keys.delayed_payment_basepoint,
34793471
htlc_basepoint: local_keys.htlc_basepoint,
3480-
first_per_commitment_point: PublicKey::from_secret_key(&self.secp_ctx, &local_commitment_secret),
3472+
first_per_commitment_point,
34813473
channel_flags: if self.config.announced_channel {1} else {0},
34823474
shutdown_scriptpubkey: OptionalField::Present(if self.config.commit_upfront_shutdown_pubkey { self.get_closing_scriptpubkey() } else { Builder::new().into_script() })
34833475
}
@@ -3494,7 +3486,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
34943486
panic!("Tried to send an accept_channel for a channel that has already advanced");
34953487
}
34963488

3497-
let local_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
3489+
let first_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
34983490
let local_keys = self.local_keys.pubkeys();
34993491

35003492
msgs::AcceptChannel {
@@ -3511,7 +3503,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
35113503
payment_point: local_keys.payment_point,
35123504
delayed_payment_basepoint: local_keys.delayed_payment_basepoint,
35133505
htlc_basepoint: local_keys.htlc_basepoint,
3514-
first_per_commitment_point: PublicKey::from_secret_key(&self.secp_ctx, &local_commitment_secret),
3506+
first_per_commitment_point,
35153507
shutdown_scriptpubkey: OptionalField::Present(if self.config.commit_upfront_shutdown_pubkey { self.get_closing_scriptpubkey() } else { Builder::new().into_script() })
35163508
}
35173509
}

‎lightning/src/ln/functional_tests.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1612,15 +1612,15 @@ fn test_fee_spike_violation_fails_htlc() {
16121612
let chan_keys = local_chan.get_local_keys();
16131613
let pubkeys = chan_keys.pubkeys();
16141614
(pubkeys.revocation_basepoint, pubkeys.htlc_basepoint, pubkeys.payment_point,
1615-
chan_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER), chan_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER - 2))
1615+
chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER), chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2))
16161616
};
16171617
let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_payment_point, remote_secret1) = {
16181618
let chan_lock = nodes[1].node.channel_state.lock().unwrap();
16191619
let remote_chan = chan_lock.by_id.get(&chan.2).unwrap();
16201620
let chan_keys = remote_chan.get_local_keys();
16211621
let pubkeys = chan_keys.pubkeys();
16221622
(pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint, pubkeys.payment_point,
1623-
chan_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER - 1))
1623+
chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 1))
16241624
};
16251625

16261626
// Assemble the set of keys we can use for signatures for our commitment_signed message.
@@ -8132,8 +8132,8 @@ fn test_counterparty_raa_skip_no_crash() {
81328132
let local_keys = &guard.by_id.get_mut(&channel_id).unwrap().local_keys;
81338133
const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1;
81348134
let next_per_commitment_point = PublicKey::from_secret_key(&Secp256k1::new(),
8135-
&SecretKey::from_slice(&local_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)).unwrap());
8136-
let per_commitment_secret = local_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER);
8135+
&SecretKey::from_slice(&local_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)).unwrap());
8136+
let per_commitment_secret = local_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER);
81378137

81388138
nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(),
81398139
&msgs::RevokeAndACK { channel_id, per_commitment_secret, next_per_commitment_point });

‎lightning/src/util/enforcing_trait_impls.rs

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,15 @@ impl EnforcingChannelKeys {
4848
}
4949

5050
impl ChannelKeys for EnforcingChannelKeys {
51-
fn commitment_secret(&self, idx: u64) -> [u8; 32] { self.inner.commitment_secret(idx) }
51+
fn get_per_commitment_point<T: secp256k1::Signing + secp256k1::Verification>(&self, idx: u64, secp_ctx: &Secp256k1<T>) -> PublicKey {
52+
self.inner.get_per_commitment_point(idx, secp_ctx)
53+
}
54+
55+
fn release_commitment_secret(&self, idx: u64) -> [u8; 32] {
56+
// TODO: enforce the ChannelKeys contract - error here if we already signed this commitment
57+
self.inner.release_commitment_secret(idx)
58+
}
59+
5260
fn pubkeys(&self) -> &ChannelPublicKeys { self.inner.pubkeys() }
5361
fn key_derivation_params(&self) -> (u64, u64) { self.inner.key_derivation_params() }
5462

@@ -71,6 +79,8 @@ impl ChannelKeys for EnforcingChannelKeys {
7179
}
7280

7381
fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
82+
// TODO: enforce the ChannelKeys contract - error if this commitment was already revoked
83+
// TODO: need the commitment number
7484
Ok(self.inner.sign_local_commitment(local_commitment_tx, secp_ctx).unwrap())
7585
}
7686

0 commit comments

Comments
 (0)