From ebb688f81fd201fe6d1a2ffe743cfc3632b261a0 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Thu, 20 Sep 2018 23:18:47 +0200 Subject: [PATCH] Address grumbles in eras PR (#782) * Address grumbles * Fix hash --- substrate/core/client/src/client.rs | 6 ++--- .../core/sr-primitives/src/generic/era.rs | 8 ++++++- .../src/generic/unchecked_mortal_extrinsic.rs | 24 +++++++++---------- substrate/core/sr-primitives/src/traits.rs | 4 ++-- substrate/node/api/src/lib.rs | 4 ++-- substrate/node/consensus/src/lib.rs | 2 +- substrate/node/executor/src/lib.rs | 7 +++--- substrate/node/transaction-pool/src/lib.rs | 10 ++++---- substrate/srml/balances/src/lib.rs | 6 ++--- substrate/srml/system/src/lib.rs | 6 ++--- 10 files changed, 42 insertions(+), 35 deletions(-) diff --git a/substrate/core/client/src/client.rs b/substrate/core/client/src/client.rs index 350d51fead..ea6ee7a0da 100644 --- a/substrate/core/client/src/client.rs +++ b/substrate/core/client/src/client.rs @@ -21,7 +21,7 @@ use futures::sync::mpsc; use parking_lot::{Mutex, RwLock}; use primitives::AuthorityId; use runtime_primitives::{bft::Justification, generic::{BlockId, SignedBlock, Block as RuntimeBlock}}; -use runtime_primitives::traits::{Block as BlockT, Header as HeaderT, Zero, One, As, NumberFor, GetHeight, BlockNumberToHash}; +use runtime_primitives::traits::{Block as BlockT, Header as HeaderT, Zero, One, As, NumberFor, CurrentHeight, BlockNumberToHash}; use runtime_primitives::BuildStorage; use primitives::{Blake2Hasher, RlpCodec, H256}; use primitives::storage::{StorageKey, StorageData}; @@ -568,13 +568,13 @@ impl Client where } } -impl GetHeight for Client where +impl CurrentHeight for Client where B: backend::Backend, E: CallExecutor + Clone, Block: BlockT, { type BlockNumber = ::Number; - fn get_height(&self) -> Self::BlockNumber { + fn current_height(&self) -> Self::BlockNumber { self.backend.blockchain().info().map(|i| i.best_number).unwrap_or_else(|_| Zero::zero()) } } diff --git a/substrate/core/sr-primitives/src/generic/era.rs b/substrate/core/sr-primitives/src/generic/era.rs index afc4eba13d..9a9d032290 100644 --- a/substrate/core/sr-primitives/src/generic/era.rs +++ b/substrate/core/sr-primitives/src/generic/era.rs @@ -79,7 +79,7 @@ impl Era { pub fn birth(self, current: u64) -> u64 { match self { Era::Immortal => 0, - Era::Mortal(period, phase) => (current - phase) / period * period + phase, + Era::Mortal(period, phase) => (current.max(phase) - phase) / period * period + phase, } } @@ -189,4 +189,10 @@ mod tests { assert_ne!(e.birth(10), 6); assert_ne!(e.birth(5), 6); } + + #[test] + fn current_less_than_phase() { + // should not panic + Era::mortal(4, 3).birth(1); + } } \ No newline at end of file diff --git a/substrate/core/sr-primitives/src/generic/unchecked_mortal_extrinsic.rs b/substrate/core/sr-primitives/src/generic/unchecked_mortal_extrinsic.rs index d2565f5f35..c494bc00ff 100644 --- a/substrate/core/sr-primitives/src/generic/unchecked_mortal_extrinsic.rs +++ b/substrate/core/sr-primitives/src/generic/unchecked_mortal_extrinsic.rs @@ -21,7 +21,7 @@ use std::fmt; use rstd::prelude::*; use codec::{Decode, Encode, Input}; -use traits::{self, Member, SimpleArithmetic, MaybeDisplay, GetHeight, BlockNumberToHash, Lookup, +use traits::{self, Member, SimpleArithmetic, MaybeDisplay, CurrentHeight, BlockNumberToHash, Lookup, Checkable}; use super::{CheckedExtrinsic, Era}; @@ -74,7 +74,7 @@ where BlockNumber: SimpleArithmetic, Hash: Encode, Context: Lookup - + GetHeight + + CurrentHeight + BlockNumberToHash, { type Checked = CheckedExtrinsic; @@ -82,9 +82,9 @@ where fn check(self, context: &Context) -> Result { Ok(match self.signature { Some((signed, signature, index, era)) => { - let h = context.block_number_to_hash(BlockNumber::sa(era.birth(context.get_height().as_()))) + let h = context.block_number_to_hash(BlockNumber::sa(era.birth(context.current_height().as_()))) .ok_or("transaction birth block ancient")?; - let payload = (index, self.function, h); + let payload = (index, self.function, era, h); let signed = context.lookup(signed)?; if !::verify_encoded_lazy(&signature, &payload, &signed) { return Err("bad signature in extrinsic") @@ -188,9 +188,9 @@ mod tests { type Target = u64; fn lookup(&self, s: u64) -> Result { Ok(s) } } - impl GetHeight for TestContext { + impl CurrentHeight for TestContext { type BlockNumber = u64; - fn get_height(&self) -> u64 { 42 } + fn current_height(&self) -> u64 { 42 } } impl BlockNumberToHash for TestContext { type BlockNumber = u64; @@ -222,7 +222,7 @@ mod tests { #[test] fn signed_codec_should_work() { - let ux = Ex::new_signed(0, DUMMY_FUNCTION, DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, (DUMMY_ACCOUNTID, DUMMY_FUNCTION, 0u64).encode()), Era::immortal()); + let ux = Ex::new_signed(0, DUMMY_FUNCTION, DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, (DUMMY_ACCOUNTID, DUMMY_FUNCTION, Era::immortal(), 0u64).encode()), Era::immortal()); let encoded = ux.encode(); assert_eq!(Ex::decode(&mut &encoded[..]), Some(ux)); } @@ -243,35 +243,35 @@ mod tests { #[test] fn immortal_signed_check_should_work() { - let ux = Ex::new_signed(0, DUMMY_FUNCTION, DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, (DUMMY_ACCOUNTID, DUMMY_FUNCTION, 0u64).encode()), Era::immortal()); + let ux = Ex::new_signed(0, DUMMY_FUNCTION, DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, (DUMMY_ACCOUNTID, DUMMY_FUNCTION, Era::immortal(), 0u64).encode()), Era::immortal()); assert!(ux.is_signed()); assert_eq!(>::check(ux, &TestContext), Ok(CEx { signed: Some((DUMMY_ACCOUNTID, 0)), function: DUMMY_FUNCTION })); } #[test] fn mortal_signed_check_should_work() { - let ux = Ex::new_signed(0, DUMMY_FUNCTION, DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, (DUMMY_ACCOUNTID, DUMMY_FUNCTION, 42u64).encode()), Era::mortal(32, 42)); + let ux = Ex::new_signed(0, DUMMY_FUNCTION, DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, (DUMMY_ACCOUNTID, DUMMY_FUNCTION, Era::mortal(32, 42), 42u64).encode()), Era::mortal(32, 42)); assert!(ux.is_signed()); assert_eq!(>::check(ux, &TestContext), Ok(CEx { signed: Some((DUMMY_ACCOUNTID, 0)), function: DUMMY_FUNCTION })); } #[test] fn later_mortal_signed_check_should_work() { - let ux = Ex::new_signed(0, DUMMY_FUNCTION, DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, (DUMMY_ACCOUNTID, DUMMY_FUNCTION, 11u64).encode()), Era::mortal(32, 11)); + let ux = Ex::new_signed(0, DUMMY_FUNCTION, DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, (DUMMY_ACCOUNTID, DUMMY_FUNCTION, Era::mortal(32, 11), 11u64).encode()), Era::mortal(32, 11)); assert!(ux.is_signed()); assert_eq!(>::check(ux, &TestContext), Ok(CEx { signed: Some((DUMMY_ACCOUNTID, 0)), function: DUMMY_FUNCTION })); } #[test] fn too_late_mortal_signed_check_should_fail() { - let ux = Ex::new_signed(0, DUMMY_FUNCTION, DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, (DUMMY_ACCOUNTID, DUMMY_FUNCTION, 10u64).encode()), Era::mortal(32, 10)); + let ux = Ex::new_signed(0, DUMMY_FUNCTION, DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, (DUMMY_ACCOUNTID, DUMMY_FUNCTION, Era::mortal(32, 10), 10u64).encode()), Era::mortal(32, 10)); assert!(ux.is_signed()); assert_eq!(>::check(ux, &TestContext), Err("bad signature in extrinsic")); } #[test] fn too_early_mortal_signed_check_should_fail() { - let ux = Ex::new_signed(0, DUMMY_FUNCTION, DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, (DUMMY_ACCOUNTID, DUMMY_FUNCTION, 43u64).encode()), Era::mortal(32, 43)); + let ux = Ex::new_signed(0, DUMMY_FUNCTION, DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, (DUMMY_ACCOUNTID, DUMMY_FUNCTION, Era::mortal(32, 43), 43u64).encode()), Era::mortal(32, 43)); assert!(ux.is_signed()); assert_eq!(>::check(ux, &TestContext), Err("bad signature in extrinsic")); } diff --git a/substrate/core/sr-primitives/src/traits.rs b/substrate/core/sr-primitives/src/traits.rs index c7453e02bf..ad82cb8051 100644 --- a/substrate/core/sr-primitives/src/traits.rs +++ b/substrate/core/sr-primitives/src/traits.rs @@ -64,12 +64,12 @@ pub trait Lookup { } /// Get the "current" block number. -pub trait GetHeight { +pub trait CurrentHeight { /// The type of the block number. type BlockNumber; /// Return the current block number. Not allowed to fail. - fn get_height(&self) -> Self::BlockNumber; + fn current_height(&self) -> Self::BlockNumber; } /// Translate a block number into a hash. diff --git a/substrate/node/api/src/lib.rs b/substrate/node/api/src/lib.rs index 01945537eb..822de492ef 100644 --- a/substrate/node/api/src/lib.rs +++ b/substrate/node/api/src/lib.rs @@ -33,7 +33,7 @@ use client::{Client, CallExecutor}; use primitives::{ AccountId, Block, BlockId, BlockNumber, Hash, Index, InherentData, SessionKey, Timestamp, UncheckedExtrinsic }; -use sr_primitives::{transaction_validity::TransactionValidity, traits::{GetHeight, BlockNumberToHash}}; +use sr_primitives::{transaction_validity::TransactionValidity, traits::{CurrentHeight, BlockNumberToHash}}; use substrate_primitives::{Blake2Hasher, RlpCodec}; /// Build new blocks. @@ -48,7 +48,7 @@ pub trait BlockBuilder { /// Trait encapsulating the node API. /// /// All calls should fail when the exact runtime is unknown. -pub trait Api: GetHeight + BlockNumberToHash { +pub trait Api: CurrentHeight + BlockNumberToHash { /// The block builder for this API type. type BlockBuilder: BlockBuilder; diff --git a/substrate/node/consensus/src/lib.rs b/substrate/node/consensus/src/lib.rs index ecb57943ee..dcb6e0441f 100644 --- a/substrate/node/consensus/src/lib.rs +++ b/substrate/node/consensus/src/lib.rs @@ -401,7 +401,7 @@ impl bft::Proposer for Proposer => MisbehaviorKind::BftDoubleCommit(round as u32, (h1, s1.signature), (h2, s2.signature)), } }; - let payload = (next_index, Call::Consensus(ConsensusCall::report_misbehavior(report)), self.client.genesis_hash()); + let payload = (next_index, Call::Consensus(ConsensusCall::report_misbehavior(report)), Era::immortal(), self.client.genesis_hash()); let signature = self.local_key.sign(&payload.encode()).into(); next_index += 1; diff --git a/substrate/node/executor/src/lib.rs b/substrate/node/executor/src/lib.rs index aa43e4e65e..5acd4da135 100644 --- a/substrate/node/executor/src/lib.rs +++ b/substrate/node/executor/src/lib.rs @@ -83,11 +83,12 @@ mod tests { fn sign(xt: CheckedExtrinsic) -> UncheckedExtrinsic { match xt.signed { Some((signed, index)) => { - let payload = (index, xt.function, GENESIS_HASH); + let era = Era::mortal(256, 0); + let payload = (index, xt.function, era, GENESIS_HASH); let pair = Pair::from(Keyring::from_public(Public::from_raw(signed.clone().into())).unwrap()); let signature = pair.sign(&payload.encode()).into(); UncheckedExtrinsic { - signature: Some((balances::address::Address::Id(signed), signature, payload.0, Era::mortal(256, 0))), + signature: Some((balances::address::Address::Id(signed), signature, payload.0, era)), function: payload.1, } } @@ -309,7 +310,7 @@ mod tests { construct_block( 2, block1(false).1, - hex!("60efe1a65e7c79041b02e56ec122d6eaedfa476e0a9f6f1f68eb0c8f402c4514").into(), + hex!("0acf8b3c169ce8f16faf5610f646f371681dcc3b544d3dd05036dbae7890e399").into(), None, vec![ CheckedExtrinsic { diff --git a/substrate/node/transaction-pool/src/lib.rs b/substrate/node/transaction-pool/src/lib.rs index 5d9db24f45..1eb2a5aa80 100644 --- a/substrate/node/transaction-pool/src/lib.rs +++ b/substrate/node/transaction-pool/src/lib.rs @@ -46,7 +46,7 @@ use transaction_pool::{Readiness, scoring::{Change, Choice}, VerifiedFor, Extrin use node_api::Api; use primitives::{AccountId, BlockId, Block, Hash, Index, BlockNumber}; use runtime::{Address, UncheckedExtrinsic}; -use sr_primitives::traits::{Bounded, Checkable, Hash as HashT, BlakeTwo256, Lookup, GetHeight, BlockNumberToHash}; +use sr_primitives::traits::{Bounded, Checkable, Hash as HashT, BlakeTwo256, Lookup, CurrentHeight, BlockNumberToHash}; pub use transaction_pool::{Options, Status, LightStatus, VerifiedTransaction as VerifiedTransactionOps}; pub use error::{Error, ErrorKind, Result}; @@ -123,10 +123,10 @@ impl ChainApi where /// /// This is due for removal when #721 lands pub struct LocalContext<'a, A: 'a>(&'a Arc); -impl<'a, A: 'a + Api> GetHeight for LocalContext<'a, A> { +impl<'a, A: 'a + Api> CurrentHeight for LocalContext<'a, A> { type BlockNumber = BlockNumber; - fn get_height(&self) -> BlockNumber { - self.0.get_height() + fn current_height(&self) -> BlockNumber { + self.0.current_height() } } impl<'a, A: 'a + Api> BlockNumberToHash for LocalContext<'a, A> { @@ -140,7 +140,7 @@ impl<'a, A: 'a + Api> Lookup for LocalContext<'a, A> { type Source = Address; type Target = AccountId; fn lookup(&self, a: Address) -> ::std::result::Result { - self.0.lookup(&BlockId::number(self.get_height()), a).unwrap_or(None).ok_or("error with lookup") + self.0.lookup(&BlockId::number(self.current_height()), a).unwrap_or(None).ok_or("error with lookup") } } diff --git a/substrate/srml/balances/src/lib.rs b/substrate/srml/balances/src/lib.rs index b26904e0fb..3cd0aa94e4 100644 --- a/substrate/srml/balances/src/lib.rs +++ b/substrate/srml/balances/src/lib.rs @@ -46,7 +46,7 @@ use codec::{Encode, Decode, Codec, Input, Output}; use runtime_support::{StorageValue, StorageMap, Parameter}; use runtime_support::dispatch::Result; use primitives::traits::{Zero, One, SimpleArithmetic, OnFinalise, MakePayment, - As, Lookup, Member, CheckedAdd, CheckedSub, GetHeight, BlockNumberToHash}; + As, Lookup, Member, CheckedAdd, CheckedSub, CurrentHeight, BlockNumberToHash}; use address::Address as RawAddress; use system::ensure_signed; @@ -659,9 +659,9 @@ impl Lookup for ChainContext { } } -impl GetHeight for ChainContext { +impl CurrentHeight for ChainContext { type BlockNumber = T::BlockNumber; - fn get_height(&self) -> Self::BlockNumber { + fn current_height(&self) -> Self::BlockNumber { >::block_number() } } diff --git a/substrate/srml/system/src/lib.rs b/substrate/srml/system/src/lib.rs index 136bc3ec60..d23502deb5 100644 --- a/substrate/srml/system/src/lib.rs +++ b/substrate/srml/system/src/lib.rs @@ -44,7 +44,7 @@ extern crate safe_mix; use rstd::prelude::*; use primitives::traits::{self, CheckEqual, SimpleArithmetic, SimpleBitOps, Zero, One, Bounded, - Hash, Member, MaybeDisplay, EnsureOrigin, Digest as DigestT, As, GetHeight, BlockNumberToHash}; + Hash, Member, MaybeDisplay, EnsureOrigin, Digest as DigestT, As, CurrentHeight, BlockNumberToHash}; use substrate_primitives::storage::well_known_keys; use runtime_support::{storage, StorageValue, StorageMap, Parameter}; use safe_mix::TripletMix; @@ -384,9 +384,9 @@ impl Module { } } -impl GetHeight for Module { +impl CurrentHeight for Module { type BlockNumber = T::BlockNumber; - fn get_height(&self) -> Self::BlockNumber { + fn current_height(&self) -> Self::BlockNumber { >::block_number() } }