diff --git a/tm2/pkg/bft/node/node.go b/tm2/pkg/bft/node/node.go index 08f1b3c58f9..7f16d6780c7 100644 --- a/tm2/pkg/bft/node/node.go +++ b/tm2/pkg/bft/node/node.go @@ -12,8 +12,6 @@ import ( "sync" "time" - goErrors "errors" - "github.com/gnolang/gno/tm2/pkg/bft/appconn" "github.com/gnolang/gno/tm2/pkg/bft/state/eventstore/file" "github.com/gnolang/gno/tm2/pkg/p2p/conn" @@ -604,12 +602,10 @@ func (n *Node) OnStart() error { } // Start the transport. - lAddr := n.config.P2P.ExternalAddress - if lAddr == "" { - lAddr = n.config.P2P.ListenAddress - } + // The listen address for the transport needs to be an address within reach of the machine NIC + listenAddress := p2pTypes.NetAddressString(n.nodeKey.ID(), n.config.P2P.ListenAddress) - addr, err := p2pTypes.NewNetAddressFromString(p2pTypes.NetAddressString(n.nodeKey.ID(), lAddr)) + addr, err := p2pTypes.NewNetAddressFromString(listenAddress) if err != nil { return fmt.Errorf("unable to parse network address, %w", err) } @@ -903,7 +899,7 @@ func makeNodeInfo( nodeInfo := p2pTypes.NodeInfo{ VersionSet: vset, - PeerID: nodeKey.ID(), + NetAddress: nil, // The shared address depends on the configuration Network: genDoc.ChainID, Version: version.Version, Channels: []byte{ @@ -918,13 +914,44 @@ func makeNodeInfo( }, } + // Make sure the discovery channel is shared with peers + // in case peer discovery is enabled if config.P2P.PeerExchange { nodeInfo.Channels = append(nodeInfo.Channels, discovery.Channel) } + // Grab the supplied listen address. + // This address needs to be valid, but it can be unspecified. + // If the listen address is unspecified (port / IP unbound), + // then this address cannot be used by peers for dialing + addr, err := p2pTypes.NewNetAddressFromString( + p2pTypes.NetAddressString(nodeKey.ID(), config.P2P.ListenAddress), + ) + if err != nil { + return p2pTypes.NodeInfo{}, fmt.Errorf("unable to parse network address, %w", err) + } + + // Use the transport listen address as the advertised address + nodeInfo.NetAddress = addr + + // Prepare the advertised dial address (if any) + // for the node, which other peers can use to dial + if config.P2P.ExternalAddress != "" { + addr, err = p2pTypes.NewNetAddressFromString( + p2pTypes.NetAddressString( + nodeKey.ID(), + config.P2P.ExternalAddress, + ), + ) + if err != nil { + return p2pTypes.NodeInfo{}, fmt.Errorf("invalid p2p external address: %w", err) + } + + nodeInfo.NetAddress = addr + } + // Validate the node info - err := nodeInfo.Validate() - if err != nil && !goErrors.Is(err, p2pTypes.ErrUnspecifiedIP) { + if err := nodeInfo.Validate(); err != nil { return p2pTypes.NodeInfo{}, fmt.Errorf("unable to validate node info, %w", err) } diff --git a/tm2/pkg/internal/p2p/p2p.go b/tm2/pkg/internal/p2p/p2p.go index 1e650e0cd25..0c8f1529b85 100644 --- a/tm2/pkg/internal/p2p/p2p.go +++ b/tm2/pkg/internal/p2p/p2p.go @@ -70,12 +70,12 @@ func MakeConnectedPeers( VersionSet: versionset.VersionSet{ versionset.VersionInfo{Name: "p2p", Version: "v0.0.0"}, }, - PeerID: key.ID(), - Network: "testing", - Software: "p2ptest", - Version: "v1.2.3-rc.0-deadbeef", - Channels: cfg.Channels, - Moniker: fmt.Sprintf("node-%d", index), + NetAddress: addr, + Network: "testing", + Software: "p2ptest", + Version: "v1.2.3-rc.0-deadbeef", + Channels: cfg.Channels, + Moniker: fmt.Sprintf("node-%d", index), Other: p2pTypes.NodeInfoOther{ TxIndex: "off", RPCAddress: fmt.Sprintf("127.0.0.1:%d", 0), @@ -231,7 +231,7 @@ func (mp *Peer) TrySend(_ byte, _ []byte) bool { return true } func (mp *Peer) Send(_ byte, _ []byte) bool { return true } func (mp *Peer) NodeInfo() p2pTypes.NodeInfo { return p2pTypes.NodeInfo{ - PeerID: mp.id, + NetAddress: mp.addr, } } func (mp *Peer) Status() conn.ConnectionStatus { return conn.ConnectionStatus{} } diff --git a/tm2/pkg/p2p/discovery/discovery.go b/tm2/pkg/p2p/discovery/discovery.go index d884b118c75..7a9da3726c0 100644 --- a/tm2/pkg/p2p/discovery/discovery.go +++ b/tm2/pkg/p2p/discovery/discovery.go @@ -160,7 +160,7 @@ func (r *Reactor) Receive(chID byte, peer p2p.PeerConn, msgBytes []byte) { // Validate the message if err := msg.ValidateBasic(); err != nil { - r.Logger.Error("unable to validate discovery message", "err", err) + r.Logger.Warn("unable to validate discovery message", "err", err) return } @@ -168,7 +168,7 @@ func (r *Reactor) Receive(chID byte, peer p2p.PeerConn, msgBytes []byte) { switch msg := msg.(type) { case *Request: if err := r.handleDiscoveryRequest(peer); err != nil { - r.Logger.Error("unable to handle discovery request", "err", err) + r.Logger.Warn("unable to handle discovery request", "err", err) } case *Response: // Make the peers available for dialing on the switch @@ -186,9 +186,21 @@ func (r *Reactor) handleDiscoveryRequest(peer p2p.PeerConn) error { peers = make([]*types.NetAddress, 0, len(localPeers)) ) - // Exclude the private peers from being shared + // Exclude the private peers from being shared, + // as well as peers who are not dialable localPeers = slices.DeleteFunc(localPeers, func(p p2p.PeerConn) bool { - return p.IsPrivate() + var ( + // Private peers are peers whose information is kept private to the node + privatePeer = p.IsPrivate() + // The reason we don't validate the net address with .Routable() + // is because of legacy logic that supports local loopbacks as advertised + // peer addresses. Introducing a .Routable() constraint will filter all + // local loopback addresses shared by peers, and will cause local deployments + // (and unit test deployments) to break and require additional setup + invalidDialAddress = p.NodeInfo().DialAddress().Validate() != nil + ) + + return privatePeer || invalidDialAddress }) // Check if there is anything to share, @@ -207,7 +219,8 @@ func (r *Reactor) handleDiscoveryRequest(peer p2p.PeerConn) error { } for _, p := range localPeers { - peers = append(peers, p.SocketAddr()) + // Make sure only routable peers are shared + peers = append(peers, p.NodeInfo().DialAddress()) } // Create the response, and marshal diff --git a/tm2/pkg/p2p/discovery/discovery_test.go b/tm2/pkg/p2p/discovery/discovery_test.go index 17404e6039a..91741c648db 100644 --- a/tm2/pkg/p2p/discovery/discovery_test.go +++ b/tm2/pkg/p2p/discovery/discovery_test.go @@ -166,7 +166,7 @@ func TestReactor_DiscoveryResponse(t *testing.T) { slices.ContainsFunc(resp.Peers, func(addr *types.NetAddress) bool { for _, localP := range peers { - if localP.SocketAddr().Equals(*addr) { + if localP.NodeInfo().DialAddress().Equals(*addr) { return true } } @@ -317,7 +317,7 @@ func TestReactor_DiscoveryResponse(t *testing.T) { slices.ContainsFunc(resp.Peers, func(addr *types.NetAddress) bool { for _, localP := range peers { - if localP.SocketAddr().Equals(*addr) { + if localP.NodeInfo().DialAddress().Equals(*addr) { return true } } @@ -373,7 +373,7 @@ func TestReactor_DiscoveryResponse(t *testing.T) { peerAddrs := make([]*types.NetAddress, 0, len(peers)) for _, p := range peers { - peerAddrs = append(peerAddrs, p.SocketAddr()) + peerAddrs = append(peerAddrs, p.NodeInfo().DialAddress()) } // Prepare the message diff --git a/tm2/pkg/p2p/mock/peer.go b/tm2/pkg/p2p/mock/peer.go index e5a01952831..5be34121924 100644 --- a/tm2/pkg/p2p/mock/peer.go +++ b/tm2/pkg/p2p/mock/peer.go @@ -57,7 +57,7 @@ func GeneratePeers(t *testing.T, count int) []*Peer { }, NodeInfoFn: func() types.NodeInfo { return types.NodeInfo{ - PeerID: key.ID(), + NetAddress: addr, } }, SocketAddrFn: func() *types.NetAddress { diff --git a/tm2/pkg/p2p/peer.go b/tm2/pkg/p2p/peer.go index 135bf4b250c..dcca81ca097 100644 --- a/tm2/pkg/p2p/peer.go +++ b/tm2/pkg/p2p/peer.go @@ -160,7 +160,7 @@ func (p *peer) OnStop() { // ID returns the peer's ID - the hex encoded hash of its pubkey. func (p *peer) ID() types.ID { - return p.nodeInfo.PeerID + return p.nodeInfo.ID() } // NodeInfo returns a copy of the peer's NodeInfo. diff --git a/tm2/pkg/p2p/peer_test.go b/tm2/pkg/p2p/peer_test.go index a74ea9e96a4..75f5172ee66 100644 --- a/tm2/pkg/p2p/peer_test.go +++ b/tm2/pkg/p2p/peer_test.go @@ -243,7 +243,9 @@ func TestPeer_Properties(t *testing.T) { }, }, nodeInfo: types.NodeInfo{ - PeerID: id, + NetAddress: &types.NetAddress{ + ID: id, + }, }, connInfo: &ConnInfo{ Outbound: testCase.outbound, diff --git a/tm2/pkg/p2p/switch.go b/tm2/pkg/p2p/switch.go index 7784c1f3989..c96e429973e 100644 --- a/tm2/pkg/p2p/switch.go +++ b/tm2/pkg/p2p/switch.go @@ -407,57 +407,76 @@ func (sw *MultiplexSwitch) runRedialLoop(ctx context.Context) { peersToDial = make([]*types.NetAddress, 0) ) + // Gather addresses of persistent peers that are missing or + // not already in the dial queue sw.persistentPeers.Range(func(key, value any) bool { var ( id = key.(types.ID) addr = value.(*types.NetAddress) ) - // Check if the peer is part of the peer set - // or is scheduled for dialing - if peers.Has(id) || sw.dialQueue.Has(addr) { - return true + if !peers.Has(id) && !sw.dialQueue.Has(addr) { + peersToDial = append(peersToDial, addr) } - peersToDial = append(peersToDial, addr) - return true }) if len(peersToDial) == 0 { - // No persistent peers are missing + // No persistent peers need dialing return } - // Calculate the dial items + // Prepare dial items with the appropriate backoff dialItems := make([]dial.Item, 0, len(peersToDial)) - for _, p := range peersToDial { - item := getBackoffItem(p.ID) + for _, addr := range peersToDial { + item := getBackoffItem(addr.ID) + if item == nil { - dialItem := dial.Item{ - Time: time.Now(), - Address: p, - } + // First attempt + now := time.Now() + + dialItems = append(dialItems, + dial.Item{ + Time: now, + Address: addr, + }, + ) - dialItems = append(dialItems, dialItem) - setBackoffItem(p.ID, &backoffItem{dialItem.Time, 0}) + setBackoffItem(addr.ID, &backoffItem{ + lastDialTime: now, + attempts: 0, + }) continue } - setBackoffItem(p.ID, &backoffItem{ - lastDialTime: time.Now().Add( + // Subsequent attempt: apply backoff + var ( + attempts = item.attempts + 1 + dialTime = time.Now().Add( calculateBackoff( item.attempts, time.Second, 10*time.Minute, ), - ), - attempts: item.attempts + 1, + ) + ) + + dialItems = append(dialItems, + dial.Item{ + Time: dialTime, + Address: addr, + }, + ) + + setBackoffItem(addr.ID, &backoffItem{ + lastDialTime: dialTime, + attempts: attempts, }) } - // Add the peers to the dial queue + // Add these items to the dial queue sw.dialItems(dialItems...) } diff --git a/tm2/pkg/p2p/switch_test.go b/tm2/pkg/p2p/switch_test.go index b10ab3faba5..e5f472cc28e 100644 --- a/tm2/pkg/p2p/switch_test.go +++ b/tm2/pkg/p2p/switch_test.go @@ -727,7 +727,7 @@ func TestMultiplexSwitch_DialPeers(t *testing.T) { // as the transport (node) p.NodeInfoFn = func() types.NodeInfo { return types.NodeInfo{ - PeerID: addr.ID, + NetAddress: &addr, } } diff --git a/tm2/pkg/p2p/transport.go b/tm2/pkg/p2p/transport.go index 255fa60152b..3d64a48f437 100644 --- a/tm2/pkg/p2p/transport.go +++ b/tm2/pkg/p2p/transport.go @@ -139,6 +139,7 @@ func (mt *MultiplexTransport) Close() error { } mt.cancelFn() + return mt.listener.Close() } diff --git a/tm2/pkg/p2p/transport_test.go b/tm2/pkg/p2p/transport_test.go index 5ec4efda1ad..840eb974e76 100644 --- a/tm2/pkg/p2p/transport_test.go +++ b/tm2/pkg/p2p/transport_test.go @@ -237,7 +237,7 @@ func TestMultiplexTransport_Accept(t *testing.T) { ni := types.NodeInfo{ Network: network, // common network - PeerID: id, + NetAddress: na, Version: "v1.0.0-rc.0", Moniker: fmt.Sprintf("node-%d", index), VersionSet: make(versionset.VersionSet, 0), // compatible version set @@ -317,7 +317,7 @@ func TestMultiplexTransport_Accept(t *testing.T) { ni := types.NodeInfo{ Network: chainID, - PeerID: id, + NetAddress: na, Version: "v1.0.0-rc.0", Moniker: fmt.Sprintf("node-%d", index), VersionSet: make(versionset.VersionSet, 0), // compatible version set @@ -389,7 +389,7 @@ func TestMultiplexTransport_Accept(t *testing.T) { ni := types.NodeInfo{ Network: network, // common network - PeerID: key.ID(), + NetAddress: na, Version: "v1.0.0-rc.0", Moniker: fmt.Sprintf("node-%d", index), VersionSet: make(versionset.VersionSet, 0), // compatible version set @@ -467,7 +467,7 @@ func TestMultiplexTransport_Accept(t *testing.T) { ni := types.NodeInfo{ Network: network, // common network - PeerID: key.ID(), + NetAddress: na, Version: "v1.0.0-rc.0", Moniker: fmt.Sprintf("node-%d", index), VersionSet: make(versionset.VersionSet, 0), // compatible version set diff --git a/tm2/pkg/p2p/types/node_info.go b/tm2/pkg/p2p/types/node_info.go index 8452cb43cb8..4080ff2d8aa 100644 --- a/tm2/pkg/p2p/types/node_info.go +++ b/tm2/pkg/p2p/types/node_info.go @@ -14,7 +14,6 @@ const ( ) var ( - ErrInvalidPeerID = errors.New("invalid peer ID") ErrInvalidVersion = errors.New("invalid node version") ErrInvalidMoniker = errors.New("invalid node moniker") ErrInvalidRPCAddress = errors.New("invalid node RPC address") @@ -30,8 +29,8 @@ type NodeInfo struct { // Set of protocol versions VersionSet versionset.VersionSet `json:"version_set"` - // Unique peer identifier - PeerID ID `json:"id"` + // The advertised net address of the peer + NetAddress *NetAddress `json:"net_address"` // Check compatibility. // Channels are HexBytes so easier to read as JSON @@ -54,12 +53,27 @@ type NodeInfoOther struct { // Validate checks the self-reported NodeInfo is safe. // It returns an error if there // are too many Channels, if there are any duplicate Channels, -// if the ListenAddr is malformed, or if the ListenAddr is a host name +// if the NetAddress is malformed, or if the NetAddress is a host name // that can not be resolved to some IP func (info NodeInfo) Validate() error { - // Validate the ID - if err := info.PeerID.Validate(); err != nil { - return fmt.Errorf("%w, %w", ErrInvalidPeerID, err) + // There are a few checks that need to be performed when validating + // the node info's net address: + // - the ID needs to be valid + // - the FORMAT of the net address needs to be valid + // + // The key nuance here is that the net address is not being validated + // for its "dialability", but whether it's of the correct format. + // + // Unspecified IPs are tolerated (ex. 0.0.0.0 or ::), + // because of legacy logic that assumes node info + // can have unspecified IPs (ex. no external address is set, use + // the listen address which is bound to 0.0.0.0). + // + // These types of IPs are caught during the + // real peer info sharing process, since they are undialable + _, err := NewNetAddressFromString(NetAddressString(info.NetAddress.ID, info.NetAddress.DialString())) + if err != nil { + return fmt.Errorf("invalid net address in node info, %w", err) } // Validate Version @@ -100,7 +114,12 @@ func (info NodeInfo) Validate() error { // ID returns the local node ID func (info NodeInfo) ID() ID { - return info.PeerID + return info.NetAddress.ID +} + +// DialAddress is the advertised peer dial address (share-able) +func (info NodeInfo) DialAddress() *NetAddress { + return info.NetAddress } // CompatibleWith checks if two NodeInfo are compatible with each other. diff --git a/tm2/pkg/p2p/types/node_info_test.go b/tm2/pkg/p2p/types/node_info_test.go index d03d77e608f..575d8ae5fbd 100644 --- a/tm2/pkg/p2p/types/node_info_test.go +++ b/tm2/pkg/p2p/types/node_info_test.go @@ -2,23 +2,43 @@ package types import ( "fmt" + "net" "testing" + "github.com/gnolang/gno/tm2/pkg/crypto" "github.com/gnolang/gno/tm2/pkg/versionset" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNodeInfo_Validate(t *testing.T) { t.Parallel() + generateNetAddress := func() *NetAddress { + var ( + key = GenerateNodeKey() + address = "127.0.0.1:8080" + ) + + tcpAddr, err := net.ResolveTCPAddr("tcp", address) + require.NoError(t, err) + + addr, err := NewNetAddress(key.ID(), tcpAddr) + require.NoError(t, err) + + return addr + } + t.Run("invalid peer ID", func(t *testing.T) { t.Parallel() info := &NodeInfo{ - PeerID: "", // zero + NetAddress: &NetAddress{ + ID: "", // zero + }, } - assert.ErrorIs(t, info.Validate(), ErrInvalidPeerID) + assert.ErrorIs(t, info.Validate(), crypto.ErrZeroID) }) t.Run("invalid version", func(t *testing.T) { @@ -47,8 +67,8 @@ func TestNodeInfo_Validate(t *testing.T) { t.Parallel() info := &NodeInfo{ - PeerID: GenerateNodeKey().ID(), - Version: testCase.version, + NetAddress: generateNetAddress(), + Version: testCase.version, } assert.ErrorIs(t, info.Validate(), ErrInvalidVersion) @@ -86,8 +106,8 @@ func TestNodeInfo_Validate(t *testing.T) { t.Parallel() info := &NodeInfo{ - PeerID: GenerateNodeKey().ID(), - Moniker: testCase.moniker, + NetAddress: generateNetAddress(), + Moniker: testCase.moniker, } assert.ErrorIs(t, info.Validate(), ErrInvalidMoniker) @@ -121,8 +141,8 @@ func TestNodeInfo_Validate(t *testing.T) { t.Parallel() info := &NodeInfo{ - PeerID: GenerateNodeKey().ID(), - Moniker: "valid moniker", + NetAddress: generateNetAddress(), + Moniker: "valid moniker", Other: NodeInfoOther{ RPCAddress: testCase.rpcAddress, }, @@ -162,9 +182,9 @@ func TestNodeInfo_Validate(t *testing.T) { t.Parallel() info := &NodeInfo{ - PeerID: GenerateNodeKey().ID(), - Moniker: "valid moniker", - Channels: testCase.channels, + NetAddress: generateNetAddress(), + Moniker: "valid moniker", + Channels: testCase.channels, } assert.ErrorIs(t, info.Validate(), testCase.expectedErr) @@ -176,9 +196,9 @@ func TestNodeInfo_Validate(t *testing.T) { t.Parallel() info := &NodeInfo{ - PeerID: GenerateNodeKey().ID(), - Moniker: "valid moniker", - Channels: []byte{10, 20, 30}, + NetAddress: generateNetAddress(), + Moniker: "valid moniker", + Channels: []byte{10, 20, 30}, Other: NodeInfoOther{ RPCAddress: "0.0.0.0:26657", },