Skip to content

Commit 8bbb6d6

Browse files
committed
Revocation enforcement in signer
We want to make sure that we don't sign revoked transactions. Given that ChannelKeys are not singletons and revocation enforcement is stateful, we need to store the revocation state in KeysInterface.
1 parent 6539afe commit 8bbb6d6

File tree

4 files changed

+87
-24
lines changed

4 files changed

+87
-24
lines changed

lightning/src/ln/functional_tests.rs

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1617,27 +1617,26 @@ fn test_fee_spike_violation_fails_htlc() {
16171617

16181618
// Get the EnforcingChannelKeys for each channel, which will be used to (1) get the keys
16191619
// needed to sign the new commitment tx and (2) sign the new commitment tx.
1620-
let (local_revocation_basepoint, local_htlc_basepoint, local_payment_point, local_secret, local_secret2) = {
1620+
let (local_revocation_basepoint, local_htlc_basepoint, local_payment_point, local_secret, next_local_point) = {
16211621
let chan_lock = nodes[0].node.channel_state.lock().unwrap();
16221622
let local_chan = chan_lock.by_id.get(&chan.2).unwrap();
16231623
let chan_keys = local_chan.get_keys();
16241624
let pubkeys = chan_keys.pubkeys();
16251625
(pubkeys.revocation_basepoint, pubkeys.htlc_basepoint, pubkeys.payment_point,
1626-
chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER), chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2))
1626+
chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER),
1627+
chan_keys.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx))
16271628
};
1628-
let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_payment_point, remote_secret1) = {
1629+
let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_payment_point, remote_point) = {
16291630
let chan_lock = nodes[1].node.channel_state.lock().unwrap();
16301631
let remote_chan = chan_lock.by_id.get(&chan.2).unwrap();
16311632
let chan_keys = remote_chan.get_keys();
16321633
let pubkeys = chan_keys.pubkeys();
16331634
(pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint, pubkeys.payment_point,
1634-
chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 1))
1635+
chan_keys.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx))
16351636
};
16361637

16371638
// Assemble the set of keys we can use for signatures for our commitment_signed message.
1638-
let commitment_secret = SecretKey::from_slice(&remote_secret1).unwrap();
1639-
let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &commitment_secret);
1640-
let commit_tx_keys = chan_utils::TxCreationKeys::derive_new(&secp_ctx, &per_commitment_point, &remote_delayed_payment_basepoint,
1639+
let commit_tx_keys = chan_utils::TxCreationKeys::derive_new(&secp_ctx, &remote_point, &remote_delayed_payment_basepoint,
16411640
&remote_htlc_basepoint, &local_revocation_basepoint, &local_htlc_basepoint).unwrap();
16421641

16431642
// Build the remote commitment transaction so we can sign it, and then later use the
@@ -1720,10 +1719,11 @@ fn test_fee_spike_violation_fails_htlc() {
17201719
let _ = nodes[1].node.get_and_clear_pending_msg_events();
17211720

17221721
// Send the RAA to nodes[1].
1723-
let per_commitment_secret = local_secret;
1724-
let next_secret = SecretKey::from_slice(&local_secret2).unwrap();
1725-
let next_per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &next_secret);
1726-
let raa_msg = msgs::RevokeAndACK{ channel_id: chan.2, per_commitment_secret, next_per_commitment_point};
1722+
let raa_msg = msgs::RevokeAndACK {
1723+
channel_id: chan.2,
1724+
per_commitment_secret: local_secret,
1725+
next_per_commitment_point: next_local_point
1726+
};
17271727
nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &raa_msg);
17281728

17291729
let events = nodes[1].node.get_and_clear_pending_msg_events();
@@ -8126,9 +8126,11 @@ fn test_counterparty_raa_skip_no_crash() {
81268126
let mut guard = nodes[0].node.channel_state.lock().unwrap();
81278127
let keys = &guard.by_id.get_mut(&channel_id).unwrap().holder_keys;
81288128
const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1;
8129+
let per_commitment_secret = keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER);
8130+
// Must revoke without gaps
8131+
keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 1);
81298132
let next_per_commitment_point = PublicKey::from_secret_key(&Secp256k1::new(),
81308133
&SecretKey::from_slice(&keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)).unwrap());
8131-
let per_commitment_secret = keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER);
81328134

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

