[1476] Fixed transaction validation bug when local permissioning is enabled (#1510)

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

Signed-off-by: Mark Terry <mark.terry@consensys.net>
pull/1535/head
mark-terry 4 years ago committed by GitHub
parent 40d5bfe746
commit 32e7f2d4c3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 3
      CHANGELOG.md
  2. 11
      acceptance-tests/dsl/src/main/java/org/hyperledger/besu/tests/acceptance/dsl/AcceptanceTestBase.java
  3. 29
      acceptance-tests/dsl/src/main/java/org/hyperledger/besu/tests/acceptance/dsl/node/configuration/BesuNodeFactory.java
  4. 98
      acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/permissioning/AccountLocalConfigPermissioningImportAcceptanceTest.java
  5. 12
      acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/permissioning/NodeSmartContractPermissioningAcceptanceTestBase.java
  6. 12
      acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/permissioning/NodeSmartContractPermissioningV2AcceptanceTestBase.java
  7. 3
      ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/TransactionFilter.java
  8. 7
      ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetTransactionValidator.java
  9. 16
      ethereum/core/src/test/java/org/hyperledger/besu/ethereum/mainnet/MainnetTransactionValidatorTest.java
  10. 30
      ethereum/permissioning/src/main/java/org/hyperledger/besu/ethereum/permissioning/account/AccountPermissioningController.java
  11. 6
      ethereum/permissioning/src/test/java/org/hyperledger/besu/ethereum/permissioning/account/AccountPermissioningControllerTest.java

@ -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

@ -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.process.ExitedWithCod
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.File;
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 @@ public class AcceptanceTestBase {
}
}
};
protected void waitForBlockHeight(final Node node, final long blockchainHeight) {
WaitUtils.waitFor(
120,
() ->
assertThat(node.execute(ethTransactions.blockNumber()))
.isGreaterThanOrEqualTo(BigInteger.valueOf(blockchainHeight)));
}
}

@ -26,11 +26,14 @@ import org.hyperledger.besu.ethereum.core.MiningParameters;
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 class BesuNodeFactory {
.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()

@ -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();
}
}

@ -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.Transaction;
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 @@ class NodeSmartContractPermissioningAcceptanceTestBase extends AcceptanceTestBas
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)));
}
}

@ -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.Transaction;
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 @@ class NodeSmartContractPermissioningV2AcceptanceTestBase extends AcceptanceTestB
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)));
}
}

@ -16,5 +16,6 @@ package org.hyperledger.besu.ethereum.core;
@FunctionalInterface
public interface TransactionFilter {
boolean permitted(Transaction transaction, boolean checkOnchainPermissions);
boolean permitted(
Transaction transaction, boolean checkLocalPermissions, boolean checkOnchainPermissions);
}

@ -206,7 +206,12 @@ public class MainnetTransactionValidator implements TransactionValidator {
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;

@ -202,9 +202,15 @@ public class MainnetTransactionValidatorTest {
@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 class MainnetTransactionValidatorTest {
validator.validateForSender(basicTransaction, accountWithNonce(0), validationParams);
assertThat(stateChangeParamCaptor.getValue()).isTrue();
assertThat(stateChangeLocalParamCaptor.getValue()).isTrue();
assertThat(stateChangeOnchainParamCaptor.getValue()).isTrue();
}
@Test
@ -334,7 +341,8 @@ public class MainnetTransactionValidatorTest {
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;
}
}

@ -45,28 +45,34 @@ public class 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 {

@ -52,7 +52,7 @@ public class AccountPermissioningControllerTest {
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 class AccountPermissioningControllerTest {
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 class AccountPermissioningControllerTest {
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();

Loading…
Cancel
Save