Conversation
marcio-diaz
added
the
A3-in_progress
marcio-diaz
force-pushed
the
marcio/simple-misbehaviour-reports
branch
from
4a15d16 to
a406442
Compare
marcio-diaz
force-pushed
the
marcio/simple-misbehaviour-reports
branch
from
a406442 to
5c2a007
Compare
marcio-diaz
added
A0-please_review
Demi-Marie
left a comment
Demi-Marie
left a comment
There was a problem hiding this comment.
18446744073709551615 needs to be replaced with u64::MAX everywhere. Also, FIXMEs and issues need to be written for where slashing has not yet been implemented.
| @@ -19,21 +19,19 @@ | |||
| //! This implements the digests for AuRa, to allow the private | |||
There was a problem hiding this comment.
Interesting. I almost made the same change for BABE in my on-chain randomness work, but reverted it.
| fn construct_equivocation_report_call( | ||
| _proof: AuraEquivocationProof<<Block as BlockT>::Header,sr25519::Signature> | ||
| ) -> Vec<u8> { | ||
| vec![] |
There was a problem hiding this comment.
I was wondering why this was empty, then realized that this is the test runtime.
| fn construct_equivocation_report_call( | ||
| _proof: AuraEquivocationProof<<Block as BlockT>::Header,ed25519::Signature> | ||
| ) -> Vec<u8> { | ||
| vec![] |
There was a problem hiding this comment.
This, however, should not be empty. It seems to me that a proof is definitely required. Even if it is not, there should be documentation explaining why the empty vector is a valid choice.
| _origin, | ||
| _equivocation_proof: AuraEquivocationProof<T::Header, T::Signature> | ||
| ) { | ||
| // This is the place where we will slash. |
There was a problem hiding this comment.
This needs an issue filed for it, along with a FIXME and the issue number in the code.
| priority: 10, | ||
| requires: vec![], | ||
| provides: vec![], | ||
| longevity: 18446744073709551615, |
There was a problem hiding this comment.
| longevity: 18446744073709551615, | |
| longevity: std::u64::MAX, |
| priority: 10, | ||
| requires: vec![], | ||
| provides: vec![], | ||
| longevity: 18446744073709551615, |
There was a problem hiding this comment.
| longevity: 18446744073709551615, | |
| longevity: u64::MAX, |
| priority: 10, | ||
| requires: vec![], | ||
| provides: vec![], | ||
| longevity: 18446744073709551615, |
There was a problem hiding this comment.
| longevity: 18446744073709551615, | |
| longevity: u64::MAX, |
| priority: 10, | ||
| requires: vec![], | ||
| provides: vec![], | ||
| longevity: 18446744073709551615, |
There was a problem hiding this comment.
| longevity: 18446744073709551615, | |
| longevity: u64::MAX, |
| priority: 10, | ||
| requires: vec![], | ||
| provides: vec![], | ||
| longevity: 18446744073709551615, |
There was a problem hiding this comment.
| longevity: 18446744073709551615, | |
| longevity: u64::MAX, |
| priority: 10, | ||
| requires: vec![], | ||
| provides: vec![], | ||
| longevity: 18446744073709551615, |
There was a problem hiding this comment.
| longevity: 18446744073709551615, | |
| longevity: u64::MAX, |
rphmeier
added
A5-grumble
and removed
A0-please_review
| } | ||
|
|
||
| #[derive(Debug, Clone, Encode, Decode, PartialEq)] | ||
| pub struct GrandpaEquivocationProof<E> { |
There was a problem hiding this comment.
docs on public items (including their public fields)
| pub equivocation: E, | ||
| } | ||
|
|
||
| pub fn localized_payload<E: Encode>(round: u64, set_id: u64, message: &E) -> Vec<u8> { |
There was a problem hiding this comment.
Docs on public items.
Also, if we're going to use this elsewhere, we should consider returning an impl Encode instead of a Vec<u8>. Then you can do e.g.
let payload = localized_payload(...);
let my_packet = (some_other_data, payload).encode();
| /// Get slot author for given block along with authorities. | ||
| pub fn slot_author<AuthorityId>(slot_num: u64, authorities: &[AuthorityId]) -> Option<&AuthorityId> | ||
| { | ||
| if authorities.is_empty() { return None } |
There was a problem hiding this comment.
I don't understand why you need these extra checks when the function returns an Option and get does it for you.
why not just
pub fn slot_author<AuthorityId>(slot_num: u64, authorities: &[AuthorityId]) -> Option<&AuthorityId> {
authorities.get(slot_num as usize)
}
| { | ||
| if authorities.is_empty() { return None } | ||
|
|
||
| let idx = slot_num % (authorities.len() as u64); |
There was a problem hiding this comment.
why wrap-around slot number here to fit in the authorities slice?
|
Is this good to be reviewed? Could you merge/rebase master if that's the case? :) edit: Actually I think a rebase would be the best. |
| } | ||
|
|
||
| #[derive(Debug, Encode, Decode, PartialEq, Eq, Clone)] | ||
| pub struct AuraEquivocationProof<H, S> { |
| let slot = maybe_slot1.expect("OK by previous line; qed"); | ||
|
|
||
| // FIX: What if there is a different set of authorities? | ||
| let authorities = <Module<T>>::authorities(); |
There was a problem hiding this comment.
right, this is the hardest part of slashing. we probably will need the historical session trie to also store some key mapped to Vec<T::Keys>. Consensus engines that need access to the entire authorities list will have to accept a proof of that branch as well.
|
@marcio-diaz Can you rebase this? |
|
Closing in favor of #3225 (between other follow ups) |
7 participants
Replaces #2800, fixing conflicts, adding tests and improving code.
TODO: Test that equivocations really do lead to reports in GRANDPA (no idea how to check it in a meaningful way since equivocations come from grandpa repo), Babe stuff (maybe follow up).