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

Update to latest trie version. #10972

Merged
merged 20 commits into from
Aug 15, 2019
Merged

Update to latest trie version. #10972

merged 20 commits into from
Aug 15, 2019

Conversation

cheme
Copy link
Contributor

@cheme cheme commented Aug 14, 2019

This PR update to latest trie crates (and parity-util-mem 0.2).
Those crates introduces some changes in codec:

  • trait changes: a Layout trait defines that we uses extension node plus the hash and codec to use.
  • codec changes: the partial slice used internally in the trie do not anymore contain a header or is encoded (this explains that the first byte is skipped in decoding of children slice and that two methods are added to encode the slices).

@cheme cheme added the A0-pleasereview 🤓 Pull request needs code review. label Aug 14, 2019
Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Looks good, minor style nits.

util/patricia-trie-ethereum/src/rlp_node_codec.rs Outdated Show resolved Hide resolved
util/patricia-trie-ethereum/src/rlp_node_codec.rs Outdated Show resolved Hide resolved
util/patricia-trie-ethereum/src/rlp_node_codec.rs Outdated Show resolved Hide resolved
util/patricia-trie-ethereum/src/rlp_node_codec.rs Outdated Show resolved Hide resolved
util/patricia-trie-ethereum/src/rlp_node_codec.rs Outdated Show resolved Hide resolved
@ordian ordian removed their assignment Aug 14, 2019
@ordian ordian added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M4-core ⛓ Core client code / Rust. labels Aug 14, 2019
@ordian ordian added this to the 2.7 milestone Aug 14, 2019
@ordian ordian removed the A0-pleasereview 🤓 Pull request needs code review. label Aug 14, 2019
@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Aug 14, 2019
cheme and others added 2 commits August 14, 2019 17:21
Co-Authored-By: Andronik Ordian <write@reusable.software>
Co-Authored-By: Andronik Ordian <write@reusable.software>
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

minor grumbles and questions

util/patricia-trie-ethereum/src/lib.rs Outdated Show resolved Hide resolved
util/patricia-trie-ethereum/src/rlp_node_codec.rs Outdated Show resolved Hide resolved
util/patricia-trie-ethereum/src/rlp_node_codec.rs Outdated Show resolved Hide resolved
if r.is_data() && r.size() == KeccakHasher::LENGTH {
Some(r.as_val().expect("Hash is the correct size; qed"))

if data.len() == KeccakHasher::LENGTH {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How come we don't need to check that data is rlp data anymore? Was that check unneeded before maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a change from upstream trie crate, the try_decode_hash is using directly the hash data, and we skip the header in decode function at
nodes[i] = Some(&value.as_raw()[1..]);. (I will change this line, writing this comment make me realize there is a cleanest way of doing this and without the implied malleability from skipping a byte).

The change is related to the fact that when trie uses this payload as an encoded children (try_decode_hash return none) trie code previously need to manage this byte.

So now this is done in decode. Thinking again that is not ideal and it would have been more direct to return either a hash or a new smaller slice from try_decode function.
Which is not perfect either (if the type of children could be encoded heading byte for instance).

util/patricia-trie-ethereum/src/rlp_node_codec.rs Outdated Show resolved Hide resolved
Better comments and formatting.
util/patricia-trie-ethereum/src/rlp_node_codec.rs Outdated Show resolved Hide resolved
util/patricia-trie-ethereum/src/rlp_node_codec.rs Outdated Show resolved Hide resolved
cheme and others added 2 commits August 15, 2019 11:39
Co-Authored-By: David <dvdplm@gmail.com>
Co-Authored-By: David <dvdplm@gmail.com>
@dvdplm dvdplm merged commit 5807402 into openethereum:master Aug 15, 2019
ordian added a commit that referenced this pull request Aug 15, 2019
* master:
  [evmbin] fix compilation (#10976)
  Update to latest trie version. (#10972)
  [blooms-db] Fix benchmarks (#10974)
  Fix ethcore/benches build. (#10964)
  tx-pool: accept local tx with higher gas price when pool full (#10901)
  Disable unsyncable expanse chain (#10926)
  Extract Machine from ethcore (#10949)
  removed redundant state_root function from spec, improve spec error types (#10955)
  Add support for Energy Web Foundation's new chains (#10957)
  [evmbin] add more tests to main.rs (#10956)
  Fix compiler warnings in util/io and upgrade to edition 2018 Upgrade mio to latest (#10953)
  unify loading spec && further spec cleanups (#10948)
  refactor: Refactor evmbin CLI (#10742)
  journaldb changes (#10929)
  Allow default block parameter to be blockHash (#10932)
  Enable sealing when engine is ready (#10938)
  Fix some warnings and typos. (#10941)
  Updated security@parity.io key (#10939)
  Change the return type of step_inner function. (#10940)
ordian added a commit that referenced this pull request Aug 15, 2019
* master:
  Verify transaction against its block during import (#10954)
  [evmbin] fix compilation (#10976)
  Update to latest trie version. (#10972)
  [blooms-db] Fix benchmarks (#10974)
  Fix ethcore/benches build. (#10964)
  tx-pool: accept local tx with higher gas price when pool full (#10901)
  Disable unsyncable expanse chain (#10926)
dvdplm added a commit that referenced this pull request Aug 15, 2019
* master:
  Extract the Engine trait (#10958)
  Better error message for rpc gas price errors (#10931)
  [.gitlab.yml] cargo check ethcore benches (#10965)
  Verify transaction against its block during import (#10954)
  [evmbin] fix compilation (#10976)
  Update to latest trie version. (#10972)
  [blooms-db] Fix benchmarks (#10974)
  Fix ethcore/benches build. (#10964)
  tx-pool: accept local tx with higher gas price when pool full (#10901)
  Disable unsyncable expanse chain (#10926)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants