Conversation
|
/cmd prdoc --audience runtime_dev --bump patch |
…time_dev --bump patch'
|
/cmd prdoc --audience runtime_dev --bump minor --force |
…time_dev --bump minor --force'
cirko33
added
the
A4-backport-stable2603
|
Few comments:
|
|
/cmd bench --pallet pallet_staking_async --runtime asset-hub-westend |
|
Command "bench --pallet pallet_staking_async --runtime asset-hub-westend" has started 🚀 See logs here |
…t_staking_async --runtime asset-hub-westend'
|
Command "bench --pallet pallet_staking_async --runtime asset-hub-westend" has finished ✅ See logs here DetailsSubweight results:
Command output:✅ Successful benchmarks of runtimes/pallets: |
| // Minimum execution time: 151_823_000 picoseconds. | ||
| Weight::from_parts(162_698_000, 0) | ||
| .saturating_add(Weight::from_parts(0, 0)) | ||
| // Estimated: `4218` |
There was a problem hiding this comment.
@ggwpez - weights generated by #10794 seemed a bit off. Bunch of Estimated: 0 for example etc
This PR doesn't introduce significant changes and affects only few benchmarks where we actually access the OffenceQueueEras and as expected - we are seeing e.g.
Staking::OffenceQueueEras max_size increased: 9 → 17 (added: 504 → 512) This is the change from this branch — the raised offence queue limit.
For the rest the results are basically very close to what we have on chain before #10794
There was a problem hiding this comment.
Hm weird... I think there is a non-determinism bug somewhere in the PoV benchmarking, but its highly unlikely to trigger for all extrinsics like it did in that MR.
No idea honestly, but can look into it when i have time.
@kianenigma , @Ank4n wdyt? Kian, in the related issue you were hinting as raising the bound as proper solution. I am fine with that, I would prefer the conversion to Vec + force_from approach but I would like to understand the reason behind your preference (or it was mostly because it was good enough and faster to implement?) |
| impl<T: Config> Get<u32> for OffenceQueueErasBound<T> { | ||
| fn get() -> u32 { | ||
| let bonding_duration = T::BondingDuration::get(); | ||
| bonding_duration.saturating_add(bonding_duration) |
There was a problem hiding this comment.
Maybe we should take a min of this and something like 10, so its never 0.
There was a problem hiding this comment.
Actually, we don't need to double it, we just need to add few eras so the system has time to clear the offences. So, something likebonding_duration.saturating_add(10) will do.
IMO this is a non-issue. I don't see how we can exceed In any case, I don't see any harm in going with this solution . Also benchmark should not really get affected because we still have bonding duration worth of eras. |
IIUC, this is true for polkadot / any real current runtime configuration. Let's consider this example:
In this case:
The check here discards new offences With After three offence reports arrive (one each for eras 0, 1, and 2), A fourth offence report for era 3 arrives => it is inserted into Do I get it right? If my example makes sense (maybe it's just theoretical / not plausible / not possible / I get something seriously wrong - all very possible 😄 ), then this PR makes the code more robust and maybe it's worth adding a similar test to prove it. |
I think you are right. This is the corner case (buggy line). When When I think we should fix the range for |
We should make this |
that should work indeed. Again, there is always the other option to convert to Vec + |
I'll quote Kian's answer (that makes kind of sense to me):
|
IMO (but not a strong opinion 😆 ) |
I don't disagree 😄 Well, to be fair, I consider this whole weak bounded vector a debatable hack by itself, not only |
I think |
| // this implies that slashes are applied immediately, so we can accept any offence up to | ||
| // bonding duration old. | ||
| active_era.index.saturating_sub(T::BondingDuration::get()) | ||
| active_era.index.saturating_sub(T::BondingDuration::get()).saturating_add(2) |
There was a problem hiding this comment.
We should also add a comment explaining the magic number 2, reduces the range to BondingDuration - 1. An extra buffer era ensures old offences have time to be cleared out.
5 participants
Raise offence queue
BondingDurationlimit to ensure no loss of offences.