From 77078dbe95a7674925db3e58e6651628062f588f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Thu, 31 Oct 2019 14:51:35 +0100 Subject: [PATCH 1/5] Don't send a heartbeat if already online. --- Cargo.lock | 1 + srml/im-online/Cargo.toml | 1 + srml/im-online/src/lib.rs | 32 ++++++++++++++++++++------ srml/im-online/src/tests.rs | 45 +++++++++++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 13fc0dcd51a77..0651a9e0f4980 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4299,6 +4299,7 @@ dependencies = [ name = "srml-im-online" version = "0.1.0" dependencies = [ + "env_logger 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)", "parity-scale-codec 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.101 (registry+https://github.com/rust-lang/crates.io-index)", "sr-io 2.0.0", diff --git a/srml/im-online/Cargo.toml b/srml/im-online/Cargo.toml index 4c7ccf1487d9b..fa975444cb160 100644 --- a/srml/im-online/Cargo.toml +++ b/srml/im-online/Cargo.toml @@ -20,6 +20,7 @@ system = { package = "srml-system", path = "../system", default-features = false [dev-dependencies] offchain = { package = "substrate-offchain", path = "../../core/offchain" } +env_logger = "0.6" [features] default = ["std", "session/historical"] diff --git a/srml/im-online/src/lib.rs b/srml/im-online/src/lib.rs index 46e26ff32fecb..5c0f6ee87405d 100644 --- a/srml/im-online/src/lib.rs +++ b/srml/im-online/src/lib.rs @@ -364,7 +364,7 @@ impl Module { Err(err) => print(err), } } else { - debug::native::trace!( + debug::native::debug!( target: "imonline", "Skipping gossip at: {:?} >= {:?} || {:?}", next_gossip, @@ -377,6 +377,7 @@ impl Module { fn do_gossip_at(block_number: T::BlockNumber) -> Result<(), OffchainErr> { // we run only when a local authority key is configured let authorities = Keys::::get(); + let mut results = Vec::new(); let mut local_keys = T::AuthorityId::all(); local_keys.sort(); @@ -388,6 +389,16 @@ impl Module { .map(|location| (index as u32, &local_keys[location])) }) { + if Self::is_online_in_current_session(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, @@ -405,14 +416,21 @@ impl Module { authority_index, block_number ); - T::SubmitTransaction::submit_unsigned(call) - .map_err(|_| OffchainErr::SubmitTransaction)?; - // 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); + results.push( + T::SubmitTransaction::submit_unsigned(call) + .map_err(|_| OffchainErr::SubmitTransaction) + ); } + + // fail only after trying all keys. + results.into_iter().collect::, 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(()) } diff --git a/srml/im-online/src/tests.rs b/srml/im-online/src/tests.rs index 42c1aa02276a5..d0fe10f34ad77 100644 --- a/srml/im-online/src/tests.rs +++ b/srml/im-online/src/tests.rs @@ -273,3 +273,48 @@ fn should_mark_online_validator_when_block_is_authored() { assert!(!ImOnline::is_online_in_current_session(2)); }); } + +#[test] +fn should_not_send_a_report_if_already_online() { + env_logger::init(); + 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::::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, + }); + }); +} From 96c094fb07ae29eac0d89aeeb7cb522fafe7c876 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Thu, 31 Oct 2019 15:21:33 +0100 Subject: [PATCH 2/5] Remove env_logger. --- srml/im-online/Cargo.toml | 1 - srml/im-online/src/tests.rs | 1 - 2 files changed, 2 deletions(-) diff --git a/srml/im-online/Cargo.toml b/srml/im-online/Cargo.toml index fa975444cb160..4c7ccf1487d9b 100644 --- a/srml/im-online/Cargo.toml +++ b/srml/im-online/Cargo.toml @@ -20,7 +20,6 @@ system = { package = "srml-system", path = "../system", default-features = false [dev-dependencies] offchain = { package = "substrate-offchain", path = "../../core/offchain" } -env_logger = "0.6" [features] default = ["std", "session/historical"] diff --git a/srml/im-online/src/tests.rs b/srml/im-online/src/tests.rs index d0fe10f34ad77..82b02c5906d38 100644 --- a/srml/im-online/src/tests.rs +++ b/srml/im-online/src/tests.rs @@ -276,7 +276,6 @@ fn should_mark_online_validator_when_block_is_authored() { #[test] fn should_not_send_a_report_if_already_online() { - env_logger::init(); use authorship::EventHandler; let mut ext = new_test_ext(); From f946592d0bd6cad4c92b2dd3ce2725542130a652 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Thu, 31 Oct 2019 15:23:33 +0100 Subject: [PATCH 3/5] Update lock. --- Cargo.lock | 1 - 1 file changed, 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 2f31493f5a767..5bee6302ec8ef 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4299,7 +4299,6 @@ dependencies = [ name = "srml-im-online" version = "0.1.0" dependencies = [ - "env_logger 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)", "parity-scale-codec 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.101 (registry+https://github.com/rust-lang/crates.io-index)", "sr-io 2.0.0", From be637ff56fbdd1c6592a1231db8e5dcea0160ea5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Thu, 31 Oct 2019 15:24:58 +0100 Subject: [PATCH 4/5] Bump runtime. --- node/runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/runtime/src/lib.rs b/node/runtime/src/lib.rs index adee13775d7d3..bdbfed730bc79 100644 --- a/node/runtime/src/lib.rs +++ b/node/runtime/src/lib.rs @@ -80,7 +80,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, }; From 41f57d3edc8a492dedb3888c6de0f2a5849755b0 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Fri, 1 Nov 2019 11:12:54 +0000 Subject: [PATCH 5/5] Merge master --- srml/im-online/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/srml/im-online/src/lib.rs b/srml/im-online/src/lib.rs index 9f58315a95ead..3feefb6d26eeb 100644 --- a/srml/im-online/src/lib.rs +++ b/srml/im-online/src/lib.rs @@ -394,7 +394,7 @@ impl Module { .map(|location| (index as u32, &local_keys[location])) }) { - if Self::is_online_in_current_session(authority_index) { + if Self::is_online(authority_index) { debug::native::info!( target: "imonline", "[index: {:?}] Skipping sending heartbeat at block: {:?}. Already online.",