Refactor permissioning config (#2235)

* refactor: RunnerBuilder Optional<PermissioningConfiguration> args

Remove redundant unpack and re-pack of Optional<PermissioningConfiguration>

Signed-off-by: Antony Denyer <git@antonydenyer.co.uk>

* refactor: BesuCommand to more consistent fluent api


Signed-off-by: Antony Denyer <git@antonydenyer.co.uk>

* refactor: remove unused builder on own instance

Signed-off-by: Antony Denyer <git@antonydenyer.co.uk>

Co-authored-by: Sally MacFarlane <sally.macfarlane@consensys.net>

acceptance-tests/dsl/src/main/java/org/hyperledger/besu/tests/acceptance/dsl/node/ThreadBesuNodeRunner.java

@@ -31,7 +31,6 @@ import org.hyperledger.besu.ethereum.eth.EthProtocolConfiguration;
 import org.hyperledger.besu.ethereum.eth.sync.SynchronizerConfiguration;
 import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolConfiguration;
 import org.hyperledger.besu.ethereum.p2p.peers.EnodeURL;
-import org.hyperledger.besu.ethereum.permissioning.PermissioningConfiguration;
 import org.hyperledger.besu.ethereum.storage.keyvalue.KeyValueStorageProvider;
 import org.hyperledger.besu.ethereum.storage.keyvalue.KeyValueStorageProviderBuilder;
 import org.hyperledger.besu.metrics.MetricsSystemFactory;
@@ -165,12 +164,7 @@ public class ThreadBesuNodeRunner implements BesuNodeRunner {
             .build();
 
     final RunnerBuilder runnerBuilder = new RunnerBuilder();
-    if (node.getPermissioningConfiguration().isPresent()) {
-      final PermissioningConfiguration permissioningConfiguration =
-          node.getPermissioningConfiguration().get();
-
-      runnerBuilder.permissioningConfiguration(permissioningConfiguration);
-    }
+    runnerBuilder.permissioningConfiguration(node.getPermissioningConfiguration());
 
     besuPluginContext.addService(
         BesuEvents.class,

besu/src/main/java/org/hyperledger/besu/RunnerBuilder.java

@@ -288,8 +288,8 @@ public class RunnerBuilder {
   }
 
   public RunnerBuilder permissioningConfiguration(
-      final PermissioningConfiguration permissioningConfiguration) {
-    this.permissioningConfiguration = Optional.of(permissioningConfiguration);
+      final Optional<PermissioningConfiguration> permissioningConfiguration) {
+    this.permissioningConfiguration = permissioningConfiguration;
     return this;
   }
 

besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java

@@ -1186,11 +1186,13 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
       // Need to create vertx after cmdline has been parsed, such that metricsSystem is configurable
       vertx = createVertx(createVertxOptions(metricsSystem.get()));
 
-      final BesuCommand controller = validateOptions().configure().controller();
+      validateOptions();
+      configure();
+      initController();
+      startPlugins();
+      preSynchronization();
+      startSynchronization();
 
-      preSynchronizationTaskRunner.runTasks(controller.besuController);
-
-      controller.startPlugins().startSynchronization();
     } catch (final Exception e) {
       throw new ParameterException(this.commandLine, e.getMessage(), e);
     }
@@ -1335,6 +1337,10 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
     }
   }
 
+  private void preSynchronization() {
+    preSynchronizationTaskRunner.runTasks(besuController);
+  }
+
   private void startSynchronization() {
     synchronize(
         besuController,
@@ -1355,7 +1361,7 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
         pidPath);
   }
 
