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] Optimize gov params schema #76

Closed
andrewnguyen22 opened this issue Apr 23, 2022 · 5 comments · Fixed by #139
Closed

[Persistence] Optimize gov params schema #76

andrewnguyen22 opened this issue Apr 23, 2022 · 5 comments · Fixed by #139
Assignees
Labels
community Open to or owned by a non-core team member core Core infrastructure - protocol related persistence Persistence specific changes

Comments

@andrewnguyen22
Copy link
Contributor

Optimize params schema for better de-duplication of data while maintaining fast lookups

NOTE: The fundamental issue with the current 'naive' design is that it recreates the entire parameters list everytime we have an edit to a single parameter. However, it is also important to note, parameters are expected to be updated at a low frequency.

Per a suggestion from @iajrz offline, this schema was suggested

param values table
|------------+------------|
| field name | field type |
|------------+------------|
| height     | bigint     |
| param_id   | int        |
| value      | string     |
|------------+------------|


params table
|------------+------------|
| field name | field type |
|------------+------------|
| id         | int        |
| value_type | int        |
| param_name | string     |
|------------+------------|

My modification to it would be:


CREATE TYPE val_type AS ENUM ('STRING', 'BIGINT', 'SMALLINT');

CREATE TABLE params (
  name    string     PRIMARY_KEY,
  height  bigint     PRIMARY_KEY,
  enabled bool      NOT NULL,
  type    val_type  NOT NULL,
  value   string     NOT NULL,
)

CREATE TABLE flags (
  name    string     PRIMARY_KEY,
  height  bigint     PRIMARY_KEY,
  enabled bool      NOT NULL,
  type    val_type  NOT NULL,
  value   string     NOT NULL,
)

We can have very similar tables for flags and params and not how the primary key is a composite key of the name of the flag/param and the height, along with an enabled/disabled bool, so we can automatically (for example) in one snapshot of the state, predefined the behaviour of a flag only being applied for some number of blocks.

@Olshansk
Copy link
Member

Olshansk commented Apr 24, 2022

Worth noting that this issue is the result of an offline discussion in order to submit #73 in a shorter time period.

We also need to decide if we should link it to the V1 Prototype milestone or not...

@Olshansk Olshansk added the persistence Persistence specific changes label Apr 24, 2022
@Olshansk Olshansk added the core Core infrastructure - protocol related label May 31, 2022
@Olshansk Olshansk changed the title Optimize gov params schema [Persistence] Optimize gov params schema Jun 6, 2022
@Olshansk Olshansk added this to the V1 Persistence Foundation milestone Jul 6, 2022
@deblasis
Copy link
Contributor

Hi there guys! I will be looking into this one if you don't mind 🙂

I just had a glance at it and I have a couple of questions to make sure I understand:

  1. What is the difference between param and flag? I am assuming it's an optimization around the datatype? If that's the case
    The schemas for params and flags look identical, including their PKs. Perhaps flags should be something like:
CREATE TABLE flags (
  name    string     PRIMARY_KEY,
  height  bigint     PRIMARY_KEY,
  enabled bool      NOT NULL,
  value     bool      NOT NULL,
)

instead?

  1. I cannot find any instance of a "flag" in the codebase, can you please point me to one example so that I can understand and potentially write the corresponding test logic?
  2. Are you OK with me using generics for the various GetXParam and SetParam? I just saw a TODO in there :)

Lastly, these are non-blocking questions and I might come up with more questions as I learn more about the codebase but I can definitely get started in the meantime 🚀

@Olshansk
Copy link
Member

Olshansk commented Jul 30, 2022

@deblasis tl;dr Appreciate the help and want to start the discussion but would hold off on implementing this one just yet...


Having created and labelled this as a starter task, I'll take the heat that I may have prematurely left it out for the community to pick up. Since the project is young, and this is a great ticket to get a feel for the whole codebase, there are still a handful of ongoing changes (primarily being implemented by @andrewnguyen22 and myself in #105 and #113).

I'm afraid the conflicts may be so great right now that is might be worth holding off for a couple of weeks to avoid too many conflicts. However, if we can have several small PRs that don't affect the whole codebase at once, we may be able to start the work here as well.

IMO #112 is another highly impactful ticket our team will need almost immediately and can be done while we create space or get started on this one.


To answer your question, though, since we can start thinking/designing now and tackle it in pieces...

  1. A param is a "chain level" value that can be changed by the DAO (e.g. fees). A flag is also something that can be voted on by the DAO but is used to enable / disable features rather than be active parameters impacting the state machine. E.g. a flag could enable / disable fees at a certain height, where the fee is set based on the parameter.

I like your suggestion about the schema for flag but am still thinking if value should be a floating point type. Not a blocker to go with bool for now though.

  1. I don't think we have any yet. Like I said, it's still early :). I could unit tests would be nice but no need to go overboard here for now.

  2. I wholeheartedly support using generics for this and is something I would ❤️

Lastly, these are non-blocking questions and I might come up with more questions as I learn more about the codebase but I can definitely get started in the meantime

For sure. In fact, I'd point you to our public #v1-dev channel, and we can collaborate there. We can also hop on an ad-hoc call there with other core team members so don't feel like you need to wait for an official V1 contributors hour.

@deblasis
Copy link
Contributor

deblasis commented Aug 1, 2022

Hi @Olshansk!

no problem, it makes sense. I am going to have a look at the issues you mentioned in order to have more context (pun intended) and I'll keep an eye on #v1-dev on DIscord.

Any conflicts I am happy to resolve them when the time comes but I agree that they should be minimized especially if big changes are expected in the short run.
When I saw your comment I had already started working on this so I created the draft PR #139.

This way you guys can have a preliminary look if you want and we can adjust as we go. For example, the flags are not yet fully implemented in there.

@Olshansk
Copy link
Member

Olshansk commented Aug 1, 2022

The PR looks awesome. Thank you so much for your contribution!

Let's keep the conversation/discussion there, but I think we should just move forward with it because it's a great change to the project

@Olshansk Olshansk added the community Open to or owned by a non-core team member label Aug 3, 2022
Olshansk pushed a commit that referenced this issue Aug 17, 2022
## Description

This PR changes the schema for `params`, adds `flags` (to be implemented) and aims at improving the way we update the values.

It should also improve maintainability by:
- centralizing Gov schema related metadata into the protobuf by attaching metadata to it. (see gov.proto)
- using generics for get/set methods

Fixes [issue/76](#76)

## Type of change
Please mark the options that are relevant.

- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Documentation
- [ ] Other (<_REPLACE_ME_WITH_DETAILS_>)

## How Has This Been Tested?
_[Not needed for integration PR]_

_Describe the tests and that you ran to verify your changes. If applicable, provide steps to reproduce. Bonus points for images and videos or gifs._

- [x] `make test_all`
- [x] [LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md)

## Checklist
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have tested my changes using the available tooling
- [ ] If applicable, I have made corresponding changes to related or global README
- [ ] If applicable, I have added new diagrams using [mermaid.js](https://mermaid-js.github.io)
- [x] If applicable, I have added tests that prove my fix is effective or that my feature works
- [x] I have documented `flags` and their differences with `params` as per [comment](#139 (comment))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Open to or owned by a non-core team member core Core infrastructure - protocol related persistence Persistence specific changes
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants