Fix offchain election to respect the weight #7215
Conversation
kianenigma
requested review from
gavofyork,
tomusdrw,
shawntabrizi and
gui1117
kianenigma
added
A0-please_review
|
This PR mostly touches offchain which is currently in crisis anyway, so I didn't review carefully yet, but the only on-chain change is making the dispatchable |
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
…bstrate into kiz-staking-OCW-check-size
kianenigma
added
the
D9-needsaudit 👮
| if step == 0 { | ||
| // We might have reduced less than expected due to rounding error. Reduce one last time. | ||
| while weight_with(voters) > max_weight { | ||
| voters -= 1; |
There was a problem hiding this comment.
This may underflow? if weight_with(0) > max_weight.
There was a problem hiding this comment.
Yes this is a bit buggy
There was a problem hiding this comment.
Fixed! Have a look at the test cases as well.
| } | ||
| // For the opposite case. | ||
| while weight_with(voters + 1) < max_weight { | ||
| voters += 1; |
There was a problem hiding this comment.
This should be bounded by size.nominators - we may reach this point if weight_with(size.nominators) < max_weight, no?
There was a problem hiding this comment.
Yes the final return is bounded, but I will add it here as well.
There was a problem hiding this comment.
nothing as changed in frame/system/src/extensions/check_weight.rs isn't it ?
apart from #7215 (comment)
EDIT: and also a guard so that only one transaction can be submitted by block author (as Tomasz said)
looks good to me
gui1117
dismissed
their stale review
I'm not certain about mandatory unsigned extrinsic
No, just variable names etc. |
|
We are done with our review for this PR and don't see any issues with it so far. We are happy to take another look once the discussion around setting the OffchainSolutionWeightLimit is resolved. |
kianenigma
added
D1-audited 👍
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
…bstrate into kiz-staking-OCW-check-size
…king-OCW-check-size
|
bot merge |
|
Trying merge. |
ghost
merged commit 
* Mockup * Fix offchain election to respect the weight * Fix builds a bit * Update frame/staking/src/offchain_election.rs Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> * Update frame/staking/src/offchain_election.rs Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> * Make it build, binary search * Fix a number of grumbles * one more fix. * remove unwrap. * better alg. * Better alg again. * Final fixes * Fix * Rollback to normal * Final touches. * Better tests. * Update frame/staking/src/lib.rs Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com> * Proper maxExtWeight * Final fix * Final fix for the find_voter Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
…up-updates * master: Async keystore + Authority-Discovery async/await (#7000) Fixes logging of target names with dashes (#7281) seal: Add automated weights for contract API calls (#7017) add ss58 id for nodle (#7279) Refactor CurrencyToVote (#6896) bump-allocator: document & poison (#7277) Reset flaming fir network (#7274) reschedule (#6860) Drop system cache for trie benchmarks (#7242) client: improve log formatting (#7272) Rework `InspectState` (#7271) SystemOrigin trait (#7226) Update ss58 registry for Dock network (#7263) .maintain/monitoring: Add alert when continuous task ends (#7250) Rename `TRANSACTION_VERSION` to `EXTRINSIC_VERSION` (#7258) Split block announce processing into two parts (#6958) Fix offchain election to respect the weight (#7215)
Closes #7170, hopefully partially fix some recent issues in polkadot. How? Using this:
Full writeup of the situation: https://hackmd.io/sP9KlLUBRpukfhL6GyPa2w
I don't have any tests for this. But I've tested it against both polkadot and kusama. It seems like actually a lot of the nominators that we have are quite insignificant, meaning that they have very small values. Here's a log from my test script that uses this branch. This is in a case where we allow 90% of the block weight to be consumed by this.
As you see, the solution hasn't changed much. The first element which is the most important is actually the same. The second, which is the total amount at stake, is indeed reduced.
This is on Polkadot, and essentially means a nominator cannot be effective with a stake less than 10_017_683_661, 10 new DOTs if I am not mistaking.
Also, I made the transaction
Operationalso that it can consume more of the weight.polkadot companion: paritytech/polkadot#1768
TODOs:
UnsignedPriorityis high enough so that this transaction is almost always the first one to be put in the block (and effectively preventing anything else from getting into the block. )OffchainSolutionWeightLimitis correct. Otherwise this whole PR is moot.