Equivocations slashing by marcio-diaz · Pull Request #3314 · paritytech/substrate

marcio-diaz · 2019-08-06T14:19:15Z

No description provided.

…to retreat-slashing

…or slash calculations

andresilva

I think some of the stuff can be simplified. But overall the structure that we're going to use to detect and construct equivocation proofs, using the runtime to construct the report and eventually signing and submitting the tx is mostly fleshed out now.

andresilva · 2019-08-07T09:06:19Z

core/consensus/babe/primitives/src/digest.rs

 pub trait CompatibleDigestItem: Sized {
 	/// Construct a digest item which contains a BABE pre-digest.
-	fn babe_pre_digest(seal: BabePreDigest) -> Self;
+	fn babe_pre_digest<D: Codec>(seal: D) -> Self;


Why is this change needed? The runtime should be using RawBabePreDigest anyway. This function signature doesn't make a lot of sense, it looks like you can give it anything and it will make a BABE seal which is not the case at all, since you need to give it some specific struct to make a proper BABE seal.

Yeah, it should be a better way to do it. But it also using a non raw pre digest.

Yeah, this really feels wrong.

andresilva · 2019-08-07T09:07:30Z

core/consensus/aura/src/lib.rs

 	client: Arc<C>,
 	phantom: PhantomData<P>,
 	inherent_data_providers: inherents::InherentDataProviders,
+	transaction_pool: Option<Arc<T>>,


This should probably be replaced with a F: Fn(Block::Extrinsic) which will just push the extrinsic to the transaction pool As it stands you can't do anything with this T, and it's probably easier to not drag any complicated TransactionPool types here.

I'l add add a trait that implements submit_one to the transaction pool, as per offline discussion :) (as it's in my draft).

andresilva · 2019-08-07T09:09:52Z

core/consensus/babe/primitives/src/lib.rs


+/// Represents an Babe equivocation proof.
+#[derive(Debug, Clone, Encode, Decode, PartialEq)]
+pub struct BabeEquivocationProof<H> {


I thought we had a type for EquivocationProof already (in slots). Why add another? I also don't know why you need the authority signatures here, if you pass the headers you can extract the author from it.

Because we are taking the signatures out of the header (I think we talk about this yesterday). In your case I will need to push the signature again, and pop it again when checking...

But current struct doesn't really enforce consistency, how do you prevent creating it with first_signature not matching first_header.

I don't think that is possible unless we keep the signature inside. I think signature outside is more flexible.

andresilva · 2019-08-07T09:10:38Z

core/consensus/common/primitives/src/lib.rs

 	}
 }
