Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trie: add new variant headers #2641

Closed
wants to merge 5 commits into from
Closed

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Jul 4, 2022

Changes

Note: This is based on #2665

  • Add node headers
  • Reject decoding new headers when using v0 state trie
  • Proof verification to use compact encoding escape header
  • Implement leaf with hashed subvalue
  • Implement branch with hashed subvalue
  • Support child tries upgrade
  • Support proof generation/verification

Questions:

  • Should Encode return the too-large values or take in a Database interface to store them directly? How about for proof generation?

  • Should Decode take in a database interface to load values directly, or be passed a map of hash<->value? How about for proof verification?

  • How does Substrate retrieves subvalues from hashes? Using database lookup

  • How to know which version to use to encode/decode nodes in the state trie?

  • How does Substrate delete subvalues? For 'dumb' KV, they prepend the subvalue hash with the node key to go around the storing another key value in another table from hash subvalue to reference counter.

  • Regarding the ESCAPE_HEADER (aka 'reserved for compact encoding' aka 00010000), is the 'value node' just the scale encoding of the node subvalue bytes? Also why is this header needed if leaf and branch with hashed subvalue already have their own header 001 and 0001 (or the opposite, why did we need those 2 new headers if we have this compact encoding escape header)?

    /// Escape header byte sequence to indicate next node is a
    /// branch or leaf with hash of value, followed by the value node.
    const ESCAPE_HEADER: Option<u8> = None;

    See State trie version v0 vs v1 paritytech/substrate#11607 (comment)

Post checks:

  • Review tests using version.V0 and make them test cases with V1 as well

Tests

go test ./lib/trie/... ./internal/trie/...

Issues

Primary Reviewer

@codecov
Copy link

codecov bot commented Jul 4, 2022

Codecov Report

Merging #2641 (8bdeb8c) into development (19c9a7e) will increase coverage by 11.16%.
The diff coverage is 100.00%.

❗ Current head 8bdeb8c differs from pull request most recent head c9c3462. Consider uploading reports for the commit c9c3462 to get more accurate results

Additional details and impacted files
@@               Coverage Diff                @@
##           development    #2641       +/-   ##
================================================
+ Coverage        51.79%   62.96%   +11.16%     
================================================
  Files              220      212        -8     
  Lines            27711    27151      -560     
================================================
+ Hits             14354    17096     +2742     
+ Misses           12128     8492     -3636     
- Partials          1229     1563      +334     

@dimartiro
Copy link
Contributor

Closed in favour of #3295

@dimartiro dimartiro closed this Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants