-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Mempool] optimize fullnode broadcast hops for latency #9309
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
e1f66ba
to
da6a924
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Config changes are the best changes 😄
testsuite/forge-cli/src/main.rs
Outdated
@@ -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_performance(duration, true, true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be rolled back? 😄
This reverts commit da6a924.
@@ -103,19 +103,25 @@ 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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why do we need to override this for the validator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't used anymore because there is no mempool broadcast between validators with Quorum Store. For the time being, I want to preserve behavior as much as possible just in case we rollback Quorum Store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment to clarify this?
testsuite/forge-cli/src/main.rs
Outdated
@@ -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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want to revert this change before landing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, now really reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
### Description * Increase single fullnode max throughput from 4K TPS to 6K TPS (max batch size 200 -> 300, scheduled every 50 ms) * Increase throughput when broadcast RTT is large, by increasing the number of outstanding requests. E.g., previously an RTT of 500 ms with 2 outstanding requests, meant only 2 requests could be made every 500 ms. ### Test Plan Run forge with PFNs and network emulation, observe that `Avg Insertion-to-Broadcast-Batched` on PFNs drops significantly, from 1-2 s for some PFNs to < 200 ms.
### Description Reduce the noise added by #9309. Make expected "errors" trace, and sample regardless.
Description
Test Plan
Run forge with PFNs and network emulation, observe that
Avg Insertion-to-Broadcast-Batched
on PFNs drops significantly, from 1-2 s for some PFNs to < 200 ms.