[Persistence] Followup on PrePersistence deprecation & Block lifecycle #165

Olshansk · 2022-08-14T22:24:38Z

Objective

Follow up on both critical and non-critical fixes to #140.

Origin Document

#140 deprecated PrePersistence and implemented multiple contexts. This PR is a follow with some stylistic changes, improvements in the readability of the tests, the addition of unit tests, bug fixes in some SQL business logic, and implementation of the read context.

Followup work is being tracked in #149.

Learnings to streamline workflow in the future

Do not conflate non-critical stylistic changes as it leads to more conflicts down the road
Implemented an automated "LocalNet" for easier testing
Try to plan work ahead of time so we can work in smaller PRs / iterations

Changelog / changes

Main persistence module changes:

Split ConnectAndInitializeDatabase into connectToDatabase and initializeDatabase
- This enables creating multiple contexts in parallel without re-initializing the DB connection
Fix the SQL query used in SelectActors, SelectAccounts & SelectPools
- Add a generalized unit test for all actors
Remove NewPersistenceModule and an injected Config + Create
- This improves isolation a a “injection-like” paradigm for unit testing
Change SetupPostgresDocker to SetupPostgresDockerPersistenceMod
- This enables more “functional” like testing by returning a persistence module and avoiding global testing variables
- Only return once a connection to the DB has been initialized reducing the likelihood of test race conditions
Implemented NewReadContext with a proper read-only context

Secondary persistence module changes

Improve return values in Commit and Release (return error, add logging, etc…)
Add pgx.Conn pointer to PostgresDB
s/db/conn/g and s/conn/tx/g in some (not all) places where appropriate
Make some exported variables / functions unexported for readability & access purposes
Add a few helpers for persistence related unit testing
Added unit tests and TODOs for handling multiple read/write contexts

General milestone checklist

Update all the relevant CHANGELOGs
Update all the relevant READMEs
Update the source code tree explanation
Add or update a state, sequence or flowchart diagram using mermaid
Create followup milestones + issues
Document small TODO along the way

Follow-up Work

All follow-up work is being tracked in #149

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 memaid.js
If applicable, I have added tests that prove my fix is effective or that my feature works

#73) ## Objective Foundational iteration of PostgreSQL based persistence module implementation. ## Origin Document #68 ## Type of change New major module implementation. ### Persistence-related core changes: - List of actors / interfaces with MVP implementation: - Applications - Fisherman - ServiceNode - Accounts - Pools - List of actors / interfaces with partial MVP implementation: - Validator - Gov params - List of actors / interfaces with minorimplementation: - Block - SQL Schema definition of the actors above - SQL Query implementation for common actor persistence functionality - PostgresContext implementation of the actors actors above - Base infrastructure for fuzz testing Non-persistence “fly-by” changes - Updates to the PrePersistence module and utility module with breaking changes - A few minor improvements/additions to the Makefile - TODOs & comment cleanups throughout the codebase ## How Has This Been Tested? ### Unit Tests ``` make test_persistence make test_all ``` ### LocalNet Ran a basic LocalNet following the instructions in the [development README](docs/development/README.md). Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com> Co-authored-by: Andrew Nguyen <andrewnguyen@Andrews-MacBook-Pro-2.local> Co-authored-by: Andrew Nguyen <andrewnguyen@Andrews-MBP-2.lan> Co-authored-by: Daniel Olshansky <olshansky@pokt.network>

…tence-foundation-interim-merge' into issue-#111

… to read / write separation

… failing

Olshansk · 2022-08-17T14:37:08Z

@andrewnguyen22 Please take a look at this PR and lmk if there any additional changes you'd like for me to make.

andrewnguyen22

All looks good. 99% there. Just two comments

persistence/test/module_test.go

persistence/gov.go

Olshansk · 2022-08-17T19:08:50Z

@andrewnguyen22 Please take a close look at the commits between HEAD and f9ddc0954de45f09559d7812de6e8b8857e9dc2d. All the unit tests are passing but I also had to make several block lifecycle modifications for LocalNet to succeed.

andrewnguyen22 · 2022-08-17T23:24:35Z

Utility context not nil when preparing a new block? Releasing for now but should not happen

consensus/module.go

persistence/genesis.go

