From fab42ad30b894bf1aca4de0ceed454e08b4db842 Mon Sep 17 00:00:00 2001 From: Brian Cho Date: Tue, 25 Jul 2023 16:16:56 -0700 Subject: [PATCH 1/4] Be more aggressive on mempool broadcast, based on forge observations --- config/src/config/mempool_config.rs | 63 +++++++++++++++------------ mempool/src/logging.rs | 2 + mempool/src/shared_mempool/network.rs | 3 +- mempool/src/shared_mempool/tasks.rs | 2 +- testsuite/forge-cli/src/main.rs | 2 +- 5 files changed, 41 insertions(+), 31 deletions(-) diff --git a/config/src/config/mempool_config.rs b/config/src/config/mempool_config.rs index 0eacbd0bd23f8..0093dbbe58f8b 100644 --- a/config/src/config/mempool_config.rs +++ b/config/src/config/mempool_config.rs @@ -60,11 +60,11 @@ impl Default for MempoolConfig { MempoolConfig { shared_mempool_tick_interval_ms: 50, shared_mempool_backoff_interval_ms: 30_000, - shared_mempool_batch_size: 200, + shared_mempool_batch_size: 300, shared_mempool_max_batch_bytes: MAX_APPLICATION_MESSAGE_SIZE as u64, shared_mempool_ack_timeout_ms: 2_000, shared_mempool_max_concurrent_inbound_syncs: 4, - max_broadcasts_per_peer: 2, + max_broadcasts_per_peer: 20, max_network_channel_size: 1024, mempool_snapshot_interval_secs: 180, capacity: 2_000_000, @@ -103,6 +103,18 @@ impl ConfigOptimizer for MempoolConfig { // Change the default configs for VFNs let mut modified_config = false; + if node_type.is_validator() { + // Set the max_broadcasts_per_peer to 2 (default is 20) + if local_mempool_config_yaml["max_broadcasts_per_peer"].is_null() { + mempool_config.max_broadcasts_per_peer = 2; + modified_config = true; + } + // Set the batch size per broadcast to 200 (default is 300) + if local_mempool_config_yaml["shared_mempool_batch_size"].is_null() { + mempool_config.shared_mempool_batch_size = 200; + modified_config = true; + } + } if node_type.is_validator_fullnode() { // Set the shared_mempool_max_concurrent_inbound_syncs to 16 (default is 4) if local_mempool_config_yaml["shared_mempool_max_concurrent_inbound_syncs"].is_null() { @@ -110,12 +122,6 @@ impl ConfigOptimizer for MempoolConfig { modified_config = true; } - // Set the max_broadcasts_per_peer to 4 (default is 2) - if local_mempool_config_yaml["max_broadcasts_per_peer"].is_null() { - mempool_config.max_broadcasts_per_peer = 4; - modified_config = true; - } - // Set the default_failovers to 0 (default is 1) if local_mempool_config_yaml["default_failovers"].is_null() { mempool_config.default_failovers = 0; @@ -158,9 +164,9 @@ mod tests { mempool_config.shared_mempool_max_concurrent_inbound_syncs, 16 ); - assert_eq!(mempool_config.max_broadcasts_per_peer, 4); + assert_eq!(mempool_config.max_broadcasts_per_peer, 20); assert_eq!(mempool_config.default_failovers, 0); - assert_eq!(mempool_config.shared_mempool_batch_size, 200); + assert_eq!(mempool_config.shared_mempool_batch_size, 300); assert_eq!(mempool_config.shared_mempool_tick_interval_ms, 10); } @@ -177,7 +183,7 @@ mod tests { ChainId::mainnet(), ) .unwrap(); - assert!(!modified_config); + assert!(modified_config); // Verify that all relevant fields are not modified let mempool_config = &node_config.mempool; @@ -186,18 +192,12 @@ mod tests { mempool_config.shared_mempool_max_concurrent_inbound_syncs, default_mempool_config.shared_mempool_max_concurrent_inbound_syncs ); - assert_eq!( - mempool_config.max_broadcasts_per_peer, - default_mempool_config.max_broadcasts_per_peer - ); + assert_eq!(mempool_config.max_broadcasts_per_peer, 2); assert_eq!( mempool_config.default_failovers, default_mempool_config.default_failovers ); - assert_eq!( - mempool_config.shared_mempool_batch_size, - default_mempool_config.shared_mempool_batch_size - ); + assert_eq!(mempool_config.shared_mempool_batch_size, 200); assert_eq!( mempool_config.shared_mempool_tick_interval_ms, default_mempool_config.shared_mempool_tick_interval_ms @@ -207,16 +207,24 @@ mod tests { #[test] fn test_optimize_vfn_config_no_overrides() { // Create the default validator config + let local_shared_mempool_max_concurrent_inbound_syncs = 1; + let local_max_broadcasts_per_peer = 1; let mut node_config = NodeConfig::get_default_vfn_config(); + node_config + .mempool + .shared_mempool_max_concurrent_inbound_syncs = + local_shared_mempool_max_concurrent_inbound_syncs; + node_config.mempool.max_broadcasts_per_peer = local_max_broadcasts_per_peer; // Create a local config YAML with some local overrides - let local_config_yaml = serde_yaml::from_str( + let local_config_yaml = serde_yaml::from_str(&format!( r#" mempool: - shared_mempool_max_concurrent_inbound_syncs: 4 - max_broadcasts_per_peer: 1 + shared_mempool_max_concurrent_inbound_syncs: {} + max_broadcasts_per_peer: {} "#, - ) + local_shared_mempool_max_concurrent_inbound_syncs, local_max_broadcasts_per_peer + )) .unwrap(); // Optimize the config and verify modifications are made @@ -234,12 +242,11 @@ mod tests { let default_mempool_config = MempoolConfig::default(); assert_eq!( mempool_config.shared_mempool_max_concurrent_inbound_syncs, - 4 + local_shared_mempool_max_concurrent_inbound_syncs ); - assert_eq!(mempool_config.max_broadcasts_per_peer, 2); - assert_ne!( - mempool_config.default_failovers, - default_mempool_config.default_failovers + assert_eq!( + mempool_config.max_broadcasts_per_peer, + local_max_broadcasts_per_peer ); assert_ne!( mempool_config.shared_mempool_tick_interval_ms, diff --git a/mempool/src/logging.rs b/mempool/src/logging.rs index 219210e25340a..e3ac583798841 100644 --- a/mempool/src/logging.rs +++ b/mempool/src/logging.rs @@ -116,6 +116,7 @@ pub struct LogSchema<'a> { #[schema(debug)] batch_id: Option<&'a MultiBatchId>, backpressure: Option, + num_txns: Option, } impl<'a> LogSchema<'a> { @@ -143,6 +144,7 @@ impl<'a> LogSchema<'a> { upstream_network: None, batch_id: None, backpressure: None, + num_txns: None, } } } diff --git a/mempool/src/shared_mempool/network.rs b/mempool/src/shared_mempool/network.rs index 5f39a5c199b2e..5869bcff63bfe 100644 --- a/mempool/src/shared_mempool/network.rs +++ b/mempool/src/shared_mempool/network.rs @@ -481,11 +481,12 @@ impl> MempoolNetworkInterf // Log all the metrics let latency = start_time.elapsed(); - trace!( + debug!( LogSchema::event_log(LogEntry::BroadcastTransaction, LogEvent::Success) .peer(&peer) .batch_id(&batch_id) .backpressure(scheduled_backoff) + .num_txns(num_txns) ); let network_id = peer.network_id(); counters::shared_mempool_broadcast_size(network_id, num_txns); diff --git a/mempool/src/shared_mempool/tasks.rs b/mempool/src/shared_mempool/tasks.rs index ff8f9dfcb2589..5f856ffdf5bd2 100644 --- a/mempool/src/shared_mempool/tasks.rs +++ b/mempool/src/shared_mempool/tasks.rs @@ -70,7 +70,7 @@ pub(crate) async fn execute_broadcast( .peer(&peer) .error(&error)), _ => { - trace!("{:?}", err) + debug!("{:?}", err) }, } } diff --git a/testsuite/forge-cli/src/main.rs b/testsuite/forge-cli/src/main.rs index 8cda6b226565e..6b0c41b99841b 100644 --- a/testsuite/forge-cli/src/main.rs +++ b/testsuite/forge-cli/src/main.rs @@ -500,7 +500,7 @@ fn single_test_suite( let single_test_suite = match test_name { // Land-blocking tests to be run on every PR: "land_blocking" => land_blocking_test_suite(duration), // to remove land_blocking, superseeded by the below - "realistic_env_max_load" => realistic_env_max_load_test(duration, test_cmd, 7, 5), + "realistic_env_max_load" => pfn_const_tps(duration, true, true), "compat" => compat(), "framework_upgrade" => framework_upgrade(), // Rest of the tests: From da6a92453ae74221c123e40967aaa22ac82279ca Mon Sep 17 00:00:00 2001 From: Brian Cho Date: Wed, 26 Jul 2023 15:58:49 -0700 Subject: [PATCH 2/4] Test pfn_performance --- testsuite/forge-cli/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testsuite/forge-cli/src/main.rs b/testsuite/forge-cli/src/main.rs index 6b0c41b99841b..0b212d78cfed0 100644 --- a/testsuite/forge-cli/src/main.rs +++ b/testsuite/forge-cli/src/main.rs @@ -500,7 +500,7 @@ fn single_test_suite( let single_test_suite = match test_name { // Land-blocking tests to be run on every PR: "land_blocking" => land_blocking_test_suite(duration), // to remove land_blocking, superseeded by the below - "realistic_env_max_load" => pfn_const_tps(duration, true, true), + "realistic_env_max_load" => pfn_performance(duration, true, true), "compat" => compat(), "framework_upgrade" => framework_upgrade(), // Rest of the tests: From 46a02ab26a673a2e5f9aa44f5e00920731578c77 Mon Sep 17 00:00:00 2001 From: Brian Cho Date: Wed, 2 Aug 2023 13:55:12 -0700 Subject: [PATCH 3/4] Revert "Test pfn_performance" This reverts commit da6a92453ae74221c123e40967aaa22ac82279ca. --- testsuite/forge-cli/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testsuite/forge-cli/src/main.rs b/testsuite/forge-cli/src/main.rs index 0b212d78cfed0..6b0c41b99841b 100644 --- a/testsuite/forge-cli/src/main.rs +++ b/testsuite/forge-cli/src/main.rs @@ -500,7 +500,7 @@ fn single_test_suite( let single_test_suite = match test_name { // Land-blocking tests to be run on every PR: "land_blocking" => land_blocking_test_suite(duration), // to remove land_blocking, superseeded by the below - "realistic_env_max_load" => pfn_performance(duration, true, true), + "realistic_env_max_load" => pfn_const_tps(duration, true, true), "compat" => compat(), "framework_upgrade" => framework_upgrade(), // Rest of the tests: From 0de5ec55aa401ab734fdc3235475a80b0877ac7c Mon Sep 17 00:00:00 2001 From: Brian Cho Date: Wed, 2 Aug 2023 14:04:36 -0700 Subject: [PATCH 4/4] revert forge test change --- testsuite/forge-cli/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testsuite/forge-cli/src/main.rs b/testsuite/forge-cli/src/main.rs index 6b0c41b99841b..8cda6b226565e 100644 --- a/testsuite/forge-cli/src/main.rs +++ b/testsuite/forge-cli/src/main.rs @@ -500,7 +500,7 @@ fn single_test_suite( let single_test_suite = match test_name { // Land-blocking tests to be run on every PR: "land_blocking" => land_blocking_test_suite(duration), // to remove land_blocking, superseeded by the below - "realistic_env_max_load" => pfn_const_tps(duration, true, true), + "realistic_env_max_load" => realistic_env_max_load_test(duration, test_cmd, 7, 5), "compat" => compat(), "framework_upgrade" => framework_upgrade(), // Rest of the tests: