[TECHDEBT] [P2P] integration with persistence for addressBook management - (Issue #271) #374

deblasis · 2022-12-05T01:41:23Z

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:

pocket/p2p/raintree/network.go

Line 29 in 444c424

    
           mempool map[uint64]struct{} // TODO (#278) replace map implementation (can grow unbounded)

Issue

Fixes #271

Type of change

Please mark the relevant option(s):

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

make develop_test
LocalNet w/ all of the steps outlined in the README

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 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 diagrams in the corresponding README(s)
I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

…stence-addrbook-management_l

Olshansk

@deblasis Left a few comments, and the integration of consensus with persistence is great, but I think I may have led you down the wrong path.

When we're doing something like consensus, the proposer does a direct send (address forwarded to P2P and P2P resolves it).
When we're doing a structured gossip (where every staked actor has the same view of all the addresses on the network), the P2P module retrieves that list from persistence, and is then responsible for resolving addresses to IPs. When an address doesn't resolve is when we need to start looking at our redundancy/cleanup/adjust/resent layers
This is making me think that in the case of full nodes, there might also be a a need for random gossip (increase data availability, but still need to mull on it).

Overall, given the comments, do you see a need for the broadcast events introduced here?

consensus/consensus_tests/utils_test.go

consensus/leader_election/module.go

consensus/pacemaker.go

p2p/module.go

p2p/raintree/network.go

shared/messaging/proto/addressbook_at_height.proto

shared/node.go

p2p/addrbook_provider/getter.go

runtime/defaults/defaults.go

Olshansk · 2022-12-14T00:12:25Z

p2p/module.go

+			return fmt.Errorf("failed to cast message to BeforeHeightChangedEvent")
+		}
+
+		// DISCUSS (https://github.com/pokt-network/pocket/pull/374#issuecomment-1341350786): decide if we want a pull or push model for integrating with persistence


To quote you previous message:

Where would this function be called currently? I would say that if it's not relevant to the scope of this issue we shouldn't do it here. Sounds like scope creep a little bit. No?

I'm personally not a huge fan because these events were out of scope and are not being used, though I can see where your head is at.

If I were to introduce a new event, I'd probably create a NewViewEvent(height, round) and/or NewHeight(height) because I can only speculate how we'd use BeforeNewHeightEvent right now and the cost to add it is low.

I don't want this to continue being a blocker, but not a fan. Let's do this:

If you feel strongly that BeforeNewHeightEvent and HeightChangedEvent events will be useful, let's keep it as is.

If you're not sure, let's do one of the following:
2.1. Remove them altogether
2.2 Just add a NewViewEvent (per my suggestions)

consensus/pacemaker.go

p2p/types/addr_book.go

persistence/genesis.go

Olshansk · 2022-12-14T00:22:28Z

persistence/genesis.go

