Handle fork ID and seq in ENRs (#12126)

Author: flcl42

src/Nethermind/Nethermind.Config/NetworkNode.cs

@@ -76,7 +76,7 @@ public static NetworkNode[] ParseNodes(string[]? nodeRecords, ILogger logger)
         return [.. nodes];
     }
 
-    public override string ToString() => IsEnode ? Enode.ToString() : Enr.EnrString;
+    public override string ToString() => IsEnode ? Enode.ToString() : Enr.ToString();
 
     public NetworkNode(PublicKey publicKey, string ip, int port, long reputation = 0)
         : this(new Enode(publicKey, IPAddress.Parse(ip), port)) => Reputation = reputation;

src/Nethermind/Nethermind.Core/Threading/InterlockedEx.cs

@@ -144,4 +144,3 @@ public static ulong Min(ref ulong location, ulong value)
         return current;
     }
 }
-

src/Nethermind/Nethermind.JsonRpc/Modules/Admin/PeerInfo.cs

@@ -65,7 +65,7 @@ private void SetBasicInfo(Peer peer, IReadOnlyList<Capability> capabilities)
             Name = peer.Node.ClientId;
             Enode = peer.Node.ToString(Node.Format.ENode);
             Caps = capabilities;
-            Enr = peer.Node.Enr;
+            Enr = peer.Node.Enr?.ToString();
         }
 
         private void SetNetworkInfo(Peer peer)

src/Nethermind/Nethermind.Network.Discovery.Test/DiscoveryV5AppTests.cs

@@ -3,7 +3,9 @@
 
 using Autofac;
 using Autofac.Features.AttributeFilters;
+using Nethermind.Blockchain;
 using Nethermind.Config;
+using Nethermind.Core;
 using Nethermind.Core.Crypto;
 using Nethermind.Core.Test.Builders;
 using Nethermind.Core.Test.Modules;
@@ -52,13 +54,21 @@ private DiscoveryV5App CreateDiscoveryV5App(IPAddress externalIp, Action<Contain
         IEnode enode = new Enode(nodeKey.PublicKey, externalIp, networkConfig.P2PPort, networkConfig.DiscoveryPort);
         IIPResolver ipResolver = new FixedIpResolver(networkConfig);
         EthereumEcdsa ecdsa = new(0);
+        IBlockTree blockTree = Substitute.For<IBlockTree>();
+        Block head = Build.A.Block.Genesis.TestObject;
+        blockTree.Head.Returns(head);
+        IForkInfo forkInfo = Substitute.For<IForkInfo>();
+        forkInfo.GetForkId(head.Header.Number, head.Header.Timestamp).Returns(new Nethermind.Network.ForkId(0, 0));
         ContainerBuilder builder = new();
         builder.RegisterInstance(LimboLogs.Instance).As<ILogManager>();
         builder.RegisterInstance(networkConfig).As<INetworkConfig>();
         builder.RegisterInstance(enode).As<IEnode>();
         builder.RegisterInstance(ipResolver).As<IIPResolver>();
         builder.RegisterInstance(nodeKey).Keyed<IProtectedPrivateKey>(IProtectedPrivateKey.NodeKey);
         builder.RegisterInstance(ecdsa).As<IEthereumEcdsa>().As<IEcdsa>();
+        builder.RegisterInstance(blockTree).As<IBlockTree>();
+        builder.RegisterInstance(forkInfo).As<IForkInfo>();
+        builder.RegisterInstance(Timestamper.Default).As<ITimestamper>();
         builder.RegisterInstance(new CryptoRandom()).As<ICryptoRandom>();
         builder.RegisterInstance(new NetworkStorage(_discoveryDb, LimboLogs.Instance)).Keyed<INetworkStorage>(DbNames.DiscoveryV5Nodes);
         builder.RegisterInstance(Substitute.For<INodeStatsManager>()).As<INodeStatsManager>();
@@ -189,7 +199,7 @@ public void Should_Accept_Public_Ip_Enr()
     public void Should_Reject_Consensus_Only_Enr()
     {
         NodeRecord enr = CreateTestEnr(TestItem.PrivateKeyA, IPAddress.Parse("8.8.8.8"), includeEth2: true);
-        NodeRecord decoded = NodeRecord.FromEnrString(enr.EnrString);
+        NodeRecord decoded = NodeRecord.FromEnrString(enr.ToString());
 
         bool result = _discoveryV5App.TryGetAcceptableNodeFromEnr(decoded, out Node? node);
 
@@ -231,7 +241,7 @@ public async Task AddNodeToDiscovery_ShouldAddValidatedEnrNode()
         NodeRecord enr = CreateTestEnr(TestItem.PrivateKeyA, IPAddress.Parse("8.8.8.8"), udpPort: 30304);
         Node node = new(TestItem.PrivateKeyA.PublicKey, "1.1.1.1", 30303)
         {
-            Enr = enr.EnrString
+            Enr = enr
         };
 
         try
@@ -242,7 +252,7 @@ public async Task AddNodeToDiscovery_ShouldAddValidatedEnrNode()
                 added.Id.Equals(TestItem.PrivateKeyA.PublicKey) &&
                 added.Host == "8.8.8.8" &&
                 added.Port == 30304 &&
-                added.Enr == enr.EnrString));
+                added.Enr == enr));
         }
         finally
         {
@@ -260,7 +270,7 @@ public async Task AddNodeToDiscovery_ShouldSkipMismatchedEnr()
         NodeRecord enr = CreateTestEnr(TestItem.PrivateKeyA, IPAddress.Parse("8.8.8.8"));
         Node node = new(TestItem.PrivateKeyB.PublicKey, "8.8.8.8", 30303)
         {
-            Enr = enr.EnrString
+            Enr = enr
         };
 
         try
@@ -318,7 +328,7 @@ public void Should_Use_Udp_Port_From_Configured_Enr_Bootnode()
         NodeRecord enr = CreateTestEnr(TestItem.PrivateKeyA, IPAddress.Parse("8.8.8.8"), udpPort: 9001, includeTcp: false);
         NetworkConfig networkConfig = new()
         {
-            Bootnodes = [new NetworkNode(enr.EnrString)]
+            Bootnodes = [new NetworkNode(enr.ToString())]
         };
         DiscoveryConfig discoveryConfig = new()
         {
@@ -331,7 +341,7 @@ public void Should_Use_Udp_Port_From_Configured_Enr_Bootnode()
         {
             Assert.That(bootNodes, Has.Count.EqualTo(1));
             Assert.That(bootNodes[0].Port, Is.EqualTo(9001));
-            Assert.That(bootNodes[0].Enr, Is.EqualTo(enr.EnrString));
+            Assert.That(bootNodes[0].Enr?.ToString(), Is.EqualTo(enr.ToString()));
         }
     }
 

src/Nethermind/Nethermind.Network.Discovery.Test/Discv4/DiscoveryMessageSerializerTests.cs

@@ -134,6 +134,53 @@ public void Ping_with_enr_there_and_back()
         Assert.That(pingMsg.EnrSequence, Is.EqualTo(3));
     }
 
