refactor(consensus): minor refactor of the manager

[matan-starkware]

1. Separate fn for handling cached messages per height
2. Return errors instead of panic when receiving messages

---

This change is 

[matan-starkware]

• refactor(consensus): minor refactor of the manager #208 👈
• refactor(consensus): create manager which encapsulates messages cached between heights #200
• refactor(consensus): consensus takes a generic network receiver #249
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @matan-starkware and the rest of your teammates on Graphite

[codecov]

Codecov Report

Attention: Patch coverage is 58.82353% with 14 lines in your changes missing coverage. Please review.

Project coverage is 76.84%. Comparing base (fe93a3b) to head (c4043fa).
Report is 1 commits behind head on main.

Files	Patch %	Lines
crates/sequencing/papyrus_consensus/src/manager.rs	60.60%	11 Missing and 2 partials ⚠️
crates/papyrus_protobuf/src/converters/mod.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #208      +/-   ##
==========================================
- Coverage   76.89%   76.84%   -0.05%     
==========================================
  Files         316      316              
  Lines       34504    34550      +46     
  Branches    34504    34550      +46     
==========================================
+ Hits        26532    26551      +19     
- Misses       5687     5708      +21     
- Partials     2285     2291       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[asmaastarkware]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ShahakShama)

[ShahakShama]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @matan-starkware)

---

crates/sequencing/papyrus_consensus/src/manager.rs line 81 at r2 (raw file):

            // TODO(matan): We need to figure out an actual cacheing strategy under 2 constraints:
            // 1. Malicious - must be capped so a malicious peer can't DoS us.

DoS -> DDoS

---

crates/sequencing/papyrus_consensus/src/manager.rs line 115 at r2 (raw file):

    fn get_current_height_messages(&mut self, height: BlockNumber) -> Vec<ConsensusMessage> {
        let mut current_height_messages = Vec::new();
        for msg in std::mem::take(&mut self.cached_messages) {

Try to remove just the problematic messages, instead of recreating the entire vector.
Plus, consider using a different data structure (maybe HashSet, or HashMap between height and a vector of ConsensusMessages and then finding all of them is much easier)

---

crates/sequencing/papyrus_consensus/src/manager.rs line 117 at r2 (raw file):

        for msg in std::mem::take(&mut self.cached_messages) {
            match height.0.cmp(&msg.height()) {
                std::cmp::Ordering::Less => self.cached_messages.push(msg),

IDK why, but logically it makes more sense to put msg.height() in the left of the comparison. I guess it's because it changs this arm into Greater, which means you're saying "This message is higher than the current height", which makes more sense since the current height is constant during this entire function and what's changing is msg

---

crates/sequencing/papyrus_consensus/src/types.rs line 167 at r2 (raw file):

    Canceled(#[from] oneshot::Canceled),
    #[error(transparent)]
    ProtobufConversionError(#[from] ProtobufConversionError),

This should be handled by reporting the relevant peer and then continuing on (maybe voting nil on the proposal if the sending peer is the proposer). For now put a TODO to handle this error internally

[matan-starkware]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ShahakShama)

---

crates/sequencing/papyrus_consensus/src/manager.rs line 81 at r2 (raw file):

Previously, ShahakShama wrote…

DoS -> DDoS

Why? A single malicious node could potentially send me so many messages that I OOM. There is no need for this to be a distributed attack.

---

crates/sequencing/papyrus_consensus/src/manager.rs line 115 at r2 (raw file):

Previously, ShahakShama wrote…

Try to remove just the problematic messages, instead of recreating the entire vector.
Plus, consider using a different data structure (maybe HashSet, or HashMap between height and a vector of ConsensusMessages and then finding all of them is much easier)

I intentionally went for a simple solution here (with Dan't support). I am not trying to optimize this since we know that this is not the final solution. This is just a solution for the sake of our early simulations. I'd rather just wait till we implement the real solution which is still under debate.

---

crates/sequencing/papyrus_consensus/src/manager.rs line 117 at r2 (raw file):

Previously, ShahakShama wrote…

IDK why, but logically it makes more sense to put msg.height() in the left of the comparison. I guess it's because it changs this arm into Greater, which means you're saying "This message is higher than the current height", which makes more sense since the current height is constant during this entire function and what's changing is msg

Done.

---

crates/sequencing/papyrus_consensus/src/types.rs line 167 at r2 (raw file):

Previously, ShahakShama wrote…

This should be handled by reporting the relevant peer and then continuing on (maybe voting nil on the proposal if the sending peer is the proposer). For now put a TODO to handle this error internally

Reviewable isn't highlighting, but I presume you are talking about InvalidProposal? If so you are basically describing the current milestone that Asmaa just wrote (code is already under review with me). I'll just leave this alone then, ok?

[ShahakShama]

Reviewed 3 of 5 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matan-starkware)

---

crates/sequencing/papyrus_consensus/src/manager.rs line 115 at r2 (raw file):

Previously, matan-starkware wrote…

I intentionally went for a simple solution here (with Dan't support). I am not trying to optimize this since we know that this is not the final solution. This is just a solution for the sake of our early simulations. I'd rather just wait till we implement the real solution which is still under debate.

But my suggestion is as simple as your suggestion.
Unblocking for now, but I think it's worth doing this change or at least putting a TODO

---

crates/sequencing/papyrus_consensus/src/types.rs line 167 at r2 (raw file):

Previously, matan-starkware wrote…

Reviewable isn't highlighting, but I presume you are talking about InvalidProposal? If so you are basically describing the current milestone that Asmaa just wrote (code is already under review with me). I'll just leave this alone then, ok?

No, I'm talking about ProtobufConversionError, but I guess the same applies for InvalidProposal
You can leave this alone if you want

[matan-starkware]

Reviewable status: 1 of 5 files reviewed, 1 unresolved discussion (waiting on @ShahakShama)

---

crates/sequencing/papyrus_consensus/src/manager.rs line 115 at r2 (raw file):

Previously, ShahakShama wrote…

But my suggestion is as simple as your suggestion.
Unblocking for now, but I think it's worth doing this change or at least putting a TODO

Took your map suggestion

---

crates/sequencing/papyrus_consensus/src/types.rs line 167 at r2 (raw file):

Previously, ShahakShama wrote…

No, I'm talking about ProtobufConversionError, but I guess the same applies for InvalidProposal
You can leave this alone if you want

I can't do anything with a protobuf conversion error (besides ReportSender to network). IDK if this is a proposal or a prevote, idk what height this is for, idk who sent it. Consensus proper must ignore.

I'll update the manager though to use report sender and add a TODO since at some point we will want the SHC to also start reporting errors.

[asmaastarkware]

Reviewed 1 of 4 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @matan-starkware and @ShahakShama)

---

crates/sequencing/papyrus_consensus/src/manager.rs line 82 at r6 (raw file):

                    self.cached_messages
                        .entry(message.height())
                        .or_insert(Vec::new())

.or_default()

Code quote:

.or_insert(Vec::new())

[matan-starkware]

Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @asmaastarkware and @ShahakShama)

---

crates/sequencing/papyrus_consensus/src/manager.rs line 82 at r6 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

.or_default()

Done.

[asmaastarkware]

Reviewed 2 of 4 files at r5, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ShahakShama)

[matan-starkware]

Merge activity

• Aug 5, 7:55 AM EDT: @matan-starkware started a stack merge that includes this pull request via Graphite.
• Aug 5, 7:55 AM EDT: @matan-starkware merged this pull request with Graphite.