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

multi: Add getaddrv2 and addrv2. #2627

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

multi: Add getaddrv2 and addrv2. #2627

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
650a935
addrmgr: Track network address types.
sefbkn Apr 4, 2021
77e2118
addrmgr: Add network address type filter.
sefbkn Apr 4, 2021
2d13ac7
peer: Partially decouple peer from wire.
sefbkn Apr 4, 2021
a32aa05
addrmgr: Remove DNS lookups from address manager.
sefbkn Mar 12, 2021
ddf3d7d
addrmgr: Add TORv3 network address type.
sefbkn Apr 4, 2021
27215a0
multi: Remove TORv2 network address type.
sefbkn Jan 25, 2022
404ec69
peer/wire: Add addrv2 and getaddrv2 message types.
sefbkn Feb 23, 2021
c8b98b1
connmgr: Allow seeding additional address types.
sefbkn Sep 27, 2021
3632c29
peer/wire: Relay TORv3 addresses.
sefbkn Mar 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 12 additions & 3 deletions addrmgr/addrmanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -684,10 +684,11 @@ func (a *AddrManager) NeedMoreAddresses() bool {
return a.numAddresses() < needAddressThreshold
}

// AddressCache returns a randomized subset of all known addresses.
// AddressCache returns a randomized subset of all addresses known to the
// address manager with a network address type matching the provided type flags.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// address manager with a network address type matching the provided type flags.
// address manager with a network address type matching the provided filter.

