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

chore(trie): refactor existing header encoding #2530

Merged
merged 9 commits into from
Jul 4, 2022

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented May 11, 2022

Changes

TLDR: Match more closely the polkadot spec documentation in the code, to allow further changes for new node variants. This doesn't add new node variants yet, this is coming in the next PR.

  • Declare node variants as bits+mask instead of just bits (or the more obscure enum integer we had)
  • encodeHeader encodes the first header byte + extra partial key length bytes instead of the convoluted/not-modular encodeHeader + encodeKeyLength
  • decodeHeader function to get variant + partial key length, instead of convoluted in-line header code in Decode + further header decoding in decodeLeaf and decodeBranch
  • Get node variant header partial key length mask dynamically from the node variant header bit mask (not a constant anymore)
  • Clarify codec documentation in a README.md document
  • Remove single byte sync pool (slower than stack byte slice of length 1)
  • ⚠️ Fix decodeKey maximum check for last byte
  • ⚠️ Fix decodeKey maximum check to accept maximum value 65535
  • ⚠️ allow to encode partial key of size 65535
  • Panic for programming errors cases
  • Revise some of the error wrapping
  • Use uint16 for partialKeyLength

Tests

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

Issues

Primary Reviewer

@EclesioMeloJunior

@qdm12 qdm12 force-pushed the qdm12/trie/merge-leaf-branch branch from 257277e to 56069ef Compare May 11, 2022 14:18
@qdm12 qdm12 marked this pull request as ready for review May 11, 2022 14:30
@codecov
Copy link

codecov bot commented May 11, 2022

Codecov Report

Merging #2530 (374a29b) into development (5dc567e) will decrease coverage by 0.04%.
The diff coverage is 94.61%.

@@               Coverage Diff               @@
##           development    #2530      +/-   ##
===============================================
- Coverage        61.98%   61.94%   -0.05%     
===============================================
  Files              215      215              
  Lines            28450    28485      +35     
===============================================
+ Hits             17636    17644       +8     
- Misses            9064     9087      +23     
- Partials          1750     1754       +4     
Flag Coverage Δ
unit-tests 61.94% <94.61%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
internal/trie/node/encode.go 93.33% <ø> (ø)
internal/trie/node/node.go 100.00% <ø> (ø)
internal/trie/node/decode.go 91.11% <78.12%> (-4.81%) ⬇️
internal/trie/node/header.go 100.00% <100.00%> (ø)
internal/trie/node/key.go 100.00% <100.00%> (ø)
dot/network/inbound.go 92.98% <0.00%> (-7.02%) ⬇️
dot/rpc/modules/grandpa.go 83.72% <0.00%> (-5.44%) ⬇️
dot/network/block_announce.go 58.40% <0.00%> (-4.81%) ⬇️
dot/network/connmgr.go 89.04% <0.00%> (-1.37%) ⬇️
lib/blocktree/blocktree.go 53.62% <0.00%> (-1.09%) ⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5dc567e...374a29b. Read the comment docs.

@qdm12 qdm12 force-pushed the qdm12/trie/merge-leaf-branch branch from 56069ef to 7312da8 Compare May 12, 2022 06:55
@qdm12 qdm12 force-pushed the qdm12/trie/v1/refactor branch 2 times, most recently from bd11034 to 03ba959 Compare May 12, 2022 14:09
@qdm12 qdm12 force-pushed the qdm12/trie/merge-leaf-branch branch 4 times, most recently from ae17eb0 to a509c65 Compare June 6, 2022 18:39
Base automatically changed from qdm12/trie/merge-leaf-branch to development June 6, 2022 21:15
internal/trie/node/decode.go Show resolved Hide resolved
internal/trie/node/decode.go Outdated Show resolved Hide resolved
internal/trie/node/decode.go Show resolved Hide resolved
internal/trie/node/decode.go Show resolved Hide resolved
internal/trie/node/header.go Outdated Show resolved Hide resolved
qdm12 and others added 7 commits June 29, 2022 15:14
- Remove `encode_doc.go`
- Add `README.md` with Codec section
- Update comments for `Encode` and `Decode` methods
- Add `node` package comment
Co-authored-by: Eclésio Junior <eclesiomelo.1@gmail.com>
Co-authored-by: Eclésio Junior <eclesiomelo.1@gmail.com>
internal/trie/node/header.go Show resolved Hide resolved
internal/trie/node/variants.go Show resolved Hide resolved
internal/trie/node/decode.go Show resolved Hide resolved
internal/trie/node/decode.go Show resolved Hide resolved
@qdm12 qdm12 requested a review from timwu20 June 29, 2022 19:31
internal/trie/node/decode.go Show resolved Hide resolved
@qdm12 qdm12 merged commit d3282f7 into development Jul 4, 2022
@qdm12 qdm12 deleted the qdm12/trie/v1/refactor branch July 4, 2022 17:52
rrtti pushed a commit to polkadot-fellows/seeding that referenced this pull request Sep 29, 2022
I am Quentin Mc Gaw, a software engineer working the Go Polkadot host **Gossamer**.
I have been working full time on Gossamer since October 2021, mostly on the state trie and storage.
I have also made a [few minor pull requests](https://github.com/w3f/polkadot-spec/pulls?q=is%3Apr+is%3Aclosed+author%3Aqdm12) to the Polkadot specification repository.

I am requesting to join the Fellowship at rank 1.

## Main contributions

### Gossamer

- Fix memory leaks
  - Trie encoding buffer pools usage fixed [#2009](ChainSafe/gossamer#2009)
  - Fix state map of tries memory leak [#2286](ChainSafe/gossamer#2286)
  - Fix sync benchmark [#2234](ChainSafe/gossamer#2234)
- Trie proof fixes ([#2604](ChainSafe/gossamer#2604), [#2661](ChainSafe/gossamer#2661))
- Fix end to end tests orchestration ([#2470](ChainSafe/gossamer#2470), [#2452](ChainSafe/gossamer#2452), [#2385](ChainSafe/gossamer#2385), [#2370](ChainSafe/gossamer#2370))
- State trie statistics ([#2378](ChainSafe/gossamer#2378), [#2310](ChainSafe/gossamer#2310), [#2272](ChainSafe/gossamer#2272))
- State trie fixes and improvements
  - Only deep copy nodes when mutation is certain [#2352](ChainSafe/gossamer#2352) and [#2223](ChainSafe/gossamer#2223)
  - Only deep copy necessary fields of a node [#2384](ChainSafe/gossamer#2384)
  - Use Merkle values for database keys instead of always hash [#2725](ChainSafe/gossamer#2725)
  - Opportunistic parallel Merkle value commputing [#2081](ChainSafe/gossamer#2081)
- Grandpa capped number of tracked messages ([#2490](ChainSafe/gossamer#2490), [#2485](ChainSafe/gossamer#2485))
- Add pprof HTTP service for profiling [#1991](ChainSafe/gossamer#1991)

Ongoing work:

- State trie lazy loading and caching
- State trie v1 support ([#2736](ChainSafe/gossamer#2736), [#2747](ChainSafe/gossamer#2747), [#2687](ChainSafe/gossamer#2687), [#2686](ChainSafe/gossamer#2686), [#2685](ChainSafe/gossamer#2685), [#2673](ChainSafe/gossamer#2673), [#2611](ChainSafe/gossamer#2611), [#2530](ChainSafe/gossamer#2530))

### Polkadot specification

➡️ [Pull requests from qdm12](https://github.com/w3f/polkadot-spec/pulls?q=is%3Apr+is%3Aclosed+author%3Aqdm12)
@github-actions
Copy link

🎉 This PR is included in version 0.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

3 participants