[BEEFY] Add runtime support for reporting fork voting

[serban300]

Related to #4523

Extracting part of #1903 (credits to @Lederstrumpf for the high-level strategy), but also introducing significant adjustments both to the approach and to the code. The main adjustment is the fact that the ForkVotingProof accepts only one vote, compared to the original version which accepted a vec![]. With this approach more calls are needed in order to report multiple equivocated votes on the same commit, but it simplifies a lot the checking logic. We can add support for reporting multiple signatures at once in the future.

There are 2 things that are missing in order to consider this issue done, but I would propose to do them in a separate PR since this one is already pretty big:

• benchmarks/computing a weight for the new extrinsic (this wasn't present in BEEFY: add support for slashing validators signing forking commitments #1903 either)
• exposing an API for generating the ancestry proof. I'm not sure if we should do this in the Mmr pallet or in the Beefy pallet

Co-authored-by: Robert Hambrock [email protected]

[svyatonik]

LGTM

[svyatonik]

Do we need to accept two different types of equivocation reports (double voting and fork voting) at a single slot? If so, we maybe need to add that type to the tag

[serban300]

I don't think we can accept 2 equivocation reports from the same offender in the same round. I think check_evidence will fail:

	fn check_evidence(
		evidence: EquivocationEvidenceFor<T>,
	) -> Result<(), TransactionValidityError> {
		let offender = evidence.checked_offender::<P>().ok_or(InvalidTransaction::BadProof)?;

		// Check if the offence has already been reported, and if so then we can discard the report.
		let time_slot = TimeSlot { set_id: evidence.set_id(), round: *evidence.round_number() };
		if R::is_known_offence(&[offender], &time_slot) {
			Err(InvalidTransaction::Stale.into())
		} else {
			Ok(())
		}
	}

[serban300]

Sorry, we could change check_evidence. I don't know what would be best. I would lean towards staying consistent with the current approach where we accept one equivocation per (offender, set_id, round).

[acatangiu]

I don't see anywhere in this PR the extrinsic used to report these ForkEquivocations..

I am thinking since you only allow single offender per report does that mean that for a slashable fork equivocation we need to report all the offenders across multiple blocks?

[serban300]

I don't see anywhere in this PR the extrinsic used to report these ForkEquivocations..

The extrinsic is here .

I am thinking since you only allow single offender per report does that mean that for a slashable fork equivocation we need to report all the offenders across multiple blocks?

Hmmm good point. Could be. I can try to implement the solution that was discussed here. It's good to have it anyway.

[acatangiu]

The extrinsic is here .

Right, I got confused. I had noticed we don't have runtime APIs for the "unsigned" version of the call. Should we add it?

[serban300]

Yes, missed that. Will add it.

[serban300]

I am thinking since you only allow single offender per report does that mean that for a slashable fork equivocation we need to report all the offenders across multiple blocks?

Hmmm good point. Could be. I can try to implement the solution that was discussed here. It's good to have it anyway.

Done. PTAL ! Relevant commit: f709cef

Right, I got confused. I had noticed we don't have runtime APIs for the "unsigned" version of the call. Should we add it?

Yes, missed that. Will add it.

On a second thought, I think it's better to do this in a separate PR together with the logic for generating the ancestry proofs since the type of the ancestry proof will depend on that (it could either be AncestryProof or OpaqueValue, depending on how we generate it). WDYT ?

[Lederstrumpf]

The main adjustment is the fact that the ForkVotingProof accepts only one vote, compared to the original version which accepted a vec![]. With this approach more calls are needed in order to report multiple equivocated votes on the same commit, but it simplifies a lot the checking logic. We can add support for reporting multiple signatures at once in the future.

When you state "at once in the future":
Is this an immediate follow-up PR prior to integrating the client side changes from #1903, or possibly delayed beyond then - i.e. adjusting the client side changes in #1903 to work with a single vote prior to moving to a vec (eventually)?

[serban300]

The main adjustment is the fact that the ForkVotingProof accepts only one vote, compared to the original version which accepted a vec![]. With this approach more calls are needed in order to report multiple equivocated votes on the same commit, but it simplifies a lot the checking logic. We can add support for reporting multiple signatures at once in the future.

When you state "at once in the future": Is this an immediate follow-up PR prior to integrating the client side changes from #1903, or possibly delayed beyond then - i.e. adjusting the client side changes in #1903 to work with a single vote prior to moving to a vec (eventually)?

Discussed offline. I am not sure if we should support a batch of votes, since it will complicate things, but if we do, we can do any of the 2 options. They both seem ok to me.

[bkontur]

@serban300 what about this TODO?

[serban300]

I plan to do this in a separate PR, as mentioned in the description. Also keeping track of it in the issue: #4523 to make sure we don't forget.

[franciscoaguirre]

Is this description relevant to runtime users?

[serban300]

I'm thinking it might be helpful for them to know of the new extrinsics.