-  private BesuCommand startPlugins() {
+  private void startPlugins() {
     besuPluginContext.addService(
         BesuEvents.class,
         new BesuEventsImpl(
@@ -1366,7 +1372,6 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
     besuPluginContext.addService(MetricsSystem.class, getMetricsSystem());
     besuController.getAdditionalPluginServices().appendPluginServices(besuPluginContext);
     besuPluginContext.startPlugins();
-    return this;
   }
 
   public void configureLogging(final boolean announce) {
@@ -1394,7 +1399,7 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
     }
   }
 
-  private BesuCommand validateOptions() {
+  private void validateOptions() {
     issueOptionWarnings();
 
     validateP2PInterface(p2pInterface);
@@ -1402,8 +1407,6 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
     validateNatParams();
     validateNetStatsParams();
     validateDnsOptionsParams();
-
-    return this;
   }
 
   @SuppressWarnings("ConstantConditions")
@@ -1535,7 +1538,7 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
     }
   }
 
-  private BesuCommand configure() throws Exception {
+  private void configure() throws Exception {
     checkPortClash();
 
     syncMode =
@@ -1580,7 +1583,7 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
     metricsConfiguration = metricsConfiguration();
 
     logger.info("Security Module: {}", securityModuleName);
-    return this;
+    instantiateSignatureAlgorithmFactory();
   }
 
   private GoQuorumPrivacyParameters configureGoQuorumPrivacy(
@@ -1639,9 +1642,8 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
     }
   }
 
-  private BesuCommand controller() {
+  private void initController() {
     besuController = buildController();
-    return this;
   }
 
   public BesuController buildController() {
@@ -2256,8 +2258,6 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
 
     checkNotNull(runnerBuilder);
 
-    permissioningConfiguration.ifPresent(runnerBuilder::permissioningConfiguration);
-
     final ObservableMetricsSystem metricsSystem = this.metricsSystem.get();
     final Runner runner =
         runnerBuilder
@@ -2269,6 +2269,7 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
             .natMethodFallbackEnabled(unstableNatOptions.getNatMethodFallbackEnabled())
             .discovery(peerDiscoveryEnabled)
             .ethNetworkConfig(ethNetworkConfig)
+            .permissioningConfiguration(permissioningConfiguration)
             .p2pAdvertisedHost(p2pAdvertisedHost)
             .p2pListenInterface(p2pListenInterface)
             .p2pListenPort(p2pListenPort)

besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java

@@ -442,7 +442,8 @@ public class BesuCommandTest extends CommandTestAbstract {
         .permissioningConfiguration(permissioningConfigurationArgumentCaptor.capture());
     verify(mockRunnerBuilder).build();
 
-    final PermissioningConfiguration config = permissioningConfigurationArgumentCaptor.getValue();
+    final PermissioningConfiguration config =
+        permissioningConfigurationArgumentCaptor.getValue().get();
     assertThat(config.getSmartContractConfig().get())
         .isEqualToComparingFieldByField(smartContractPermissioningConfiguration);
 
@@ -468,7 +469,8 @@ public class BesuCommandTest extends CommandTestAbstract {
         .permissioningConfiguration(permissioningConfigurationArgumentCaptor.capture());
     verify(mockRunnerBuilder).build();
 
-    final PermissioningConfiguration config = permissioningConfigurationArgumentCaptor.getValue();
+    final PermissioningConfiguration config =
+        permissioningConfigurationArgumentCaptor.getValue().get();
     assertThat(config.getSmartContractConfig().get())
         .isEqualToComparingFieldByField(expectedConfig);
 
@@ -496,7 +498,8 @@ public class BesuCommandTest extends CommandTestAbstract {
         .permissioningConfiguration(permissioningConfigurationArgumentCaptor.capture());
     verify(mockRunnerBuilder).build();
 
-    final PermissioningConfiguration config = permissioningConfigurationArgumentCaptor.getValue();
+    final PermissioningConfiguration config =
+        permissioningConfigurationArgumentCaptor.getValue().get();
     assertThat(config.getSmartContractConfig().get())
         .isEqualToComparingFieldByField(expectedConfig);
 
@@ -570,7 +573,7 @@ public class BesuCommandTest extends CommandTestAbstract {
     verify(mockRunnerBuilder)
         .permissioningConfiguration(permissioningConfigurationArgumentCaptor.capture());
     final PermissioningConfiguration permissioningConfiguration =
-        permissioningConfigurationArgumentCaptor.getValue();
+        permissioningConfigurationArgumentCaptor.getValue().get();
     assertThat(permissioningConfiguration.getSmartContractConfig()).isPresent();
 
     final SmartContractPermissioningConfiguration effectiveSmartContractConfig =
@@ -697,7 +700,8 @@ public class BesuCommandTest extends CommandTestAbstract {
         .permissioningConfiguration(permissioningConfigurationArgumentCaptor.capture());
     verify(mockRunnerBuilder).build();
 
-    final PermissioningConfiguration config = permissioningConfigurationArgumentCaptor.getValue();
+    final PermissioningConfiguration config =
+        permissioningConfigurationArgumentCaptor.getValue().get();
     assertThat(config.getLocalConfig().get())
         .isEqualToComparingFieldByField(localPermissioningConfiguration);
 
@@ -724,7 +728,7 @@ public class BesuCommandTest extends CommandTestAbstract {
     verify(mockRunnerBuilder)
         .permissioningConfiguration(permissioningConfigurationArgumentCaptor.capture());
     final PermissioningConfiguration permissioningConfiguration =
-        permissioningConfigurationArgumentCaptor.getValue();
+        permissioningConfigurationArgumentCaptor.getValue().get();
     assertThat(permissioningConfiguration.getLocalConfig()).isPresent();
 
     final LocalPermissioningConfiguration effectiveLocalPermissioningConfig =

besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java

@@ -164,7 +164,8 @@ public abstract class CommandTestAbstract {
   @Captor protected ArgumentCaptor<DataStorageConfiguration> dataStorageConfigurationArgumentCaptor;
 
   @Captor
-  protected ArgumentCaptor<PermissioningConfiguration> permissioningConfigurationArgumentCaptor;
+  protected ArgumentCaptor<Optional<PermissioningConfiguration>>
+      permissioningConfigurationArgumentCaptor;
 
   @Captor protected ArgumentCaptor<TransactionPoolConfiguration> transactionPoolConfigCaptor;
 
@@ -216,6 +217,7 @@ public abstract class CommandTestAbstract {
     when(mockRunnerBuilder.p2pAdvertisedHost(anyString())).thenReturn(mockRunnerBuilder);
     when(mockRunnerBuilder.p2pListenPort(anyInt())).thenReturn(mockRunnerBuilder);
     when(mockRunnerBuilder.p2pListenInterface(anyString())).thenReturn(mockRunnerBuilder);
+    when(mockRunnerBuilder.permissioningConfiguration(any())).thenReturn(mockRunnerBuilder);
     when(mockRunnerBuilder.maxPeers(anyInt())).thenReturn(mockRunnerBuilder);
     when(mockRunnerBuilder.limitRemoteWireConnectionsEnabled(anyBoolean()))
         .thenReturn(mockRunnerBuilder);