Skip to content

ChannelKeys - separate commitment revocation from getting the per-commitment point #655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 22, 2020
Merged

ChannelKeys - separate commitment revocation from getting the per-commitment point #655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions lightning/src/chain/keysinterface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,9 +195,20 @@ impl Readable for SpendableOutputDescriptor {
// TODO: We should remove Clone by instead requesting a new ChannelKeys copy when we create
// ChannelMonitors instead of expecting to clone the one out of the Channel into the monitors.
pub trait ChannelKeys : Send+Clone {
/// Gets the commitment seed for a specific commitment number
/// Note that the commitment number starts at (1 << 48) - 1 and counts backwards
fn commitment_secret(&self, idx: u64) -> [u8; 32];
/// 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<T: secp256k1::Signing + secp256k1::Verification>(&self, idx: u64, secp_ctx: &Secp256k1<T>) -> PublicKey;
/// Gets the commitment secret for a specific commitment number as part of the revocation process
///
/// An external signer implementation should error here if the commitment was already signed
/// and should refuse to sign it in the future.
///
/// May be called more than once for the same index.
///
/// Note that the commitment number starts at (1 << 48) - 1 and counts backwards.
/// TODO: return a Result so we can signal a validation error
fn release_commitment_secret(&self, idx: u64) -> [u8; 32];
/// Gets the local channel public keys and basepoints
fn pubkeys(&self) -> &ChannelPublicKeys;
/// Gets arbitrary identifiers describing the set of keys which are provided back to you in
Expand All @@ -217,6 +228,7 @@ pub trait ChannelKeys : Send+Clone {
/// Create a signature for a local commitment transaction. This will only ever be called with
/// the same local_commitment_tx (or a copy thereof), though there are currently no guarantees
/// that it will not be called multiple times.
/// An external signer implementation should check that the commitment has not been revoked.
//
// TODO: Document the things someone using this interface should enforce before signing.
// TODO: Add more input vars to enable better checking (preferably removing commitment_tx and
Expand Down Expand Up @@ -405,7 +417,12 @@ impl InMemoryChannelKeys {
}

impl ChannelKeys for InMemoryChannelKeys {
fn commitment_secret(&self, idx: u64) -> [u8; 32] {
fn get_per_commitment_point<T: secp256k1::Signing + secp256k1::Verification>(&self, idx: u64, secp_ctx: &Secp256k1<T>) -> PublicKey {
let commitment_secret = SecretKey::from_slice(&chan_utils::build_commitment_secret(&self.commitment_seed, idx)).unwrap();
PublicKey::from_secret_key(secp_ctx, &commitment_secret)
}

fn release_commitment_secret(&self, idx: u64) -> [u8; 32] {
chan_utils::build_commitment_secret(&self.commitment_seed, idx)
}

Expand Down
4 changes: 2 additions & 2 deletions lightning/src/ln/chan_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,8 +497,8 @@ pub fn build_htlc_transaction(prev_hash: &Txid, feerate_per_kw: u32, to_self_del

#[derive(Clone)]
/// We use this to track local commitment transactions and put off signing them until we are ready
/// to broadcast. Eventually this will require a signer which is possibly external, but for now we
/// just pass in the SecretKeys required.
/// to broadcast. This class can be used inside a signer implementation to generate a signature
/// given the relevant secret key.
pub struct LocalCommitmentTransaction {
// TODO: We should migrate away from providing the transaction, instead providing enough to
// allow the ChannelKeys to construct it from scratch. Luckily we already have HTLC data here,
Expand Down
42 changes: 17 additions & 25 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -785,13 +785,6 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
Ok(chan)
}

// Utilities to derive keys:

fn build_local_commitment_secret(&self, idx: u64) -> SecretKey {
let res = self.local_keys.commitment_secret(idx);
SecretKey::from_slice(&res).unwrap()
}

// Utilities to build transactions:

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

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

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

fn get_last_revoke_and_ack(&self) -> msgs::RevokeAndACK {
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &self.build_local_commitment_secret(self.cur_local_commitment_transaction_number));
let per_commitment_secret = self.local_keys.commitment_secret(self.cur_local_commitment_transaction_number + 2);
let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
let per_commitment_secret = self.local_keys.release_commitment_secret(self.cur_local_commitment_transaction_number + 2);
msgs::RevokeAndACK {
channel_id: self.channel_id,
per_commitment_secret,
Expand Down Expand Up @@ -2751,7 +2743,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
if msg.next_remote_commitment_number > 0 {
match msg.data_loss_protect {
OptionalField::Present(ref data_loss) => {
if self.local_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1) != data_loss.your_last_per_commitment_secret {
let expected_point = self.local_keys.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1, &self.secp_ctx);
let given_secret = SecretKey::from_slice(&data_loss.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.secp_ctx, &given_secret) {
return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided".to_owned()));
}
if msg.next_remote_commitment_number > INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number {
Expand Down Expand Up @@ -2787,8 +2782,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
}

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

let resend_funding_locked = if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number == 1 {
// We should never have to worry about MonitorUpdateFailed resending FundingLocked
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
Some(msgs::FundingLocked {
channel_id: self.channel_id(),
next_per_commitment_point: next_per_commitment_point,
Expand Down Expand Up @@ -3405,8 +3398,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
//a protocol oversight, but I assume I'm just missing something.
if need_commitment_update {
if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 {
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
return Ok((Some(msgs::FundingLocked {
channel_id: self.channel_id,
next_per_commitment_point: next_per_commitment_point,
Expand Down Expand Up @@ -3457,7 +3449,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
panic!("Tried to send an open_channel for a channel that has already advanced");
}

let local_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
let first_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
let local_keys = self.local_keys.pubkeys();

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

let local_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
let first_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
let local_keys = self.local_keys.pubkeys();

msgs::AcceptChannel {
Expand All @@ -3511,7 +3503,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
payment_point: local_keys.payment_point,
delayed_payment_basepoint: local_keys.delayed_payment_basepoint,
htlc_basepoint: local_keys.htlc_basepoint,
first_per_commitment_point: PublicKey::from_secret_key(&self.secp_ctx, &local_commitment_secret),
first_per_commitment_point,
shutdown_scriptpubkey: OptionalField::Present(if self.config.commit_upfront_shutdown_pubkey { self.get_closing_scriptpubkey() } else { Builder::new().into_script() })
}
}
Expand Down
8 changes: 4 additions & 4 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1612,15 +1612,15 @@ fn test_fee_spike_violation_fails_htlc() {
let chan_keys = local_chan.get_local_keys();
let pubkeys = chan_keys.pubkeys();
(pubkeys.revocation_basepoint, pubkeys.htlc_basepoint, pubkeys.payment_point,
chan_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER), chan_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER - 2))
chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER), chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2))
};
let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_payment_point, remote_secret1) = {
let chan_lock = nodes[1].node.channel_state.lock().unwrap();
let remote_chan = chan_lock.by_id.get(&chan.2).unwrap();
let chan_keys = remote_chan.get_local_keys();
let pubkeys = chan_keys.pubkeys();
(pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint, pubkeys.payment_point,
chan_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER - 1))
chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 1))
};

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

nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(),
&msgs::RevokeAndACK { channel_id, per_commitment_secret, next_per_commitment_point });
Expand Down
12 changes: 11 additions & 1 deletion lightning/src/util/enforcing_trait_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,15 @@ impl EnforcingChannelKeys {
}

impl ChannelKeys for EnforcingChannelKeys {
fn commitment_secret(&self, idx: u64) -> [u8; 32] { self.inner.commitment_secret(idx) }
fn get_per_commitment_point<T: secp256k1::Signing + secp256k1::Verification>(&self, idx: u64, secp_ctx: &Secp256k1<T>) -> PublicKey {
self.inner.get_per_commitment_point(idx, secp_ctx)
}

fn release_commitment_secret(&self, idx: u64) -> [u8; 32] {
// TODO: enforce the ChannelKeys contract - error here if we already signed this commitment
self.inner.release_commitment_secret(idx)
}

fn pubkeys(&self) -> &ChannelPublicKeys { self.inner.pubkeys() }
fn key_derivation_params(&self) -> (u64, u64) { self.inner.key_derivation_params() }

Expand All @@ -71,6 +79,8 @@ impl ChannelKeys for EnforcingChannelKeys {
}

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

Expand Down