Use parity-common to make substrate generic over hasher and trie encoding codec #297

dvdplm · 2018-07-11T13:23:38Z

As part of the effort to extract and use common code between parity-ethereum and polkadot, this PR introduces the crates in parity-common which in turn means switch to using generics for the hash type and node encoder and add concrete impls (BlakeHasher and BlakeRlpCodec).
Part of the code is completely generic, e.g. state-machine, whereas other parts use BlakeHasher and BlakeRlpCodec to build concrete types. The exact layering of which parts are generic and which aren't is not very well thought out and feedback on how to improve that is most welcome.

gnunicorn

Minor changes and questions. Aside from that LGTM.

gnunicorn · 2018-07-12T08:19:38Z

substrate/state-machine/src/backend.rs

 	inner: Arc<HashMap<Vec<u8>, Vec<u8>>>,
+	_hasher: PhantomData<H>,


Curious: why did you opt for an underscore here?

No real good reason. I keep going back and forth on how to name my phantoms, so I end up being inconsistent. Sometimes I like marker, sometimes I name it for what it contains, sometimes with a _ prefix to signal to the reader "never mind this, it's there to make the code compile and is not relevant".

I'd like to have a rule to follow here. What do you use?

Clippy states that underscore-prefixed names are expected to be in there for compile-passing-purposes only and aren't actually used. Which is the case here, and combined with a proper descriptive name, I think this is totally the appropriate way to do it, @dvdplm :) .

"compile-passing-purposes" – rofl

gnunicorn · 2018-07-12T08:25:14Z

substrate/state-machine/src/ext.rs

@@ -123,7 +136,7 @@ impl<'a, B: 'a> Externalities for Ext<'a, B>
 		42
 	}

