Submit equivocation reports by marcio-diaz · Pull Request #2800 · paritytech/substrate

marcio-diaz · 2019-06-05T14:24:31Z

No description provided.

Does not work

Fixes 6e253c8

Also remove some in-progress code that would not wind up being useful anyway.

rphmeier · 2019-06-11T16:07:40Z

core/finality-grandpa/src/tests.rs

+	delay: Mutex<Option<std::sync::mpsc::Receiver<()>>>,
+}
+
+impl txpool::ChainApi for TestPoolApi {


(e.g. if we didn't tightly couple to PoolApi we could have a much more useful Report hook for tests).

rphmeier · 2019-06-11T16:09:28Z

core/consensus/babe/src/lib.rs

+					equiv_proof.first_header().hash(),
+					equiv_proof.second_header().hash(),
 				);
+				// transaction_pool.as_ref().map(|txpool|


commented-out code?

rphmeier · 2019-06-11T16:10:43Z

core/consensus/safety/src/lib.rs

+/// Submit report call to the transaction pool.
+pub fn submit_report_call<C, T, Block>(
+	client: &Arc<C>,
+	transaction_pool: &Arc<T>,


These seem like they could both be &X instead of &Arc<X>. Why leak Arc into it?

rphmeier · 2019-06-11T16:11:28Z

core/finality-grandpa/src/environment.rs

 	) {
 		warn!(target: "afg", "Detected prevote equivocation in the finality worker: {:?}", equivocation);
-		// nothing yet; this could craft misbehavior reports of some kind.
+		let proof = (self.set_id, round, equivocation).encode();


Would be nice to have this be a struct in finality-grandpa-primitives.

rphmeier · 2019-06-11T16:12:13Z

core/finality-grandpa/src/environment.rs

+		submit_report_call(
+			&self.inner,
+			&self.transaction_pool,
+			Call::Grandpa(GrandpaCall::report_precommit_equivocation(equivocation.encode())),


prevote equivocations take the (set_id, round) as well. That seems right to me -- any reason it's only equivocation here?

rphmeier · 2019-06-11T16:12:51Z

srml/aura/Cargo.toml

-serde = { version = "1.0", optional = true }
+serde = { version = "1.0", optional = true, features = ["derive"] }
+
+# substrate-primitives = { package = "substrate-primitives", path = "../../core/primitives" }


commented-out?

rphmeier · 2019-06-11T16:13:32Z

srml/aura/src/lib.rs

+
+		let proof_is_valid = fst_author.map_or(false, |f| snd_author.map_or(false, |s| f == s));
+
+		if  proof_is_valid {


Suggested change

if proof_is_valid {

if proof_is_valid {

rphmeier · 2019-06-11T16:14:03Z

srml/aura/src/lib.rs

+	None
+}
+
+fn handle_equivocation_proof<T>(proof: &Vec<u8>) -> TransactionValidity


Suggested change

fn handle_equivocation_proof<T>(proof: &Vec<u8>) -> TransactionValidity

fn handle_equivocation_proof<T>(proof: &[u8]) -> TransactionValidity

rphmeier · 2019-06-11T16:17:25Z

srml/aura/src/lib.rs

-	pub struct Module<T: Trait> for enum Call where origin: T::Origin { }
+	pub struct Module<T: Trait> for enum Call where origin: T::Origin {
+		/// Report equivocation in block production.
+		fn report_equivocation(_origin, _equivocation_proof: Vec<u8>) {


Why can't the equivocation_proof be strongly typed as AuraEquivocationProof?

Do I understand right that this is invoked as a no-op after ValidateUnsigned checks the call and this is the place where slashing::slash would be invoked?

Yes, exactly.

rphmeier · 2019-06-11T16:17:50Z

srml/grandpa/src/lib.rs

+
+	type Signature: Verify + Codec + Clone;
+	// type DigestItem: CompatibleDigestItem<Self::Signature>
+	// 	+ DigestItem<Hash = Self::Hash>;


more commented-out code

rphmeier · 2019-06-11T16:20:14Z

srml/grandpa/src/lib.rs

-			// FIXME: https://github.com/paritytech/substrate/issues/1112
-		}
+		/// Report Prevote equivocation in Grandpa.
+		fn report_prevote_equivocation(_origin, _equivocation_proof: Vec<u8>) {}


likewise here: I'm surprised we can't strongly-type the call.

rphmeier · 2019-06-11T16:20:34Z

srml/grandpa/src/lib.rs

-		/// Report some misbehavior.
-		fn report_misbehavior(origin, _report: Vec<u8>) {
-			ensure_signed(origin)?;
-			// FIXME: https://github.com/paritytech/substrate/issues/1112


is this issue fixed?

It's going to be handled here (grandpa equivocations), I'll link it.

rphmeier

Generally looks decent. A bunch of style issues, some questions, and some architecture issues that need to be addressed.
Most importantly, the SubmitReport trait instead of tightly coupling to the transaction pool would be nice -- unless there is a strong justification for it.

Blocking on:

Test that equivocations really do lead to reports in GRANDPA
Test that equivocations really do lead to reports in Aura
Test that equivocation reports are evaluated correctly on the runtime side.

rphmeier · 2019-06-12T13:29:28Z

core/consensus/safety/src/lib.rs

+
+use client;
+use transaction_pool::txpool::{self, PoolApi};
+use node_runtime::{UncheckedExtrinsic, Call};


I didn't catch this in the first review, but coupling this to the node-runtime is wrong. We'll need general utilities for constructing calls.

Nothing in core should depend on anything in node.

rphmeier · 2019-06-14T19:02:35Z

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

 		fn slot_duration() -> u64;
+
+		/// Construct a call to report the equivocation.
+		fn construct_equiv_report_call(proof: AuraEquivProof<<Block as BlockT>::Header, Signature>) -> Vec<u8>;


style: no compact names. prefer construct_equivocation_report_call and AuraEquivocationProof

I wonder if these constructor functions should/can be automatically generated using some kind of annotation in the target function.

It would be cool to auto-generate them. I'm not sure what you mean by the target though

The idea was to decorate the function using something like #[create_constructor]:

decl_module! { pub struct Module<T: Trait> for enum Call where origin: T::Origin { #[create_constructor] fn report_equivocation(...) { } } }

to avoid polluting the API declaration. That should implicitly implement the following runtime API call:

fn construct_report_equivocation_call(...) -> Vec<u8> { let report_call = Call::Aura(AuraCall::report_equivocation(proof)); let extrinsic = UncheckedExtrinsic::new_unsigned(report_call); extrinsic.encode() }

But now I don't like so much the idea of creating implicit API functions. (By implicit I mean that doesn't appear under decl_runtime_apis!)

Maybe @bkchr has an opinion on this.

I would really like to have implicit API functions, but this does not work. You can not export a function that involves some generics.

seerscode and others added 30 commits May 21, 2019 10:40

feat: give access to tx pool from import_queue

fc0e8de

feat: add tx pool to submit report call

1c81334

feat: start using validate_transaction for unsigned

2f1a9cb

fix: is_valid() works

beec144

clean

fac3b7a

fix: replace crypto::Pair by ed25519::Pair to get rid of std, :/

682232c

fix: use verify

365eab0

compiles

b4f4d0c

fix: is_valid to srml

c4e03e1

chore: remove unused bonds

ca29674

fix: remove is_valid

ef25821

fix: merge

93216fa

fix: merge issues

0ef9bd0

Merge branch 'master' into marcio/equivocation-aura-complete

7ca3d66

chore: refactoring

5819b99

Try to fix runtime panic

902ca43

Does not work

Merge remote-tracking branch 'origin/master' into z

ed508f2

feat: add safety module

66015a3

Merge remote-tracking branch 'origin/master' into z

86e6db9

Fix trivial typo

c4d31ee

Remove not used imports

3b6c922

Add trace

3f39ae0

Add runtime support for PreRuntime and Consensus digests

c2a9786

Fixes 6e253c8

Fix silly compile error.

fbdccd8

Fix overly-long lines

79f195e

Also remove some in-progress code that would not wind up being useful anyway.

Merge branch 'master' into marcio/equivocation-aura-complete

e193bfe

Move babe equiv

8b7a39b

Add signature to equivocation proof

e48bcb0

Fix comments

040d382

Add imports

37c59a1

rphmeier reviewed Jun 11, 2019

View reviewed changes

rphmeier suggested changes Jun 11, 2019

View reviewed changes

rphmeier added A5-grumble and removed A0-please_review Pull request needs code review. labels Jun 11, 2019

seerscode added 7 commits June 12, 2019 10:22

Add enum equiv proof

d506d64

Add SubmitReport trait

a30bd0f

Rename EquivocationProof

4c5ae98

Use SubmitReport trait

db09467

Remove Arc

220da0b

Remove poolapi from environment

fef763c

Remove uncommented code

80f4840

rphmeier reviewed Jun 12, 2019

View reviewed changes

seerscode added 3 commits June 13, 2019 06:44

Add Aura Api call

7fe6451

Use Pair from ed25519

1b5de6d

Add grandpa api calls

69565db

rphmeier reviewed Jun 14, 2019

View reviewed changes

marcio-diaz closed this Jun 21, 2019

marcio-diaz mentioned this pull request Jun 21, 2019

Equivocation misbehaviour reports #2923

Closed

bkchr deleted the mar-report-equivocation-grandpa branch June 21, 2019 12:30


		let proof_is_valid = fst_author.map_or(false, \|f\| snd_author.map_or(false, \|s\| f == s));

		if proof_is_valid {

	fn handle_equivocation_proof<T>(proof: &Vec<u8>) -> TransactionValidity
	fn handle_equivocation_proof<T>(proof: &[u8]) -> TransactionValidity

Conversation

marcio-diaz commented Jun 5, 2019

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!

marcio-diaz Jun 12, 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!

rphmeier left a comment • 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!

rphmeier Jun 12, 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!

marcio-diaz Jun 15, 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!

marcio-diaz Jun 20, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

marcio-diaz Jun 12, 2019 •

edited

Loading

rphmeier left a comment •

edited

Loading

rphmeier Jun 12, 2019 •

edited

Loading

marcio-diaz Jun 15, 2019 •

edited

Loading

marcio-diaz Jun 20, 2019 •

edited

Loading