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

If nonce is invalid, do not delete during mining #1422

Merged
merged 4 commits into from
May 9, 2019
Merged

If nonce is invalid, do not delete during mining #1422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
210a164
If nonce is invalid, do not delete
May 9, 2019
d1e6562
Fix errorprone and spotless
May 9, 2019
e34e66f
more fixes
May 9, 2019
3d09454
Merge branch 'master' into nonce_deletion
rain-on May 9, 2019
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
12 changes: 10 additions & 2 deletions .../src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/BlockTransactionSelector.java
View file
Open in desktop
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
Open in desktop
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