Load active peers from previous session saved into file. #2230

asoto-iov · 2024-01-18T11:56:45Z

A client node would be able to re-connect to previously connected peers on restart.

Description

Persist discovered peers across node restarts

Motivation and Context

As of now, RSKj node keeps discovered peers in a so-called distance table in memory, so that after node restart those are being lost. Next time that the node starts over it has to start peer discovery process from scratch by having only list of boot nodes.

To improve, and most importantly speed the process up, we want to preserve a list of already discovered peers on a disk.

Note: it looks like we could make use of the org.ethereum.util.MapSnapshot<> class for that purpose.

Expected behaviour:

when node stops, it should save all discovered peers to a file

when node starts, it should check if the file with discovered peers exists in a database directory, and if so, load peer list from there. Otherwise, do nothing and proceed as before

Notes:

this functionality is part of the peer discovery protocol (see the PeerExplorer class)

the node should continue using bootstrap nodes the same way as it does now

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Tests for the changes have been added (for bug fixes / features)
Requires Activation Code (Hard Fork)

Other information:

rskj-core/src/main/java/org/ethereum/util/SimpleFileWriter.java

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

fmacleal

Good job!

I just have some few recommendations. Let me know what you think about them.

rskj-core/src/test/java/co/rsk/config/RskSystemPropertiesTest.java

rskj-core/src/test/java/co/rsk/net/discovery/DiscoveredPeersPersistenceServiceTest.java

rskj-core/src/main/java/org/ethereum/util/SimpleFileWriter.java

rskj-core/src/test/java/co/rsk/net/discovery/DiscoveredPeersPersistenceServiceTest.java

rskj-core/src/test/java/org/ethereum/util/SimpleFileWriterTest.java

rskj-core/src/test/java/co/rsk/config/RskSystemPropertiesTest.java

fmacleal

Good job! I see that all recommendations were handled, I am approving it. Well done! :)

Vovchyk · 2024-02-06T12:15:44Z

rskj-core/src/main/java/co/rsk/NodeRunnerImpl.java

@@ -145,7 +145,11 @@ public synchronized void stop() {
        logger.info("Shutting down RSK node");

        for (int i = internalServices.size() - 1; i >= 0; i--) {
-            internalServices.get(i).stop();
+            try {
+                internalServices.get(i).stop();


could you elaborate a bit more on why you added this try-catch? any specific need?

ok, I see - so this is not required by the feature itself

I guess it's a tradeoff: non-properly finished service may sometimes result to data inconsistency in next services. We should be very careful with such changes in shutdown process

(previous deleted reply) If the stop of any of the services fails throwing an exception the execution would stop and the missing services won't stop nicely.

ok, I see - so this is not required by the feature itself

I guess it's a tradeoff: non-properly finished service may sometimes result to data inconsistency in next services. We should be very careful with such changes in shutdown process

Huh if it could happen I think it is something we should fix. A wrong/different ending of one service shouldn't affect to the others

if that happens then there's a bug in the code which has to be fixed. We don't expect service.stop() to throw any checked exception. Only unchecked exceptions can happen here (eg. RuntimeException), and if that happens we should propagate it call stack up. Not sure it's a good idea to catch an exception and continue stopping other services as node is already in inconsistent state - best we can do here is to catch it at upper level and log it. So I'd rather not to add this try-catch in here

ok got it, I'll remove it. But In my opinion the services should be independent and in case there are dependencies we could create inner groups and close them together. Maybe is something we can try to do in the future.
As an example, in the case of this service. If the service throws an error due to the file persist or any other thing processing an unexpected input data or so, the others services shouldn't be affected and should be stopped properly.

yeah, that was my point. If a service for any reason throws an unchecked exception, which means that the node most probably cannot recover from that state. If the service can recover from an exception that was thrown in the stop() method, then it should not be propagated up but rather handled in it.

Vovchyk · 2024-02-06T12:18:59Z

rskj-core/src/main/java/co/rsk/config/RskSystemProperties.java

@@ -61,6 +61,7 @@ public class RskSystemProperties extends SystemProperties {
    private static final int CHUNK_SIZE = 192;

    public static final String PROPERTY_SYNC_TOP_BEST = "sync.topBest";
+    public static final String USE_PEERS_FROM_LAST_SESSION = "peer.usePeersFromLastSession";


what about keeping it under peer.discovery config section?

rskj-core/src/main/java/co/rsk/net/discovery/DiscoveredPeersPersistenceService.java

rskj-core/src/main/java/org/ethereum/config/SystemProperties.java

rskj-core/src/test/java/co/rsk/net/discovery/DiscoveredPeersPersistenceServiceTest.java

rskj-core/src/test/java/co/rsk/config/RskSystemPropertiesTest.java

Vovchyk · 2024-02-15T08:20:10Z

pipeline:run

rskj-core/src/main/java/co/rsk/net/discovery/PeerExplorer.java

rskj-core/src/main/java/co/rsk/net/discovery/KnownPeersSaver.java

rskj-core/src/main/java/org/ethereum/util/SimpleFileWriter.java

rskj-core/src/main/java/co/rsk/net/discovery/PeerExplorer.java

rskj-core/src/main/java/co/rsk/RskContext.java

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

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

rskj-core/src/test/java/co/rsk/util/SimpleFileWriterTest.java

+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+class SimpleFileWriterTest {


rskj-core/src/main/java/co/rsk/util/SimpleFileWriter.java

+    }
+
+    public void savePropertiesIntoFile(Properties properties, Path filePath) throws IOException {
+        File tempFile = File.createTempFile(filePath.toString(), TMP);


rskj-core/src/main/java/co/rsk/util/SimpleFileWriter.java

+    }
+    public void saveDataIntoFile(String data, Path filePath) throws IOException {
+
+        File tempFile = File.createTempFile(filePath.toString(), TMP);


sonarqubecloud · 2024-02-16T19:09:24Z

Quality Gate passed

Issues
6 New issues

Measures
2 Security Hotspots
73.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

rmoreliovlabs

LGTM

…ed into peerExplorer Updating known peers origin, adding tests and small fixes Removing unnecesary log to avoid log injection Load active peers from previous session saved into file.

…lorer dispose method

sonarqubecloud · 2024-04-09T13:04:19Z

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
2 Security Hotspots
73.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-advanced-security bot found potential problems Jan 18, 2024

View reviewed changes

rskj-core/src/main/java/org/ethereum/util/SimpleFileWriter.java Fixed Show resolved Hide resolved

rskj-core/src/main/java/org/ethereum/net/server/ChannelManagerImpl.java Fixed Show fixed Hide fixed

asoto-iov force-pushed the reconect_previous_peers branch from 8be6a43 to a1e85d5 Compare January 18, 2024 12:01

Vovchyk requested changes Jan 22, 2024

View reviewed changes

rskj-core/src/main/java/org/ethereum/net/server/ChannelManagerImpl.java Outdated Show resolved Hide resolved

fmacleal reviewed Feb 2, 2024

View reviewed changes

asoto-iov force-pushed the reconect_previous_peers branch from 666a418 to 87a3e6a Compare February 5, 2024 17:14

github-advanced-security bot found potential problems Feb 5, 2024

View reviewed changes

fmacleal previously approved these changes Feb 6, 2024

View reviewed changes

Vovchyk requested changes Feb 9, 2024

View reviewed changes

asoto-iov dismissed fmacleal’s stale review via b9ed487 February 13, 2024 15:30

asoto-iov force-pushed the reconect_previous_peers branch from b9ed487 to e155aa7 Compare February 14, 2024 17:48

github-advanced-security bot found potential problems Feb 14, 2024

View reviewed changes

rskj-core/src/test/java/co/rsk/net/discovery/DiscoveredPeersPersistenceServiceTest.java Fixed Show fixed Hide fixed

rskj-core/src/test/java/co/rsk/config/RskSystemPropertiesTest.java Fixed Show fixed Hide fixed

Vovchyk requested changes Feb 15, 2024

View reviewed changes

github-advanced-security bot found potential problems Feb 16, 2024

View reviewed changes

rmoreliovlabs approved these changes Feb 22, 2024

View reviewed changes

asoto-iov force-pushed the reconect_previous_peers branch from 3d37038 to 8e827eb Compare April 8, 2024 08:48

asoto-iov added 3 commits April 9, 2024 14:36

Updating file name, property path and known peer service to be includ…

720e353

…ed into peerExplorer Updating known peers origin, adding tests and small fixes Removing unnecesary log to avoid log injection Load active peers from previous session saved into file.

updating where the peers are saved moving from a service into PeerExp…

8647367

…lorer dispose method

removed unused import

aa6bce5

asoto-iov force-pushed the reconect_previous_peers branch from 8e827eb to aa6bce5 Compare April 9, 2024 12:36

Vovchyk approved these changes Apr 10, 2024

View reviewed changes

Vovchyk merged commit 12c9b32 into master Apr 10, 2024
10 checks passed

Vovchyk deleted the reconect_previous_peers branch April 10, 2024 09:42

aeidelman added this to the Arrowhead 6.2.0 milestone May 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load active peers from previous session saved into file. #2230

Load active peers from previous session saved into file. #2230

asoto-iov commented Jan 18, 2024

fmacleal left a comment

fmacleal left a comment

Vovchyk Feb 6, 2024

Vovchyk Feb 9, 2024

asoto-iov Feb 9, 2024

asoto-iov Feb 9, 2024

Vovchyk Feb 9, 2024

asoto-iov Feb 12, 2024

Vovchyk Feb 13, 2024

Vovchyk Feb 6, 2024

Vovchyk commented Feb 15, 2024

sonarqubecloud bot commented Feb 16, 2024

rmoreliovlabs left a comment

sonarqubecloud bot commented Apr 9, 2024


		import static org.junit.jupiter.api.Assertions.assertEquals;

		class SimpleFileWriterTest {

Load active peers from previous session saved into file. #2230

Load active peers from previous session saved into file. #2230

Conversation

asoto-iov commented Jan 18, 2024

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

fmacleal left a comment

Choose a reason for hiding this comment

fmacleal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vovchyk commented Feb 15, 2024

sonarqubecloud bot commented Feb 16, 2024

Quality Gate passed

rmoreliovlabs left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Apr 9, 2024

Quality Gate passed