From d40bf3cf365c694f2807ccbcc118226b84502e07 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 6 May 2020 10:52:44 +0200 Subject: [PATCH] client/authority-discovery: Do not double encode signature (#5901) Previously, when publishing ones address onto the DHT, the signature signing those addresses would be SCALE encoded twice. This commit removes the second encoding and adjusts the tests to catch future regressions. --- .../client/authority-discovery/src/lib.rs | 8 +- .../client/authority-discovery/src/tests.rs | 121 +++++++++--------- substrate/primitives/core/src/traits.rs | 6 +- 3 files changed, 69 insertions(+), 66 deletions(-) diff --git a/substrate/client/authority-discovery/src/lib.rs b/substrate/client/authority-discovery/src/lib.rs index e5b7c986a4..bc76c14331 100644 --- a/substrate/client/authority-discovery/src/lib.rs +++ b/substrate/client/authority-discovery/src/lib.rs @@ -58,7 +58,7 @@ use futures::task::{Context, Poll}; use futures::{Future, FutureExt, ready, Stream, StreamExt}; use futures_timer::Delay; -use codec::{Decode, Encode}; +use codec::Decode; use error::{Error, Result}; use log::{debug, error, log_enabled}; use prometheus_endpoint::{Counter, CounterVec, Gauge, Opts, U64, register}; @@ -262,16 +262,16 @@ where ) .map_err(|_| Error::Signing)?; - for (sign_result, key) in signatures.iter().zip(keys) { + for (sign_result, key) in signatures.into_iter().zip(keys) { let mut signed_addresses = vec![]; // sign_with_all returns Result signature // is generated for a public key that is supported. // Verify that all signatures exist for all provided keys. - let signature = sign_result.as_ref().map_err(|_| Error::MissingSignature(key.clone()))?; + let signature = sign_result.map_err(|_| Error::MissingSignature(key.clone()))?; schema::SignedAuthorityAddresses { addresses: serialized_addresses.clone(), - signature: Encode::encode(&signature), + signature, } .encode(&mut signed_addresses) .map_err(Error::EncodingProto)?; diff --git a/substrate/client/authority-discovery/src/tests.rs b/substrate/client/authority-discovery/src/tests.rs index c9b5e392d8..c13bca894c 100644 --- a/substrate/client/authority-discovery/src/tests.rs +++ b/substrate/client/authority-discovery/src/tests.rs @@ -162,8 +162,8 @@ sp_api::mock_impl_runtime_apis! { } } -#[derive(Default)] struct TestNetwork { + peer_id: PeerId, // Whenever functions on `TestNetwork` are called, the function arguments are added to the // vectors below. pub put_value_call: Arc)>>>, @@ -171,6 +171,17 @@ struct TestNetwork { pub set_priority_group_call: Arc)>>>, } +impl Default for TestNetwork { + fn default() -> Self { + TestNetwork { + peer_id: PeerId::random(), + put_value_call: Default::default(), + get_value_call: Default::default(), + set_priority_group_call: Default::default(), + } + } +} + impl NetworkProvider for TestNetwork { fn set_priority_group( &self, @@ -193,11 +204,11 @@ impl NetworkProvider for TestNetwork { impl NetworkStateInfo for TestNetwork { fn local_peer_id(&self) -> PeerId { - PeerId::random() + self.peer_id.clone() } fn external_addresses(&self) -> Vec { - vec![] + vec!["/ip6/2001:db8::".parse().unwrap()] } } @@ -224,34 +235,6 @@ fn new_registers_metrics() { assert!(registry.gather().len() > 0); } -#[test] -fn publish_ext_addresses_puts_record_on_dht() { - let (_dht_event_tx, dht_event_rx) = channel(1000); - let network: Arc = Arc::new(Default::default()); - let key_store = KeyStore::new(); - let public = key_store - .write() - .sr25519_generate_new(key_types::AUTHORITY_DISCOVERY, None) - .unwrap(); - let test_api = Arc::new(TestApi { - authorities: vec![public.into()], - }); - - let mut authority_discovery = AuthorityDiscovery::new( - test_api, - network.clone(), - vec![], - dht_event_rx.boxed(), - Role::Authority(key_store), - None, - ); - - authority_discovery.publish_ext_addresses().unwrap(); - - // Expect authority discovery to put a new record onto the dht. - assert_eq!(network.put_value_call.lock().unwrap().len(), 1); -} - #[test] fn request_addresses_of_others_triggers_dht_get_query() { let _ = ::env_logger::try_init(); @@ -284,15 +267,57 @@ fn request_addresses_of_others_triggers_dht_get_query() { } #[test] -fn handle_dht_events_with_value_found_should_call_set_priority_group() { +fn publish_discover_cycle() { let _ = ::env_logger::try_init(); - // Create authority discovery. + // Node A publishing its address. + + let (_dht_event_tx, dht_event_rx) = channel(1000); + + let network: Arc = Arc::new(Default::default()); + let node_a_multiaddr = { + let peer_id = network.local_peer_id(); + let address = network.external_addresses().pop().unwrap(); + + address.with(libp2p::core::multiaddr::Protocol::P2p( + peer_id.into(), + )) + }; + + let key_store = KeyStore::new(); + let node_a_public = key_store + .write() + .sr25519_generate_new(key_types::AUTHORITY_DISCOVERY, None) + .unwrap(); + let test_api = Arc::new(TestApi { + authorities: vec![node_a_public.into()], + }); + + let mut authority_discovery = AuthorityDiscovery::new( + test_api, + network.clone(), + vec![], + dht_event_rx.boxed(), + Role::Authority(key_store), + None, + ); + + authority_discovery.publish_ext_addresses().unwrap(); + + // Expect authority discovery to put a new record onto the dht. + assert_eq!(network.put_value_call.lock().unwrap().len(), 1); + + let dht_event = { + let (key, value) = network.put_value_call.lock().unwrap().pop().unwrap(); + sc_network::DhtEvent::ValueFound(vec![(key, value)]) + }; + + // Node B discovering node A's address. let (mut dht_event_tx, dht_event_rx) = channel(1000); - let key_pair = AuthorityPair::from_seed_slice(&[1; 32]).unwrap(); let test_api = Arc::new(TestApi { - authorities: vec![key_pair.public()], + // Make sure node B identifies node A as an authority. + authorities: vec![node_a_public.into()], }); let network: Arc = Arc::new(Default::default()); let key_store = KeyStore::new(); @@ -306,32 +331,10 @@ fn handle_dht_events_with_value_found_should_call_set_priority_group() { None, ); - // Create sample dht event. - - let authority_id_1 = hash_authority_id(key_pair.public().as_ref()); - let address_1: Multiaddr = "/ip6/2001:db8::".parse().unwrap(); - - let mut serialized_addresses = vec![]; - schema::AuthorityAddresses { - addresses: vec![address_1.to_vec()], - } - .encode(&mut serialized_addresses) - .unwrap(); - - let signature = key_pair.sign(serialized_addresses.as_ref()).encode(); - let mut signed_addresses = vec![]; - schema::SignedAuthorityAddresses { - addresses: serialized_addresses, - signature, - } - .encode(&mut signed_addresses) - .unwrap(); - - let dht_event = sc_network::DhtEvent::ValueFound(vec![(authority_id_1, signed_addresses)]); dht_event_tx.try_send(dht_event).unwrap(); - // Make authority discovery handle the event. let f = |cx: &mut Context<'_>| -> Poll<()> { + // Make authority discovery handle the event. if let Poll::Ready(e) = authority_discovery.handle_dht_events(cx) { panic!("Unexpected error: {:?}", e); } @@ -343,7 +346,7 @@ fn handle_dht_events_with_value_found_should_call_set_priority_group() { network.set_priority_group_call.lock().unwrap()[0], ( "authorities".to_string(), - HashSet::from_iter(vec![address_1.clone()].into_iter()) + HashSet::from_iter(vec![node_a_multiaddr.clone()].into_iter()) ) ); diff --git a/substrate/primitives/core/src/traits.rs b/substrate/primitives/core/src/traits.rs index cca1a8fa8c..133a4a4e19 100644 --- a/substrate/primitives/core/src/traits.rs +++ b/substrate/primitives/core/src/traits.rs @@ -120,7 +120,7 @@ pub trait BareCryptoStore: Send + Sync { /// Given a list of public keys, find the first supported key and /// sign the provided message with that key. /// - /// Returns a tuple of the used key and the signature + /// Returns a tuple of the used key and the SCALE encoded signature. fn sign_with_any( &self, id: KeyTypeId, @@ -144,8 +144,8 @@ pub trait BareCryptoStore: Send + Sync { /// Provided a list of public keys, sign a message with /// each key given that the key is supported. /// - /// Returns a list of `Result`s each representing the signature of each key or - /// a BareCryptoStoreError for non-supported keys. + /// Returns a list of `Result`s each representing the SCALE encoded + /// signature of each key or a BareCryptoStoreError for non-supported keys. fn sign_with_all( &self, id: KeyTypeId,