[Quorum Store] networking integration #5779

bchocho · 2022-12-06T01:52:35Z

Description

Quorum Store related changes to consensus networking.

Add messages for Fragment, BatchRequest, Batch, SignedDigest, ProofOfStore
Filter these messages if quorum store is not enabled
Add sender-only verification for some messages
Add a separate FIFO local channel for quorum store messages
Add and use broadcast without sending to self

Test Plan

Existing tests. The logic is coming in future PRs.

bchocho · 2022-12-09T00:57:27Z

consensus/consensus-types/src/proof_of_store.rs

+    }
+}
+
+// TODO: implement properly (and proper place) w.o. public fields.


@sasha8 @gelash would you consider this resolved?

Not sure. Fields in SignedDigestInfo and SignedDigest are public. @gelash ?

maybe it's okay, but I guess we wanted to revisit whether the fields should be pub or not.

this looks very weird especially one field is private and has getter and others are public, we should be consistent

bchocho · 2022-12-09T00:57:40Z

consensus/src/quorum_store/types.rs

+    pub(crate) batch_info: BatchInfo,
+}
+
+// TODO: make epoch, source, signature fields treatment consistent across structs.


@sasha8 @gelash would you consider this resolved?

sasha8

Left a few comments. Mostly around quorum_store_enabled verification.

sasha8 · 2022-12-09T01:32:28Z

consensus/src/network.rs

+        let msg = ConsensusMsg::ProofOfStoreMsg(Box::new(proof_of_store));
+        self.broadcast_without_self(msg).await
+    }
+


Should we move the two above methods to QuorumStoreSender trait?

sasha8 · 2022-12-09T01:37:07Z

consensus/src/network_interface.rs

+    /// Quorum Store: Send a signed batch digest. This is a vote for the batch and a promise that
+    /// the batch of transactions was received and will be persisted until batch expiration.
+    SignedDigestMsg(Box<SignedDigest>),
+    /// Quorum Store: Broadcast a completed proof of store (a digest that received 2f+1 votes).


Completed -> certified or valid?

sasha8 · 2022-12-09T01:55:55Z

