Merge pull request #2070 from rsksmart/balanced-maxconn-networkcidr

Improve network block validation process

rskj-core/src/main/java/co/rsk/scoring/InetAddressCidrBlock.java

@@ -27,7 +27,7 @@
 import java.util.Objects;
 
 /**
- * InetAddressBlock represents a range of InetAddress as in CIDR subnet
+ * InetAddressCidrBlock represents a range of InetAddress as in CIDR subnet
  */
 public class InetAddressCidrBlock {
     private final String description;

rskj-core/src/main/java/co/rsk/scoring/InetAddressUtils.java

@@ -6,21 +6,21 @@
 
 /**
  * InetAddressUtils has static methods
- * to perform operations on InetAddress and InetAddressBlock
+ * to perform operations on InetAddress and InetAddressCidrBlock
  * given an String representation
  * <p>
  * Created by ajlopez on 15/07/2017.
  */
 public final class InetAddressUtils {
-    private InetAddressUtils() {}
+    private InetAddressUtils() {
+    }
 
     /**
      * Returns <tt>true</tt> if the specified texts represent an address with mask
      * ie "192.168.51.1/16" has a mask
-     *    "192.168.51.1" has no mask
-     *
-     * @param   text    the address
+     * "192.168.51.1" has no mask
      *
+     * @param text the address
      * @return <tt>true</tt> or <tt>false</tt>
      */
     public static boolean hasMask(String text) {
@@ -37,9 +37,8 @@ public static boolean hasMask(String text) {
      * Convert a text representation to an InetAddress
      * It supports IPV4 and IPV6 formats
      *
-     * @param   hostname    the address
-     *
-     * @return  the text converted to an InetAddress
+     * @param hostname the address
+     * @return the text converted to an InetAddress
      */
     public static InetAddress getAddressForBan(@CheckForNull String hostname) throws InvalidInetAddressException {
         if (hostname == null) {
@@ -60,8 +59,7 @@ public static InetAddress getAddressForBan(@CheckForNull String hostname) throws
             }
 
             return address;
-        }
-        catch (UnknownHostException ex) {
+        } catch (UnknownHostException ex) {
             throw new InvalidInetAddressException("unknown host: '" + name + "'", ex);
         }
     }
@@ -72,12 +70,11 @@ public static InetAddress getAddressForBan(@CheckForNull String hostname) throws
      * It supports IPV4 and IPV6 formats
      * ie "192.168.51.1/16" is a valid text
      *
-     * @param   text    the address with mask
-     *
-     * @return  the text converted to an InetAddressBlock
-     * @throws  InvalidInetAddressException if the text is invalid
+     * @param text the address with mask
+     * @return the text converted to an InetAddressBlock
+     * @throws InvalidInetAddressException if the text is invalid
      */
-    public static InetAddressCidrBlock parse(String text) throws InvalidInetAddressException {
+    public static InetAddressCidrBlock createCidrBlock(String text) throws InvalidInetAddressException {
         //TODO(mmarquez): should we validate address format ??
         String[] parts = text.split("/");
 
@@ -89,11 +86,15 @@ public static InetAddressCidrBlock parse(String text) throws InvalidInetAddressE
 
         try {
             nbits = Integer.parseInt(parts[1]);
-        }
-        catch (NumberFormatException ex) {
+        } catch (NumberFormatException ex) {
             throw new InvalidInetAddressBlockException("Invalid mask", ex);
         }
 
+        return createCidrBlock(address, nbits);
+    }
+
+
+    public static InetAddressCidrBlock createCidrBlock(InetAddress address, int nbits) throws InvalidInetAddressException {
         if (nbits <= 0 || nbits > address.getAddress().length * 8) {
             throw new InvalidInetAddressBlockException("Invalid mask", null);
         }

rskj-core/src/main/java/co/rsk/scoring/PeerScoringManager.java

@@ -203,7 +203,7 @@ public void banAddress(InetAddress address) {
     public void banAddress(String address) throws InvalidInetAddressException {
         boolean isAddressBlock = InetAddressUtils.hasMask(address);
         if (isAddressBlock) {
-            this.banAddressBlock(InetAddressUtils.parse(address));
+            this.banAddressBlock(InetAddressUtils.createCidrBlock(address));
         } else {
             this.banAddress(InetAddressUtils.getAddressForBan(address));
         }
@@ -232,7 +232,7 @@ public void unbanAddress(InetAddress address) {
     public void unbanAddress(String address) throws InvalidInetAddressException {
         boolean isAddressBlock = InetAddressUtils.hasMask(address);
         if (isAddressBlock) {
-            this.unbanAddressBlock(InetAddressUtils.parse(address));
+            this.unbanAddressBlock(InetAddressUtils.createCidrBlock(address));
         } else {
             this.unbanAddress(InetAddressUtils.getAddressForBan(address));
         }

rskj-core/src/main/java/org/ethereum/net/server/ChannelManagerImpl.java

@@ -24,7 +24,8 @@
 import co.rsk.net.NodeID;
 import co.rsk.net.Status;
 import co.rsk.net.messages.*;
-import co.rsk.scoring.InetAddressBlock;
+import co.rsk.scoring.InetAddressUtils;
+import co.rsk.scoring.InvalidInetAddressException;
 import com.google.common.annotations.VisibleForTesting;
 import org.ethereum.config.NodeFilter;
 import org.ethereum.core.Block;
@@ -271,11 +272,11 @@ public void notifyDisconnect(Channel channel) {
         logger.debug("Peer {}: notifies about disconnect", channel.getPeerId());
         channel.onDisconnect();
         synchronized (newPeers) {
-            if(newPeers.remove(channel)) {
+            if (newPeers.remove(channel)) {
                 logger.info("Peer removed from new peers list: {}", channel.getPeerId());
             }
-            synchronized (activePeersLock){
-                if(activePeers.values().remove(channel)) {
+            synchronized (activePeersLock) {
+                if (activePeers.values().remove(channel)) {
                     logger.info("Peer removed from active peers list: {}", channel.getPeerId());
                 }
             }
@@ -295,8 +296,16 @@ public boolean isAddressBlockAvailable(InetAddress inetAddress) {
             //TODO(lsebrie): save block address in a data structure and keep updated on each channel add/remove
             //TODO(lsebrie): check if we need to use a different networkCIDR for ipv6
             return activePeers.values().stream()
-                    .map(ch -> new InetAddressBlock(ch.getInetSocketAddress().getAddress(), networkCIDR))
-                    .filter(block -> block.contains(inetAddress))
+                    .map(ch -> {
+                        try {
+                            return InetAddressUtils.createCidrBlock(ch.getInetSocketAddress().getAddress(), networkCIDR);
+                        } catch (InvalidInetAddressException e) {
+                            logger.error(e.getMessage(), e);
+                        }
+
+                        return null;
+                    })
+                    .filter(block -> block != null && block.contains(inetAddress))
                     .count() < maxConnectionsAllowed;
         }
     }
@@ -321,7 +330,7 @@ public Set<NodeID> broadcastTransaction(@Nonnull final Transaction transaction,
      * the peers with an id belonging to the skip set.
      *
      * @param transactions List of Transactions to be sent
-     * @param skip        the set of peers to avoid sending the message.
+     * @param skip         the set of peers to avoid sending the message.
      * @return a set containing the ids of the peers that received the transaction.
      */
     @Override

rskj-core/src/main/resources/reference.conf

@@ -132,7 +132,7 @@ peer {
         # max number of connections allowed on a single address block
         maxConnections = 16
         # the cidr bits used to define a subnet in IPV4 - i.e. 32 bits is a full address
-        networkCidr = 24
+        networkCidr = 16
     }
 
     discovery = {