Slash and prove membership of prior sessions #2970

rphmeier · 2019-06-27T17:26:49Z

Background

We want Substrate chains to have a property known as "accountable safety": that misbehavior leads to a slash of bonded funds. The srml-staking library keeps those funds bonded for a certain number of eras.

Storing all historical validator sets and session keys for the bonding period would be expensive. So instead we only keep a trie root which can be used to prove historical ownership of keys. These trie roots are pruned after some time.

When we slash a validator, we want to also slash their nominators. Furthermore, we want to slash only the nominators at the time of misbehavior. And only for the amount that they were nominated on the misbehaving validator at that time.

Implementation

The staking module decides how many prior sessions we keep, but prior sessions themselves are managed by a session::historical module. Pruning is O(n) in the number of sessions to prune.

What the session::historical trie roots reference is a mapping of (KeyTypeId, Vec<u8>) -> FullIdentification. It is populated with all the of session keys of that session and the FullIdentification of the owning validator.

The FullIdentification in practice is a staking::Exposure object that tells us who to slash when this key has misbehaved. This lets us slash the correct group of people for the right amounts.

I moved session key deduplication to use raw storage APIs so we could make sure all trie keys were prefixed with a constant. This means that they no longer interfere with the growth of the rest of the trie.

There is a generic trait KeyOwnerProofSystem for key ownership proofs. Consensus modules should use something along these lines (pseudocode):

trait Trait {
    // if your key type implements `AsRef<[u8]>` _and_ the `AsRef` value is the same as the encoded
    // then use that instead of `Vec<u8>`.
    type KeyOwnerSystem: KeyOwnerProofSystem<(KeyTypeId, Vec<u8>)>;
}

impl<T: Trait> Module<Trait> {
    // called off-chain.
    fn generate_misbehavior_report_call() -> MyMisbehaviorReportCall {
        // invoke `KeyOwnerSystem::prove((key_type_id, key_data))` and put the proof in the call.
    }

    // called on-chain.
    fn check_misbehavior(call: MyMisbehaviorReportCall) {
         let session_key = call.misbehaving_key;
         let proof = call.membership_proof;

         let to_punish = match KeyOwnerSystem::check_proof((key_type_id, session_key), proof) {
             None => return,
             Some(x) => x,
         };

         // check that the key really did misbehave.

         // if so, invoke something like `slashing::slash(to_punish, misbehavior_id)`.
    }
}

…h-slash-old

bkchr · 2019-07-05T08:19:08Z

core/sr-primitives/src/testing.rs

