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

[P2P / runtime] chore: p2p connection type #450

Merged
merged 8 commits into from
Jan 25, 2023
Merged

[P2P / runtime] chore: p2p connection type #450

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
33aaafd
refactor: move ConnectionType to avoid cyclic import
bryanchriswhite Jan 19, 2023
9addbb0
chore: update makefile
bryanchriswhite Jan 19, 2023
0b792c7
chore: replace `P2PConfig#IsEmptyConnectionType` with `P2PConfig#Conn…
bryanchriswhite Jan 19, 2023
0fa1cd7
chore: replace DefaultIsEmptyConnectionType with DefaultConnectionType
bryanchriswhite Jan 19, 2023
49c177b
fix: use PROTOC_SHARED in makefile
bryanchriswhite Jan 23, 2023
246e670
chore: remove redundant protoc args
bryanchriswhite Jan 24, 2023
8f87f88
Merge branch 'main' into chore/p2p-connection-type
bryanchriswhite Jan 25, 2023
5dfadfd
fix: revert unreleased version numbers & push date
bryanchriswhite Jan 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,8 @@ protogen_local: go_protoc-go-inject-tag ## Generate go structures for all of the
$(PROTOC) -I=./shared/codec/proto --go_out=./shared/codec ./shared/codec/proto/*.proto

# Runtime
$(PROTOC_SHARED) -I=./runtime/configs/proto --go_out=./runtime/configs ./runtime/configs/proto/*.proto
$(PROTOC) -I=./runtime/configs/types/proto --go_out=./runtime/configs/types ./runtime/configs/types/proto/*.proto
$(PROTOC) -I=./runtime/configs/proto -I=./runtime/configs/types/proto --go_out=./runtime/configs ./runtime/configs/proto/*.proto
$(PROTOC_SHARED) -I=./runtime/genesis/proto --go_out=./runtime/genesis ./runtime/genesis/proto/*.proto
protoc-go-inject-tag -input="./runtime/genesis/*.pb.go"

Expand Down
5 changes: 4 additions & 1 deletion p2p/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

## [0.0.0.20] - 2023-01-20

- Updated `P2PConfig#IsEmptyConnectionType` bool to `P2PConfig#ConnectionType` enum

## [0.0.0.19] - 2023-01-19

- Rewrite `interface{}` to `any`

## [0.0.0.18] - 2023-01-11

- Add a lock to the mempool to avoid parallel messages which has caused the node to crash in the past
Expand Down
17 changes: 9 additions & 8 deletions p2p/transport/transport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package transport

