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][Utility] Separate all CreateAndApply functions into more functional components #508

Closed
16 tasks
jessicadaugherty opened this issue Feb 14, 2023 · 2 comments · Fixed by #652
Closed
16 tasks
Assignees
Labels
code health Nice to have code improvement consensus Consensus specific changes core Core infrastructure - protocol related persistence Persistence specific changes utility Utility specific changes

Comments

@jessicadaugherty
Copy link
Contributor

jessicadaugherty commented Feb 14, 2023

Objective

Improve the interface exposed by UtilityContext, so it is more functional and has fewer side effects. This will make it clearer how to use it in the context of block preparation, validation and application.

Origin Document

The UtilityContext in shared/modules/utility_module.go partially exposes the following interface:

type UtilityContext interface {
	// ...
	SetProposalBlock(blockHash string, proposerAddr []byte, txs [][]byte) error
	// ...
	CreateAndApplyProposalBlock(proposer []byte, maxTxBytes int) (stateHash string, txs [][]byte, err error)
	// ...
	ApplyBlock() (stateHash string, err error)
	// ...	
}

There are various side effects, as well as implicit behaviour and expectations. For example:

  • SetProposalBlock must be called before ApplyBlock
  • Only the leader should call CreateAndApplyProposalBlock
  • CreateAndApplyProposalBlock modifies the state (not reversible) of the chain even before other validators sign it off

Goals

  • Decouple the business logic of block creation and application
  • Understand the flow of block preparation, validation and application (for the assignee)
  • Make the interface clearer (by improving the interface & documentation) for others to use (e.g. consensus module) and reduce side effects
  • Think through ways of rolling back the changes CreateAndApplyProposalBlock because it does not imply that 2/3 of signatures were received

Deliverable

  • Split CreateAndApplyBlock into CreateBlock and ApplyBlock
  • Reduce the code duplication in block.go between the functions CreateAndApplyProposalBlock and ApplyBlock
  • Add logs to understand to track the process of block creation and application
  • Increase (a bit) test coverage for this functionality
  • Search for all CreateAndApplyProposalBlock or prepareAndApplyBlock instances in the codebase and refactor those parts of the code
    • Update consensus module code
    • Update utility module code
    • Update documentation and end-to-end flows

Non-goals / Non-deliverables

  • Implementing rollbacks

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

  • New tests related to block creation & validation
  • All tests: make test_all
  • LocalNet: verify a LocalNet is still functioning correctly by following the instructions at docs/development/README.md

Creator: @jessicadaugherty
Co-owner: @Olshansk

@jessicadaugherty jessicadaugherty converted this from a draft issue Feb 14, 2023
@jessicadaugherty jessicadaugherty moved this to Backlog in V1 Dashboard Feb 14, 2023
@jessicadaugherty jessicadaugherty added utility Utility specific changes persistence Persistence specific changes code health Nice to have code improvement labels Feb 14, 2023
@Olshansk Olshansk added core Core infrastructure - protocol related consensus Consensus specific changes labels Feb 14, 2023
@deblasis deblasis moved this from Backlog to In Progress in V1 Dashboard Feb 22, 2023
@jessicadaugherty
Copy link
Contributor Author

@deblasis please review and place in the correct iteration (current or next) when you have a chance. Thank you!

@deblasis deblasis moved this from In Progress to Up Next in V1 Dashboard Mar 9, 2023
@deblasis
Copy link
Contributor

deblasis commented Mar 9, 2023

@jessicadaugherty I think this will be handled in the current iteration.

@Olshansk Olshansk moved this from Up Next to In Progress in V1 Dashboard Mar 9, 2023
deblasis added a commit that referenced this issue Mar 26, 2023
…into UtilityUnitOfWork (Issue #563) (#577)

## Description

This PR creates the logical abstraction that would represent the "Unit
Of Work" (inspired by [Martin Fowler's work in his "Patterns of
Enterprise Application
Architecture"](https://martinfowler.com/eaaCatalog/unitOfWork.html))

Essentially what previously was known as `UtilityContext` becomes
`LeaderUtilityUnitOfWork` or `ReplicaUtilityUnitOfWork` depending on the
fact that the current node is the Leader or not (in Consensus).

It touches many files because it moves everything that is interacting
with `Persistence` into the `utility/unit_of_work` folder.
This is to facilitate subsequent refactorings (see related PRs under
#562) into more modular components.

This PR also sets the ground up for the completion of #508 

## Issue

Fixes #563 

## Type of change

Please mark the relevant option(s):

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

## List of changes

- Refactored `UtilityContext` to be a polymorphic `UtilityOfWork`
- Introduced `LeaderUtilityUnitOfWork` and `ReplicaUtilityUnitOfWork`
that expose the methods that are going be to used depending on the
"role" of the node in the consensus process.
- Moved `utilityContext` related code under `utility/unit_of_work` and
renamed all references accordingly

## 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`

<!-- REMOVE this comment block after following the instructions
 If you added additional tests or infrastructure, describe it here.
 Bonus points for images and videos or gifs.
-->

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

- [x] 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)

---------

Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
@jessicadaugherty jessicadaugherty moved this from In Progress to In Review in V1 Dashboard Apr 5, 2023
deblasis added a commit that referenced this issue Apr 7, 2023
…e functional components - Issue #508 (#652)

## Description

This PR tends to the deliverables in the parent issue

## Issue

Fixes #508

## Type of change

Please mark the relevant option(s):

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

## List of changes

#### Consensus
- Updated to reflect the new `ApplyBlock` signature from
`utilityUnitOfWork`
- Updated to use `utilityUnitOfWork.GetStateHash()`
- Removed duplicate import
- Renamed `CreateAndApplyProposalBlock` to `CreateProposalBlock`

#### Shared 
- Renamed `CreateAndApplyProposalBlock` to `CreateProposalBlock`
- Added `GetStateHash` to `UtilityUnitOfWork`

#### Utility
- Renamed `CreateAndApplyProposalBlock` to `CreateProposalBlock`
- Added `GetPrevBlockByzantineValidators` and `ProposalBlockNotSet`
errors
- Instrumented `CreateProposalBlock` and `ApplyBlock` with log
statements
- Refactored functions for block creation and application to be more
readable/modular
- Added TODOs for future refactoring
- Renamed `u` to `uow` for consistency
- Added `stateHash` validation against `proposalBlock`
- Added feature flag for `stateHash` validation (for testing purposes)
- Updated tests accordingly


## Testing

- [x] `make develop_test`; if any code changes were made
- [X] [Docker Compose
LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md);
if any major functionality was changed or introduced
- [x] [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any infrastructure or configuration changes were made

## 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 added, or updated, [`godoc` format
comments](https://go.dev/blog/godoc) on touched members (see:
[tip.golang.org/doc/comment](https://tip.golang.org/doc/comment))
- [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)

---------

Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
Co-authored-by: Daniel Olshansky <olshansky@pokt.network>
@github-project-automation github-project-automation bot moved this from In Review to Done in V1 Dashboard Apr 7, 2023
bryanchriswhite added a commit that referenced this issue Apr 10, 2023
* pokt/main:
  [Utility][RPC][CLI] Querying governance parameters (Issue #619) (#622)
  [Persistence][Utility] Separate all CreateAndApply functions into more functional components - Issue #508 (#652)
  [Persistence][Utility] Pools Address hack removal + state accessor fix for params and flags (#654)
  [PERSISTENCE] SavePoints and Rollbacks design document (Issue #493) (#533)
  Update reviewpad.yml
  Added ChatGPT-CodeReview workflow (#649)
  Update reviewpad.yml
  Added default reviewpad.yml file (#648)
  [DevNet] tweaks for remote environments (#601)
  [Documentation] Swap validator and non-validator triggers when finished synching (#646)
  [Consensus] Configuration entry point state sync (#528)
bryanchriswhite added a commit to bryanchriswhite/pocket that referenced this issue Apr 10, 2023
…p-modules

* pokt/main:
  [Utility][RPC][CLI] Querying governance parameters (Issue pokt-network#619) (pokt-network#622)
  [Persistence][Utility] Separate all CreateAndApply functions into more functional components - Issue pokt-network#508 (pokt-network#652)
  [Persistence][Utility] Pools Address hack removal + state accessor fix for params and flags (pokt-network#654)
  [PERSISTENCE] SavePoints and Rollbacks design document (Issue pokt-network#493) (pokt-network#533)
  Update reviewpad.yml
  Added ChatGPT-CodeReview workflow (pokt-network#649)
  Update reviewpad.yml
  Added default reviewpad.yml file (pokt-network#648)
  [DevNet] tweaks for remote environments (pokt-network#601)
  [Documentation] Swap validator and non-validator triggers when finished synching (pokt-network#646)
  [Consensus] Configuration entry point state sync (pokt-network#528)
bryanchriswhite added a commit that referenced this issue Apr 12, 2023
…p-modules

* pokt/main:
  update pocket repo read.me (#667)
  Update reviewpad.yml
  [KEYBASE] Add improve comment on keybase config (#665)
  [E2E] Chore: Doc updates (#663)
  [E2E] Adds staking, unstaking, and sending tests (#653)
  [Utility][RPC][CLI] Querying governance parameters (Issue #619) (#622)
  [Persistence][Utility] Separate all CreateAndApply functions into more functional components - Issue #508 (#652)
  [Persistence][Utility] Pools Address hack removal + state accessor fix for params and flags (#654)
  [PERSISTENCE] SavePoints and Rollbacks design document (Issue #493) (#533)
  Update reviewpad.yml
  Added ChatGPT-CodeReview workflow (#649)
  Update reviewpad.yml
  Added default reviewpad.yml file (#648)
  [DevNet] tweaks for remote environments (#601)
  [Documentation] Swap validator and non-validator triggers when finished synching (#646)
  [Consensus] Configuration entry point state sync (#528)
bryanchriswhite added a commit that referenced this issue Apr 12, 2023
…p-modules

* pokt/main:
  update pocket repo read.me (#667)
  Update reviewpad.yml
  [KEYBASE] Add improve comment on keybase config (#665)
  [E2E] Chore: Doc updates (#663)
  [E2E] Adds staking, unstaking, and sending tests (#653)
  [Utility][RPC][CLI] Querying governance parameters (Issue #619) (#622)
  [Persistence][Utility] Separate all CreateAndApply functions into more functional components - Issue #508 (#652)
  [Persistence][Utility] Pools Address hack removal + state accessor fix for params and flags (#654)
  [PERSISTENCE] SavePoints and Rollbacks design document (Issue #493) (#533)
  Update reviewpad.yml
  Added ChatGPT-CodeReview workflow (#649)
  Update reviewpad.yml
  Added default reviewpad.yml file (#648)
  [DevNet] tweaks for remote environments (#601)
  [Documentation] Swap validator and non-validator triggers when finished synching (#646)
  [Consensus] Configuration entry point state sync (#528)
@okdas okdas mentioned this issue Jul 17, 2023
20 tasks
okdas added a commit that referenced this issue Jul 20, 2023
## Description

After the discussion about WASM and Go 1.21, I thought we should upgrade
to 1.20 first to make sure the 1.19 -> 1.20 upgrade can be done without
issues, for easier upgrade to 1.21 later.

We have a ticket #588 that touches on that, but it also recommends
another change that can be taken care of in another PR later.

<!-- reviewpad:summarize:start -->
### Summary generated by Reviewpad on 20 Jul 23 18:54 UTC
This pull request includes several changes to bump the golang version to
1.20. The patch modifies various files such as workflows, Dockerfiles,
and documentation files. Additionally, a new document called
GOLANG_UPGRADE.md is added to provide a checklist for future Go version
upgrades.
<!-- reviewpad:summarize:end -->

#508

Fixes #<issue_number>

## Type of change

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [ ] Bug fix
- [ ] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [x] Compiler version change

## List of changes

- Bumped Go to 1.20

## Testing

- [x] `make develop_test`; if any code changes were made
- [x] `make test_e2e` on [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any code changes were made
- [x] `e2e-devnet-test` passes tests on
[DevNet](https://pocketnetwork.notion.site/How-to-DevNet-ff1598f27efe44c09f34e2aa0051f0dd);
if any code was changed
- [ ] [Docker Compose
LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md);
if any major functionality was changed or introduced
- [x] [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any infrastructure or configuration changes were made

## Required Checklist

- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added, or updated, [`godoc` format
comments](https://go.dev/blog/godoc) on touched members (see:
[tip.golang.org/doc/comment](https://tip.golang.org/doc/comment))
- [ ] I have tested my changes using the available tooling
- [ ] 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
code health Nice to have code improvement consensus Consensus specific changes core Core infrastructure - protocol related persistence Persistence specific changes utility Utility specific changes
Projects
Status: Done
3 participants