search

polkadot-fellows / runtimes

The various runtimes which make up the core subsystems of networks for which the Fellowship is represented.
GNU General Public License v3.0
126 stars 74 forks source link

Investigate too big change for benchmark `set_candidacy_bond` for 1.2.0 release #230

Closed bkontur closed 3 months ago

bkontur commented 3 months ago

Regenerated weights here: https://github.com/polkadot-fellows/runtimes/pull/223. Runtimes diff here: https://github.com/polkadot-fellows/runtimes/compare/release-v1.1.2...bkontur:runtimes:bko-weights AssetHubKusama pallet_colator_selection.rs weights diff: https://github.com/polkadot-fellows/runtimes/compare/release-v1.1.2...bkontur:runtimes:bko-weights#diff-fc25abf846969ae26257408784cdbc5c82c0b701bd859d06a26e99ef33c7bfcdL122-R147

subweight compare commits          --path-pattern "./**/weights/**/pallet_collator_selection.rs"          --format markdown --no-color           --change added changed removed          --method asymptotic --ignore-errors          remotes/polkadot-fellows/release-v1.1.2          origin/bko-weights
File Extrinsic Old New Change [%]
system-parachains/collectives/collectives-polkadot/src/weights/pallet_collator_selection.rs set_candidacy_bond 105.07us 21.21ms +20087.24
system-parachains/bridge-hubs/bridge-hub-polkadot/src/weights/pallet_collator_selection.rs set_candidacy_bond 104.95us 21.18ms +20076.22
system-parachains/bridge-hubs/bridge-hub-kusama/src/weights/pallet_collator_selection.rs set_candidacy_bond 104.99us 21.18ms +20073.76
system-parachains/asset-hubs/asset-hub-polkadot/src/weights/pallet_collator_selection.rs set_candidacy_bond 104.91us 21.16ms +20071.15
system-parachains/asset-hubs/asset-hub-kusama/src/weights/pallet_collator_selection.rs set_candidacy_bond 105.19us 21.18ms +20029.38

TODO

bkontur commented 3 months ago

image

georgepisaltu commented 3 months ago

It's a normal increase because now the extrinsic iterates over the candidate list and kicks candidates which don't meet the new bond requirement. Previously the function only did a storage write and candidates were grandfathered into the list through this loophole.

However, when the bond decreases there are no iterations, but we still charge for the length of the candidate list as if we iterated over all the candidates but kicked none. In this particular case, we should refund weight as if the list was empty. This needs to be fixed in pallet_collator_selection::set_candidacy_bond.

The weight increase isn't too much of a concern because the extrinsic is guarded by a privileged origin pallet_collator_selection::Config::UpdateOrigin and it's called very rarely.

georgepisaltu commented 3 months ago

However, when the bond decreases there are no iterations, but we still charge for the length of the candidate list as if we iterated over all the candidates but kicked none. In this particular case, we should refund weight as if the list was empty. This needs to be fixed in pallet_collator_selection::set_candidacy_bond.

Proposed fix https://github.com/paritytech/polkadot-sdk/pull/3643

bkontur commented 3 months ago

@georgepisaltu thank you, closing this