Include ValidatorIndex in the dispute statement

[rphmeier]

paritytech/polkadot#3348 (comment)

@eskimor Has been pushing strongly to include ValidatorIndex in the dispute statement. Our major disagreement has been over the fact that the SignedDisputeStatement can't actually check whether the public key corresponds to the session info.

I propose that we introduce a type in the vein of std::panic::AssertUnwindSafe for the purpose of encouraging that the caller to be very explicit and very careful about checking the index.

/// While this type does no checking on its own, it's a good way to force the caller to assert that they have indeed checked
/// that the associated public key does indeed correspond to the validator index at the given session.
AssertSessionValidator(SessionIndex, ValidatorPublic, ValidatorIndex);

Instead of wrapping these types independently, I think that SignedDisputeStatement should wrap the AssertSessionValidator so the constructors of the function have to explicitly accept it. The API of SignedDisputeSession should have an accessor to the AssertSessionValidator to be used as statement.validator().session(), statement.validator().id(), and statement.validator().index().

This makes it clear to the receiver is a SignedDisputeStatement that they are dealing with an asserted-valid type and thus will make it more apparent that receiving code is reasonably foregoing expensive validator-set membership checks. Although the API is clunkier than statement.validator_index(), I would much rather read code a year from now where I'd ask "what's that validator() call about and why is this not verifying the index is correct?" instead of "why the f*** isn't this verifying the index??".

[eskimor]

Revisiting this again (as I have to touch relevant code). To me the safest approach is still to only include the ValidatorIndex in the signed data, together with the SessionIndex. This way the only way to actually check the signature is to look up the ValidatorIndex in some session. While in theory the checker could use the wrong session, the SessionIndex is part of the signed data, so the checking code would need to use the correct index for the signing context in order to verify correctly, but the wrong index for looking up the session.

Looking at the CheckedSigned::try_from_unchecked signature:

	pub fn try_from_unchecked<H: Encode>(
		unchecked: UncheckedSigned<Payload, RealPayload>,
		context: &SigningContext<H>,
		key: &ValidatorId,
	) -> Result<CheckedSigned> {

we indeed leave unnecessary room for error here. I would propose to change it to
the following:

impl CheckedSigned {
  pub fn try_from_unchecked(
    unchecked: UncheckedSigned,
    relay_parent: Hash,
    session_info: LookedUpSessionInfo,
   ) -> Result<CheckedSigned> {
   }
}

where LookedUpSessionInfo would look like this:

struct LookedUpSessionInfo {
  index: SessionIndex,
  info: SessionInfo,
}

There will likely only be a single code in the source tree, taking a
SessionIndex and returning LookedUpSessionInfo with that index. Room for err
would be close to zero and by above signature of try_from_unchecked we have a
guarantee that the same index is used for the signed payload. In theory we could
make the LookedUpSessionInfo type protected to actually have even stronger
guarantees. In practice this does not work without unsafe or other hacks as
the type would need to live in primitives, while the lookup code depends on
primitives, so we would get a cycle. Still to mess this up, you almost have to
be a malicious contributor with collaborating reviewers.

[rphmeier]

We shouldn't put the entire SessionInfo into the signed data type because it's relatively heavy and we need to pass around thousands of signed messages. The AssertSessionValidator datatype seems like it does roughly the same thing otherwise.

[eskimor]

Oh yeah, I wasn't suggesting that. Just that we pass a reference to the SessionInfo to the checking function, so it can pick the right validator key for signature checking based on the passed SessionIndex which it will also use in the SigningContext.

Reading your text again, I guess we basically mean the same thing. (I also initially read your idea, that you want to put that AssertSessionValidator into the signed type 😆 ).

So if I got this right, instead of passing in the SessionInfo you would pass in the AssertSessionValidator type into the checking function. Both would result in doing the wrong thing becoming bluntly obvious and really hard to do by accident.