consensus/consensus-types/src/common.rs

        validator: &ValidatorVerifier,
        quorum_store_enabled: bool,
    ) -> anyhow::Result<()> {
        match (quorum_store_enabled, self) {
            (false, Payload::DirectMempool(_)) => Ok(()),
            (true, Payload::InQuorumStore(proof_with_status)) => {
                for proof in proof_with_status.proofs.iter() {
-                    proof.verify(validator)?;
+                    proof.verify(peer_id, validator, quorum_store_enabled)?;


Do we need to pass quorum_store_enabled here? We know it is true from the match so why check again inside the verify?

sasha8 · 2022-12-09T02:04:03Z

consensus/consensus-types/src/proof_of_store.rs

+    ) -> anyhow::Result<()> {
+        if !quorum_store_enabled {
+            return Err(anyhow::anyhow!(
+                "Quorum store is not enabled locally. Sender: {}, epoch: {}",


I am not sure we need this as this verify will always be called with quorum_store_enabled = true. See the comment above.

sasha8 · 2022-12-09T02:09:11Z

consensus/consensus-types/src/proof_of_store.rs

+    }
+}
+
+// TODO: implement properly (and proper place) w.o. public fields.


Not sure. Fields in SignedDigestInfo and SignedDigest are public. @gelash ?

sasha8 · 2022-12-09T02:22:51Z

consensus/src/round_manager.rs

@@ -91,6 +98,23 @@ impl UnverifiedEvent {
                cd.verify(validator)?;
                VerifiedEvent::CommitDecision(cd)
            }
+            UnverifiedEvent::FragmentMsg(f) => {
+                f.verify(peer_id, quorum_store_enabled)?;


Will it be cleaner to check quorum_store_enabled before? We know already here that if it is false we should not even got quorum store messages.
Maybe check it in process_message() in epoch_manager?

sasha8 · 2022-12-09T02:36:59Z

consensus/src/quorum_store/types.rs

+                "Quorum store is not enabled locally. Sender: {}, epoch: {}",
+                peer_id,
+                self.epoch(),
+            ));


Same comment as above. I would check quorum_store_enabled when getting the message.

- Add messages for Fragment, Batch, SignedDigest, ProofOfStore - Add sender-only verification for some messages - Add a separate FIFO local channel for quorum store messages - Add and use broadcast without sending to self

sasha8 · 2022-12-10T01:07:52Z

consensus/src/epoch_manager.rs

@@ -836,6 +846,29 @@ impl EpochManager {
        Ok(None)
    }

+    async fn check_quorum_store_enabled(


The name is confusing. Maybe check_unverified_event_type?

this is not an async function, also isn't this redundant since we already pass self.quorum_store_enabled to verify function

I originally did the check in the verify function, but changed it to do the separate check based on Sasha's feedback (#5779 (comment)). I think this is a bit clearer; it avoids dirtying the verify function which should really verify the correctness of the message.

davidiw

how are we testing backwards compatibility?

zekun000 · 2022-12-11T09:40:46Z

consensus/consensus-types/src/proof_of_store.rs

+    }
+}
+
+// TODO: implement properly (and proper place) w.o. public fields.


this looks very weird especially one field is private and has getter and others are public, we should be consistent

zekun000 · 2022-12-11T09:43:52Z

consensus/src/epoch_manager.rs

@@ -836,6 +846,29 @@ impl EpochManager {
        Ok(None)
    }

+    async fn check_quorum_store_enabled(


this is not an async function, also isn't this redundant since we already pass self.quorum_store_enabled to verify function

zekun000 · 2022-12-11T09:45:23Z

consensus/src/network.rs

@@ -289,6 +344,11 @@ impl NetworkTask {
    ) -> (NetworkTask, NetworkReceivers) {
        let (consensus_messages_tx, consensus_messages) =
            aptos_channel::new(QueueStyle::LIFO, 1, Some(&counters::CONSENSUS_CHANNEL_MSGS));
+        let (quorum_store_messages_tx, quorum_store_messages) = aptos_channel::new(
+            QueueStyle::FIFO,
+            1000,


this is a lot of messages, why do we need to buffer so many?

We really shouldn't need to buffer this many, especially if back pressure is working well. However, it looks like we'll hve to redo back pressure, so I'm inclined to just keep this for now and revisit it later.

this may consume significant amount of memory if not dequeue fast enough, imagine a fragment message with maximum message size (~64MB) * 1000

Yeah that's probably risky even with a single bad validator and quorum store off. Reduced to 50 for now.

zekun000 · 2022-12-11T09:46:44Z

consensus/src/network.rs

+                                    BlockStage::NETWORK_RECEIVED,
+                                );
+                            }
+                            if let Err(e) = self.consensus_messages_tx.push(


this piece of code seems repetitive

zekun000 · 2022-12-11T09:49:04Z

consensus/src/quorum_store/types.rs

+        }
+    }
+
+    pub(crate) fn take_transactions(self) -> Vec<SerializedTransaction> {


this is typically called into_transactions, take mostly refers to a &mut self function

zekun000 · 2022-12-11T09:51:02Z

consensus/src/quorum_store/types.rs

+#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq)]
+pub struct Batch {
+    pub(crate) source: PeerId,
+    // None is a request, Some(payload) is a response.


this seems hacky, we should just have two types

- Private for all members of "message" types. We will likely have to implement getters/setters when integrating QS implementation - Public for all members of *Info types. These are pure containers. - Remove (crate) where the mod is already pub(crate)

bchocho · 2022-12-16T23:50:38Z

@davidiw for these integration PRs the "compat" e2e test should be enough.

For quorum store as a whole, it will be enabled via onchain configs, and we plan to write forge tests that test the transition when the config is flipped.

bchocho · 2022-12-17T00:54:02Z

@zekun000 I iterated on your comments and also merged the latest changes in main. Please review!

zekun000 · 2022-12-17T05:00:19Z

consensus/src/network.rs

@@ -289,6 +344,11 @@ impl NetworkTask {
    ) -> (NetworkTask, NetworkReceivers) {
        let (consensus_messages_tx, consensus_messages) =
            aptos_channel::new(QueueStyle::LIFO, 1, Some(&counters::CONSENSUS_CHANNEL_MSGS));
+        let (quorum_store_messages_tx, quorum_store_messages) = aptos_channel::new(
+            QueueStyle::FIFO,
+            1000,


this may consume significant amount of memory if not dequeue fast enough, imagine a fragment message with maximum message size (~64MB) * 1000

github-actions · 2022-12-18T19:57:14Z

✅ Forge suite `land_blocking` success on `0b90884d4a47b1f2235f44fee95324c07a313b6f`

performance benchmark with full nodes : 5734 TPS, 6914 ms latency, 12500 ms p99 latency,(!) expired 940 out of 2449500 txns
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

github-actions · 2022-12-18T20:01:39Z

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `0b90884d4a47b1f2235f44fee95324c07a313b6f`

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 0b90884d4a47b1f2235f44fee95324c07a313b6f (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7240 TPS, 5339 ms latency, 6900 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 0b90884d4a47b1f2235f44fee95324c07a313b6f
compatibility::simple-validator-upgrade::single-validator-upgrade : 4510 TPS, 9229 ms latency, 12400 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 0b90884d4a47b1f2235f44fee95324c07a313b6f
compatibility::simple-validator-upgrade::half-validator-upgrade : 4163 TPS, 9873 ms latency, 13200 ms p99 latency,no expired txns
4. upgrading second batch to new version: 0b90884d4a47b1f2235f44fee95324c07a313b6f
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6232 TPS, 6293 ms latency, 11800 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 0b90884d4a47b1f2235f44fee95324c07a313b6f passed
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

bchocho force-pushed the brian/quorum-store-networking branch from a36cc36 to 4c22759 Compare December 8, 2022 23:28

bchocho commented Dec 9, 2022

View reviewed changes

bchocho marked this pull request as ready for review December 9, 2022 00:57

bchocho requested review from sasha8, gelash and zekun000 as code owners December 9, 2022 00:57

bchocho requested a review from igor-aptos December 9, 2022 00:57

sasha8 reviewed Dec 9, 2022

View reviewed changes

bchocho added 6 commits December 9, 2022 11:48

Quorum Store related changes to consensus networking.

286d34e

- Add messages for Fragment, Batch, SignedDigest, ProofOfStore - Add sender-only verification for some messages - Add a separate FIFO local channel for quorum store messages - Add and use broadcast without sending to self

do generate again

86ba9ee

some cleanup

ea2068a

Additional verify params

854a42d

Move quorum_store_enabled check to epoch_manager

8e27878

Add quorum store sender messages to QuorumStoreSender trait

cc72c96

bchocho force-pushed the brian/quorum-store-networking branch from 19adda4 to cc72c96 Compare December 9, 2022 19:49

sasha8 approved these changes Dec 10, 2022

View reviewed changes

davidiw reviewed Dec 11, 2022

View reviewed changes

zekun000 reviewed Dec 11, 2022

View reviewed changes

bchocho added 6 commits December 16, 2022 09:38

Add BatchRequest

c1d64c9

Clean up visibility

9897303

- Private for all members of "message" types. We will likely have to implement getters/setters when integrating QS implementation - Public for all members of *Info types. These are pure containers. - Remove (crate) where the mod is already pub(crate)

Some cleanup and fixes

0abd9fc

Merge branch 'main' into brian/quorum-store-networking

5a219d5

remove redundant code

eef6752

more cleanup

a762ec1

bchocho added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Dec 16, 2022

This comment has been minimized.

Sign in to view

regenerate record

404814e

This comment has been minimized.

Sign in to view

zekun000 approved these changes Dec 17, 2022

View reviewed changes

reduce queue size and add TODO

0b90884

bchocho enabled auto-merge (squash) December 18, 2022 19:13

This comment has been minimized.

Sign in to view

bchocho merged commit d264c3b into main Dec 18, 2022

bchocho deleted the brian/quorum-store-networking branch December 18, 2022 20:02

Markuze mentioned this pull request Dec 26, 2022

markuze/aptos perf #5998

Closed

Markuze mentioned this pull request Jan 3, 2023

[Network] Adding AptosPerf Client #6054

Closed

thepomeranian mentioned this pull request Apr 20, 2023

[AIP-26][Discussion]Quorum Store aptos-foundation/AIPs#108

Closed

[Quorum Store] networking integration #5779

[Quorum Store] networking integration #5779

Conversation

bchocho commented Dec 6, 2022 • edited Loading

Description

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sasha8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sasha8 Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sasha8 Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

sasha8 Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidiw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bchocho commented Dec 16, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bchocho commented Dec 17, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Dec 18, 2022

✅ Forge suite land_blocking success on 0b90884d4a47b1f2235f44fee95324c07a313b6f

github-actions bot commented Dec 18, 2022

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 0b90884d4a47b1f2235f44fee95324c07a313b6f

bchocho commented Dec 6, 2022 •

edited

Loading

sasha8 Dec 9, 2022 •

edited

Loading

sasha8 Dec 9, 2022 •

edited

Loading

sasha8 Dec 9, 2022 •

edited

Loading

✅ Forge suite `land_blocking` success on `0b90884d4a47b1f2235f44fee95324c07a313b6f`

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `0b90884d4a47b1f2235f44fee95324c07a313b6f`