Skip to content

Split parsing and transaction management for local transactions between Chanmon/Onchain #559

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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
36 changes: 35 additions & 1 deletion lightning/src/chain/keysinterface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ use util::logger::Logger;
use util::ser::{Writeable, Writer, Readable};

use ln::chan_utils;
use ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment, make_funding_redeemscript, ChannelPublicKeys};
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 @@ -215,6 +216,26 @@ pub trait ChannelKeys : Send+Clone {
/// 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, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
Copy link
Member

Choose a reason for hiding this comment

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

I think a pattern of returning a Result<Signature, Error> (rather than mutating the tx) is a cleaner API:

  • is a pure function, so easy to see that it doesn't have any unexpected side-effects
  • allows return of an error result. for example, see here where an error may happen and we don't want to fail silently. In the future, there may be other failures we want to communicate, related to policy checks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. Sadly, this PR has gotten large, and kinda leaves a few of the API bits around signing logic to future PRs. See #567 for the tracking issue.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your input, I agree we have room to improve the ChannelKeys API. I would rather do it in follow-up PRs, at first this one was scoped to move generation of local transactions in one place (OnchainTxHandler). Extending ChannelKeys to sign local was just an easy way to avoid breaking signature during the move.

I referenced your comments in #567 to be sure to not forget them.


/// Create a signature for a local commitment transaction without enforcing one-time signing.
///
/// 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, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);

/// 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 @@ -342,6 +363,19 @@ impl ChannelKeys for InMemoryChannelKeys {
Ok((commitment_sig, htlc_sigs))
}

fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
}

