Skip to content

Provide additional parameters to sign_remote_commitment #442

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

Closed
wants to merge 6 commits into from
Closed

Provide additional parameters to sign_remote_commitment #442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bee53fa
Provide redeem scripts to sign_remote_commitment
devrandom Jan 8, 2020
164af7f
Provide remote per-commitment point to sign_remote_commitment
devrandom Jan 8, 2020
ffba0d3
comments
devrandom Jan 9, 2020
78c17d5
more checking
devrandom Jan 13, 2020
0a99bd9
refactor output sorting
devrandom Jan 15, 2020
81afee9
move expensive checks to enforcing trait
devrandom Jan 15, 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
20 changes: 18 additions & 2 deletions lightning/src/chain/keysinterface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,13 +139,27 @@ pub trait ChannelKeys : Send {
///
/// Note that if signing fails or is rejected, the channel will be force-closed.
///
/// The commitment_tx follows BOLT-3 lexicographical output ordering and has a single input.
///
/// The redeem_scripts vector is 1-1 mapped to commitment_tx outputs. For p2wpkh, the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Then it should probably be a vec containing pairs, not two vecs.

Copy link
Member Author

Choose a reason for hiding this comment

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

The outputs vec is buried inside the Transaction object, so I assume this has to wait until we move to tx construction in the signer.

Copy link
Member Author

@devrandom devrandom Jan 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Unless you're thinking of moving the redeem script vec into Transaction?

/// redeem script should be empty.
///
/// 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>(&self, channel_value_satoshis: u64, channel_funding_redeemscript: &Script, feerate_per_kw: u64, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], to_self_delay: u16, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()>;
fn sign_remote_commitment<T: secp256k1::Signing>(&self, channel_value_satoshis: u64,
channel_funding_redeemscript: &Script,
feerate_per_kw: u64, commitment_tx: &Transaction,
keys: &TxCreationKeys,
htlcs: &[&HTLCOutputInCommitment],
to_self_delay: u16, secp_ctx: &Secp256k1<T>,
redeem_scripts: &Vec<Script>,
remote_per_commitment_point: &PublicKey) -> Result<(Signature, Vec<Signature>), ()>;

/// Create a signature for a (proposed) closing transaction.
///
/// The closing_tx follows BOLT-3 lexicographical output ordering and has a single input.
///
/// Note that, due to rounding, there may be one "missing" satoshi, and either party may have
/// chosen to forgo their output as dust.
fn sign_closing_transaction<T: secp256k1::Signing>(&self, channel_value_satoshis: u64, channel_funding_redeemscript: &Script, closing_tx: &Transaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;
Expand Down Expand Up @@ -184,8 +198,10 @@ impl ChannelKeys for InMemoryChannelKeys {
fn htlc_base_key(&self) -> &SecretKey { &self.htlc_base_key }
fn commitment_seed(&self) -> &[u8; 32] { &self.commitment_seed }

fn sign_remote_commitment<T: secp256k1::Signing>(&self, channel_value_satoshis: u64, channel_funding_redeemscript: &Script, feerate_per_kw: u64, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], to_self_delay: u16, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()> {
fn sign_remote_commitment<T: secp256k1::Signing>(&self, channel_value_satoshis: u64, channel_funding_redeemscript: &Script, feerate_per_kw: u64, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], to_self_delay: u16, secp_ctx: &Secp256k1<T>, redeem_scripts: &Vec<Script>, remote_per_commitment_point: &PublicKey) -> Result<(Signature, Vec<Signature>), ()> {
if commitment_tx.input.len() != 1 { return Err(()); }
if commitment_tx.output.len() != redeem_scripts.len() { return Err(()); }

let commitment_sighash = hash_to_message!(&bip143::SighashComponents::new(&commitment_tx).sighash_all(&commitment_tx.input[0], &channel_funding_redeemscript, channel_value_satoshis)[..]);
let commitment_sig = secp_ctx.sign(&commitment_sighash, &self.funding_key);

Expand Down
65 changes: 39 additions & 26 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -800,7 +800,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
/// Note that below-dust HTLCs are included in the third return value, but not the second, and
/// sources are provided only for outbound HTLCs in the third return value.
#[inline]
fn build_commitment_transaction(&self, commitment_number: u64, keys: &TxCreationKeys, local: bool, generated_by_local: bool, feerate_per_kw: u64) -> (Transaction, usize, Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>) {
fn build_commitment_transaction(&self, commitment_number: u64, keys: &TxCreationKeys, local: bool, generated_by_local: bool, feerate_per_kw: u64) -> (Transaction, usize, Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>, Vec<Script>) {
let obscured_commitment_transaction_number = self.get_commitment_transaction_number_obscure_factor() ^ (INITIAL_COMMITMENT_NUMBER - commitment_number);

let txins = {
Expand All @@ -814,7 +814,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
ins
};

let mut txouts: Vec<(TxOut, Option<(HTLCOutputInCommitment, Option<&HTLCSource>)>)> = Vec::with_capacity(self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len() + 2);
let mut txouts: Vec<(TxOut, (Option<(HTLCOutputInCommitment, Option<&HTLCSource>)>, Script))> =
Vec::with_capacity(self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len() + 2);
let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new();

let dust_limit_satoshis = if local { self.our_dust_limit_satoshis } else { self.their_dust_limit_satoshis };
Expand Down Expand Up @@ -842,10 +843,11 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
let htlc_in_tx = get_htlc_in_commitment!($htlc, true);
if $htlc.amount_msat / 1000 >= dust_limit_satoshis + (feerate_per_kw * HTLC_TIMEOUT_TX_WEIGHT / 1000) {
log_trace!(self, " ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, log_bytes!($htlc.payment_hash.0), $htlc.amount_msat);
let script = chan_utils::get_htlc_redeemscript(&htlc_in_tx, &keys);
txouts.push((TxOut {
script_pubkey: chan_utils::get_htlc_redeemscript(&htlc_in_tx, &keys).to_v0_p2wsh(),
script_pubkey: script.to_v0_p2wsh(),
value: $htlc.amount_msat / 1000
}, Some((htlc_in_tx, $source))));
}, (Some((htlc_in_tx, $source)), script)));
} else {
log_trace!(self, " ...including {} {} dust HTLC {} (hash {}) with value {} due to dust limit", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, log_bytes!($htlc.payment_hash.0), $htlc.amount_msat);
included_dust_htlcs.push((htlc_in_tx, $source));
Expand All @@ -854,10 +856,11 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
let htlc_in_tx = get_htlc_in_commitment!($htlc, false);
if $htlc.amount_msat / 1000 >= dust_limit_satoshis + (feerate_per_kw * HTLC_SUCCESS_TX_WEIGHT / 1000) {
log_trace!(self, " ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, log_bytes!($htlc.payment_hash.0), $htlc.amount_msat);
let script = chan_utils::get_htlc_redeemscript(&htlc_in_tx, &keys);
txouts.push((TxOut { // "received HTLC output"
script_pubkey: chan_utils::get_htlc_redeemscript(&htlc_in_tx, &keys).to_v0_p2wsh(),
script_pubkey: script.to_v0_p2wsh(),
value: $htlc.amount_msat / 1000
}, Some((htlc_in_tx, $source))));
}, (Some((htlc_in_tx, $source)), script)));
} else {
log_trace!(self, " ...including {} {} dust HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, log_bytes!($htlc.payment_hash.0), $htlc.amount_msat);
included_dust_htlcs.push((htlc_in_tx, $source));
Expand Down Expand Up @@ -925,7 +928,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
assert!(value_to_self_msat >= 0);
// Note that in case they have several just-awaiting-last-RAA fulfills in-progress (ie
// AwaitingRemoteRevokeToRemove or AwaitingRemovedRemoteRevoke) we may have allowed them to
// "violate" their reserve value by couting those against it. Thus, we have to convert
// "violate" their reserve value by counting those against it. Thus, we have to convert
// everything to i64 before subtracting as otherwise we can overflow.
let value_to_remote_msat: i64 = (self.channel_value_satoshis * 1000) as i64 - (self.value_to_self_msat as i64) - (remote_htlc_total_msat as i64) - value_to_self_msat_offset;
assert!(value_to_remote_msat >= 0);
Expand Down Expand Up @@ -957,25 +960,29 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {

if value_to_a >= (dust_limit_satoshis as i64) {
log_trace!(self, " ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a);
let script = chan_utils::get_revokeable_redeemscript(&keys.revocation_key,
if local { self.their_to_self_delay } else { self.our_to_self_delay },
&keys.a_delayed_payment_key);
txouts.push((TxOut {
script_pubkey: chan_utils::get_revokeable_redeemscript(&keys.revocation_key,
if local { self.their_to_self_delay } else { self.our_to_self_delay },
&keys.a_delayed_payment_key).to_v0_p2wsh(),
script_pubkey: script.to_v0_p2wsh(),
value: value_to_a as u64
}, None));
}, (None, script)));
}

if value_to_b >= (dust_limit_satoshis as i64) {
log_trace!(self, " ...including {} output with value {}", if local { "to_remote" } else { "to_local" }, value_to_b);
let script = Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Looks like moving script to here is used, so needless hunk.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps I'm missing something because Rust is new to me.

I pulled the argument out into a variable for readability. Are you saying that it's more efficient to have the expression inline in the next line? I did an experiment as follows, and got identical compiler output with rustc -O:

#[derive(Debug)]
pub struct Obj(u8);

fn bla() -> Obj {
    return Obj(5);
}

fn main() {
    let script = bla();
    println!("hello world, {:?}", script);
//    commenting the above and uncommenting below results in the same compiler output
//    println!("hello world, {:?}", bla());
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, sorry, not commenting about effeciency at all here, just noting that the hunk isn't doing anything. I usually point out such things as its otherwise awkward in the same commit and I didn't automatically assume it was for readability. Doesn't matter too much either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, so readability improvements in separate commits, so the intention is more transparent? or attach a PR note to the hunk?

.push_slice(&Hash160::hash(&keys.b_payment_key.serialize())[..])
.into_script();
// p2wpkh doesn't need a redeem script. Provide a placeholder.
let redeem_script = Builder::new().into_script();
txouts.push((TxOut {
script_pubkey: Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0)
.push_slice(&Hash160::hash(&keys.b_payment_key.serialize())[..])
.into_script(),
script_pubkey: script,
value: value_to_b as u64
}, None));
}, (None, redeem_script)));
}

transaction_utils::sort_outputs(&mut txouts, |a, b| {
transaction_utils::sort_outputs(&mut txouts, |(a, _), (b, _)| {
if let &Some(ref a_htlc) = a {
if let &Some(ref b_htlc) = b {
a_htlc.0.cltv_expiry.cmp(&b_htlc.0.cltv_expiry)
Expand All @@ -990,10 +997,12 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
});

let mut outputs: Vec<TxOut> = Vec::with_capacity(txouts.len());
let mut scripts: Vec<Script> = Vec::with_capacity(txouts.len());
let mut htlcs_included: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(txouts.len() + included_dust_htlcs.len());
for (idx, mut out) in txouts.drain(..).enumerate() {
outputs.push(out.0);
if let Some((mut htlc, source_option)) = out.1.take() {
for (idx, (out, (mut htlc_data, script))) in txouts.drain(..).enumerate() {
outputs.push(out);
scripts.push(script);
if let Some((mut htlc, source_option)) = htlc_data.take() {
htlc.transaction_output_index = Some(idx as u32);
htlcs_included.push((htlc, source_option));
}
Expand All @@ -1006,7 +1015,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
lock_time: ((0x20 as u32) << 8*3) | ((obscured_commitment_transaction_number & 0xffffffu64) as u32),
input: txins,
output: outputs,
}, non_dust_htlc_count, htlcs_included)
}, non_dust_htlc_count, htlcs_included, scripts)
}

#[inline]
Expand Down Expand Up @@ -1423,8 +1432,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
let localtx = LocalCommitmentTransaction::new_missing_local_sig(local_initial_commitment_tx, sig, &PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.funding_key()), self.their_funding_pubkey.as_ref().unwrap());

let remote_keys = self.build_remote_transaction_keys()?;
let remote_initial_commitment_tx = self.build_commitment_transaction(self.cur_remote_commitment_transaction_number, &remote_keys, false, false, self.feerate_per_kw).0;
let remote_signature = self.local_keys.sign_remote_commitment(self.channel_value_satoshis, &self.get_funding_redeemscript(), self.feerate_per_kw, &remote_initial_commitment_tx, &remote_keys, &Vec::new(), self.our_to_self_delay, &self.secp_ctx)
let commitment_tx = self.build_commitment_transaction(self.cur_remote_commitment_transaction_number, &remote_keys, false, false, self.feerate_per_kw);
let remote_initial_commitment_tx = commitment_tx.0;
let scripts = commitment_tx.3;
let remote_signature = self.local_keys.sign_remote_commitment(self.channel_value_satoshis, &self.get_funding_redeemscript(), self.feerate_per_kw, &remote_initial_commitment_tx, &remote_keys, &Vec::new(), self.our_to_self_delay, &self.secp_ctx, &scripts, &self.their_cur_commitment_point.unwrap())
.map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed"))?.0;

// We sign the "remote" commitment transaction, allowing them to broadcast the tx if they wish.
Expand Down Expand Up @@ -1743,7 +1754,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
let mut local_commitment_tx = {
let mut commitment_tx = self.build_commitment_transaction(self.cur_local_commitment_transaction_number, &local_keys, true, false, feerate_per_kw);
let htlcs_cloned: Vec<_> = commitment_tx.2.drain(..).map(|htlc| (htlc.0, htlc.1.map(|h| h.clone()))).collect();
(commitment_tx.0, commitment_tx.1, htlcs_cloned)
(commitment_tx.0, commitment_tx.1, htlcs_cloned, commitment_tx.3)
};
let local_commitment_txid = local_commitment_tx.0.txid();
let local_sighash = hash_to_message!(&bip143::SighashComponents::new(&local_commitment_tx.0).sighash_all(&local_commitment_tx.0.input[0], &funding_script, self.channel_value_satoshis)[..]);
Expand Down Expand Up @@ -3149,8 +3160,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
/// If an Err is returned, it is a ChannelError::Close (for get_outbound_funding_created)
fn get_outbound_funding_created_signature(&mut self) -> Result<(Signature, Transaction), ChannelError> {
let remote_keys = self.build_remote_transaction_keys()?;
let remote_initial_commitment_tx = self.build_commitment_transaction(self.cur_remote_commitment_transaction_number, &remote_keys, false, false, self.feerate_per_kw).0;
Ok((self.local_keys.sign_remote_commitment(self.channel_value_satoshis, &self.get_funding_redeemscript(), self.feerate_per_kw, &remote_initial_commitment_tx, &remote_keys, &Vec::new(), self.our_to_self_delay, &self.secp_ctx)
let commitment_tx = self.build_commitment_transaction(self.cur_remote_commitment_transaction_number, &remote_keys, false, false, self.feerate_per_kw);
let remote_initial_commitment_tx = commitment_tx.0;
let scripts = commitment_tx.3;
Ok((self.local_keys.sign_remote_commitment(self.channel_value_satoshis, &self.get_funding_redeemscript(), self.feerate_per_kw, &remote_initial_commitment_tx, &remote_keys, &Vec::new(), self.our_to_self_delay, &self.secp_ctx, &scripts, &self.their_cur_commitment_point.unwrap())
.map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed"))?.0, remote_initial_commitment_tx))
}

Expand Down Expand Up @@ -3458,7 +3471,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
htlcs.push(htlc);
}

let res = self.local_keys.sign_remote_commitment(self.channel_value_satoshis, &self.get_funding_redeemscript(), feerate_per_kw, &remote_commitment_tx.0, &remote_keys, &htlcs, self.our_to_self_delay, &self.secp_ctx)
let res = self.local_keys.sign_remote_commitment(self.channel_value_satoshis, &self.get_funding_redeemscript(), feerate_per_kw, &remote_commitment_tx.0, &remote_keys, &htlcs, self.our_to_self_delay, &self.secp_ctx, &remote_commitment_tx.3, &self.their_cur_commitment_point.unwrap())
.map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed"))?;
signature = res.0;
htlc_signatures = res.1;
Expand Down
20 changes: 17 additions & 3 deletions lightning/src/util/enforcing_trait_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use bitcoin::blockdata::transaction::Transaction;
use bitcoin::blockdata::script::Script;

use secp256k1;
use secp256k1::key::SecretKey;
use secp256k1::key::{SecretKey, PublicKey};
use secp256k1::{Secp256k1, Signature};

/// Enforces some rules on ChannelKeys calls. Eventually we will probably want to expose a variant
Expand All @@ -35,8 +35,22 @@ impl ChannelKeys for EnforcingChannelKeys {
fn htlc_base_key(&self) -> &SecretKey { self.inner.htlc_base_key() }
fn commitment_seed(&self) -> &[u8; 32] { self.inner.commitment_seed() }

fn sign_remote_commitment<T: secp256k1::Signing>(&self, channel_value_satoshis: u64, channel_funding_script: &Script, feerate_per_kw: u64, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], to_self_delay: u16, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()> {
fn sign_remote_commitment<T: secp256k1::Signing>(&self, channel_value_satoshis: u64, channel_funding_script: &Script, feerate_per_kw: u64, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], to_self_delay: u16, secp_ctx: &Secp256k1<T>, redeem_scripts: &Vec<Script>, remote_per_commitment_point: &PublicKey) -> Result<(Signature, Vec<Signature>), ()> {
if commitment_tx.input.len() != 1 { panic!(); }
if commitment_tx.output.len() != redeem_scripts.len() { panic!(); }

for (out, redeem_script) in commitment_tx.output.iter().zip(redeem_scripts.iter()) {
if out.script_pubkey.is_v0_p2wpkh() {
if !redeem_script.is_empty() {
return Err(())
}
} else {
if out.script_pubkey != redeem_script.to_v0_p2wsh() {
return Err(())
}
}
}

let obscured_commitment_transaction_number = (commitment_tx.lock_time & 0xffffff) as u64 | ((commitment_tx.input[0].sequence as u64 & 0xffffff) << 3*8);

{
Expand All @@ -49,7 +63,7 @@ impl ChannelKeys for EnforcingChannelKeys {
commitment_data.1 = cmp::max(commitment_number, commitment_data.1)
}

Ok(self.inner.sign_remote_commitment(channel_value_satoshis, channel_funding_script, feerate_per_kw, commitment_tx, keys, htlcs, to_self_delay, secp_ctx).unwrap())
Ok(self.inner.sign_remote_commitment(channel_value_satoshis, channel_funding_script, feerate_per_kw, commitment_tx, keys, htlcs, to_self_delay, secp_ctx, redeem_scripts, remote_per_commitment_point).unwrap())
}

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