Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Make substrate generic #169

Merged
merged 63 commits into from
Jun 6, 2018
Merged

Make substrate generic #169

merged 63 commits into from
Jun 6, 2018

Conversation

gavofyork
Copy link
Member

@gavofyork gavofyork commented May 18, 2018

With the eventual aim to remove substrate/primitives/block.rs entirely. This will mean making the bft modules generic since they have substantial dependencies on several of the types inherent in Header. Yet to do:

  • place Hashing type in Header in favour of Hash;
  • get runtime compiling;
  • move substrate_primitives::bft module into substrate_runtime_primitives and make generic over substrate_runtime_primitives::{Block, Header, Hashing} traits;
  • make substrate/bft similarly generic;
  • make substrate/network similarly generic;
  • utilise the new generic types for polkadot code's Block, Header &c;
  • port the polkadot/consensus module to use the new types.

@@ -40,10 +40,11 @@ build_rpc_trait! {
/// Polkadot blockchain API
pub trait ChainApi {
type Metadata;
ASSOCIATED type Header;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why caps?

Copy link
Member Author

@gavofyork gavofyork May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to distinguish from the rust-like grammar. happy to change though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I might not understand how this works. What's really the difference between declaring type Header and type Metadata?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type Metadata; already existed; it's some sort of marker that the macro uses to change the code that's generated.

i had to add the ASSOCIATED tag in order to allow for doc-comments of the type, since otherwise the grammar is ambiguous.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like it should be reversed, that the "weird" associated type carries the non-standard grammar. I'm fine for now as long as we file to patch the JSONRPC crate at some point.

@@ -147,10 +149,86 @@ impl<A: Executable, B: Executable> Executable for (A, B) {
}
}

/// Abstraction around hashing
pub trait Hashing {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more idiomatic to call it Hash since it describes a hash function

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's actually just moving existing code around, so rather out of scope for this pr

}

/// Produce the patricia-trie root of a mapping from indices to byte slices.
fn enumerated_trie_root(items: &[&[u8]]) -> Self::Output;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this and other trie_root functions should be able to go away with upcoming generalization. Although it will use a slightly different Hash trait

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree. one for a later refactoring pr though.

>(input: I) -> Self::Output;

/// Acquire the global storage root.
fn storage_root() -> Self::Output;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that doesn't seem like something that belongs in a hash trait, rather it should be some functionality which is parameterized by it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was already there and being used, so if there's any refactoring to be done, i would prefer to leave it for another pr.

@rphmeier
Copy link
Contributor

icing until #113 is in (some conflicting changes)

#[cfg(feature = "std")]
impl<Block: BlockT> fmt::Display for Id<Block> {
fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
format!("{:?}", self)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be write!(f, "{:?}", self), currently it just creates a string on the heap and drops it.

@dvdplm
Copy link
Contributor

dvdplm commented May 24, 2018

Trying to build this I get a compilation error like so:

error[E0271]: type mismatch resolving `<I as std::iter::IntoIterator>::Item == (std::vec::Vec<u8>, std::vec::Vec<u8>)`
  --> substrate/runtime-io/src/../with_std.rs:96:2
   |
96 |     triehash::trie_root(input).0
   |     ^^^^^^^^^^^^^^^^^^^ expected type parameter, found struct `std::vec::Vec`

I suspect it's because Cargo.lock lists two triehash crates, one from crates.io and the other from parity, both at v0.10. Could it be that parity's triehash should get a version bump and get pushed to crates.io?

@gavofyork
Copy link
Member Author

@dvdplm this is very much inprogress - not expected to build quite yet. here just to show off direction.

@gavofyork gavofyork added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A1-onice labels May 24, 2018
@gavofyork
Copy link
Member Author

that particular issue was fixed in the latest commit, fwiw; triehash crate had a breaking API change committed with only a tiny version increase. should really have been a minor increase.

@dvdplm
Copy link
Contributor

dvdplm commented May 24, 2018

here just to show off direction.

Yeah, came here to look for some inspiration on generics as I struggle a bit with this stuff.

@rphmeier
Copy link
Contributor

triehash crate had a breaking API change committed with only a tiny version increase. should really have been a minor increase.

should be a major change if we follow semver

@gavofyork
Copy link
Member Author

should be a major change if we follow semver

it was pre-1.0, so i think minor bumps for breaking changes are the convention.

@@ -36,7 +37,7 @@ pub struct BlockBuilder<B, E, Block, Hashing> where
executor: E,
state: B::State,
changes: state_machine::OverlayedChanges,
// will probably need PhantomData<Hashing> here...
dummy: PhantomData<Hashing>,
Copy link
Contributor

@rphmeier rphmeier May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The block (or even header) type ought to specify what hash algorithm it's using. Otherwise you need to specify Hashing everywhere you want to do a simple transformation from block header to hash.

@rphmeier
Copy link
Contributor

Posting here so it's not hidden: The block (or even header) type ought to specify what hash algorithm it's using. Otherwise you need to specify Hashing everywhere you want to do a simple transformation from block header to hash.

@gavofyork
Copy link
Member Author

Yup - should indeed be the Header so it's available within the runtime in the appropriate modules.

@rphmeier 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. labels Jun 5, 2018
@rphmeier
Copy link
Contributor

rphmeier commented Jun 5, 2018

Ready for review. Substrate is fully generic at compile-time, but the polkadot-runtime, in order to maintain on-chain upgradeability, has to use runtime polymorphism of the extrinsic type.

polkadot-primitives exports an UncheckedExtrinsic = Vec<u8>, and Block = generic::Block<Header, UncheckedExtrinsic>.

polkadot-runtime has the Call type available, so it exports UncheckedExtrinsic = generic::UncheckedExtrinsic<..., Call> and Block = generic::Block<Header, UncheckedExtrinsic>.

All the generic client code used in polkadot is instantiated with polkadot_primitives::Block as opposed to polkadot_runtime::Block in order to remain forwards-compatible. The only places which use polkadot_runtime::Block are polkadot-consensus (since block authorship requires knowledge of extrinsic types) and polkadot-transaction-pool (until we find a way to generalize meaningfully).

Copy link
Member Author

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be really nice to ensure that test_runtime remained actually trivial rather than forcing the Call model onto it. Fact is that protocols like Blitz and a Z-cash UXTO chain (and many chains that are being prepped as a parachain) won't have a "Call" type - it's pretty specific to Polkadot/Edgware chains.

}

/// Parachain data types.
pub mod parachain {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to inline this rather than leave as a separate file?


/// Provides a type-safe wrapper around a structurally valid block.
#[cfg(feature = "std")]
pub struct CheckedBlock {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could do to be in separate file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the runtime file isn't that big, I don't see any reason to extract yet

let id = self.api.check_id(BlockId::Hash(best_block)).expect("Best block is always valid; qed.");
let id = match self.api.check_id(BlockId::hash(best_block)) {
Ok(id) => id,
Err(_) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curlies are unneeded and, from what I've seen recently, non-idiomatic

match xt.check() {
Ok(xt) => {
let hash = substrate_primitives::hashing::blake2_256(&message);
let hash = BlakeTwo256::hash_of(&message);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this should be BlakeTwo256::hash (hash_of first encodes using Slicable which would add a length prefix).

Downloading {
len: BlockNumber,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not <B::Header as HeaderT>::Number?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be able to convert safely between it and usize.

@@ -47,6 +46,9 @@ pub trait AuxCallable {
type Call: AuxDispatchable + Slicable + Clone + PartialEq + Eq;
}

// dirty hack to work around serde_derive issue.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to know more about this issue - a link & description of what to do when the issue is sorted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rust-lang/rust#51331

see the linked serde issue

@@ -111,17 +110,20 @@ impl Slicable for Action {
}
}

/// Type alias for extracting message type from block.
pub type MessageFor<B> = Message<B, <B as ::traits::Block>::Hash>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "message" mean here? What if the block type doesn't have the notion of "message"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's in the bft module, it's fair to assume that this is a BFT message. The BFT subsystem is something that operates over block types as opposed to being a property of them.

Hash: Member + Slicable,
DigestItem: Member + Slicable,
// Hack to work around the fact that deriving deserialize doesn't work nicely with
// the `hashing` trait used as a parameter.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link to issue + description of how to change once issue sorted would be good.

/// Verify a signature on an encoded value in a lazy manner. This can be
/// an optimization if the signature scheme has an "unsigned" escape hash.
pub fn verify_encoded_lazy<V: Verify, T: codec::Slicable>(sig: &V, item: &T, signer: &V::Signer) -> bool {
// TODO: unfortunately this is a lifetime relationship that can't
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue link + description of how to change once issue is sorted would be good.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we would need rust-lang/rust#44265 and then the Fn* traits would have to be extended to use it somehow. I don't think it's likely to be something we can work around until a few more epochs of Rust. I'll remove the TODO because it's not reasonable to do in the near future.

fn state_root(&self) -> &Self::Hash { &self.state_root }
fn parent_hash(&self) -> &Self::Hash { &self.parent_hash }
fn digest(&self) -> &Self::Digest { &self.digest }
fn new(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did new go?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a little above

@rphmeier rphmeier merged commit 2e26322 into master Jun 6, 2018
@rphmeier rphmeier deleted the gav-make-substrate-generic branch June 6, 2018 15:58
lamafab pushed a commit to lamafab/substrate that referenced this pull request Jun 16, 2020
lamafab pushed a commit to lamafab/substrate that referenced this pull request Jun 16, 2020
* Rebuild runtime

* Remove invalid value from chainspec (paritytech#68)

* service: use grandpa block import for locally sealed aura blocks (paritytech#85)

* bump version to v0.3.1

* Update lock file.

* limit number of transactions when building blocks (paritytech#91)

* Update to latest Substrate

* Bump to 0.3.2

* Actually bump.

* v0.3.2 (paritytech#98)

* bump substrate version

* fix polkadot-collator

* point to alexander-backports of substrate

* bump version

* cli: fix node shutdown (paritytech#100)

* update to latest substrate, change to v0.3.4

* update to latest substrate, bump version to 0.3.5

* v0.3.6

* try to build on every v0.3 commit and update alexander-backports

* bump to v0.3.7

* bump to 0.3.8

* Bump to 0.3.9: network and pruning improvements

* Bump to 0.3.10: reduce network bandwidth usage

* Use libp2p-kad 0.3.2 (paritytech#122)

* Bump libp2p-identify to 0.3.1 (paritytech#123)

* Bump to 0.3.12 (paritytech#127)

* Update Substrate again (paritytech#128)

* update substrate and bump version to v0.3.13

* bump version to v0.3.14: fix --reserved-nodes

* add a manually curated grandpa module (paritytech#136)

* updating v0.3 to use substrate v0.10 (paritytech#146)

* updating to latest substrate v0.10

* better handling of outer poll

* nit

* fix tests

* remove comment

* reduce indentation

* use self.poll

* bring oneshot into scope

* spaces

* wrap

* remove match

* wrap

* Update primitives/Cargo.toml

Co-Authored-By: gterzian <[email protected]>

* Update runtime/wasm/Cargo.toml

Co-Authored-By: gterzian <[email protected]>

* Update runtime/wasm/Cargo.toml

Co-Authored-By: gterzian <[email protected]>

* Update test-parachains/adder/collator/src/main.rs

Co-Authored-By: gterzian <[email protected]>

* indent

* add paranthese

* config: fix wrong ip for alexander bootnode (paritytech#161)

* fix curated-grandpa and rebuild wasm (paritytech#162)

* [v0.3] Integrates new gossip system into Polkadot (paritytech#166)

* new gossip validation in network

* integrate new gossip into service

* network: guard validation network future under exit signal (paritytech#168)

* bump version to v0.3.15: substrate v0.10

* [v0.3] update to substrate master (paritytech#175)

* update to substrate master

* fix test

* service: fix telemetry endpoints on alexander chainspec (paritytech#169) (paritytech#178)

* Update v0.3 to latest Substrate master (paritytech#177)

* update substrate v0.3 to latest master

* bump spec version

* update to latest master: remove fees module

* update runtime blobs

* bump version to 0.3.16

* replace sr25519 accountid with anysigner

* bump version to v0.3.17

* Some PoC-3 GRANDPA tweaks (paritytech#181)

* call on_finalise after triggering curated_grandpa change

* make grandpa rounds shorter for faster finalization

* use authorities when calculating duty roster (paritytech#185)

* [v0.3] Update to substrate master (paritytech#183)

* update to latest substrate master

* bump version to 0.3.18

* update to latest substrate master

* bump spec version

* update runtime wasm blobs

* remove current_offline_slash from chain spec

* update to substrate master: bump version to v0.3.19 (paritytech#188)

* update to substrate master: bump version to v0.3.19

libp2p network improvements

* network: replace NodeIndex with PeerId

* network: fix tests

* polkadot v0.3.20 (paritytech#190)

* update to substrate master: bump version to 0.3.20

* runtime: add offchain worker trait

* runtime: rebuild wasm blobs

* bump spec version (paritytech#191)

* Fix compilation

* Update version to 0.4.0

* Switch to use `polkadot-master` branch from substrate

* Remove unused struct

* Remove `grandpa::SyncedAuthorities` from `OnSessionChange`
JoshOrndorff added a commit to moonbeam-foundation/substrate that referenced this pull request Apr 21, 2021
* Initial sketch

* Override relay spec for well-known para specs

* Update existing spec generation scripts

* Update specs/README.md

Co-authored-by: Amar Singh <[email protected]>

* Prettier

* note

* Better naming

* cargo fmt

But actually, I don't like converting to and from String like this 
anyway.

* Clean tests readme

* Newlines at end of spec files

* line length

* cleaner way to associate a relay spec.

* Newer spec files. These are mocks

* oops import

* cargo fmt

* prettier spec

* Fix type

Basti confirmed I "should" use the rococo one here. He also mentioned it 
didn't really matter when using a raw spec.

* use specs for alphanet blue

* remove spec publishing from CI

* Fixes staking parameter to include 18 decimals

* fix previous formatting issue

* Forces ts target to 2020 for tests

Co-authored-by: Amar Singh <[email protected]>
Co-authored-by: crystalin <[email protected]>
Co-authored-by: Crystalin <[email protected]>
liuchengxu pushed a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021
liuchengxu added a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021
* Split out xp-runtime

* Commit xp-runtime

* Move xss_check() to xp-runtime
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants