Merge main into develop

.github/pull_request_template.md

@@ -1,29 +1,35 @@
 > [!IMPORTANT]
 >
-> * Please read our [Contributing Guidelines](https://github.com/radixdlt/babylon-node/blob/main/CONTRIBUTING.md) before opening a PR.
-> * Before creating your PR, please ensure you have used the _correct base branch_ as per the [branching strategy](https://github.com/radixdlt/babylon-node/blob/main/docs/branching-strategy.md), both for branching from, and in the PR UI above.
->   * For most code changes, this is `develop`.
->   * For stand-alone docs changes, this is `main`.
->   * For workflow changes, this is the oldest supported `release/*` branch. 
-> * Please remove these sections as you fill out your PR.
+> * Please read our [Contributing Guidelines](/CONTRIBUTING.md) before opening a PR.
+> * Before creating your PR, please ensure you read the [branching strategy](/docs/branching-strategy.md). The end result after completing the merge actions should be that `main <= release/XXX <= develop`, where `XXX` is the latest released protocol version. This ensures that we minimise merge conflicts, and that work doesn't go missing.
+> * As per the branching strategy, **you must ensure you select the _correct base branch_**, both for branching from, and in the PR UI above. The following process can be used to decide the base branch:
+>   * For code changes which can wait until the next protocol update to be released, use `develop`. This should be the default for code changes.
+>   * For code changes which need to go out as a fully-interoperable update to the node at the current protocol version, use `release/XXX`.
+>     * Such changes must be tested and reviewed more carefully to mitigate the risk of regression.
+>     * Once the change is merged, it is the merger's responsibility to ensure `release/XXX` is merged into the `develop` branch.
+>   * For github workflow changes, use `main`.
+>     * Once the change is merged, it is the merger's responsibility to ensure `main` is merged into both `release/XXX` and `develop`, so that the changes also apply to hotfixes, and to current development.
+>   * For changes to README files, use `main`.
+>     * Once the change is merged, it is the merger's responsibility to ensure `main` is merged into both `release/XXX` and `develop`, so that the changes also apply on these branches.
 > 
+> _Please remove this section once you confirm you follow its guidance._
 
 ## Summary
 
+<!--
 > [!TIP]
 > 
 > Start with the context of your PR. Why are you making this change? What does it address? Link back to an issue if relevant.
 > 
-> Then summarise the changes that were made. Bullet points are fine.
-
-## Details
-
-> [!TIP]
-> 
-> This section is optional. Go into more detail about the changes that were made, or the thinking behind the changes.
+> Then summarise the changes that were made.
+> * Bullet points are fine.
+> * Feel free to add additional subheadings (using ###) with more information if required.
+-->
 
 ## Testing
 
+<!--
 > [!TIP]
 > 
 > Explain what testing / verification is done, including manual testing or automated testing.
+-->

core/src/main/java/com/radixdlt/p2p/hostip/NetworkQueryHostIp.java

@@ -72,8 +72,6 @@
 import com.radixdlt.lang.Result;
 import com.radixdlt.utils.properties.RuntimeProperties;
 import java.io.IOException;
-import java.net.MalformedURLException;
-import java.net.URL;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -98,21 +96,21 @@ final class NetworkQueryHostIp {
 
   public record VotedResult(
       Optional<HostIp> conclusiveHostIp,
-      ImmutableMap<URL, Result<HostIp, IOException>> individualQueryResults) {}
+      ImmutableMap<String, Result<HostIp, IOException>> individualQueryResults) {}
 
   @VisibleForTesting static final String QUERY_URLS_PROPERTY = "network.host_ip_query_urls";
 
   @VisibleForTesting
-  static final ImmutableList<URL> DEFAULT_QUERY_URLS =
+  static final ImmutableList<String> DEFAULT_QUERY_URLS =
       ImmutableList.of(
-          makeurl("https://checkip.amazonaws.com/"),
-          makeurl("https://ipv4.icanhazip.com/"),
-          makeurl("https://myexternalip.com/raw"),
-          makeurl("https://ipecho.net/plain"),
-          makeurl("https://www.trackip.net/ip"),
-          makeurl("https://ifconfig.co/ip"));
-
-  static NetworkQueryHostIp create(Collection<URL> urls) {
+          "https://checkip.amazonaws.com/",
+          "https://ipv4.icanhazip.com/",
+          "https://myexternalip.com/raw",
+          "https://ipecho.net/plain",
+          "https://www.trackip.net/ip",
+          "https://ifconfig.co/ip");
+
+  static NetworkQueryHostIp create(Collection<String> urls) {
     return new NetworkQueryHostIp(urls);
   }
 
@@ -121,18 +119,20 @@ static NetworkQueryHostIp create(RuntimeProperties properties) {
     if (urlsProperty == null || urlsProperty.trim().isEmpty()) {
       return create(DEFAULT_QUERY_URLS);
     }
-    ImmutableList<URL> urls =
-        Arrays.asList(urlsProperty.split(",")).stream()
-            .map(NetworkQueryHostIp::makeurl)
-            .collect(ImmutableList.toImmutableList());
+    ImmutableList<String> urls =
+        Arrays.asList(urlsProperty.split(",")).stream().collect(ImmutableList.toImmutableList());
     return create(urls);
   }
 
-  private final List<URL> hosts;
+  private final List<String> hosts;
   private final OkHttpClient okHttpClient;
   private final Supplier<VotedResult> result = Suppliers.memoize(this::get);
 
-  NetworkQueryHostIp(Collection<URL> urls) {
+  NetworkQueryHostIp() {
+    this(DEFAULT_QUERY_URLS);
+  }
+
+  NetworkQueryHostIp(Collection<String> urls) {
     if (urls.isEmpty()) {
       throw new IllegalArgumentException("At least one URL must be specified");
     }
@@ -144,20 +144,24 @@ int count() {
     return this.hosts.size();
   }
 
+  List<String> hosts() {
+    return this.hosts;
+  }
+
   public VotedResult queryNetworkHosts() {
     return result.get();
   }
 
   VotedResult get() {
     // Make sure we don't DoS the first one on the list
-    Collections.shuffle(this.hosts);
-    log.debug("Using hosts {}", this.hosts);
+    Collections.shuffle(this.hosts());
+    log.debug("Using hosts {}", this.hosts());
     final Map<HostIp, AtomicInteger> successCounts = Maps.newHashMap();
-    final ImmutableMap.Builder<URL, Result<HostIp, IOException>> queryResults =
+    final ImmutableMap.Builder<String, Result<HostIp, IOException>> queryResults =
         ImmutableMap.builder();
     int maxCount = 0;
     Optional<HostIp> maxResult = Optional.empty();
-    for (URL url : this.hosts) {
+    for (String url : this.hosts()) {
       final Result<HostIp, IOException> result = query(url);
       queryResults.put(url, result);
       if (result.isSuccess()) {
@@ -191,7 +195,7 @@ VotedResult get() {
     return new VotedResult(maxResult, queryResults.build());
   }
 
-  Result<HostIp, IOException> query(URL url) {
+  Result<HostIp, IOException> query(String url) {
     try {
       // Some services simply require the headers we set here:
       final var request =
@@ -214,12 +218,4 @@ Result<HostIp, IOException> query(URL url) {
       return Result.error(ex);
     }
   }
-
-  private static URL makeurl(String s) {
-    try {
-      return new URL(s);
-    } catch (MalformedURLException ex) {
-      throw new IllegalStateException("While constructing URL for " + s, ex);
-    }
-  }
 }

core/src/test/java/com/radixdlt/p2p/hostip/NetworkQueryHostIpTest.java

@@ -65,18 +65,19 @@
 package com.radixdlt.p2p.hostip;
 
 import static org.junit.Assert.*;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.*;
 
+import com.radixdlt.lang.Result;
 import com.radixdlt.utils.properties.RuntimeProperties;
-import java.io.ByteArrayInputStream;
 import java.io.IOException;
-import java.net.HttpURLConnection;
-import java.net.URL;
-import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
 import java.util.Collections;
-import java.util.List;
+import java.util.Map;
 import java.util.Optional;
 import org.junit.Test;
+import org.mockito.Mockito;
 
 public class NetworkQueryHostIpTest {
 
@@ -112,43 +113,31 @@ public void testCollectionEmpty() {
 
   @Test
   public void testCollectionNotEmptyQueryNotSuccessful() throws IOException {
-    NetworkQueryHostIp nqhip = NetworkQueryHostIp.create(List.of(makeUrl(404, "not found")));
-    Optional<HostIp> host = nqhip.queryNetworkHosts().conclusiveHostIp();
-    assertFalse(host.isPresent());
-  }
-
-  @Test
-  public void testCollectionNotEmptyQueryIoException() throws IOException {
-    URL url = mock(URL.class);
-    doReturn("https://mock.url/with-io-problems").when(url).toString();
-    doThrow(new IOException("test exception")).when(url).openConnection();
-    NetworkQueryHostIp nqhip = NetworkQueryHostIp.create(List.of(url));
+    NetworkQueryHostIp nqhip =
+        makeNetworkQueryHostIp(Map.of("a", Result.error(new IOException(""))));
     Optional<HostIp> host = nqhip.queryNetworkHosts().conclusiveHostIp();
     assertFalse(host.isPresent());
   }
 
   @Test
   public void testCollectionAllDifferent() throws IOException {
-    List<URL> urls =
-        List.of(
-            makeUrl(200, "127.0.0.1"),
-            makeUrl(200, "127.0.0.2"),
-            makeUrl(200, "127.0.0.3"),
-            makeUrl(200, "127.0.0.4"));
-    NetworkQueryHostIp nqhip = NetworkQueryHostIp.create(urls);
+    NetworkQueryHostIp nqhip =
+        makeNetworkQueryHostIp(
+            Map.of(
+                "a", Result.success(new HostIp("1")),
+                "b", Result.success(new HostIp("2")),
+                "c", Result.success(new HostIp("2")),
+                "d", Result.success(new HostIp("4"))));
     Optional<HostIp> host = nqhip.queryNetworkHosts().conclusiveHostIp();
-    assertFalse(host.isPresent());
+    assertEquals(Optional.of(new HostIp("2")), host);
   }
 
-  private static URL makeUrl(int status, String body) throws IOException {
-    HttpURLConnection conn = mock(HttpURLConnection.class);
-    doReturn(status).when(conn).getResponseCode();
-    doReturn(new ByteArrayInputStream(body.getBytes(StandardCharsets.UTF_8)))
-        .when(conn)
-        .getInputStream();
-    URL url = mock(URL.class);
-    doReturn(conn).when(url).openConnection();
-    doReturn("https://mock.url/?result=%s".formatted(body)).when(url).toString();
-    return url;
+  NetworkQueryHostIp makeNetworkQueryHostIp(Map<String, Result<HostIp, IOException>> responses) {
+    NetworkQueryHostIp nqhip = spy(NetworkQueryHostIp.class);
+    doReturn(new ArrayList<String>(responses.keySet())).when(nqhip).hosts();
+    for (var entry : responses.entrySet()) {
+      doReturn(entry.getValue()).when(nqhip).query(Mockito.eq(entry.getKey()));
+    }
+    return nqhip;
   }
 }

docs/branching-strategy.md

@@ -3,67 +3,62 @@
 
 Once you have read the [contributing guide](../CONTRIBUTING.md), if you want to start development, you will need to know which branches to use.
 
-> [!NOTE]
-> Supported base branches:
-> * `develop` - The primary development branch for upcoming features and enhancements
-> * `release/*` - Various past and future releases
-> * `main` - A mirror of the latest release
-> 
-> When clicking merge on a PR to one of these branches, it is your duty to ensure that PRs are raised to merge that branch into all later/downstream supported base branches.
-
-## Summary of approach
-
-### The base branches
-
 We use a variant of `git-flow`, where there are three types of base branches: the `main`, `develop`, and `release/*` branches.
 
-* The `main` branch is the public-facing base branch and **represents the last official release**. It's also used for docs.
-* The `develop` branch is the primary integration branch, for work targeting the next protocol version.
-* The `release/*` branches are for all named protocol versions (i.e. each 1.X in the naming scheme. Typically patch releases should re-use same branch). A subset of release branches are **currently supported** - typically these are those currently on a live environment, or under development.
-
-At any given time, there is a strict ordering on the supported base branches, where the process aims to guarantee all work on previous branches is in the later/downstream branches. This order (from earliest/most upstream to latest/most downstream) is as follows:
-
-* Released `release/*` branches still compatibile with a mainnet network
-* `main`
-* Unreleased `release/*` branches
-* `develop`
-
-The latest ordering is at the top of this document.
+> [!IMPORTANT]
+> The currently supported base branches are as follows:
+> * `develop` - The primary development branch for features and enhancements to be released at the next protocol version.
+> * `release/XXX` = `release/cuttlefish` - The latest published protocol version. This is used for hotfixes or low-risk protocol-compatible changes which will be released as an optional node update.
+> * `main` - The public-facing base branch, which represents the docs and code for the latest official release.
+>
+> This list is ordered by "each branch should contain every change in each branch below it". When clicking merge on a PR to one of these base branches `B`, it is your duty to ensure that a new PR is raised to merge `B` into all branches above `B` in the list. This ensures that work ends up in the right place, we minimise merge conflicts, and that work doesn't go missing. See the [Development and Merge Process](#developmentmerge-process) section for more information.
 
-### Development process
+## Development/Merge process
 
 When working on changes:
-* You will first need to select the correct base branch to create your feature branch from. For some epics, it is acceptable to choose a long running `feature/*` or `epic/*` branch as your base, to break up the work into separate reviews.
+* You will first need to select the correct base branch to create your feature branch from. For some epics, it is acceptable to choose a long running `feature/*` or `epic/*` branch as your base, to break up the work into separate reviews, but to merge it atomically once it's been properly tested.
 * Your branch should start `feature/*`, or variants on naming such as `hotfix/*`, `tweak/*`, `docs/*` are permitted. The specific name should be prefixed by a JIRA ticket or Github issue reference where appropriate, e.g. `feature/NODE-123-my-feature` for JIRA tickets or `feature/gh-1235-my-feature` for github issues.
 * When you raise a PR, you will need to ensure you select the appropriate base branch before creating the PR. **The default, `main` is typically not correct!**
 
-> [!IMPORTANT]
-> 
-> Finally, when a PR is merged, it is **the PR merger's responsibility** to ensure that the _base branch_ that was merged into is then merged into _all downstream base branches_ (ideally one by one, like a waterfall).
->
-> If there is a merge conflict, this should be handled by creating a special `conflict/X-into-Y-DATE` branch (for branches `X`, `Y` and `DATE`) from `X`, and putting in a PR with a merge target of `Y`.
->
-> But if this process is properly followed, such merge conflicts will be rare. 
+Finally, when a PR is merged, it is **the PR merger's responsibility** to ensure that the _base branch_ that was merged into is then merged into _all downstream base branches_. If there is a merge conflict, this should be handled by creating a special `conflict/X-into-Y-DATE` branch (for branches `X`, `Y` and `DATE`) from `X`, and putting in a PR with a merge target of `Y`. If this process is properly followed, such merge conflicts will be rare. 
 
-## Which base branch should I use for X?
+## Which base branch should I use for my change?
 
 ### Code changes
 
-Features branches usually branch from `develop`, unless they need to target the current/previous protocol version, in which case, they will need to target the appropriate `release/*` branch.
+For most code changes, choose `develop`. Code against the `develop` branch will be released at the next protocol update.
 
-### Stand-alone doc changes
+For code changes which need to go out as a fully-interoperable update to the node at the current protocol version, use the current `release/XXX` branch. Such changes will be reviewed more carefully to mitigate the risk of regression. Once the change is merged, it is the merger's responsibility to ensure `release/XXX` is merged into the develop branch.
 
-Public facing docs change unrelated to another ticket should use a base branch of `main` - as this is the branch which is first visible when someone looks at the repository.
+### Stand-alone README changes
+
+Public facing docs change unrelated to another ticket should use a base branch of `main` - as this is the branch which is first visible when someone looks at the repository. Once the change is merged, it is the merger's responsibility to ensure `main` is merged into both `release/XXX` and `develop` branches to avoid merge conflicts or confusion.
 
 ### Workflow / CI changes
 
-Workflow changes should branch from the _most upstream_ (earliest) supported branch. Typically this is a `release/*` branch.
+For github workflow changes, start by branching off of and merging to the current `main` branch.
+
+Once the change is merged, it is the merger's responsibility to ensure `main` is merged into both `release/XXX` and `develop`, so that the changes also apply for current development, and for any hotfixes which need to be built and release.
+
+## Base branch change process
 
-Once the post-merge process is followed, this change will find itself on all later/downstream base branches too.
+* When a release is published, as part of the release process, `release/XXX` will get merged into `main`, which should effectively set `main == release/XXX`.
+* When a new protocol update is about to be published, late in the process:
+  * A `release/YYY` branch will be created from `develop`
+  * Any existing PRs will be reviewed and either have their base branch adjusted to `release/YYY` or kept against `develop`
+  * The active branch at the top of this file will be updated to `release/YYY`
 
-This ensures that these changes are on all supported branches, so builds can be built on `develop` or on all supported branches.
+## Background Detail
 
-## Merge or Rebase/Cherry-pick?
+### Diagram
+
+The following demonstrates a possible branch structure under this strategy, under the hypothetical scenario where `bottlenose` is the current live protocol version, and `cuttlefish` is being prepared but not yet live.
+
+Admittedly, this isn't particularly easy to follow. The key with this strategy is following the rules. If the rules are followed, you don't need to visualize the structure.
+
+![Diagram summarising the branching strategy](./branching-diagram.png)
+
+### Merge or Rebase/Cherry-pick?
 
 This strategy relies on the fact that we always merge.
 
@@ -77,15 +72,7 @@ We acknowledge the weakness of merging that this can make the git history messie
 
 At merge time, it is acceptable but not recommended to squash-merge. We encourage developers to instead squash commits before asking for a review. This results in a better record of the review / iteration process.
 
-## Diagram
-
-The following demonstrates a possible branch structure under this strategy, under the hypothetical scenario where `bottlenose` is the current live protocol version, and `cuttlefish` is being prepared but not yet live.
-
-Admittedly, this isn't particularly easy to follow. The key with this strategy is following the rules. If the rules are followed, you don't need to visualize the structure.
-
-![Diagram summarising the branching strategy](./branching-diagram.png)
-
-## Why do we follow this model?
+### Why do we follow this model?
 
 In order to support a network built upon deterministic execution of the radix engine, we need to have a very clear policy of what is compatible with what. This is where the protocol version strategy comes in. And this maps to git via the `release/*` branch strategy.