Refactor election-provider-multi-phase solution checks

[coriolinus]

Actually, I have a good reorg in mind, let me know what you think:

Currently, some checks in feasibility_check is repeated in unsigned_pre_dispatch_checks. unsigned_pre_dispatch_checks itself is called both in validate_unsigned (which is used by the txpool), and in pre_dispatch (which is called exactly when the name says so: prior to dispatch, not validation). Currently, I am being superconservative:

1. I repeat unsigned_pre_dispatch_checks is submit_unsigned itself which is redundant, because it is already called in pre_dispatch.

2. Some individual components are checked twice: DesiredTargets is checked both in feasibility_check and unsigned_pre_dispatch_checks, and given the previous note is technically checked 3 times in a single dispatch pipeline:

3. in the call to unsigned_pre_dispatch_checks in pre_dispatch.

4. in the call to unsigned_pre_dispatch_checks in submit_unsigned.

5. lastly in the body of the feasibility_check.

We should rename unsigned_pre_dispatch_checks to a proper pre_dispatch_checks that checks:

1. phase
2. round
3. desired_targets
4. score

and everything else is done in feasibility_check.

The standard assumption is that all solutions must go through both of these eventually (and at least once), so we can deduplicate them: things that are checked in pre_dispatch need not be checked again in feasibility_check. Then the question is, what should be in pre_dispatch and what not?

The role of pre_dispatch is this: feel the gap about criteria that could change over time. For example, if you are honest, you will always generate a solution that is passes feasibility_check. But, if you took an hour to compute it:

1. The phase/round might have changed
2. <QueuedSolution> might now contain a better solution.

With this logic, the pre_dispatch should check for:

1. phase
2. round (the combination of which guarantees that)
3. score

to make sure if you were honest (used the correct snapshot), you will not submit a bad solution due to being out of date (round/phase) or the fact that a better solution was recently accepted (score).

The reason that I historically stuffed desired_targets into pre_dispatch is: During weight/length trimming, we could remove a target, in which case the solution is now invalid, and we don't want to submit this. As a easy fix, I put it in pre_dispatch_checks because it was called in validate_unsigned and fixed this. But this is not a criterion that changes over time: once you generate a solution and trim it, if it has enough winner, it will stay so (until the round maybe changes). So calling into feasibility_check in mine_checked is enough.

So all in all:

• The mine_checked should still call both of these just in case.
• feasibility_check should still be called only in submit_unsigned (except for OCW code that calls it).
• pre_dispatch should be called in both validate and pre_dispatch of validate_unsigned.

Lastly, the cream on the case is that, here you can do something like:

let maybe_call_and_score = restore_solution::<T>()
   .and_then(|call|  
       // `Ok` IFF call.solution is `pre_dispatch`. No need to check `feasibility` given all the notes above.
   ).or_else(|_| {
       // Same as before: mine_checked, save and return Ok
   })

This might sound too big, so maybe make it a follow-up, but the current layout is kinda confused as well. At the least, you can move the check to score only in and_then, and since now we are okay with duplicate checks, just call unsigned_pre_dispatch_checks in there to check round and score (with the other two as... bonus?)

Originally posted by @kianenigma in #8290 (comment)

[gui1117]

The role of pre_dispatch is this: feel the gap about criteria that could change over time. For example, if you are honest, you will always generate a solution that is passes feasibility_check. But, if you took an hour to compute it:

1. The phase/round might have changed

2. `<QueuedSolution>` might now contain a better solution.

This is not the case in the PR #8290
in my example #8290 (comment)
A honest validator retrieve from storage a wrong solution. which doesn't pass feasibility_check.

* `feasibility_check` should still be called only in `submit_unsigned` (except for OCW code that calls it).

What does that mean? submit_unsigned will always do the feasibility_check. I can't understand the exception.

let maybe_call_and_score = restore_solution::<T>()
   .and_then(|call|  
       // `Ok` IFF call.solution is `pre_dispatch`. No need to check `feasibility` given all the notes above.
   ).or_else(|_| {
       // Same as before: mine_checked, save and return Ok
   })

I don't see why we don't need to do the feasibility considering the point I raised above, the restored solution can be invalid.

In my opinion we can:

• associate the solution to its election data:
  • by hashing the election data, so that if the solution is computed for another election data, then it is not used retrieved because it can be invalid.
  • or by associating to the block hash of the block which started the election (and fixed the election data). (but it doesn't seem straightforward to get the block hash of the block which started the election when we want to check it).
• or at the very least run the feasibility check when retrieving the solution.

Anyway I don't see where my example is wrong, and in the above example lots of validator mine only one wrong solution for the chain. Which is pretty bad IMO.

[kianenigma]

What does that mean? submit_unsigned will always do the feasibility_check. I can't understand the exception.

I mean feasibility check should only be used internally inside submit_unsigned, except for OCW code. the OCW code currently calls feasibility_check in fn mine_checked as well.

A honest validator retrieve from storage a wrong solution. which doesn't pass feasibility_check.

What we should do is to add a hash of the snapshot to the things that are checked in pre_dispatch_checks. This also fits into the category of things that can change over time (due to reorg) and then we won't need to call feasibility_check before submission.

associate the solution to its election data:

• by hashing the election data, so that if the solution is computed for another election data, then it is not used retrieved because it can be invalid.

Totally agree.

[stale]

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

[kianenigma]

Quite important.

[kianenigma]

We might ditch this, since we will soon get a new election pallet and we don't want to support multi-phase forever.