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

@@ -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<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;
@@ -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<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)
@@ -351,7 +348,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 	macro_rules! make_node {
 		($node_id: expr, $fee_estimator: 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_estimator.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
-	/// by an implementation of KeysInterface.
-	pub fn new_with_revoked(inner: InMemorySigner, revoked_commitment: Arc<Mutex<u64>>, 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<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_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,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_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,
+		}
+	}
+}

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<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>>>>,
 	expectations: Mutex<Option<VecDeque<OnGetShutdownScriptpubkey>>>,
 }
 
@@ -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<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_cell(&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)
 	}
 }