+    [Test]
+    public void Pong_with_enr_there_and_back()
+    {
+        PongMsg pongMsg = new(
+            new IPEndPoint(TestItem.IPEndPointA.Address, 30303),
+            long.MaxValue,
+            TestItem.KeccakA.ValueHash256,
+            3);
+        using DisposableByteBuffer serialized = _messageSerializationService.ZeroSerialize(pongMsg).AsDisposable();
+        pongMsg = _messageSerializationService.Deserialize<PongMsg>(serialized);
+        Assert.That(pongMsg.EnrSequence, Is.EqualTo(3));
+    }
+
+    [Test]
+    public void Pong_with_enr_and_future_trailing_value_deserializes()
+    {
+        byte[] ip = TestItem.IPEndPointA.Address.GetAddressBytes();
+        const int port = 30303;
+        const ulong enrSequence = 3;
+        const string futureValue = "future";
+        ValueHash256? pingMdc = TestItem.KeccakA.ValueHash256;
+        int addressLength = Rlp.LengthOf(ip) + 2 * Rlp.LengthOf(port);
+        int contentLength =
+            Rlp.LengthOfSequence(addressLength) +
+            Rlp.LengthOf(in pingMdc) +
+            Rlp.LengthOf(long.MaxValue) +
+            Rlp.LengthOf(enrSequence) +
+            Rlp.LengthOf(futureValue);
+
+        byte[] data = new byte[Rlp.LengthOfSequence(contentLength)];
+        RlpWriter writer = new(data);
+        writer.StartSequence(contentLength);
+        writer.StartSequence(addressLength);
+        writer.Encode(ip);
+        writer.Encode(port);
+        writer.Encode(port);
+        writer.Encode(in pingMdc);
+        writer.Encode(long.MaxValue);
+        writer.Encode(enrSequence);
+        writer.Encode(futureValue);
+
+        PongMsg pongMsg = _messageSerializationService.Deserialize<PongMsg>(
+            SignAndWrapDiscoveryPacket((byte)MsgType.Pong, data));
+
+        Assert.That(pongMsg.EnrSequence, Is.EqualTo(enrSequence));
+    }
+
     [Test]
     public void Enr_request_there_and_back()
     {

src/Nethermind/Nethermind.Network.Discovery.Test/Discv4/DiscoveryPersistenceManagerTests.cs

@@ -184,7 +184,7 @@ public async Task RunDiscoveryPersistenceCommit_Should_Preserve_Enr_In_Common_St
             NodeRecord enr = TestEnrBuilder.BuildSigned(TestItem.PrivateKeyA, IPAddress.Parse("8.8.8.8"), tcpPort: 30303, udpPort: 30304);
             Node node = new(TestItem.PrivateKeyA.PublicKey, "8.8.8.8", 30304)
             {
-                Enr = enr.EnrString
+                Enr = enr
             };
 
             using CancellationTokenSource cts = new(TimeSpan.FromSeconds(10));
@@ -211,7 +211,7 @@ public async Task RunDiscoveryPersistenceCommit_Should_Preserve_Enr_In_Common_St
                 NodeRecord? persistedEnr = persistedNode.Enr;
                 Assert.That(persistedNode.IsEnr, Is.True);
                 Assert.That(persistedEnr, Is.Not.Null);
-                Assert.That(persistedEnr!.EnrString, Is.EqualTo(enr.EnrString));
+                Assert.That(persistedEnr!.ToString(), Is.EqualTo(enr.ToString()));
                 Assert.That(persistedNode.NodeId, Is.EqualTo(TestItem.PrivateKeyA.PublicKey));
                 Assert.That(persistedNode.Host, Is.EqualTo("8.8.8.8"));
                 Assert.That(persistedNode.Port, Is.EqualTo(30304));

src/Nethermind/Nethermind.Network.Discovery.Test/Discv4/EIP8DiscoveryTests.cs

@@ -52,6 +52,7 @@ public void PongFormatTest()
                                  "42124e";
             PongMsg pong = _messageSerializationService.Deserialize<PongMsg>(Bytes.FromHexString(encodedPong));
             Assert.That(pong.ExpirationTime, Is.EqualTo(1136239445));
+            Assert.That(pong.EnrSequence, Is.Null);
         }
 
         [Test]

src/Nethermind/Nethermind.Network.Discovery.Test/Discv4/Kademlia/KademliaAdapterTests.cs

@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: LGPL-3.0-only
 
 using System;
+using System.Collections.Generic;
 using System.Linq;
 using System.Net;
 using System.Threading;
@@ -176,6 +177,45 @@ private DiscoveryMsg CreateUnsolicitedResponse(MsgType msgType) =>
                 _ => throw new ArgumentOutOfRangeException(nameof(msgType), msgType, null)
             };
 
+        private NodeRecord ConfigureRemoteEnrRefresh(ulong advertisedSequence, ulong responseSequence)
+        {
+            NodeRecord remoteRecord = TestEnrBuilder.BuildSigned(
+                TestItem.PrivateKeyB,
+                IPAddress.Parse("192.168.1.2"),
+                tcpPort: null,
+                udpPort: 30303,
+                enrSequence: responseSequence);
+
+            _msgSender
+                .When(x => x.SendMsg(Arg.Any<PingMsg>()))
+                .Do(ci =>
+                {
+                    PingMsg sent = (PingMsg)ci[0]!;
+                    using DisposableByteBuffer buffer = _receiverSerializationManager.ZeroSerialize(sent).AsDisposable();
+                    PingMsg msg = _receiverSerializationManager.Deserialize<PingMsg>(buffer);
+                    PongMsg pong = new(
+                        msg.FarPublicKey!,
+                        _timestamper.UnixTime.SecondsLong + 1,
+                        sent.Mdc!.Value,
+                        advertisedSequence);
+                    pong.FarAddress = _receiver.Address;
+                    Task.Run(() => _adapter.OnIncomingMsg(pong));
+                });
+
+            _msgSender
+                .When(x => x.SendMsg(Arg.Any<EnrRequestMsg>()))
+                .Do(ci =>
+                {
+                    EnrRequestMsg sent = (EnrRequestMsg)ci[0]!;
+                    ValueHash256 requestHash = TestItem.KeccakA.ValueHash256;
+                    sent.Hash = requestHash;
+                    EnrResponseMsg response = AddReceiverFarAddress(new EnrResponseMsg(_receiver.Address, remoteRecord, new Hash256(requestHash)));
+                    Task.Run(() => _adapter.OnIncomingMsg(response));
+                });
+
+            return remoteRecord;
+        }
+
         [Test]
         [CancelAfter(10000)]
         public async Task Ping_should_send_ping_and_receive_pong(CancellationToken token)
