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

Commit

Permalink
BlockCreator does not delete transactions with invalid nonce
Browse files Browse the repository at this point in the history
  • Loading branch information
@rain-on
rain-on authored May 9, 2019
1 parent 23afc3a commit f7b9aee
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 153 deletions.
12 changes: 10 additions & 2 deletions .../src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/BlockTransactionSelector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import tech.pegasys.pantheon.ethereum.eth.transactions.PendingTransactions.TransactionSelectionResult;
import tech.pegasys.pantheon.ethereum.mainnet.MainnetBlockProcessor.TransactionReceiptFactory;
import tech.pegasys.pantheon.ethereum.mainnet.TransactionProcessor;
import tech.pegasys.pantheon.ethereum.mainnet.TransactionValidator.TransactionInvalidReason;
import tech.pegasys.pantheon.ethereum.vm.BlockHashLookup;

import java.util.List;
Expand Down Expand Up @@ -59,6 +60,7 @@ public class BlockTransactionSelector {
private static final double MIN_BLOCK_OCCUPANCY_RATIO = 0.8;

public static class TransactionSelectionResults {

private final List<Transaction> transactions = Lists.newArrayList();
private final List<TransactionReceipt> receipts = Lists.newArrayList();
private long cumulativeGasUsed = 0;
Expand Down Expand Up @@ -172,8 +174,14 @@ private TransactionSelectionResult evaluateTransaction(final Transaction transac
worldStateUpdater.commit();
updateTransactionResultTracking(transaction, result);
} else {
// Remove invalid transactions from the transaction pool but continue looking for valid ones
// as the block may not yet be full.
// If the transaction has an incorrect nonce, leave it in the pool and continue
if (result
.getValidationResult()
.getInvalidReason()
.equals(TransactionInvalidReason.INCORRECT_NONCE)) {
return TransactionSelectionResult.CONTINUE;
}
// If the transaction was invalid for any other reason, delete it, and continue.
return TransactionSelectionResult.DELETE_TRANSACTION_AND_CONTINUE;
}
return TransactionSelectionResult.CONTINUE;
Expand Down
223 changes: 72 additions & 151 deletions .../test/java/tech/pegasys/pantheon/ethereum/blockcreation/BlockTransactionSelectorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,36 +65,40 @@ public class BlockTransactionSelectorTest {
private static final KeyPair keyPair = KeyPair.generate();
private final MetricsSystem metricsSystem = new NoOpMetricsSystem();

private final PendingTransactions pendingTransactions =
new PendingTransactions(
PendingTransactions.DEFAULT_TX_RETENTION_HOURS, 5, TestClock.fixed(), metricsSystem);
private final Blockchain blockchain = new TestBlockchain();
private final DefaultMutableWorldState worldState = inMemoryWorldState();
private final Supplier<Boolean> isCancelled = () -> false;
private final TransactionProcessor transactionProcessor = mock(TransactionProcessor.class);

private ProcessableBlockHeader createBlockWithGasLimit(final long gasLimit) {
return BlockHeaderBuilder.create()
.parentHash(Hash.EMPTY)
.coinbase(Address.fromHexString(String.format("%020x", 1)))
.difficulty(UInt256.ONE)
.number(1)
.gasLimit(gasLimit)
.timestamp(Instant.now().toEpochMilli())
.buildProcessableBlockHeader();
}

@Test
public void emptyPendingTransactionsResultsInEmptyVettingResult() {
final ProtocolSchedule<Void> protocolSchedule =
FixedDifficultyProtocolSchedule.create(GenesisConfigFile.development().getConfigOptions());
final Blockchain blockchain = new TestBlockchain();
final TransactionProcessor transactionProcessor =
final TransactionProcessor mainnetTransactionProcessor =
protocolSchedule.getByBlockNumber(0).getTransactionProcessor();
final DefaultMutableWorldState worldState = inMemoryWorldState();

final PendingTransactions pendingTransactions =
new PendingTransactions(
PendingTransactions.DEFAULT_TX_RETENTION_HOURS, 5, TestClock.fixed(), metricsSystem);

final Supplier<Boolean> isCancelled = () -> false;

final ProcessableBlockHeader blockHeader =
BlockHeaderBuilder.create()
.parentHash(Hash.EMPTY)
.coinbase(Address.fromHexString(String.format("%020x", 1)))
.difficulty(UInt256.ONE)
.number(1)
.gasLimit(5000)
.timestamp(Instant.now().toEpochMilli())
.buildProcessableBlockHeader();
// The block should fit 5 transactions only
final ProcessableBlockHeader blockHeader = createBlockWithGasLimit(5000);

final Address miningBeneficiary = AddressHelpers.ofValue(1);

final BlockTransactionSelector selector =
new BlockTransactionSelector(
transactionProcessor,
mainnetTransactionProcessor,
blockchain,
worldState,
pendingTransactions,
Expand All @@ -114,33 +118,15 @@ public void emptyPendingTransactionsResultsInEmptyVettingResult() {

@Test
public void failedTransactionsAreIncludedInTheBlock() {
final PendingTransactions pendingTransactions =
new PendingTransactions(
PendingTransactions.DEFAULT_TX_RETENTION_HOURS, 5, TestClock.fixed(), metricsSystem);

final Transaction transaction = createTransaction(1);
pendingTransactions.addRemoteTransaction(transaction);

final TransactionProcessor transactionProcessor = mock(TransactionProcessor.class);

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

final Blockchain blockchain = new TestBlockchain();
final DefaultMutableWorldState worldState = inMemoryWorldState();
final Supplier<Boolean> isCancelled = () -> false;

// The block should fit 3 transactions only
final ProcessableBlockHeader blockHeader =
BlockHeaderBuilder.create()
.parentHash(Hash.EMPTY)
.coinbase(Address.fromHexString(String.format("%020x", 1)))
.difficulty(UInt256.ONE)
.number(1)
.gasLimit(5000)
.timestamp(Instant.now().toEpochMilli())
.buildProcessableBlockHeader();
final ProcessableBlockHeader blockHeader = createBlockWithGasLimit(5000);

final Address miningBeneficiary = AddressHelpers.ofValue(1);

Expand All @@ -167,18 +153,13 @@ public void failedTransactionsAreIncludedInTheBlock() {

@Test
public void invalidTransactionsTransactionProcessingAreSkippedButBlockStillFills() {
final PendingTransactions pendingTransactions =
new PendingTransactions(
PendingTransactions.DEFAULT_TX_RETENTION_HOURS, 5, TestClock.fixed(), metricsSystem);

final List<Transaction> transactionsToInject = Lists.newArrayList();
for (int i = 0; i < 5; i++) {
final Transaction tx = createTransaction(i);
transactionsToInject.add(tx);
pendingTransactions.addRemoteTransaction(tx);
}

final TransactionProcessor transactionProcessor = mock(TransactionProcessor.class);
when(transactionProcessor.processTransaction(any(), any(), any(), any(), any(), any(), any()))
.thenReturn(
MainnetTransactionProcessor.Result.successful(
Expand All @@ -191,20 +172,8 @@ public void invalidTransactionsTransactionProcessingAreSkippedButBlockStillFills
.thenReturn(
MainnetTransactionProcessor.Result.invalid(ValidationResult.invalid(NONCE_TOO_LOW)));

final Blockchain blockchain = new TestBlockchain();
final DefaultMutableWorldState worldState = inMemoryWorldState();
final Supplier<Boolean> isCancelled = () -> false;

// The block should fit 3 transactions only
final ProcessableBlockHeader blockHeader =
BlockHeaderBuilder.create()
.parentHash(Hash.EMPTY)
.coinbase(Address.fromHexString(String.format("%020x", 1)))
.difficulty(UInt256.ONE)
.number(1)
.gasLimit(5000)
.timestamp(Instant.now().toEpochMilli())
.buildProcessableBlockHeader();
final ProcessableBlockHeader blockHeader = createBlockWithGasLimit(5000);

final Address miningBeneficiary = AddressHelpers.ofValue(1);

Expand All @@ -231,18 +200,14 @@ public void invalidTransactionsTransactionProcessingAreSkippedButBlockStillFills

@Test
public void subsetOfPendingTransactionsIncludedWhenBlockGasLimitHit() {
final PendingTransactions pendingTransactions =
new PendingTransactions(
PendingTransactions.DEFAULT_TX_RETENTION_HOURS, 5, TestClock.fixed(), metricsSystem);

final List<Transaction> transactionsToInject = Lists.newArrayList();
// Transactions are reported in reverse order.
for (int i = 0; i < 5; i++) {
final Transaction tx = createTransaction(i);
transactionsToInject.add(tx);
pendingTransactions.addRemoteTransaction(tx);
}
final TransactionProcessor transactionProcessor = mock(TransactionProcessor.class);

when(transactionProcessor.processTransaction(any(), any(), any(), any(), any(), any(), any()))
.thenReturn(
MainnetTransactionProcessor.Result.successful(
Expand All @@ -251,21 +216,7 @@ public void subsetOfPendingTransactionsIncludedWhenBlockGasLimitHit() {
BytesValue.EMPTY,
ValidationResult.valid()));

final Blockchain blockchain = new TestBlockchain();

final DefaultMutableWorldState worldState = inMemoryWorldState();

final Supplier<Boolean> isCancelled = () -> false;

final ProcessableBlockHeader blockHeader =
BlockHeaderBuilder.create()
.parentHash(Hash.EMPTY)
.coinbase(Address.fromHexString(String.format("%020x", 1)))
.difficulty(UInt256.ONE)
.number(1)
.gasLimit(301)
.timestamp(Instant.now().toEpochMilli())
.buildProcessableBlockHeader();
final ProcessableBlockHeader blockHeader = createBlockWithGasLimit(301);

final Address miningBeneficiary = AddressHelpers.ofValue(1);

Expand Down Expand Up @@ -298,28 +249,9 @@ public void subsetOfPendingTransactionsIncludedWhenBlockGasLimitHit() {

@Test
public void transactionOfferingGasPriceLessThanMinimumIsIdentifiedAndRemovedFromPending() {
final PendingTransactions pendingTransactions =
new PendingTransactions(
PendingTransactions.DEFAULT_TX_RETENTION_HOURS, 5, TestClock.fixed(), metricsSystem);

final Blockchain blockchain = new TestBlockchain();

final DefaultMutableWorldState worldState = inMemoryWorldState();

final Supplier<Boolean> isCancelled = () -> false;

final ProcessableBlockHeader blockHeader =
BlockHeaderBuilder.create()
.parentHash(Hash.EMPTY)
.coinbase(Address.fromHexString(String.format("%020x", 1)))
.difficulty(UInt256.ONE)
.number(1)
.gasLimit(301)
.timestamp(Instant.now().toEpochMilli())
.buildProcessableBlockHeader();
final ProcessableBlockHeader blockHeader = createBlockWithGasLimit(301);

final Address miningBeneficiary = AddressHelpers.ofValue(1);
final TransactionProcessor transactionProcessor = mock(TransactionProcessor.class);
final BlockTransactionSelector selector =
new BlockTransactionSelector(
transactionProcessor,
Expand All @@ -344,26 +276,8 @@ public void transactionOfferingGasPriceLessThanMinimumIsIdentifiedAndRemovedFrom

@Test
public void transactionTooLargeForBlockDoesNotPreventMoreBeingAddedIfBlockOccupancyNotReached() {
final PendingTransactions pendingTransactions =
new PendingTransactions(
PendingTransactions.DEFAULT_TX_RETENTION_HOURS, 5, TestClock.fixed(), metricsSystem);

final Blockchain blockchain = new TestBlockchain();
final DefaultMutableWorldState worldState = inMemoryWorldState();
final Supplier<Boolean> isCancelled = () -> false;

final ProcessableBlockHeader blockHeader =
BlockHeaderBuilder.create()
.parentHash(Hash.EMPTY)
.coinbase(Address.fromHexString(String.format("%020x", 1)))
.difficulty(UInt256.ONE)
.number(1)
.gasLimit(300)
.timestamp(Instant.now().toEpochMilli())
.buildProcessableBlockHeader();
final ProcessableBlockHeader blockHeader = createBlockWithGasLimit(300);

// TransactionProcessor mock assumes all gas in the transaction was used (i.e. gasLimit).
final TransactionProcessor transactionProcessor = mock(TransactionProcessor.class);
when(transactionProcessor.processTransaction(any(), any(), any(), any(), any(), any(), any()))
.thenReturn(
MainnetTransactionProcessor.Result.successful(
Expand Down Expand Up @@ -417,26 +331,9 @@ public void transactionTooLargeForBlockDoesNotPreventMoreBeingAddedIfBlockOccupa

@Test
public void transactionSelectionStopsWhenSufficientBlockOccupancyIsReached() {
final PendingTransactions pendingTransactions =
new PendingTransactions(
PendingTransactions.DEFAULT_TX_RETENTION_HOURS, 5, TestClock.fixed(), metricsSystem);

final Blockchain blockchain = new TestBlockchain();
final DefaultMutableWorldState worldState = inMemoryWorldState();
final Supplier<Boolean> isCancelled = () -> false;

final ProcessableBlockHeader blockHeader =
BlockHeaderBuilder.create()
.parentHash(Hash.EMPTY)
.coinbase(Address.fromHexString(String.format("%020x", 1)))
.difficulty(UInt256.ONE)
.number(1)
.gasLimit(300)
.timestamp(Instant.now().toEpochMilli())
.buildProcessableBlockHeader();
final ProcessableBlockHeader blockHeader = createBlockWithGasLimit(300);

// TransactionProcessor mock assumes all gas in the transaction was used (i.e. gasLimit).
final TransactionProcessor transactionProcessor = mock(TransactionProcessor.class);
when(transactionProcessor.processTransaction(any(), any(), any(), any(), any(), any(), any()))
.thenReturn(
MainnetTransactionProcessor.Result.successful(
Expand Down Expand Up @@ -501,24 +398,7 @@ public void transactionSelectionStopsWhenSufficientBlockOccupancyIsReached() {

@Test
public void shouldDiscardTransactionsThatFailValidation() {
final PendingTransactions pendingTransactions =
new PendingTransactions(
PendingTransactions.DEFAULT_TX_RETENTION_HOURS, 5, TestClock.fixed(), metricsSystem);

final TransactionProcessor transactionProcessor = mock(TransactionProcessor.class);
final Blockchain blockchain = new TestBlockchain();
final DefaultMutableWorldState worldState = inMemoryWorldState();
final Supplier<Boolean> isCancelled = () -> false;

final ProcessableBlockHeader blockHeader =
BlockHeaderBuilder.create()
.parentHash(Hash.EMPTY)
.coinbase(Address.fromHexString(String.format("%020x", 1)))
.difficulty(UInt256.ONE)
.number(1)
.gasLimit(300)
.timestamp(Instant.now().toEpochMilli())
.buildProcessableBlockHeader();
final ProcessableBlockHeader blockHeader = createBlockWithGasLimit(300);

final Address miningBeneficiary = AddressHelpers.ofValue(1);
final BlockTransactionSelector selector =
Expand Down Expand Up @@ -571,6 +451,47 @@ public void shouldDiscardTransactionsThatFailValidation() {
assertThat(pendingTransactions.getTransactionByHash(invalidTransaction.hash())).isNotPresent();
}

@Test
public void transactionWithIncorrectNonceRemainsInPoolAndNotSelected() {
final ProcessableBlockHeader blockHeader = createBlockWithGasLimit(5000);

final TransactionTestFixture txTestFixture = new TransactionTestFixture();
final Transaction futureTransaction =
txTestFixture.nonce(5).gasLimit(1).createTransaction(keyPair);

pendingTransactions.addRemoteTransaction(futureTransaction);

when(transactionProcessor.processTransaction(
eq(blockchain),
any(WorldUpdater.class),
eq(blockHeader),
eq(futureTransaction),
any(),
any(),
any()))
.thenReturn(
Result.invalid(ValidationResult.invalid(TransactionInvalidReason.INCORRECT_NONCE)));

final Address miningBeneficiary = AddressHelpers.ofValue(1);
final BlockTransactionSelector selector =
new BlockTransactionSelector(
transactionProcessor,
blockchain,
worldState,
pendingTransactions,
blockHeader,
this::createReceipt,
Wei.ZERO,
isCancelled,
miningBeneficiary);

final BlockTransactionSelector.TransactionSelectionResults results =
selector.buildTransactionListForBlock();

assertThat(pendingTransactions.getTransactionByHash(futureTransaction.hash())).isPresent();
assertThat(results.getTransactions().size()).isEqualTo(0);
}

private Transaction createTransaction(final int transactionNumber) {
return Transaction.builder()
.gasLimit(100)
Expand Down

0 comments on commit f7b9aee

Please sign in to comment.