Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

PAN-2794: Including flag for onchain permissioning check on tx processor #1571

Merged
merged 2 commits into from
Jun 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,8 @@ private TransactionSelectionResult evaluateTransaction(final Transaction transac
transaction,
miningBeneficiary,
blockHashLookup,
false);
false,
true);

if (!result.isInvalid()) {
worldStateUpdater.commit();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -122,7 +123,7 @@ public void failedTransactionsAreIncludedInTheBlock() {
pendingTransactions.addRemoteTransaction(transaction);

when(transactionProcessor.processTransaction(
any(), any(), any(), eq(transaction), any(), any(), any()))
any(), any(), any(), eq(transaction), any(), any(), anyBoolean(), anyBoolean()))
.thenReturn(MainnetTransactionProcessor.Result.failed(5, ValidationResult.valid()));

// The block should fit 3 transactions only
Expand Down Expand Up @@ -160,15 +161,23 @@ public void invalidTransactionsTransactionProcessingAreSkippedButBlockStillFills
pendingTransactions.addRemoteTransaction(tx);
}

when(transactionProcessor.processTransaction(any(), any(), any(), any(), any(), any(), any()))
when(transactionProcessor.processTransaction(
any(), any(), any(), any(), any(), any(), anyBoolean(), anyBoolean()))
.thenReturn(
MainnetTransactionProcessor.Result.successful(
new LogSeries(Lists.newArrayList()),
0,
BytesValue.EMPTY,
ValidationResult.valid()));
when(transactionProcessor.processTransaction(
any(), any(), any(), eq(transactionsToInject.get(1)), any(), any(), any()))
any(),
any(),
any(),
eq(transactionsToInject.get(1)),
any(),
any(),
anyBoolean(),
anyBoolean()))
.thenReturn(
MainnetTransactionProcessor.Result.invalid(ValidationResult.invalid(NONCE_TOO_LOW)));

Expand Down Expand Up @@ -208,7 +217,8 @@ public void subsetOfPendingTransactionsIncludedWhenBlockGasLimitHit() {
pendingTransactions.addRemoteTransaction(tx);
}

when(transactionProcessor.processTransaction(any(), any(), any(), any(), any(), any(), any()))
when(transactionProcessor.processTransaction(
any(), any(), any(), any(), any(), any(), anyBoolean(), anyBoolean()))
.thenReturn(
MainnetTransactionProcessor.Result.successful(
new LogSeries(Lists.newArrayList()),
Expand Down Expand Up @@ -278,7 +288,8 @@ public void transactionOfferingGasPriceLessThanMinimumIsIdentifiedAndRemovedFrom
public void transactionTooLargeForBlockDoesNotPreventMoreBeingAddedIfBlockOccupancyNotReached() {
final ProcessableBlockHeader blockHeader = createBlockWithGasLimit(300);

when(transactionProcessor.processTransaction(any(), any(), any(), any(), any(), any(), any()))
when(transactionProcessor.processTransaction(
any(), any(), any(), any(), any(), any(), anyBoolean(), anyBoolean()))
.thenReturn(
MainnetTransactionProcessor.Result.successful(
new LogSeries(Lists.newArrayList()),
Expand Down Expand Up @@ -334,7 +345,8 @@ public void transactionSelectionStopsWhenSufficientBlockOccupancyIsReached() {
final ProcessableBlockHeader blockHeader = createBlockWithGasLimit(300);

// TransactionProcessor mock assumes all gas in the transaction was used (i.e. gasLimit).
when(transactionProcessor.processTransaction(any(), any(), any(), any(), any(), any(), any()))
when(transactionProcessor.processTransaction(
any(), any(), any(), any(), any(), any(), anyBoolean(), anyBoolean()))
.thenReturn(
MainnetTransactionProcessor.Result.successful(
new LogSeries(Lists.newArrayList()),
Expand Down Expand Up @@ -429,7 +441,8 @@ public void shouldDiscardTransactionsThatFailValidation() {
eq(validTransaction),
any(),
any(),
any()))
anyBoolean(),
anyBoolean()))
.thenReturn(
Result.successful(
LogSeries.empty(), 10000, BytesValue.EMPTY, ValidationResult.valid()));
Expand All @@ -440,7 +453,8 @@ public void shouldDiscardTransactionsThatFailValidation() {
eq(invalidTransaction),
any(),
any(),
any()))
anyBoolean(),
anyBoolean()))
.thenReturn(
Result.invalid(
ValidationResult.invalid(TransactionInvalidReason.EXCEEDS_BLOCK_GAS_LIMIT)));
Expand Down Expand Up @@ -468,7 +482,8 @@ public void transactionWithIncorrectNonceRemainsInPoolAndNotSelected() {
eq(futureTransaction),
any(),
any(),
any()))
anyBoolean(),
anyBoolean()))
.thenReturn(
Result.invalid(ValidationResult.invalid(TransactionInvalidReason.INCORRECT_NONCE)));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ public void shouldTraceSStoreOperation() {
createTransaction,
genesisBlock.getHeader().getCoinbase(),
blockHashLookup,
false,
false);
assertThat(result.isSuccessful()).isTrue();
final Account createdContract =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,5 @@

