[1476] Fixed transaction validation bug when local permissioning is enabled

CHANGELOG.md

@@ -14,7 +14,8 @@
 
 ### Bug Fixes
 
-* Fix a bug on `eth_estimateGas` which returned `Internal error` instead of `Execution reverted` in case of reverted transaction [\#1478](https://github.com/hyperledger/besu/pull/1478)
+* Fix a bug on `eth_estimateGas` which returned `Internal error` instead of `Execution reverted` in case of reverted transaction. [\#1478](https://github.com/hyperledger/besu/pull/1478)
+* Fixed a bug where Local Account Permissioning was being incorrectly enforced on block import/validation. [\#1510](https://github.com/hyperledger/besu/pull/1510)
 
 ## Deprecated 
 

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

@@ -15,6 +15,7 @@
 package org.hyperledger.besu.tests.acceptance.dsl;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.assertj.core.api.Assertions.assertThat;
 
 import org.hyperledger.besu.tests.acceptance.dsl.account.Accounts;
 import org.hyperledger.besu.tests.acceptance.dsl.blockchain.Blockchain;
@@ -30,6 +31,7 @@
 import org.hyperledger.besu.tests.acceptance.dsl.condition.txpool.TxPoolConditions;
 import org.hyperledger.besu.tests.acceptance.dsl.condition.web3.Web3Conditions;
 import org.hyperledger.besu.tests.acceptance.dsl.contract.ContractVerifier;
+import org.hyperledger.besu.tests.acceptance.dsl.node.Node;
 import org.hyperledger.besu.tests.acceptance.dsl.node.cluster.Cluster;
 import org.hyperledger.besu.tests.acceptance.dsl.node.configuration.BesuNodeFactory;
 import org.hyperledger.besu.tests.acceptance.dsl.node.configuration.permissioning.PermissionedNodeBuilder;
@@ -51,6 +53,7 @@
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.lang.ProcessBuilder.Redirect;
+import java.math.BigInteger;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 
@@ -216,4 +219,12 @@ protected void succeeded(final Description description) {
           }
         }
       };
+
+  protected void waitForBlockHeight(final Node node, final long blockchainHeight) {
+    WaitUtils.waitFor(
+        120,
+        () ->
+            assertThat(node.execute(ethTransactions.blockNumber()))
+                .isGreaterThanOrEqualTo(BigInteger.valueOf(blockchainHeight)));
+  }
 }

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

@@ -26,11 +26,14 @@
 import org.hyperledger.besu.ethereum.core.MiningParametersTestBuilder;
 import org.hyperledger.besu.ethereum.core.PrivacyParameters;
 import org.hyperledger.besu.ethereum.core.Wei;
+import org.hyperledger.besu.ethereum.permissioning.LocalPermissioningConfiguration;
+import org.hyperledger.besu.ethereum.permissioning.PermissioningConfiguration;
 import org.hyperledger.besu.tests.acceptance.dsl.node.BesuNode;
 import org.hyperledger.besu.tests.acceptance.dsl.node.Node;
 import org.hyperledger.besu.tests.acceptance.dsl.node.RunnableNode;
 import org.hyperledger.besu.tests.acceptance.dsl.node.configuration.genesis.GenesisConfigurationFactory;
 
+import java.io.File;
 import java.io.IOException;
 import java.net.URI;
 import java.net.URISyntaxException;
@@ -286,6 +289,32 @@ public BesuNode createIbft2NonValidatorBootnode(final String name, final String
             .build());
   }
 
+  public BesuNode createIbft2NodeWithLocalAccountPermissioning(
+      final String name,
+      final String genesisFile,
+      final List<String> accountAllowList,
+      final File configFile)
+      throws IOException {
+    final LocalPermissioningConfiguration config = LocalPermissioningConfiguration.createDefault();
+    config.setAccountAllowlist(accountAllowList);
+    config.setAccountPermissioningConfigFilePath(configFile.getAbsolutePath());
+    final PermissioningConfiguration permissioningConfiguration =
+        new PermissioningConfiguration(Optional.of(config), Optional.empty());
+    return create(
+        new BesuNodeConfigurationBuilder()
+            .name(name)
+            .miningEnabled()
+            .jsonRpcConfiguration(node.createJsonRpcWithIbft2AdminEnabledConfig())
+            .webSocketConfiguration(node.createWebSocketEnabledConfig())
+            .permissioningConfiguration(permissioningConfiguration)
+            .devMode(false)
+            .genesisConfigProvider(
+                validators ->
+                    genesis.createIbft2GenesisConfigFilterBootnode(validators, genesisFile))
+            .bootnodeEligible(false)
+            .build());
+  }
+
   public BesuNode createIbft2Node(final String name, final String genesisFile) throws IOException {
     return create(
         new BesuNodeConfigurationBuilder()

acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/permissioning/AccountLocalConfigPermissioningImportAcceptanceTest.java

@@ -0,0 +1,98 @@
+/*
+ * Copyright ConsenSys AG.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+package org.hyperledger.besu.tests.acceptance.permissioning;
+
+import org.hyperledger.besu.ethereum.permissioning.AllowlistPersistor;
+import org.hyperledger.besu.tests.acceptance.dsl.AcceptanceTestBase;
+import org.hyperledger.besu.tests.acceptance.dsl.account.Account;
+import org.hyperledger.besu.tests.acceptance.dsl.node.BesuNode;
+import org.hyperledger.besu.tests.acceptance.dsl.node.cluster.Cluster;
+import org.hyperledger.besu.tests.acceptance.dsl.node.cluster.ClusterConfigurationBuilder;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.List;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+public class AccountLocalConfigPermissioningImportAcceptanceTest extends AcceptanceTestBase {
+
+  @Rule public TemporaryFolder folder = new TemporaryFolder();
+
+  private static final String GENESIS_FILE = "/ibft/ibft.json";
+
+  private Account sender;
+  private Account beneficiary;
+  private BesuNode bootnode;
+  private BesuNode nodeA;
+  private BesuNode nodeB;
+  private Cluster permissionedCluster;
+
+  @Before
+  public void setUp() throws IOException {
+    sender = accounts.getPrimaryBenefactor();
+    beneficiary = accounts.createAccount("beneficiary");
+    final List<String> allowList = List.of(sender.getAddress(), beneficiary.getAddress());
+    final File sharedFile = folder.newFile();
+    persistAllowList(allowList, sharedFile.toPath());
+    bootnode = besu.createIbft2NonValidatorBootnode("bootnode", GENESIS_FILE);
+    nodeA =
+        besu.createIbft2NodeWithLocalAccountPermissioning(
+            "nodeA", GENESIS_FILE, allowList, sharedFile);
+    nodeB =
+        besu.createIbft2NodeWithLocalAccountPermissioning(
+            "nodeB", GENESIS_FILE, allowList, sharedFile);
+    permissionedCluster =
+        new Cluster(new ClusterConfigurationBuilder().awaitPeerDiscovery(false).build(), net);
+
+    permissionedCluster.start(bootnode, nodeA, nodeB);
+  }
+
+  @Test
+  public void transactionFromDeniedAccountShouldNotBreakBlockImport() throws IOException {
+    final File newPermissionsFile = folder.newFile();
+    final List<String> allowList = List.of(beneficiary.getAddress());
+    persistAllowList(allowList, newPermissionsFile.toPath());
+    final BesuNode nodeC =
+        besu.createIbft2NodeWithLocalAccountPermissioning(
+            "nodeC", GENESIS_FILE, allowList, newPermissionsFile);
+
+    waitForBlockHeight(bootnode, 5);
+
+    nodeA.verify(beneficiary.balanceEquals(0));
+    nodeA.execute(accountTransactions.createTransfer(sender, beneficiary, 1));
+    nodeA.verify(beneficiary.balanceEquals(1));
+
+    permissionedCluster.startNode(nodeC);
+
+    waitForBlockHeight(bootnode, 10);
+    waitForBlockHeight(nodeC, 10);
+  }
+
+  private void persistAllowList(final List<String> allowList, final Path path) throws IOException {
+    AllowlistPersistor.addNewConfigItem(
+        AllowlistPersistor.ALLOWLIST_TYPE.ACCOUNTS, allowList, path);
+  }
+
+  @After
+  public void tearDown() {
+    permissionedCluster.stop();
+  }
+}

acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/permissioning/NodeSmartContractPermissioningAcceptanceTestBase.java

@@ -14,11 +14,8 @@
  */
 package org.hyperledger.besu.tests.acceptance.permissioning;
 
-import static org.assertj.core.api.Assertions.assertThat;
-
 import org.hyperledger.besu.ethereum.core.Hash;
 import org.hyperledger.besu.tests.acceptance.dsl.AcceptanceTestBase;
-import org.hyperledger.besu.tests.acceptance.dsl.WaitUtils;
 import org.hyperledger.besu.tests.acceptance.dsl.condition.Condition;
 import org.hyperledger.besu.tests.acceptance.dsl.condition.perm.NodeSmartContractPermissioningConditions;
 import org.hyperledger.besu.tests.acceptance.dsl.node.Node;
@@ -30,7 +27,6 @@
 import org.hyperledger.besu.tests.acceptance.dsl.transaction.perm.NodeSmartContractPermissioningTransactions;
 
 import java.io.IOException;
-import java.math.BigInteger;
 
 class NodeSmartContractPermissioningAcceptanceTestBase extends AcceptanceTestBase {
 
@@ -132,12 +128,4 @@ protected Condition connectionIsForbidden(final Node source, final Node target)
     return nodeSmartContractPermissioningConditions.connectionIsForbidden(
         CONTRACT_ADDRESS, source, target);
   }
-
-  protected void waitForBlockHeight(final Node node, final long blockchainHeight) {
-    WaitUtils.waitFor(
-        120,
-        () ->
-            assertThat(node.execute(ethTransactions.blockNumber()))
-                .isGreaterThanOrEqualTo(BigInteger.valueOf(blockchainHeight)));
-  }
 }

acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/permissioning/NodeSmartContractPermissioningV2AcceptanceTestBase.java

@@ -14,11 +14,8 @@
  */
 package org.hyperledger.besu.tests.acceptance.permissioning;
 
-import static org.assertj.core.api.Assertions.assertThat;
-
 import org.hyperledger.besu.ethereum.core.Hash;
 import org.hyperledger.besu.tests.acceptance.dsl.AcceptanceTestBase;
-import org.hyperledger.besu.tests.acceptance.dsl.WaitUtils;
 import org.hyperledger.besu.tests.acceptance.dsl.condition.Condition;
 import org.hyperledger.besu.tests.acceptance.dsl.condition.perm.NodeSmartContractPermissioningV2Conditions;
 import org.hyperledger.besu.tests.acceptance.dsl.node.Node;
@@ -30,7 +27,6 @@
 import org.hyperledger.besu.tests.acceptance.dsl.transaction.perm.NodeSmartContractPermissioningV2Transactions;
 
 import java.io.IOException;
-import java.math.BigInteger;
 
 class NodeSmartContractPermissioningV2AcceptanceTestBase extends AcceptanceTestBase {
 
@@ -125,12 +121,4 @@ protected Condition connectionIsForbidden(final Node node) {
   protected Condition connectionIsAllowed(final Node node) {
     return nodeSmartContractPermissioningConditionsV2.connectionIsAllowed(CONTRACT_ADDRESS, node);
   }
-
-  protected void waitForBlockHeight(final Node node, final long blockchainHeight) {
-    WaitUtils.waitFor(
-        120,
-        () ->
-            assertThat(node.execute(ethTransactions.blockNumber()))
-                .isGreaterThanOrEqualTo(BigInteger.valueOf(blockchainHeight)));
-  }
 }

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/TransactionFilter.java

@@ -16,5 +16,6 @@
 
 @FunctionalInterface
 public interface TransactionFilter {
-  boolean permitted(Transaction transaction, boolean checkOnchainPermissions);
+  boolean permitted(
+      Transaction transaction, boolean checkLocalPermissions, boolean checkOnchainPermissions);
 }

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetTransactionValidator.java

@@ -206,7 +206,12 @@ private boolean isSenderAllowed(
       final Transaction transaction, final TransactionValidationParams validationParams) {
     if (validationParams.checkLocalPermissions() || validationParams.checkOnchainPermissions()) {
       return transactionFilter
-          .map(c -> c.permitted(transaction, validationParams.checkOnchainPermissions()))
+          .map(
+              c ->
+                  c.permitted(
+                      transaction,
+                      validationParams.checkLocalPermissions(),
+                      validationParams.checkOnchainPermissions()))
           .orElse(true);
     } else {
       return true;

ethereum/core/src/test/java/org/hyperledger/besu/ethereum/mainnet/MainnetTransactionValidatorTest.java

@@ -202,9 +202,15 @@ public void shouldAcceptValidTransactionIfAccountIsPermitted() {
 
   @Test
   public void shouldPropagateCorrectStateChangeParamToTransactionFilter() {
-    final ArgumentCaptor<Boolean> stateChangeParamCaptor = ArgumentCaptor.forClass(Boolean.class);
+    final ArgumentCaptor<Boolean> stateChangeLocalParamCaptor =
+        ArgumentCaptor.forClass(Boolean.class);
+    final ArgumentCaptor<Boolean> stateChangeOnchainParamCaptor =
+        ArgumentCaptor.forClass(Boolean.class);
     final TransactionFilter transactionFilter = mock(TransactionFilter.class);
-    when(transactionFilter.permitted(any(Transaction.class), stateChangeParamCaptor.capture()))
+    when(transactionFilter.permitted(
+            any(Transaction.class),
+            stateChangeLocalParamCaptor.capture(),
+            stateChangeOnchainParamCaptor.capture()))
         .thenReturn(true);
 
     final MainnetTransactionValidator validator =
@@ -216,7 +222,8 @@ public void shouldPropagateCorrectStateChangeParamToTransactionFilter() {
 
     validator.validateForSender(basicTransaction, accountWithNonce(0), validationParams);
 
-    assertThat(stateChangeParamCaptor.getValue()).isTrue();
+    assertThat(stateChangeLocalParamCaptor.getValue()).isTrue();
+    assertThat(stateChangeOnchainParamCaptor.getValue()).isTrue();
   }
 
   @Test
@@ -334,7 +341,8 @@ private Account account(final Wei balance, final long nonce) {
 
   private TransactionFilter transactionFilter(final boolean permitted) {
     final TransactionFilter transactionFilter = mock(TransactionFilter.class);
-    when(transactionFilter.permitted(any(Transaction.class), anyBoolean())).thenReturn(permitted);
+    when(transactionFilter.permitted(any(Transaction.class), anyBoolean(), anyBoolean()))
+        .thenReturn(permitted);
     return transactionFilter;
   }
 }

ethereum/permissioning/src/main/java/org/hyperledger/besu/ethereum/permissioning/account/AccountPermissioningController.java

@@ -45,28 +45,34 @@ public AccountPermissioningController(
         transactionSmartContractPermissioningController;
   }
 
-  public boolean isPermitted(final Transaction transaction, final boolean includeOnChainCheck) {
+  public boolean isPermitted(
+      final Transaction transaction,
+      final boolean includeLocalCheck,
+      final boolean includeOnChainCheck) {
     final Hash transactionHash = transaction.getHash();
     final Address sender = transaction.getSender();
 
     LOG.trace("Account permissioning: Checking transaction {}", transactionHash);
 
-    boolean permitted;
-    if (includeOnChainCheck) {
-      permitted =
-          accountLocalConfigPermissioningController
-                  .map(c -> c.isPermitted(transaction))
-                  .orElse(true)
-              && transactionSmartContractPermissioningController
-                  .map(c -> c.isPermitted(transaction))
-                  .orElse(true);
-    } else {
-      permitted =
+    boolean permittedLocal = true;
+    boolean permittedOnchain = true;
+
+    if (includeLocalCheck) {
+      permittedLocal =
           accountLocalConfigPermissioningController
               .map(c -> c.isPermitted(transaction))
               .orElse(true);
     }
 
+    if (includeOnChainCheck) {
+      permittedOnchain =
+          transactionSmartContractPermissioningController
+              .map(c -> c.isPermitted(transaction))
+              .orElse(true);
+    }
+
+    final boolean permitted = permittedLocal && permittedOnchain;
+
     if (permitted) {
       LOG.trace("Account permissioning: Permitted transaction {} from {}", transactionHash, sender);
     } else {

ethereum/permissioning/src/test/java/org/hyperledger/besu/ethereum/permissioning/account/AccountPermissioningControllerTest.java

@@ -52,7 +52,7 @@ public void before() {
   public void shouldOnlyCheckLocalConfigControllerWhenNotPersistingState() {
     when(localConfigController.isPermitted(any())).thenReturn(true);
 
-    boolean isPermitted = permissioningController.isPermitted(mock(Transaction.class), false);
+    boolean isPermitted = permissioningController.isPermitted(mock(Transaction.class), true, false);
 
     assertThat(isPermitted).isTrue();
 
@@ -65,7 +65,7 @@ public void shouldOnlyCheckLocalConfigAndSmartContractControllerWhenPersistingSt
     when(localConfigController.isPermitted(any())).thenReturn(true);
     when(smartContractController.isPermitted(any())).thenReturn(true);
 
-    boolean isPermitted = permissioningController.isPermitted(mock(Transaction.class), true);
+    boolean isPermitted = permissioningController.isPermitted(mock(Transaction.class), true, true);
 
     assertThat(isPermitted).isTrue();
 
@@ -78,7 +78,7 @@ public void shouldReturnFalseIfOneControllerPermitsAndTheOtherDoesNot() {
     when(localConfigController.isPermitted(any())).thenReturn(true);
     when(smartContractController.isPermitted(any())).thenReturn(false);
 
-    boolean isPermitted = permissioningController.isPermitted(mock(Transaction.class), true);
+    boolean isPermitted = permissioningController.isPermitted(mock(Transaction.class), true, true);
 
     assertThat(isPermitted).isFalse();