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

[Persistence] Add logic for updateParamsTree and updateFlagsTree functions for the Sparse Merkle Tree #453

Merged
merged 19 commits into from
Jan 27, 2023

Conversation

h5law
Copy link
Contributor

@h5law h5law commented Jan 20, 2023

Description

This PR is in response to the comments left in the peristence/state.go file:

image

The functions work by utilising the new getParamsUpdated(height int64) and getFlagsUpdated(height int64) functions that return a slice of []*coreTypes.Param and []*coreTypes.Flag respectively. The function runs a query against the PostgreSQL DB to get all the parameter's/flag's at the given height, their names, values, and heights (+enabled status for flags) are then loaded from the returned rows into individual Param or Flag protobufs and these are then returned.

image

The updateParamsTree and updateFlagsTree functions then iterate over each protobuf in this slice and for each one runs the SMT Update function using the result of crypto.SHA3Hash([]byte(proto.String())) function (where proto is either param or flag depending on the function) as the key, and the marshalled protobuf as a []byte as the value.

The corresponding hashes have been updated in the TestStateHash_DeterministicStateWhenUpdatingAppStake test, following the comment at the start of the function.

Issue

Fixes N/A

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other

List of changes

  • Create Param and Flag protobufs
  • Create the getParamsOrFlagsAtHeightQuery() and getParamsUpdated() and getFlagsUpdated() functions to return the a slice of the respective protobufs for the given height
  • Define the logic in the updateParamsTree() and updateFlagsTree() functions to utilise these functions and update their respective Merkle Trees
  • Update the TestStateHash_DeterministicStateWhenUpdatingAppStake test hardcoded hashes to use new ones as the business logic has changed for calculating these
  • Replace all uses of proto.Marshal with codec.GetCodec().Marshal() in persistence/state.go

Testing

  • make develop_test
  • LocalNet w/ all of the steps outlined in the README

Required Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have tested my changes using the available tooling
  • I have updated the corresponding CHANGELOG

If Applicable Checklist

  • I have updated the corresponding README(s); local and/or global
  • I have added tests that prove my fix is effective or that my feature works
  • I have added, or updated, mermaid.js diagrams in the corresponding README(s)
  • I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

@h5law h5law added core Core infrastructure - protocol related persistence Persistence specific changes labels Jan 20, 2023
@h5law h5law self-assigned this Jan 20, 2023
@h5law h5law added this to the M1: Pocket PoS (Proof of Stake) milestone Jan 20, 2023
@h5law h5law requested a review from Olshansk January 20, 2023 14:57
@h5law
Copy link
Contributor Author

h5law commented Jan 20, 2023

Something I was concerned about is whether the ParamOrFlag struct definition is in an appropriate file? I thought peristence/types/gov.go as this is where other Parameter/Flag stuff is located.

Also whether the method for generating the keys/values to actually update the trees is alright? I chose this method as the other functions have types already so defining the struct and its Hash() and GetBytes() method seemed to follow a good pattern at least, but the value string I am using (before converting to []byte) was really out of nowhere due to this type being explicitly made for this purpose.

@h5law h5law changed the title [Persistence] Add logic for updateParamsTree and updateFlagsTree functions for the Sparse Merkle Tree [WIP][Persistence] Add logic for updateParamsTree and updateFlagsTree functions for the Sparse Merkle Tree Jan 20, 2023
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Definitely heading in the right direction and most of the logic is in place. Left a couple of major regarding next steps.

Let me know if you need any more clarifying details

persistence/account_shared_sql.go Outdated Show resolved Hide resolved
persistence/state.go Outdated Show resolved Hide resolved
persistence/types/gov.go Outdated Show resolved Hide resolved
@h5law h5law changed the title [WIP][Persistence] Add logic for updateParamsTree and updateFlagsTree functions for the Sparse Merkle Tree [Persistence] Add logic for updateParamsTree and updateFlagsTree functions for the Sparse Merkle Tree Jan 25, 2023
@h5law h5law marked this pull request as ready for review January 25, 2023 19:36
@h5law h5law requested a review from Olshansk January 25, 2023 19:36
@h5law h5law changed the title [Persistence] Add logic for updateParamsTree and updateFlagsTree functions for the Sparse Merkle Tree [WIP][Persistence] Add logic for updateParamsTree and updateFlagsTree functions for the Sparse Merkle Tree Jan 25, 2023
@h5law h5law marked this pull request as draft January 25, 2023 20:19
@h5law h5law changed the title [WIP][Persistence] Add logic for updateParamsTree and updateFlagsTree functions for the Sparse Merkle Tree [Persistence] Add logic for updateParamsTree and updateFlagsTree functions for the Sparse Merkle Tree Jan 25, 2023
@h5law h5law marked this pull request as ready for review January 25, 2023 20:44
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Almost good to go!

shared/core/types/proto/param.proto Outdated Show resolved Hide resolved
persistence/state.go Outdated Show resolved Hide resolved
persistence/state.go Outdated Show resolved Hide resolved
persistence/gov.go Outdated Show resolved Hide resolved
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Do you mind replacing the other proto.Marshal to codec.GetCodec().Marhsal()?

After that, feel free to merge!

@h5law h5law merged commit 884e470 into main Jan 27, 2023
@h5law h5law deleted the smt_update_param_flag_trees branch January 27, 2023 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core infrastructure - protocol related persistence Persistence specific changes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants