Init store for slots-headers #2492

marcio-diaz · 2019-05-06T19:05:10Z

Starting PR for issue paritytech/polkadot-sdk#70.

Introduces a map in the Aux Storage: slot -> vec![(header, signer)].
It contains the last MAX_SLOT_CAPACITY slots.
It's pruned when reaching PRUNING_BOUND % slot_now == 0, with PRUNING_BOUND > MAX_SLOT_CAPACITY. Note: I don't know if the pruning may be too slow.
The size of the vector is O(validators) in the worst scenario, since we save only one header by signer, i.e. duplicates are ignored and different headers generate equivocations.
When checking headers we use the map for checking for equivocations and throw an error if it's the case. Follow up PR will produce a report.

andresilva · 2019-05-08T11:02:25Z

core/consensus/slots/src/aux_schema.rs

+}
+
+/// Check if the header is an equivocation and returns the proof in that case.
+/// Assumes all the headers in the same slot are signed by the same Signer.


I don't think we can make this assumption for BABE (remember that uncles are allowed). I'm also not sure it holds for Aura in the presence of forks.

Ok, I'm checking the public key now, it's executed after verification.

andresilva · 2019-05-08T11:05:24Z

core/consensus/slots/src/aux_schema.rs

+		slot_header_map = slot_header_map.split_off(&(slot - MAX_SLOT_CAPACITY));
+	}
+
+	backend.insert_aux(


Maybe we can come up with a simple way to avoid dumping everything to disk. If we store the different elements under different keys (e.g. SLOT_HEADER_MAP_KEY + index), then we'd only need to delete one key and add a new one as we import new blocks. Not sure this is strictly needed for this PR.

Fixed by adding SLOT_HEADER_MAP_KEY + index as keys.

andresilva · 2019-05-08T11:06:54Z

core/consensus/aura/src/lib.rs

-			Ok(CheckedHeader::Checked(header, digest_item))
+			match check_equivocation(client, slot_num, header.clone()) {
+				Ok(Some(equivocation_proof)) => {
+					// TODO: dispatch report here.


Do we need to add a reporting module as part of this PR?

A runtime module? I think we'll hold that off for another PR. srml-aura should get a function for checking equivocation, given two headers.

Demi-Marie

Commented-out code needs to be either removed or finished, and a potential denial of service attack from many equivocations needs to be addressed.

Demi-Marie · 2019-05-08T20:14:57Z

core/consensus/aura/src/lib.rs

+			match check_equivocation::<_, _, <P as Pair>::Public>(client, slot_num, header.clone(), public.clone()) {
+				Ok(Some(equivocation_proof)) => {
+					// TODO: dispatch report here.
+					Err(format!("Slot author is equivocating with headers {:?} and {:?}",


We should log this with severity at least warn, as it should not happen normally.

I added it as info.

Demi-Marie · 2019-05-08T20:18:02Z

core/consensus/slots/src/aux_schema.rs

+	let mut v = load_decode::<_, Vec<(H, P)>>(backend.clone(), &key[..])?
+		.unwrap_or_else(Vec::new);
+
+	for (prev_header, prev_signer) in v.iter() {


This is O(n), and is called O(n) times, so it is O(n^2) (where n is the number of messages from any particular validator).

I think is O(validators) now, worst case scenario, since we store only one message by validated signer.

Demi-Marie · 2019-05-08T20:19:02Z

core/consensus/slots/src/aux_schema.rs

+		}
+	}
+
+	// match slot_header_map.entry(slot) {


The commented-out code should either be removed or uncommented.

Demi-Marie · 2019-05-08T20:19:09Z

core/consensus/slots/src/aux_schema.rs

+	// };
+
+	if slot % PRUNING_BOUND == 0 {
+		// slot_header_map = slot_header_map.split_off(&(slot - MAX_SLOT_CAPACITY));


marcio-diaz · 2019-05-09T15:40:31Z

I addressed the previous issues and it's ready for another round of review.

fixed

DemiMarie-temp

The algorithm for finding equivocations is rather subtle. Would adding some comments be a good idea?

DemiMarie-temp · 2019-05-13T14:27:18Z

core/consensus/aura/src/lib.rs

+				public.clone()
+			) {
+				Ok(Some(equivocation_proof)) => {
+					info!(


Can the code duplication between this and the following statement be removed?

DemiMarie-temp · 2019-05-13T14:28:28Z

core/consensus/aura/src/lib.rs

+					Ok(CheckedHeader::Checked(header, digest_item))
+				},
+				Err(e) => {
+					println!("{}", e.to_string());


This should be logged as error!, not written to stdout.

DemiMarie-temp · 2019-05-13T14:34:08Z

core/consensus/slots/src/aux_schema.rs

+					snd_header: header.clone(),
+				}));
+			} else {
+				return Ok(None)


This appears to be correct, since at least the first equivocation will be reported. That said, I suspect a comment explaining why would be justified.

I added some comments, hope it helps.

gavofyork · 2019-05-14T10:14:05Z

@demimarie-parity happy with this now?

andresilva · 2019-05-14T11:30:47Z

core/consensus/babe/src/lib.rs

@@ -1103,4 +1163,40 @@ mod tests {
 			Keyring::Charlie.into()
 		]);
 	}
+


stray newline

andresilva · 2019-05-14T11:30:56Z

core/consensus/aura/src/lib.rs


 pub use aura_primitives::*;
 pub use consensus_common::{SyncOracle, ExtraVerification};

+


stray newline

andresilva · 2019-05-14T11:32:07Z

core/consensus/slots/src/aux_schema.rs

+	}
+}
+
+#[derive(Debug, Clone)]


Missing docs on public item

andresilva · 2019-05-14T11:32:22Z

core/consensus/slots/src/aux_schema.rs

+}
+
+impl<H> EquivocationProof<H> {
+	pub fn slot(&self) -> u64 {


andresilva · 2019-05-14T11:35:55Z

core/consensus/slots/src/aux_schema.rs

+	where
+		H: Header,
+		C: AuxStore,
+		P: Encode + Decode + PartialEq + std::fmt::Debug,


Debug bound seems unnecessary.

andresilva · 2019-05-14T11:36:18Z

core/primitives/src/crypto.rs

@@ -290,7 +290,7 @@ impl<T: AsMut<[u8]> + AsRef<[u8]> + Default + Derive> Ss58Codec for T {
 #[cfg(feature = "std")]
 pub trait Pair: Sized + 'static {
 	/// TThe type which is used to encode a public key.
-	type Public;
+	type Public: Clone + std::fmt::Debug;


I don't think these are necessary.

andresilva

underflow

core/consensus/slots/src/aux_schema.rs

Co-Authored-By: André Silva <andre.beat@gmail.com>

* init store for slots * fix: add check_equivocation to Aura/Babe * fix tests * fix: add pruning bound Co-Authored-By: André Silva <andre.beat@gmail.com> * use saturating_sub

seerscode added 5 commits May 6, 2019 21:00

init store for slots

aab5005

aux store compiles

4dc1bde

fix: add check_equivocation to Aura/Babe

07d6877

fix: add aura test

0fc8b34

fix tests

b43f8b2

marcio-diaz marked this pull request as ready for review May 7, 2019 17:26

marcio-diaz requested review from rphmeier and andresilva May 7, 2019 17:27

marcio-diaz force-pushed the marcio/equivocation-block-production branch from 1ccf396 to 93b70bf Compare May 7, 2019 17:30

chore: clean a bit

c0dfff5

marcio-diaz force-pushed the marcio/equivocation-block-production branch from 93b70bf to c0dfff5 Compare May 7, 2019 17:38

fix: add pruning bound

86b488c

marcio-diaz added the A0-please_review Pull request needs code review. label May 8, 2019

marcio-diaz force-pushed the marcio/equivocation-block-production branch from 997bb08 to 0a03d71 Compare May 8, 2019 08:48

fix: use map len

01a38e3

marcio-diaz force-pushed the marcio/equivocation-block-production branch from 0a03d71 to 01a38e3 Compare May 8, 2019 08:55

andresilva previously requested changes May 8, 2019

View reviewed changes

rphmeier requested a review from Demi-Marie May 8, 2019 18:57

fix: add public to map

6f53d0d

Demi-Marie previously requested changes May 8, 2019

View reviewed changes

fix: pruning, but still ugly

696bc7b

marcio-diaz force-pushed the marcio/equivocation-block-production branch 2 times, most recently from 097392b to 502d47e Compare May 9, 2019 11:48

fix: simplify

5ee6d79

marcio-diaz force-pushed the marcio/equivocation-block-production branch from 502d47e to 5ee6d79 Compare May 9, 2019 12:02

seerscode added 2 commits May 9, 2019 15:47

fix: add logs

77d326f

fix: tests

3076793

marcio-diaz requested review from Demi-Marie and andresilva May 9, 2019 16:30

gavofyork added the A3-needsresolving label May 10, 2019

fix: merge conflicts

eb6d098

marcio-diaz force-pushed the marcio/equivocation-block-production branch from 21c00a9 to eb6d098 Compare May 13, 2019 08:08

marcio-diaz removed the A3-needsresolving label May 13, 2019

DemiMarie-temp suggested changes May 13, 2019

View reviewed changes

seerscode added 2 commits May 14, 2019 09:07

chore: add comments

f7780a4

chore: add log str

d975e01

andresilva suggested changes May 14, 2019

View reviewed changes

fix: add comments, remove unnecessary bounds

a821f9e

marcio-diaz force-pushed the marcio/equivocation-block-production branch from 8da401f to a821f9e Compare May 14, 2019 13:27

gavofyork requested a review from andresilva May 14, 2019 15:51

andresilva suggested changes May 14, 2019

View reviewed changes

core/consensus/slots/src/aux_schema.rs Outdated Show resolved Hide resolved

core/consensus/slots/src/aux_schema.rs Outdated Show resolved Hide resolved

seerscode and others added 2 commits May 14, 2019 18:38

use saturating_sub

79f3791

Co-Authored-By: André Silva <andre.beat@gmail.com>

use saturating_sub

f814381

Co-Authored-By: André Silva <andre.beat@gmail.com>

andresilva approved these changes May 14, 2019

View reviewed changes

seerscode added 5 commits May 15, 2019 01:57

fix: bug

a1cec61

fix: remove clone

784f9de

fix: reintroduce forbid warnings

1029f81

fix: default slot saved

26d1208

fix: improve a bit tests

a6ab02d

marcio-diaz merged commit 541e9c8 into master May 15, 2019

marcio-diaz deleted the marcio/equivocation-block-production branch May 15, 2019 10:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Init store for slots-headers #2492

Init store for slots-headers #2492

marcio-diaz commented May 6, 2019 •

edited

Loading

andresilva May 8, 2019

marcio-diaz May 9, 2019

andresilva May 8, 2019

marcio-diaz May 9, 2019

andresilva May 8, 2019

rphmeier May 8, 2019 •

edited

Loading

Demi-Marie left a comment

Demi-Marie May 8, 2019

marcio-diaz May 9, 2019

Demi-Marie May 8, 2019

marcio-diaz May 9, 2019 •

edited

Loading

Demi-Marie May 8, 2019

Demi-Marie May 8, 2019

marcio-diaz commented May 9, 2019

DemiMarie-temp left a comment

DemiMarie-temp May 13, 2019

DemiMarie-temp May 13, 2019

DemiMarie-temp May 13, 2019

marcio-diaz May 14, 2019

gavofyork commented May 14, 2019

andresilva May 14, 2019

andresilva May 14, 2019

andresilva May 14, 2019

andresilva May 14, 2019

andresilva May 14, 2019

andresilva May 14, 2019

andresilva left a comment


		pub use aura_primitives::*;
		pub use consensus_common::{SyncOracle, ExtraVerification};

Init store for slots-headers #2492

Init store for slots-headers #2492

Conversation

marcio-diaz commented May 6, 2019 • 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 May 8, 2019 • edited Loading

Choose a reason for hiding this comment

Demi-Marie left a 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

marcio-diaz May 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcio-diaz commented May 9, 2019

DemiMarie-temp left a 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

gavofyork commented May 14, 2019

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

andresilva left a comment

Choose a reason for hiding this comment

marcio-diaz commented May 6, 2019 •

edited

Loading

rphmeier May 8, 2019 •

edited

Loading

marcio-diaz May 9, 2019 •

edited

Loading