Multi-phase elections solution resubmission #8290
Conversation
This is a much smaller diff than that PR contained, but I think it contains all the essentials.
github-actions
bot
added
the
A3-in_progress
coriolinus
changed the title
coriolinus
added
B0-silent
github-actions
bot
added
A0-please_review
coriolinus
added
the
D9-needsaudit 👮
| /// Save a given call into OCW storage. | ||
| fn save_solution<T: Config>(call: &Call<T>) { | ||
| let storage = StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL); | ||
| storage.set(&call); |
There was a problem hiding this comment.
You should use the atomic ::mutate here. https://github.com/paritytech/substrate/pull/7976/files
There was a problem hiding this comment.
Why? We don't care about the current value, so what benefit is there in reading it? We're not using the get-check-set pattern mentioned in the docs.
There was a problem hiding this comment.
let me know if it is unclear, it is an important thing to grasp, the overlapping possibility of OCWs.
There was a problem hiding this comment.
It's very surprising that set would allow for reads of an incomplete write, even from a separate, parallel worker. I traced the function calls back a bit, and ended up in trait sp_core::offchain::Externalities. For that trait, local_storage_set requires &mut self.
Obviously, we can run OCWs in parallel, so the storage needs to be shared. I haven't traced it back exhaustively, but what I'd expect to find is something like RwLock<dyn Externalities>. Given that, an OCW which wants to read the storage has to wait until its peer with the write lock is completely finished writing, before it has a chance to read.
I believe that the atomic nature of the mutate method is to avoid this scenario:
- OCW A: read storage
Foo - OCW B: update storage
Foo - OCW A: update storage
Foobased on its then-current value, clobbering whatBwrote
AFAICT, normal mutable aliasing rules prevent OCW B from ever reading while OCW A is in the middle of writing. If I'm right, then we really don't need mutate here.
There was a problem hiding this comment.
I am totally happy to accept what you are proposing, if you actually find out that it is designed and works as you hypothesize, not based on numerous assumptions that you are making.
I am not sure about any of this myself, to be frank, and generally prefer consulting and the trusting the opinion of someone who actually does (Tomek in the case of the linked discussion). You can probably ping him if you want to clarify this.
There was a problem hiding this comment.
update?
(learning from the past, I think you should just change this 1 line to a 1 liner that uses mutate and call it a day and be safe and save time as well.)
There was a problem hiding this comment.
Yes ok. I just can't find the actual implementation of offchain storage anywhere; everything seems to be abstractions that layer on abstractions. I'll just use mutate here. 8b31773
|
All requested items except burnin have now been completed. There's some more work necessary before this can be merged--burnin and CI--but this is in a good place for review now. |
coriolinus
added
the
D9-needsaudit 👮
| let storage = StorageValueRef::persistent(&OFFCHAIN_LOCK); | ||
|
|
||
| let mutate_stat = | ||
| storage.mutate::<_, &'static str, _>(|maybe_head: Option<Option<T::BlockNumber>>| { |
There was a problem hiding this comment.
sidenote about the code below:
As far as I know, we run offchain worker only on the best block, if we do reorg we don't run the offchain worker on every block in the reorg.
If this is true then we could run the offchain worker when now is less than head on forks, because if we consider another chain to be the best then we should submit to this chain, no matter what we did in a previous fork. no ?
should address https://github.com/paritytech/substrate/pull/8290/files#r617533358 restructures how the checks are sequenced, which simplifies legibility.
jakoblell
added
D1-audited 👍
|
bot merge |
|
Waiting for commit status. |
|
Merge aborted: Checks failed for 6ae73c4 |
|
bot merge |
|
Trying merge. |
ghost
merged commit * Companion for Multi-phase elections solution resubmission paritytech/substrate#8290 * update Substrate Co-authored-by: parity-processbot <>
This reverts commit 92c15dd.
This reverts commit 92c15dd.
* 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Guillaume Thiolliere <[email protected]>
* Companion for Multi-phase elections solution resubmission paritytech/substrate#8290 * update Substrate Co-authored-by: parity-processbot <>
* Companion for Multi-phase elections solution resubmission paritytech/substrate#8290 * update Substrate Co-authored-by: parity-processbot <>
Migration of #7976 to multi-phase-elections.
polkadot companion: paritytech/polkadot#2648