Rerun Staking OCW periodically. #7976

kianenigma · 2021-01-25T13:48:45Z

Fix issues such as https://github.com/paritytech/stakingops/issues/130

Our OCW runner in staking is too conservative now. It only runs once at the beginning of the election window, and only attempts to submit once. If for any reason (e.g. secondary/primary slot overlap, fork and rollback) all validators temporarily think that a solution is submitted, they will all drop their computed solution and never try again. In essence, they will attempt to submit it, which will fail because there is already a good solution on-chain, and the process ends. In this scenario, if the submitted solution gets reverted, we are left with nothing, and thus a heavy on-chain election.

This is a minimal patch to fix the situation. OCW will compute, save and submit the solution initially, and until the end of the election window periodically try to re-submitted the saved solution, if none is stored yet.

Note that I tried to make this patch minimal, as #7909 will have to reintroduce this again.

This needs a burn-in, but not a typical one. I will prepare a patch to the current polkadot release that contains this runtime change but does not bump the spec version. This is okay because there is no runtime change, and the new runtime should work with the old one. Then we can run this first on a full node, and the once we are confident, a validator node. -l=staking=trace is important in all stages.

Polkadot dry-run branch: https://github.com/paritytech/polkadot/tree/kiz-staking-OCW-rerun-dryrun-polkadot
Created using: https://hackmd.io/MMGn9TnKSx2oganhfcQrpw

…king-OCW-rerun

tomusdrw

if the submitted solution gets reverted, we are left with nothing, and thus a heavy on-chain election

Under what conditions can the solution be reverted?

frame/staking/src/offchain_election.rs

frame/staking/src/lib.rs

This reverts commit b2e93d9.

tomusdrw

Looks good! So the assumption right now is that:

If you run OCW at the election window opening block, you always attempt to compute a solution.
When the window stays open AND there is no submission already, you attempt to re-submit your solution.

In theory we could attempt to submit if we've found a better solution, but we can leave that as a future improvement, cause afaiu the main goal right now is to ensure that there is ANY solution to never fall back to on-chain process.

frame/staking/src/offchain_election.rs

bkchr

How complicated would it to write a test for this? :D

frame/staking/src/offchain_election.rs

frame/staking/src/lib.rs

kianenigma · 2021-02-01T15:44:51Z

How complicated would it to write a test for this? :D

Probably in the order that I'd say given the dry-run, not needed 😄 but I will give it a try before merge.

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

…te into kiz-staking-OCW-rerun

kianenigma · 2021-02-08T18:37:23Z

Burnin looks well, will leave it running for another day with full logs to check everything, else we're good to go.

…king-OCW-rerun

kianenigma · 2021-02-24T14:40:36Z

I see it very likely that we will close this and just re-introduce it in the multi-phase election pallet, given that the audit has taken a while.

kianenigma · 2021-03-05T05:29:09Z

Closing this since will be re-introduced by @coriolinus soon.

This is a much smaller diff than that PR contained, but I think it contains all the essentials.

* not climate * explain the intent of the bool in the unsigned phase * remove glob imports from unsigned.rs * add OffchainRepeat parameter to ElectionProviderMultiPhase * migrate core logic from #7976 This is a much smaller diff than that PR contained, but I think it contains all the essentials. * improve formatting * fix test build failures * cause test to pass * Apply suggestions from code review Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * collapse imports * threshold acquired directly within try_acquire_offchain_lock * add test of resubmission after interval * add test that ocw can regenerate a failed cache when resubmitting * ensure that OCW solutions are of the correct round This should help prevent stale cached solutions from persisting past the election for which they are intended. * add test of pre-dispatch round check * use `RawSolution.round` instead of redundantly externally * unpack imports Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * rename `OFFCHAIN_HEAD_DB` -> `OFFCHAIN_LOCK` * rename `mine_call` -> `mine_checked_call` * eliminate extraneous comma * check cached call is current before submitting * remove unused consts introduced by bad merge. Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com> * resubmit when our solution beats queued solution * clear call cache if solution fails to submit * use local storage; clear on ElectionFinalized * Revert "use local storage; clear on ElectionFinalized" This reverts commit 4b46a93. * BROKEN: try to filter local events in OCW * use local storage; simplify score fetching * fix event filter * mutate storage instead of setting it * StorageValueRef::local isn't actually implemented yet * add logging for some events of interest in OCW miner * rename kill_solution -> kill_ocw_solution to avoid ambiguity * defensive err instead of unreachable given unreachable code * doc punctuation Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * distinguish miner errors between "out of date" and "call invalid" * downgrade info logs -> debug * ensure encoded call decodes as a call * fix semantics of validation of pre-dispatch failure for wrong round * move score check within `and_then` * add test that offchain workers clear their cache after election * ensure that bad ocw submissions are not retained for resubmission * simplify fn ocw_solution_exists * add feasibility check when restoring cached solution should address https://github.com/paritytech/substrate/pull/8290/files#r617533358 restructures how the checks are sequenced, which simplifies legibility. * simplify checks again Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

* not climate * explain the intent of the bool in the unsigned phase * remove glob imports from unsigned.rs * add OffchainRepeat parameter to ElectionProviderMultiPhase * migrate core logic from paritytech#7976 This is a much smaller diff than that PR contained, but I think it contains all the essentials. * improve formatting * fix test build failures * cause test to pass * Apply suggestions from code review Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * collapse imports * threshold acquired directly within try_acquire_offchain_lock * add test of resubmission after interval * add test that ocw can regenerate a failed cache when resubmitting * ensure that OCW solutions are of the correct round This should help prevent stale cached solutions from persisting past the election for which they are intended. * add test of pre-dispatch round check * use `RawSolution.round` instead of redundantly externally * unpack imports Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * rename `OFFCHAIN_HEAD_DB` -> `OFFCHAIN_LOCK` * rename `mine_call` -> `mine_checked_call` * eliminate extraneous comma * check cached call is current before submitting * remove unused consts introduced by bad merge. Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com> * resubmit when our solution beats queued solution * clear call cache if solution fails to submit * use local storage; clear on ElectionFinalized * Revert "use local storage; clear on ElectionFinalized" This reverts commit 4b46a93. * BROKEN: try to filter local events in OCW * use local storage; simplify score fetching * fix event filter * mutate storage instead of setting it * StorageValueRef::local isn't actually implemented yet * add logging for some events of interest in OCW miner * rename kill_solution -> kill_ocw_solution to avoid ambiguity * defensive err instead of unreachable given unreachable code * doc punctuation Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * distinguish miner errors between "out of date" and "call invalid" * downgrade info logs -> debug * ensure encoded call decodes as a call * fix semantics of validation of pre-dispatch failure for wrong round * move score check within `and_then` * add test that offchain workers clear their cache after election * ensure that bad ocw submissions are not retained for resubmission * simplify fn ocw_solution_exists * add feasibility check when restoring cached solution should address https://github.com/paritytech/substrate/pull/8290/files#r617533358 restructures how the checks are sequenced, which simplifies legibility. * simplify checks again Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

kianenigma added 4 commits January 21, 2021 08:16

boilerplate

2ba0ee6

Finish

363d03a

Log cleanup.

3941513

Merge branch 'master' of github.com:paritytech/substrate into kiz-sta…

ec06724

…king-OCW-rerun

document it.

8613776

paritytech-ci added the D2-breaksapi label Jan 25, 2021

kianenigma changed the title ~~Kiz staking ocw rerun~~ Rerun Staking OCW periodically. Jan 25, 2021

kianenigma requested review from tomusdrw, gui1117 and bkchr January 25, 2021 14:38

Remove unused.

e5e1725

tomusdrw reviewed Jan 25, 2021

View reviewed changes

frame/staking/src/offchain_election.rs Outdated Show resolved Hide resolved

frame/staking/src/lib.rs Outdated Show resolved Hide resolved

kianenigma added 5 commits January 26, 2021 09:16

Use a better match to ensure we don't run old

dd4eb08

reformat a bit into a simpler form

b2e93d9

Revert "reformat a bit into a simpler form"

2b96aa4

This reverts commit b2e93d9.

Fix order

4072fd6

Last touches.

0b338a2

tomusdrw approved these changes Jan 26, 2021

View reviewed changes

frame/staking/src/offchain_election.rs Outdated Show resolved Hide resolved

frame/staking/src/offchain_election.rs Show resolved Hide resolved

kianenigma added 3 commits January 27, 2021 07:49

Better loggin

960fecb

Final grumbles.

843d989

Fix doc

7b9af84

github-actions bot added the A7-needspolkadotpr label Jan 27, 2021

bkchr approved these changes Jan 27, 2021

View reviewed changes

kianenigma and others added 2 commits February 8, 2021 14:24

Update frame/staking/src/offchain_election.rs

e961c86

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Update frame/staking/src/offchain_election.rs

51969da

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

kianenigma and others added 4 commits February 8, 2021 14:26

Update frame/staking/src/lib.rs

c43fe6b

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Some logging

09298ce

Merge branch 'kiz-staking-OCW-rerun' of github.com:paritytech/substra…

cca1f42

…te into kiz-staking-OCW-rerun

Ready for merge.

221b31a

Merge branch 'master' of github.com:paritytech/substrate into kiz-sta…

ab14640

…king-OCW-rerun

gui1117 mentioned this pull request Feb 11, 2021

Decouple Staking and Election - Part 2 Unsigned Phase #7909

Merged

3 tasks

kianenigma added A6-seemsok and removed D2-breaksapi labels Feb 24, 2021

kianenigma closed this Mar 5, 2021

coriolinus added a commit that referenced this pull request Mar 8, 2021

migrate core logic from #7976

f6a0fd9

This is a much smaller diff than that PR contained, but I think it contains all the essentials.

coriolinus mentioned this pull request Mar 8, 2021

Multi-phase elections solution resubmission #8290

Merged

shawntabrizi removed the D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rerun Staking OCW periodically. #7976

Rerun Staking OCW periodically. #7976

kianenigma commented Jan 25, 2021 •

edited

Loading

tomusdrw left a comment

tomusdrw left a comment

bkchr left a comment

kianenigma commented Feb 1, 2021

kianenigma commented Feb 8, 2021

kianenigma commented Feb 24, 2021

kianenigma commented Mar 5, 2021

Rerun Staking OCW periodically. #7976

Rerun Staking OCW periodically. #7976

Conversation

kianenigma commented Jan 25, 2021 • edited Loading

tomusdrw left a comment

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

kianenigma commented Feb 1, 2021

kianenigma commented Feb 8, 2021

kianenigma commented Feb 24, 2021

kianenigma commented Mar 5, 2021

kianenigma commented Jan 25, 2021 •

edited

Loading