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

[TECHDEBT] Consolidate & encapsulate ValidatorMap logic #203

Closed
5 tasks
Olshansk opened this issue Sep 11, 2022 · 2 comments · Fixed by #402
Closed
5 tasks

[TECHDEBT] Consolidate & encapsulate ValidatorMap logic #203

Olshansk opened this issue Sep 11, 2022 · 2 comments · Fixed by #402
Assignees
@deblasis
Labels
code health Nice to have code improvement consensus Consensus specific changes core Core infrastructure - protocol related
Milestone
M1: Pocket PoS (P...

Comments

@Olshansk
Copy link
Member

Objective

Encapsulate the concept of a ValidatorMap only to the consensus module

Origin Document

This discussion in #178:

Screen Shot 2022-09-11 at 3 15 53 PM

Goals

  • Remove all occurrences/references to the ValidatorMap throughout the codebase
  • Only maintain knowledge/understanding o the ValidatorMap in the consensus module

Deliverable

  • Do a grep for ValidatorMap throughout the whole codebase and it should only show up in the consensus module
  • A single PR refactoring / cleaning up this goal

Non-goals / Non-deliverables

  • Adding new functionality

General issue deliverables

  • Update the appropriate CHANGELOG
  • Update any relevant READMEs (local and/or global)
  • Update any relevant global documentation & references

Testing Methodology

  • All tests: make test_all
  • LocalNet: verify a LocalNet is still functioning correctly by following the instructions at docs/development/README.md

Creator: @Olshansk
@Olshansk Olshansk added core Core infrastructure - protocol related consensus Consensus specific changes code health Nice to have code improvement labels Sep 11, 2022
@Olshansk Olshansk self-assigned this Sep 11, 2022
@Olshansk Olshansk moved this to Up Next in V1 Dashboard Sep 11, 2022
@Olshansk Olshansk mentioned this issue Sep 11, 2022
Merged
7 tasks
@jessicadaugherty jessicadaugherty moved this from Up Next to Backlog in V1 Dashboard Oct 5, 2022
@Olshansk Olshansk assigned deblasis and unassigned Olshansk Oct 5, 2022
@Olshansk
Copy link
Member Author

Olshansk commented Oct 5, 2022 •
edited
Loading

This is related to #271 in the greater scope/vision of having dynamic validator sets as they change with each height.

#271 is scoped to only P2P and doing structured gossip amongst the validator nodes (for now). In general, P2P does care who the nodes are but simply gossips amongst the actors it is given.

This ticket is scoped to consensus (techdebt cleanup) to make sure the concept of a "validator set" only exists in consensus.

To keep in mind

  • As this is done (in isolation), the goal is to think how to implement dynamic additions of validators & P2P actors.
  • Further, thinking through how LocalNet can be designed/updated to dynamically add new validators is a non-trivial amount of work that's of high importance.

@Olshansk Olshansk moved this from Backlog to Up Next in V1 Dashboard Nov 14, 2022
@deblasis deblasis moved this from Up Next to In Progress in V1 Dashboard Dec 5, 2022
@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.

This was referenced Dec 14, 2022
Closed
Merged
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 15, 2022
Merged
16 tasks
@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 23, 2022
deblasis added a commit that referenced this issue Jan 9, 2023
@deblasis @Olshansk
[TECHDEBT] Consolidate & encapsulate ValidatorMap logic - (Issue #203) (
a86c0e6 
#402)

## Description

## Issue

Fixes #203

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

- Refactored so that references to `modules.ValidatorMap` are
removed/internalized within `Consensus`
- [WIP] Make sure that the debug client can discover the validators in
`LocalNet`

## Testing

- [x] `make develop_test`
- [ ]
[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>
@github-project-automation github-project-automation bot moved this from In Review to Done in V1 Dashboard Jan 9, 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 core Core infrastructure - protocol related
Projects
V1 Dashboard
Status: Done
Milestone
M1: Pocket PoS (Proof of Stake)
Development

Successfully merging a pull request may close this issue.

[TECHDEBT] Consolidate & encapsulate ValidatorMap logic - (Issue #203) pokt-network/pocket
2 participants
@Olshansk @deblasis