@@ -261,6 +301,54 @@ public async Task SendEnrRequest_should_reject_unsolicited_response_with_wrong_k
             Assert.That(result, Is.Null);
         }
 
+        private static IEnumerable<TestCaseData> RemoteEnrRefreshCases()
+        {
+            yield return new TestCaseData(0UL, 0UL, false, false)
+                .SetName("Ping_should_not_request_remote_enr_when_pong_has_no_advertised_sequence");
+            yield return new TestCaseData(2UL, 2UL, true, true)
+                .SetName("Ping_should_cache_remote_enr_when_response_sequence_matches_advertised_sequence");
+            yield return new TestCaseData(3UL, 2UL, true, false)
+                .SetName("Ping_should_not_cache_remote_enr_when_response_sequence_is_below_advertised_sequence");
+            yield return new TestCaseData(3UL, 4UL, true, true)
+                .SetName("Ping_should_cache_remote_enr_when_response_sequence_is_above_advertised_sequence");
+        }
+
+        [TestCaseSource(nameof(RemoteEnrRefreshCases))]
+        [CancelAfter(10000)]
+        public async Task Ping_should_refresh_remote_enr_from_advertised_sequence(
+            ulong advertisedSequence,
+            ulong responseSequence,
+            bool shouldRequestEnr,
+            bool shouldCacheEnr,
+            CancellationToken token)
+        {
+            NodeRecord remoteRecord = ConfigureRemoteEnrRefresh(advertisedSequence, responseSequence);
+
+            bool result = await _adapter.Ping(_receiver, token);
+
+            Assert.That(result, Is.True);
+            if (shouldRequestEnr)
+            {
+                await _msgSender.Received(1).SendMsg(Arg.Is<EnrRequestMsg>(m => m.FarAddress!.Equals(_receiver.Address)));
+            }
+            else
+            {
+                await _msgSender.DidNotReceive().SendMsg(Arg.Any<EnrRequestMsg>());
+            }
+
+            if (shouldCacheEnr)
+            {
+                _kademliaMessageReceiver.Received(1).AddOrRefresh(Arg.Is<Node>(n =>
+                    n.Id.Equals(_receiver.Id) &&
+                    n.Enr != null &&
+                    n.Enr.ToString() == remoteRecord.ToString()));
+            }
+            else
+            {
+                _kademliaMessageReceiver.DidNotReceive().AddOrRefresh(Arg.Any<Node>());
+            }
+        }
+
         [Test]
         [CancelAfter(10000)]
         public async Task Timed_out_response_handler_should_not_consume_later_unsolicited_message(CancellationToken token)

src/Nethermind/Nethermind.Network.Discovery.Test/Discv5/CodecTests.cs

@@ -305,7 +305,7 @@ public void MessageCodec_Roundtrips_Nodes_From_NonZero_ArraySegment()
         Assert.That(decodedNodes.RequestId, Is.EqualTo(message.RequestId));
         Assert.That(decodedNodes.Total, Is.EqualTo(message.Total));
         Assert.That(decodedNodes.Records.Count, Is.EqualTo(1));
-        Assert.That(decodedNodes.Records[0].EnrString, Is.EqualTo(expectedRecord.EnrString));
+        Assert.That(decodedNodes.Records[0].ToString(), Is.EqualTo(expectedRecord.ToString()));
     }
 
     [Test]
@@ -330,7 +330,7 @@ public void MessageCodec_Skips_Invalid_Enrs_In_Nodes()
         Assert.That(decoded, Is.InstanceOf<NodesMsg>());
         NodesMsg nodes = (NodesMsg)decoded;
         Assert.That(nodes.Records.Count, Is.EqualTo(1));
-        Assert.That(nodes.Records[0].EnrString, Is.EqualTo(expectedRecord.EnrString));
+        Assert.That(nodes.Records[0].ToString(), Is.EqualTo(expectedRecord.ToString()));
     }
 
     [Test]