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.

Sign up for GitHub

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

Jump to bottom

chore(lib/runtime): refactor version struct and codec #2673

Merged
merged 9 commits into from
Aug 16, 2022
Merged

chore(lib/runtime): refactor version struct and codec #2673

merged 9 commits into from
Aug 16, 2022

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Jul 15, 2022 •
edited
Loading

Changes

  • Version code
    • Single Version struct
    • Encoding the Version always encode all the last fields
    • Decoding the version data to Version sets possible absent fields as zero if not in the data (transaction_version)
    • Remove LegacyVersionData struct, Version interface, getter methods, cloned structs for codec
    • Move decoding logic from lib/runtime/wasmer/exports.go to lib/runtime/version.go in DecodeVersion function
    • Use struct value for Version instead of its pointer (no reason to use a pointer)
  • Test code
    • Refactor multiple TestInstance_Version* in one test with test cases
    • Add runtime API items as expected fields in tests
    • Remove version runtime mockery mock
    • Simplify testing in other packages since we no longer need to use a test-only exported constructor NewVersionData

Tests

Issues

I had to change a whole bunch of version related code to persist the state version, and I thought I would simplify it since it was rather confusing as a reader.

Primary Reviewer

@timwu20

@qdm12 qdm12 force-pushed the qdm12/runtime/version-structs-refactor branch 2 times, most recently from a82106d to 358f997 Compare July 19, 2022 15:22
@qdm12 qdm12 marked this pull request as ready for review July 19, 2022 15:51
@qdm12 qdm12 force-pushed the qdm12/runtime/version-structs-refactor branch 4 times, most recently from ad971fa to 5704086 Compare July 20, 2022 15:16
@codecov
Copy link

codecov bot commented Jul 20, 2022 •
edited
Loading

Codecov Report

Merging #2673 (3386e46) into development (8b0f6f0) will decrease coverage by 0.04%.
The diff coverage is 85.71%.

Additional details and impacted files 
@@               Coverage Diff               @@
##           development    #2673      +/-   ##
===============================================
- Coverage        62.91%   62.86%   -0.05%     
===============================================
  Files              213      213              
  Lines            27081    26993      -88     
===============================================
- Hits             17037    16970      -67     
+ Misses            8491     8475      -16     
+ Partials          1553     1548       -5

@qdm12 qdm12 force-pushed the qdm12/runtime/version-structs-refactor branch 2 times, most recently from 0bfc86f to acff5db Compare July 20, 2022 18:52
@qdm12 qdm12 force-pushed the qdm12/runtime/version-structs-refactor branch from acff5db to ecfdd74 Compare July 28, 2022 14:54
@qdm12 qdm12 force-pushed the qdm12/runtime/version-structs-refactor branch from ecfdd74 to 9675efa Compare August 8, 2022 15:24
noot
noot approved these changes Aug 8, 2022
View reviewed changes
Copy link
Contributor

@noot noot left a comment

Choose a reason for hiding this comment

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

i think this is fine assuming the encoding is the same (should be cause tests pass :p)

@qdm12 qdm12 force-pushed the qdm12/runtime/version-structs-refactor branch from aa3bad5 to b187971 Compare August 9, 2022 17:04
timwu20
timwu20 requested changes Aug 9, 2022
View reviewed changes
dot/core/service.go Show resolved Hide resolved
dot/rpc/subscription/listeners.go Show resolved Hide resolved
lib/runtime/version.go Outdated Show resolved Hide resolved
@qdm12 qdm12 mentioned this pull request Aug 9, 2022
Merged
@qdm12 qdm12 force-pushed the qdm12/runtime/version-structs-refactor branch from b187971 to ab9721f Compare August 9, 2022 22:26
@qdm12
Copy link
Contributor Author

qdm12 commented Aug 10, 2022

⚠️ do not merge, this needs more changes (to support N versions of the version since the new state_version field is yet another version).

@qdm12 qdm12 mentioned this pull request Aug 10, 2022
Merged
@qdm12 qdm12 force-pushed the qdm12/runtime/version-structs-refactor branch 3 times, most recently from 901376b to c4e418d Compare August 10, 2022 18:51
@qdm12 qdm12 force-pushed the qdm12/runtime/version-structs-refactor branch from c4e418d to a2b9f95 Compare August 10, 2022 18:57
@qdm12 qdm12 requested review from timwu20 and noot August 11, 2022 00:09
@qdm12
Copy link
Contributor Author

qdm12 commented Aug 11, 2022

This is ready for re-reviews again, thanks!

@qdm12 qdm12 changed the title chore(lib/runtime): export fields of VersionData chore(lib/runtime): refactor version struct and codec Aug 11, 2022
This was referenced Aug 11, 2022
Merged
Merged
Merged
@qdm12 qdm12 force-pushed the qdm12/runtime/version-structs-refactor branch 2 times, most recently from 11e5200 to 6486321 Compare August 15, 2022 19:22
@qdm12
chore(lib/runtime): export fields of VersionData
476c188 
- Simplify testing in other packages
- Remove version decode method (just use `scale.Unmarshal` directly)
- Remove clone struct with exported fields for scale codec
- Remove version runtime mockery mock
- Refactor multiple `TestInstance_Version*` in one test with test cases
- Add runtime API items as expected fields in tests
- side-effect: add `Get` prefix to getter methods (sus but better than having messy double-structs-which-make-testing-hard)
@qdm12
Only single VersionData struct
242bb5d 
- Remove `Version` interface
- Use `legacy` bool field in version struct
- Unexport `LegacyVersionData`
- Remove all getter methods
- Move decoding logic from wasmer to lib/runtime/version.go
- Use struct value instead of its pointer
@qdm12
Rename VersionData to Version
3f6c09c
@qdm12
Decode method -> DecodeVersion function
07c9d3e
@qdm12
Add more comments to version.Encode usage
756d00e
@qdm12
Fix Test_Instance_Version for Kusama
930c938 
- Add `WithLegacy()` method to set unexported `legacy` field in wasmer test
@qdm12
More tests
41c705f
@qdm12
Core_Version codec changes
f48ee8e 
- Always encode using all latest  fields
- Decode required fields then optional fields
- Remove `legacyData` struct
- Update `Encode` and `DecodeVersion` comments
- Update unit tests
@qdm12
Remove version Encode method
3386e46 
- Update comment for `scale.Marshal` on `Version`
- Remove `Test_Version_Encode`
@qdm12 qdm12 force-pushed the qdm12/runtime/version-structs-refactor branch from 6486321 to 3386e46 Compare August 16, 2022 00:08
@qdm12 qdm12 merged commit 5c8446e into development Aug 16, 2022
@qdm12 qdm12 deleted the qdm12/runtime/version-structs-refactor branch August 16, 2022 12:09
@qdm12 qdm12 mentioned this pull request Sep 29, 2022
Merged
rrtti pushed a commit to polkadot-fellows/seeding that referenced this pull request Sep 29, 2022
@qdm12
Member Request (#28)
a37485d 
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
Reviewers

@timwu20 timwu20 timwu20 approved these changes

@EclesioMeloJunior EclesioMeloJunior EclesioMeloJunior approved these changes

@edwardmack edwardmack Awaiting requested review from edwardmack

@jimjbrettj jimjbrettj Awaiting requested review from jimjbrettj jimjbrettj is a code owner

@kishansagathiya kishansagathiya Awaiting requested review from kishansagathiya kishansagathiya is a code owner

@noot noot Awaiting requested review from noot

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@qdm12 @timwu20 @EclesioMeloJunior @noot