Introduce EnforcementState for EnforcingSigner

as we add more enforcement state variables, we want to keep track of them under a single structure

Author: devrandom

fuzz/src/chanmon_consistency.rs

@@ -39,7 +39,7 @@ use lightning::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
 use lightning::ln::channelmanager::{ChainParameters, ChannelManager, PaymentSendFailure, ChannelManagerReadArgs};
 use lightning::ln::features::{ChannelFeatures, InitFeatures, NodeFeatures};
 use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, DecodeError, UpdateAddHTLC, Init};
-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;
@@ -148,7 +148,7 @@ impl chain::Watch<EnforcingSigner> for TestChainMonitor {
 struct KeyProvider {
 	node_id: u8,
 	rand_bytes_id: atomic::AtomicU32,
-	revoked_commitments: Mutex<HashMap<[u8;32], Arc<Mutex<u64>>>>,
+	enforcement_states: Mutex<HashMap<[u8;32], Arc<Mutex<EnforcementState>>>>,
 }
 impl KeysInterface for KeyProvider {
 	type Signer = EnforcingSigner;
@@ -183,7 +183,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)
 	}
 
@@ -198,14 +198,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,
 		})
 	}
@@ -216,10 +213,10 @@ impl KeysInterface for KeyProvider {
 }
 
 impl KeyProvider {
-	fn make_revoked_commitment_cell(&self, commitment_seed: [u8; 32]) -> Arc<Mutex<u64>> {
-		let mut revoked_commitments = self.revoked_commitments.lock().unwrap();
+	fn make_enforcement_state_cell(&self, commitment_seed: [u8; 32]) -> Arc<Mutex<EnforcementState>> {
+		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)
@@ -335,7 +332,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 	macro_rules! make_node {
 		($node_id: expr) => { {
 			let logger: Arc<dyn Logger> = 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_est.clone(), Arc::new(TestPersister{}), Arc::clone(&keys_manager)));
 
 			let mut config = UserConfig::default();

lightning/src/util/enforcing_trait_impls.rs

@@ -46,34 +46,31 @@ 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<Mutex<Option<u64>>>,
-	/// The last holder commitment number we revoked, backwards counting
-	pub revoked_commitment: Arc<Mutex<u64>>,
+	/// Channel state used for policy enforcement
+	pub state: Arc<Mutex<EnforcementState>>,
 	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
 		}
 	}
 
 	/// 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
+	/// 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, revoked_commitment: Arc<Mutex<u64>>, disable_revocation_policy_check: bool) -> Self {
+	pub fn new_with_revoked(inner: InMemorySigner, state: Arc<Mutex<EnforcementState>>, 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_commitment_number.unwrap_or(actual_commitment_number);
 			// 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_commitment_number = Some(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,20 +172,18 @@ impl Sign for EnforcingSigner {}
 impl Writeable for EnforcingSigner {
 	fn write<W: Writer>(&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(())
 	}
 }
 
 impl Readable for EnforcingSigner {
 	fn read<R: io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
 		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_commitment_number: Option<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_commitment_number: None,
+			revoked_commitment: INITIAL_REVOKED_COMMITMENT_NUMBER
+		}
+	}
+}

lightning/src/util/test_utils.rs

@@ -19,7 +19,7 @@ use chain::keysinterface;
 use ln::features::{ChannelFeatures, InitFeatures};
 use ln::msgs;
 use ln::msgs::OptionalField;
-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};
@@ -451,7 +451,7 @@ pub struct TestKeysInterface {
 	pub override_session_priv: Mutex<Option<[u8; 32]>>,
 	pub override_channel_id_priv: Mutex<Option<[u8; 32]>>,
 	pub disable_revocation_policy_check: bool,
-	revoked_commitments: Mutex<HashMap<[u8;32], Arc<Mutex<u64>>>>,
+	enforcement_states: Mutex<HashMap<[u8;32], Arc<Mutex<EnforcementState>>>>,
 }
 
 impl keysinterface::KeysInterface for TestKeysInterface {
@@ -462,8 +462,8 @@ impl keysinterface::KeysInterface for TestKeysInterface {
 	fn get_shutdown_pubkey(&self) -> PublicKey { self.backing.get_shutdown_pubkey() }
 	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(keys.commitment_seed);
+		EnforcingSigner::new_with_revoked(keys, state, self.disable_revocation_policy_check)
 	}
 
 	fn get_secure_random_bytes(&self) -> [u8; 32] {
@@ -485,14 +485,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(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,
 		})
 	}
@@ -511,21 +508,22 @@ 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()),
 		}
 	}
 	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(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<Mutex<u64>> {
-		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(&self, commitment_seed: [u8; 32]) -> Arc<Mutex<EnforcementState>> {
+		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)
 	}
 }