Switch to shiny new fast, RLP-less trie #795

gavofyork · 2018-09-24T15:46:06Z

Closes #664

Also expunges RLP from the codebase. There's not really any new logic outside of the substrate-trie crate.

The codebase is no longer generic over NodeCodec; the reason for this twofold: firstly, trie node format allows for much less opinionated selection than the hashing algorithm. Formats do not significantly change in terms of security. Performance tweaks are non-controversial and are easily folded in over time. Secondly, it would require a two generic parameters, not one as before.

gavofyork · 2018-09-24T20:31:40Z

core/state-machine/src/changes_trie/build.rs

 		assert_eq!(changes_trie_nodes, Some(vec![
 			InputPair::ExtrinsicIndex(ExtrinsicIndex { block: 4, key: vec![100] }, vec![0, 2, 3]),
 			InputPair::ExtrinsicIndex(ExtrinsicIndex { block: 4, key: vec![101] }, vec![1]),
 			InputPair::ExtrinsicIndex(ExtrinsicIndex { block: 4, key: vec![103] }, vec![0, 1]),

-			InputPair::DigestIndex(DigestIndex { block: 4, key: vec![100] }, vec![1, 3]),


@svyatonik I had to change this to make the tests work. any idea why?

@gavofyork I'm getting an error "Error while iterating by prefix: Database missing expected key: 0x9500103000000000000000464cfe10021c011404000000001c01140401000000" here. So:

too bad that this error is discarded - will note this && prepare PR when other is done;

this seems like a similar issue when we have moved from KeccakHasher to Blake2Hasher, but this time I'm not sure where this constant cames from (probably it is in )

That said, this change ([1, 3] -> [1] + similar) is wrong.

ok if it's an issue with the iterator, then it would be good to have a test where i can reproduce it...

here's the test: 93e13a8

thanks. that's fixed now, but the fix didn't invalidate the changes here...

nm false alarm. tests confirm fix.

gavofyork · 2018-09-24T20:31:51Z

core/state-machine/src/changes_trie/build.rs

 			InputPair::DigestIndex(DigestIndex { block: 4, key: vec![101] }, vec![1]),
 			InputPair::DigestIndex(DigestIndex { block: 4, key: vec![102] }, vec![2]),
-			InputPair::DigestIndex(DigestIndex { block: 4, key: vec![105] }, vec![1, 3]),


@svyatonik here, too

gavofyork · 2018-09-24T20:32:05Z

core/state-machine/src/changes_trie/build.rs

 		assert_eq!(changes_trie_nodes, Some(vec![
 			InputPair::ExtrinsicIndex(ExtrinsicIndex { block: 4, key: vec![100] }, vec![0, 2, 3]),
 			InputPair::ExtrinsicIndex(ExtrinsicIndex { block: 4, key: vec![101] }, vec![1]),
 			InputPair::ExtrinsicIndex(ExtrinsicIndex { block: 4, key: vec![103] }, vec![0, 1]),

-			InputPair::DigestIndex(DigestIndex { block: 4, key: vec![100] }, vec![1, 3]),
+			InputPair::DigestIndex(DigestIndex { block: 4, key: vec![100] }, vec![1]),


@svyatonik ...and here...

gavofyork · 2018-09-24T20:32:10Z

core/state-machine/src/changes_trie/build.rs

 			InputPair::DigestIndex(DigestIndex { block: 4, key: vec![101] }, vec![1]),
 			InputPair::DigestIndex(DigestIndex { block: 4, key: vec![102] }, vec![2]),
-			InputPair::DigestIndex(DigestIndex { block: 4, key: vec![105] }, vec![1, 3]),


@svyatonik ...and here

gavofyork · 2018-09-24T20:35:24Z

@svyatonik quite a few of the change-trie tests include raw roots. there's little i can do except to replace with the expected root. i don't know how to ensure these trie changes haven't broken their logic.

svyatonik · 2018-09-24T20:41:59Z

@gavofyork The same is true for Header::state_root - it always changes when storage is altered => https://github.com/paritytech/substrate/blob/master/node/executor/src/lib.rs is updated often too. I'll take a look at the reason of "I had to change this to make the tests work. any idea why?", but generally I assume that the same steps should be taken with changes_trie_root.

gavofyork · 2018-09-24T20:52:46Z

@svyatonik yeah the hashes i'm less bothered about (though it is rather tedious). however, those four changes i pointed to above are switching out some of the structure's values, which is concerning.

rphmeier · 2018-09-24T21:43:43Z

core/trie/src/lib.rs

@@ -0,0 +1,635 @@
+// Copyright 2015-2018 Parity Technologies (UK) Ltd.


docs are missing in this file/crate

rphmeier · 2018-09-24T21:44:11Z

core/trie/src/lib.rs

+}
+
+/*
+// TODO: Remove


committing commented code

rphmeier · 2018-09-24T21:44:20Z

core/trie/src/lib.rs

+	use trie_standardmap::{Alphabet, ValueMode, StandardMap};
+
+	fn check_equivalent(input: &Vec<(&[u8], &[u8])>) {
+/*		{


rphmeier · 2018-09-25T10:35:21Z

core/executor/Cargo.toml

 twox-hash = "1.1.0"
 lazy_static = "1.0"
 parking_lot = "*"
 log = "0.3"
-hashdb = "0.2.1"
+hash-db = { git = "https://github.com/paritytech/trie" }


kind of unfortunate that we are moving back off of crates.io deps and into GitHub land

once it stablises (i.e. sometime between now and 1.0 final), i'm happy to publish and move back.

rphmeier · 2018-09-25T10:42:03Z

core/trie/src/node_codec.rs

+	fn leaf_node(partial: &[u8], value: &[u8]) -> Vec<u8> {
+		let mut output = partial_to_key(partial, LEAF_NODE_OFFSET, LEAF_NODE_BIG);
+		value.encode_to(&mut output);
+//		println!("leaf_node: {:#x?}", output);


stray println commented here

rphmeier · 2018-09-25T10:42:24Z

core/trie/src/node_codec.rs

+			ChildReference::Inline(inline_data, len) =>
+				(&AsRef::<[u8]>::as_ref(&inline_data)[..len]).encode_to(&mut output),
+		};
+//		println!("ext_node: {:#x?}", output);


rphmeier · 2018-09-25T10:42:28Z

core/trie/src/node_codec.rs

+			None => false,
+		}));
+		output[0..3].copy_from_slice(&prefix[..]);
+//		println!("branch_node: {:#x?}", output);


rphmeier · 2018-09-25T11:37:21Z

core/trie/src/node_codec_alt.rs

+// You should have received a copy of the GNU General Public License
+// along with Parity.  If not, see <http://www.gnu.org/licenses/>.
+
+//! `NodeCodec` implementation for Rlp


is this "alt" implementation for RLP for Parity Codec?

arkpar · 2018-09-25T14:00:06Z

core/trie/README.md

@@ -0,0 +1,2 @@
+This crate provides utility functions to validate and initialize tries using flexible input.
+It is used extensively in `parity-ethereum` to validate blocks (mostly transactions and receipt roots).


This looks irrelevant.

arkpar · 2018-09-25T14:05:42Z

core/trie/src/node_header.rs

+	Null,
+	Branch(bool),
+	Extension(usize),
+	Leaf(usize),


usize? Is it supposed to be platform-dependent? u32 should probably enough

this is mostly being assigned/compared to sizes of Vecs and has a custom Decode/Encode. switching to u32 would necessitate most casting but wouldn't provide any material benefit.

arkpar · 2018-09-25T14:07:48Z

core/trie/src/lib.rs

@@ -0,0 +1,421 @@
+// Copyright 2015-2018 Parity Technologies (UK) Ltd.
+// This file is part of Parity.


Needs updating?

dvdplm · 2018-09-25T13:36:36Z

core/trie/README.md

@@ -0,0 +1,2 @@
+This crate provides utility functions to validate and initialize tries using flexible input.
+It is used extensively in `parity-ethereum` to validate blocks (mostly transactions and receipt roots).


Might want to remove this line.

dvdplm · 2018-09-25T14:34:14Z

core/trie/src/lib.rs

+}
+
+/// Determine a trie root node's data given its ordered contents, closed form.
+pub fn unhashed_trie<H: Hasher, I, A, B>(input: I) -> Vec<u8> where


Is this method used for anything outside tests?

dvdplm · 2018-09-25T14:35:51Z

core/trie/src/node_codec.rs

+// NOTE: what we'd really like here is:
+// `impl<H: Hasher> NodeCodec<H> for RlpNodeCodec<H> where H::Out: Decodable`
+// but due to the current limitations of Rust const evaluation we can't
+// do `const HASHED_NULL_NODE: H::Out = H::Out( … … )`. Perhaps one day soon?


This comment is probably not helpful anymore.

dvdplm · 2018-09-25T14:43:37Z

core/trie/src/trie_stream.rs

+			_ => {
+//				println!("[append_substream] would have hashed, because data.len() = {}", data.len());
+//				data.encode_to(&mut self.buffer)
+				// TODO: re-enable hashing before merging


Some cleanup to do in this file.

* Remove Cargo.lock from gitignore Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * cargo: Add `Cargo.lock` Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * Update the releasing process Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

gavofyork added 8 commits September 21, 2018 15:27

Bump codec

c3d6f3c

Fix tests

a452f15

Patricia trie builds

71f5a11

Introduce trie

c3cf27b

Merge remote-tracking branch 'origin/master' into gav-compact

c560b60

Some yak shaving.

f5d2092

Some fixes

325c1e2

Remove RLP ref

0f55b2f

rphmeier added the A3-in_progress Pull request is in progress. No review needed at this stage. label Sep 24, 2018

gavofyork added 3 commits September 24, 2018 20:05

Fixes

5c5ba2d

It builds!

d1fa8be

Some tests fixed

f810cff

gavofyork commented Sep 24, 2018

View reviewed changes

gavofyork added 2 commits September 24, 2018 21:45

Merge remote-tracking branch 'origin/master' into gav-compact

910c899

Another test fix

f205823

gavofyork 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 Sep 24, 2018

Rejig more hashes

884c2de

rphmeier reviewed Sep 24, 2018

View reviewed changes

core/trie/src/lib.rs

@@ -0,0 +1,635 @@

// Copyright 2015-2018 Parity Technologies (UK) Ltd.

Copy link

Contributor

rphmeier Sep 24, 2018

Choose a reason for hiding this comment

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

docs are missing in this file/crate

rphmeier reviewed Sep 24, 2018

View reviewed changes

core/trie/src/lib.rs Outdated

}

/*

// TODO: Remove

Copy link

Contributor

rphmeier Sep 24, 2018

Choose a reason for hiding this comment

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

committing commented code

rphmeier reviewed Sep 24, 2018

View reviewed changes

svyatonik and others added 2 commits September 25, 2018 01:11

substrate-trie::iterator_works test

93e13a8

Merge remote-tracking branch 'origin/gav-compact-test' into gav-compact

972d61d

rphmeier reviewed Sep 25, 2018

View reviewed changes

gavofyork added 4 commits September 25, 2018 14:00

Update lock

ba666e0

Polish

9e243c2

Docs

ea36f87

Undo incorrect "fix" for tests

68ba63a

arkpar reviewed Sep 25, 2018

View reviewed changes

Fix nits

3139037

arkpar approved these changes Sep 25, 2018

View reviewed changes

arkpar merged commit e4c07ba into master Sep 25, 2018

arkpar deleted the gav-compact branch September 25, 2018 14:32

dvdplm reviewed Sep 25, 2018

View reviewed changes

liuchengxu pushed a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021

network type error (paritytech#795)

99a8bc9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to shiny new fast, RLP-less trie #795

Switch to shiny new fast, RLP-less trie #795

gavofyork commented Sep 24, 2018 •

edited

Loading

gavofyork Sep 24, 2018

svyatonik Sep 24, 2018

gavofyork Sep 24, 2018

svyatonik Sep 24, 2018

gavofyork Sep 25, 2018

gavofyork Sep 25, 2018

gavofyork Sep 24, 2018

gavofyork Sep 24, 2018

gavofyork Sep 24, 2018

gavofyork commented Sep 24, 2018

svyatonik commented Sep 24, 2018

gavofyork commented Sep 24, 2018

rphmeier Sep 24, 2018

rphmeier Sep 24, 2018

rphmeier Sep 24, 2018

rphmeier Sep 25, 2018

gavofyork Sep 25, 2018

rphmeier Sep 25, 2018

rphmeier Sep 25, 2018

rphmeier Sep 25, 2018

rphmeier Sep 25, 2018

arkpar Sep 25, 2018

arkpar Sep 25, 2018

gavofyork Sep 25, 2018

arkpar Sep 25, 2018

dvdplm Sep 25, 2018

dvdplm Sep 25, 2018

dvdplm Sep 25, 2018

dvdplm Sep 25, 2018

		@@ -0,0 +1,635 @@
		// Copyright 2015-2018 Parity Technologies (UK) Ltd.

		@@ -0,0 +1,2 @@
		This crate provides utility functions to validate and initialize tries using flexible input.
		It is used extensively in `parity-ethereum` to validate blocks (mostly transactions and receipt roots).

		@@ -0,0 +1,421 @@
		// Copyright 2015-2018 Parity Technologies (UK) Ltd.
		// This file is part of Parity.

Switch to shiny new fast, RLP-less trie #795

Switch to shiny new fast, RLP-less trie #795

Conversation

gavofyork commented Sep 24, 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

gavofyork commented Sep 24, 2018

svyatonik commented Sep 24, 2018

gavofyork commented Sep 24, 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

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

gavofyork commented Sep 24, 2018 •

edited

Loading