import (
"fmt"
types "github.com/pokt-network/pocket/runtime/configs/types"
"io/ioutil"
"net"

Expand All @@ -14,24 +15,24 @@ const (
)

func CreateListener(cfg *configs.P2PConfig) (typesP2P.Transport, error) {
switch cfg.IsEmptyConnectionType { // TECHDEBT kept in switch format because this should be an enum not a bool
case true:
switch cfg.ConnectionType {
case types.ConnectionType_EmptyConnection:
return createEmptyListener(cfg)
case false:
case types.ConnectionType_TCPConnection:
return createTCPListener(cfg)
default:
return nil, fmt.Errorf("unsupported connection type for listener: %v", cfg.IsEmptyConnectionType)
return nil, fmt.Errorf("unsupported connection type for listener: %v", cfg.ConnectionType)
}
}

func CreateDialer(cfg *configs.P2PConfig, url string) (typesP2P.Transport, error) {
switch cfg.IsEmptyConnectionType {
case true:
switch cfg.ConnectionType {
case types.ConnectionType_EmptyConnection:
return createEmptyDialer(cfg, url)
case false:
case types.ConnectionType_TCPConnection:
return createTCPDialer(cfg, url)
default:
return nil, fmt.Errorf("unsupported connection type for dialer: %v", cfg.IsEmptyConnectionType)
return nil, fmt.Errorf("unsupported connection type for dialer: %v", cfg.ConnectionType)
}
}

Expand Down
9 changes: 5 additions & 4 deletions p2p/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"crypto/ed25519"
"encoding/binary"
"fmt"
types "github.com/pokt-network/pocket/runtime/configs/types"
"log"
"sort"
"sync"
Expand Down Expand Up @@ -121,10 +122,10 @@ func createMockRuntimeMgrs(t *testing.T, numValidators int) []modules.RuntimeMgr
RootDirectory: "",
PrivateKey: valKeys[i].String(),
P2P: &configs.P2PConfig{
PrivateKey: valKeys[i].String(),
ConsensusPort: 8080,
UseRainTree: true,
IsEmptyConnectionType: true,
PrivateKey: valKeys[i].String(),
ConsensusPort: 8080,
UseRainTree: true,
ConnectionType: types.ConnectionType_EmptyConnection,
},
}

Expand Down
8 changes: 4 additions & 4 deletions runtime/configs/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@ func NewDefaultConfig(options ...func(*Config)) *Config {
BlockStorePath: defaults.DefaultPersistenceBlockStorePath,
},
P2P: &P2PConfig{
ConsensusPort: defaults.DefaultP2PConsensusPort,
UseRainTree: defaults.DefaultP2PUseRainTree,
IsEmptyConnectionType: defaults.DefaultP2PIsEmptyConnectionType,
MaxMempoolCount: defaults.DefaultP2PMaxMempoolCount,
ConsensusPort: defaults.DefaultP2PConsensusPort,
UseRainTree: defaults.DefaultP2PUseRainTree,
ConnectionType: defaults.DefaultP2PConnectionType,
MaxMempoolCount: defaults.DefaultP2PMaxMempoolCount,
},
Telemetry: &TelemetryConfig{
Enabled: defaults.DefaultTelemetryEnabled,
Expand Down
9 changes: 3 additions & 6 deletions runtime/configs/proto/p2p_config.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,15 @@ syntax = "proto3";

package configs;

import "connection.proto";

option go_package = "github.com/pokt-network/pocket/runtime/configs";

message P2PConfig {
string private_key = 1;
uint32 consensus_port = 2;
bool use_rain_tree = 3;
bool is_empty_connection_type = 4; // TODO: Switch back to enum
conn.ConnectionType connection_type = 4;
uint64 max_mempool_count = 5; // this is used to limit the number of nonces that can be stored in the mempool, after which a FIFO mechanism is used to remove the oldest nonces and make space for the new ones
bool is_client_only = 6;
}

enum ConnectionType {
EmptyConnection = 0;
TCPConnection = 1;
}
10 changes: 10 additions & 0 deletions runtime/configs/types/proto/connection.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
syntax = "proto3";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think consolidating this with runtime/configs/proto/p2p_config.proto would be cleaner. Wdyt?

Copy link
Contributor Author

@bryanchriswhite bryanchriswhite Jan 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it would be simpler for P2PConfig and ConnectionTypes to be in the same package (as it was before my changes). There's a slight hangup though in the runtime/defaults package. When replacing defaults.DefaultP2PIsEmptyConnectionType with defaults.DefaultP2PConnectionType we need to import the ConnectionType enum, which causes a cyclic import (as configs/config.go is currently consuming defaults). I'm not sure there's much that can be done in the way of consolidation so long as configs is importing defaults and config.go shares a package with the ConnectionType generated proto code. We could, of course, move more of the runtime/configs package into the new one (e.g. P2PConfig). The most aggressive version of this would result in separating the all the generated protobuf into the new package. While I think, in principal, this is probably the right direction to go, it would require many more changes to account for imports of other configs proto types.

Another alternative would be to consider moving (at least) the ConnectionType enum to the shared protobuf files (e.g. shared/types/proto) as this would necessarily change its package but makes less sense in my opinion as I don't think it's a fair to characterize this enum as "shared" based on current usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also very open to suggestions on better names for both the go and proto package (types and conn, respectively, as of 33aaafd).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate the explanation @bryanchriswhite. I understand the point around consolidation and would say that lets just keep it as you have it.

The primary goal of this issue of using enums has been achieved relatively cleanly, and we'll have a lot more opportunities to clean up the code in the future.

The name lgtm!


package conn;

option go_package = "github.com/pokt-network/pocket/runtime/configs/types";

enum ConnectionType {
EmptyConnection = 0;
TCPConnection = 1;
}
10 changes: 6 additions & 4 deletions runtime/defaults/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package defaults

import (
"fmt"

types "github.com/pokt-network/pocket/runtime/configs/types"
)

const (
Expand All @@ -27,10 +29,10 @@ var (
DefaultPersistencePostgresUrl = "postgres://postgres:postgres@pocket-db:5432/postgres"
DefaultPersistenceBlockStorePath = "/var/blockstore"
// p2p
DefaultP2PConsensusPort = uint32(8080)
DefaultP2PUseRainTree = true
DefaultP2PIsEmptyConnectionType = false
DefaultP2PMaxMempoolCount = uint64(1e5)
DefaultP2PConsensusPort = uint32(8080)
DefaultP2PUseRainTree = true
DefaultP2PConnectionType = types.ConnectionType_TCPConnection
DefaultP2PMaxMempoolCount = uint64(1e5)
// telemetry
DefaultTelemetryEnabled = true
DefaultTelemetryAddress = "0.0.0.0:9000"
Expand Down
9 changes: 8 additions & 1 deletion runtime/docs/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

## [0.0.0.9] - 2023-01-23
## [0.0.0.10] - 2023-01-23

- Updated README.md with information about node profiling

## [0.0.0.9] - 2023-01-20

- move ConnectionType enum into its own package to avoid a cyclic import between configs and defaults packages (i.e. configs -> defaults -> configs) in the resulting, generated go package
- update makefile protogen_local target to build additional proto file and include it in the import path for runtime/configs/proto/p2p_config.proto
- replace `P2PConfig#IsEmptyConnectionType` bool with `P2PConfig#ConnectionType` enum
- replace `DefaultP2PIsEmptyConnectionType` bool with `DefaultP2PConnectionType` enum

bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved
## [0.0.0.8] - 2023-01-19

- Rewrite `interface{}` to `any`
Expand Down
21 changes: 11 additions & 10 deletions runtime/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/benbjohnson/clock"
"github.com/pokt-network/pocket/runtime/configs"
configTypes "github.com/pokt-network/pocket/runtime/configs/types"
"github.com/pokt-network/pocket/runtime/defaults"
"github.com/pokt-network/pocket/runtime/genesis"
"github.com/pokt-network/pocket/runtime/test_artifacts"
Expand Down Expand Up @@ -226,11 +227,11 @@ func TestNewManagerFromReaders(t *testing.T) {
HealthCheckPeriod: "5m",
},
P2P: &configs.P2PConfig{
PrivateKey: "c6c136d010d07d7f5e9944aa3594a10f9210dd3e26ebc1bc1516a6d957fd0df353ee26c82826694ffe1773d7b60d5f20dd9e91bdf8745544711bec5ff9c6fb4a",
ConsensusPort: 8080,
UseRainTree: true,
IsEmptyConnectionType: false,
MaxMempoolCount: 1e5,
PrivateKey: "c6c136d010d07d7f5e9944aa3594a10f9210dd3e26ebc1bc1516a6d957fd0df353ee26c82826694ffe1773d7b60d5f20dd9e91bdf8745544711bec5ff9c6fb4a",
ConsensusPort: 8080,
UseRainTree: true,
ConnectionType: configTypes.ConnectionType_TCPConnection,
MaxMempoolCount: 1e5,
},
Telemetry: &configs.TelemetryConfig{
Enabled: true,
Expand Down Expand Up @@ -272,11 +273,11 @@ func TestNewManagerFromReaders(t *testing.T) {
want: &Manager{
config: &configs.Config{
P2P: &configs.P2PConfig{
PrivateKey: "6fd0bc54cc2dd205eaf226eebdb0451629b321f11d279013ce6fdd5a33059256b2eda2232ffb2750bf761141f70f75a03a025f65b2b2b417c7f8b3c9ca91e8e4",
ConsensusPort: 8080,
UseRainTree: true,
IsEmptyConnectionType: false,
MaxMempoolCount: defaults.DefaultP2PMaxMempoolCount,
PrivateKey: "6fd0bc54cc2dd205eaf226eebdb0451629b321f11d279013ce6fdd5a33059256b2eda2232ffb2750bf761141f70f75a03a025f65b2b2b417c7f8b3c9ca91e8e4",
ConsensusPort: 8080,
UseRainTree: true,
ConnectionType: configTypes.ConnectionType_TCPConnection,
MaxMempoolCount: defaults.DefaultP2PMaxMempoolCount,
},
},
genesisState: expectedGenesis,
Expand Down