[Core] Consolidate all node configurations into a new configs package - (Issue #396) #403

deblasis · 2022-12-15T22:06:40Z

Description

The main goal of this PR is to centralize configurations into a single configs package.
The feedback received was that the previous implementation was probably overly abstracted. Mockability is still preserved thanks to the fact that we are ultimately just dealing with structs without any logic attached to them.

It also tends to address:

// IMPROVE(#361): Design a better way to generate deterministic keys for testing.

https://github.com/pokt-network/pocket/compare/issue/396-consolidate-config?expand=1#diff-ee06bf613911836fdf48424397b7ec556c2e1a8ddba5adecca4c1782aea24087L30

Issue

Type of change

Please mark the relevant option(s):

List of changes

Centralized configs into the configs package
Introduced a keygenerator that can work deterministically or randomly at will
Centralize genesis as well, from two structs with overlapping states into a single one
Simplified shared core types (Actor, Account, etc) under coreTypes
Added missing max_mempool_count in config (it was causing P2P instabilities in LocalNet)

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)

deblasis · 2022-12-15T22:13:05Z

@Olshansk I would appreciate a quick scan on this, just to make sure it's getting where it's supposed to be.

Please point out blatant gaps (apart from the fact that I need to centralize genesis as well but I am waiting for this initial feedback on the design before proceeding).
Close your eyes when you see leftover comments and other nits 🙂

🙏

Olshansk

tl;dr I ❤️ where this is going

Co-authored-by: Daniel Olshansky <[email protected]>

Olshansk · 2023-01-03T16:25:22Z

shared/core/types/pools.go

