Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

client/beefy: add some bounds on enqueued votes #12562

Merged
merged 20 commits into from
Dec 5, 2022
Merged

client/beefy: add some bounds on enqueued votes #12562

merged 20 commits into from
Dec 5, 2022

Conversation

dharjeezy
Copy link
Contributor

@dharjeezy dharjeezy commented Oct 25, 2022 •
edited
Loading

Introduces BoundedBTreeMap for pending_votes and pending_justifications
closes #12484

Polkadot address: 12ZNas89oEagaxLVNbpqmvfMxdrGrqN7gJKSpwthTUPZsrku

@dharjeezy dharjeezy marked this pull request as ready for review October 27, 2022 20:00
acatangiu
acatangiu reviewed Nov 10, 2022
View reviewed changes
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for contributing! I left some feedback below:

client/beefy/src/lib.rs Outdated Show resolved Hide resolved
client/beefy/src/lib.rs Outdated Show resolved Hide resolved
client/beefy/src/worker.rs Outdated Show resolved Hide resolved
client/beefy/src/worker.rs Outdated Show resolved Hide resolved
client/beefy/src/worker.rs Outdated Show resolved Hide resolved
client/beefy/src/worker.rs Outdated Show resolved Hide resolved
client/beefy/src/worker.rs Outdated Show resolved Hide resolved
client/beefy/src/worker.rs Outdated Show resolved Hide resolved
@acatangiu acatangiu added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Nov 16, 2022
@acatangiu
Copy link
Contributor

@dharjeezy do you need help with getting this over the finish line?

@dharjeezy
Copy link
Contributor Author

@dharjeezy do you need help with getting this over the finish line?

Apologies. I have been busy with personal stuffs. Will pick this up for this week. @acatangiu

@dharjeezy dharjeezy closed this Nov 22, 2022
@dharjeezy dharjeezy force-pushed the dharjeezy/bounds-on-enqueued-votes branch from 657ed78 to 06d7a23 Compare November 22, 2022 21:06
@dharjeezy
requested changes
9026803
@dharjeezy
Merge remote-tracking branch 'origin/dharjeezy/bounds-on-enqueued-vot…
cc7183b 
…es' into dharjeezy/bounds-on-enqueued-votes

# Conflicts:
#	client/beefy/src/lib.rs
#	client/beefy/src/worker.rs
@dharjeezy dharjeezy reopened this Nov 22, 2022
@dharjeezy dharjeezy marked this pull request as draft November 23, 2022 09:56
@dharjeezy
implement deref mut for BoundedBTreeMap
8391fd9 
convert BTreeMap to a BoundedBTreeMap
acatangiu
acatangiu reviewed Nov 24, 2022
View reviewed changes
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

client/beefy/src/worker.rs Outdated Show resolved Hide resolved
client/beefy/src/worker.rs Outdated Show resolved Hide resolved
@dharjeezy dharjeezy marked this pull request as ready for review November 24, 2022 12:41
@acatangiu
Copy link
Contributor

bot rebase

@paritytech-processbot
Copy link

Rebased

@acatangiu acatangiu added A0-please_review Pull request needs code review. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. and removed A3-in_progress Pull request is in progress. No review needed at this stage. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Nov 24, 2022
@acatangiu
Copy link
Contributor

bot rebase

@paritytech-processbot
Copy link

Rebased

svyatonik
svyatonik reviewed Dec 5, 2022
View reviewed changes
client/beefy/src/worker.rs
if self.pending_votes.len() < MAX_BUFFERED_VOTE_ROUNDS {
let votes_vec = self.pending_votes.entry(block_num).or_default();
if votes_vec.try_push(vote).is_err() {
warn!(target: "beefy", "🥩 Buffer vote dropped for round: {:?}", block_num)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just random thought: value in pending_votes map may store duplicated votes, right? So in theory malicious validator may try to send MAX_BUFFERED_VOTES_PER_ROUND duplicate votes to all other validators, blocking round from concluding. If that's true, then it is something that must be tracked during equivocation impl.

Copy link
Contributor

@acatangiu acatangiu Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's correct - added explicit comment for this to #12692

@acatangiu acatangiu mentioned this pull request Dec 5, 2022
Closed
@acatangiu acatangiu merged commit 05ebde1 into paritytech:master Dec 5, 2022
@acatangiu
Copy link
Contributor

/tip small

@substrate-tip-bot
Copy link

@acatangiu You are not allowed to request a tip. Only shawntabrizi, gavofyork, rphmeier, athei, andresilva, arkpar, bkchr, eskimor, drahnr, dvdplm, robbepop, cmichi, tomaka, pepyakin, tomusdrw, kianenigma, jacogr, rossbulat are allowed.

@dharjeezy
Copy link
Contributor Author

dharjeezy commented Dec 5, 2022 •
edited
Loading

/tip small

Thanks for the tip @acatangiu

@dharjeezy
Copy link
Contributor Author

dharjeezy commented Dec 5, 2022 •
edited
Loading

@acatangiu You are not allowed to request a tip. Only shawntabrizi, gavofyork, rphmeier, athei, andresilva, arkpar, bkchr, eskimor, drahnr, dvdplm, robbepop, cmichi, tomaka, pepyakin, tomusdrw, kianenigma, jacogr, rossbulat are allowed.

Can you help here? @shawntabrizi @bkchr

@AurevoirXavier AurevoirXavier mentioned this pull request Jan 10, 2023
Closed
@kacperzuk-neti kacperzuk-neti mentioned this pull request Jan 23, 2023
Merged
ltfschoen pushed a commit to ltfschoen/substrate that referenced this pull request Feb 22, 2023
@dharjeezy @acatangiu @ltfschoen
client/beefy: add some bounds on enqueued votes (paritytech#12562)
93b597f 
Introduce bounds on the justifications and votes queues, so they do not grow forever if voter cannot make progress and consume from them. When bounds are hit, new votes or justifications get dropped.

* use a BTreeMap and check for bounds

* cargo fmt

* use usize

Co-authored-by: Adrian Catangiu <adrian@parity.io>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@dharjeezy @acatangiu @ark0f
client/beefy: add some bounds on enqueued votes (paritytech#12562)
f3d837f 
Introduce bounds on the justifications and votes queues, so they do not grow forever if voter cannot make progress and consume from them. When bounds are hit, new votes or justifications get dropped.

* use a BTreeMap and check for bounds

* cargo fmt

* use usize

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@acatangiu acatangiu acatangiu approved these changes

@svyatonik svyatonik svyatonik approved these changes

@andresilva andresilva Awaiting requested review from andresilva

Assignees

@dharjeezy dharjeezy

Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

client/beefy: add some bounds on enqueued votes
3 participants
@dharjeezy @acatangiu @svyatonik