persistence/gov.go

persistence/module.go

shared/modules/persistence_module.go

Olshansk · 2022-08-18T10:53:15Z

EDIT: I'm thinking that even though we're going to remove it in https://github.com/pokt-network/pocket/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc, I'll add back the previous block lifecycle / should hydrate genesis logic so we have a more stable main with less bugs.

@andrewnguyen22 comment regarding the genesis / lifecycle piece.

When we added the blockstore in #121 , we also added a shouldHydrateGenesis function so that:

We don't run populateGenesis if there is already a previous state
Pick up the previous state if we already had one (hence the logic around loading the previous hash and height which I'm just logging)

https://github.com/pokt-network/pocket/pull/140/files?file-filters%5B%5D=.go&file-filters%5B%5D=.json&file-filters%5B%5D=.md&file-filters%5B%5D=.mod&file-filters%5B%5D=.sum&file-filters%5B%5D=No+extension&show-viewed-files=true&show-deleted-files=false

My understanding is that per the other comments / discussions (screenshot below), we didn't want to bring it back in favor of the future configs changes happening, but I had to add a couple stop-gap hacks in the meantime so that we load up the genesis correctly every time.

I'd personally prefer to add it back in this PR so there are less hacks / confusion, but also unsure per you your other comments. Lmk how you think I should proceed.

Olshansk · 2022-08-18T14:39:45Z

@andrewnguyen22 I removed the hacks, added the TODOs and replied to some of the outstanding comments.

Even though we'll make it more real in the next config change and delete some of this, it'll avoid us having more hacks, and a better baseline we can reference or revert to on mainline.

I added a small gif showing the state being loaded when we restart local net and some of the tooling we can use to investigate it manually.

The quality is low because github messages is limited to 10MB but a full video is available here.

#### Intention Currently merge ready with #128 but not considering #165 since it's under active development. ## Description - Deprecated old placeholder genesis_state and genesis_config - Added utility_genesis_state to genesis_state - Added consensus_genesis_state to genesis_state - Added genesis_time to consensus_genesis_state - Added chainID to consensus_genesis_state - Added max_block_bytes to consensus_genesis_state - Added accounts and pools to utility_genesis_state - Added validators to utility_genesis_state - Added applications to utility_genesis_state - Added service_nodes to utility_genesis_state - Added fishermen to utility_genesis_state - Deprecated shared/config/ - Added new shared config proto3 structure - Added base_config to config - Added utility_config to config - Added consensus_config to config - Added persistence_config to config - Added p2p_config to config - Added telemetry_config to config - Opened followup issue #163 - Added config and genesis generator to build package - Deprecated old build files - Use new config and genesis files for make compose_and_watch - Use new config and genesis files for make client_start && make client_connect closes #154 ## Type of change Please mark the options that are relevant. - [x] 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? make test_all && make compose_and_watch - [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 - [x] If applicable, I have made corresponding changes to related or global README - [ ] If applicable, I have added added new diagrams using [mermaid.js](https://mermaid-js.github.io) - [ ] If applicable, I have added tests that prove my fix is effective or that my feature works --- Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com> Co-authored-by: Andrew Nguyen <andrewnguyen@Andrews-MacBook-Pro-2.local> Co-authored-by: Andrew Nguyen <andrewnguyen@Andrews-MBP-2.lan> Co-authored-by: Daniel Olshansky <olshansky@pokt.network>

Andrew Nguyen and others added 30 commits July 6, 2022 16:49

Merge with main and fix tests

199b416

only committing due to merge, wip

58bdbb9

Merge remote-tracking branch 'origin/origin/104-persistence-v1-persis…

444f6b8

…tence-foundation-interim-merge' into issue-#111

issue-#111

61c9178

merged into main

e66cf87

see pull request changes on #111

38684af

using transaction architecture for persistence module, begin to swith…

d7b3be6

… to read / write separation

deprecate pre-persistence

8b3fcdb

merged with main

b05504d

Merge branch 'main' into issue-#128

9bebb69

Remove lingering pre2p files

86bb929

Minor changelog update

9c118a9

Move major var to the top of file

86fa3f5

Minor code cleanup across multiple files

6e6c469

Replaced log.Fatal(fmt.Sprintf with log.Fatalf

4824d26

Minor updates

8e5c20f

requested changes 1 for pull/140

628b993

merge with main

6853095

requested changes 2 for pull/140

d2edd36

Merge remote-tracking branch 'origin/main' into issue-#128

8dbb9e0

pull and resolve

c8e9dc4

Replace staking status numbers with iota

5330794

Added a TODO

3a0b69a

Cleaned up a few tests - but still a WIP

dd8690d

Add a default constant for account amount

c2b4bce

Reviewed persistence/genesis.go

2058f35

Checkpoint commit - removed NewPersistenceModule but some tests still…

362797c

… failing

Updated .gitignore

298d3d5

Updated Makefile - remove old unused targets and reorder existing ones

775ccae

Update todo_in_this_commit make target

4d48983

Olshansk marked this pull request as ready for review August 17, 2022 00:03

Olshansk requested a review from a team as a code owner August 17, 2022 00:03

Olshansk changed the title ~~Issue #128 fixes~~ [Persistence] Followup on PrePersistence deprecation & Block lifecycle Aug 17, 2022

Olshansk requested a review from andrewnguyen22 August 17, 2022 00:17

andrewnguyen22 suggested changes Aug 17, 2022

View reviewed changes

persistence/test/module_test.go Outdated Show resolved Hide resolved

persistence/gov.go Outdated Show resolved Hide resolved

Olshansk added 7 commits August 17, 2022 12:38

Error on multiple write contexts

edb55d0

Remove deprecated tests

44ee0c0

Merge with main

79ad1f2

Followup fixes after merge with main - unit tests still failing

b20e0cd

Use a read context in consensus tests when loading

3da70e3

Fixed the block lifecycle after testing with LocalNet

64a5411

Added hack to clear previous state in genesis state hydration

aed6fed

Olshansk requested a review from andrewnguyen22 August 17, 2022 19:08

Olshansk removed the do not merge Prevent merging even with sufficient approvals label Aug 17, 2022

Populate genesis state in DEBUG_CLEAR_STATE

d44ca6c

andrewnguyen22 suggested changes Aug 17, 2022

View reviewed changes

consensus/module.go Outdated Show resolved Hide resolved

persistence/genesis.go Outdated Show resolved Hide resolved

persistence/gov.go Outdated Show resolved Hide resolved

persistence/module.go Show resolved Hide resolved

shared/modules/persistence_module.go Show resolved Hide resolved

Remove the hack for loading a previous state

63ba621

Olshansk requested a review from andrewnguyen22 August 18, 2022 14:39

Olshansk added 2 commits August 18, 2022 12:15

Update comment

fdaa577

Added a comment explaining refreshUtilityContext

c6b896e

andrewnguyen22 approved these changes Aug 18, 2022

View reviewed changes

Olshansk merged commit 77ea699 into main Aug 18, 2022

Olshansk deleted the issue-#128-fixes branch August 18, 2022 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Persistence] Followup on PrePersistence deprecation & Block lifecycle #165

[Persistence] Followup on PrePersistence deprecation & Block lifecycle #165

Olshansk commented Aug 14, 2022 •

edited

Loading

Olshansk commented Aug 17, 2022

andrewnguyen22 left a comment

Olshansk commented Aug 17, 2022

andrewnguyen22 commented Aug 17, 2022

Olshansk commented Aug 18, 2022 •

edited

Loading

Olshansk commented Aug 18, 2022

[Persistence] Followup on PrePersistence deprecation & Block lifecycle #165

[Persistence] Followup on PrePersistence deprecation & Block lifecycle #165

Conversation

Olshansk commented Aug 14, 2022 • edited Loading

Objective

Origin Document

Learnings to streamline workflow in the future

Changelog / changes

General milestone checklist

Follow-up Work

Checklist

Olshansk commented Aug 17, 2022

andrewnguyen22 left a comment

Choose a reason for hiding this comment

Olshansk commented Aug 17, 2022

andrewnguyen22 commented Aug 17, 2022

Olshansk commented Aug 18, 2022 • edited Loading

Olshansk commented Aug 18, 2022

Olshansk commented Aug 14, 2022 •

edited

Loading

Olshansk commented Aug 18, 2022 •

edited

Loading