+
+// IMPROVE: This is a proof of concept. Ideally we should have a single query that returns all actors.
+func (p PostgresContext) GetAllStakedActors(height int64) (actors []modules.Actor, err error) {
+	actors = make([]modules.Actor, 0)


if we don't initialize this, does it break?

Oh, well, good to know! Thanks @Olshansk 🙏

https://go.dev/play/p/GjBg0QuZEbB

Nice

persistence/genesis.go

p2p/raintree/peers_manager.go

p2p/addrbook_provider/persistence.go

Olshansk · 2022-12-14T00:32:04Z

@deblasis Sorry for hom many back & forths this one is taking, but it's looking better with every iteration, and thank you for bearing!

deblasis · 2022-12-14T17:27:04Z

@deblasis Sorry for hom many back & forths this one is taking, but it's looking better with every iteration, and thank you for bearing!

It's all good @Olshansk ! It's all part of the process. I should be the one apologizing if you think about it.

I'll address your comments and re-request review later today. 🙏

Co-authored-by: Daniel Olshansky <olshansky@pokt.network>

…stence-addrbook-management_l Signed-off-by: Alessandro De Blasis <alex@deblasis.net>

to DefaultP2PMempoolMaxNonceCount

…s.go to actor.go

persistence/genesis.go

deblasis · 2022-12-14T22:44:15Z

I have removed the events, addressed comments and it's ready for another pass. Fewer things to look at I guess. We are getting there.

When you are happy I'll update the CHANGELOGs

Olshansk

LGTM, let's just update the CHANGELOG and we're good to go

p2p/raintree/utils_test.go

p2p/addrbook_provider/persistence.go

Co-authored-by: Daniel Olshansky <olshansky@pokt.network>

deblasis · 2022-12-15T01:30:03Z

@Olshansk CHANGELOGs updated

Olshansk · 2022-12-15T01:38:37Z

runtime/docs/CHANGELOG.md

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ## [Unreleased]

+## [0.0.1] - 2022-12-14
+
+- Added `DefaultP2PMempoolMaxNonceCount`


Optional: Rename this to DefaultP2PMempoolMaxCount instead DefaultP2PMempoolMaxNonceCount to match the interface name

I see your DefaultP2PMempoolMaxCount and I am going all-in with DefaultP2PMaxMempoolCount since the interface method is called GetMaxMempoolCount

… - (Issue #270) (#378) ## Description This PR builds on top of #374 (the base branch is not `main` in order to highlight the delta of the changes) in order to tend to the TECHDEBT TODOs listed in #270 ## Issue Fixes #270 ## 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  ## List of changes - Consensus - `LeaderElectionModule`'s `electNextLeaderDeterministicRoundRobin` now uses `Persistence` to access the list of validators instead of the static `ValidatorMap`. - Updated tests (mock configs) accounting for the updated logic - P2P - `ValidatorMapToAddrBook` renamed to `ActorToAddrBook` - `ValidatorToNetworkPeer` renamed to `ActorToNetworkPeer` ## 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>

… - (Issue #270) (#378) This PR builds on top of #374 (the base branch is not `main` in order to highlight the delta of the changes) in order to tend to the TECHDEBT TODOs listed in #270 Fixes #270 Please mark the relevant option(s): - [x] New feature, functionality or library - [ ] Bug fix - [x] Code health or cleanup - [ ] Major breaking change - [ ] Documentation - [ ] Other  - Consensus - `LeaderElectionModule`'s `electNextLeaderDeterministicRoundRobin` now uses `Persistence` to access the list of validators instead of the static `ValidatorMap`. - Updated tests (mock configs) accounting for the updated logic - P2P - `ValidatorMapToAddrBook` renamed to `ActorToAddrBook` - `ValidatorToNetworkPeer` renamed to `ActorToNetworkPeer` - [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`  - [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 - [ ] 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 added 17 commits December 4, 2022 21:01

feat(Messaging): new events and message

79e988c

feat(Consensus): the pacemaker emits Height related events

f925a1f

feat(P2P): reacts to BeforeHeightChanged and broadcasts addrb

27859db

feat(Messaging): eventTypes for new messages/events

e19041b

chore(P2P): ErrUnknownEventType

550c47e

feat(P2P): HandleEvent

3dc5ffd

feat(Shared): event handling for height events and addressbook

3c9cbc7

docs(Consensus): changelog

a4abab8

docs(Shared): changelog

4d01838

test(Consensus): P2PMock fix (HandleEvent)

12ee0ec

feat(P2P): raintree mempool map not unbounded anymore

520b8b9

docs(P2P): changelog

caaad9c

feat(P2P): raintree now allows ephemeral peersmanager

9506419

docs(P2P): changelog

0cf393c

feat(P2P): AddrBookProvider

3fdc817

test(P2P): fix tests

10a1bb6

fix(P2P): fix client P2P.Start() 💩

5a7c14c

deblasis added p2p P2P specific changes code health Nice to have code improvement labels Dec 5, 2022

deblasis added this to the M1: Pocket PoS (Proof of Stake) milestone Dec 5, 2022

deblasis requested a review from Olshansk December 5, 2022 01:41

deblasis self-assigned this Dec 5, 2022

deblasis mentioned this pull request Dec 5, 2022

[TECHDEBT] [P2P] P2P integration with utility for addressBook lookups - (Issue #270) #378

Merged

16 tasks

deblasis added 4 commits December 5, 2022 21:41

feat(Consensus): leader election gets validators from persistence

8e5b367

Merge remote-tracking branch 'upstream/main' into issue/271-p2p-persi…

53d496b

…stence-addrbook-management_l

test(Consensus): fixed tests (mock config due to changed flow)

47540f6

test(messaging): contentType tests

dbb24d5

Olshansk requested changes Dec 7, 2022

View reviewed changes

deblasis added 2 commits December 7, 2022 10:55

test(consensus): consolidated numValidators and mocks mgmt

09aa018

fix(pacemaker): logging packing errors if any

f773345

Olshansk reviewed Dec 14, 2022

View reviewed changes

Olshansk requested changes Dec 14, 2022

View reviewed changes

deblasis and others added 7 commits December 14, 2022 21:34

Update p2p/addrbook_provider/getter.go

86ae4d8

Co-authored-by: Daniel Olshansky <olshansky@pokt.network>

Merge remote-tracking branch 'upstream/main' into issue/271-p2p-persi…

52ea911

…stence-addrbook-management_l Signed-off-by: Alessandro De Blasis <alex@deblasis.net>

chore(P2P): improved TODO

11a41eb

refactor(P2P): improved error handling

7c9fe8c

refactor(P2P): renamed DefaultP2PMempoolMaxNonces

1c65cb9

to DefaultP2PMempoolMaxNonceCount

refactor(Shared): removed height related events, premature. KISS 💋

d8fbd35

refactor(persistence): improved GetAllStakedActors, moved from genesi…

b2fa44f

…s.go to actor.go

deblasis commented Dec 14, 2022

View reviewed changes

persistence/genesis.go Show resolved Hide resolved

deblasis requested a review from Olshansk December 14, 2022 22:45

docs(Persistence): updated TODO

3131e00

Olshansk requested changes Dec 15, 2022

View reviewed changes

p2p/raintree/utils_test.go Outdated Show resolved Hide resolved

p2p/addrbook_provider/persistence.go Outdated Show resolved Hide resolved

deblasis and others added 4 commits December 15, 2022 00:52

Update p2p/addrbook_provider/persistence.go

b5f2ef5

Co-authored-by: Daniel Olshansky <olshansky@pokt.network>

refactor(P2P): renamed mockDummyBus to mockBus

9eb0646

docs(Consensus): CHANGELOG

5899337

docs(Shared): CHANGELOGs

ac11ed1

deblasis requested a review from Olshansk December 15, 2022 01:28

fix(P2P): changelog missing

9a32d51

Olshansk approved these changes Dec 15, 2022

View reviewed changes

refactor(P2P): DefaultP2PMaxMempoolCount

8438d29

deblasis merged commit 1e83081 into main Dec 15, 2022

Olshansk deleted the issue/271-p2p-persistence-addrbook-management_l branch December 15, 2022 06:00

Olshansk mentioned this pull request Dec 16, 2022

[Consensus][Bug] Fix HotPOKT broadcast from RainTree to *-pattern #405

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TECHDEBT] [P2P] integration with persistence for addressBook management - (Issue #271) #374

[TECHDEBT] [P2P] integration with persistence for addressBook management - (Issue #271) #374

deblasis commented Dec 5, 2022 •

edited

Loading

Olshansk left a comment

Olshansk Dec 14, 2022

Olshansk Dec 14, 2022

deblasis Dec 14, 2022

Olshansk Dec 15, 2022

Olshansk commented Dec 14, 2022

deblasis commented Dec 14, 2022

deblasis commented Dec 14, 2022

Olshansk left a comment

deblasis commented Dec 15, 2022

Olshansk Dec 15, 2022

deblasis Dec 15, 2022

Olshansk Dec 15, 2022

[TECHDEBT] [P2P] integration with persistence for addressBook management - (Issue #271) #374

[TECHDEBT] [P2P] integration with persistence for addressBook management - (Issue #271) #374

Conversation

deblasis commented Dec 5, 2022 • edited Loading

Description

Issue

Type of change

List of changes

Testing

Required Checklist

If Applicable Checklist

Olshansk left a comment

Choose a reason for hiding this comment

Olshansk Dec 14, 2022

Choose a reason for hiding this comment

Olshansk Dec 14, 2022

Choose a reason for hiding this comment

deblasis Dec 14, 2022

Choose a reason for hiding this comment

Olshansk Dec 15, 2022

Choose a reason for hiding this comment

Olshansk commented Dec 14, 2022

deblasis commented Dec 14, 2022

deblasis commented Dec 14, 2022

Olshansk left a comment

Choose a reason for hiding this comment

deblasis commented Dec 15, 2022

Olshansk Dec 15, 2022

Choose a reason for hiding this comment

deblasis Dec 15, 2022

Choose a reason for hiding this comment

Olshansk Dec 15, 2022

Choose a reason for hiding this comment

deblasis commented Dec 5, 2022 •

edited

Loading