Refactor Peerdb and PeerManager (#2660)

## Proposed Changes

This is a refactor of the PeerDB and PeerManager. A number of bugs have been surfacing around the connection state of peers and their interaction with the score state. 

This refactor tightens the mutability properties of peers such that only specific modules are able to modify the state of peer information preventing inadvertant state changes that can lead to our local peer manager db being out of sync with libp2p. 

Further, the logic around connection and scoring was quite convoluted and the distinction between the PeerManager and Peerdb was not well defined. Although these issues are not fully resolved, this PR is step to cleaning up this logic. The peerdb solely manages most mutability operations of peers leaving high-order logic to the peer manager. 

A single `update_connection_state()` function has been added to the peer-db making it solely responsible for modifying the peer's connection state. The way the peer's scores can be modified have been reduced to three simple functions (`update_scores()`, `update_gossipsub_scores()` and `report_peer()`). This prevents any add-hoc modifications of scores and only natural processes of score modification is allowed which simplifies the reasoning of score and state changes.

beacon_node/http_api/tests/common.rs

@@ -7,7 +7,7 @@ use eth2_libp2p::{
     discv5::enr::{CombinedKey, EnrBuilder},
     rpc::methods::{MetaData, MetaDataV2},
     types::{EnrAttestationBitfield, EnrSyncCommitteeBitfield, SyncState},
-    Enr, NetworkGlobals, PeerId,
+    ConnectedPoint, Enr, NetworkConfig, NetworkGlobals, PeerId, PeerManager,
 };
 use http_api::{Config, Context};
 use network::NetworkMessage;
@@ -46,7 +46,7 @@ pub struct ApiServer<E: EthSpec, SFut: Future<Output = ()>> {
 }
 
 impl<E: EthSpec> InteractiveTester<E> {
-    pub fn new(spec: Option<ChainSpec>, validator_count: usize) -> Self {
+    pub async fn new(spec: Option<ChainSpec>, validator_count: usize) -> Self {
         let harness = BeaconChainHarness::new(
             E::default(),
             spec,
@@ -59,7 +59,7 @@ impl<E: EthSpec> InteractiveTester<E> {
             shutdown_tx: _server_shutdown,
             network_rx,
             ..
-        } = create_api_server(harness.chain.clone(), harness.logger().clone());
+        } = create_api_server(harness.chain.clone(), harness.logger().clone()).await;
 
         tokio::spawn(server);
 
@@ -82,7 +82,7 @@ impl<E: EthSpec> InteractiveTester<E> {
     }
 }
 
-pub fn create_api_server<T: BeaconChainTypes>(
+pub async fn create_api_server<T: BeaconChainTypes>(
     chain: Arc<BeaconChain<T>>,
     log: Logger,
 ) -> ApiServer<T::EthSpec, impl Future<Output = ()>> {
@@ -96,15 +96,30 @@ pub fn create_api_server<T: BeaconChainTypes>(
     });
     let enr_key = CombinedKey::generate_secp256k1();
     let enr = EnrBuilder::new("v4").build(&enr_key).unwrap();
-    let network_globals =
-        NetworkGlobals::new(enr.clone(), TCP_PORT, UDP_PORT, meta_data, vec![], &log);
+    let network_globals = Arc::new(NetworkGlobals::new(
+        enr.clone(),
+        TCP_PORT,
+        UDP_PORT,
+        meta_data,
+        vec![],
+        &log,
+    ));
 
+    // Only a peer manager can add peers, so we create a dummy manager.
+    let network_config = NetworkConfig::default();
+    let mut pm = PeerManager::new(&network_config, network_globals.clone(), &log)
+        .await
+        .unwrap();
+
+    // add a peer
     let peer_id = PeerId::random();
-    network_globals
-        .peers
-        .write()
-        .connect_ingoing(&peer_id, EXTERNAL_ADDR.parse().unwrap(), None);
 
+    let connected_point = ConnectedPoint::Listener {
+        local_addr: EXTERNAL_ADDR.parse().unwrap(),
+        send_back_addr: EXTERNAL_ADDR.parse().unwrap(),
+    };
+    let num_established = std::num::NonZeroU32::new(1).unwrap();
+    pm.inject_connection_established(peer_id, connected_point, num_established, None);
     *network_globals.sync_state.write() = SyncState::Synced;
 
     let eth1_service = eth1::Service::new(eth1::Config::default(), log.clone(), chain.spec.clone());
@@ -119,7 +134,7 @@ pub fn create_api_server<T: BeaconChainTypes>(
         },
         chain: Some(chain.clone()),
         network_tx: Some(network_tx),
-        network_globals: Some(Arc::new(network_globals)),
+        network_globals: Some(network_globals),
         eth1_service: Some(eth1_service),
         log,
     });

beacon_node/http_api/tests/fork_tests.rs

@@ -17,7 +17,7 @@ async fn sync_committee_duties_across_fork() {
     let validator_count = E::sync_committee_size();
     let fork_epoch = Epoch::new(8);
     let spec = altair_spec(fork_epoch);
-    let tester = InteractiveTester::<E>::new(Some(spec.clone()), validator_count);
+    let tester = InteractiveTester::<E>::new(Some(spec.clone()), validator_count).await;
     let harness = &tester.harness;
     let client = &tester.client;
 
@@ -102,7 +102,7 @@ async fn attestations_across_fork_with_skip_slots() {
     let validator_count = E::sync_committee_size();
     let fork_epoch = Epoch::new(8);
     let spec = altair_spec(fork_epoch);
-    let tester = InteractiveTester::<E>::new(Some(spec.clone()), validator_count);
+    let tester = InteractiveTester::<E>::new(Some(spec.clone()), validator_count).await;
     let harness = &tester.harness;
     let client = &tester.client;
 
@@ -152,7 +152,7 @@ async fn sync_contributions_across_fork_with_skip_slots() {
     let validator_count = E::sync_committee_size();
     let fork_epoch = Epoch::new(8);
     let spec = altair_spec(fork_epoch);
-    let tester = InteractiveTester::<E>::new(Some(spec.clone()), validator_count);
+    let tester = InteractiveTester::<E>::new(Some(spec.clone()), validator_count).await;
     let harness = &tester.harness;
     let client = &tester.client;
 
@@ -199,7 +199,7 @@ async fn sync_committee_indices_across_fork() {
     let validator_count = E::sync_committee_size();
     let fork_epoch = Epoch::new(8);
     let spec = altair_spec(fork_epoch);
-    let tester = InteractiveTester::<E>::new(Some(spec.clone()), validator_count);
+    let tester = InteractiveTester::<E>::new(Some(spec.clone()), validator_count).await;
     let harness = &tester.harness;
     let client = &tester.client;
 

beacon_node/http_api/tests/interactive_tests.rs

@@ -18,7 +18,7 @@ async fn deposit_contract_custom_network() {
     // Arbitrary contract address.
     spec.deposit_contract_address = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".parse().unwrap();
 
-    let tester = InteractiveTester::<E>::new(Some(spec.clone()), validator_count);
+    let tester = InteractiveTester::<E>::new(Some(spec.clone()), validator_count).await;
     let client = &tester.client;
 
     let result = client.get_config_deposit_contract().await.unwrap().data;

beacon_node/http_api/tests/tests.rs

@@ -63,14 +63,14 @@ struct ApiTester {
 }
 
 impl ApiTester {
-    pub fn new() -> Self {
+    pub async fn new() -> Self {
         // This allows for testing voluntary exits without building out a massive chain.
         let mut spec = E::default_spec();
         spec.shard_committee_period = 2;
-        Self::new_from_spec(spec)
+        Self::new_from_spec(spec).await
     }
 
-    pub fn new_from_spec(spec: ChainSpec) -> Self {
+    pub async fn new_from_spec(spec: ChainSpec) -> Self {
         let harness = BeaconChainHarness::new(
             MainnetEthSpec,
             Some(spec.clone()),
@@ -181,7 +181,7 @@ impl ApiTester {
             network_rx,
             local_enr,
             external_peer_id,
-        } = create_api_server(chain.clone(), log);
+        } = create_api_server(chain.clone(), log).await;
 
         tokio::spawn(server);
 
@@ -213,7 +213,7 @@ impl ApiTester {
         }
     }
 
-    pub fn new_from_genesis() -> Self {
+    pub async fn new_from_genesis() -> Self {
         let harness = BeaconChainHarness::new(
             MainnetEthSpec,
             None,
@@ -260,7 +260,7 @@ impl ApiTester {
             network_rx,
             local_enr,
             external_peer_id,
-        } = create_api_server(chain.clone(), log);
+        } = create_api_server(chain.clone(), log).await;
 
         tokio::spawn(server);
 
@@ -2453,7 +2453,7 @@ async fn poll_events<S: Stream<Item = Result<EventKind<T>, eth2::Error>> + Unpin
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn get_events() {
-    ApiTester::new().test_get_events().await;
+    ApiTester::new().await.test_get_events().await;
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
@@ -2461,6 +2461,7 @@ async fn get_events_altair() {
     let mut spec = E::default_spec();
     spec.altair_fork_epoch = Some(Epoch::new(0));
     ApiTester::new_from_spec(spec)
+        .await
         .test_get_events_altair()
         .await;
 }
@@ -2468,6 +2469,7 @@ async fn get_events_altair() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn get_events_from_genesis() {
     ApiTester::new_from_genesis()
+        .await
         .test_get_events_from_genesis()
         .await;
 }
@@ -2475,6 +2477,7 @@ async fn get_events_from_genesis() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn beacon_get() {
     ApiTester::new()
+        .await
         .test_beacon_genesis()
         .await
         .test_beacon_states_root()
@@ -2515,17 +2518,21 @@ async fn beacon_get() {
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn post_beacon_blocks_valid() {
-    ApiTester::new().test_post_beacon_blocks_valid().await;
+    ApiTester::new().await.test_post_beacon_blocks_valid().await;
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn post_beacon_blocks_invalid() {
-    ApiTester::new().test_post_beacon_blocks_invalid().await;
+    ApiTester::new()
+        .await
+        .test_post_beacon_blocks_invalid()
+        .await;
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn beacon_pools_post_attestations_valid() {
     ApiTester::new()
+        .await
         .test_post_beacon_pool_attestations_valid()
         .await;
 }
@@ -2533,6 +2540,7 @@ async fn beacon_pools_post_attestations_valid() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn beacon_pools_post_attestations_invalid() {
     ApiTester::new()
+        .await
         .test_post_beacon_pool_attestations_invalid()
         .await;
 }
@@ -2540,6 +2548,7 @@ async fn beacon_pools_post_attestations_invalid() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn beacon_pools_post_attester_slashings_valid() {
     ApiTester::new()
+        .await
         .test_post_beacon_pool_attester_slashings_valid()
         .await;
 }
@@ -2547,6 +2556,7 @@ async fn beacon_pools_post_attester_slashings_valid() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn beacon_pools_post_attester_slashings_invalid() {
     ApiTester::new()
+        .await
         .test_post_beacon_pool_attester_slashings_invalid()
         .await;
 }
@@ -2554,6 +2564,7 @@ async fn beacon_pools_post_attester_slashings_invalid() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn beacon_pools_post_proposer_slashings_valid() {
     ApiTester::new()
+        .await
         .test_post_beacon_pool_proposer_slashings_valid()
         .await;
 }
@@ -2561,6 +2572,7 @@ async fn beacon_pools_post_proposer_slashings_valid() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn beacon_pools_post_proposer_slashings_invalid() {
     ApiTester::new()
+        .await
         .test_post_beacon_pool_proposer_slashings_invalid()
         .await;
 }
@@ -2568,6 +2580,7 @@ async fn beacon_pools_post_proposer_slashings_invalid() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn beacon_pools_post_voluntary_exits_valid() {
     ApiTester::new()
+        .await
         .test_post_beacon_pool_voluntary_exits_valid()
         .await;
 }
@@ -2575,6 +2588,7 @@ async fn beacon_pools_post_voluntary_exits_valid() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn beacon_pools_post_voluntary_exits_invalid() {
     ApiTester::new()
+        .await
         .test_post_beacon_pool_voluntary_exits_invalid()
         .await;
 }
@@ -2582,6 +2596,7 @@ async fn beacon_pools_post_voluntary_exits_invalid() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn config_get() {
     ApiTester::new()
+        .await
         .test_get_config_fork_schedule()
         .await
         .test_get_config_spec()
@@ -2593,6 +2608,7 @@ async fn config_get() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn debug_get() {
     ApiTester::new()
+        .await
         .test_get_debug_beacon_states()
         .await
         .test_get_debug_beacon_heads()
@@ -2602,6 +2618,7 @@ async fn debug_get() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn node_get() {
     ApiTester::new()
+        .await
         .test_get_node_version()
         .await
         .test_get_node_syncing()
@@ -2620,17 +2637,24 @@ async fn node_get() {
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn get_validator_duties_early() {
-    ApiTester::new().test_get_validator_duties_early().await;
+    ApiTester::new()
+        .await
+        .test_get_validator_duties_early()
+        .await;
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn get_validator_duties_attester() {
-    ApiTester::new().test_get_validator_duties_attester().await;
+    ApiTester::new()
+        .await
+        .test_get_validator_duties_attester()
+        .await;
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn get_validator_duties_attester_with_skip_slots() {
     ApiTester::new()
+        .await
         .skip_slots(E::slots_per_epoch() * 2)
         .test_get_validator_duties_attester()
         .await;
@@ -2638,12 +2662,16 @@ async fn get_validator_duties_attester_with_skip_slots() {
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn get_validator_duties_proposer() {
-    ApiTester::new().test_get_validator_duties_proposer().await;
+    ApiTester::new()
+        .await
+        .test_get_validator_duties_proposer()
+        .await;
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn get_validator_duties_proposer_with_skip_slots() {
     ApiTester::new()
+        .await
         .skip_slots(E::slots_per_epoch() * 2)
         .test_get_validator_duties_proposer()
         .await;
@@ -2651,12 +2679,13 @@ async fn get_validator_duties_proposer_with_skip_slots() {
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn block_production() {
-    ApiTester::new().test_block_production().await;
+    ApiTester::new().await.test_block_production().await;
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn block_production_with_skip_slots() {
     ApiTester::new()
+        .await
         .skip_slots(E::slots_per_epoch() * 2)
         .test_block_production()
         .await;
@@ -2664,12 +2693,16 @@ async fn block_production_with_skip_slots() {
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn get_validator_attestation_data() {
-    ApiTester::new().test_get_validator_attestation_data().await;
+    ApiTester::new()
+        .await
+        .test_get_validator_attestation_data()
+        .await;
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn get_validator_attestation_data_with_skip_slots() {
     ApiTester::new()
+        .await
         .skip_slots(E::slots_per_epoch() * 2)
         .test_get_validator_attestation_data()
         .await;
@@ -2678,6 +2711,7 @@ async fn get_validator_attestation_data_with_skip_slots() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn get_validator_aggregate_attestation() {
     ApiTester::new()
+        .await
         .test_get_validator_aggregate_attestation()
         .await;
 }
@@ -2685,6 +2719,7 @@ async fn get_validator_aggregate_attestation() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn get_validator_aggregate_attestation_with_skip_slots() {
     ApiTester::new()
+        .await
         .skip_slots(E::slots_per_epoch() * 2)
         .test_get_validator_aggregate_attestation()
         .await;
@@ -2693,6 +2728,7 @@ async fn get_validator_aggregate_attestation_with_skip_slots() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn get_validator_aggregate_and_proofs_valid() {
     ApiTester::new()
+        .await
         .test_get_validator_aggregate_and_proofs_valid()
         .await;
 }
@@ -2700,6 +2736,7 @@ async fn get_validator_aggregate_and_proofs_valid() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn get_validator_aggregate_and_proofs_valid_with_skip_slots() {
     ApiTester::new()
+        .await
         .skip_slots(E::slots_per_epoch() * 2)
         .test_get_validator_aggregate_and_proofs_valid()
         .await;
@@ -2708,6 +2745,7 @@ async fn get_validator_aggregate_and_proofs_valid_with_skip_slots() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn get_validator_aggregate_and_proofs_invalid() {
     ApiTester::new()
+        .await
         .test_get_validator_aggregate_and_proofs_invalid()
         .await;
 }
@@ -2715,6 +2753,7 @@ async fn get_validator_aggregate_and_proofs_invalid() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn get_validator_aggregate_and_proofs_invalid_with_skip_slots() {
     ApiTester::new()
+        .await
         .skip_slots(E::slots_per_epoch() * 2)
         .test_get_validator_aggregate_and_proofs_invalid()
         .await;
@@ -2723,6 +2762,7 @@ async fn get_validator_aggregate_and_proofs_invalid_with_skip_slots() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn get_validator_beacon_committee_subscriptions() {
     ApiTester::new()
+        .await
         .test_get_validator_beacon_committee_subscriptions()
         .await;
 }
@@ -2730,6 +2770,7 @@ async fn get_validator_beacon_committee_subscriptions() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn lighthouse_endpoints() {
     ApiTester::new()
+        .await
         .test_get_lighthouse_health()
         .await
         .test_get_lighthouse_syncing()