Adds first version of `validate_block` by bkchr · Pull Request #3 · paritytech/cumulus

bkchr · 2019-03-07T13:12:25Z

Depends on: paritytech/substrate#1926

runtime/src/lib.rs

runtime/src/validate_block/storage_functions.rs

rphmeier · 2019-04-15T10:38:14Z

runtime/src/validate_block/implementation.rs

+type HashOf<B> = <B as BlockT>::Hash;
+
+/// Abstract the storage into a trait without `Block` generic.
+trait StorageT {


a bit weird to call it StorageT. why not just Storage?

Because I have a struct that is called Storage in the same module 😅

rphmeier · 2019-04-15T10:40:26Z

runtime/src/validate_block/implementation.rs

+		self.overlay.get(key).cloned().or_else(|| {
+			read_trie_value(
+				&self.witness_data,
+				&self.storage_root, key


key should go on new line.

rphmeier · 2019-04-15T10:41:27Z

runtime/src/validate_block/implementation.rs

+	}
+}
+
+unsafe fn ext_set_storage(


i would prefer to extract these to a submodule where we can be a bit more sure about invariants

Hmm, this is already its own module and you are sure about the invariants. This function is only usable by validate_block. I would not put this into another file.

rphmeier · 2019-04-15T10:43:50Z

runtime/src/validate_block/implementation.rs

+
+use hash_db::HashDB;
+
+static mut STORAGE: Option<Box<StorageT>> = None;


eep. still not a fan of the static mut. Why not at least a RefCell?

TBH I see no reason why, with native-runtime optimization, this is guaranteed not to be used from multiple threads

This will never be callable by the native side. First we don't support replacing the functions on native, second register_validate_block! resolves to nothing on native.

rphmeier · 2019-04-15T10:44:54Z

runtime/src/validate_block/implementation.rs

+struct Storage<B: BlockT> {
+	witness_data: MemoryDB<<HashingOf<B> as HashT>::Hasher>,
+	overlay: hashbrown::HashMap<Vec<u8>, Option<Vec<u8>>>,
+	storage_root: HashOf<B>,


B::Hash is easier

rphmeier · 2019-04-15T10:45:01Z

runtime/src/validate_block/implementation.rs

+		storage_root: HashOf<B>
+	) -> Result<Self, &'static str> {
+		let mut db = MemoryDB::default();
+		data.into_iter().for_each(|i| { db.insert(&[], &i); });


what does the &[] mean here?

I don't really know. I copied this code from substrate. I think that &[] is the prefix. Maybe @cheme can help us here?

That is the prefix for the MemoryDB, I believe here if MemoryDB is used from substrate trie crate, there is no key transformation with the prefix so it is safe to put nothing (a variant called PrefixedMemoryDB does insert the prefix before the key when inserting/querying the keyvaluedb to avoid collision in trie with unhashed value).

hm, maybe we need this when using a chain with child tries

there is the key conflict with child trie, but this way of doing things cannot solve it directly (for two identical child trie there is the sametrie related prefix). I was also wondering about exposing this key transformation trait to insert a child trie specific prefix with a specialized implementation (paritytech/trie#12 last comment), but using a hashdb specific implementation would achieve the same thing.

I assume that we then need to adapt this? This is basically the functionality I use to generate the proof. I assume we would need to extend the proof with the prefix somehow?

If you use child trie I think you need paritytech/substrate#2209 (I should split it in two has I believe the api change would not be merged or at least requires some consensus but the proof side of the pr seems ok).
But otherwhise the prefix thing should only apply to avoid reference count in db, so the Recorder of trie does not use the prefix so it should not matter (that is the good thing about the way Arkadiy solves the issue).

rphmeier · 2019-04-15T10:45:43Z

runtime/src/validate_block/implementation.rs

+		Some(mut value) => {
+			*written_out = value.len() as u32;
+			let ptr = value.as_mut_ptr();
+			mem::forget(ptr);


what's the purpose of forgetting ptr (which is copy) and then using it right after?

Was incorrect and is fixed now.

cheme · 2019-04-15T12:11:32Z

runtime/Cargo.toml

+	"substrate-trie/std",
+]
+wasm = [
+	"hashbrown/nightly",


is this wasm feature something native or something that require some feature declaration?
(I remember that I wanted to use hashbrown but use hasmap_core in trie because it felt easier to manage with 'std' substrate feature, just thinking let's wait for hashbrown to be std and see if at some point it makes it to alloc).

This requires a feature declaration.

Fix version

…ion-fix Feature/new session fix

bkchr added 3 commits March 6, 2019 12:53

Adds validate_block support for runtime

ecbf2c4

Adds test-runtime to start working on tests

68100e7

Make the tests pass

86fc199

bkchr requested a review from rphmeier March 7, 2019 13:12

paritytech deleted a comment from parity-cla-bot Mar 7, 2019

rphmeier reviewed Mar 11, 2019

View reviewed changes

runtime/src/lib.rs Show resolved Hide resolved

rphmeier reviewed Mar 11, 2019

View reviewed changes

runtime/src/lib.rs Outdated Show resolved Hide resolved

rphmeier reviewed Mar 11, 2019

View reviewed changes

runtime/src/lib.rs Outdated Show resolved Hide resolved

rphmeier reviewed Mar 11, 2019

View reviewed changes

runtime/src/validate_block/storage_functions.rs Outdated Show resolved Hide resolved

rphmeier reviewed Mar 11, 2019

View reviewed changes

runtime/src/validate_block/storage_functions.rs Outdated Show resolved Hide resolved

rphmeier reviewed Mar 11, 2019

View reviewed changes

runtime/src/validate_block/storage_functions.rs Outdated Show resolved Hide resolved

rphmeier added the A4-gotissues label Mar 11, 2019

bkchr added 8 commits March 28, 2019 17:33

Update to latest polkadot/substrate

80b222c

Start using trie db

2c6bf07

Adapt to latest changes in the specification

101d338

Trying to get the tests back working

815e466

Fixes simple test

f213d5e

Implement test that executes extrinsics

cbaef32

Some docs

4fa5d35

Some fixes

391792f

rphmeier reviewed Apr 15, 2019

View reviewed changes

cheme reviewed Apr 15, 2019

View reviewed changes

bkchr added 2 commits April 17, 2019 11:21

Forget the correct value

092f9ac

Fix review comments

f124354

rphmeier added A8-looksgood and removed A4-gotissues labels Apr 17, 2019

rphmeier approved these changes Apr 17, 2019

View reviewed changes

rphmeier merged commit 2700e3b into master Apr 17, 2019

rphmeier deleted the bkchr-validate_block branch April 17, 2019 12:26

coriolinus pushed a commit that referenced this pull request Mar 24, 2020

Merge pull request #3 from substrate-developer-hub/ricardo-fix-version

97eecd6

Fix version

bkchr mentioned this pull request Sep 26, 2022

Member Request polkadot-fellows/seeding#3

Merged

imstar15 pushed a commit to imstar15/cumulus that referenced this pull request Nov 16, 2022

Merge pull request paritytech#3 from mangata-finance/feature/new-sess…

7cceb91

…ion-fix Feature/new session fix


		use hash_db::HashDB;

		static mut STORAGE: Option<Box<StorageT>> = None;

Conversation

bkchr commented Mar 7, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cheme Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cheme Apr 15, 2019 •

edited

Loading