#[cfg(test)]
fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, 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_closing_transaction<T: secp256k1::Signing>(&self, closing_tx: &Transaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
if closing_tx.input.len() != 1 { return Err(()); }
if closing_tx.input[0].witness.len() != 0 { return Err(()); }
Expand Down
180 changes: 134 additions & 46 deletions lightning/src/ln/chan_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ use secp256k1::key::{SecretKey, PublicKey};
use secp256k1::{Secp256k1, Signature};
use secp256k1;

use std::{cmp, mem};

const MAX_ALLOC_SIZE: usize = 64*1024;

pub(super) const HTLC_SUCCESS_TX_WEIGHT: u64 = 703;
pub(super) const HTLC_TIMEOUT_TX_WEIGHT: u64 = 663;

Expand Down Expand Up @@ -355,7 +359,7 @@ impl_writeable!(HTLCOutputInCommitment, 1 + 8 + 4 + 32 + 5, {
});

#[inline]
pub(super) fn get_htlc_redeemscript_with_explicit_keys(htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey) -> Script {
pub(crate) fn get_htlc_redeemscript_with_explicit_keys(htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey) -> Script {
let payment_hash160 = Ripemd160::hash(&htlc.payment_hash.0[..]).into_inner();
if htlc.offered {
Builder::new().push_opcode(opcodes::all::OP_DUP)
Expand Down Expand Up @@ -475,62 +479,44 @@ pub fn build_htlc_transaction(prev_hash: &Sha256dHash, feerate_per_kw: u64, to_s
}
}

/// Signs a transaction created by build_htlc_transaction. If the transaction is an
/// HTLC-Success transaction (ie htlc.offered is false), preimage must be set!
pub(crate) fn sign_htlc_transaction<T: secp256k1::Signing>(tx: &mut Transaction, their_sig: &Signature, preimage: &Option<PaymentPreimage>, htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey, per_commitment_point: &PublicKey, htlc_base_key: &SecretKey, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Script), ()> {
if tx.input.len() != 1 { return Err(()); }
if tx.input[0].witness.len() != 0 { return Err(()); }

let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&htlc, a_htlc_key, b_htlc_key, revocation_key);

let our_htlc_key = derive_private_key(secp_ctx, per_commitment_point, htlc_base_key).map_err(|_| ())?;
let sighash = hash_to_message!(&bip143::SighashComponents::new(&tx).sighash_all(&tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]);
let local_tx = PublicKey::from_secret_key(&secp_ctx, &our_htlc_key) == *a_htlc_key;
let our_sig = secp_ctx.sign(&sighash, &our_htlc_key);

tx.input[0].witness.push(Vec::new()); // First is the multisig dummy

if local_tx { // b, then a
tx.input[0].witness.push(their_sig.serialize_der().to_vec());
tx.input[0].witness.push(our_sig.serialize_der().to_vec());
} else {
tx.input[0].witness.push(our_sig.serialize_der().to_vec());
tx.input[0].witness.push(their_sig.serialize_der().to_vec());
}
tx.input[0].witness[1].push(SigHashType::All as u8);
tx.input[0].witness[2].push(SigHashType::All as u8);

if htlc.offered {
tx.input[0].witness.push(Vec::new());
assert!(preimage.is_none());
} else {
tx.input[0].witness.push(preimage.unwrap().0.to_vec());
}

tx.input[0].witness.push(htlc_redeemscript.as_bytes().to_vec());

Ok((our_sig, htlc_redeemscript))
}

#[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.
pub(crate) struct LocalCommitmentTransaction {
tx: Transaction
pub struct LocalCommitmentTransaction {
tx: Transaction,
//TODO: modify Channel methods to integrate HTLC material at LocalCommitmentTransaction generation to drop Option here
local_keys: Option<TxCreationKeys>,
feerate_per_kw: Option<u64>,
per_htlc: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>
}
impl LocalCommitmentTransaction {
#[cfg(test)]
pub fn dummy() -> Self {
let dummy_input = TxIn {
previous_output: OutPoint {
txid: Default::default(),
vout: 0,
},
script_sig: Default::default(),
sequence: 0,
witness: vec![vec![], vec![], vec![]]
};
Self { tx: Transaction {
version: 2,
input: Vec::new(),
input: vec![dummy_input],
output: Vec::new(),
lock_time: 0,
} }
},
local_keys: None,
feerate_per_kw: None,
per_htlc: Vec::new()
}
}

pub fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey) -> LocalCommitmentTransaction {
/// Generate a new LocalCommitmentTransaction based on a raw commitment transaction,
/// remote signature and both parties keys
pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey) -> LocalCommitmentTransaction {
if tx.input.len() != 1 { panic!("Tried to store a commitment transaction that had input count != 1!"); }
if tx.input[0].witness.len() != 0 { panic!("Tried to store a signed commitment transaction?"); }

Expand All @@ -546,13 +532,20 @@ impl LocalCommitmentTransaction {
tx.input[0].witness.push(Vec::new());
}

Self { tx }
Self { tx,
local_keys: None,
feerate_per_kw: None,
per_htlc: Vec::new()
}
}

/// Get the txid of the local commitment transaction contained in this
/// LocalCommitmentTransaction
pub fn txid(&self) -> Sha256dHash {
self.tx.txid()
}

/// Check if LocalCommitmentTransaction has already been signed by us
pub fn has_local_sig(&self) -> bool {
if self.tx.input.len() != 1 { panic!("Commitment transactions must have input count == 1!"); }
if self.tx.input[0].witness.len() == 4 {
Expand All @@ -567,6 +560,15 @@ impl LocalCommitmentTransaction {
}
}

/// Add local signature for LocalCommitmentTransaction, do nothing if signature is already
/// present
///
/// Funding key is your key included in the 2-2 funding_outpoint lock. Should be provided
/// by your ChannelKeys.
/// Funding redeemscript is script locking funding_outpoint. This is the mutlsig script
/// between your own funding key and your counterparty's. Currently, this is provided in
/// ChannelKeys::sign_local_commitment() calls directly.
/// Channel value is amount locked in funding_outpoint.
pub fn add_local_sig<T: secp256k1::Signing>(&mut self, funding_key: &SecretKey, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
if self.has_local_sig() { return; }
let sighash = hash_to_message!(&bip143::SighashComponents::new(&self.tx)
Expand All @@ -584,11 +586,74 @@ impl LocalCommitmentTransaction {
self.tx.input[0].witness.push(funding_redeemscript.as_bytes().to_vec());
}

pub fn without_valid_witness(&self) -> &Transaction { &self.tx }
/// Get raw transaction without asserting if witness is complete
pub(crate) fn without_valid_witness(&self) -> &Transaction { &self.tx }
/// Get raw transaction with panics if witness is incomplete
pub fn with_valid_witness(&self) -> &Transaction {
assert!(self.has_local_sig());
&self.tx
}

/// Set HTLC cache to generate any local HTLC transaction spending one of htlc ouput
/// from this local commitment transaction
pub(crate) fn set_htlc_cache(&mut self, local_keys: TxCreationKeys, feerate_per_kw: u64, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>) {
self.local_keys = Some(local_keys);
self.feerate_per_kw = Some(feerate_per_kw);
self.per_htlc = htlc_outputs;
}

/// Add local signature for a htlc transaction, do nothing if a cached signed transaction is
/// already present
pub fn add_htlc_sig<T: secp256k1::Signing>(&mut self, htlc_base_key: &SecretKey, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {
if self.local_keys.is_none() || self.feerate_per_kw.is_none() { return; }
let local_keys = self.local_keys.as_ref().unwrap();
let txid = self.txid();
for this_htlc in self.per_htlc.iter_mut() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oof. I didn't mean to scan the whole vec each time we want to sign something, you can keep them in htlc_index order. Again, this will get fixed as a side-effect of a better API so fine to leave it, but that hurts.

Copy link
Author

Choose a reason for hiding this comment

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

That's not a hot path given that's only when we go onchain, so good enough to wait for an API clean up :)

if this_htlc.0.transaction_output_index.unwrap() == htlc_index {
if this_htlc.2.is_some() { return; } // we already have a cached htlc transaction at provided index
let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw.unwrap(), local_csv, &this_htlc.0, &local_keys.a_delayed_payment_key, &local_keys.revocation_key);
if !this_htlc.0.offered && preimage.is_none() { return; } // if we don't have preimage for HTLC-Success, don't try to generate
let htlc_secret = if !this_htlc.0.offered { preimage } else { None }; // if we have a preimage for HTLC-Timeout, don't use it that's likely a duplicate HTLC hash
if this_htlc.1.is_none() { return; } // we don't have any remote signature for this htlc
if htlc_tx.input.len() != 1 { return; }
if htlc_tx.input[0].witness.len() != 0 { return; }

let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &local_keys.a_htlc_key, &local_keys.b_htlc_key, &local_keys.revocation_key);

if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &local_keys.per_commitment_point, htlc_base_key) {
let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, this_htlc.0.amount_msat / 1000)[..]);
let our_sig = secp_ctx.sign(&sighash, &our_htlc_key);

htlc_tx.input[0].witness.push(Vec::new()); // First is the multisig dummy

htlc_tx.input[0].witness.push(this_htlc.1.unwrap().serialize_der().to_vec());
htlc_tx.input[0].witness.push(our_sig.serialize_der().to_vec());
htlc_tx.input[0].witness[1].push(SigHashType::All as u8);
htlc_tx.input[0].witness[2].push(SigHashType::All as u8);

if this_htlc.0.offered {
htlc_tx.input[0].witness.push(Vec::new());
assert!(htlc_secret.is_none());
} else {
htlc_tx.input[0].witness.push(htlc_secret.unwrap().0.to_vec());
}

htlc_tx.input[0].witness.push(htlc_redeemscript.as_bytes().to_vec());

this_htlc.2 = Some(htlc_tx);
} else { return; }
}
}
}
/// Expose raw htlc transaction, guarante witness is complete if non-empty
pub fn htlc_with_valid_witness(&self, htlc_index: u32) -> &Option<Transaction> {
for this_htlc in self.per_htlc.iter() {
if this_htlc.0.transaction_output_index.unwrap() == htlc_index {
return &this_htlc.2;
}
}
&None
}
}
impl PartialEq for LocalCommitmentTransaction {
// We dont care whether we are signed in equality comparison
Expand All @@ -604,6 +669,14 @@ impl Writeable for LocalCommitmentTransaction {
_ => panic!("local tx must have been well-formed!"),
}
}
self.local_keys.write(writer)?;
self.feerate_per_kw.write(writer)?;
writer.write_all(&byte_utils::be64_to_array(self.per_htlc.len() as u64))?;
for &(ref htlc, ref sig, ref htlc_tx) in self.per_htlc.iter() {
htlc.write(writer)?;
sig.write(writer)?;
htlc_tx.write(writer)?;
}
Ok(())
}
}
Expand All @@ -616,12 +689,27 @@ impl Readable for LocalCommitmentTransaction {
_ => return Err(DecodeError::InvalidValue),
},
};
let local_keys = Readable::read(reader)?;
let feerate_per_kw = Readable::read(reader)?;
let htlcs_count: u64 = Readable::read(reader)?;
let mut per_htlc = Vec::with_capacity(cmp::min(htlcs_count as usize, MAX_ALLOC_SIZE / mem::size_of::<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>()));
for _ in 0..htlcs_count {
let htlc: HTLCOutputInCommitment = Readable::read(reader)?;
let sigs = Readable::read(reader)?;
let htlc_tx = Readable::read(reader)?;
per_htlc.push((htlc, sigs, htlc_tx));
}

