Conversation
|
@cheme needs resolving, and you'll probably want to bump |
|
@svyatonik @arkpar any chance you could take a look at the trie logic here and verify it's sensible? |
| //for test_i in 0..10000 { | ||
| for test_i in 0..1000 { | ||
| if test_i % 50 == 0 { | ||
| println!("{:?} of 10000 stress tests done", test_i); |
| BranchWithValue, | ||
| } | ||
|
|
||
| impl Encode for NodeHeader { |
There was a problem hiding this comment.
Isn't it the same encoding/decoding as ReferenceTrieStreamNoExt? Can't we just re-use that?
There was a problem hiding this comment.
That is the same implementation, we got this double implementation situation (reference-test from trie is also problematic for other reason).
It could be possible to move this code in its own crate in trie, but it will not be in substrate.
We could also use this implementation from reference trie but at the cost of a dependency to substrate (maybe with a published crate this won't be such an issue).
There was a problem hiding this comment.
Thinking twice, codec implementation needs to be in substrate repo, so to use it for tests in trie repo we could create a new crate core/trie/nodecodec that does not depends on any substrate crate and could easily be published.
That would not solve the issue with trie repo reference trie crate dependencies but would remove this duplicated code.
Do you think it shall be done now or later (with a issue of course)?
node-template/runtime/build.rs
Outdated
| WasmBuilderSource::Crates("1.0.4"), | ||
| WasmBuilderSource::CratesOrPath { | ||
| path: "../../core/utils/wasm-builder", | ||
| version: "1.0.4", |
There was a problem hiding this comment.
This can be reverted (I need it for patching wasm, but when trie crates will be published this won't be necessary anymore, I should have put a comment).
|
trie changes merged, so i guess the repo path for that dependency can change. |
|
Nice, I will publish and update first thing in the morning tomorrow. |
(require another pr to update version).
|
@cheme please ping @arkpar and @svyatonik when this is updated and you've checked all existing comments and typical issues relating to code quality. i'd like to get this in today. |
|
Yes I got a small unexpected issue, If anyone can review/approve paritytech/trie#26 (it is just a version update). |
|
@arkpar @svyatonik I did update patched dependency, the PR should be ready for review. About the fact that codec code is duplicated, I think it should remain in substrate repo (and possibly be use by reference trie after getting split in a less dependant crate), but did not do the necessary work in this PR. CI is failing but it seems to be global to all CI pipelines. |
This PR link substrate code base with trie code base post paritytech/trie#11 .
Currently review and publishing of the change on trie crates are required first.
Yet this is important code to help with the trie review.
I labelled this PR as
in progressbecause it is currently using patched dependancies, but I see the code as reviewable.This PR switch to the new trie definition (see polkadot re spec https://github.com/w3f/polkadot-re-spec ).
The switch can be de-activated by adding 'legacy-trie' from default features in crates containing this feature, to check former implementation.
Ideally the trie definition should not be local to trie crate but use as a type parameter. I started this refactoring with the trie crate (this explain some of the design), but to change it for substrate it would require replacing the ubiquitous
Hashertype parameter byTrieOps.That is a task that touch all code base and probably something that should be merge quickly, so I did not make it in this PR (the 'legacy-trie' feature is a temporary alternative). More generally an type parameter with
TrieOpsas associatid trait may be better to avoid this kind of refactoring whenever a new trait need to be accessible almost everywhere likeHasher.The legacy trie code should probably be dropped, but it seems important to me to test substrate with it because paritytech/trie#11 contains a few api change.
Also a few test are temporary disabled (macros until we stop using patched crate).