[Persistence] Optimize gov params schema (Issue #76)

[deblasis]

Description

Currently WIP and paused because of #76 (comment)

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

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)
• 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.

• make test_all
• LocalNet

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
• If applicable, I have made corresponding changes to related or global README
• If applicable, I have added added new diagrams using mermaid.js
• If applicable, I have added tests that prove my fix is effective or that my feature works
• I have documented flags and their differences with params as per comment

[Olshansk]

I did a quick first review (will do an in-depth one next time) but this is frikkin awesome!

I think we should continue moving forward with it and the changes are well contained enough that I don't think the conflicts with deprecating persistence will be that bad. CC @andrewnguyen22

[deblasis]

Thanks @Olshansk! I have caught up with main and updated a few things but I will dive deeper and also test the parts I didn't test yet (Get/Set) after your next in-depth look and which should give me more clarity around how to use enabled and snapshotting 🙂.

[Olshansk]

Left additional comments throughout the PR (some impacting the rest of the DB).

At a high-level, the summary of the architectural changes is:

• shared/modules/persistence_module.go needs to be modified to accept a height for each Get call - Eventually I want to simplify that too, but we can do that later
• Remove the nullify query altogether - it's legacy and we can add it back later
• Params should now have an enabled field but flags should
• Flags are not used in the database yet, but we already use it for v0 and I think most of the foundation we need for it in v1 is already in this PR.

Let me know if you need any more details and we can jump on a call!

[Olshansk]

tl;dr Almost . Please hold off on squashing and merging

I've re-reviewed this whole PR and just left a couple of minor final NITs.

Overall, I really like the change and it'll make it much easier to both add and extend on our param+flag logic.

Going to approve this change but please hold off on @andrewnguyen22's review before submitting. After he take a look, we'll also need to make a call on whether we want to submit this before or after #140 as there will definitely be conflicts, but I'm sure we can figure it out.

[Olshansk]

Unapprove to avoid accidental merge.

[deblasis]

Unapprove to avoid accidental merge.

This gave me the inspiration needed for this #160 and consequently #161

[Olshansk]

@andrewnguyen22 In terms of ordering / priority, what are your thoughts on

1. [Persistence] PrePersistence deprecation & Block lifecycle improvements (Issue #128 && Issue #105) #140
2. This PR
3. [Persistence] Followup on PrePersistence deprecation & Block lifecycle  #165
4. [Configuration] Genesis and Node Configurations (Issue#154) #166

That way we disperse the burden of merge conflicts amongst every

[andrewnguyen22]

I think that makes sense.

But let's talk offline per #128

EDIT: The one blocker I see here are that I'm not sure 'flags' were ever documented as asked in here

[Olshansk]

@deblasis We're likely going to merge #140 soon, so just a heads up that it'll require a fair amount of conflict resolution.

Otherwise, the only blocker here after synching with @andrewnguyen22 is adding some documentation explaining the different between flags and params. There is an FAQ section under persistence/README.md, so I think that'd be a good spot to add it.

[deblasis]

@deblasis We're likely going to merge #140 soon, so just a heads up that it'll require a fair amount of conflict resolution.

ACK. No problem, I subscribed to that issue so I have been quietly stalking you guys and I saw this coming soon my way 🙂

Otherwise, the only blocker here after synching with @andrewnguyen22 is adding some documentation explaining the different between flags and params. There is an FAQ section under persistence/README.md, so I think that'd be a good spot to add it.

Cool, will do, thanks @Olshansk !
(updated PR checklist)

[deblasis]

I noticed that the sentence here is truncated :)

[Olshansk]

Hahaha, I think open to suggestions is what we wanted to say 😅

[deblasis]

@andrewnguyen22: I have added some color here about flags/params.

Everything (very little 😅) I know about flags comes from this comment #76 (comment) and subsequent conversations with @Olshansk in the form of comments to this PR.

One thing I assumed initially, that I later changed to the current code, was that flags shouldn't have a value like params but just a boolean flag. Maybe the truth is a bit of both: we should remove the enabled in flags and create logic around the value that we can define as float to handle multivariate flags later on, perhaps this would be out of scope for this PR but I am happy to change things if needed.

As things stand, these descriptions should fit, even though we are not yet using flags anywhere so I am assuming that this is very much subject to change as we iterate regardless.

[Olshansk]

Thanks for the extra context. I think the flag concept will definitely evolve but the current description and implementation is more than sufficient to move forward.

[Olshansk]

LGTM! Let's

I removed the do not merge, retriggered the jobs and am approving.

@deblasis What we've been doing is doing a squash-and-merge and copy-pasting the github description into the commit.

I looked at the two issues in the jobs, and they are related to our docker images and a test that's flaky in our consensus algorithm (related to the pacemaker). Will sync with @okdas on this but it doesn't block this PR.

[andrewnguyen22]

Why is the automation failing on this? @Olshansk

[Olshansk]

Same reasons as last night:

1. Flaky consensus test related to the pacemaker where I use timeouts. AI on @Olshansk to fix it
2. Bad credentials to log into dockerhub to build the image. AI on @okdas.

Attaching screenshots from the link above for context. I don't think either are blockers.