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.

Sign up for GitHub

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

Jump to bottom

[SPIKE][Consensus] Remove validatorMap from the consensusModule struct #331

Closed
9 tasks
andrewnguyen22 opened this issue Nov 1, 2022 · 1 comment · Fixed by #425
Closed
9 tasks

[SPIKE][Consensus] Remove validatorMap from the consensusModule struct #331

andrewnguyen22 opened this issue Nov 1, 2022 · 1 comment · Fixed by #425
Assignees
@deblasis
Labels
code health Nice to have code improvement consensus Consensus specific changes
Milestone
M*: Pocket QoL (Q...

Comments

@andrewnguyen22
Copy link
Contributor

Objective

Remove validatorMap from the consensusModule struct state & access it from the persistence layer (added n the past)

The validatorMap is just a placeholder for the global ValidatorSet which is stored at the persistence layer.

NOTE: this is a multi-module issue

Origin Document

Originally scoped in issue #315 - separate issue to downscope

Goals

  • Remove validatorMap from the consensusModule struct and port it to the persistence module's GetActor(s) (Validator)
  • Update Changelog.md

Deliverable

  • Code complete
  • Supporting documentation

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

Creator: @andrewnguyen22
@andrewnguyen22 andrewnguyen22 added consensus Consensus specific changes code health Nice to have code improvement labels Nov 1, 2022
@andrewnguyen22 andrewnguyen22 mentioned this issue Nov 1, 2022
Merged
20 tasks
@Olshansk Olshansk moved this to Up Next in V1 Dashboard Nov 2, 2022
@Olshansk Olshansk self-assigned this Nov 2, 2022
andrewnguyen22 pushed a commit that referenced this issue Nov 4, 2022
[Persistence] TxIndexer to StateHash Bridge (Issue-#315) (#332)
9da5055 
## 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)
@Olshansk Olshansk changed the title [Consensus] Remove validatorMap from the consensusModule struct [TECHDEBT][Consensus] Remove validatorMap from the consensusModule struct Nov 13, 2022
@Olshansk Olshansk removed their assignment Nov 14, 2022
@Olshansk Olshansk changed the title [TECHDEBT][Consensus] Remove validatorMap from the consensusModule struct [SPIKE][Consensus] Remove validatorMap from the consensusModule struct Nov 28, 2022
@deblasis deblasis moved this from Up Next to In Progress in V1 Dashboard Dec 5, 2022
@deblasis deblasis mentioned this issue Dec 14, 2022
Closed
5 tasks
@deblasis
Copy link
Contributor

@jessicadaugherty heads up I am a bit behind schedule with #331 and #203 (ValidatorMap related work). I was supposed to be done yesterday but I think I'll need another day or two to have them both under review.

@deblasis deblasis mentioned this issue Dec 15, 2022
Merged
16 tasks
deblasis added a commit that referenced this issue Dec 15, 2022
@deblasis @Olshansk
[TECHDEBT] [P2P] integration with persistence for addressBook managem…
1e83081 
…ent - (Issue #271) (#374)

## Description

This PR is a stepping stone for the removal of the `ValidatorMap`.
Specifically it allows us to use per-Height addressBook and lays the
ground for churn management.
Right before a new height is reached, the addressBook is propagated via
a message on the `Bus` so that other interested modules can handle it.
This part is out-of-scope for this PR and will be covered later on in a
specific issue, unless differently agreed during code-review.

Also, this PR tends to the TODO:

https://github.com/pokt-network/pocket/blob/444c4242f8d165ba7702f160f6e94cd416fd0b8f/p2p/raintree/network.go#L29

## Issue

Fixes #271

## Type of change

Please mark the relevant option(s):

- [x] 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
- Consolidated number of validators in tests in a single constant:
`numValidators`
- Fixed typo in `make test_consensus_concurrent_tests` so that we can
run the correct test matrix
  - Using `GetBus()` instead of `bus` wherever possible
- P2P
- mempool cap is now configurable via P2PConfig. Tests implement the
mock accordingly.
- Introduced the concept of a `addrbookProvider` that abstracts the
fetching and the mapping from `Actor` to `NetworkPeer`
- Temporary hack to allow access to the `addrBook` to the debug client
(will be removed in an upcoming PR already in the works for issues
[#203](#203) and
[#331](#331))
  - Transport related functions are now in the `transport` package
- Updated tests to source the `addrBook` from the `addrbookProvider` and
therefore `Persistence`
  - Updated Raintree network constructur with dependency injection
  - Updated stdNetwork constructur with dependency injection
  - Improved documentation for the `peersManager
- Raintree mempool cannot grow unbounded anymore. It's now bounded by a
constant limit and when new nonces are inserted the oldest ones are
removed.
Olshansk marked this conversation as resolved.
Show resolved
- Raintree is now capable of fetching the address book for a previous
height and to instantiate an ephemeral `peersManager` with it.
- Persistence
  - Moved Actor related getters from `genesis.go` to `actor.go`
  - Added `GetAllStakedActors()` that returns all Actors
- Shared
  - Added `GetMaxMempoolCount`

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

Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
Co-authored-by: Daniel Olshansky <olshansky@pokt.network>
@deblasis deblasis mentioned this issue Dec 22, 2022
Closed
10 tasks
@jessicadaugherty jessicadaugherty moved this from In Progress to In Review in V1 Dashboard Dec 27, 2022
This was referenced Jan 1, 2023
Merged
Closed
deblasis added a commit that referenced this issue Jan 10, 2023
@deblasis @Olshansk
[SPIKE][Consensus] Remove validatorMap from the consensusModule struc…
08e438b 
…t - (Issue #331) (#425)

## Description

This PR builds upon #402 and removes `validatorMap` from the
`consensusModule` struct, encapsulating the logic so that the validator
set is dynamic and retrieved from the `persistence` module.

Also, since the change introduced the natural dependency on
`persistence`, it made me question and revisit the way the modules
register themselves with the `bus` and also how the modules are
instantiated.

In an effort to simply the codebase and testing, while we improve the
way we do dependency injection, the modules now are instantiated with a
`bus` that will allow them to access to all the dependencies they need.

Previously the modules required a `runtimeMgr` in their constructor and
the bus registration was handled like an after-thought.

## Issue

Fixes #331 

## Type of change

Please mark the relevant option(s):

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

- Removed `validatorMap` from `consensusModule` and encapsulated the
logic so that it's dynamic, per-height and sourced from `persistence`
- Refactored module registration in `bus`
- Refactored modules instantiation and DI (`runtimeMgr` -> `bus`)
- The `runtimeMgr` is now a dependency and it can be accessed via the
bus
- Minor improvements in tests

## 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
- [ ] I have updated the corresponding CHANGELOG

### If Applicable Checklist
- [x] I have updated the corresponding README(s); local and/or global
- [x] 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 Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@deblasis deblasis

Labels
code health Nice to have code improvement consensus Consensus specific changes
Projects
V1 Dashboard
Status: Done
Milestone
M*: Pocket QoL (Quality of Life)
3 participants
@Olshansk @andrewnguyen22 @deblasis