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

First iteration of a PostgreSQL based Persistence Schema #73

Merged

Conversation

andrewnguyen22
Copy link
Contributor

@andrewnguyen22 andrewnguyen22 commented Apr 18, 2022

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.

@andrewnguyen22 andrewnguyen22 self-assigned this Apr 18, 2022
Olshansk and others added 4 commits April 18, 2022 15:53
- Start a postgres db with `compose_and_watch` if it hasn't already started
- Connect to the local DB with `make connect_db`
- Insert a random user ID into the node's specific schema every time it starts up
- View schemas with `SELECT schema_name FROM information_schema.schemata;`
- View random user IDs with `SELECT * from node1.users;`
@andrewnguyen22 andrewnguyen22 force-pushed the issue68/local_postgres branch from 0083ef9 to 1cc647e Compare April 18, 2022 19:55
persistence/module.go Outdated Show resolved Hide resolved
persistence/sql.go Outdated Show resolved Hide resolved
persistence/sql.go Outdated Show resolved Hide resolved
persistence/sql.go Outdated Show resolved Hide resolved
@andrewnguyen22 andrewnguyen22 force-pushed the issue68/local_postgres branch from d197174 to 8cad2df Compare April 21, 2022 23:42
@andrewnguyen22 andrewnguyen22 force-pushed the issue68/local_postgres branch from 8cad2df to 2bec340 Compare April 21, 2022 23:45
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.

Left a few non-trivial comments, so let's discuss tomorrow.

persistence/sql.go Outdated Show resolved Hide resolved
persistence/schema/gov.go Outdated Show resolved Hide resolved
persistence/schema/fisherman.go 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.

My comments might require an offline discussion since it is a large change to what was proposed, so lmk.

Otherwise:

  1. Make sure to update the PR name & description since we'll probably end up submitting this all at once.

  2. Some of the comments regarding the schema may be redundant, so feel free to resolve them if they were addressed elsewhere.

  3. After reading https://github.com/pokt-network/pocket-network-protocol, my understanding is that the postgres state only represents the state of the blockchain at the height it's currently synched to. We don't need to store history in the psotgres DB (which will really bloat it over time), because that's guaranteed by the stateHash. In conjunction with the created_at and deleted_at fields, along with the stateHash stored in each block, we can replay transactions and recreate a small & efficient postgres DB at every height.

THis way, we can also use the address as a primary key, avoid two tables when one is sufficient, and overall keep it much simpler.

