[NOD-46] Immediately associate received address to their subnetwork (#196)

* [NOD-46] Immediately associate received address to their subnetwork

* [NOD-46] fix comment

* [NOD-46] Disconnect peer if it sends an MsgAddr with no subnetwork

* [NOD-46] Disconnect peer if it sends an MsgAddr with incorrect subnetwork id

* [NOD-46] add parenthesis to condition

* [NOD-46] change order of conditions
This commit is contained in:
Ori Newman 2019-03-07 16:33:10 +02:00 committed by Evgeny Khirin
parent a5e304d3ed
commit aa46c167c8
4 changed files with 65 additions and 36 deletions

View File

@ -169,7 +169,7 @@ const (
// updateAddress is a helper function to either update an address already known
// to the address manager, or to add the address if not already known.
func (a *AddrManager) updateAddress(netAddr, srcAddr *wire.NetAddress) {
func (a *AddrManager) updateAddress(netAddr, srcAddr *wire.NetAddress, subnetworkID *subnetworkid.SubnetworkID) {
// Filter out non-routable addresses. Note that non-routable
// also includes invalid and local addresses.
if !IsRoutable(netAddr) {
@ -193,6 +193,7 @@ func (a *AddrManager) updateAddress(netAddr, srcAddr *wire.NetAddress) {
naCopy.Timestamp = netAddr.Timestamp
naCopy.AddService(netAddr.Services)
ka.na = &naCopy
ka.subnetworkID = subnetworkID
}
// If already in tried, we have nothing to do here.
@ -216,9 +217,9 @@ func (a *AddrManager) updateAddress(netAddr, srcAddr *wire.NetAddress) {
// updated elsewhere in the addrmanager code and would otherwise
// change the actual netaddress on the peer.
netAddrCopy := *netAddr
ka = &KnownAddress{na: &netAddrCopy, srcAddr: srcAddr, subnetworkID: &wire.SubnetworkIDUnknown}
ka = &KnownAddress{na: &netAddrCopy, srcAddr: srcAddr, subnetworkID: subnetworkID}
a.addrIndex[addr] = ka
a.nNew[wire.SubnetworkIDUnknown]++
a.nNew[*subnetworkID]++
// XXX time penalty?
}
@ -635,28 +636,28 @@ func (a *AddrManager) Stop() error {
// AddAddresses adds new addresses to the address manager. It enforces a max
// number of addresses and silently ignores duplicate addresses. It is
// safe for concurrent access.
func (a *AddrManager) AddAddresses(addrs []*wire.NetAddress, srcAddr *wire.NetAddress) {
func (a *AddrManager) AddAddresses(addrs []*wire.NetAddress, srcAddr *wire.NetAddress, subnetworkID *subnetworkid.SubnetworkID) {
a.mtx.Lock()
defer a.mtx.Unlock()
for _, na := range addrs {
a.updateAddress(na, srcAddr)
a.updateAddress(na, srcAddr, subnetworkID)
}
}
// AddAddress adds a new address to the address manager. It enforces a max
// number of addresses and silently ignores duplicate addresses. It is
// safe for concurrent access.
func (a *AddrManager) AddAddress(addr, srcAddr *wire.NetAddress) {
func (a *AddrManager) AddAddress(addr, srcAddr *wire.NetAddress, subnetworkID *subnetworkid.SubnetworkID) {
a.mtx.Lock()
defer a.mtx.Unlock()
a.updateAddress(addr, srcAddr)
a.updateAddress(addr, srcAddr, subnetworkID)
}
// AddAddressByIP adds an address where we are given an ip:port and not a
// wire.NetAddress.
func (a *AddrManager) AddAddressByIP(addrIP string) error {
func (a *AddrManager) AddAddressByIP(addrIP string, subnetworkID *subnetworkid.SubnetworkID) error {
// Split IP and port
addr, portStr, err := net.SplitHostPort(addrIP)
if err != nil {
@ -672,7 +673,7 @@ func (a *AddrManager) AddAddressByIP(addrIP string) error {
return fmt.Errorf("invalid port %s: %s", portStr, err)
}
na := wire.NewNetAddressIPPort(ip, uint16(port), 0)
a.AddAddress(na, na) // XXX use correct src address
a.AddAddress(na, na, subnetworkID) // XXX use correct src address
return nil
}
@ -708,7 +709,7 @@ func (a *AddrManager) NeedMoreAddresses() bool {
a.mtx.Lock()
defer a.mtx.Unlock()
allAddrs := a.numAddresses(a.localSubnetworkID) + a.numAddresses(&wire.SubnetworkIDUnknown)
allAddrs := a.numAddresses(a.localSubnetworkID)
if !a.localSubnetworkID.IsEqual(&wire.SubnetworkIDSupportsAll) {
allAddrs += a.numAddresses(&wire.SubnetworkIDSupportsAll)
}
@ -827,9 +828,6 @@ func (a *AddrManager) GetAddress() *KnownAddress {
defer a.mtx.Unlock()
subnetworkID := *a.localSubnetworkID
if a.nTried[subnetworkID] == 0 && a.nNew[subnetworkID] == 0 {
subnetworkID = wire.SubnetworkIDUnknown
}
// Use a 50% chance for choosing between tried and new table entries.
if a.nTried[subnetworkID] > 0 && (a.nNew[subnetworkID] == 0 || a.rand.Intn(2) == 0) {

View File

@ -140,7 +140,7 @@ func TestAddAddressByIP(t *testing.T) {
amgr := addrmgr.New("testaddressbyip", nil, &wire.SubnetworkIDSupportsAll)
for i, test := range tests {
err := amgr.AddAddressByIP(test.addrIP)
err := amgr.AddAddressByIP(test.addrIP, &wire.SubnetworkIDSupportsAll)
if test.err != nil && err == nil {
t.Errorf("TestGood test %d failed expected an error and got none", i)
continue
@ -214,7 +214,7 @@ func TestAttempt(t *testing.T) {
n := addrmgr.New("testattempt", lookupFunc, &wire.SubnetworkIDSupportsAll)
// Add a new address and get it
err := n.AddAddressByIP(someIP + ":8333")
err := n.AddAddressByIP(someIP+":8333", &wire.SubnetworkIDSupportsAll)
if err != nil {
t.Fatalf("Adding address failed: %v", err)
}
@ -236,7 +236,7 @@ func TestConnected(t *testing.T) {
n := addrmgr.New("testconnected", lookupFunc, &wire.SubnetworkIDSupportsAll)
// Add a new address and get it
err := n.AddAddressByIP(someIP + ":8333")
err := n.AddAddressByIP(someIP+":8333", &wire.SubnetworkIDSupportsAll)
if err != nil {
t.Fatalf("Adding address failed: %v", err)
}
@ -272,7 +272,7 @@ func TestNeedMoreAddresses(t *testing.T) {
srcAddr := wire.NewNetAddressIPPort(net.IPv4(173, 144, 173, 111), 8333, 0)
n.AddAddresses(addrs, srcAddr)
n.AddAddresses(addrs, srcAddr, &wire.SubnetworkIDSupportsAll)
numAddrs := n.TotalNumAddresses()
if numAddrs > addrsToAdd {
t.Errorf("Number of addresses is too many %d vs %d", numAddrs, addrsToAdd)
@ -306,7 +306,7 @@ func TestGood(t *testing.T) {
srcAddr := wire.NewNetAddressIPPort(net.IPv4(173, 144, 173, 111), 8333, 0)
n.AddAddresses(addrs, srcAddr)
n.AddAddresses(addrs, srcAddr, &wire.SubnetworkIDSupportsAll)
for i, addr := range addrs {
n.Good(addr, subnetworkIDs[i%subnetworkCount])
}
@ -341,7 +341,7 @@ func TestGetAddress(t *testing.T) {
}
// Add a new address and get it
err := n.AddAddressByIP(someIP + ":8333")
err := n.AddAddressByIP(someIP+":8332", localSubnetworkID)
if err != nil {
t.Fatalf("Adding address failed: %v", err)
}
@ -349,11 +349,37 @@ func TestGetAddress(t *testing.T) {
if ka == nil {
t.Fatalf("Did not get an address where there is one in the pool")
}
// Checks that we don't get it if we find that it has other subnetwork ID than expected.
actualSubnetworkID := &subnetworkid.SubnetworkID{0xfe}
n.Good(ka.NetAddress(), actualSubnetworkID)
ka = n.GetAddress()
if ka != nil {
t.Errorf("Didn't expect to get an address because there shouldn't be any address from subnetwork ID %s or %s", localSubnetworkID, wire.SubnetworkIDSupportsAll)
}
// Checks that the total number of addresses incremented although the new address is not full node or a partial node of the same subnetwork as the local node.
numAddrs := n.TotalNumAddresses()
if numAddrs != 1 {
t.Errorf("Wrong number of addresses: got %d, want %d", numAddrs, 1)
}
// Now we repeat the same process, but now the address has the expected subnetwork ID.
// Add a new address and get it
err = n.AddAddressByIP(someIP+":8333", localSubnetworkID)
if err != nil {
t.Fatalf("Adding address failed: %v", err)
}
ka = n.GetAddress()
if ka == nil {
t.Fatalf("Did not get an address where there is one in the pool")
}
if ka.NetAddress().IP.String() != someIP {
t.Errorf("Wrong IP: got %v, want %v", ka.NetAddress().IP.String(), someIP)
}
if *ka.SubnetworkID() != wire.SubnetworkIDUnknown {
t.Errorf("Wrong Subnetwork ID: got %v, want %v", *ka.SubnetworkID(), wire.SubnetworkIDUnknown)
if !ka.SubnetworkID().IsEqual(localSubnetworkID) {
t.Errorf("Wrong Subnetwork ID: got %v, want %v", *ka.SubnetworkID(), localSubnetworkID)
}
// Mark this as a good address and get it
@ -369,8 +395,8 @@ func TestGetAddress(t *testing.T) {
t.Errorf("Wrong Subnetwork ID: got %v, want %v", ka.SubnetworkID(), localSubnetworkID)
}
numAddrs := n.TotalNumAddresses()
if numAddrs != 1 {
numAddrs = n.TotalNumAddresses()
if numAddrs != 2 {
t.Errorf("Wrong number of addresses: got %d, want %d", numAddrs, 1)
}
}

View File

@ -1122,6 +1122,13 @@ func (sp *Peer) OnAddr(_ *peer.Peer, msg *wire.MsgAddr) {
return
}
if msg.SubnetworkID == nil || (!msg.SubnetworkID.IsEqual(config.MainConfig().SubnetworkID) && !msg.SubnetworkID.IsEqual(&wire.SubnetworkIDSupportsAll)) {
peerLog.Errorf("Only %s and %s subnetwork IDs are allowed in [%s] command, but got subnetwork ID %s from %s",
wire.SubnetworkIDSupportsAll, config.MainConfig().SubnetworkID, msg.Command(), msg.SubnetworkID, sp.Peer)
sp.Disconnect()
return
}
for _, na := range msg.AddrList {
// Don't add more address if we're disconnecting.
if !sp.Connected() {
@ -1145,7 +1152,7 @@ func (sp *Peer) OnAddr(_ *peer.Peer, msg *wire.MsgAddr) {
// addresses, and last seen updates.
// XXX bitcoind gives a 2 hour time penalty here, do we want to do the
// same?
sp.server.addrManager.AddAddresses(msg.AddrList, sp.NA())
sp.server.addrManager.AddAddresses(msg.AddrList, sp.NA(), msg.SubnetworkID)
}
// OnRead is invoked when a peer receives a message and it is used to update
@ -1899,21 +1906,22 @@ func (s *Server) peerHandler() {
}
if !config.MainConfig().DisableDNSSeed {
seedFn := func(addrs []*wire.NetAddress) {
// Bitcoind uses a lookup of the dns seeder here. Since seeder returns
// IPs of nodes and not its own IP, we can not know real IP of
// source. So we'll take first returned address as source.
s.addrManager.AddAddresses(addrs, addrs[0])
seedFromSubNetwork := func(subnetworkID *subnetworkid.SubnetworkID) {
connmgr.SeedFromDNS(config.ActiveNetParams(), defaultRequiredServices,
&wire.SubnetworkIDSupportsAll, serverutils.BTCDLookup, func(addrs []*wire.NetAddress) {
// Bitcoind uses a lookup of the dns seeder here. Since seeder returns
// IPs of nodes and not its own IP, we can not know real IP of
// source. So we'll take first returned address as source.
s.addrManager.AddAddresses(addrs, addrs[0], subnetworkID)
})
}
// Add full nodes discovered through DNS to the address manager.
connmgr.SeedFromDNS(config.ActiveNetParams(), defaultRequiredServices,
&wire.SubnetworkIDSupportsAll, serverutils.BTCDLookup, seedFn)
seedFromSubNetwork(&wire.SubnetworkIDSupportsAll)
if !config.MainConfig().SubnetworkID.IsEqual(&wire.SubnetworkIDSupportsAll) {
// Node is partial - fetch nodes with same subnetwork
connmgr.SeedFromDNS(config.ActiveNetParams(), defaultRequiredServices,
config.MainConfig().SubnetworkID, serverutils.BTCDLookup, seedFn)
seedFromSubNetwork(config.MainConfig().SubnetworkID)
}
}
go s.connManager.Start()

View File

@ -119,9 +119,6 @@ var (
// SubnetworkIDRegistry is the subnetwork ID which is used for adding new sub networks to the registry
SubnetworkIDRegistry = subnetworkid.SubnetworkID{2}
// SubnetworkIDUnknown is the subnetwork ID which is used for marking subnetwork ID as unknown in the adress manager
SubnetworkIDUnknown = subnetworkid.SubnetworkID{3}
)
// scriptFreeList defines a free list of byte slices (up to the maximum number