-	fn get_raw(&self, _: usize) -> &[u8] { unsafe { &std::mem::transmute::<_, &[u8; 8]>(&self.0)[..] } }
-	fn get<T: Decode>(&self, _: usize) -> Option<T> { self.0.using_encoded(|mut x| T::decode(&mut x)) }
+	fn get_raw(&self, _: KeyTypeId) -> &[u8] { unsafe {
+		std::slice::from_raw_parts(&self.0 as *const _ as *const u8, std::mem::size_of::<u64>())


Ohh :D
At least format it better:

fn get_raw(&self, _: KeyTypeId) -> &[u8] { unsafe { std::slice::from_raw_parts(&self.0 as *const _ as *const u8, std::mem::size_of::<u64>()) } }

core/sr-primitives/src/traits.rs

bkchr · 2019-07-05T09:04:53Z

core/trie/src/lib.rs

@@ -36,6 +36,8 @@ pub use node_codec::NodeCodec;
 pub use trie_db::{Trie, TrieMut, DBValue, Recorder, Query};
 /// Various re-exports from the `memory-db` crate.
 pub use memory_db::{KeyFunction, prefixed_key};
+/// Various re-exports from the `hash-db` crate.
+pub use hash_db::{HashDB as HashDBT};


Suggested change

pub use hash_db::{HashDB as HashDBT};

pub use hash_db::HashDB as HashDBT;

srml/session/src/historical.rs

bkchr · 2019-07-05T09:32:37Z

srml/session/src/historical.rs

+		<CachedObsolete<T>>::remove(&ending);
+
+		if let Some((new_validators, old_exposures))
+			= <I as OnSessionEnding<_, _>>::on_session_ending(ending, applied_at)


let (new_validators, old_exposures) = <I as OnSessionEnding<_, _>>::on_session_ending(ending, applied_at); // every session from `ending+1 .. applied_at` now has obsolete `FullIdentification` // now that a new validator election has occurred. // we cache these in the trie until those sessions themselves end. for obsolete in (ending + 1) .. applied_at { <CachedObsolete<T>>::insert(obsolete, &old_exposures); } Some(new_validators)

Is maybe a little bit better at reading?

let (new_validators, old_exposures) = <I as OnSessionEnding<_, _>>::on_session_ending(ending, applied_at)

I assume you wanted a ? there?

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

dvc94ch

LGTM

cheme

I put a few safety check remarks (I do not know everything well).
Regarding trie stuff, the mechanism seems pretty heavy, but I do not see a reason with this implementation that will make a future switch to child_trie impossible, it really depend on the size of this trie (if it is a few entry the system here seems fine).

In srml/session/src/lib.rs , I wonder if the DEDUP_KEY prefixed storage couldn't simply be a traditional mapping srml storage (the code seems pretty close to what the macro produce).

cheme · 2019-07-05T10:55:43Z

core/sr-primitives/src/testing.rs

-	fn get_raw(&self, _: usize) -> &[u8] { unsafe { &std::mem::transmute::<_, &[u8; 8]>(&self.0)[..] } }
-	fn get<T: Decode>(&self, _: usize) -> Option<T> { self.0.using_encoded(|mut x| T::decode(&mut x)) }
+	fn get_raw(&self, _: KeyTypeId) -> &[u8] { unsafe {
+		std::slice::from_raw_parts(&self.0 as *const _ as *const u8, std::mem::size_of::<u64>())


Can it lead to issue in scenario where we got a combination of wasm execution and native exectution in a test case (for native envt not in le (wasm is le, i am not sure anymore))?
Or is get_raw only use locally to one of those execution?

well, not any more so than the old code

cheme · 2019-07-05T11:02:01Z

core/sr-primitives/src/testing.rs

 impl OpaqueKeys for UintAuthorityId {
-	fn count() -> usize { 1 }
+	type KeyTypeIds = std::iter::Cloned<std::slice::Iter<'static, KeyTypeId>>;


wonder if non cloned iter is possible here and could be more flexible? (totally not sure just wondering).

we could do an iter::Once chain but I don't see why it matters. it should be fast regardless

yes it is just the key type id, please forget about that

Could be replaced by std::iter::Copied

cheme · 2019-07-05T12:03:36Z

srml/session/src/lib.rs

-	{
-		let i = Inner::find_author(digests)?;
+			ensure!(
+				Self::key_owner(id, key).map_or(true, |owner| &owner == who),


Does the &owner == who condition means that we allow exchanging key (probably a very corner case)?

what it means is that we're allowed to set key A without changing key B.

cheme · 2019-07-05T12:04:51Z

srml/session/src/lib.rs

-		validators.get(i as usize).map(|k| k.clone())
-	}
-}
+			// ensure keys are without duplication.


I understand this code as update ownership only if key change.

right, needs to be moved up a bit

cheme · 2019-07-05T12:11:50Z

srml/session/src/lib.rs

+fn dedup_trie_key<T: Trait, K: Encode>(key: &K) -> [u8; 32 + DEDUP_KEY_LEN] {
+	key.using_encoded(|s| {
+		// take at most 32 bytes from the hash of the value.
+		let hash = <T as system::Trait>::Hashing::hash(s);


Do we need to hash the key (are validatorId unique or keytypeid unique)?

well, we don't strictly-speaking have to hash the key. but it may make it harder to bias the tree

yes really depends on the nature of the keys.

cheme · 2019-07-05T12:53:28Z

srml/session/src/historical.rs

+		// now that a new validator election has occurred.
+		// we cache these in the trie until those sessions themselves end.
+		for obsolete in (ending + 1) .. applied_at {
+			<CachedObsolete<T>>::insert(obsolete, &old_exposures);


this is pretty heavy redundancy over old_exposures but I have no idea if the case where there is multiple index is frequent. If frequent, maybe an intermeediate maping 'session->range' and 'range->cacheobsolet' can be good.

see other comment -- in practice here it shouldn't be an issue because (ending + 1) .. applied_at will have only 1 item in the current runtime. The range thing is a bit harder to GC.

cheme · 2019-07-05T12:53:54Z

srml/session/src/historical.rs

+		/// Mapping from historical session indices to session-data root hash.
+		HistoricalSessions get(historical_root): map SessionIndex => Option<T::Hash>;
+		/// Queued full identifications for queued sessions whose validators have become obsolete.
+		CachedObsolete get(cached_obsolete): map SessionIndex


If I understand correctly, cachedobsolete needs to have the session to prove something, it contains key value needed to rebuild the trie with generate trie for either building a proof (may use current validators also) or verifying a proof.
So for other comments I expect a cached obsolete to be rather big in content.

it should really only ever hold one or zero items, since we buffer at most 1 validator set in the current implementation. it only ever needs to hold something when we've done a validator election

rphmeier · 2019-07-05T14:11:28Z

I do not see a reason with this implementation that will make a future switch to child_trie impossible, it really depend on the size of this trie (if it is a few entry the system here seems fine).

we need trie proofs for child trie, basically. other than that we could migrate it over.

cheme · 2019-07-05T14:18:23Z

Do https://github.com/paritytech/substrate/pull/2209/files#r300701049 reply to this need (before that the proof was using parent + child query, after this pr the proof will be parent proof and child proof in two time)?

rphmeier · 2019-07-05T14:25:05Z

I don't think it helps really. We'd need a method to generate merkle proofs of a child storage within the runtime.

cheme · 2019-07-05T14:57:08Z

Oh yes this is not creating the proof from within the runtime, would need some ext function, certainly more work than it seems indeed. Does not seems doable if we need to generate for current block (uncommited block).

rphmeier · 2019-07-05T15:00:40Z

Does not seems doable if we need to generate for current block (uncommited block).

well, it would have to force a root computation. and we would then kill the child trie right after. but TBH the MemoryDB approach isn't that bad and will scale a little.

andresilva

changes lgtm, but I'm not very familiar with these modules.

andresilva · 2019-07-05T15:14:08Z

srml/session/src/historical.rs

+			let up_to = rstd::cmp::min(up_to, end);
+
+			if up_to < start {
+				return // out of bounds. harmless.


Suggested change

return // out of bounds. harmless.

return; // out of bounds. harmless.

it's fine to omit, no?

yeah nitpick, I just like to have statements terminate with ;

srml/staking/src/lib.rs

srml/session/src/historical.rs

gavofyork · 2019-07-08T08:51:04Z

compile error should go once #2819 is merged.

skeleton for tracking historical sessions

3cfb569

rphmeier added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jun 27, 2019

devops-parity added the A4-gotissues label Jun 27, 2019

rphmeier added 6 commits June 28, 2019 17:51

refactor OpaqueKeys

27add34

some more skeleton work

dfb42dd

adjust session to new OpaqueKeys API

5a9b010

further refactoring of key-type-ids

017bfe0

session gets validator ID parameter

abd25de

run up against compiler

bc30f54

devops-parity added the B2-breaksapi label Jul 1, 2019

rphmeier and others added 13 commits July 1, 2019 18:23

tweak staking to support new session changes

dcfdce4

first run at child storage for deduplication

1deed52

Make session use AccountId as ValidatorId

feb197e

Merge branch 'rh-slash-old' of github.com:paritytech/substrate into r…

2e9ffb8

…h-slash-old

run up against child trie issues

e27c079

switch to using normal trie but with a fixed prefix

427c97e

clear out some println

5d159e6

add dedup test

ee44178

Merge branch 'master' into rh-slash-old

9fdf3b6

flesh out historical module more

de5c568

introduce ExposureOf for staking

6f49495

test the historical module

8d67a7c

WASM compiles

6ce416e

rphmeier 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. A4-gotissues labels Jul 3, 2019

rphmeier requested a review from marcio-diaz July 3, 2019 17:06

rphmeier added 3 commits July 4, 2019 11:53

tests all compile

ea09e61

Merge branch 'master' into rh-slash-old

44a6e16

do some mock change

f07b7a1

rphmeier added 3 commits July 4, 2019 18:31

fix staking tests

4e08e35

Merge branch 'master' into rh-slash-old

3d0f3e3

test obsolecence mechanic

115f019

rphmeier mentioned this pull request Jul 4, 2019

Runtime API for generating proofs-of-inclusion #3027

Open

Merge branch 'rh-slash-old' of github.com:paritytech/substrate into r…

babe162

…h-slash-old

rphmeier mentioned this pull request Jul 5, 2019

BABE: epochs #3032

Closed

bkchr reviewed Jul 5, 2019

View reviewed changes

rphmeier and others added 3 commits July 5, 2019 12:34

Apply suggestions from code review

611bf36

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

some more style nits

c3cceec

a couple more nits

b6cfa58

dvc94ch approved these changes Jul 5, 2019

View reviewed changes

cheme reviewed Jul 5, 2019

View reviewed changes

rphmeier added 2 commits July 5, 2019 16:39

tweak tries

eab4671

Merge branch 'master' into rh-slash-old

4c5682d

andresilva reviewed Jul 5, 2019

View reviewed changes

srml/staking/src/lib.rs Show resolved Hide resolved

niklasad1 reviewed Jul 6, 2019

View reviewed changes

srml/session/src/historical.rs Outdated Show resolved Hide resolved

fix typo thie -> this

c6052c3

gui1117 mentioned this pull request Jul 7, 2019

Staking rate targeting and specific rewards. #2882

Merged

3 tasks

Merge branch 'master' into rh-slash-old

03a2368

rphmeier merged commit 60c54f0 into master Jul 8, 2019

rphmeier deleted the rh-slash-old branch July 8, 2019 12:36

andresilva mentioned this pull request Jul 10, 2019

node: pass stash accounts to session module config #3095

Merged

rphmeier mentioned this pull request Sep 26, 2022

Member Request polkadot-fellows/seeding#4

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slash and prove membership of prior sessions #2970

Slash and prove membership of prior sessions #2970

rphmeier commented Jun 27, 2019 •

edited

Loading

bkchr Jul 5, 2019

bkchr Jul 5, 2019

bkchr Jul 5, 2019

rphmeier Jul 5, 2019

bkchr Jul 5, 2019

dvc94ch left a comment

cheme left a comment

cheme Jul 5, 2019

rphmeier Jul 5, 2019 •

edited

Loading

cheme Jul 5, 2019

rphmeier Jul 5, 2019 •

edited

Loading

cheme Jul 5, 2019

niklasad1 Jul 6, 2019

cheme Jul 5, 2019

rphmeier Jul 5, 2019

cheme Jul 5, 2019

rphmeier Jul 5, 2019

cheme Jul 5, 2019

rphmeier Jul 5, 2019

cheme Jul 5, 2019

cheme Jul 5, 2019

rphmeier Jul 5, 2019

cheme Jul 5, 2019

rphmeier Jul 5, 2019

rphmeier commented Jul 5, 2019

cheme commented Jul 5, 2019 •

edited

Loading

rphmeier commented Jul 5, 2019

cheme commented Jul 5, 2019

rphmeier commented Jul 5, 2019 •

edited

Loading

andresilva left a comment

andresilva Jul 5, 2019

rphmeier Jul 5, 2019

andresilva Jul 5, 2019

gavofyork commented Jul 8, 2019

	pub use hash_db::{HashDB as HashDBT};
	pub use hash_db::HashDB as HashDBT;

	return // out of bounds. harmless.
	return; // out of bounds. harmless.

Slash and prove membership of prior sessions #2970

Slash and prove membership of prior sessions #2970

Conversation

rphmeier commented Jun 27, 2019 • edited Loading

Background

Implementation

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

dvc94ch left a comment

Choose a reason for hiding this comment

cheme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier Jul 5, 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

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 commented Jul 5, 2019

cheme commented Jul 5, 2019 • edited Loading

rphmeier commented Jul 5, 2019

cheme commented Jul 5, 2019

rphmeier commented Jul 5, 2019 • edited Loading

andresilva 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

gavofyork commented Jul 8, 2019

rphmeier commented Jun 27, 2019 •

edited

Loading

rphmeier Jul 5, 2019 •

edited

Loading

rphmeier Jul 5, 2019 •

edited

Loading

cheme commented Jul 5, 2019 •

edited

Loading

rphmeier commented Jul 5, 2019 •

edited

Loading