substrate icon indicating copy to clipboard operation
substrate copied to clipboard

client/beefy: add some bounds on enqueued votes

Open acatangiu opened this issue 3 years ago • 3 comments

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 avatar Oct 12 '22 16:10 acatangiu

@acatangiu I will like to work on this

dharjeezy avatar Oct 15 '22 00:10 dharjeezy

am working on it

MrishoLukamba avatar Oct 18 '22 17:10 MrishoLukamba

am working on it

Oh... I already started @MrishoLukamba

dharjeezy avatar Oct 18 '22 17:10 dharjeezy

Okey continue

MrishoLukamba avatar Oct 18 '22 18:10 MrishoLukamba

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

dharjeezy avatar Oct 20 '22 18:10 dharjeezy

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

acatangiu avatar Oct 21 '22 08:10 acatangiu

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).

acatangiu avatar Oct 21 '22 08:10 acatangiu

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

dharjeezy avatar Oct 24 '22 09:10 dharjeezy