multi: Add getaddrv2 and addrv2. #2627
Conversation
sefbkn
changed the title
davecgh
added
the
wire protocol change
|
Does this handle torv3 addresses? I also think the new addrmgr.NetAddress belongs in |
@dajohi No, torv3 addresses are not supported or broadcast in this PR, but the commit that introduces it and builds on these changes can be included if it seems appropriate for this PR. Regarding putting this new type in the wire package, I need to follow back up on this one. |
|
Converting this to a draft until #2596 is merged, since this pull request builds on that. |
|
#2596 has been merged. |
|
I was hoping to get this into v1.7, but it looks like I'm going to have to move this to 1.8 unless @sefbkn plans on finalizing it within the next few days. |
This commit adds support for determining and tracking the type of a network address when constructed.
This commit introduces a network address type filter that allows a caller to indicate the types of addresses that can be returned from the address manager. This allows finer control of what types of addresses may be broadcast to a particular peer. The filters are decided by the protocol version of the peer in a way that allows the address manager's internal implementation to remain agnostic of the protocol versioning logic.
This prevents sending addresses in a version message that cannot be converted to a wire network address.
This removes the need to perform DNS lookups in the address manager.
| @@ -3781,6 +3979,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. | |||
There was a problem hiding this comment.
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.
This change adds support for TORv3 addresses to the address manager. It also allows connecting to a TORv3 hostname with the cli --connect flag when starting dcrd. It does not support relaying TORv3 addresses over the peer to peer network.
This change removes support for the TORv2 network address persistence and propogation across the peer to peer network.
This commit modifies the wire protocol by adding two new message types addrv2 and getaddrv2 and bumps the wire protocol version to 10. It does not introduce any new network address types and is intended to implement the minimum framework necessary to support easily adding new address types in future commits. These two new message types are intended to eventually replace their older counterparts, which are getaddr and addr respectively. A peer sending a getaddrv2 or addrv2 message with a protocol version less than 10 is in violation of the wire protocol and the peer is disconnected. Similarly, peers advertising a protocol version greater than or equal to 10 that send an addr or gettaddr message are in violation of the wire protocol and are disconnected. A getaddrv2 message is similar in structure and purpose to a getaddr message in that it has no payload and functions as a request for a peer to reply with an addrv2 message if it has addresses to share. An addrv2 message is similar in structure and function to an addr message with a few key differences: - Port is now encoded as a little endian value rather than big endian. - Timestamp is encoded as a 64 bit value rather than a 32 bit value. - A network address type field is now serialized with each address to indicate the length and type of the address it precedes. - A message with zero addresses is no longer serializable. - Addresses are serialized their most compact form, rather than always being encoded into a 16 byte structure.
This commit adds support for address types supported by the address manager when recieving addresses from a seeder.
This commit relays TORv3 addresses across the peer to peer network and bumps the wire protocol version to 11.
There was a problem hiding this comment.
I'm reviewing this commit by commit and done with the first one. I've identified various minor things, but it looks pretty good overall thus far.
Review progress:
- addrmgr: Track network address types.
- addrmgr: Add network address type filter.
- peer: Partially decouple peer from wire.
- addrmgr: Remove DNS lookups from address manager.
- addrmgr: Add TORv3 network address type.
- multi: Remove TORv2 network address type.
- peer/wire: Add addrv2 and getaddrv2 message types.
- connmgr: Allow seeding additional address types.
- peer/wire: Relay TORv3 addresses.
addrmgr/addrmanager.go
Outdated
| if strings.HasSuffix(host, ".onion") { | ||
| // Check if this is a TorV2 address. | ||
| if len(host) == 22 { | ||
| data, err := base32.StdEncoding.DecodeString( |
There was a problem hiding this comment.
I'd suggest leaving the comment that explains why this is doing the case conversion.
There was a problem hiding this comment.
Also, I know this is essentially what was already there, but since it's being modified, I'd suggest taking the opportunity to make it more efficient by reducing the number of allocations with something similar to the following:
const prefix = "\xfd\x87\xd8\x7e\xeb\x43"
const hostLen = 16
data := make([]byte, len(prefix)+base32.StdEncoding.DecodedLen(hostLen))
copy(data, prefix[:])
// Go base32 encoding uses uppercase (as does the RFC), but Tor tends to
// user lowercase, so switch case here.
hostUpper := []byte(strings.ToUpper(host[:hostLen]))
_, err := base32.StdEncoding.Decode(data[len(prefix):], hostUpper)
if err != nil {
return UnknownAddressType, nil, err
}
return TORv2Address, data, nil| // based on the type of the network address, if applicable. | ||
| func canonicalizeIP(addrType NetAddressType, addrBytes []byte) []byte { | ||
| if addrBytes == nil { | ||
| return []byte{} |
There was a problem hiding this comment.
I don't see any reason to force an allocation here.
| return []byte{} | |
| return nil |
|
|
||
| // assertNetAddressTypeValid returns an error if the suggested address type does | ||
| // not appear to match the provided address. | ||
| func assertNetAddressTypeValid(netAddressType NetAddressType, addrBytes []byte) error { |
There was a problem hiding this comment.
Would suggest calling this checkNetAddressType. The term assert tends to imply it will panic if it's not correct, which isn't the case. Also, it's more consistent with the rest of the code base which tends to use checkX for these types of funcs.
There was a problem hiding this comment.
Also, perhaps these should all be consistent with the param name. canonicalizeIP uses addrType while this one is netAddressType. I personally prefer addrType since I find it to be perfectly descriptive and it's shorter.
Either one is fine, but I would prefer they are all consistent.
| case len == 16: | ||
| return IPv6Address, nil | ||
| } | ||
| strErr := fmt.Sprintf("unable to determine address type from raw network "+ |
There was a problem hiding this comment.
Everything else in the code base tends to use str for these name here. Not a big deal, but would be better for consistency.
| strErr := fmt.Sprintf("unable to determine address type from raw network "+ | |
| str := fmt.Sprintf("unable to determine address type from raw network "+ |
| } | ||
|
|
||
| // NewNetAddressByType creates a new network address using the provided | ||
| // parameters. If the provided network id does not appear to match the address, |
There was a problem hiding this comment.
| // parameters. If the provided network id does not appear to match the address, | |
| // parameters. If the provided address type does not appear to match the address, |
| @@ -91,15 +166,30 @@ func (a *AddrManager) newAddressFromString(addr string) (*NetAddress, error) { | |||
| return nil, err | |||
| } | |||
|
|
|||
| return a.HostToNetAddress(host, uint16(port), wire.SFNodeNetwork) | |||
| networkID, addrBytes, err := ParseHost(host) | |||
There was a problem hiding this comment.
Seem like this should be addrType.
addrmgr/netaddress_test.go
Outdated
| addrBytes []byte | ||
| want *NetAddress | ||
| }{ | ||
| { |
There was a problem hiding this comment.
It looks like you ultimately correct the formatting in a later commit, but since the tests are being added here in this commit (addrmgr: Track network address types.), that should be done here.
addrmgr/network.go
Outdated
| @@ -121,29 +121,12 @@ func isOnionCatTor(netIP net.IP) bool { | |||
| type NetAddressType uint8 | |||
|
|
|||
| const ( | |||
| LocalAddress NetAddressType = iota | |||
| UnknownAddressType NetAddressType = iota | |||
There was a problem hiding this comment.
This (and I presume other changes in upcoming commits) is a breaking change to the public API, so a new major version of the module will be needed before this PR can be merged.
There was a problem hiding this comment.
Review progress:
- addrmgr: Track network address types.
- addrmgr: Add network address type filter.
- peer: Partially decouple peer from wire.
- addrmgr: Remove DNS lookups from address manager.
- addrmgr: Add TORv3 network address type.
- multi: Remove TORv2 network address type.
- peer/wire: Add addrv2 and getaddrv2 message types.
- connmgr: Allow seeding additional address types.
- peer/wire: Relay TORv3 addresses.
| @@ -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. | |||
There was a problem hiding this comment.
| // 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 { |
There was a problem hiding this comment.
| func (a *AddrManager) AddressCache(supportedNetAddressType NetAddressTypeFilter) []*NetAddress { | |
| func (a *AddrManager) AddressCache(filter NetAddressTypeFilter) []*NetAddress { |
There was a problem hiding this comment.
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.
| @@ -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 { | |||
There was a problem hiding this comment.
| func (a *AddrManager) GetBestLocalAddress(remoteAddr *NetAddress, supportedNetAddressType NetAddressTypeFilter) *NetAddress { | |
| func (a *AddrManager) GetBestLocalAddress(remoteAddr *NetAddress, filter NetAddressTypeFilter) *NetAddress { |
| @@ -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 { | |||
There was a problem hiding this comment.
Similar to comments on the previous commit, I would suggest a consistent param name addrType.
server.go
Outdated
| // 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 { |
There was a problem hiding this comment.
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.
| @@ -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 { | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Review progress:
- addrmgr: Track network address types.
- addrmgr: Add network address type filter.
- peer: Partially decouple peer from wire.
- addrmgr: Remove DNS lookups from address manager.
- addrmgr: Add TORv3 network address type.
- multi: Remove TORv2 network address type.
- peer/wire: Add addrv2 and getaddrv2 message types.
- connmgr: Allow seeding additional address types.
- peer/wire: Relay TORv3 addresses.
|
|
||
| replace ( | ||
| github.com/decred/dcrd/addrmgr/v2 => ../addrmgr | ||
| github.com/decred/dcrd/dcrec/secp256k1/v4 => ../dcrec/secp256k1 |
There was a problem hiding this comment.
I don't believe any of these other replacements aside from the addrmgr are needed.
| @@ -4,10 +4,18 @@ go 1.11 | |||
|
|
|||
| require ( | |||
| github.com/davecgh/go-spew v1.1.1 | |||
| github.com/decred/dcrd/addrmgr/v2 v2.0.0 // indirect | |||
There was a problem hiding this comment.
This does not appear to be correct. It's a direct dependency since it's now imported.
| @@ -385,7 +378,7 @@ type AddrFunc func(remoteAddr *wire.NetAddress) *wire.NetAddress | |||
| // HostToNetAddrFunc is a func which takes a host, port, services and returns | |||
| // the netaddress. | |||
| type HostToNetAddrFunc func(host string, port uint16, | |||
| services wire.ServiceFlag) (*wire.NetAddress, error) | |||
| services wire.ServiceFlag) (*addrmgr.NetAddress, error) | |||
There was a problem hiding this comment.
This is a major breaking change to the public API of the peer module and thus will need a major module version bump.
| @@ -564,7 +557,7 @@ func (p *Peer) ID() int32 { | |||
| // NA returns the peer network address. | |||
| // | |||
| // This function is safe for concurrent access. | |||
| func (p *Peer) NA() *wire.NetAddress { | |||
| func (p *Peer) NA() *addrmgr.NetAddress { | |||
There was a problem hiding this comment.
Same here regarding being a breaking change to the API.
| na := wire.NewNetAddressIPPort(ip, port, services) | ||
| return na, nil | ||
| func newNetAddress(addr net.Addr, services wire.ServiceFlag) (*addrmgr.NetAddress, error) { | ||
| host, portStr, err := net.SplitHostPort(addr.String()) |
There was a problem hiding this comment.
This is introducing a bunch of extra allocations that were previously avoided for the most common cases which is why it was the way it was already.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| ip := net.ParseIP(host) | ||
|
|
||
| if addrType == addrmgr.UnknownAddressType { |
There was a problem hiding this comment.
This can probably move after the port parse since there is no reason to do it if the parse fails.
| // If the address cannot be converted, an IPv4 network address consisting of all | ||
| // zeroes is returned. | ||
| func addrmgrToWireNetAddress(addr *addrmgr.NetAddress, proxyAddr string) *wire.NetAddress { | ||
| if addr.Type != addrmgr.IPv4Address && |
There was a problem hiding this comment.
You don't have to change it if you prefer the code how it is, but what do you think about using a switch here so it's easier to expand in the future if needed? e.g.
switch addr.Type {
case addrmgr.IPv4Address, addrmgr.IPv6Address, addrmgr.TORv2Address:
// Recognized types. Handled below.
default:
return wire.NewNetAddressIPPort(zeroIPv4, 0, addr.Services)
}| // | ||
| // If the address cannot be converted, an IPv4 network address consisting of all | ||
| // zeroes is returned. | ||
| func addrmgrToWireNetAddress(addr *addrmgr.NetAddress, proxyAddr string) *wire.NetAddress { |
There was a problem hiding this comment.
I'm a little bit worried about how this is being split out and named as if it is for generic use, but it's not due to the additional proxy handling. It's really specifically for use during version negotiation.
| @@ -313,20 +314,20 @@ type peerState struct { | |||
| } | |||
|
|
|||
| // ConnectionsWithIP returns the number of connections with the given IP. | |||
| func (ps *peerState) ConnectionsWithIP(ip net.IP) int { | |||
| func (ps *peerState) ConnectionsWithIP(ip []byte) int { | |||
There was a problem hiding this comment.
Perhaps this is being changed for an upcoming commit, but it doesn't really make sense to me to make this change in isolation as of this commit.
Would you mind elaborating on the thought process for the change?
There was a problem hiding this comment.
I really don't understand the thought process of the changes being made here to remove the name resolution bits from the address manager. It's supposed to manage addresses, which, to me, includes name resolution using a resolver provided the caller.
As it stands, up to the point that I've reviewed, there is what seems like some kind of arbitrary line drawn where it sort of half parses hosts into IPs for certain things, but then not others and returns UnknownAddressType expecting the caller to do more with it, but every single caller (more than just dcrd) that uses the addrmgr will need to do that same logic.
Maybe there are some more upcoming changes that will provide more context, or it's that the naming of things is confusing me as to what the overall goal is, but at this point, the changes seem to be removing things that are useful to other callers and making them inaccessible.
Review progress:
- addrmgr: Track network address types.
- addrmgr: Add network address type filter.
- peer: Partially decouple peer from wire.
- addrmgr: Remove DNS lookups from address manager.
- addrmgr: Add TORv3 network address type.
- multi: Remove TORv2 network address type.
- peer/wire: Add addrv2 and getaddrv2 message types.
- connmgr: Allow seeding additional address types.
- peer/wire: Relay TORv3 addresses.
| @@ -0,0 +1,107 @@ | |||
| // Copyright (c) 2021 The Decred developers | |||
server_test.go
Outdated
| wantErr bool | ||
| want *addrmgr.NetAddress | ||
| }{ | ||
| { |
There was a problem hiding this comment.
Since this is being moved, please update it to the latest formatting for consistency.
There was a problem hiding this comment.
I've confirmed the Tor address encoding and parsing logic is correct per the Tor v3 address specification.
Review progress:
- addrmgr: Track network address types.
- addrmgr: Add network address type filter.
- peer: Partially decouple peer from wire.
- addrmgr: Remove DNS lookups from address manager.
- addrmgr: Add TORv3 network address type.
- multi: Remove TORv2 network address type.
- peer/wire: Add addrv2 and getaddrv2 message types.
- connmgr: Allow seeding additional address types.
- peer/wire: Relay TORv3 addresses.
| @@ -224,6 +232,39 @@ func isRFC6598(netIP net.IP) bool { | |||
| return rfc6598Net.Contains(netIP) | |||
| } | |||
|
|
|||
| // calcTORv3Checksum returns the checksum bytes given a 32 byte | |||
| // TORv3 public key. | |||
| func calcTORv3Checksum(publicKey []byte) []byte { | |||
There was a problem hiding this comment.
I'd suggest using fixed-size and stack allocated code here to make it allocation free since it appears like this code will be called quite frequently. e.g.:
func calcTORv3Checksum(publicKey [32]byte) [2]byte {
const (
prefix = ".onion checksum"
prefixLen = len(prefix)
inputLen = prefixLen + len(publicKey) + 1
)
var input [inputLen]byte
copy(input[:], prefix)
copy(input[prefixLen:], publicKey[:])
input[inputLen-1] = torV3VersionByte
digest := sha3.Sum256(input[:])
var result [2]byte
copy(result[:], digest[:])
return result
}Naturally the calling code needs to be updated to copy instead of sending in byte slices too. The result will end up being faster and avoid churning the GC.
addrmgr/network.go
Outdated
| // group is keyed off the first 4 bits of the actual onion key. | ||
| return fmt.Sprintf("tor:%d", netIP[6]&((1<<4)-1)) | ||
| // Group is keyed off the first 4 bits of the actual onion key. | ||
| return fmt.Sprintf("torv2:%d", netIP[6]&((1<<4)-1)) |
There was a problem hiding this comment.
Won't changing this mess up all of the existing entries?
|
|
||
| // Determine the network that the address belongs to and return early if | ||
| // a DNS lookup should not be performed for the address. | ||
| networkID, _, err := addrmgr.ParseHost(host) |
There was a problem hiding this comment.
Review progress:
- addrmgr: Track network address types.
- addrmgr: Add network address type filter.
- peer: Partially decouple peer from wire.
- addrmgr: Remove DNS lookups from address manager.
- addrmgr: Add TORv3 network address type.
- multi: Remove TORv2 network address type.
- peer/wire: Add addrv2 and getaddrv2 message types.
- connmgr: Allow seeding additional address types.
- peer/wire: Relay TORv3 addresses.
| IPv6Address | ||
| TORv2Address | ||
| TORv3Address | ||
| UnknownAddressType NetAddressType = 0 |
There was a problem hiding this comment.
Should call out why it's not using iota, imo (because it's used in the serialization, etc). There are examples in various parts of the code base.
|
|
||
| "github.com/decred/dcrd/addrmgr/v2" | ||
| "github.com/decred/dcrd/wire" | ||
| ) | ||
|
|
||
| var zeroTime = time.Time{} | ||
|
|
||
| var ( |
There was a problem hiding this comment.
These should be defined inside the test scope (unless they're going to be used in a future commit).
| if test.wantErr == true && err == nil { | ||
| t.Errorf("%q: expected error but one was not returned", test.name) | ||
| return |
There was a problem hiding this comment.
These should all be continue to move on to the next test.
There was a problem hiding this comment.
I would like to see the peer, wire, and server bits split up where the wire bits come first. You can take a look at previous PRs I've done to add new protocol messages for inspiration. For example (#1906). It helps the review process and also helps prevent accidental breaking changes to the protocol through the layers.
Review progress:
- addrmgr: Track network address types.
- addrmgr: Add network address type filter.
- peer: Partially decouple peer from wire.
- addrmgr: Remove DNS lookups from address manager.
- addrmgr: Add TORv3 network address type.
- multi: Remove TORv2 network address type.
- peer/wire: Add addrv2 and getaddrv2 message types.
- connmgr: Allow seeding additional address types.
- peer/wire: Relay TORv3 addresses.
| // slice. | ||
| // | ||
| // This function is safe for concurrent access. | ||
| func (p *Peer) PushAddrV2Msg(addresses []*wire.NetAddressV2) []*wire.NetAddressV2 { |
There was a problem hiding this comment.
I know the existing one made use of a slice of pointers, but we have been switching things over to use a slice of concrete objects when possible to reduce allocations and provide better cache locality. To that end, please update this to be the following (and of course update the code and caller accordingly):
func (p *Peer) PushAddrV2Msg(addresses []wire.NetAddressV2) []wire.NetAddressV2 {| @@ -326,6 +327,12 @@ func TestPeerListeners(t *testing.T) { | |||
| ok := make(chan wire.Message, 20) | |||
| peerCfg := &Config{ | |||
| Listeners: MessageListeners{ | |||
| OnGetAddrV2: func(p *Peer, msg *wire.MsgGetAddrV2) { | |||
There was a problem hiding this comment.
While I know the compiler doesn't care, these really should be specified in the same order that they are defined in for consistency.
| @@ -688,6 +710,20 @@ func TestOutboundPeer(t *testing.T) { | |||
| t.Errorf("PushAddrMsg: unexpected err %v\n", err) | |||
| return | |||
| } | |||
|
|
|||
| var addrsV2 []*wire.NetAddressV2 | |||
There was a problem hiding this comment.
Not a huge deal since this is tests, but you should pretty much always avoid doing raw appends like this because it causes additional allocations and copies.
e.g.
addrsV2 := make([]*wire.NetAddressV2, 0, 5)
for i := 0; i < 5; i++ {
na := &wire.NetAddressV2{}
addrsV2 = append(addrsV2, na)
}Do note however, that with the change I recommended to use concrete types instead of pointers, this whole block will be simplified to a simple addrsV2 := make([]wire.NetAddressV2, 5).
| @@ -1,5 +1,5 @@ | |||
| // Copyright (c) 2013-2016 The btcsuite developers | |||
| // Copyright (c) 2015-2020 The Decred developers | |||
| // Copyright (c) 2015-2021 The Decred developers | |||
There was a problem hiding this comment.
These are all 2022 now. I'll only comment on this one, but there are several others.
| @@ -48,6 +48,9 @@ const ( | |||
| // is received. | |||
| ErrPayloadChecksum | |||
|
|
|||
| // ErrTooFewAddrs is returned when an address list has zero addresses. | |||
| ErrTooFewAddrs | |||
There was a problem hiding this comment.
This is a major breaking change to the public API because wire still uses iota for error codes. Eventually we are going to have to bump it to v2 and move to the new error methodology which isn't subject to this issue, but we have been holding off because bumping the wire major is going to cause almost every single thing in the code base to also require a major module bump, so we want to handle the move to primitives at the same time whenever that is done.
The fix is to just move this down to the bottom so it doesn't change the value of any of the existing error codes.
| // 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 { | ||
| return isSupportedNetAddressTypeV1 | ||
| func getNetAddressTypeFilter(protocolVersion uint32) addrmgr.NetAddressTypeFilter { |
There was a problem hiding this comment.
Please keep pver. It's used everywhere and is thus consistent.
| if !p.Inbound() { | ||
| peerLog.Errorf("Received getaddrv2 request from outbound peer %s", | ||
| sp.Peer) | ||
| sp.server.BanPeer(sp) |
There was a problem hiding this comment.
This will incorrectly ban peers since the message is sent to outbound peers.
| // via one or more addrv2 messages (MsgAddrV2). | ||
| // | ||
| // This message has no payload. | ||
| type MsgGetAddrV2 struct{} |
There was a problem hiding this comment.
I commented as I was reviewing, but after having gone through the entire commit, I don't really see any reason to introduce getaddrv2 given it is identical to getaddr and the protocol version determines whether or not to respond with a addr or addrv2.
| @@ -1443,6 +1518,45 @@ func (sp *serverPeer) OnGetAddr(p *peer.Peer, msg *wire.MsgGetAddr) { | |||
| sp.pushAddrMsg(addrCache) | |||
| } | |||
|
|
|||
| // OnGetAddrV2 is invoked when a peer receives a getaddrv2 wire message and is | |||
| // used to provide the peer with known addresses from the address manager. | |||
| func (sp *serverPeer) OnGetAddrV2(p *peer.Peer, msg *wire.MsgGetAddrV2) { | |||
There was a problem hiding this comment.
As discussed elsewhere, I don't think a separate getaddrv2 is necessary since it is identical to getaddr and the protocol version dictates whether to respond with an addr or addrv2 message.
| @@ -2231,6 +2392,8 @@ func newPeerConfig(sp *serverPeer) *peer.Config { | |||
| OnGetCFilterV2: sp.OnGetCFilterV2, | |||
| OnGetCFHeaders: sp.OnGetCFHeaders, | |||
| OnGetCFTypes: sp.OnGetCFTypes, | |||
| OnGetAddrV2: sp.OnGetAddrV2, | |||
| OnAddrV2: sp.OnAddrV2, | |||
There was a problem hiding this comment.
Review progress:
- addrmgr: Track network address types.
- addrmgr: Add network address type filter.
- peer: Partially decouple peer from wire.
- addrmgr: Remove DNS lookups from address manager.
- addrmgr: Add TORv3 network address type.
- multi: Remove TORv2 network address type.
- peer/wire: Add addrv2 and getaddrv2 message types.
- connmgr: Allow seeding additional address types.
- peer/wire: Relay TORv3 addresses.
| @@ -92,7 +93,7 @@ type node struct { | |||
| // The available filters can be set via the exported functions that start with | |||
| // the prefix SeedFilter. See the documentation for each function for more | |||
| // details. | |||
| func SeedAddrs(ctx context.Context, seeder string, dialFn DialFunc, filters ...func(f *HttpsSeederFilters)) ([]*wire.NetAddress, error) { | |||
| func SeedAddrs(ctx context.Context, seeder string, dialFn DialFunc, filters ...func(f *HttpsSeederFilters)) ([]*addrmgr.NetAddress, error) { | |||
There was a problem hiding this comment.
Breaking change to the API. connmgr needs a major module version bump for this since it hasn't been done since the last release.
There was a problem hiding this comment.
Also, for reasons mentioned elsewhere, please prefer []addrmgr.NetAddress over the pointer variant.
| @@ -723,7 +704,9 @@ func (sp *serverPeer) pushAddrV2Msg(addresses []*addrmgr.NetAddress) { | |||
| addrs := make([]*wire.NetAddressV2, 0, len(addresses)) | |||
| for _, addr := range addresses { | |||
| if !sp.addressKnown(addr) { | |||
| addrV2 := addrmgrToWireNetAddressV2(addr) | |||
| wireNetAddrType := wire.NetAddressType(addr.Type) | |||
There was a problem hiding this comment.
Calling this out again about the cast since I see the function I commented on in an earlier commit is removed.
There was a problem hiding this comment.
As before, I would prefer the wire, peer and server bits be split up into individual commits for the same reason.
Review progress:
- addrmgr: Track network address types.
- addrmgr: Add network address type filter.
- peer: Partially decouple peer from wire.
- addrmgr: Remove DNS lookups from address manager.
- addrmgr: Add TORv3 network address type.
- multi: Remove TORv2 network address type.
- peer/wire: Add addrv2 and getaddrv2 message types.
- connmgr: Allow seeding additional address types.
- peer/wire: Relay TORv3 addresses.
| @@ -792,7 +793,7 @@ func isSupportedNetAddressTypeV2(netAddressType addrmgr.NetAddressType) bool { | |||
| // specific address manager network address type is supported by the | |||
| // provided protocol version. | |||
| func getNetAddressTypeFilter(protocolVersion uint32) addrmgr.NetAddressTypeFilter { | |||
| if protocolVersion < wire.AddrV2Version { | |||
| if protocolVersion < wire.RelayTORv3Version { | |||
There was a problem hiding this comment.
Shouldn't this still be AddrV2Version? It's v1 before that point, not v2.
| const maxAddressSize = 16 | ||
| const portSize = 2 | ||
| if pver < RelayTORv3Version { | ||
| const ( |
There was a problem hiding this comment.
These could all go outside of the if and be reused since all of them are the same except maxAddressSize.
|
Alright, I'm through with the initial review. This also needs to be rebased to resolve the conflicts. Once everything is addressed, I'll give it another pass and thoroughly test it manually. |
|
Reminder on this one. Will need a rebase to resolve conflicts as well. |
davecgh
added
the
waiting for changes
This depends on #2596 and the removal of the reject wire message.
This PR modifies the wire protocol by adding two new message types addrv2 and getaddrv2 and bumps the wire protocol version to 10. It also removes the need to perform DNS lookups in the address manager through HostToNetAddress by pushing these calls up the call stack and further decoupling the address manager from its caller. It does not introduce any new network address types and is intended to implement the minimum framework necessary to facilitate easily adding new address types in the future. The intention of this design is to allow supporting new address types through addrv2 through protocol version bumps moving forward. Changes that are not transmitted across the peer to peer network should not require version bumps and may be implemented without doing so.
These two new message types are intended to eventually replace their older counterparts, which are getaddr and addr respectively. A peer sending a getaddrv2 or addrv2 message with a protocol version less than 10 is in violation of the wire protocol and the peer is disconnected. Similarly, peers advertising a protocol version greater than or equal to 10 that send an addr or gettaddr message are in violation of the wire protocol and are disconnected.
A getaddrv2 message is similar in structure and purpose to a getaddr message in that it has no payload and functions as a request for a peer to reply with an addrv2 message if it has addresses to share. An addrv2 message is similar in structure and function to an addr message with a few key differences: