[quorum store] refactor batch_reader/store and batch requester #6785

zekun000 · 2023-02-26T08:08:41Z

This commit merges batch_reader and batch_store into a single struct and uses it as synchronous component (via function calls)
instead of asynchronous one (via channels).

Also batch_requester is refactored to use network rpc directly.

sasha8 · 2023-02-28T00:46:23Z

consensus/src/quorum_store/network_listener.rs

    network_msg_rx: aptos_channel::Receiver<PeerId, VerifiedEvent>,
-    batch_reader_tx: Sender<BatchReaderCommand>,
+    batch_reader: Arc<BatchReader<NetworkSender>>,


@gelash will be so proud

sasha8 · 2023-02-28T00:48:57Z

consensus/src/quorum_store/network_listener.rs

-                        .await
-                        .expect("could not push Batch batch_reader");
+                    if let Ok(value) = self.batch_reader.get_batch_from_local(&request.digest()) {
+                        let batch =


@gelash will be so so proud

sasha8 · 2023-02-28T01:04:45Z

consensus/src/quorum_store/network_listener.rs

+                            Batch::new(self.my_peer_id, self.epoch, request.digest(), value);
+                        self.network_sender
+                            .send_batch(batch, vec![request.source()])
+                            .await;


Can the await be a performance problem since it delays handling messages on the critical path?

sasha8 · 2023-02-28T01:04:55Z

consensus/src/quorum_store/network_listener.rs

-                        .expect("could not push Batch batch_reader");
+                    self.batch_reader
+                        .receive_batch(batch.digest(), batch.into_payload())
+                        .await;


Same for this await

sasha8 · 2023-02-28T01:11:15Z

consensus/src/quorum_store/batch_reader.rs

+                    proof.shuffled_signers(&self.validator_verifier),
+                    tx,
+                )
+                .await;


Same comment about the await. Now the execution thread is calling it. Before the execution thread can do other stuff in parallel. Do you think it is significant? For example get data fro the other batches in the meantime.

sasha8 · 2023-02-28T01:47:29Z

consensus/src/quorum_store/batch_reader.rs

-
-            self.self_tx
-                .send(BatchReaderCommand::GetBatchForSelf(proof, tx))
+            // TODO: handle timeout


Critical logic.

consensus/src/twins/basic_twins_test.rs

This commit merges batch_reader and batch_store into a single struct and uses it as synchronous component (via function calls) instead of asynchronous one (via channels). Also batch_requester is refactored to use network rpc directly.

github-actions · 2023-03-08T19:11:52Z

✅ Forge suite `consensus_only_perf_benchmark` success on `consensus_only_perf_test_f77d1d2c5a571b20989d62aaed92d96e2a45e041` ==> `f77d1d2c5a571b20989d62aaed92d96e2a45e041`

Test Ok

github-actions · 2023-03-08T19:14:58Z

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `f77d1d2c5a571b20989d62aaed92d96e2a45e041`

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> f77d1d2c5a571b20989d62aaed92d96e2a45e041 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 8085 TPS, 4735 ms latency, 7200 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: f77d1d2c5a571b20989d62aaed92d96e2a45e041
compatibility::simple-validator-upgrade::single-validator-upgrade : 4722 TPS, 8601 ms latency, 12100 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: f77d1d2c5a571b20989d62aaed92d96e2a45e041
compatibility::simple-validator-upgrade::half-validator-upgrade : 4961 TPS, 7728 ms latency, 10500 ms p99 latency,no expired txns
4. upgrading second batch to new version: f77d1d2c5a571b20989d62aaed92d96e2a45e041
compatibility::simple-validator-upgrade::rest-validator-upgrade : 7018 TPS, 5367 ms latency, 9000 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> f77d1d2c5a571b20989d62aaed92d96e2a45e041 passed
Test Ok

github-actions · 2023-03-08T19:14:58Z

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `f77d1d2c5a571b20989d62aaed92d96e2a45e041`

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> f77d1d2c5a571b20989d62aaed92d96e2a45e041 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7805 TPS, 4886 ms latency, 7300 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: f77d1d2c5a571b20989d62aaed92d96e2a45e041
compatibility::simple-validator-upgrade::single-validator-upgrade : 4864 TPS, 8496 ms latency, 11700 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: f77d1d2c5a571b20989d62aaed92d96e2a45e041
compatibility::simple-validator-upgrade::half-validator-upgrade : 5176 TPS, 7394 ms latency, 10900 ms p99 latency,no expired txns
4. upgrading second batch to new version: f77d1d2c5a571b20989d62aaed92d96e2a45e041
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6834 TPS, 5685 ms latency, 10200 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> f77d1d2c5a571b20989d62aaed92d96e2a45e041 passed
Test Ok

github-actions · 2023-03-08T19:15:33Z

✅ Forge suite `land_blocking` success on `f77d1d2c5a571b20989d62aaed92d96e2a45e041`

performance benchmark with full nodes : 5904 TPS, 6660 ms latency, 12800 ms p99 latency,(!) expired 2180 out of 2523380 txns
Test Ok

github-actions · 2023-03-08T19:16:20Z

✅ Forge suite `consensus_only_perf_benchmark` success on `consensus_only_perf_test_f77d1d2c5a571b20989d62aaed92d96e2a45e041` ==> `f77d1d2c5a571b20989d62aaed92d96e2a45e041`

Test Ok

github-actions · 2023-03-08T19:18:12Z

✅ Forge suite `framework_upgrade` success on `cb4ba0a57c998c60cbab65af31a64875d2588ca5` ==> `f77d1d2c5a571b20989d62aaed92d96e2a45e041`

Compatibility test results for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> f77d1d2c5a571b20989d62aaed92d96e2a45e041 (PR)
Upgrade the nodes to version: f77d1d2c5a571b20989d62aaed92d96e2a45e041
framework_upgrade::framework-upgrade::full-framework-upgrade : 6984 TPS, 5666 ms latency, 8300 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> f77d1d2c5a571b20989d62aaed92d96e2a45e041 passed
Test Ok

zekun000 added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Feb 26, 2023