lightning/src/ln/onchaintx.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -962,7 +962,7 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
962962
pub(crate) fn get_fully_signed_copy_holder_tx(&mut self, funding_redeemscript: &Script) -> Option<Transaction> {
963963
if let Some(ref mut holder_commitment) = self.holder_commitment {
964964
let holder_commitment = holder_commitment.clone();
965-
match self.key_storage.sign_holder_commitment(&holder_commitment, &self.secp_ctx) {
965+
match self.key_storage.unsafe_sign_holder_commitment(&holder_commitment, &self.secp_ctx) {
966966
Ok(sig) => Some(holder_commitment.add_holder_sig(funding_redeemscript, sig)),
967967
Err(_) => return None,
968968
}

lightning/src/util/enforcing_trait_impls.rs

Lines changed: 37 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,19 +24,32 @@ use util::ser::{Writeable, Writer, Readable};
2424
use std::io::Error;
2525
use ln::msgs::DecodeError;
2626

27+
/// Initial value for revoked commitment downward counter
28+
pub const INITIAL_REVOKED_COMMITMENT_NUMBER: u64 = 1 << 48;
29+
2730
/// Enforces some rules on ChannelKeys calls. Eventually we will probably want to expose a variant
2831
/// of this which would essentially be what you'd want to run on a hardware wallet.
2932
#[derive(Clone)]
3033
pub struct EnforcingChannelKeys {
3134
pub inner: InMemoryChannelKeys,
32-
commitment_number_obscure_and_last: Arc<Mutex<(Option<u64>, u64)>>,
35+
pub(crate) revoked_commitment: Arc<Mutex<u64>>,
36+
pub(crate) commitment_number_obscure_and_last: Arc<Mutex<(Option<u64>, u64)>>,
3337
}
3438

3539
impl EnforcingChannelKeys {
3640
pub fn new(inner: InMemoryChannelKeys) -> Self {
3741
Self {
3842
inner,
3943
commitment_number_obscure_and_last: Arc::new(Mutex::new((None, 0))),
44+
revoked_commitment: Arc::new(Mutex::new(INITIAL_REVOKED_COMMITMENT_NUMBER))
45+
}
46+
}
47+
48+
pub fn new_with_revoked(inner: InMemoryChannelKeys, revoked_commitment: Arc<Mutex<u64>>) -> Self {
49+
Self {
50+
inner,
51+
commitment_number_obscure_and_last: Arc::new(Mutex::new((None, 0))),
52+
revoked_commitment
4053
}
4154
}
4255
}
@@ -62,7 +75,11 @@ impl ChannelKeys for EnforcingChannelKeys {
6275
}
6376

6477
fn release_commitment_secret(&self, idx: u64) -> [u8; 32] {
65-
// TODO: enforce the ChannelKeys contract - error here if we already signed this commitment
78+
{
79+
let mut revoked = self.revoked_commitment.lock().unwrap();
80+
assert!(idx == *revoked || idx == *revoked - 1, "can only revoke the current or next unrevoked commitment - trying {}, revoked {}", idx, *revoked);
81+
*revoked = idx;
82+
}
6683
self.inner.release_commitment_secret(idx)
6784
}
6885

@@ -90,6 +107,20 @@ impl ChannelKeys for EnforcingChannelKeys {
90107
fn sign_holder_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, holder_commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
91108
// TODO: enforce the ChannelKeys contract - error if this commitment was already revoked
92109
// TODO: need the commitment number
110+
let revoked = self.revoked_commitment.lock().unwrap();
111+
let keys = &holder_commitment_tx.keys;
112+
if keys.per_commitment_point != self.inner.get_per_commitment_point(*revoked - 1, secp_ctx) {
113+
if keys.per_commitment_point != self.inner.get_per_commitment_point(*revoked - 2, secp_ctx) {
114+
if keys.per_commitment_point == self.inner.get_per_commitment_point(*revoked, secp_ctx) {
115+
println!("attempted to sign the latest revoked local commitment {}", self.inner.commitment_seed[0]);
116+
return Err(())
117+
} else {
118+
println!("can only sign the next two unrevoked commitment numbers, {} revoked={} point={}",
119+
self.inner.commitment_seed[0], *revoked, keys.per_commitment_point);
120+
return Err(())
121+
}
122+
}
123+
}
93124
Ok(self.inner.sign_holder_commitment(holder_commitment_tx, secp_ctx).unwrap())
94125
}
95126

@@ -149,11 +180,12 @@ impl Writeable for EnforcingChannelKeys {
149180

150181
impl Readable for EnforcingChannelKeys {
151182
fn read<R: ::std::io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
152-
let inner = Readable::read(reader)?;
183+
let inner: InMemoryChannelKeys = Readable::read(reader)?;
153184
let obscure_and_last = Readable::read(reader)?;
154185
Ok(EnforcingChannelKeys {
155-
inner: inner,
156-
commitment_number_obscure_and_last: Arc::new(Mutex::new(obscure_and_last))
186+
inner,
187+
commitment_number_obscure_and_last: Arc::new(Mutex::new(obscure_and_last)),
188+
revoked_commitment: Arc::new(Mutex::new(INITIAL_REVOKED_COMMITMENT_NUMBER)),
157189
})
158190
}
159191
}

lightning/src/util/test_utils.rs

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ use chain::keysinterface;
1818
use ln::features::{ChannelFeatures, InitFeatures};
1919
use ln::msgs;
2020
use ln::msgs::OptionalField;
21-
use util::enforcing_trait_impls::EnforcingChannelKeys;
21+
use util::enforcing_trait_impls::{EnforcingChannelKeys, INITIAL_REVOKED_COMMITMENT_NUMBER};
2222
use util::events;
2323
use util::logger::{Logger, Level, Record};
2424
use util::ser::{Readable, ReadableArgs, Writer, Writeable};
@@ -35,7 +35,7 @@ use bitcoin::secp256k1::{SecretKey, PublicKey, Secp256k1, Signature};
3535
use regex;
3636

3737
use std::time::Duration;
38-
use std::sync::Mutex;
38+
use std::sync::{Mutex, Arc};
3939
use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering};
4040
use std::{cmp, mem};
4141
use std::collections::{HashMap, HashSet};
@@ -402,6 +402,7 @@ pub struct TestKeysInterface {
402402
backing: keysinterface::KeysManager,
403403
pub override_session_priv: Mutex<Option<[u8; 32]>>,
404404
pub override_channel_id_priv: Mutex<Option<[u8; 32]>>,
405+
revoked_commitments: Mutex<HashMap<[u8;32], Arc<Mutex<u64>>>>,
405406
}
406407

407408
impl keysinterface::KeysInterface for TestKeysInterface {
@@ -411,7 +412,9 @@ impl keysinterface::KeysInterface for TestKeysInterface {
411412
fn get_destination_script(&self) -> Script { self.backing.get_destination_script() }
412413
fn get_shutdown_pubkey(&self) -> PublicKey { self.backing.get_shutdown_pubkey() }
413414
fn get_channel_keys(&self, inbound: bool, channel_value_satoshis: u64) -> EnforcingChannelKeys {
414-
EnforcingChannelKeys::new(self.backing.get_channel_keys(inbound, channel_value_satoshis))
415+
let keys = self.backing.get_channel_keys(inbound, channel_value_satoshis);
416+
let revoked_commitment = self.make_revoked_commitment_cell(keys.commitment_seed);
417+
EnforcingChannelKeys::new_with_revoked(keys, revoked_commitment)
415418
}
416419

417420
fn get_secure_random_bytes(&self) -> [u8; 32] {
@@ -429,22 +432,37 @@ impl keysinterface::KeysInterface for TestKeysInterface {
429432
self.backing.get_secure_random_bytes()
430433
}
431434

432-
fn read_chan_signer(&self, reader: &[u8]) -> Result<Self::ChanKeySigner, msgs::DecodeError> {
433-
EnforcingChannelKeys::read(&mut std::io::Cursor::new(reader))
435+
fn read_chan_signer(&self, buffer: &[u8]) -> Result<Self::ChanKeySigner, msgs::DecodeError> {
436+
let mut reader = std::io::Cursor::new(buffer);
437+
438+
let inner: InMemoryChannelKeys = Readable::read(&mut reader)?;
439+
let revoked_commitment = self.make_revoked_commitment_cell(inner.commitment_seed);
440+
441+
let obscure_and_last = Readable::read(&mut reader)?;
442+
443+
Ok(EnforcingChannelKeys {
444+
inner,
445+
commitment_number_obscure_and_last: Arc::new(Mutex::new(obscure_and_last)),
446+
revoked_commitment,
447+
})
434448
}
435449
}
436450

451+
437452
impl TestKeysInterface {
438453
pub fn new(seed: &[u8; 32], network: Network) -> Self {
439454
let now = Duration::from_secs(genesis_block(network).header.time as u64);
440455
Self {
441456
backing: keysinterface::KeysManager::new(seed, network, now.as_secs(), now.subsec_nanos()),
442457
override_session_priv: Mutex::new(None),
443458
override_channel_id_priv: Mutex::new(None),
459+
revoked_commitments: Mutex::new(HashMap::new()),
444460
}
445461
}
446462
pub fn derive_channel_keys(&self, channel_value_satoshis: u64, user_id_1: u64, user_id_2: u64) -> EnforcingChannelKeys {
447-
EnforcingChannelKeys::new(self.backing.derive_channel_keys(channel_value_satoshis, user_id_1, user_id_2))
463+
let keys = self.backing.derive_channel_keys(channel_value_satoshis, user_id_1, user_id_2);
464+
let revoked_commitment = self.make_revoked_commitment_cell(keys.commitment_seed);
465+
EnforcingChannelKeys::new_with_revoked(keys, revoked_commitment)
448466
}
449467
}
450468

@@ -486,3 +504,14 @@ impl chain::Filter for TestChainSource {
486504
self.watched_outputs.lock().unwrap().insert((*outpoint, script_pubkey.clone()));
487505
}
488506
}
507+
508+
impl TestKeysInterface {
509+
fn make_revoked_commitment_cell(&self, commitment_seed: [u8; 32]) -> Arc<Mutex<u64>> {
510+
let mut revoked_commitments = self.revoked_commitments.lock().unwrap();
511+
if !revoked_commitments.contains_key(&commitment_seed) {
512+
revoked_commitments.insert(commitment_seed, Arc::new(Mutex::new(INITIAL_REVOKED_COMMITMENT_NUMBER)));
513+
}
514+
let cell = revoked_commitments.get(&commitment_seed).unwrap();
515+
Arc::clone(cell)
516+
}
517+
}

0 commit comments

Comments
 (0)