//
// This function is safe for concurrent access.
func (a *AddrManager) AddressCache() []*NetAddress {
func (a *AddrManager) AddressCache(supportedNetAddressType NetAddressTypeFilter) []*NetAddress {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func (a *AddrManager) AddressCache(supportedNetAddressType NetAddressTypeFilter) []*NetAddress {
func (a *AddrManager) AddressCache(filter NetAddressTypeFilter) []*NetAddress {

Copy link
Member

Choose a reason for hiding this comment

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

Also a breaking change to the public API which needs a major module version bump. I'll stop calling these out from now on since I mentioned it in my previous review and once the major is bumped, the API can be changed at will.

a.mtx.Lock()
defer a.mtx.Unlock()

Expand All @@ -700,6 +701,10 @@ func (a *AddrManager) AddressCache() []*NetAddress {
allAddr := make([]*NetAddress, 0, addrLen)
// Iteration order is undefined here, but we randomize it anyway.
for _, v := range a.addrIndex {
// Skip address types not requested by the caller.
if !supportedNetAddressType(v.na.Type) {
continue
}
// Skip low quality addresses.
if v.isBad() {
continue
Expand Down Expand Up @@ -1220,14 +1225,18 @@ func getReachabilityFrom(localAddr, remoteAddr *NetAddress) NetAddressReach {
// for the given remote address.
//
// This function is safe for concurrent access.
func (a *AddrManager) GetBestLocalAddress(remoteAddr *NetAddress) *NetAddress {
func (a *AddrManager) GetBestLocalAddress(remoteAddr *NetAddress, supportedNetAddressType NetAddressTypeFilter) *NetAddress {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func (a *AddrManager) GetBestLocalAddress(remoteAddr *NetAddress, supportedNetAddressType NetAddressTypeFilter) *NetAddress {
func (a *AddrManager) GetBestLocalAddress(remoteAddr *NetAddress, filter NetAddressTypeFilter) *NetAddress {

a.lamtx.Lock()
defer a.lamtx.Unlock()

bestreach := Default
var bestscore AddressPriority
var bestAddress *NetAddress
for _, la := range a.localAddresses {
// Only return addresses matching the type flags provided by the caller.
if !supportedNetAddressType(la.na.Type) {
continue
}
reach := getReachabilityFrom(la.na, remoteAddr)
if reach > bestreach ||
(reach == bestreach && la.score > bestscore) {
Expand Down
14 changes: 10 additions & 4 deletions addrmgr/addrmanager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ import (
// Put some IP in here for convenience. Points to google.
var someIP = "173.194.115.66"

// defaultNetAddressTypeFilter defines a filter that instructs address manager
// operations that accept it to return network addresses of any type.
func defaultNetAddressTypeFilter(netAddressType NetAddressType) bool {
return true
}

func lookupFunc(host string) ([]net.IP, error) {
return nil, errors.New("not implemented")
}
Expand Down Expand Up @@ -341,7 +347,7 @@ func TestGood(t *testing.T) {
addrsToAdd)
}

numCache := len(n.AddressCache())
numCache := len(n.AddressCache(defaultNetAddressTypeFilter))
if numCache >= numAddrs/4 {
t.Fatalf("Number of addresses in cache: got %d, want %d", numCache,
numAddrs/4)
Expand Down Expand Up @@ -536,7 +542,7 @@ func TestGetBestLocalAddress(t *testing.T) {

// Test against default when there's no address
for x, test := range tests {
got := amgr.GetBestLocalAddress(test.remoteAddr)
got := amgr.GetBestLocalAddress(test.remoteAddr, defaultNetAddressTypeFilter)
if !reflect.DeepEqual(test.want0.IP, got.IP) {
t.Errorf("TestGetBestLocalAddress test1 #%d failed for remote address %s: want %s got %s",
x, test.remoteAddr.IP, test.want1.IP, got.IP)
Expand All @@ -550,7 +556,7 @@ func TestGetBestLocalAddress(t *testing.T) {

// Test against want1
for x, test := range tests {
got := amgr.GetBestLocalAddress(test.remoteAddr)
got := amgr.GetBestLocalAddress(test.remoteAddr, defaultNetAddressTypeFilter)
if !reflect.DeepEqual(test.want1.IP, got.IP) {
t.Errorf("TestGetBestLocalAddress test1 #%d failed for remote address %s: want %s got %s",
x, test.remoteAddr.IP, test.want1.IP, got.IP)
Expand All @@ -564,7 +570,7 @@ func TestGetBestLocalAddress(t *testing.T) {

// Test against want2
for x, test := range tests {
got := amgr.GetBestLocalAddress(test.remoteAddr)
got := amgr.GetBestLocalAddress(test.remoteAddr, defaultNetAddressTypeFilter)
if !reflect.DeepEqual(test.want2.IP, got.IP) {
t.Errorf("TestGetBestLocalAddress test2 #%d failed for remote address %s: want %s got %s",
x, test.remoteAddr.IP, test.want2.IP, got.IP)
Expand Down
6 changes: 6 additions & 0 deletions addrmgr/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,12 @@ const (
TORv2Address
)

// NetAddressTypeFilter represents a function that returns whether a particular
// network address type matches a filter. Internally, it is used to ensure that
// only addresses that pass the filter's constraints are returned from the
// address manager.
type NetAddressTypeFilter func(NetAddressType) bool

// isRFC1918 returns whether or not the passed address is part of the IPv4
// private network address space as defined by RFC1918 (10.0.0.0/8,
// 172.16.0.0/12, or 192.168.0.0/16).
Expand Down
35 changes: 30 additions & 5 deletions server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,9 +444,8 @@ func (ps *peerState) ResolveLocalAddress(netType addrmgr.NetAddressType, addrMgr

// Add a local address if the network address is a probable external
// endpoint of the listener.
lNa := wire.NewNetAddressIPPort(listenerIP, uint16(port), services)
lNet := addrmgr.IPv4Address
if lNa.IP.To4() == nil {
if listenerIP.To4() == nil {
lNet = addrmgr.IPv6Address
}

Expand Down Expand Up @@ -705,6 +704,25 @@ func hasServices(advertised, desired wire.ServiceFlag) bool {
return advertised&desired == desired
}

// isSupportedNetAddressTypeV1 returns whether the provided address manager
// network address type is supported by the addr wire message.
func isSupportedNetAddressTypeV1(netAddressType addrmgr.NetAddressType) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Similar to comments on the previous commit, I would suggest a consistent param name addrType.

switch netAddressType {
case addrmgr.IPv4Address:
case addrmgr.IPv6Address:
case addrmgr.TORv2Address:
return true
}
return false
}

// getNetAddressTypeFilter returns a function that determines whether a
// specific address manager network address type is supported by the
// provided protocol version.
func getNetAddressTypeFilter(pver uint32) addrmgr.NetAddressTypeFilter {
Copy link
Member

Choose a reason for hiding this comment

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

Per effective Go, funcs really shouldn't be named with get as it is either redundant or not really descriptive of its intention. In this case, it appears the intention is to return a filter for the supported address types based on the protocol version. So, I'd suggest something like supportedAddrTypesFilter(pver uint32).

As an aside, the comment doesn't match what it's really doing either. It claims that it determines whether a specific address type is supported, but what it's really doing is returning a filter for the address types supported by the provided protocol version.

return isSupportedNetAddressTypeV1
}

// OnVersion is invoked when a peer receives a version wire message and is used
// to negotiate the protocol version details as well as kick start the
// communications.
Expand All @@ -720,6 +738,7 @@ func (sp *serverPeer) OnVersion(_ *peer.Peer, msg *wire.MsgVersion) {
// it is updated regardless in the case a new minimum protocol version is
// enforced and the remote node has not upgraded yet.
isInbound := sp.Inbound()
msgProtocolVersion := uint32(msg.ProtocolVersion)
remoteAddr := wireToAddrmgrNetAddress(sp.NA())
addrManager := sp.server.addrManager
if !cfg.SimNet && !cfg.RegNet && !isInbound {
Expand All @@ -730,7 +749,7 @@ func (sp *serverPeer) OnVersion(_ *peer.Peer, msg *wire.MsgVersion) {
}

// Reject peers that have a protocol version that is too old.
if msg.ProtocolVersion < int32(wire.SendHeadersVersion) {
if msgProtocolVersion < wire.SendHeadersVersion {
Copy link
Member

Choose a reason for hiding this comment

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

This is changing the semantics improperly and could lead to a bunch of undesirable things.

Currently, a negative protocol version will be rejected due to this check, but with this change, negative protocol versions would become large positive versions and thus improperly pass this check.

This must be reverted.

The following code demonstrates:

package main

import "fmt"

func main() {
	var pver int32 = -1
	var testPver uint32 = 3
	if pver < int32(testPver) {
		fmt.Println("original too low as expected")
	}
	if uint32(pver) < testPver {
		fmt.Println("notice the modified version does not see it as too low")
	} else {
		fmt.Println("treated as ok even though it's not")
	}
}

Output:

original too low as expected
treated as ok even though it's not

srvrLog.Debugf("Rejecting peer %s with protocol version %d prior to "+
"the required version %d", sp.Peer, msg.ProtocolVersion,
wire.SendHeadersVersion)
Expand Down Expand Up @@ -760,7 +779,8 @@ func (sp *serverPeer) OnVersion(_ *peer.Peer, msg *wire.MsgVersion) {
// known tip.
if !cfg.DisableListen && sp.server.syncManager.IsCurrent() {
// Get address that best matches.
lna := addrManager.GetBestLocalAddress(remoteAddr)
naTypeFilter := getNetAddressTypeFilter(msgProtocolVersion)
lna := addrManager.GetBestLocalAddress(remoteAddr, naTypeFilter)
if lna.IsRoutable() {
// Filter addresses the peer already knows about.
addresses := []*addrmgr.NetAddress{lna}
Expand Down Expand Up @@ -1387,7 +1407,9 @@ func (sp *serverPeer) OnGetAddr(p *peer.Peer, msg *wire.MsgGetAddr) {
sp.addrsSent = true

// Get the current known addresses from the address manager.
addrCache := sp.server.addrManager.AddressCache()
pver := sp.ProtocolVersion()
naTypeFilter := getNetAddressTypeFilter(pver)
addrCache := sp.server.addrManager.AddressCache(naTypeFilter)

// Push the addresses.
sp.pushAddrMsg(addrCache)
Expand Down Expand Up @@ -3781,6 +3803,9 @@ func newServer(ctx context.Context, listenAddrs []string, db database.DB,
if !cfg.SimNet && !cfg.RegNet && len(cfg.ConnectPeers) == 0 {
newAddressFunc = func() (net.Addr, error) {
for tries := 0; tries < 100; tries++ {
// Note that this does not filter by address type. Unsupported
// network address types should be pruned from the address
// manager's internal storage prior to calling this function.
Copy link
Member Author

Choose a reason for hiding this comment

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

addrmgr.GetAddress should accept a filter function argument that excludes network addresses that cannot be connected to. For example, if dcrd is not routing traffic through a tor proxy then it should not attempt to fetch + connect to a TORv3 address stored in the address manager.

addr := s.addrManager.GetAddress()
if addr == nil {
break
Expand Down