build/deployments/docker-compose.yaml Outdated Show resolved Hide resolved
persistence/schema/account.go Outdated Show resolved Hide resolved
persistence/schema/account.go Outdated Show resolved Hide resolved
persistence/schema/account.go Outdated Show resolved Hide resolved
persistence/schema/account.go Show resolved Hide resolved
persistence/schema/fisherman.go Show resolved Hide resolved
persistence/schema/fisherman.go Outdated Show resolved Hide resolved
persistence/schema/fisherman.go Outdated Show resolved Hide resolved
persistence/schema/validator.go Outdated Show resolved Hide resolved
persistence/schema/service_node.go Outdated Show resolved Hide resolved
Olshansk and others added 11 commits April 22, 2022 22:53
- Start a postgres db with `compose_and_watch` if it hasn't already started
- Connect to the local DB with `make connect_db`
- Insert a random user ID into the node's specific schema every time it starts up
- View schemas with `SELECT schema_name FROM information_schema.schemata;`
- View random user IDs with `SELECT * from node1.users;`
- Moved schema over to configs
- Added PGADmin
- Working on migrations
This PR is meant to cleanup documentation related to the V1 prototype in order to close out the [V1 Milestone Protoype](https://github.com/pokt-network/pocket/milestone/1). It fixes a few minor bugs, but also refactors and updates the documentation so it is easier to follow.

**Fixes issues**
* #21
* #20

**Other fixes**
This change also fixes some issues (protobuf, configuration, etc) that were introduced in the last stages of the integration phase. Anyone cloning the repository anew should be able to run a localnet by following the instructions.

Please mark the options that are relevant.

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [x] This change requires a documentation update

This change was tested by following the new instructions added to `docs/development/README.md`.

- [x] My code follows the style guidelines of this project
- [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 made corresponding changes to the documentation
- [ ] No new warnings are generated by my changes
- [X] I have added tests that prove my fix is effective or that my feature works
- [X] New and existing unit tests pass locally with my changes
- [X] Any dependent changes have been merged and published in downstream modules
@Olshansk Olshansk force-pushed the issue68/local_postgres branch from c090257 to 2471607 Compare April 23, 2022 05:55
@Olshansk Olshansk changed the base branch from milestone/v1-prototype to issue68/base_postgres_infa April 23, 2022 06:02
@Olshansk Olshansk changed the title Issue68/local postgres Base Persistence Schema Apr 23, 2022
@Olshansk Olshansk self-assigned this Jul 1, 2022
@Olshansk Olshansk removed the request for review from derrandz July 1, 2022 20:58
@Olshansk Olshansk added core Core infrastructure - protocol related persistence Persistence specific changes labels Jul 1, 2022
@Olshansk Olshansk marked this pull request as ready for review July 1, 2022 20:58
@Olshansk Olshansk requested a review from a team as a code owner July 1, 2022 20:58
@Olshansk
Copy link
Member

Olshansk commented Jul 1, 2022

Next steps for this PR are:

  1. @andrewnguyen22 re-reviews all the recently pushed changes and makes the appropriate changes
  2. @Olshansk updates the changelog, persistence/README and GitHub PR description
  3. @luyzdeleon reviews the PR (possibly after merging) to track follow-up work
  4. Follow-up Github issues are created for the team & community based on the TODOs in the README

@andrewnguyen22
Copy link
Contributor Author

Pushed the changes and approve

@Olshansk Olshansk requested review from derrandz and removed request for derrandz and luyzdeleon July 5, 2022 22:07
@Olshansk Olshansk linked an issue Jul 5, 2022 that may be closed by this pull request
@Olshansk
Copy link
Member

Olshansk commented Jul 6, 2022

@andrewnguyen22 I updated the README, github description and changelog.

The only change I wanted to bring to your attention is commit 1b17b0c which I had to do to fix some tests. PTAL.

@derrandz This review is blocked since you requested changes to please review and/or approve when you have a chance.

Copy link
Contributor

@derrandz derrandz left a comment

Choose a reason for hiding this comment

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

I have not reviewed, I am approving due to my last changes request to unblock merging, but this looked really good in the protocol hour, so keep it coming guys.

@Olshansk Olshansk changed the base branch from main to 104-persistence-v1-persistence-foundation July 6, 2022 23:48
@Olshansk Olshansk added this to the V1 Persistence Foundation milestone Jul 6, 2022
@Olshansk Olshansk merged commit fb3a6fc into 104-persistence-v1-persistence-foundation Jul 6, 2022
@Olshansk Olshansk deleted the issue68/local_postgres branch July 6, 2022 23:49
andrewnguyen22 pushed a commit that referenced this pull request Jul 30, 2022
# Objective
Reduce the code footprint of the utility module, so it is more understandable to new readers while finding a balance between minimizing legacy complexity in the future and not over-generalizing the existing code.

# Origin Document

PR73 ([First iteration of a PostgreSQL based Persistence Schema](#73)) underwent several iterations in order to make the [persistence module implementation](https://github.com/pokt-network/pocket/tree/main/persistence) have the optionality to remain verbose on a per-actor basis but also avoid code duplication and redundancy.

The first iteration of the [utility module](https://github.com/pokt-network/pocket/tree/main/persistence) did not undergo the same iterations and therefore has some low-hanging fruit to simplify the codebase before further functionality is added.

# Goals / Deliverables
- [x] Extract common actor functionality into general interfaces
- [x] Reduce the code complexity and code footprint so it is more approachable and maintainable
- [ ] Identify and document/cleanup parts of the code that are unclear to new readers
- [x] Generalize both unit tests and functionality where possible
- [x] Maintain verbose entry points for each actor
General:
- [x] Update the CHANGELOG
- [x] Update the README
- [x] Update the global documentation & references
- [x] Document small issues / TODOs along the way

## Testing Methodology
 - **Utility tests**: `make test_utility_*`
 - **All tests**: `make test_all`
 - **LocalNet**: works following the instructions in [docs/development](https://github.com/pokt-network/pocket/tree/main/docs/development)

## Non-goals
- Adding any new functionality

---

## Changes Made
- Removed transaction fees from the transaction structure as fees will be enforced at the state level
- Removed actor specific messages (besides DoubleSign) and added actorType field to the struct
- Removed pause messages and functionality as it is out of scope for the current POS iteration
- Removed session and test-scoring as it's out of scope for the current POS iteration
- Consolidated unit test functionality for actors
- Modified pre-persistence to match persistence for Update(actor), 'amountToAdd' is now just 'amount'

## Future work
- There's more documentation that is needed for this to be complete. In a separate issue, I'd like to tackle connecting Utility with Persistence.
- Further cleanup / refactoring / optimizations can be made, but that can be left to future iteration of the module.

---

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

[Persistence] Module First Implementation
4 participants