feat: dynamic min authority count

[msgmaxim]

Pull Request

Closes: PRO-960

Checklist

Please conduct a thorough self-review before opening the PR.

• I am confident that the code works.
• I have updated documentation where appropriate.

Summary

Now the rotation will be aborted (resulting in a new auction) if the number of non-banned candidates goes below MAX_AUTHORITY_SET_CONTRACTION_PERCENT * current_authority_size() (previously it would only do so when reaching min_size from auction parameters). Hopefully this is what you had in mind @dandanlen.

• Do we want MAX_AUTHORITY_SET_CONTRACTION_PERCENT to be configurable (currently set to 70%)? Do we maybe want it as part of SetSizeParameters next to max_expanstion?

Note this is not the same as min auction resolution size.

Wouldn't it also make sense to apply this to auction resolution? For example, looks like you can be disqualified due to missed heartbeats, which I imagine could also be manipulated with DOS attacks (or there could even be a bug in our code resulting in a large number of nodes not being able to submit heartbeats).

[linear]

PRO-960 Authority set min size should be based on % decrease

We currently use a constant minimum authority set size of 2.

This is risky because it may open us up to DOS attacks whereby the number of available keygen participants are reduced to point where the attacker has a supermajority.

We should instead calculate the min set size dynamically such that the total set size cannot decrease below some % of its previous size.

Note this is not the same as min auction resolution size. The auction resolves with some set of participants, and those are then whittled down until a successful keygen (using secondary candidates as appropriate).

[kylezs]

There's an extra commit in here?

[dandanlen]

I would prefer if this was a storage value that can be updated by governance (can be added to the update_pallet_config extrinsic).

Also, instead of a raw u32, we can use the Percent type from substrate that will ensure the value is within a suitable range.

[msgmaxim]

Updated to use Percent.

I would prefer if this was a storage value that can be updated by governance (can be added to the update_pallet_config extrinsic).

I thought about that too, hence my question in the description. I now changed it to be a storage item that can be updated with update_pallet_config, as you suggested.

Also, did you see this part in the PR description:

Wouldn't it also make sense to apply this to auction resolution? For example, looks like you can be disqualified due to missed heartbeats, which I imagine could also be manipulated with DOS attacks (or there could even be a bug in our code resulting in a large number of nodes not being able to submit heartbeats).

[dandanlen]

Wouldn't it also make sense to apply this to auction resolution? For example, looks like you can be disqualified due to missed heartbeats, which I imagine could also be manipulated with DOS attacks (or there could even be a bug in our code resulting in a large number of nodes not being able to submit heartbeats).

Sorry I forgot to comment on this.

We always include all bidders in the auction resolution, and apply qualification criteria after that. So I think (if i understand your concern correctly) that we don't want to restrict at auction resolution.

[msgmaxim]

We always include all bidders in the auction resolution, and apply qualification criteria after that.

This looks to me like we exclude unqualified nodes before resolving auction (and it makes sense):

resolver.resolve_auction(
    T::BidderProvider::get_qualified_bidders::<T::KeygenQualification>(),
    AuctionBidCutoffPercentage::<T>::get(),
)

So I think (if i understand your concern correctly) that we don't want to restrict at auction resolution.

Looks like there are two concerns:

1. You can target high bidding nodes and reduce MAB. Not sure yet how to address this, or whether we even care. At least in theory, is it possible to force all nodes to fail qualificaiton (with an unreasonably large DDOS attack or, more likily, by exploiting some bug) and register a large number of low-bidding nodes that will overtake the network? Not sure if we have a mechanism right now to prevent MAB from dropping too low.

2. You can target bidding nodes to reduce the size of the quailified set to as low as SetSizeParameters::min_size. This is not really a problem now because this PR would prevent us from proceeding to perform a keygen/rotation, but I was wondering why we don't also put this check in resolve_auction, and what is the actual meaning of SetSizeParameters::min_size now (other than preventing the set size from dropping to 0 essentially). I'm guessing the idea is to raise it from 2 to something more reasonable for mainnet?

[msgmaxim]

There's an extra commit in here?

Yeah, will rebase to remove it (since the PR is small). Thanks.

[msgmaxim]

I now addresed all comments @dandanlen. Since there wasn't much in the initial PR, I ended up rebasing to keep things clear.

[codecov]

Codecov Report

Merging #4224 (3e6920a) into main (22e9288) will increase coverage by 0%.
The diff coverage is 82%.

@@          Coverage Diff          @@
##            main   #4224   +/-   ##
=====================================
  Coverage     72%     72%           
=====================================
  Files        384     384           
  Lines      62967   63007   +40     
  Branches   62967   63007   +40     
=====================================
+ Hits       45023   45052   +29     
- Misses     15609   15615    +6     
- Partials    2335    2340    +5     

Files	Coverage Δ
state-chain/cf-integration-tests/src/funding.rs	97% <100%> (+<1%)	⬆️
...ate-chain/cf-integration-tests/src/mock_runtime.rs	100% <100%> (ø)
state-chain/pallets/cf-validator/src/mock.rs	95% <100%> (+<1%)	⬆️
state-chain/pallets/cf-validator/src/tests.rs	98% <100%> (+<1%)	⬆️
state-chain/primitives/src/lib.rs	58% <ø> (ø)
state-chain/node/src/chain_spec.rs	1% <0%> (-<1%)	⬇️
state-chain/pallets/cf-validator/src/lib.rs	73% <69%> (-<1%)	⬇️

... and 4 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

[dandanlen]

I force-pushed again with signed commits so we can merge this.