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] TxIndexer to StateHash Bridge #315

Closed
12 tasks
Olshansk opened this issue Oct 20, 2022 · 0 comments · Fixed by #332
Closed
12 tasks

[Persistence] TxIndexer to StateHash Bridge #315

Olshansk opened this issue Oct 20, 2022 · 0 comments · Fixed by #332
Assignees
Labels
consensus Consensus specific changes core Core infrastructure - protocol related persistence Persistence specific changes utility Utility specific changes

Comments

@Olshansk
Copy link
Member

Olshansk commented Oct 20, 2022

Objective

The objective of this issue is to "bridge" the work being done in #302 (tx indexer integration) and #285 (first state hash implementation.

Origin Document

The interface and diagrams for the state hash computation were being done in #252 while the first implementation began in #285. However, since both of these are dependent on the integration of the tx indexer in #302, some conflicting design decisions arose.

The goal of this ticket is to unblock #302, while also capturing the results of an offline discussion that led to a decision where the "ephemeral block state" should be wholly managed by the persistence context, being a single, "roll-backable" point of reference.

Goals

  • Remove ephemeral block-related state from the consensus module & utility module, and only maintain it in the persistence module
  • Simply outward-facing module interfaces to only rely on primitive types
  • Enable committing of a block to persistence by simply calling Commit(quorumCert); see [Core] StateHash Interfaces & Diagrams #252 for more details

After #302, #285 and this ticket is done, we should have a design that more closely resembles the following

  • P2P - Wakes up consensus module (pipe for consensus module)
  • Consensus - State Machine Orchestration
  • Utility module - State Machine
  • Postgres Context - Maintains state

Deliverable

A PR that achieves the goals and includes, but not limited to, the following changes:

  • Remove lastAppHash from the consensusModule struct state & access it from the persistence layer (added n the past)
  • Remove validatorMap from the consensusModule struct state & access it from the persistence layer (added n the past)
  • Remove TxResult from the utility module interface (added in [TxIndexer] Integration of transaction indexer (issue-#168) #302)
  • Encapsulate TxIndexer logic only inside the persistence context

Non-goals / Non-deliverables

  • Implementation of the state commitment (i.e. state hash)

General issue deliverables

  • Update the appropriate CHANGELOG
  • Update any relevant READMEs (local and/or global)
  • Update any relevant global documentation & references
  • If applicable, update the source code tree explanation
  • If applicable, add or update a state, sequence or flowchart diagram using mermaid

Testing Methodology

  • Add new unit tests if applicable
  • All tests: make develop_test
  • LocalNet: verify a LocalNet is still functioning correctly by following the instructions at docs/development/README.md

Creator: @Olshansk
Co-Owners: @andrewnguyen22

@Olshansk Olshansk added core Core infrastructure - protocol related consensus Consensus specific changes utility Utility specific changes persistence Persistence specific changes labels Oct 20, 2022
andrewnguyen22 pushed a commit that referenced this issue Nov 1, 2022
andrewnguyen22 pushed a commit that referenced this issue Nov 3, 2022
andrewnguyen22 pushed a commit that referenced this issue Nov 4, 2022
## Description

**TL;DR Encapsulate and commit the block and block parts within a persistence context**

The objective of this issue is to "bridge" the work being done in #302 (tx indexer integration) and #285 (first state hash implementation.

The interface and diagrams for the state hash computation were being done in #252 while the first implementation began in #285. However, since both of these are dependent on the integration of the tx indexer in #302, some conflicting design decisions arose.

The goal of this ticket is to unblock #302, while also capturing the results of an offline discussion that led to a decision where the "ephemeral block state" should be wholly managed by the persistence context, being a single, "roll-backable" point of reference

- [x]  **Enable committing of a block to persistence by simply committing the persistence context**
- [x] Remove ephemeral block-related state from the consensus module & utility module, and only maintain it in the persistence module
- [x]  Simply outward-facing module interfaces to only rely on primitive types
- [ ] Remove validatorMap from the consensusModule struct state & access it from the persistence layer (added n the past) 

Opened new issue for last one to prevent scope from getting out of hand: #331

## Issue

Fixes #315 

## Type of change

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [x] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

<!-- List out all the changes made-->
- Removed `apphash` and `txResults` from `consensus Module` structure
- Modified lifecycle to `set` the proposal block within a Persistence Context 
- Allow block and parts to be committed with the persistence context
- Ported over storing blocks and block parts to persistence module from Consensus and Utility
- Encapsulate TxIndexer logic only inside the persistence context
- Remove TxResult from the utility module interface (added in [TxIndexer] Integration of transaction indexer (issue-#168) #302)
- Combined creation and application of block in proposer lifecycle

## Testing

- [x] `make develop_test`
- [x] [LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md) w/ all of the steps outlined in the `README`

## Required 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] 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](https://mermaid-js.github.io) diagrams in the corresponding README(s)
- [ ] I have added, or updated, documentation and [mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*` if I updated `shared/*`README(s)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus Consensus specific changes core Core infrastructure - protocol related persistence Persistence specific changes utility Utility specific changes
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants