Move plugin validation to happen after startPlugins (#2957)

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

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

@@ -130,8 +130,10 @@ public class ProcessBesuNodeRunner implements BesuNodeRunner {
         params.add(node.getPrivacyParameters().getEnclavePublicKeyFile().getAbsolutePath());
       }
 
-      params.add("--privacy-marker-transaction-signing-key-file");
-      params.add(node.homeDirectory().resolve("key").toString());
+      if (!node.getExtraCLIOptions().contains("--plugin-privacy-service-signing-enabled=true")) {
+        params.add("--privacy-marker-transaction-signing-key-file");
+        params.add(node.homeDirectory().resolve("key").toString());
+      }
 
       if (node.getPrivacyParameters().isOnchainPrivacyGroupsEnabled()) {
         params.add("--privacy-onchain-groups-enabled");

acceptance-tests/dsl/src/main/java/org/hyperledger/besu/tests/acceptance/dsl/privacy/PrivacyNode.java

@@ -196,18 +196,23 @@ public class PrivacyNode implements AutoCloseable {
       final Path dataDir = Files.createTempDirectory("acctest-privacy");
       final Path dbDir = dataDir.resolve(DATABASE_PATH);
 
-      privacyParameters =
+      var builder =
           new PrivacyParameters.Builder()
               .setEnabled(true)
               .setEnclaveUrl(enclave.clientUrl())
-              .setPrivacyUserIdUsingFile(enclave.getPublicKeyPaths().get(0).toFile())
               .setStorageProvider(createKeyValueStorageProvider(dataDir, dbDir))
               .setPrivateKeyPath(KeyPairUtil.getDefaultKeyFile(besu.homeDirectory()).toPath())
               .setEnclaveFactory(new EnclaveFactory(vertx))
               .setOnchainPrivacyGroupsEnabled(isOnchainPrivacyEnabled)
               .setMultiTenancyEnabled(isMultitenancyEnabled)
-              .setPrivacyPluginEnabled(isPrivacyPluginEnabled)
-              .build();
+              .setPrivacyPluginEnabled(isPrivacyPluginEnabled);
+
+      if (enclave.getPublicKeyPaths().size() > 0) {
+        builder.setPrivacyUserIdUsingFile(enclave.getPublicKeyPaths().get(0).toFile());
+      }
+
+      privacyParameters = builder.build();
+
     } catch (final IOException e) {
       throw new RuntimeException(e);
     }

acceptance-tests/test-plugins/src/main/java/org/hyperledger/besu/plugins/BadCLIOptionsPlugin.java

@@ -45,11 +45,13 @@ public class BadCLIOptionsPlugin implements BesuPlugin {
     callbackDir = new File(System.getProperty("besu.plugins.dir", "plugins"));
     writeStatus("init");
 
-    context
-        .getService(PicoCLIOptions.class)
-        .ifPresent(
-            picoCLIOptions ->
-                picoCLIOptions.addPicoCLIOptions("bad-cli", BadCLIOptionsPlugin.this));
+    if (System.getProperty("TEST_BAD_CLI", "false").equals("true")) {
+      context
+          .getService(PicoCLIOptions.class)
+          .ifPresent(
+              picoCLIOptions ->
+                  picoCLIOptions.addPicoCLIOptions("bad-cli", BadCLIOptionsPlugin.this));
+    }
 
     writeStatus("register");
   }

acceptance-tests/test-plugins/src/main/java/org/hyperledger/besu/plugins/TestPrivacyServicePlugin.java

@@ -34,7 +34,6 @@ public class TestPrivacyServicePlugin implements BesuPlugin {
   PrivacyPluginService pluginService;
   BesuContext context;
 
-  TestPrivacyPluginPayloadProvider payloadProvider = new TestPrivacyPluginPayloadProvider();
   TestPrivacyGroupGenesisProvider privacyGroupGenesisProvider =
       new TestPrivacyGroupGenesisProvider();
   TestSigningPrivateMarkerTransactionFactory privateMarkerTransactionFactory =
@@ -46,8 +45,6 @@ public class TestPrivacyServicePlugin implements BesuPlugin {
 
     context.getService(PicoCLIOptions.class).get().addPicoCLIOptions("privacy-service", this);
     pluginService = context.getService(PrivacyPluginService.class).get();
-
-    pluginService.setPayloadProvider(payloadProvider);
     pluginService.setPrivacyGroupGenesisProvider(privacyGroupGenesisProvider);
 
     LOG.info("Registering Plugins with options " + this);
@@ -57,6 +54,9 @@ public class TestPrivacyServicePlugin implements BesuPlugin {
   public void start() {
     LOG.info("Start Plugins with options " + this);
 
+    TestPrivacyPluginPayloadProvider payloadProvider = new TestPrivacyPluginPayloadProvider();
+
+    pluginService.setPayloadProvider(payloadProvider);
     payloadProvider.setPluginPayloadPrefix(prefix);
 
     if (genesisEnabled) {

acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/plugins/BadCLIOptionsPluginTest.java

@@ -28,6 +28,7 @@ import java.util.concurrent.TimeUnit;
 import java.util.stream.Stream;
 
 import org.awaitility.Awaitility;
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Ignore;
 import org.junit.Test;
@@ -37,12 +38,19 @@ public class BadCLIOptionsPluginTest extends AcceptanceTestBase {
 
   @Before
   public void setUp() throws Exception {
+    System.setProperty("TEST_BAD_CLI", "true");
+
     node =
         besu.createPluginsNode(
             "node1", Collections.singletonList("testPlugins"), Collections.emptyList());
     cluster.start(node);
   }
 
+  @After
+  public void tearDown() {
+    System.setProperty("TEST_BAD_CLI", "false");
+  }
+
   @Test
   public void shouldNotRegister() {
     final Path registrationFile = node.homeDirectory().resolve("plugins/badCLIOptions.init");

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

@@ -285,7 +285,7 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
   private final StorageServiceImpl storageService;
   private final SecurityModuleServiceImpl securityModuleService;
   private final PermissioningServiceImpl permissioningService;
-  private final PrivacyPluginServiceImpl privacyPluginPluginService;
+  private final PrivacyPluginServiceImpl privacyPluginService;
   private final RpcEndpointServiceImpl rpcEndpointServiceImpl;
 
   private final Map<String, String> environment;
@@ -1161,7 +1161,7 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
       final StorageServiceImpl storageService,
       final SecurityModuleServiceImpl securityModuleService,
       final PermissioningServiceImpl permissioningService,
-      final PrivacyPluginServiceImpl privacyPluginPluginService,
+      final PrivacyPluginServiceImpl privacyPluginService,
       final PkiBlockCreationConfigurationProvider pkiBlockCreationConfigProvider,
       final RpcEndpointServiceImpl rpcEndpointServiceImpl) {
     this.logger = logger;
@@ -1175,7 +1175,7 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
     this.storageService = storageService;
     this.securityModuleService = securityModuleService;
     this.permissioningService = permissioningService;
-    this.privacyPluginPluginService = privacyPluginPluginService;
+    this.privacyPluginService = privacyPluginService;
     pluginCommonConfiguration = new BesuCommandConfigurationService();
     besuPluginContext.addService(BesuConfiguration.class, pluginCommonConfiguration);
     this.pkiBlockCreationConfigProvider = pkiBlockCreationConfigProvider;
@@ -1224,6 +1224,7 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
       configure();
       initController();
       startPlugins();
+      validatePluginOptions();
       preSynchronization();
       startSynchronization();
 
@@ -1317,7 +1318,7 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
     besuPluginContext.addService(StorageService.class, storageService);
     besuPluginContext.addService(MetricCategoryRegistry.class, metricCategoryRegistry);
     besuPluginContext.addService(PermissioningService.class, permissioningService);
-    besuPluginContext.addService(PrivacyPluginService.class, privacyPluginPluginService);
+    besuPluginContext.addService(PrivacyPluginService.class, privacyPluginService);
     besuPluginContext.addService(RpcEndpointService.class, rpcEndpointServiceImpl);
 
     // register built-in plugins
@@ -1418,6 +1419,47 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
     besuPluginContext.startPlugins();
   }
 
+  private void validatePluginOptions() {
+    // plugins do not 'wire up' until start has been called
+    // consequently you can only do some configuration checks
+    // after start has been called on plugins
+
+    if (isPrivacyEnabled) {
+
+      if (privateMarkerTransactionSigningKeyPath != null
+          && privacyPluginService != null
+          && privacyPluginService.getPrivateMarkerTransactionFactory() != null) {
+        throw new ParameterException(
+            commandLine,
+            "--privacy-marker-transaction-signing-key-file can not be used in conjunction with a plugin that specifies a PrivateMarkerTransactionFactory");
+      }
+
+      if (Wei.ZERO.compareTo(minTransactionGasPrice) < 0) {
+        // if gas is required, cannot use random keys to sign private tx
+        // ie --privacy-marker-transaction-signing-key-file must be set
+        if (privateMarkerTransactionSigningKeyPath == null
+            && (privacyPluginService == null
+                || privacyPluginService.getPrivateMarkerTransactionFactory() == null)) {
+          throw new ParameterException(
+              commandLine,
+              "Not a free gas network. --privacy-marker-transaction-signing-key-file must be specified and must be a funded account. Private transactions cannot be signed by random (non-funded) accounts in paid gas networks");
+        }
+      }
+
+      if (unstablePrivacyPluginOptions.isPrivacyPluginEnabled()
+          && privacyPluginService.getPayloadProvider() == null) {
+        throw new ParameterException(
+            commandLine,
+            "No Payload Provider has been provided. You must register one when enabling privacy plugin!");
+      }
+
+      if (unstablePrivacyPluginOptions.isPrivacyPluginEnabled() && isFlexiblePrivacyGroupsEnabled) {
+        throw new ParameterException(
+            commandLine, "Privacy Plugin can not be used with flexible (onchain) privacy groups");
+      }
+    }
+  }
+
   public void configureLogging(final boolean announce) {
     // To change the configuration if color was enabled/disabled
     Configurator.reconfigure();
@@ -2165,19 +2207,6 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
             "Privacy multi-tenancy requires either http authentication to be enabled or WebSocket authentication to be enabled");
       }
 
-      if (unstablePrivacyPluginOptions.isPrivacyPluginEnabled()
-          && privacyPluginPluginService.getPayloadProvider() == null) {
-        // The plugin may register the payload provider in start or register.
-        // At this point we have only called start.
-        logger.warn(
-            "No Payload Provider has been provided. You must register one when enabling privacy plugin!");
-      }
-
-      if (unstablePrivacyPluginOptions.isPrivacyPluginEnabled() && isFlexiblePrivacyGroupsEnabled) {
-        throw new ParameterException(
-            commandLine, "Privacy Plugin can not be used with flexible (onchain) privacy groups");
-      }
-
       privacyParametersBuilder.setEnabled(true);
       privacyParametersBuilder.setEnclaveUrl(privacyUrl);
       privacyParametersBuilder.setMultiTenancyEnabled(isPrivacyMultiTenancyEnabled);
@@ -2192,7 +2221,9 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
             commandLine, "Privacy multi-tenancy and privacy public key cannot be used together");
       }
 
-      if (!hasPrivacyPublicKey && !isPrivacyMultiTenancyEnabled) {
+      if (!hasPrivacyPublicKey
+          && !isPrivacyMultiTenancyEnabled
+          && !unstablePrivacyPluginOptions.isPrivacyPluginEnabled()) {
         throw new ParameterException(
             commandLine, "Please specify Enclave public key file path to enable privacy");
       }
@@ -2209,26 +2240,6 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
         }
       }
 
-      if (Wei.ZERO.compareTo(minTransactionGasPrice) < 0) {
-        // if gas is required, cannot use random keys to sign private tx
-        // ie --privacy-marker-transaction-signing-key-file must be set
-        if (privateMarkerTransactionSigningKeyPath == null
-            && (privacyPluginPluginService == null
-                || privacyPluginPluginService.getPrivateMarkerTransactionFactory() == null)) {
-          throw new ParameterException(
-              commandLine,
-              "Not a free gas network. --privacy-marker-transaction-signing-key-file must be specified and must be a funded account. Private transactions cannot be signed by random (non-funded) accounts in paid gas networks");
-        }
-      }
-
-      if (privateMarkerTransactionSigningKeyPath != null
-          && privacyPluginPluginService != null
-          && privacyPluginPluginService.getPrivateMarkerTransactionFactory() != null) {
-        throw new ParameterException(
-            commandLine,
-            "--privacy-marker-transaction-signing-key-file can not be used in conjunction with a plugin that specifies a PrivateMarkerTransactionFactory");
-      }
-
       privacyParametersBuilder.setPrivateKeyPath(privateMarkerTransactionSigningKeyPath);
       privacyParametersBuilder.setStorageProvider(
           privacyKeyStorageProvider(keyValueStorageName + "-privacy"));
@@ -2252,7 +2263,7 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
             || rpcWsApis.contains(RpcApis.GOQUORUM.name()))) {
       logger.warn("Cannot use GOQUORUM API methods when not in GoQuorum mode.");
     }
-    privacyParametersBuilder.setPrivacyService(privacyPluginPluginService);
+    privacyParametersBuilder.setPrivacyService(privacyPluginService);
     final PrivacyParameters privacyParameters = privacyParametersBuilder.build();
 
     if (isPrivacyEnabled) {

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

@@ -42,6 +42,7 @@ import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.ArgumentMatchers.isNotNull;
 import static org.mockito.Mockito.atLeast;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoInteractions;
 import static org.mockito.Mockito.when;
@@ -75,6 +76,7 @@ import org.hyperledger.besu.metrics.prometheus.MetricsConfiguration;
 import org.hyperledger.besu.nat.NatMethod;
 import org.hyperledger.besu.pki.config.PkiKeyStoreConfiguration;
 import org.hyperledger.besu.plugin.data.EnodeURL;
+import org.hyperledger.besu.plugin.services.privacy.PrivateMarkerTransactionFactory;
 import org.hyperledger.besu.plugin.services.rpc.PluginRpcRequest;
 import org.hyperledger.besu.util.number.Fraction;
 import org.hyperledger.besu.util.number.Percentage;
@@ -3504,13 +3506,83 @@ public class BesuCommandTest extends CommandTestAbstract {
 
   @Test
   public void privateMarkerTransactionSigningKeyFileRequiredIfMinGasPriceNonZero() {
-    parseCommand("--privacy-enabled", "--privacy-public-key-file", ENCLAVE_PUBLIC_KEY_PATH);
+    parseCommand(
+        "--privacy-enabled",
+        "--privacy-public-key-file",
+        ENCLAVE_PUBLIC_KEY_PATH,
+        "--min-gas-price",
+        "1");
 
     assertThat(commandErrorOutput.toString())
         .startsWith(
             "Not a free gas network. --privacy-marker-transaction-signing-key-file must be specified");
   }
 
+  @Test
+  public void
+      privateMarkerTransactionSigningKeyFileNotRequiredIfMinGasPriceNonZeroAndUsingPluginPrivateMarkerTransactionFactory() {
+
+    when(privacyPluginService.getPrivateMarkerTransactionFactory())
+        .thenReturn(mock(PrivateMarkerTransactionFactory.class));
+
+    parseCommand(
+        "--privacy-enabled",
+        "--privacy-public-key-file",
+        ENCLAVE_PUBLIC_KEY_PATH,
+        "--min-gas-price",
+        "1");
+
+    assertThat(commandOutput.toString()).isEmpty();
+    assertThat(commandErrorOutput.toString()).isEmpty();
+  }
+
+  @Test
+  public void
+      privateMarkerTransactionSigningKeyFileNotCanNotBeUsedWithPluginPrivateMarkerTransactionFactory()
+          throws IOException {
+    privacyPluginService.setPrivateMarkerTransactionFactory(
+        mock(PrivateMarkerTransactionFactory.class));
+    final Path toml =
+        createTempFile(
+            "key",
+            "8f2a55949038a9610f50fb23b5883af3b4ecb3c3bb792cbcefbd1542c692be63".getBytes(UTF_8));
+
+    parseCommand(
+        "--privacy-enabled",
+        "--privacy-public-key-file",
+        ENCLAVE_PUBLIC_KEY_PATH,
+        "--privacy-marker-transaction-signing-key-file",
+        toml.toString());
+
+    assertThat(commandOutput.toString()).isEmpty();
+    assertThat(commandErrorOutput.toString()).isEmpty();
+  }
+
+  @Test
+  public void mustProvidePayloadWhenPrivacyPluginEnabled() {
+    parseCommand("--privacy-enabled", "--Xprivacy-plugin-enabled", "--min-gas-price", "0");
+
+    assertThat(commandOutput.toString()).isEmpty();
+    assertThat(commandErrorOutput.toString())
+        .startsWith(
+            "No Payload Provider has been provided. You must register one when enabling privacy plugin!");
+  }
+
+  @Test
+  public void canNotUseFlexiblePrivacyWhenPrivacyPluginEnabled() {
+    parseCommand(
+        "--privacy-enabled",
+        "--Xprivacy-plugin-enabled",
+        "--min-gas-price",
+        "0",
+        "--privacy-flexible-groups-enabled");
+
+    assertThat(commandOutput.toString()).isEmpty();
+    assertThat(commandErrorOutput.toString())
+        .startsWith(
+            "No Payload Provider has been provided. You must register one when enabling privacy plugin!");
+  }
+
   private Path createFakeGenesisFile(final JsonObject jsonGenesis) throws IOException {
     final Path genesisFile = Files.createTempFile("genesisFile", "");
     Files.write(genesisFile, encodeJsonGenesis(jsonGenesis).getBytes(UTF_8));

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

@@ -163,6 +163,7 @@ public abstract class CommandTestAbstract {
   @Mock protected MutableBlockchain mockMutableBlockchain;
   @Mock protected WorldStateArchive mockWorldStateArchive;
   @Mock protected TransactionPool mockTransactionPool;
+  @Mock protected PrivacyPluginServiceImpl privacyPluginService;
 
   @SuppressWarnings("PrivateStaticFinalLoggers") // @Mocks are inited by JUnit
   @Mock
@@ -362,7 +363,8 @@ public abstract class CommandTestAbstract {
             environment,
             storageService,
             securityModuleService,
-            mockPkiBlockCreationConfigProvider);
+            mockPkiBlockCreationConfigProvider,
+            privacyPluginService);
     besuCommands.add(besuCommand);
 
     File defaultKeyFile =
@@ -400,7 +402,8 @@ public abstract class CommandTestAbstract {
         final Map<String, String> environment,
         final StorageServiceImpl storageService,
         final SecurityModuleServiceImpl securityModuleService,
-        final PkiBlockCreationConfigurationProvider pkiBlockCreationConfigProvider) {
+        final PkiBlockCreationConfigurationProvider pkiBlockCreationConfigProvider,
+        final PrivacyPluginServiceImpl privacyPluginService) {
       super(
           mockLogger,
           mockBlockImporter,
@@ -413,7 +416,7 @@ public abstract class CommandTestAbstract {
           storageService,
           securityModuleService,
           new PermissioningServiceImpl(),
-          new PrivacyPluginServiceImpl(),
+          privacyPluginService,
           pkiBlockCreationConfigProvider,
           rpcEndpointServiceImpl);
     }