Parachains double vote handler initial implementation. #840

montekki · 2020-02-12T13:31:13Z

A runtime handler to slash validators for double votes.

Fixes #760

rphmeier · 2020-02-12T13:44:12Z

runtime/common/src/parachains.rs

+				None => return Err("Not in validator set".into()),
+			};
+
+			let offender = match T::FullIdentificationOf::convert(validators[offender_idx].clone()) {


At the moment, this will only work for validators who are in the current set. In the future we probably have to accept another argument to the DoubleVoteReport that we can use with session::historical::Module to extract the FullIdentification.

rphmeier · 2020-02-12T13:45:16Z

runtime/common/src/parachains.rs

+				(Statement::Valid(hash), Statement::Candidate(candidate)) |
+				(Statement::Invalid(hash), Statement::Candidate(candidate))
+				if candidate.hash() == hash => {
+					T::ReportDoubleVote::report_offence(vec![], offence);


what is the plan to determine reporters further on?

also - a side PR on Substrate for report_offence to return a value if the report should be ignored would be helpful in practice, this is when pallet-offences triages and finds that all reports are duplicate.

The reason I want that is so we can reject duplicate offence reports. It should not be possible for a single equivocation to allow huge spam on the chain.

Looks like it should be a signed extrinsic and the reporer's Id is ensure_signed result?

This bears some investigation. My understanding was that the signed extrinsic bears a fee (which we don't want). Maybe setting weight to 0 is the correct approach, though. The other thing you should check before using signed extrinsics is that returning an error means that the extrinsic cannot be included in the block. That's for the same anti-spam reason.

rphmeier · 2020-02-12T13:49:25Z

runtime/common/src/parachains.rs

+	}
+
+	fn time_slot(&self) -> Self::TimeSlot {
+		self.session_index


Since reports are deduplicated based on time slot, we might want to (for correctness) use the relay parent hash as the time slot instead of the session. But it doesn't make that much difference, since we slash 100%.

rphmeier · 2020-02-25T23:18:47Z

runtime/common/src/parachains.rs

+	/// Identity of the double-voter.
+	pub identity: ValidatorId,
+	/// First vote of the double-vote.
+	pub first: (Statement, ValidityAttestation),


I'm not convinced that Statement, ValidityAttestation pairs make sense. A ValidityAttestation, CandidateHash pair would make sense, or a Statement, Signature pair.

Statement, Signature makes most sense, since ValidityAttestation does not allow for Invalid attestations.

rphmeier · 2020-02-25T23:21:02Z

runtime/common/src/parachains.rs

+		let first = self.first.clone();
+		let second = self.second.clone();
+
+		Self::verify_vote(first, &parent_hash, self.identity.clone())?;


should check that the first statement != the second.

Also a test that a double-vote report with both times the same report fails would be good!

rphmeier · 2020-02-25T23:21:35Z

runtime/common/src/parachains.rs

+
+		let (payload, sig) = match attestation {
+			ValidityAttestation::Implicit(sig) => {
+				let payload = localized_payload(statement, parent_hash);


i.e. a ValidityAttestation::Implicit corresponds directly to a Statement::Candidate. and a ValidityAttestation::Explicit corresponds to a Statement::Valid. However, the statement isn't checked against that type, so the code doesn't make sense. In this case, a Statement, Signature pair would make most sense.

rphmeier · 2020-02-25T23:45:53Z

runtime/common/src/parachains.rs

+				let e = TransactionValidityError::from(
+					InvalidTransaction::Custom(DoubleVoteValidityError::NotDoubleVote as u8)
+				);
+				match (&report.first.0, &report.second.0) {


I see now that all pairs are checked here, but IMO the logic should not be so spread out. Probably should be checked in report.verify.

Thinking on this a bit further - I think we also need to check against duplicate reports (report_offence & friends) before Oking the SignedExtension. Otherwise useless duplicates will be included for free. That is a spam vector that should be tested against.

rphmeier · 2020-02-25T23:46:45Z

runtime/common/src/parachains.rs

+				);
+				report.verify(parent_hash).map_err(|_| e)?;
+
+				let e = TransactionValidityError::from(


This pre-emptive let e = ... pattern and then return Err(e) is kind of weird compared to return Err(...). Mostly because e is rebound several times and exists in a scope where it is not relevant. Strikes me as a bit disorganized and makes the code harder to follow.

rphmeier · 2020-02-25T23:47:38Z

runtime/common/src/parachains.rs

@@ -1746,4 +2063,273 @@ mod tests {
 		let hashed_null_node = <NodeCodec<Blake2Hasher> as trie_db::NodeCodec>::hashed_null_node();
 		assert_eq!(hashed_null_node, EMPTY_TRIE_ROOT.into())
 	}
+
+	#[test]
+	fn double_vote_works() {


prefer to split the many tests here into multiple functions.

bkchr · 2020-02-27T20:36:41Z

runtime/common/src/parachains.rs

+			}
+		};
+
+		match sig.verify(&payload[..], &authority) {


Should be an if.

bkchr · 2020-02-27T20:38:11Z

runtime/common/src/parachains.rs

+				(payload, sig)
+			}
+			ValidityAttestation::Explicit(sig) => {
+				let payload = localized_payload(statement, parent_hash);


This can be moved out of the match (the same code exists in the other branch).

bkchr · 2020-02-27T20:46:07Z

runtime/common/src/parachains.rs

+		) -> DispatchResult {
+			let reporter = ensure_signed(origin)?;
+
+			// The following code duplicates the logic in `ValidateDoubeVoteReports::validate`


Why isn't it a function that is called by both?

bkchr · 2020-02-27T20:46:54Z

runtime/common/src/parachains.rs

+			let parent_hash = <system::Module<T>>::block_hash(<system::Module<T>>::block_number());
+
+			let authorities = Self::authorities();
+			let offender_idx = match authorities.iter().position(|a| *a == report.identity) {


Suggested change

let offender_idx = match authorities.iter().position(|a| *a == report.identity) {

let offender_idx = authorities.iter().position(|a| *a == report.identity).ok_or_else(|| "Not in validator set")?;

Much shorter.

bkchr · 2020-02-27T20:49:07Z

runtime/common/src/parachains.rs

+
+/// Ensure that double vote reports are only processed if valid.
+#[derive(Encode, Decode, Clone, Eq, PartialEq)]
+pub struct ValidateDoubleVoteReports<T: Trait + Send + Sync>(rstd::marker::PhantomData<T>) where


Suggested change

pub struct ValidateDoubleVoteReports<T: Trait + Send + Sync>(rstd::marker::PhantomData<T>) where

pub struct ValidateDoubleVoteReports<T>(rstd::marker::PhantomData<T>);

Trait bounds, if not required, should not be added to the definition.

runtime/common/src/parachains.rs

rphmeier · 2020-03-17T18:32:28Z

runtime/common/src/parachains.rs

+		/// The mapping from parent block hashes to session indexes.
+		///
+		/// Used for double vote report validation.
+		/// This is not pruned at the moment.


Do you have any thoughts on how it might be pruned in the future? I'd imagine we can get an API from staking/session that notifies us every time a session is removed from a bonding window (likely batches of sessions as the bonding window shrinks by era). It makes sense to account for such an API when setting up the storage now, even though it will not currently be triggered.

What do you think about implementing a custome type T::SessionInterface (not it is Self) that would proxy calls to the already existing implementation, it looks like we need prune_historical_up_to. My understanding is that this will be called on every era start pruning all data up to it.

it's not that easy: implementing a custom SessionInterface on Runtime is conflicting with implementation in substrate. I will read further into the code.

I don't think we want to be responsible for the call to prune_historical_up_to, but we would want to receive notifications when that is called, and for which sessions.

Co-Authored-By: Robert Habermeier <rphmeier@gmail.com>

rphmeier · 2020-03-20T15:29:35Z

runtime/common/src/parachains.rs

+			// this here to get the full identification of the offender.
+			let offender = T::KeyOwnerProofSystem::check_proof(
+					(PARACHAIN_KEY_TYPE_ID, report.identity.encode()),
+					report.proof.clone(),


clone is probably unnecessary here. i'd prefer to avoid the clone as the FullIdentification will be heavy

rphmeier

General approach & tests look good. Would still like to get more clarity on how we can prevent the ParentToSessionIndex map from blowing up and make sure the schema allows for pruning that in the future.

montekki · 2020-03-21T09:43:17Z

@rphmeier I've added an attempt at pruning, the idea is to ask Staking::current_era() in on_initialize and keep it until the era changes. Upon the era change the remove_all is called for the ParentToSessionIndex mapping. What do you think?

gavofyork · 2020-03-21T12:46:02Z

@rphmeier can this be merged now?

rphmeier · 2020-03-22T05:38:33Z

@rphmeier I've added an attempt at pruning, the idea is to ask Staking::current_era() in on_initialize and keep it until the era changes. Upon the era change the remove_all is called for the ParentToSessionIndex mapping. What do you think?

This is definitely too aggressive but I will not block on it. Will file an issue with my full thoughts

@gavofyork

@rphmeier can this be merged now?

Yup happy to merge it as soon as ready

* slightly changed relay loop initialization * git mv * clippy * more clippy * loop_run -> run_loop * review and clippy * clippy

Parachains double vote handler initial implementation.

257c9a2

montekki added the A3-in_progress Pull request is in progress. No review needed at this stage. label Feb 12, 2020

paritytech-ci added the B2-breaksapi label Feb 12, 2020

rphmeier reviewed Feb 12, 2020

View reviewed changes

montekki added 2 commits February 19, 2020 15:44

Make tests test the actual slashing.

a0ea78f

Implement SignedExtension validation of double vote reports.

ed78c75

montekki marked this pull request as ready for review February 20, 2020 14:28

montekki added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Feb 20, 2020

montekki requested review from bkchr and rphmeier February 20, 2020 14:28

montekki added 2 commits February 20, 2020 18:02

Merge branch 'master' into fs-double-vote-slash-handler

b02b7d2

Fixes build after merge

8f1c879

rphmeier reviewed Feb 25, 2020

View reviewed changes

Merge branch 'master' into fs-double-vote-slash-handler

e8c70b9

bkchr reviewed Feb 27, 2020

View reviewed changes

montekki mentioned this pull request Feb 27, 2020

Offence reporting returns a result paritytech/substrate#5082

Merged

Merge branch 'master' into fs-double-vote-slash-handler

72e8a24

montekki added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Feb 29, 2020

Review fixes

55ce722

montekki mentioned this pull request Mar 4, 2020

Adds a session getter to historical proofs paritytech/substrate#5125

Merged

Adds historical session proofs

9b06b3a

montekki added 3 commits March 12, 2020 16:46

Change signature type on DoubleVoteReport

4861836

Adds docs and removes blank lines

456a94a

Removes leftover code

f60b058

rphmeier reviewed Mar 14, 2020

View reviewed changes

runtime/common/src/parachains.rs Outdated Show resolved Hide resolved

montekki added 2 commits March 15, 2020 14:30

Merge branch 'master' into fs-double-vote-slash-handler

1f866cb

Fix build

c12d17a

gabreal mentioned this pull request Mar 16, 2020

check polkadot for companion pull requests paritytech/substrate#5262

Merged

Merge branch 'master' into fs-double-vote-slash-handler

d144ec0

montekki added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 17, 2020

Fix build after a merge

de45a66

rphmeier reviewed Mar 17, 2020

View reviewed changes

runtime/common/src/parachains.rs Outdated Show resolved Hide resolved

rphmeier reviewed Mar 17, 2020

View reviewed changes

Apply suggestions from code review

71b858d

Co-Authored-By: Robert Habermeier <rphmeier@gmail.com>

rphmeier reviewed Mar 20, 2020

View reviewed changes

rphmeier approved these changes Mar 20, 2020

View reviewed changes

Prune ParentToSessionIndex

212e94c

gavofyork added A8-looksgood and removed A0-please_review Pull request needs code review. labels Mar 21, 2020

gavofyork and others added 2 commits March 21, 2020 13:47

Merge branch 'master' into fs-double-vote-slash-handler

47f2763

Remove a clone and a warning

0f2183d

rphmeier mentioned this pull request Mar 22, 2020

Parent hash to session index mapping in parachains module too aggressive #924

Closed

montekki added 2 commits March 22, 2020 09:09

Merge branch 'master' into fs-double-vote-slash-handler

a98afd7

Merge branch 'master' into fs-double-vote-slash-handler

4be887d

montekki merged commit b361171 into paritytech:master Mar 22, 2020

montekki mentioned this pull request Apr 3, 2020

Adds an offchain call to submit double vote reports #966

Merged

tomusdrw pushed a commit that referenced this pull request Mar 26, 2021

Changed how relay loops are started (#840)

68541c2

* slightly changed relay loop initialization * git mv * clippy * more clippy * loop_run -> run_loop * review and clippy * clippy

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parachains double vote handler initial implementation. #840

Parachains double vote handler initial implementation. #840

montekki commented Feb 12, 2020

rphmeier Feb 12, 2020

rphmeier Feb 12, 2020

rphmeier Feb 12, 2020

montekki Feb 12, 2020

rphmeier Feb 12, 2020

rphmeier Feb 12, 2020

rphmeier Feb 25, 2020

rphmeier Mar 4, 2020

rphmeier Feb 25, 2020

rphmeier Feb 25, 2020

rphmeier Feb 25, 2020

rphmeier Feb 25, 2020

rphmeier Feb 26, 2020

rphmeier Feb 25, 2020 •

edited

Loading

rphmeier Feb 25, 2020 •

edited

Loading

bkchr Feb 27, 2020

bkchr Feb 27, 2020

bkchr Feb 27, 2020

bkchr Feb 27, 2020

bkchr Feb 27, 2020

rphmeier Mar 17, 2020 •

edited

Loading

montekki Mar 18, 2020

montekki Mar 18, 2020

rphmeier Mar 19, 2020

rphmeier Mar 20, 2020

rphmeier left a comment •

edited

Loading

montekki commented Mar 21, 2020

gavofyork commented Mar 21, 2020

rphmeier commented Mar 22, 2020 •

edited

Loading

	let offender_idx = match authorities.iter().position(\|a\| *a == report.identity) {
	let offender_idx = authorities.iter().position(\|a\| *a == report.identity).ok_or_else(\|\| "Not in validator set")?;

	pub struct ValidateDoubleVoteReports<T: Trait + Send + Sync>(rstd::marker::PhantomData<T>) where
	pub struct ValidateDoubleVoteReports<T>(rstd::marker::PhantomData<T>);

Parachains double vote handler initial implementation. #840

Parachains double vote handler initial implementation. #840

Conversation

montekki commented Feb 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

rphmeier Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier left a comment • edited Loading

Choose a reason for hiding this comment

montekki commented Mar 21, 2020

gavofyork commented Mar 21, 2020

rphmeier commented Mar 22, 2020 • edited Loading

rphmeier Feb 25, 2020 •

edited

Loading

rphmeier Feb 25, 2020 •

edited

Loading

rphmeier Mar 17, 2020 •

edited

Loading

rphmeier left a comment •

edited

Loading

rphmeier commented Mar 22, 2020 •

edited

Loading