+	poolFriendlyNames = map[Pools]string{
+		Pools_POOLS_UNSPECIFIED:        "Unspecified",
+		Pools_POOLS_DAO:                "DAO",
+		Pools_POOLS_FEE_COLLECTOR:      "FeeCollector",


Suggested change

Pools_POOLS_FEE_COLLECTOR: "FeeCollector",

Pools_POOLS_FEE_COLLECTOR: "FeeCollectorPool",

Are you sure you want to change this? The current values reflect what's currently on main, also, I could argue that they are entities in the table pool so the suffix is redundant.

Olshansk · 2023-01-03T16:25:32Z

shared/core/types/pools.go

+func init() {
+	poolFriendlyNames = map[Pools]string{
+		Pools_POOLS_UNSPECIFIED:        "Unspecified",
+		Pools_POOLS_DAO:                "DAO",


Suggested change

Pools_POOLS_DAO: "DAO",

Pools_POOLS_DAO: "DAOPool",

#403 (comment)

Olshansk · 2023-01-03T16:37:54Z

persistence/genesis.go

+	stakedActorsInsertConfigs := []struct {
+		Name     string
+		Getter   func() []*coreTypes.Actor
+		InsertFn func(address []byte, publicKey []byte, output []byte, paused bool, status int32, serviceURL string, stakedTokens string, chains []string, pausedHeight int64, unstakingHeight int64) error


I would say yes but if we want to enforce a style I would say let's be consistent and scout the whole codebase for similar instances and rectify accordingly otherwise it becomes a mix and match.

Agreed. I was trying to be consistent with line 83 of this file (just local consistency).

I believe we can enforce not specifying types (when they're the same) using the automatic linters @okdas set-up since these are standard golang suggestions. They're currently off because we need to do a one-time cleanup.

You are correct that #295 encompasses this.

Another thing we could do is adding newlines when the signatures are particularly long. I think you did that somewhere in the codebase and I like it since it allows the reader to avoid horizontal scrolling.

I do like it but think it's overkill here because the function is anonymous.

Let me know how you want to go about your suggestion and, more generally, about styling guidelines, perhaps this could be related to linting? (https://github.com/pokt-network/pocket/issues/295)

Let's just be consistent between this and line 83 and #295 will encompass the work of enforcing repo-wide guidelines.

Olshansk · 2023-01-03T16:42:50Z

persistence/shared_sql.go

@@ -92,7 +92,7 @@ func (p *PostgresContext) getActor(actorSchema types.ProtocolActorSchema, addres
 	if err != nil {
 		return
 	}
-
+	actor.ActorType = actorSchema.GetActorType()


getActor uses getActorFromRow under the hood

BUT

GetAllXXX uses getActorFromRow directly

So this won't actually fully work.

I'm okay with leaving it as is because we have a ton of scope creep, but could you add a TODO to fix it (in a follow up PR). I also think this is a really good opportunity to do some TDD to be future-proof.

I coudn't resist... fb42810 and 6ff3c3a

Olshansk · 2023-01-03T16:43:07Z

persistence/docs/CHANGELOG.md

-## [0.0.0.10] - 2022-12-21
+## [0.0.0.17] - 2022-12-28
+
+- Added missing `ActorType` in `GetAllXXXX()` functions


Left a comment in persistence/shared_sql.go

Olshansk · 2023-01-03T16:44:32Z

p2p/CHANGELOG.md

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

 ## [Unreleased]

+## [0.0.0.14] - 2022-12-28
+
+- `ActorsToAddrBook` now skips actors that are not validators since they don't have a serviceUrl generic parameter


Regarding libp2p - makes sense when we get there.

Regarding the val filtering - see the comment I left in p2p/addrbook_provider/persistence.go

Also, what is this concept of generic parameter? Where can I learn more about it?

This is just a poor design decision that was the result of a compromise during the shared module refactor.

We (currently) have 4 protocol actor types:

Fisherman

Validator

Servicer

Application

There are things they all share (e.g. an address) and there are things that are specific:

Servicers need a "serviceURL" to service nodes

Fisherman need a "serviceURL" to enable QoS

Applications need a 'maxRelaysCount' which is only specific to them

The two potential future paths we take are:

One Protobuf per actor

One BaseActor protobuf that's embedded in each ProtocolActor type

See #313 for the origin document.

Olshansk · 2023-01-03T16:47:07Z

persistence/module.go

-	config       modules.PersistenceConfig
-	genesisState modules.PersistenceGenesisState
+	config       *configs.PersistenceConfig
+	genesisState *genesis.GenesisState


Ditto - fair point.

Given what you said, I don't love either approach but prefer what we have for now over the alternative.

Olshansk · 2023-01-03T16:51:00Z

p2p/addrbook_provider/persistence.go

 	book := make(typesP2P.AddrBook, 0)
 	for _, v := range actors {
+		// only add validator actors since they are the only ones having a service url in their generic param at the moment
+		if v.ActorType != coreTypes.ActorType_ACTOR_TYPE_VAL {


Related to one of my other comments, fisherman and serviceNodes also have serviceURLs in their parameter, and can (and should) also participate in structured broadcast.

Should they not be included as well?

TL;DR: The way I see it, NO...

Like you said, it looks like these serviceUrls have a different meaning depending on the actor type and therefore using them to connect to a node for consensus related messaging/structured broadcast is a... shortcut.

Nodes are listening on the ConsensuPort which by default is 8080 and it seems that serviceUrl has been used to have an easy way to address them.

I guess this is because we don't have -yet- a full-fledged P2P module that owns the addressbook and can be relied upon for routing and messaging (that will come with peer discovery and churn).

We won't address messages via serviceUrl but rather by address or nodeId, then P2P would figure out how to reach that peer.

In LocalNet we spin up 4 validator nodes and the debug commands are all related to consensus.
This explains why previously we had 👇👈 while now we have 👉👇

If we consider other actors I can only imagine that since we have overlaps (eg: node1.consensus:8080 in genesis.json is a Validator, a ServiceNode and also a Fisherman) we would have duplicate messages at a minimum.

I wish Brian was here to pick his brain on this :) the ways I see it, structured gossip happens under the hood in the P2P module, when you say "broadcast to all validators" you don't know/care if the message is also handled and retransmitted by a Fisherman in order to reach a "nearby" Validator. That's implementation detail of the P2P algorithm.

I would say that for now we have to filter. Happy to discuss further even offline.

Just let me know how to proceed here.

Solid explanation. I'm convinced.

Excited to see what our next steps are going to be here :)

Added #426 PTAL 🙏

Olshansk

One tiny comment about the addition of val1.json and val2.json to the root dir.

Add it to .gitignore
Update docs/demos/iteration_3_end_to_end_tx.md to move it into /tmp and reference that directly.

Olshansk · 2023-01-03T21:35:28Z

p2p/CHANGELOG.md

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

 ## [Unreleased]

+## [0.0.0.14] - 2022-12-28
+
+- `ActorsToAddrBook` now skips actors that are not validators since they don't have a serviceUrl generic parameter


Olshansk · 2023-01-03T21:36:27Z

val1.json

@@ -0,0 +1 @@
+"6fd0bc54cc2dd205eaf226eebdb0451629b321f11d279013ce6fdd5a33059256b2eda2232ffb2750bf761141f70f75a03a025f65b2b2b417c7f8b3c9ca91e8e4"


Intentional?

nope. Genuine mistake

Olshansk · 2023-01-03T21:37:47Z

persistence/test/actor_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestGetAllStakedActors(t *testing.T) {


Olshansk · 2023-01-03T21:39:49Z

p2p/addrbook_provider/persistence.go

 	book := make(typesP2P.AddrBook, 0)
 	for _, v := range actors {
+		// only add validator actors since they are the only ones having a service url in their generic param at the moment
+		if v.ActorType != coreTypes.ActorType_ACTOR_TYPE_VAL {


Solid explanation. I'm convinced.

Excited to see what our next steps are going to be here :)

Olshansk

LFG!

🚢 🚢🚢

deblasis added 11 commits December 15, 2022 21:55

refactor(shared): centralized config_protos

80eb15b

feat(test_artifacts): deterministic keygen

35b128a

refactor(runtime): refactored to use centralized configs

df5c92e

refactor(Consensus): refactored to use centralized config

97803dd

refactor(logger): refactored to use centralized logging

d439459

refactor(P2P): refactored to use centralized configs

807086f

refactor(Persistence): refactored to use centralized configs

774e9fd

refactor(RPC): refactored to use centralized configs

c88f455

refactor(telemetry): refactored to use centralized configs

d2f3ba7

refactor(Utility): refactored to use centralized configs

ab115f6

refactor(shared): refactored to use centralized configs

80a7ba2

deblasis added the core Core infrastructure - protocol related label Dec 15, 2022

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

deblasis self-assigned this Dec 15, 2022

style(shared): 🧹

6187b29

Olshansk reviewed Dec 16, 2022

View reviewed changes

deblasis added 13 commits December 19, 2022 23:19

refactor(config): removed baseconfig, added max_mempool_count

fe88719

refactor(config): removed base_config

375fcac

refactor(genesis): single genesis

2ea6b95

refactor(core): core types

f599882

refactor(shared): core types

43c560f

refactor(genesis): centralized genesis

f60517b

refactor(consensus): removed converter

a689e90

refactor(persistence): removed converter

b4a5fd9

chore(go.mod): tidy

535130d

refactor(consensus): centralized genesis + core types

00de795

refactor(persistence): centralized genesis + core types

0165bd4

refactor(p2p): centralized genesis + core types

2248e74

refactor(runtime): centralized genesis + core types

fa09298

deblasis and others added 9 commits December 30, 2022 08:06

Update docs/demos/iteration_3_end_to_end_tx.md

4bddf5f

Co-authored-by: Daniel Olshansky <[email protected]>

Update persistence/docs/CHANGELOG.md

0532b07

Co-authored-by: Daniel Olshansky <[email protected]>

refactor(persistence): s/InitParams/InitGenesisParams

c86f8c5

docs(runtime): config options godoc

69cb8eb

refactor(runtime): default values in defaults package

00ae143

refactor(core): poolNames -> pools

5b237f2

docs(P2P): added TODO

8d9cb1a

refactor(persistence): actorType from schema

d7a521e

chore(shared): CHANGELOG 📆

b8d1946

deblasis requested a review from Olshansk December 30, 2022 13:41

Olshansk requested changes Jan 3, 2023

View reviewed changes

deblasis added 4 commits January 3, 2023 19:55

test(persistence): added test for GetAllStakedActors

fb42810

refactor(persistence): actorType set in getActorForRow

6ff3c3a

chore(persistence): argument types in InsertFn

414276a

docs(shared): CHANGELOGs dates

d82b018

deblasis requested a review from Olshansk January 3, 2023 21:18

Olshansk requested changes Jan 3, 2023

View reviewed changes

deblasis added 3 commits January 3, 2023 21:49

chore(shared): rm val keys

6dea794

chore(gitignore): added validator keys

52b500b

docs(iteration3demo): updated to source keys from /tmp/

ac0e99d

Olshansk approved these changes Jan 3, 2023

View reviewed changes

deblasis mentioned this pull request Jan 3, 2023

[P2P] Replace addressing via ServiceUrl/GenericParam with P2P routing #426

Closed

11 tasks

deblasis added 4 commits January 3, 2023 22:30

docs(P2P): added todo #426

3524dc9

docs(CLI): updated README

636638f

docs(runtime): updated README

2f73d5e

docs(development): README

26ef163

deblasis merged commit bbf8205 into main Jan 3, 2023

Olshansk mentioned this pull request Jan 9, 2023

[LocalNet] Execution Height and Connection Errors #433

Closed

6 tasks

Olshansk deleted the issue/396-consolidate-config branch June 2, 2023 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Consolidate all node configurations into a new configs package - (Issue #396) #403

[Core] Consolidate all node configurations into a new configs package - (Issue #396) #403

deblasis commented Dec 15, 2022 •

edited

Loading

deblasis commented Dec 15, 2022 •

edited

Loading

Olshansk left a comment

Olshansk Jan 3, 2023

deblasis Jan 3, 2023

Olshansk Jan 3, 2023

deblasis Jan 3, 2023

Olshansk Jan 3, 2023

Olshansk Jan 3, 2023

deblasis Jan 3, 2023

Olshansk Jan 3, 2023

Olshansk Jan 3, 2023

Olshansk Jan 3, 2023

Olshansk Jan 3, 2023

deblasis Jan 3, 2023

Olshansk Jan 3, 2023

deblasis Jan 3, 2023

Olshansk left a comment

Olshansk Jan 3, 2023

Olshansk Jan 3, 2023

deblasis Jan 3, 2023

Olshansk Jan 3, 2023

Olshansk Jan 3, 2023

Olshansk left a comment

	Pools_POOLS_FEE_COLLECTOR: "FeeCollector",
	Pools_POOLS_FEE_COLLECTOR: "FeeCollectorPool",

		@@ -0,0 +1 @@
		"6fd0bc54cc2dd205eaf226eebdb0451629b321f11d279013ce6fdd5a33059256b2eda2232ffb2750bf761141f70f75a03a025f65b2b2b417c7f8b3c9ca91e8e4"

[Core] Consolidate all node configurations into a new configs package - (Issue #396) #403

[Core] Consolidate all node configurations into a new configs package - (Issue #396) #403

Conversation

deblasis commented Dec 15, 2022 • edited Loading

Description

Issue

Type of change

List of changes

Testing

Required Checklist

If Applicable Checklist

deblasis commented Dec 15, 2022 • edited Loading

Olshansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

deblasis commented Dec 15, 2022 •

edited

Loading

deblasis commented Dec 15, 2022 •

edited

Loading