Skip to content

Clean up ChannelKeys API #598

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 7 commits into from
Apr 25, 2020
Merged

Clean up ChannelKeys API #598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
359b3d5
Dry-up InputMaterial::Funding
Apr 21, 2020
ddd85fb
Track signing of local txn in channelmonitor and refuse updates
TheBlueMatt Apr 19, 2020
bf74bb6
Return Result<Signature> instead of modifying args in ChannelKeys
TheBlueMatt Apr 19, 2020
7159d15
Batch-sign local HTLC txn with a well-doc'd API, returning sigs
TheBlueMatt Apr 20, 2020
29199fa
Don't modify LocalCommitmemntTransaction after construction
TheBlueMatt Apr 23, 2020
92c0698
Remove TODOs from documentation in keysinterface
TheBlueMatt Apr 20, 2020
03316cd
Remove trivial assertions in LocalCommitmentTransaction fns
TheBlueMatt Apr 24, 2020
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
75 changes: 42 additions & 33 deletions lightning/src/chain/keysinterface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ use util::ser::{Writeable, Writer, Readable};

use ln::chan_utils;
use ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment, make_funding_redeemscript, ChannelPublicKeys, LocalCommitmentTransaction};
use ln::channelmanager::PaymentPreimage;
use ln::msgs;

use std::sync::Arc;
Expand Down Expand Up @@ -185,11 +184,11 @@ pub trait KeysInterface: Send + Sync {
/// Readable/Writable to serialize out a unique reference to this set of keys so
/// that you can serialize the full ChannelManager object.
///
/// (TODO: We shouldn't require that, and should have an API to get them at deser time, due mostly
/// to the possibility of reentrancy issues by calling the user's code during our deserialization
/// routine).
/// 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.
// (TODO: We shouldn't require that, and should have an API to get them at deser time, due mostly
// to the possibility of reentrancy issues by calling the user's code during our deserialization
// routine).
// 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 private key for the anchor tx
fn funding_key<'a>(&'a self) -> &'a SecretKey;
Expand All @@ -210,32 +209,42 @@ pub trait ChannelKeys : Send+Clone {
/// Create a signature for a remote commitment transaction and associated HTLC transactions.
///
/// Note that if signing fails or is rejected, the channel will be force-closed.
///
/// 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
/// making the callee generate it via some util function we expose)!
//
// 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
// making the callee generate it via some util function we expose)!
fn sign_remote_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, feerate_per_kw: u64, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], to_self_delay: u16, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()>;

/// Create a signature for a local commitment transaction
///
/// 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
/// TODO: Ensure test-only version doesn't enforce uniqueness of signature when it's enforced in this method
/// making the callee generate it via some util function we expose)!
fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
/// 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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I think that's good to say you may want to resign multiples times, even the same commitment transaction, like if you do a CPFP, you may want to verify every time the transaction chain back to the funding_outpoint, and therefore redoing the whole signing process.

//
// 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
fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;

/// Same as sign_local_commitment, but exists only for tests to get access to local commitment
/// transactions which will be broadcasted later, after the channel has moved on to a newer
/// state. Thus, needs its own method as sign_local_commitment may enforce that we only ever
/// get called once.
#[cfg(test)]
fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;

/// Create a signature for a local commitment transaction without enforcing one-time signing.
/// Create a signature for each HTLC transaction spending a local commitment transaction.
///
/// Testing revocation logic by our test framework needs to sign multiple local commitment
/// transactions. This unsafe test-only version doesn't enforce one-time signing security
/// requirement.
#[cfg(test)]
fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
/// Unlike sign_local_commitment, this may be called multiple times with *different*
/// local_commitment_tx values. While this will never be called with a revoked
/// local_commitment_tx, it is possible that it is called with the second-latest
/// local_commitment_tx (only if we haven't yet revoked it) if some watchtower/secondary
/// ChannelMonitor decided to broadcast before it had been updated to the latest.
///
/// Either an Err should be returned, or a Vec with one entry for each HTLC which exists in
/// local_commitment_tx. For those HTLCs which have transaction_output_index set to None
/// (implying they were considered dust at the time the commitment transaction was negotiated),
/// a corresponding None should be included in the return value. All other positions in the
/// return value must contain a signature.
fn sign_local_commitment_htlc_transactions<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, local_csv: u16, secp_ctx: &Secp256k1<T>) -> Result<Vec<Option<Signature>>, ()>;

/// Signs a transaction created by build_htlc_transaction. If the transaction is an
/// HTLC-Success transaction, preimage must be set!
/// TODO: should be merged with sign_local_commitment as a slice of HTLC transactions to sign
fn sign_htlc_transaction<T: secp256k1::Signing>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>);
/// Create a signature for a (proposed) closing transaction.
///
/// Note that, due to rounding, there may be one "missing" satoshi, and either party may have
Expand Down Expand Up @@ -363,25 +372,25 @@ impl ChannelKeys for InMemoryChannelKeys {
Ok((commitment_sig, htlc_sigs))
}

fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);

local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
Ok(local_commitment_tx.get_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx))
}

#[cfg(test)]
fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);

local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
Ok(local_commitment_tx.get_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx))
}

fn sign_htlc_transaction<T: secp256k1::Signing>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {
local_commitment_tx.add_htlc_sig(&self.htlc_base_key, htlc_index, preimage, local_csv, secp_ctx);
fn sign_local_commitment_htlc_transactions<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, local_csv: u16, secp_ctx: &Secp256k1<T>) -> Result<Vec<Option<Signature>>, ()> {
local_commitment_tx.get_htlc_sigs(&self.htlc_base_key, local_csv, secp_ctx)
}

fn sign_closing_transaction<T: secp256k1::Signing>(&self, closing_tx: &Transaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
Expand Down
Loading