@FunctionalInterface
public interface TransactionFilter {
boolean permitted(Transaction transaction, boolean isStateChange);
boolean permitted(Transaction transaction, boolean checkOnchainPermissions);
}
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ public Result processBlock(
transaction,
miningBeneficiary,
blockHashLookup,
true,
true);
if (result.isInvalid()) {
return Result.failed();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,8 @@ public Result processTransaction(
final Address miningBeneficiary,
final OperationTracer operationTracer,
final BlockHashLookup blockHashLookup,
final Boolean isPersistingState) {
final Boolean isPersistingState,
final Boolean checkOnchainPermissions) {
LOG.trace("Starting execution of {}", transaction);

ValidationResult<TransactionInvalidReason> validationResult =
Expand All @@ -162,7 +163,7 @@ public Result processTransaction(
final TransactionValidationParams validationParams =
new TransactionValidationParams.Builder()
.allowFutureNonce(false)
.stateChange(isPersistingState)
.checkOnchainPermissions(checkOnchainPermissions)
.build();

final Address senderAddress = transaction.getSender();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ public ValidationResult<TransactionInvalidReason> validateForSender(
transaction.getNonce(), senderNonce));
}

if (!isSenderAllowed(transaction, validationParams.isStateChange())) {
if (!isSenderAllowed(transaction, validationParams.checkOnchainPermissions())) {
return ValidationResult.invalid(
TX_SENDER_NOT_AUTHORIZED,
String.format("Sender %s is not on the Account Whitelist", transaction.getSender()));
Expand Down Expand Up @@ -162,8 +162,11 @@ public ValidationResult<TransactionInvalidReason> validateTransactionSignature(
return ValidationResult.valid();
}

private boolean isSenderAllowed(final Transaction transaction, final boolean isStateChange) {
return transactionFilter.map(c -> c.permitted(transaction, isStateChange)).orElse(true);
private boolean isSenderAllowed(
final Transaction transaction, final boolean checkOnchainPermissions) {
return transactionFilter
.map(c -> c.permitted(transaction, checkOnchainPermissions))
.orElse(true);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ default boolean isSuccessful() {
* @param miningBeneficiary The address which is to receive the transaction fee
* @param blockHashLookup The {@link BlockHashLookup} to use for BLOCKHASH operations
* @param isPersistingState Whether the state will be modified by this process
* @param checkOnchainPermissions Whether a transaction permissioning check should check onchain
* permissioning rules
* @return the transaction result
*/
default Result processTransaction(
Expand All @@ -116,7 +118,8 @@ default Result processTransaction(
final Transaction transaction,
final Address miningBeneficiary,
final BlockHashLookup blockHashLookup,
final Boolean isPersistingState) {
final Boolean isPersistingState,
final Boolean checkOnchainPermissions) {
return processTransaction(
blockchain,
worldState,
Expand All @@ -125,7 +128,8 @@ default Result processTransaction(
miningBeneficiary,
NO_TRACING,
blockHashLookup,
isPersistingState);
isPersistingState,
checkOnchainPermissions);
}

/**
Expand All @@ -141,6 +145,27 @@ default Result processTransaction(
* @param isPersistingState Whether the state will be modified by this process
* @return the transaction result
*/
default Result processTransaction(
final Blockchain blockchain,
final WorldUpdater worldState,
final ProcessableBlockHeader blockHeader,
final Transaction transaction,
final Address miningBeneficiary,
final OperationTracer operationTracer,
final BlockHashLookup blockHashLookup,
final Boolean isPersistingState) {
return processTransaction(
blockchain,
worldState,
blockHeader,
transaction,
miningBeneficiary,
operationTracer,
blockHashLookup,
isPersistingState,
false);
}

Result processTransaction(
Blockchain blockchain,
WorldUpdater worldState,
Expand All @@ -149,5 +174,6 @@ Result processTransaction(
Address miningBeneficiary,
OperationTracer operationTracer,
BlockHashLookup blockHashLookup,
Boolean isPersistingState);
Boolean isPersistingState,
Boolean checkOnchainPermissions);
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,38 +15,39 @@
public class TransactionValidationParams {

private final boolean allowFutureNonce;
private final boolean stateChange;
private final boolean checkOnchainPermissions;

private TransactionValidationParams(final boolean allowFutureNonce, final boolean stateChange) {
private TransactionValidationParams(
final boolean allowFutureNonce, final boolean checkOnchainPermissions) {
this.allowFutureNonce = allowFutureNonce;
this.stateChange = stateChange;
this.checkOnchainPermissions = checkOnchainPermissions;
}

public boolean isAllowFutureNonce() {
return allowFutureNonce;
}

public boolean isStateChange() {
return stateChange;
public boolean checkOnchainPermissions() {
return checkOnchainPermissions;
}

public static class Builder {

private boolean allowFutureNonce = false;
private boolean stateChange = false;
private boolean checkOnchainPermissions = false;

public Builder allowFutureNonce(final boolean allowFutureNonce) {
this.allowFutureNonce = allowFutureNonce;
return this;
}

public Builder stateChange(final boolean stateChange) {
this.stateChange = stateChange;
public Builder checkOnchainPermissions(final boolean checkOnchainPermissions) {
this.checkOnchainPermissions = checkOnchainPermissions;
return this;
}

public TransactionValidationParams build() {
return new TransactionValidationParams(allowFutureNonce, stateChange);
return new TransactionValidationParams(allowFutureNonce, checkOnchainPermissions);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ private Optional<TransactionSimulatorResult> process(
transaction,
protocolSpec.getMiningBeneficiaryCalculator().calculateBeneficiary(header),
new BlockHashLookup(header, blockchain),
false,
false);

return Optional.of(new TransactionSimulatorResult(transaction, result));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public void before() {
transactionValidationParamCaptor();

final TransactionValidationParams expectedValidationParams =
new TransactionValidationParams.Builder().stateChange(false).build();
new TransactionValidationParams.Builder().checkOnchainPermissions(false).build();

transactionProcessor.processTransaction(
blockchain,
Expand All @@ -80,6 +80,7 @@ public void before() {
transaction,
Address.fromHexString("1"),
blockHashLookup,
false,
false);

assertThat(txValidationParamCaptor.getValue())
Expand All @@ -93,7 +94,7 @@ public void before() {
transactionValidationParamCaptor();

final TransactionValidationParams expectedValidationParams =
new TransactionValidationParams.Builder().stateChange(true).build();
new TransactionValidationParams.Builder().checkOnchainPermissions(true).build();

transactionProcessor.processTransaction(
blockchain,
Expand All @@ -102,6 +103,7 @@ public void before() {
transaction,
Address.fromHexString("1"),
blockHashLookup,
true,
true);

assertThat(txValidationParamCaptor.getValue())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ public void shouldPropagateCorrectStateChangeParamToTransactionFilter() {
validator.setTransactionFilter(transactionFilter);

final TransactionValidationParams validationParams =
new TransactionValidationParams.Builder().stateChange(true).build();
new TransactionValidationParams.Builder().checkOnchainPermissions(true).build();

validator.validateForSender(basicTransaction, accountWithNonce(0), validationParams);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
Expand Down Expand Up @@ -344,13 +345,14 @@ private void mockProcessorStatusForTransaction(
}

when(transactionProcessor.processTransaction(
any(), any(), any(), eq(transaction), any(), any(), any()))
any(), any(), any(), eq(transaction), any(), any(), anyBoolean(), anyBoolean()))
.thenReturn(result);
}

private void verifyTransactionWasProcessed(final Transaction expectedTransaction) {
verify(transactionProcessor)
.processTransaction(any(), any(), any(), eq(expectedTransaction), any(), any(), any());
.processTransaction(
any(), any(), any(), eq(expectedTransaction), any(), any(), anyBoolean(), anyBoolean());
}

private CallParameter callParameter() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ public static void executeTest(final GeneralStateTestCaseEipSpec spec) {
transaction,
blockHeader.getCoinbase(),
new BlockHashLookup(blockHeader, blockchain),
false,
false);
final Account coinbase = worldStateUpdater.getOrCreate(spec.blockHeader().getCoinbase());
if (coinbase != null && coinbase.isEmpty() && shouldClearEmptyAccounts(spec.eip())) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,10 @@ private ValidationResult<TransactionInvalidReason> validateTransaction(
}

final TransactionValidationParams validationParams =
new TransactionValidationParams.Builder().allowFutureNonce(true).stateChange(false).build();
new TransactionValidationParams.Builder()
.allowFutureNonce(true)
.checkOnchainPermissions(false)
.build();

return protocolContext
.getWorldStateArchive()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,10 @@ public void shouldCallValidatorWithExpectedValidationParameters() {
.thenReturn(valid());

final TransactionValidationParams expectedValidationParams =
new TransactionValidationParams.Builder().stateChange(false).allowFutureNonce(true).build();
new TransactionValidationParams.Builder()
.checkOnchainPermissions(false)
.allowFutureNonce(true)
.build();

transactionPool.addLocalTransaction(transaction1);

Expand Down
Loading