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 #12484

Closed
Tracked by #1632
acatangiu opened this issue Oct 12, 2022 · 8 comments · Fixed by #12562
Closed
Tracked by #1632

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

acatangiu opened this issue Oct 12, 2022 · 8 comments · Fixed by #12562
Assignees
Labels
I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. U3-nice_to_have Issue is worth doing eventually. Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder

Comments

@acatangiu
Copy link
Contributor

When BEEFY voter runs behind rest of voters it will enqueue gossiped votes and process them as it catches up.

We should introduce some bounds on this queue to not grow forever if voter is stuck - we should instead drop votes once bounds are hit.

@acatangiu acatangiu added I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. U3-nice_to_have Issue is worth doing eventually. Z2-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. labels Oct 12, 2022
@acatangiu acatangiu added this to BEEFY Oct 12, 2022
@acatangiu acatangiu moved this to Need for Kusama 🗒 in BEEFY Oct 12, 2022
@acatangiu acatangiu moved this to Draft in Parity Roadmap Oct 12, 2022
@acatangiu acatangiu moved this from Draft to Open in Parity Roadmap Oct 12, 2022
@dharjeezy
Copy link
Contributor

@acatangiu I will like to work on this

@MrishoLukamba
Copy link
Contributor

am working on it

@dharjeezy
Copy link
Contributor

am working on it

Oh... I already started @MrishoLukamba

@MrishoLukamba
Copy link
Contributor

Okey continue

@dharjeezy
Copy link
Contributor

So, I am currently working on this, @acatangiu

I am imploring the use of a BoundedBTreeMap for the buffer to hold votes
BoundedBTreeMap<NumberFor<B>, Vec<VoteMessage<NumberFor<B>, AuthorityId, Signature>>, ConstU32<U>> where U is a const type parameter.

There is a to_process_for_bound which is inside try_pending_justif_and_votes which i will need to split due to the new structure that the pending_votes is attaining with the BoundedBTreeMap.

is it ok to split the function? or should I go ahead to make pending_votes and also pending_justifications a BoundedBTreeMap structure?

I am trying to ask so as to confirm if i am on the right track. @acatangiu

@acatangiu
Copy link
Contributor Author

Yes, the approach is good and you should put bounds on both pending_votes and pending_justifications.

@acatangiu
Copy link
Contributor Author

You should also bound inner vec in pending_votes, use BoundedVec or some simple custom vec wrapper for BoundedBTreeMap<NumberFor<B>, BoundedVec<_, _>>.

Bounds sizes should be smth in the order of a few MBs for each map - just define some constants, document them and we can fine tune the value as a final detail.

For the inner votes Vec it would be great to have some way to configure bound based on authority_set size (basically make it runtime configurable instead of hardcoded const).

When bounds are reached:

  • votes can be dropped with a warn! log,
  • justifications can be dropped with a error! log (in the future we might even decide to stop the voter with "fatal" error for dropping justifications, but for now let's start with logging errors).

@dharjeezy
Copy link
Contributor

dharjeezy commented Oct 24, 2022

You should also bound inner vec in pending_votes, use BoundedVec or some simple custom vec wrapper for BoundedBTreeMap<NumberFor<B>, BoundedVec<_, _>>.

Bounds sizes should be smth in the order of a few MBs for each map - just define some constants, document them and we can fine tune the value as a final detail.

For the inner votes Vec it would be great to have some way to configure bound based on authority_set size (basically make it runtime configurable instead of hardcoded const).

When bounds are reached:

  • votes can be dropped with a warn! log,
  • justifications can be dropped with a error! log (in the future we might even decide to stop the voter with "fatal" error for dropping justifications, but for now let's start with logging errors).

Alright @acatangiu
I Will open a PR soon enough

@acatangiu acatangiu moved this from Need for Kusama 🗒 to In Progress 🛠 in BEEFY Nov 14, 2022
@acatangiu acatangiu moved this from In Progress 🛠 to Code in review 🧐 in BEEFY Nov 16, 2022
@acatangiu acatangiu added Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder and removed Z2-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. labels Nov 24, 2022
Repository owner moved this from Code in review 🧐 to Done ✅ in BEEFY Dec 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. U3-nice_to_have Issue is worth doing eventually. Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants