From 50966e25ba4bc9792d10702d46b75baa5a013b30 Mon Sep 17 00:00:00 2001 From: Devrandom Date: Mon, 9 Aug 2021 10:56:15 +0200 Subject: [PATCH] Introduce EnforcementState for EnforcingSigner as we add more enforcement state variables, we want to keep track of them under a single structure --- fuzz/src/chanmon_consistency.rs | 21 +++---- lightning/src/util/enforcing_trait_impls.rs | 68 +++++++++++++-------- lightning/src/util/test_utils.rs | 32 +++++----- 3 files changed, 67 insertions(+), 54 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 7eacf253..88826d65 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -41,7 +41,7 @@ use lightning::ln::channel::FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE; use lightning::ln::features::{ChannelFeatures, InitFeatures, NodeFeatures}; use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, DecodeError, UpdateAddHTLC, Init}; use lightning::ln::script::ShutdownScript; -use lightning::util::enforcing_trait_impls::{EnforcingSigner, INITIAL_REVOKED_COMMITMENT_NUMBER}; +use lightning::util::enforcing_trait_impls::{EnforcingSigner, EnforcementState}; use lightning::util::errors::APIError; use lightning::util::events; use lightning::util::logger::Logger; @@ -161,7 +161,7 @@ impl chain::Watch for TestChainMonitor { struct KeyProvider { node_id: u8, rand_bytes_id: atomic::AtomicU32, - revoked_commitments: Mutex>>>, + enforcement_states: Mutex>>>, } impl KeysInterface for KeyProvider { type Signer = EnforcingSigner; @@ -198,7 +198,7 @@ impl KeysInterface for KeyProvider { channel_value_satoshis, [0; 32], ); - let revoked_commitment = self.make_revoked_commitment_cell(keys.commitment_seed); + let revoked_commitment = self.make_enforcement_state_cell(keys.commitment_seed); EnforcingSigner::new_with_revoked(keys, revoked_commitment, false) } @@ -213,14 +213,11 @@ impl KeysInterface for KeyProvider { let mut reader = std::io::Cursor::new(buffer); let inner: InMemorySigner = Readable::read(&mut reader)?; - let revoked_commitment = self.make_revoked_commitment_cell(inner.commitment_seed); - - let last_commitment_number = Readable::read(&mut reader)?; + let state = self.make_enforcement_state_cell(inner.commitment_seed); Ok(EnforcingSigner { inner, - last_commitment_number: Arc::new(Mutex::new(last_commitment_number)), - revoked_commitment, + state, disable_revocation_policy_check: false, }) } @@ -231,10 +228,10 @@ impl KeysInterface for KeyProvider { } impl KeyProvider { - fn make_revoked_commitment_cell(&self, commitment_seed: [u8; 32]) -> Arc> { - let mut revoked_commitments = self.revoked_commitments.lock().unwrap(); + fn make_enforcement_state_cell(&self, commitment_seed: [u8; 32]) -> Arc> { + let mut revoked_commitments = self.enforcement_states.lock().unwrap(); if !revoked_commitments.contains_key(&commitment_seed) { - revoked_commitments.insert(commitment_seed, Arc::new(Mutex::new(INITIAL_REVOKED_COMMITMENT_NUMBER))); + revoked_commitments.insert(commitment_seed, Arc::new(Mutex::new(EnforcementState::new()))); } let cell = revoked_commitments.get(&commitment_seed).unwrap(); Arc::clone(cell) @@ -351,7 +348,7 @@ pub fn do_test(data: &[u8], out: Out) { macro_rules! make_node { ($node_id: expr, $fee_estimator: expr) => { { let logger: Arc = Arc::new(test_logger::TestLogger::new($node_id.to_string(), out.clone())); - let keys_manager = Arc::new(KeyProvider { node_id: $node_id, rand_bytes_id: atomic::AtomicU32::new(0), revoked_commitments: Mutex::new(HashMap::new()) }); + let keys_manager = Arc::new(KeyProvider { node_id: $node_id, rand_bytes_id: atomic::AtomicU32::new(0), enforcement_states: Mutex::new(HashMap::new()) }); let monitor = Arc::new(TestChainMonitor::new(broadcast.clone(), logger.clone(), $fee_estimator.clone(), Arc::new(TestPersister{}), Arc::clone(&keys_manager))); let mut config = UserConfig::default(); diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs index baa9e7cd..cd3ead64 100644 --- a/lightning/src/util/enforcing_trait_impls.rs +++ b/lightning/src/util/enforcing_trait_impls.rs @@ -46,20 +46,18 @@ pub const INITIAL_REVOKED_COMMITMENT_NUMBER: u64 = 1 << 48; #[derive(Clone)] pub struct EnforcingSigner { pub inner: InMemorySigner, - /// The last counterparty commitment number we signed, backwards counting - pub last_commitment_number: Arc>>, - /// The last holder commitment number we revoked, backwards counting - pub revoked_commitment: Arc>, + /// Channel state used for policy enforcement + pub state: Arc>, pub disable_revocation_policy_check: bool, } impl EnforcingSigner { /// Construct an EnforcingSigner pub fn new(inner: InMemorySigner) -> Self { + let state = Arc::new(Mutex::new(EnforcementState::new())); Self { inner, - last_commitment_number: Arc::new(Mutex::new(None)), - revoked_commitment: Arc::new(Mutex::new(INITIAL_REVOKED_COMMITMENT_NUMBER)), + state, disable_revocation_policy_check: false } } @@ -67,13 +65,12 @@ impl EnforcingSigner { /// Construct an EnforcingSigner with externally managed storage /// /// Since there are multiple copies of this struct for each channel, some coordination is needed - /// so that all copies are aware of revocations. A pointer to this state is provided here, usually - /// by an implementation of KeysInterface. - pub fn new_with_revoked(inner: InMemorySigner, revoked_commitment: Arc>, disable_revocation_policy_check: bool) -> Self { + /// so that all copies are aware of enforcement state. A pointer to this state is provided + /// here, usually by an implementation of KeysInterface. + pub fn new_with_revoked(inner: InMemorySigner, state: Arc>, disable_revocation_policy_check: bool) -> Self { Self { inner, - last_commitment_number: Arc::new(Mutex::new(None)), - revoked_commitment, + state, disable_revocation_policy_check } } @@ -86,9 +83,9 @@ impl BaseSign for EnforcingSigner { fn release_commitment_secret(&self, idx: u64) -> [u8; 32] { { - let mut revoked = self.revoked_commitment.lock().unwrap(); - assert!(idx == *revoked || idx == *revoked - 1, "can only revoke the current or next unrevoked commitment - trying {}, revoked {}", idx, *revoked); - *revoked = idx; + let mut state = self.state.lock().unwrap(); + assert!(idx == state.revoked_commitment || idx == state.revoked_commitment - 1, "can only revoke the current or next unrevoked commitment - trying {}, revoked {}", idx, state.revoked_commitment); + state.revoked_commitment = idx; } self.inner.release_commitment_secret(idx) } @@ -100,13 +97,13 @@ impl BaseSign for EnforcingSigner { self.verify_counterparty_commitment_tx(commitment_tx, secp_ctx); { - let mut last_commitment_number_guard = self.last_commitment_number.lock().unwrap(); + let mut state = self.state.lock().unwrap(); let actual_commitment_number = commitment_tx.commitment_number(); - let last_commitment_number = last_commitment_number_guard.unwrap_or(actual_commitment_number); + let last_commitment_number = state.last_counterparty_commitment; // These commitment numbers are backwards counting. We expect either the same as the previously encountered, // or the next one. assert!(last_commitment_number == actual_commitment_number || last_commitment_number - 1 == actual_commitment_number, "{} doesn't come after {}", actual_commitment_number, last_commitment_number); - *last_commitment_number_guard = Some(cmp::min(last_commitment_number, actual_commitment_number)) + state.last_counterparty_commitment = cmp::min(last_commitment_number, actual_commitment_number) } Ok(self.inner.sign_counterparty_commitment(commitment_tx, secp_ctx).unwrap()) @@ -117,12 +114,12 @@ impl BaseSign for EnforcingSigner { let commitment_txid = trusted_tx.txid(); let holder_csv = self.inner.counterparty_selected_contest_delay(); - let revoked = self.revoked_commitment.lock().unwrap(); + let state = self.state.lock().unwrap(); let commitment_number = trusted_tx.commitment_number(); - if *revoked - 1 != commitment_number && *revoked - 2 != commitment_number { + if state.revoked_commitment - 1 != commitment_number && state.revoked_commitment - 2 != commitment_number { if !self.disable_revocation_policy_check { panic!("can only sign the next two unrevoked commitment numbers, revoked={} vs requested={} for {}", - *revoked, commitment_number, self.inner.commitment_seed[0]) + state.revoked_commitment, commitment_number, self.inner.commitment_seed[0]) } } @@ -175,8 +172,7 @@ impl Sign for EnforcingSigner {} impl Writeable for EnforcingSigner { fn write(&self, writer: &mut W) -> Result<(), Error> { self.inner.write(writer)?; - let last = *self.last_commitment_number.lock().unwrap(); - last.write(writer)?; + // NOTE - the commitment state is maintained by KeysInterface, so we don't persist it Ok(()) } } @@ -184,11 +180,10 @@ impl Writeable for EnforcingSigner { impl Readable for EnforcingSigner { fn read(reader: &mut R) -> Result { let inner = Readable::read(reader)?; - let last_commitment_number = Readable::read(reader)?; + let state = Arc::new(Mutex::new(EnforcementState::new())); Ok(EnforcingSigner { inner, - last_commitment_number: Arc::new(Mutex::new(last_commitment_number)), - revoked_commitment: Arc::new(Mutex::new(INITIAL_REVOKED_COMMITMENT_NUMBER)), + state, disable_revocation_policy_check: false, }) } @@ -207,3 +202,26 @@ impl EnforcingSigner { .expect("derived different per-tx keys or built transaction") } } + +/// The state used by [`EnforcingSigner`] in order to enforce policy checks +/// +/// This structure is maintained by KeysInterface since we may have multiple copies of +/// the signer and they must coordinate their state. +#[derive(Clone)] +pub struct EnforcementState { + /// The last counterparty commitment number we signed, backwards counting + pub last_counterparty_commitment: u64, + /// The last holder commitment number we revoked, backwards counting + pub revoked_commitment: u64, + +} + +impl EnforcementState { + /// Enforcement state for a new channel + pub fn new() -> Self { + EnforcementState { + last_counterparty_commitment: INITIAL_REVOKED_COMMITMENT_NUMBER, + revoked_commitment: INITIAL_REVOKED_COMMITMENT_NUMBER, + } + } +} diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 15157e50..fed16768 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -20,7 +20,7 @@ use ln::features::{ChannelFeatures, InitFeatures}; use ln::msgs; use ln::msgs::OptionalField; use ln::script::ShutdownScript; -use util::enforcing_trait_impls::{EnforcingSigner, INITIAL_REVOKED_COMMITMENT_NUMBER}; +use util::enforcing_trait_impls::{EnforcingSigner, EnforcementState}; use util::events; use util::logger::{Logger, Level, Record}; use util::ser::{Readable, ReadableArgs, Writer, Writeable}; @@ -452,7 +452,7 @@ pub struct TestKeysInterface { pub override_session_priv: Mutex>, pub override_channel_id_priv: Mutex>, pub disable_revocation_policy_check: bool, - revoked_commitments: Mutex>>>, + enforcement_states: Mutex>>>, expectations: Mutex>>, } @@ -474,8 +474,8 @@ impl keysinterface::KeysInterface for TestKeysInterface { fn get_channel_signer(&self, inbound: bool, channel_value_satoshis: u64) -> EnforcingSigner { let keys = self.backing.get_channel_signer(inbound, channel_value_satoshis); - let revoked_commitment = self.make_revoked_commitment_cell(keys.commitment_seed); - EnforcingSigner::new_with_revoked(keys, revoked_commitment, self.disable_revocation_policy_check) + let state = self.make_enforcement_state_cell(keys.commitment_seed); + EnforcingSigner::new_with_revoked(keys, state, self.disable_revocation_policy_check) } fn get_secure_random_bytes(&self) -> [u8; 32] { @@ -497,14 +497,11 @@ impl keysinterface::KeysInterface for TestKeysInterface { let mut reader = io::Cursor::new(buffer); let inner: InMemorySigner = Readable::read(&mut reader)?; - let revoked_commitment = self.make_revoked_commitment_cell(inner.commitment_seed); - - let last_commitment_number = Readable::read(&mut reader)?; + let state = self.make_enforcement_state_cell(inner.commitment_seed); Ok(EnforcingSigner { inner, - last_commitment_number: Arc::new(Mutex::new(last_commitment_number)), - revoked_commitment, + state, disable_revocation_policy_check: self.disable_revocation_policy_check, }) } @@ -522,7 +519,7 @@ impl TestKeysInterface { override_session_priv: Mutex::new(None), override_channel_id_priv: Mutex::new(None), disable_revocation_policy_check: false, - revoked_commitments: Mutex::new(HashMap::new()), + enforcement_states: Mutex::new(HashMap::new()), expectations: Mutex::new(None), } } @@ -538,16 +535,17 @@ impl TestKeysInterface { pub fn derive_channel_keys(&self, channel_value_satoshis: u64, id: &[u8; 32]) -> EnforcingSigner { let keys = self.backing.derive_channel_keys(channel_value_satoshis, id); - let revoked_commitment = self.make_revoked_commitment_cell(keys.commitment_seed); - EnforcingSigner::new_with_revoked(keys, revoked_commitment, self.disable_revocation_policy_check) + let state = self.make_enforcement_state_cell(keys.commitment_seed); + EnforcingSigner::new_with_revoked(keys, state, self.disable_revocation_policy_check) } - fn make_revoked_commitment_cell(&self, commitment_seed: [u8; 32]) -> Arc> { - let mut revoked_commitments = self.revoked_commitments.lock().unwrap(); - if !revoked_commitments.contains_key(&commitment_seed) { - revoked_commitments.insert(commitment_seed, Arc::new(Mutex::new(INITIAL_REVOKED_COMMITMENT_NUMBER))); + fn make_enforcement_state_cell(&self, commitment_seed: [u8; 32]) -> Arc> { + let mut states = self.enforcement_states.lock().unwrap(); + if !states.contains_key(&commitment_seed) { + let state = EnforcementState::new(); + states.insert(commitment_seed, Arc::new(Mutex::new(state))); } - let cell = revoked_commitments.get(&commitment_seed).unwrap(); + let cell = states.get(&commitment_seed).unwrap(); Arc::clone(cell) } } -- 2.30.2