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(all): version trie calls #2774

Closed
wants to merge 5 commits into from
Closed

chore(all): version trie calls #2774

wants to merge 5 commits into from

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Aug 23, 2022 •
edited
Loading

Changes

  • Version relevant trie calls (Put, Hash)
  • Convert wasmer Core_version Version to typed trie.Version
  • Adapt tests/test helpers to read the state version from the genesis runtime code
  • Fix unit tests
  • Fix integration tests

Tests

Entire codebase test suite

Issues

#2418

Primary Reviewer

@edwardmack

@qdm12 qdm12 force-pushed the qdm12/trie/versioned branch 2 times, most recently from 76f78d0 to 34e45da Compare August 23, 2022 21:08
@qdm12 qdm12 changed the base branch from development to qdm12/wasmer/persist-version August 23, 2022 21:33
qdm12
qdm12 commented Aug 23, 2022
View reviewed changes
dot/core/service.go
@@ -60,6 +61,7 @@ type Service struct {

// Config holds the configuration for the core Service.
type Config struct {
// TODO add state version field here
Copy link
Contributor Author

@qdm12 qdm12 Aug 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

TODO once tests are fixed

dot/services.go Outdated Show resolved Hide resolved
qdm12
qdm12 commented Aug 24, 2022
View reviewed changes
dot/core/service.go
Comment on lines +488 to +459
// Note: not too sure why this trie state call is needed,
// but some RPC tests fail without it.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO see #2769

dot/state/block.go Outdated
}

return stateVersion, nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add unit/integ test

dot/sync/chain_processor.go
Comment on lines +230 to 235
// TODO shall we remove this? Both are coming from the internal state
rootV0 := ts.MustRoot(trie.V0)
if !rootV0.Equal(parent.StateRoot) {
// TODO add extra check with rootV1 when v1 is supported
panic("parent state root does not match snapshot state root")
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opinions on that? Should we remove it? Otherwise we'll have to hash tries for v0 and v1 which might have a performance impact.

lib/genesis/genesis.go
@@ -94,6 +97,23 @@ func (g *Genesis) ToRaw() error {
return nil
}

var ErrRuntimeCodeNotFound = errors.New("runtime code not found")

func (g *Genesis) RuntimeCode() (runtimeCode []byte, err error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add unit test

lib/runtime/wasmer/exports.go Outdated
// This is cheap to call since the instance version is cached.
func (in *Instance) StateVersion() (stateVersion trie.Version, err error) {
coreVersion := in.ctx.Version
return trie.ParseVersion(coreVersion.StateVersion)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add unit test

lib/runtime/wasmer/imports.go Outdated Show resolved Hide resolved
lib/runtime/wasmer/instance.go
return stateVersion, fmt.Errorf("parsing state version: %w", err)
}

return stateVersion, nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add unit test

lib/trie/version.go
Comment on lines +36 to +38
func ParseVersion[T string | uint32](x T) (version Version, err error) {
var s string
switch value := any(x).(type) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a fun use of generic imo

Base automatically changed from qdm12/wasmer/persist-version to development August 24, 2022 21:17
@qdm12 qdm12 force-pushed the qdm12/trie/versioned branch 2 times, most recently from 78dce0a to f931f24 Compare August 25, 2022 15:21
@qdm12 qdm12 changed the base branch from development to qdm12/state/newtries August 25, 2022 20:41
@qdm12 qdm12 marked this pull request as ready for review August 25, 2022 20:48
Base automatically changed from qdm12/state/newtries to development August 29, 2022 11:01
This was referenced Aug 29, 2022
Closed
Merged
Merged
@qdm12 qdm12 force-pushed the qdm12/trie/versioned branch 2 times, most recently from c5ab500 to 3ac2daf Compare September 6, 2022 15:01
@qdm12 qdm12 changed the base branch from development to qdm12/genesis/less-dependencies September 6, 2022 15:37
@qdm12 qdm12 force-pushed the qdm12/genesis/less-dependencies branch from 1a6b2ee to 227f81e Compare September 6, 2022 15:38
@qdm12 qdm12 force-pushed the qdm12/genesis/less-dependencies branch from 227f81e to bcb4229 Compare September 7, 2022 23:44
@qdm12 qdm12 changed the base branch from qdm12/genesis/less-dependencies to qdm12/dot/sync/handle-just September 8, 2022 15:41
@qdm12 qdm12 force-pushed the qdm12/dot/sync/handle-just branch 2 times, most recently from ceb798e to 1d661d6 Compare September 16, 2022 12:29
@qdm12 qdm12 force-pushed the qdm12/trie/versioned branch 2 times, most recently from bf21a41 to 10a8565 Compare September 19, 2022 16:00
qdm12 added a commit that referenced this pull request Sep 20, 2022
@qdm12
chore(lib/genesis): less dependencies on other packages (#2795)
1e38c35 
- Remove all utils/test helping global variables from `lib/genesis` and inline them where needed
- Duplicate test-only code helping functions where they are needed in `helpers_test.go` and unexport them
- Change test helping functions to return genesis, trie and block header as values instead of pointers
- Move lib/genesis's `NewGenesisBlockFromTrie` function as `lib/trie`  `Trie` method `GenesisBlock()`
- Move lib/genesis's `NewTrieFromGenesis` function to `lib/runtime/wasmer` since it will need in #2774 to use wasmer functions
- Move key constants from `lib/runtime/constants.go` to `lib/genesis/constants.go`
@qdm12 qdm12 force-pushed the qdm12/trie/versioned branch 2 times, most recently from a6fe2af to b3412a6 Compare September 20, 2022 15:59
Base automatically changed from qdm12/dot/sync/handle-just to development September 22, 2022 18:15
@qdm12 qdm12 mentioned this pull request Oct 4, 2022
Merged
@qdm12 qdm12 marked this pull request as draft November 2, 2022 17:58
@qdm12 qdm12 mentioned this pull request May 12, 2023
Closed
@dimartiro dimartiro mentioned this pull request May 29, 2023
Closed
27 tasks
@dimartiro
Copy link
Contributor

Closed in favor of #3433

@dimartiro dimartiro closed this Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noot noot Awaiting requested review from noot

@edwardmack edwardmack Awaiting requested review from edwardmack

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

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

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

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

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

Successfully merging this pull request may close these issues.
2 participants
@qdm12 @dimartiro