+
+pub trait AuthorshipEquivocationProof {


IMO this is not needed, it's not abstracting over anything anyway.

It's abstracting over Aura and Babe equivocation proof (I have the Aura implementation with low priority).

https://github.com/paritytech/substrate/pull/3314/files/1a88f9016ae3b16937afce5e3fb189749dd8e521#diff-1144b2e008f250f4ca22726b35ee22c7R65

How will you implement this for GRANDPA? Same goes for some other methods which are specific to block proposal equivocations. My point is there's too little in common between BABE and GRANDPA equivocations to abstract anything, so there's no point in abstracting. They'll be reported to different modules anyway so I don't know why you need a common type.

It's interfacing Aura and Babe with the Slots module. If I remove this I need to duplicate code in Slots module, or there is a better way?

Sorry I misread your comment earlier and thought this was supposed to abstract over GRANDPA and BABE.

andresilva · 2019-08-07T09:11:39Z

core/consensus/slots/src/aux_schema.rs

-					fst_header: prev_header.clone(),
-					snd_header: header.clone(),
-				}));
+				return Ok(Some(AuthorshipEquivocationProof::new(


Why not just use the existing struct and just pass the remaining data as arguments to the construct report call?

Why not doing it in this way? Anyway the proof and the SessionIndex should be in the Equivocation, right?.

…lashing

rphmeier · 2019-08-18T10:39:23Z

core/consensus/babe/primitives/src/digest.rs

 }

+/// Get the slot.
+pub fn get_slot<H: Header>(header: &H) -> Result<SlotNumber, &str>


The documentation and function naming needs real improvement

rphmeier · 2019-08-18T10:39:54Z

core/consensus/babe/primitives/src/digest.rs

+
+/// Extract the BABE pre digest from the given header. Pre-runtime digests are
+/// mandatory, the function will return `Err` if none is found.
+pub fn find_pre_digest<H: Header, D: Codec>(header: &H) -> Result<D, &str>


it's strange to return a str with the lifetime of the given header as opposed to a &'static str.

rphmeier · 2019-08-18T10:43:11Z

core/consensus/babe/src/lib.rs

+		let block_id = BlockId::number(client.info().best_number);
+		let maybe_report_transaction = client
+			.runtime_api()
+			.construct_equivocation_transaction_with_context(


This is IMO not the right level of abstraction.

It's better to think of things in terms of making code testable: rather than a SubmitExtrinsic trait (which is itself better than passing a raw transaction queue), you should have a SubmitEquivocationProof trait, which deals with extrinsics internally.

This means that tests don't have to deal with extrinsics, which is way more nasty.

rphmeier · 2019-08-18T10:43:27Z

core/consensus/babe/src/lib.rs

+			});
+			info!(target: "afg", "Babe equivocation report has been submitted")
+		} else {
+			error!(target: "afg", "Error constructing Babe equivocation report")


rphmeier · 2019-08-18T10:44:18Z

core/consensus/babe/src/tests.rs

 				time_source: Default::default(),
-				transaction_pool : Default::default(),
+				// TODO [slashing] get the transaction pool from somewhere.
+				transaction_pool: Default::default(),


see -- you have trouble writing the test because you've tried to push too much onto the BABE logic.

rphmeier · 2019-08-18T10:45:02Z

core/consensus/common/primitives/src/lib.rs

 use client::decl_runtime_apis;
 use rstd::vec::Vec;

+/// Represents an Babe equivocation proof.


but the crate is consensus-common

rphmeier · 2019-08-18T10:45:09Z

core/consensus/common/primitives/src/lib.rs

+#[derive(Clone, PartialEq, Eq, Decode, Encode)]
+pub struct EquivocationProof<H, P, S> {
+	pub reporter: P,
+	pub identity: P,


rphmeier · 2019-08-18T10:45:49Z

core/consensus/common/primitives/src/lib.rs

+#[cfg_attr(feature = "std", derive(Debug))]
+#[derive(Clone, PartialEq, Eq, Decode, Encode)]
+pub struct EquivocationProof<H, P, S> {
+	pub reporter: P,


why should the proof care? runtime should just infer this from the transaction author.

rphmeier · 2019-08-18T10:47:01Z

core/consensus/common/primitives/src/lib.rs

+pub struct EquivocationProof<H, P, S> {
+	pub reporter: P,
+	pub identity: P,
+	pub slot: u64,


should use a strong type alias as in the one from babe primitives.

I'm not sure why consensus-common needs to define this type. Trying to unify things like equivocation proofs across all consensus engines is going to fail.

rphmeier · 2019-08-18T10:48:16Z

core/consensus/slots/src/lib.rs

-#![deny(warnings)]
-#![forbid(unsafe_code, missing_docs)]
+// #![deny(warnings)]
+// #![forbid(unsafe_code, missing_docs)]


don't comment out code

it should be either removed or left there

rphmeier · 2019-08-18T10:49:56Z

core/finality-grandpa/primitives/src/lib.rs

+#[derive(Clone, PartialEq, Eq, Encode, Decode)]
+pub struct GrandpaEquivocation<H, N> {
+	/// Reporter of the equivocation.
+	pub reporter: AuthorityId,


why does it matter here?

rphmeier · 2019-08-18T10:50:05Z

core/finality-grandpa/primitives/src/lib.rs

+	(message, round, set_id).encode()
+}
+
+// TODO [slashing] consider removing and use generic `EquivocationProof`.


rphmeier · 2019-08-18T10:50:30Z

core/finality-grandpa/primitives/src/lib.rs

+	pub second: (Message<H, N>, AuthoritySignature),
+}
+
+pub type GrandpaEquivocationFrom<Block> = GrandpaEquivocation<


docs? convention is Of, not From

rphmeier · 2019-08-18T10:51:10Z

core/finality-grandpa/src/environment.rs

 	pub(crate) network: crate::communication::NetworkBridge<Block, N>,
 	pub(crate) set_id: u64,
 	pub(crate) voter_set_state: SharedVoterSetState<Block>,
+	pub(crate) transaction_pool: Arc<T>,


Again: generation of the extrinsic is beyond the scope of this structure.

rphmeier · 2019-08-18T10:51:31Z

core/finality-grandpa/src/environment.rs

+				reporter,
+				round_number: equivocation.round_number,
+				// TODO [slashing] get proper session index.
+				session_index: SessionIndex::default(),


why do you want to do this in the node and not in the runtime?

rphmeier · 2019-08-18T10:59:26Z

node/runtime/src/lib.rs

 impl babe::Trait for Runtime {
 	type EpochDuration = EpochDuration;
 	type ExpectedBlockTime = ExpectedBlockTime;
+	type IdentificationTuple = historical::IdentificationTuple<Self>;


this is redundant because it can be accessed by the KeyOwnerSystem. (it should be called KeyOwnerProofSystem)

rphmeier · 2019-08-18T10:59:45Z

node/runtime/src/lib.rs


 impl grandpa::Trait for Runtime {
 	type Event = Event;
+	type IdentificationTuple = historical::IdentificationTuple<Self>;


also redundant

rphmeier · 2019-08-18T11:02:06Z

srml/babe/src/lib.rs

+	if equivocation.slot == first_slot && first_slot == second_slot {
+		let author = &equivocation.identity;
+
+		if !author.verify(&first_header.hash(), &equivocation.first_signature) {


this is a serious misunderstanding of BABE - the hash includes the seal includes the signature, so it will always fail. I'm not sure why the entire header needs to go into the proof at all.

rphmeier · 2019-08-18T11:04:22Z

srml/babe/src/lib.rs

+				return Err("invalid equivocation")
+			}
+
+			let to_punish = <T as Trait>::KeyOwnerSystem::check_proof(


I would prefer if the proof was checked first as opposed to afterwards

rphmeier · 2019-08-18T11:07:32Z

srml/babe/src/lib.rs

+			let raw_payload = (function, extra.clone(), genesis_hash, genesis_hash);
+
+			let maybe_signature: Option<app::Signature> = raw_payload.using_encoded(|payload| if payload.len() > 256 {
+				reporter.sign(&sr_io::blake2_256(payload))


have you tested that this actually works? I think you're assuming that authority key is an account-crypto key.

rphmeier · 2019-08-18T11:09:50Z

srml/grandpa/src/lib.rs

+
+	let identity = &equivocation.identity;
+
+	if first_vote == second_vote { // TODO: fix equality before merging


rphmeier

The APIs could be cleaner and consider relationships and responsibilities of types more carefully.

Needs tests (as I'm not convinced this will work as-is), and there are a number of un-done TODOs

tomusdrw · 2019-08-21T06:33:13Z

The commit history of this PR grew too much and there is still a lot of work that needs changing. This PR is going to be superseded by a new branch where we will copy over the correct parts.

Closing it now to save the CI and reviewers cycles.

tomusdrw and others added 19 commits August 6, 2019 16:28

Remove offline slashing logic from staking.

4f24928

Initial version of reworked offence module, can report offences

9f1cad1

Merge branch 'retreat-slashing' of github.com:paritytech/substrate in…

3fc6eaa

…to retreat-slashing

Clean up staking example.

89e623b

Commit SlashingOffence

0e1d398

Force new era on slash.

d5c9cd1

Add offenders in the SlashingOffence trait.

45e5907

Introduce the ReportOffence trait.

b93118f

Rename Offence.

051001a

Add on_before_session_ending handler.

2dae936

Move offence related stuff under sr-primitives.

5c3a249

Fix cargo check.

aaa8159

Import new im-online implementation.

06ee813

Adding validator count to historical session storage as it's needed f…

a807dae

…or slash calculations

Add a comment about offence.

32cba20

Add BabeEquivocationOffence

39f7ed0

GrandpaEquivocationOffence

c5579b3

slash_fraction and fix

ecd8d81

current_era_start_session_index

cfe194d

marcio-diaz force-pushed the marcio/all-equivocation-0 branch from ac9ff84 to c6f1ed4 Compare August 7, 2019 07:47

andresilva reviewed Aug 7, 2019

View reviewed changes

pepyakin and others added 9 commits August 7, 2019 12:19

UnresponsivnessOffence

4cb0bfe

Finalise OnOffenceHandler traits, and stub impl for staking.

7ede025

slash_fraction doesn't really need &self

a56a7b4

Note that offenders count is greater than 0

da2cc83

Merge branch 'master' into retreat-slashing

0e2047a

Add a test to ensure that I got the math right

61c6c22

Merge remote-tracking branch 'origin/retreat-slashing' into retreat-s…

c94f253

…lashing

Use FullIdentification in offences.

f7a8711

Use FullIndentification.

7b14a37

seerscode added 2 commits August 17, 2019 10:35

Move implementations of construct_equivocation to srmls.

8530934

Make tests compile.

4dde8a2

rphmeier reviewed Aug 18, 2019

View reviewed changes

rphmeier suggested changes Aug 18, 2019

View reviewed changes

seerscode added 3 commits August 19, 2019 22:07

Merge master

e3cbd31

Fix node-template

9348769

Merge master

8fe54c3

tomusdrw closed this Aug 21, 2019

marcio-diaz deleted the marcio/all-equivocation-0 branch September 16, 2019 01:19


		let identity = &equivocation.identity;

		if first_vote == second_vote { // TODO: fix equality before merging

Conversation

marcio-diaz commented Aug 6, 2019

Uh oh!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcio-diaz Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier Aug 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier Aug 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcio-diaz Aug 7, 2019 •

edited

Loading

rphmeier Aug 18, 2019 •

edited

Loading

rphmeier Aug 18, 2019 •

edited

Loading

rphmeier Aug 18, 2019 •

edited

Loading