Skip to content

Commit

Permalink
feat: clean up p2p & implement missing peering functionality (#2852)
Browse files Browse the repository at this point in the history
## Description

Closes #2308 

There is a lot happening in this PR, so don't be discouraged by the
files changed.
I'll outline the way the PR should be reviewed, and give you pointers
along the way.

## What this PR set out to do

This PR initially set out to implement peer discovery in the TM2 `p2p`
module -- that's it, basically.

The change was meant to be minimal, and not involve larger changes.
After spending more time than I'd like to admit in the `p2p` codebase,
I've come to a couple of realizations:
- the code is insanely complex for what it needs to be doing.
- there are premature "optimizations" on every corner, with no real
reason for them.
- the unit tests in the `p2p` package are sketchy to say the least, and
not convincing at all that we were covering actual functionality we
needed to cover.
- there are random disconnection issues with larger clusters.

All of these are temporarily fine, and not blocking us.

But the pattern was there -- to add peer discovery, it would require
continuing the same pattern of code gymnastics present in the `p2p`
module for the last 5+ years.

I took this opportunity, ahead of the mainnet launch, to fill out a few
checkboxes:
- **simplify the code, trim _everything_ that's excess**. This is in
line with our project `PHILOSOPHY.md`.
- create a safety net in the form of integration tests, and unit tests,
that run _instantly_, and give us the confidence stuff actually works.
- make it easy peasy for us to debug future p2p problems, **when** they
arise (we already keep seeing them on existing testnets, like `test4`
and `test5`.

I wanted to implement all of this, without breaking any existing TM2
functionality that relies on the `p2p` module, or introducing additional
complexities.

## What this PR actually accomplished

I'm proud to say that this PR brings more than a few bells and whistles
to the table, in terms of TM2 improvements:
- _greatly_ simplified and faster `Switch` and `Transport`
implementations. No more gazillion redundant checks, or expensive
lookups, or convoluted APIs.
- a unit testing suite we can be proud of, and have confidence in. I
rewrote the entire testing suite for the module, because the old
implementation had severe limitations on mock-ability.
- peer discovery that works, and is not invasive. Goodbye random network
pockets, and hanging nodes!
- many bugs, and potential issues squashed and erased. Regressions
added, and passing.

For the sake of not making groundbreaking changes ahead of mainnet, I
didn't touch a few things, and this will be evident from the code:
- I didn't touch the `conn` package, or how the multiplex connections
are established and maintained (or the STS implementation) -- this would
be too much, and require an exponential amount of time to get right.
- the `Peer` abstraction is still the same, and TM2 modules interact
with the peers in the same way as before (directly, as `Reactor`s). I've
outlined the issues with this in the README, so check it out.

In retrospect, I should've limited the scope of this PR by a lot. At the
time I was at the mid-way point, I committed fully to leaving this
module in a better state than I found it in, rather than leave
additional tech debt for future cleanup.

The other primary goal of this PR is to scope out changes needed to
upgrade the networking layer implementation into utilizing a stack like
libp2p. I am happy to say that we have 0 limitations in terms of p2p
functionality to make the switch. We're just bound by time. This upgrade
is scheduled for after the mainnet MVP launch 🤞

## How do I review this PR?

There is no point in looking at the older implementation, and trying to
figure out the changes from there.

There are just too many, and it can get overwhelming quickly.

Instead, as the **first step** -- read the new `p2p`
[README](https://github.com/gnolang/gno/blob/dev/zivkovicmilos/bootnodes/tm2/pkg/p2p/README.md).
It outlines how the `p2p` module works on a core level, and highlights
current challenges.

After the README, open the `p2p` package in `tm2/pkg/p2p`, and start
looking at the implementation from there. Leave comments on things that
are unclear, or can be improved. I'll try to answer and give as much
context as I can.

## What `p2p` config params are changed?

Here is a complete list of changed `p2p` configuration params, and the
reasoning behind them:

- `UPNP` - UPNP port forwarding, it was completely unused, **removed**.
- `PexReactor` - peer exchange reactor enable-ment flag, renamed to
`PeerExchange`, **rename**
- `SeedMode` - enabled network crawls, was tied to `PexReactor` being
`true`. Useless flag, since `PeerExchange` exists, **removed**
- `AllowDuplicateIP` - useless flag to prevent same-IP dials, even
outside previous dial "filters", **removed**
- `HandshakeTimeout` - excessive config option for setting an STS
timeout, sane default is `3s`, **removed**
- `DialTimeout` - excessive config option for finishing a peer dial,
sane default is `3s`, **removed**

The following config options were **removed**, as they related to
testing, and were replaced by unit / integration tests.
- `TestDialFail`
- `TestFuzz`
- `TestFuzzConfig`

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
  • Loading branch information
zivkovicmilos authored and albttx committed Jan 10, 2025
1 parent 0a2b463 commit 62ac5e8
Show file tree
Hide file tree
Showing 118 changed files with 8,091 additions and 5,833 deletions.
1 change: 1 addition & 0 deletions .github/golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ issues:
- gosec # Disabled linting of weak number generators
- makezero # Disabled linting of intentional slice appends
- goconst # Disabled linting of common mnemonics and test case strings
- unused # Disabled linting of unused mock methods
- path: _\.gno
linters:
- errorlint # Disabled linting of error comparisons, because of lacking std lib support
1 change: 1 addition & 0 deletions contribs/gnodev/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ require (
github.com/rogpeppe/go-internal v1.12.0 // indirect
github.com/rs/cors v1.11.1 // indirect
github.com/rs/xid v1.6.0 // indirect
github.com/sig-0/insertion-queue v0.0.0-20241004125609-6b3ca841346b // indirect
github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7 // indirect
github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect
github.com/yuin/goldmark v1.7.2 // indirect
Expand Down
2 changes: 2 additions & 0 deletions contribs/gnodev/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions contribs/gnofaucet/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ require (
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/rs/cors v1.11.1 // indirect
github.com/rs/xid v1.6.0 // indirect
github.com/sig-0/insertion-queue v0.0.0-20241004125609-6b3ca841346b // indirect
go.opentelemetry.io/otel v1.29.0 // indirect
go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc v1.29.0 // indirect
go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp v1.29.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions contribs/gnofaucet/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions contribs/gnogenesis/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ require (
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/rs/cors v1.11.1 // indirect
github.com/rs/xid v1.6.0 // indirect
github.com/sig-0/insertion-queue v0.0.0-20241004125609-6b3ca841346b // indirect
github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7 // indirect
github.com/zondax/hid v0.9.2 // indirect
github.com/zondax/ledger-go v0.14.3 // indirect
Expand All @@ -49,6 +50,7 @@ require (
golang.org/x/exp v0.0.0-20240613232115-7f521ea00fb8 // indirect
golang.org/x/mod v0.20.0 // indirect
golang.org/x/net v0.28.0 // indirect
golang.org/x/sync v0.8.0 // indirect
golang.org/x/sys v0.24.0 // indirect
golang.org/x/term v0.23.0 // indirect
golang.org/x/text v0.17.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions contribs/gnogenesis/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions contribs/gnohealth/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ require (
github.com/peterbourgon/ff/v3 v3.4.0 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/rs/xid v1.6.0 // indirect
github.com/sig-0/insertion-queue v0.0.0-20241004125609-6b3ca841346b // indirect
github.com/stretchr/testify v1.9.0 // indirect
go.opentelemetry.io/otel v1.29.0 // indirect
go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc v1.29.0 // indirect
Expand All @@ -34,6 +35,7 @@ require (
golang.org/x/exp v0.0.0-20240613232115-7f521ea00fb8 // indirect
golang.org/x/mod v0.20.0 // indirect
golang.org/x/net v0.28.0 // indirect
golang.org/x/sync v0.8.0 // indirect
golang.org/x/sys v0.24.0 // indirect
golang.org/x/term v0.23.0 // indirect
golang.org/x/text v0.17.0 // indirect
Expand Down
4 changes: 4 additions & 0 deletions contribs/gnohealth/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions contribs/gnokeykc/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ require (
github.com/pkg/errors v0.9.1 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/rs/xid v1.6.0 // indirect
github.com/sig-0/insertion-queue v0.0.0-20241004125609-6b3ca841346b // indirect
github.com/stretchr/testify v1.9.0 // indirect
github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7 // indirect
github.com/zondax/hid v0.9.2 // indirect
Expand All @@ -52,6 +53,7 @@ require (
golang.org/x/exp v0.0.0-20240613232115-7f521ea00fb8 // indirect
golang.org/x/mod v0.20.0 // indirect
golang.org/x/net v0.28.0 // indirect
golang.org/x/sync v0.8.0 // indirect
golang.org/x/sys v0.24.0 // indirect
golang.org/x/term v0.23.0 // indirect
golang.org/x/text v0.17.0 // indirect
Expand Down
4 changes: 4 additions & 0 deletions contribs/gnokeykc/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions contribs/gnomigrate/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ require (
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/rs/cors v1.11.1 // indirect
github.com/rs/xid v1.6.0 // indirect
github.com/sig-0/insertion-queue v0.0.0-20241004125609-6b3ca841346b // indirect
github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7 // indirect
go.etcd.io/bbolt v1.3.11 // indirect
go.opentelemetry.io/otel v1.29.0 // indirect
Expand All @@ -44,6 +45,7 @@ require (
golang.org/x/exp v0.0.0-20240613232115-7f521ea00fb8 // indirect
golang.org/x/mod v0.20.0 // indirect
golang.org/x/net v0.28.0 // indirect
golang.org/x/sync v0.8.0 // indirect
golang.org/x/sys v0.24.0 // indirect
golang.org/x/term v0.23.0 // indirect
golang.org/x/text v0.17.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions contribs/gnomigrate/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

54 changes: 3 additions & 51 deletions gno.land/cmd/gnoland/config_get_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -289,14 +289,6 @@ func TestConfig_Get_Base(t *testing.T) {
},
true,
},
{
"filter peers flag fetched",
"filter_peers",
func(loadedCfg *config.Config, value []byte) {
assert.Equal(t, loadedCfg.FilterPeers, unmarshalJSONCommon[bool](t, value))
},
false,
},
}

verifyGetTestTableCommon(t, testTable)
Expand Down Expand Up @@ -616,27 +608,19 @@ func TestConfig_Get_P2P(t *testing.T) {
},
true,
},
{
"upnp toggle",
"p2p.upnp",
func(loadedCfg *config.Config, value []byte) {
assert.Equal(t, loadedCfg.P2P.UPNP, unmarshalJSONCommon[bool](t, value))
},
false,
},
{
"max inbound peers",
"p2p.max_num_inbound_peers",
func(loadedCfg *config.Config, value []byte) {
assert.Equal(t, loadedCfg.P2P.MaxNumInboundPeers, unmarshalJSONCommon[int](t, value))
assert.Equal(t, loadedCfg.P2P.MaxNumInboundPeers, unmarshalJSONCommon[uint64](t, value))
},
false,
},
{
"max outbound peers",
"p2p.max_num_outbound_peers",
func(loadedCfg *config.Config, value []byte) {
assert.Equal(t, loadedCfg.P2P.MaxNumOutboundPeers, unmarshalJSONCommon[int](t, value))
assert.Equal(t, loadedCfg.P2P.MaxNumOutboundPeers, unmarshalJSONCommon[uint64](t, value))
},
false,
},
Expand Down Expand Up @@ -676,15 +660,7 @@ func TestConfig_Get_P2P(t *testing.T) {
"pex reactor toggle",
"p2p.pex",
func(loadedCfg *config.Config, value []byte) {
assert.Equal(t, loadedCfg.P2P.PexReactor, unmarshalJSONCommon[bool](t, value))
},
false,
},
{
"seed mode",
"p2p.seed_mode",
func(loadedCfg *config.Config, value []byte) {
assert.Equal(t, loadedCfg.P2P.SeedMode, unmarshalJSONCommon[bool](t, value))
assert.Equal(t, loadedCfg.P2P.PeerExchange, unmarshalJSONCommon[bool](t, value))
},
false,
},
Expand All @@ -704,30 +680,6 @@ func TestConfig_Get_P2P(t *testing.T) {
},
true,
},
{
"allow duplicate IP",
"p2p.allow_duplicate_ip",
func(loadedCfg *config.Config, value []byte) {
assert.Equal(t, loadedCfg.P2P.AllowDuplicateIP, unmarshalJSONCommon[bool](t, value))
},
false,
},
{
"handshake timeout",
"p2p.handshake_timeout",
func(loadedCfg *config.Config, value []byte) {
assert.Equal(t, loadedCfg.P2P.HandshakeTimeout, unmarshalJSONCommon[time.Duration](t, value))
},
false,
},
{
"dial timeout",
"p2p.dial_timeout",
func(loadedCfg *config.Config, value []byte) {
assert.Equal(t, loadedCfg.P2P.DialTimeout, unmarshalJSONCommon[time.Duration](t, value))
},
false,
},
}

verifyGetTestTableCommon(t, testTable)
Expand Down
74 changes: 1 addition & 73 deletions gno.land/cmd/gnoland/config_set_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,19 +244,6 @@ func TestConfig_Set_Base(t *testing.T) {
assert.Equal(t, value, loadedCfg.ProfListenAddress)
},
},
{
"filter peers flag updated",
[]string{
"filter_peers",
"true",
},
func(loadedCfg *config.Config, value string) {
boolVal, err := strconv.ParseBool(value)
require.NoError(t, err)

assert.Equal(t, boolVal, loadedCfg.FilterPeers)
},
},
}

verifySetTestTableCommon(t, testTable)
Expand Down Expand Up @@ -505,19 +492,6 @@ func TestConfig_Set_P2P(t *testing.T) {
assert.Equal(t, value, loadedCfg.P2P.PersistentPeers)
},
},
{
"upnp toggle updated",
[]string{
"p2p.upnp",
"false",
},
func(loadedCfg *config.Config, value string) {
boolVal, err := strconv.ParseBool(value)
require.NoError(t, err)

assert.Equal(t, boolVal, loadedCfg.P2P.UPNP)
},
},
{
"max inbound peers updated",
[]string{
Expand Down Expand Up @@ -588,20 +562,7 @@ func TestConfig_Set_P2P(t *testing.T) {
boolVal, err := strconv.ParseBool(value)
require.NoError(t, err)

assert.Equal(t, boolVal, loadedCfg.P2P.PexReactor)
},
},
{
"seed mode updated",
[]string{
"p2p.seed_mode",
"false",
},
func(loadedCfg *config.Config, value string) {
boolVal, err := strconv.ParseBool(value)
require.NoError(t, err)

assert.Equal(t, boolVal, loadedCfg.P2P.SeedMode)
assert.Equal(t, boolVal, loadedCfg.P2P.PeerExchange)
},
},
{
Expand All @@ -614,39 +575,6 @@ func TestConfig_Set_P2P(t *testing.T) {
assert.Equal(t, value, loadedCfg.P2P.PrivatePeerIDs)
},
},
{
"allow duplicate IPs updated",
[]string{
"p2p.allow_duplicate_ip",
"false",
},
func(loadedCfg *config.Config, value string) {
boolVal, err := strconv.ParseBool(value)
require.NoError(t, err)

assert.Equal(t, boolVal, loadedCfg.P2P.AllowDuplicateIP)
},
},
{
"handshake timeout updated",
[]string{
"p2p.handshake_timeout",
"1s",
},
func(loadedCfg *config.Config, value string) {
assert.Equal(t, value, loadedCfg.P2P.HandshakeTimeout.String())
},
},
{
"dial timeout updated",
[]string{
"p2p.dial_timeout",
"1s",
},
func(loadedCfg *config.Config, value string) {
assert.Equal(t, value, loadedCfg.P2P.DialTimeout.String())
},
},
}

verifySetTestTableCommon(t, testTable)
Expand Down
6 changes: 3 additions & 3 deletions gno.land/cmd/gnoland/secrets_common.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"github.com/gnolang/gno/tm2/pkg/amino"
"github.com/gnolang/gno/tm2/pkg/bft/privval"
"github.com/gnolang/gno/tm2/pkg/crypto"
"github.com/gnolang/gno/tm2/pkg/p2p"
"github.com/gnolang/gno/tm2/pkg/p2p/types"
)

var (
Expand Down Expand Up @@ -54,7 +54,7 @@ func isValidDirectory(dirPath string) bool {
}

type secretData interface {
privval.FilePVKey | privval.FilePVLastSignState | p2p.NodeKey
privval.FilePVKey | privval.FilePVLastSignState | types.NodeKey
}

// readSecretData reads the secret data from the given path
Expand Down Expand Up @@ -145,7 +145,7 @@ func validateValidatorStateSignature(
}

// validateNodeKey validates the node's p2p key
func validateNodeKey(key *p2p.NodeKey) error {
func validateNodeKey(key *types.NodeKey) error {
if key.PrivKey == nil {
return errInvalidNodeKey
}
Expand Down
Loading

0 comments on commit 62ac5e8

Please sign in to comment.