-	fn storage_root(&mut self) -> [u8; 32] {
+	fn storage_root(&mut self) -> H::Out where H::Out: Ord + Encodable {


I know this line isn't super long, however I think it reduces readability complexity if we had the where in the second line

gnunicorn · 2018-07-12T08:32:30Z

substrate/state-machine/src/testing.rs

+}
+
+impl<H: Hasher> TestExternalities<H> {
+	#[cfg(test)]


is this #[cfg] necessary?

I think it's wrong. TestExternalities is exported (and re-exported again in runtime-io) so I think it needs to be pub. Good catch.

…and this change will break things in other crates. I don't think this PR can be merged as-is.

gnunicorn · 2018-07-12T08:36:13Z

substrate/state-machine/src/proving_backend.rs

 			.get_with(key, &mut *proof_recorder).map(|x| x.map(|val| val.to_vec())).map_err(map_e)
 	}

+	// TODO: "error[E0283]: type annotations required: cannot resolve `_: patricia_trie::NodeCodec<H>`"/"note: required because of the requirements on the impl of `backend::Backend<H, _>` for `trie_backend::TrieBackend<H>`")


Also comments shouldn't be longer than 100chars, please wrap :) .

gnunicorn · 2018-07-12T08:43:13Z

substrate/keystore/Cargo.toml

@@ -4,7 +4,7 @@ version = "0.1.0"
 authors = ["Parity Technologies <[email protected]>"]

 [dependencies]
-ethcore-crypto = { git = "https://github.com/paritytech/parity.git", default_features = false }
+parity-crypto = { git = "https://github.com/paritytech/parity-common.git", default_features = false }


You missed renaming an import, see travis test.

fixed, thanks!

…bstrate-state-machine-generic * refactor/environmental-generic-traits: whitespace Teach environmental! about generics Validator side of the collation protocol. (#295) Improve Wasm extern errors and increase heap (#299) Issue 212 - refactor Checkable trait to be more generic (#287) Fix for nightly 2018-07-10 (#296) PoC-2 tweaks (#293)

arkpar · 2018-07-13T14:56:05Z

Closes #292

* master: README: fixed typo in docker run command (#518) Merge *_at methods. (#515) New flags to listen to all interfaces (#495) If contract reaches max depth, return Err (#503) Some networking cleanups (#504) Derivable Encode & Decode (#509) substrate: return Option in all storage related RPC methods (#510) Build with locked Cargo.lock on CI (#514) Place call data into a newly allocated pages (#502)

svyatonik · 2018-08-09T12:23:34Z

Haven't followed this PR from the beginning, but I wonder - why there are still direct requirements for BlakeHasher/BlakeRlpCodec in substrate (like this one). I thought that the point of the PR is to avoid requirements for concrete implementations in substrate. Probably I'm missing something, but could you please elaborate?

gavofyork · 2018-08-09T13:33:04Z

substrate/client/src/block_builder.rs

 	Block: BlockT,
+	H: Hasher,


reason for not integrating H and C into BlockT?

Not sure I understand what you mean here. You mean something like this?

pub trait Block: Clone + Send + Sync + Codec + Eq + MaybeSerializeDebug + 'static { type Extrinsic: Member + Codec; type Header: Header<Hash=Self::Hash>; type Hash: Member + ::rstd::hash::Hash + Copy + MaybeDisplay + Default + SimpleBitOps + Codec + AsRef<[u8]>; type Hasher: Hasher<Out=<Self::Hash>::Output>; type NodeCodec: NodeCodec<Self::Hasher>; …

There is some significant overlap between the hashdb::Hasher and substrate own Hash trait and when talking this over with @rphmeier his idea was to get this in first and refine/unify further down the line. Not sure if Block is the proper unification point but might very well be?

gavofyork · 2018-08-09T13:36:41Z

substrate/primitives/src/rlp_codec.rs

+	fn is_empty_node(data: &[u8]) -> bool {
+		Rlp::new(data).is_empty()
+	}
+    fn empty_node() -> ElasticArray1024<u8> {


get them spaces out of here!!!!

gavofyork · 2018-08-09T13:36:59Z

substrate/primitives/src/rlp_codec.rs

+
+	fn branch_node<I>(children: I, value: Option<ElasticArray128<u8>>) -> ElasticArray1024<u8>
+	where I: IntoIterator<Item=Option<ChildReference<<BlakeHasher as Hasher>::Out>>>
+    {


gah the indenting is borked. spaces!

gavofyork · 2018-08-09T13:43:15Z

two concerns:

same as @svyatonik - why do I see BlakeHasher so frequently in substrate - isn't the point to keep it generic? it looks very much like most of the runtime and much of the backend can only be used with BlakeHasher, drastically reducing this PR's benefit.
there's a lot of extra code noise with these changes. i'm thinking of (quite numerous) instances of:

- ) -> Result<(Vec<u8>, B::Transaction), Box<Error>> {
+ ) -> Result<(Vec<u8>, B::Transaction), Box<Error>>
+ where
+ 	H: Hasher,
+ 	C: NodeCodec<H>,
+ 	Exec: CodeExecutor<H>,
+ 	B: Backend<H, C>,
+ 	H::Out: Ord + Encodable,
+ 	Handler: FnOnce(Result<Vec<u8>, Exec::Error>, Result<Vec<u8>, Exec::Error>) -> Result<Vec<u8>, Exec::Error>
+ {

this makes previously quite readable and concise code bloated and boilerplatey.

dvdplm · 2018-08-09T14:48:02Z

I thought that the point of the PR is to avoid requirements for concrete implementations in substrate.

Up to a point yes, the goal is to make things generic but the main motivating force was to share code with parity-ethereum. There is something of a "hard limit" as to how generic substrate can be, though. I found it quite tricky to make things generic beyond this here:

environmental!(ext: trait Externalities<BlakeHasher>);

That line can not not use concrete types, and this somewhat limits the extent to which the rest can be generic. I am sure I've missed places that could be generic and instead went for concrete types (do holler if you spot some!).

The result is "a lot of BlakeHasher" all over. I think this can be alleviated in many places with some convenience types but I'd like to address that in a separate PR. I think it is easier to get good review feedback with all the nastiness laid bare and then sweep stuff under the rug in a a phase 2. If you disagree I'm happy to do it right away.

…:paritytech/polkadot into refactor/substrate-state-machine-generic * 'refactor/substrate-state-machine-generic' of github.com:paritytech/polkadot: Update Cargo.lock Remove safe-mix (#521)

gavofyork · 2018-08-09T15:36:55Z

ok - make an issue for it and i guess we can revisit.

…paritytech#297) * Make `polkadot-parachain` call `validate_block` instead of `validate` Also switch to rust 2018 in the crate * Use `rstd` * Make `load_params` a pointer

* from 8 to 25 * bump spec version * Update runtime/src/lib.rs

dvdplm added 8 commits July 11, 2018 11:53

Genric over hasher

45e0ee8

WIP start adding NodeCodec

eb26a69

Add codec to TrieBackend

acf8284

Typechecks

90707dc

Fix error type

4ccc851

Cleanup

21b7892

Tests build (and fail)

6b2fa02

Fix tests: don't use MemoryDB::default()

f331b0c

dvdplm changed the title ~~Make generic over Hasher/NodeCodec~~ Make substrate/state-machine generic over Hasher/NodeCodec Jul 11, 2018

dvdplm added A3-in_progress Pull request is in progress. No review needed at this stage. I7-refactor Code needs refactoring. M4-core 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 Jul 11, 2018

Lockfile

f0f2413

gnunicorn previously approved these changes Jul 12, 2018

View reviewed changes

gnunicorn reviewed Jul 12, 2018

View reviewed changes

dvdplm added 2 commits July 12, 2018 13:57

Address grumbles

87740d7

Teach environmental! about generics

a34f614

dvdplm mentioned this pull request Jul 12, 2018

environmental! handles generics #307

Merged

dvdplm added 3 commits July 12, 2018 17:07

Add Finder artifacts

c7503ad

whitespace

711da4f

gnunicorn changed the title ~~Make substrate/state-machine generic over Hasher/NodeCodec~~ [WIP] Make substrate/state-machine generic over Hasher/NodeCodec Jul 12, 2018

gnunicorn added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jul 12, 2018

arkpar mentioned this pull request Jul 13, 2018

Make state generic over Hash #292

Closed

gavofyork added A1-onice and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 15, 2018

dvdplm added 2 commits August 9, 2018 10:27

Cleanup

35b29ba

Implement blake2_256 for no_std

04b7065

dvdplm 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 Aug 9, 2018

dvdplm added 3 commits August 9, 2018 12:19

Back on parity-common master

8152a78

missing type params

11ea517

dvdplm requested review from svyatonik and rphmeier August 9, 2018 12:02

Merge branch 'master' into refactor/substrate-state-machine-generic

4d14020

gavofyork reviewed Aug 9, 2018

View reviewed changes

Update Cargo.lock

19d07db

dvdplm added 2 commits August 9, 2018 16:51

whitespace

60df8c1

Merge branch 'refactor/substrate-state-machine-generic' of github.com…

6833f7b

…:paritytech/polkadot into refactor/substrate-state-machine-generic * 'refactor/substrate-state-machine-generic' of github.com:paritytech/polkadot: Update Cargo.lock Remove safe-mix (#521)

gavofyork added A6-mustntgrumble and removed A0-please_review Pull request needs code review. labels Aug 9, 2018

gavofyork approved these changes Aug 9, 2018

View reviewed changes

gavofyork merged commit d5de4bc into master Aug 9, 2018

gavofyork deleted the refactor/substrate-state-machine-generic branch August 9, 2018 15:37

dvdplm mentioned this pull request Aug 9, 2018

Cleanup BlakeHasher/BlakeRlpCodec #529

Open

gavofyork restored the refactor/substrate-state-machine-generic branch August 12, 2018 09:19

gavofyork mentioned this pull request Aug 14, 2018

Refactor/substrate state machine generic #553

Merged

JoshOrndorff pushed a commit to moonbeam-foundation/substrate that referenced this pull request Apr 21, 2021

increase max collators per nominator from 8 to 25 (paritytech#297)

eaedea0

* from 8 to 25 * bump spec version * Update runtime/src/lib.rs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use parity-common to make substrate generic over hasher and trie encoding codec #297

Use parity-common to make substrate generic over hasher and trie encoding codec #297

dvdplm commented Jul 11, 2018 •

edited

Loading

gnunicorn left a comment

gnunicorn Jul 12, 2018

dvdplm Jul 12, 2018 •

edited

Loading

gnunicorn Jul 12, 2018

dvdplm Jul 12, 2018

gnunicorn Jul 12, 2018

gnunicorn Jul 12, 2018

dvdplm Jul 12, 2018

dvdplm Jul 12, 2018

gnunicorn Jul 12, 2018

gnunicorn Jul 12, 2018

dvdplm Jul 12, 2018

arkpar commented Jul 13, 2018

svyatonik commented Aug 9, 2018

gavofyork Aug 9, 2018

dvdplm Aug 9, 2018

gavofyork Aug 9, 2018

gavofyork Aug 9, 2018

gavofyork commented Aug 9, 2018 •

edited

Loading

dvdplm commented Aug 9, 2018

gavofyork commented Aug 9, 2018 •

edited

Loading

		inner: Arc<HashMap<Vec<u8>, Vec<u8>>>,
		_hasher: PhantomData<H>,

Use parity-common to make substrate generic over hasher and trie encoding codec #297

Use parity-common to make substrate generic over hasher and trie encoding codec #297

Conversation

dvdplm commented Jul 11, 2018 • edited Loading

gnunicorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvdplm Jul 12, 2018 • 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

arkpar commented Jul 13, 2018

svyatonik commented Aug 9, 2018

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 Aug 9, 2018 • edited Loading

dvdplm commented Aug 9, 2018

gavofyork commented Aug 9, 2018 • edited Loading

dvdplm commented Jul 11, 2018 •

edited

Loading

dvdplm Jul 12, 2018 •

edited

Loading

gavofyork commented Aug 9, 2018 •

edited

Loading

gavofyork commented Aug 9, 2018 •

edited

Loading