Avoid sending heartbeat if we are already considered online. (#3981)

* Don't send a heartbeat if already online.

* Remove env_logger.

* Update lock.

* Bump runtime.

* Merge master

substrate/node/runtime/src/lib.rs

@@ -81,7 +81,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	// and set impl_version to equal spec_version. If only runtime
 	// implementation changes and behavior does not, then leave spec_version as
 	// is and increment impl_version.
-	spec_version: 191,
+	spec_version: 192,
 	impl_version: 191,
 	apis: RUNTIME_API_VERSIONS,
 };

substrate/srml/im-online/src/lib.rs

@@ -369,7 +369,7 @@ impl<T: Trait> Module<T> {
 				Err(err) => print(err),
 			}
 		} else {
-			debug::native::trace!(
+			debug::native::debug!(
 				target: "imonline",
 				"Skipping gossip at: {:?} >= {:?} || {:?}",
 				next_gossip,
@@ -382,6 +382,7 @@ impl<T: Trait> Module<T> {
 	fn do_gossip_at(block_number: T::BlockNumber) -> Result<(), OffchainErr> {
 		// we run only when a local authority key is configured
 		let authorities = Keys::<T>::get();
+		let mut results = Vec::new();
 		let mut local_keys = T::AuthorityId::all();
 		local_keys.sort();
 
@@ -393,6 +394,16 @@ impl<T: Trait> Module<T> {
 					.map(|location| (index as u32, &local_keys[location]))
 			})
 		{
+			if Self::is_online(authority_index) {
+				debug::native::info!(
+					target: "imonline",
+					"[index: {:?}] Skipping sending heartbeat at block: {:?}. Already online.",
+					authority_index,
+					block_number
+				);
+				continue;
+			}
+
 			let network_state = runtime_io::network_state().map_err(|_| OffchainErr::NetworkState)?;
 			let heartbeat_data = Heartbeat {
 				block_number,
@@ -410,14 +421,21 @@ impl<T: Trait> Module<T> {
 				authority_index,
 				block_number
 			);
-			T::SubmitTransaction::submit_unsigned(call)
-				.map_err(|_| OffchainErr::SubmitTransaction)?;
 
-			// once finished we set the worker status without comparing
+			results.push(
-			// if the existing value changed in the meantime. this is
+				T::SubmitTransaction::submit_unsigned(call)
-			// because at this point the heartbeat was definitely submitted.
+					.map_err(|_| OffchainErr::SubmitTransaction)
-			Self::set_worker_status(block_number, true);
+			);
 		}
+
+		// fail only after trying all keys.
+		results.into_iter().collect::<Result<Vec<_>, OffchainErr>>()?;
+
+		// once finished we set the worker status without comparing
+		// if the existing value changed in the meantime. this is
+		// because at this point the heartbeat was definitely submitted.
+		Self::set_worker_status(block_number, true);
+
 		Ok(())
 	}
 

substrate/srml/im-online/src/tests.rs

@@ -273,3 +273,47 @@ fn should_mark_online_validator_when_block_is_authored() {
 		assert!(!ImOnline::is_online(2));
 	});
 }
+
+#[test]
+fn should_not_send_a_report_if_already_online() {
+	use authorship::EventHandler;
+
+	let mut ext = new_test_ext();
+	let (offchain, state) = TestOffchainExt::new();
+	ext.register_extension(OffchainExt::new(offchain));
+
+	ext.execute_with(|| {
+		advance_session();
+		// given
+		VALIDATORS.with(|l| *l.borrow_mut() = Some(vec![1, 2, 3, 4, 5, 6]));
+		assert_eq!(Session::validators(), Vec::<u64>::new());
+		// enact the change and buffer another one
+		advance_session();
+		assert_eq!(Session::current_index(), 2);
+		assert_eq!(Session::validators(), vec![1, 2, 3]);
+		ImOnline::note_author(2);
+		ImOnline::note_uncle(3, 0);
+
+		// when
+		UintAuthorityId::set_all_keys(vec![0]); // all authorities use session key 0
+		ImOnline::offchain(4);
+
+		// then
+		let transaction = state.write().transactions.pop().unwrap();
+		// All validators have `0` as their session key, but we should only produce 1 hearbeat.
+		assert_eq!(state.read().transactions.len(), 0);
+		// check stuff about the transaction.
+		let ex: Extrinsic = Decode::decode(&mut &*transaction).unwrap();
+		let heartbeat = match ex.1 {
+			crate::mock::Call::ImOnline(crate::Call::heartbeat(h, _)) => h,
+			e => panic!("Unexpected call: {:?}", e),
+		};
+
+		assert_eq!(heartbeat, Heartbeat {
+			block_number: 4,
+			network_state: runtime_io::network_state().unwrap(),
+			session_index: 2,
+			authority_index: 0,
+		});
+	});
+}