feat: clean up p2p & implement missing peering functionality
#2852
Conversation
zivkovicmilos
added
🌱 feature
github-actions
bot
added
the
📦 ⛰️ gno.land
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
zivkovicmilos
changed the title
seed peers) supportp2p & implement bootnode (seed peers) support
| @@ -36,10 +26,10 @@ type P2PConfig struct { | |||
| UPNP bool `json:"upnp" toml:"upnp" comment:"UPNP port forwarding"` | |||
|
|
|||
| // Maximum number of inbound peers | |||
| MaxNumInboundPeers int `json:"max_num_inbound_peers" toml:"max_num_inbound_peers" comment:"Maximum number of inbound peers"` | |||
| MaxNumInboundPeers uint64 `json:"max_num_inbound_peers" toml:"max_num_inbound_peers" comment:"Maximum number of inbound peers"` | |||
There was a problem hiding this comment.
Unsigned may be better than signed if we don't need to use patterns like -1. However, since we're constrained by the OS limits, a size of 64 may be excessive.
|
There is a leftover issue with this PR that I'm currently investigating, which is a hanging test somewhere in TM2. I will resolve this before opening the PR for reviews 🙏 |
|
Opening this up for an initial review 🙏 I have no idea why the |
There was a problem hiding this comment.
Do you check somewhere to not have a number of peers greater than max_num_outboud_peers?
I see some calls to NumOutbound(), but i don't think i saw this limitation.
I've made a test with a local node, and i'm failing to peer with test5.
A devnet cluster with this PR would be amazing to make some tests
| // peers. If another node asks it for addresses, it responds and disconnects. | ||
| // | ||
| // Does not work if the peer-exchange reactor is disabled. | ||
| SeedMode bool `json:"seed_mode" toml:"seed_mode" comment:"Seed mode, in which node constantly crawls the network and looks for\n peers. If another node asks it for addresses, it responds and disconnects.\n\n Does not work if the peer-exchange reactor is disabled."` |
There was a problem hiding this comment.
So there is no seed_mode anymore right ? So all pex node are seeding and constently crowling the network ?
There was a problem hiding this comment.
Seed mode and Pex are essentially tied together in Comet -- you cannot have seed mode if pex is false.
I've dropped these concepts, and adopted a much more simpler PeerExchange that does network crawling
| // the consensus machine | ||
| SwitchToConsensus(sm.State, int) | ||
| } | ||
| type SwitchToConsensusFn func(sm.State, int) |
There was a problem hiding this comment.
Should we add some comments here to understand why this is needed?
| @@ -1777,7 +1777,7 @@ func TestStateOutputVoteStats(t *testing.T) { | |||
|
|
|||
| cs, vss := randConsensusState(2) | |||
| // create dummy peer | |||
| peer := p2pmock.NewPeer(nil) | |||
| peer := &p2pmock.Peer{} | |||
There was a problem hiding this comment.
why one is a pointer and the other is not?
| @@ -162,7 +163,7 @@ func (memR *Reactor) Receive(chID byte, src p2p.Peer, msgBytes []byte) { | |||
|
|
|||
| switch msg := msg.(type) { | |||
| case *TxMessage: | |||
| peerID := memR.ids.GetForPeer(src) | |||
| peerID := memR.ids.GetForPeer(src.ID()) | |||
There was a problem hiding this comment.
A bit strange IMO. src is already a Peer with an ID, why do we have to call ids.GetForPeer to get the peerID?
| reactors := []nodeReactor{ | ||
| { | ||
| "MEMPOOL", mempoolReactor, | ||
| }, | ||
| { | ||
| "BLOCKCHAIN", bcReactor, | ||
| }, | ||
| { | ||
| "CONSENSUS", consensusReactor, | ||
| }, | ||
| } |
There was a problem hiding this comment.
should we use constants for these strings?
| if config.P2P.PeerExchange { | ||
| discoveryReactor = discovery.NewReactor() | ||
|
|
||
| discoveryReactor.SetLogger(logger.With("module", "discovery")) |
There was a problem hiding this comment.
should we use constant for these strings?
| } | ||
|
|
||
| // PeerSet has a (immutable) subset of the methods of PeerSet. | ||
| type PeerSet interface { |
There was a problem hiding this comment.
No error handling, so really difficult to implement that interface using other persistence than memory...
|
|
||
| // Grab a random peer, and engage | ||
| // them for peer discovery | ||
| peers := r.Switch.Peers().List() |
There was a problem hiding this comment.
How many elements peers are we expecting to have here? are they always on memory? I think that this interface should return an iterator instead.
There was a problem hiding this comment.
The code of discovery.go and Switch is a bit flaky in general, there is almost no error handling. For something like network connections, it is super important to have a robust system.
| Send(byte, []byte) bool | ||
| TrySend(byte, []byte) bool |
There was a problem hiding this comment.
No error handling, so the amount of info you can send to the callee is just yes/no. Also, what is the difference between Send and TrySend?
There was a problem hiding this comment.
I agree with @ajnavarro that it is hard to understand the difference between the two methods.
I guess bool is an alternative of the classic err pattern?
| Accept(context.Context, PeerBehavior) (Peer, error) | ||
|
|
||
| // Dial dials a peer, and returns it | ||
| Dial(context.Context, types.NetAddress, PeerBehavior) (Peer, error) |
There was a problem hiding this comment.
Why Dial is returning a Peer instead of something like a PeerConnection?
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):No automated checks match this pull request. ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
There was a problem hiding this comment.
It is not easy to review the whole PR since it involves a lot of changes. I generally understood that it aims to make all the p2p networking longer smooth, and the PR description and the README (tm2/pkg/p2p/README.md) explains the main goals perfectly.
I tend to agree with @ajnavarro that we should somehow merge&see, since it is not easy to imagine any possible corner case or error handling required.
Finally I think that this is a great improvement and apart from maybe more testing and tighter error handling, it should be considered as the cornerstone for any other future improvement related to the p2p networking.
| sw.peerBehavior.reactorsByCh[chID] = reactor | ||
| } | ||
|
|
||
| sw.reactors[name] = reactor |
There was a problem hiding this comment.
The MultiplexSwitch is holding the map of reactors and each reactor is setting the switch in turn.
Is the reactor existence tied to the application running, or they their removal is handled somewhere?
| This process repeats at specific intervals. It is worth nothing that if the limit of outbound peers is reached, the peer | ||
| dials have no effect. | ||
|
|
||
| #### Bootnodes (Seeds) |
There was a problem hiding this comment.
Is this differentiation present somehow in the code?
Because currently it is not easy to understand the purpose or the difference between a persistent peer and a boot/seed node (as long as boot and seed do mean the same thing here)
| There is a final service that runs alongside the previously-mentioned `Switch` services — peer discovery. | ||
|
|
||
| Every blockchain node needs an adequate amount of peers to communicate with, in order to ensure smooth functioning. For | ||
| validator nodes, they need to be *loosely connected* to at least 2/3+ of the validator set in order to participate and |
There was a problem hiding this comment.
Should the node have any awareness of that?
I mean since the network is loosely connected it is not straightforward for the node to know that is current topology network is connected enough to satisfy the 2/3.
| Send(byte, []byte) bool | ||
| TrySend(byte, []byte) bool |
There was a problem hiding this comment.
I agree with @ajnavarro that it is hard to understand the difference between the two methods.
I guess bool is an alternative of the classic err pattern?
| @@ -154,10 +153,14 @@ import ( | |||
| // } | |||
| // | |||
| // ``` | |||
| func NetInfo(ctx *rpctypes.Context) (*ctypes.ResultNetInfo, error) { | |||
| out, in, _ := p2pPeers.NumPeers() | |||
| func NetInfo(_ *rpctypes.Context) (*ctypes.ResultNetInfo, error) { | |||
There was a problem hiding this comment.
I don't understand why exactly this method, which is supposed to give information about the network, is actually updating the network information too.
Does it mean the network status is only updated upon an explicit request?
There was a problem hiding this comment.
The purpose of this methods is confusing. It is repeatedly reported testing purpose, so why they are not in an explicit _test file or package?
It seems that it is only used within test files.
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
p2pmodule -- 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
p2pcodebase, I've come to a couple of realizations:p2ppackage are sketchy to say the least, and not convincing at all that we were covering actual functionality we needed to cover.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
p2pmodule for the last 5+ years.I took this opportunity, ahead of the mainnet launch, to fill out a few checkboxes:
PHILOSOPHY.md.test4andtest5.I wanted to implement all of this, without breaking any existing TM2 functionality that relies on the
p2pmodule, 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:
SwitchandTransportimplementations. No more gazillion redundant checks, or expensive lookups, or convoluted APIs.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:
connpackage, 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.Peerabstraction is still the same, and TM2 modules interact with the peers in the same way as before (directly, asReactors). 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
p2pREADME. It outlines how thep2pmodule works on a core level, and highlights current challenges.After the README, open the
p2ppackage intm2/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
p2pconfig params are changed?Here is a complete list of changed
p2pconfiguration params, and the reasoning behind them:UPNP- UPNP port forwarding, it was completely unused, removed.PexReactor- peer exchange reactor enable-ment flag, renamed toPeerExchange, renameSeedMode- enabled network crawls, was tied toPexReactorbeingtrue. Useless flag, sincePeerExchangeexists, removedAllowDuplicateIP- useless flag to prevent same-IP dials, even outside previous dial "filters", removedHandshakeTimeout- excessive config option for setting an STS timeout, sane default is3s, removedDialTimeout- excessive config option for finishing a peer dial, sane default is3s, removedThe following config options were removed, as they related to testing, and were replaced by unit / integration tests.
TestDialFailTestFuzzTestFuzzConfigContributors' checklist...
BREAKING CHANGE: xxxmessage was included in the description