if tx.input.len() != 1 {
// Ensure tx didn't hit the 0-input ambiguity case.
return Err(DecodeError::InvalidValue);
}
Ok(Self { tx })
Ok(Self {
tx,
local_keys,
feerate_per_kw,
per_htlc,
})
}
}

Expand Down
19 changes: 12 additions & 7 deletions lightning/src/ln/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4433,7 +4433,7 @@ mod tests {

assert_eq!(PublicKey::from_secret_key(&secp_ctx, chan_keys.funding_key()).serialize()[..],
hex::decode("023da092f6980e58d2c037173180e9a465476026ee50f96695963e8efe436f54eb").unwrap()[..]);
let keys_provider = Keys { chan_keys };
let keys_provider = Keys { chan_keys: chan_keys.clone() };

let their_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
let mut config = UserConfig::default();
Expand Down Expand Up @@ -4475,6 +4475,7 @@ mod tests {

let mut unsigned_tx: (Transaction, Vec<HTLCOutputInCommitment>);

let mut localtx;
macro_rules! test_commitment {
( $their_sig_hex: expr, $our_sig_hex: expr, $tx_hex: expr) => {
unsigned_tx = {
Expand All @@ -4489,20 +4490,20 @@ mod tests {
let sighash = Message::from_slice(&bip143::SighashComponents::new(&unsigned_tx.0).sighash_all(&unsigned_tx.0.input[0], &redeemscript, chan.channel_value_satoshis)[..]).unwrap();
secp_ctx.verify(&sighash, &their_signature, chan.their_funding_pubkey()).unwrap();

let mut localtx = LocalCommitmentTransaction::new_missing_local_sig(unsigned_tx.0.clone(), &their_signature, &PublicKey::from_secret_key(&secp_ctx, chan.local_keys.funding_key()), chan.their_funding_pubkey());
localtx.add_local_sig(chan.local_keys.funding_key(), &redeemscript, chan.channel_value_satoshis, &chan.secp_ctx);
localtx = LocalCommitmentTransaction::new_missing_local_sig(unsigned_tx.0.clone(), &their_signature, &PublicKey::from_secret_key(&secp_ctx, chan.local_keys.funding_key()), chan.their_funding_pubkey());
chan_keys.sign_local_commitment(&mut localtx, &redeemscript, chan.channel_value_satoshis, &chan.secp_ctx);

assert_eq!(serialize(localtx.with_valid_witness())[..],
hex::decode($tx_hex).unwrap()[..]);
};
}

macro_rules! test_htlc_output {
( $htlc_idx: expr, $their_sig_hex: expr, $our_sig_hex: expr, $tx_hex: expr ) => {
( $htlc_idx: expr, $their_sig_hex: expr, $our_sig_hex: expr, $tx_hex: expr) => {
let remote_signature = Signature::from_der(&hex::decode($their_sig_hex).unwrap()[..]).unwrap();

let ref htlc = unsigned_tx.1[$htlc_idx];
let mut htlc_tx = chan.build_htlc_transaction(&unsigned_tx.0.txid(), &htlc, true, &keys, chan.feerate_per_kw);
let htlc_tx = chan.build_htlc_transaction(&unsigned_tx.0.txid(), &htlc, true, &keys, chan.feerate_per_kw);
let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &keys);
let htlc_sighash = Message::from_slice(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]).unwrap();
secp_ctx.verify(&htlc_sighash, &remote_signature, &keys.b_htlc_key).unwrap();
Expand All @@ -4519,8 +4520,12 @@ mod tests {
assert!(preimage.is_some());
}

chan_utils::sign_htlc_transaction(&mut htlc_tx, &remote_signature, &preimage, &htlc, &keys.a_htlc_key, &keys.b_htlc_key, &keys.revocation_key, &keys.per_commitment_point, chan.local_keys.htlc_base_key(), &chan.secp_ctx).unwrap();
assert_eq!(serialize(&htlc_tx)[..],
let mut per_htlc = Vec::new();
per_htlc.push((htlc.clone(), Some(remote_signature), None));
localtx.set_htlc_cache(keys.clone(), chan.feerate_per_kw, per_htlc);
chan_keys.sign_htlc_transaction(&mut localtx, $htlc_idx, preimage, chan.their_to_self_delay, &chan.secp_ctx);

assert_eq!(serialize(localtx.htlc_with_valid_witness($htlc_idx).as_ref().unwrap())[..],
hex::decode($tx_hex).unwrap()[..]);
};
}
Expand Down
Loading