[PAN-1062] Evict old transactions (1 of 2) #1299

smatthewenglish · 2019-04-17T18:17:33Z

PR description

The acceptance criteria for this task has two components:

Configuration option to specify maximum length of time transactions are held in the pending transaction list before being dropped
When a transaction has been in the pending transaction list for the configured maximum period, it is removed

This change is intended to address the latter.

Fixed Issue(s)

Evict transactions from the pending transaction list when addedToPoolAt predates a specified criteria.

ajsutton · 2019-04-17T21:55:00Z

...h/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactionsTest.java

+
+  @Test
+  public void shouldEvictOldTransactions() {
+    Clock clock = Clock.systemUTC();


We shouldn't use a real clock in tests as it tends to make them indeterminate. TestClock makes it easy to get a clock with a fixed time for testing with a stepMillis method to let you progress time in a controlled way. Then you can update the test to add a few transactions at different time points and check that only the ones that have reached the max age are actually evicted.

ajsutton · 2019-04-17T21:56:39Z

...m/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactions.java

@@ -119,6 +119,15 @@ boolean addLocalTransaction(final Transaction transaction) {
    return addTransaction;
  }

+  void evictOldTransactions(Instant maximumRetentionPeriod) {
+    for (final TransactionInfo transactionInfo : prioritizedTransactions) {


This would all need to be inside a synchronized block.

ajsutton · 2019-04-17T21:57:15Z

...m/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactions.java

+  void evictOldTransactions(Instant maximumRetentionPeriod) {
+    for (final TransactionInfo transactionInfo : prioritizedTransactions) {
+      int difference = transactionInfo.getAddedToPoolAt().compareTo(maximumRetentionPeriod);
+      if (difference > 0) {


We can use Instant.isBefore here.

ajsutton · 2019-04-17T22:01:07Z

...m/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactions.java

@@ -119,6 +119,15 @@ boolean addLocalTransaction(final Transaction transaction) {
    return addTransaction;
  }

+  void evictOldTransactions(Instant maximumRetentionPeriod) {


maximumRetentionPeriod seems like a misleading name - this would have to be the earliestAllowedTransaction or should be a Duration which is subtracted from the current time in this method.

ajsutton · 2019-04-17T22:03:35Z

...m/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactions.java

+    for (final TransactionInfo transactionInfo : prioritizedTransactions) {
+      int difference = transactionInfo.getAddedToPoolAt().compareTo(maximumRetentionPeriod);
+      if (difference > 0) {
+        pendingTransactions.remove(transactionInfo.getTransaction().hash());


This isn't fully removing the transaction from the pool (prioritizedTransactions and transactionsBySender aren't updated and dropped transaction notifications aren't sent). It needs to call removeTransaction to ensure the removal is done correctly.

Once that's fixed, I suspect this will cause a ConcurrentModificationException when there are multiple transactions in the pool because you're removing from the set as you iterate it. You'll need to create a list of transactions to remove with something like:

final List<TransactionInfo> transactionsToRemove = prioritizedTransactions.stream().filter(transaction -> transaction.getAddedToPoolAt().isBefore(earliestAllowedAddTime)).collect(toList()); transactionsToRemove.forEach(this::removeTransaction)

ajsutton

Looks to be on the right track. I think the key thing is if TimerUtil is moved out of PendingTransactions it will be much easier to test and require fewer changes overall.

ajsutton · 2019-04-22T04:25:06Z

...c/test/java/tech/pegasys/pantheon/consensus/clique/blockcreation/CliqueBlockCreatorTest.java

@@ -108,12 +112,17 @@ public void proposerAddressCanBeExtractFromAConstructedBlock() {
    final CliqueExtraData extraData =
        new CliqueExtraData(BytesValue.wrap(new byte[32]), null, validatorList);

+    final Vertx vertx = Vertx.vertx();
+    final TimerUtil timerUtil = new VertxTimerUtil(vertx);
+    final long TRANSACTION_EVICTION_INTERVAL_MS = TimeUnit.HOURS.toMillis(1);


We shouldn't be introducing a real Vertx here. just a mock or stub TimerUtil would be enough.
Probably should make Transaction_EVICTION_INTERVAL_MS an actual constant as well instead of duplicating it for each test.

Same thing for all the other test code.

ajsutton · 2019-04-22T04:28:09Z

ethereum/blockcreation/build.gradle

@@ -15,6 +15,7 @@ jar {
 dependencies {
  implementation project(':ethereum:core')
  implementation project(':ethereum:eth')
+  implementation project(':ethereum:p2p')


We shouldn't need to add this dependency. I'm guessing you need it for TimerUtil but we need to work something out to avoid it.

One possibility is that PendingTransactions shouldn't have TimerUtil injected, but just exposes the method that it would periodically call. Then as part of creating PendingTransactions a periodic timer can be setup to call that method. PeriodicTransactions then remains completely separate from the mechanism used to periodically perform the call.

ajsutton · 2019-04-22T04:32:45Z

...m/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactions.java

+  private void evictOldTransactions() {
+    synchronized (pendingTransactions) {
+      final List<TransactionInfo> transactionsToRemove =
+          prioritizedTransactions.stream().filter(this::applyEvictionThreshold).collect(toList());


This is basically just style but I'd inline applyEvictionThreshold here and calculate the cut-off time just once. That makes the keep/remove condition really straight forward so it's a bit easier to follow.

Also, System.currentTimeMillis is a native method so incurs JNI overhead - we don't want to call that for every transaction in the pool if we can help it given we're inside a synchronized block.

I'd still recommend this change.

ajsutton · 2019-04-22T04:34:17Z

...h/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactionsTest.java

@@ -43,8 +47,18 @@
  private static final KeyPair KEYS2 = KeyPair.generate();

  private final MetricsSystem metricsSystem = new NoOpMetricsSystem();
+
+  final Vertx vertx = Vertx.vertx();
+  final TimerUtil timerUtil = new VertxTimerUtil(vertx);


We should be using a MockTimerUtil here so that we can test things in a. single thread and avoid creating a Vertx instance. Also note that every created Vertx must be closed or threads leak and cause problems for other tests.

ajsutton · 2019-04-22T04:34:56Z

...h/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactionsTest.java

+      TimeUnit.SECONDS.sleep(2);
+    } catch (Exception ignored) {
+    }
+    assertThat(transactions.size()).isEqualTo(0);


We should have a test that has a combination of old and new transactions so we know it doesn't just evict all transactions.

ajsutton

Generally good but we need to switch back to using Instant for the addedToPoolAt value as the change to a long has leaked out to affect one of our JSON-RPC responses.

ajsutton · 2019-04-22T23:04:38Z

...m/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactions.java

+  private void evictOldTransactions() {
+    synchronized (pendingTransactions) {
+      final List<TransactionInfo> transactionsToRemove =
+          prioritizedTransactions.stream().filter(this::applyEvictionThreshold).collect(toList());


I'd still recommend this change.

ajsutton · 2019-04-22T23:05:50Z

...m/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactions.java

    private final long sequence; // Allows prioritization based on order transactions are added

    TransactionInfo(
        final Transaction transaction,
        final boolean receivedFromLocalSource,
-        final Instant addedToPoolAt) {
+        final long addedToPoolAt) {


We should keep this as an Instant - no need to change it here and having better typing is always good.

ajsutton · 2019-04-22T23:06:45Z

...th/src/main/java/tech/pegasys/pantheon/ethereum/eth/transactions/TransactionPoolFactory.java


 public class TransactionPoolFactory {

+  private static final long TRANSACTION_EVICTION_INTERVAL_MS = TimeUnit.MINUTES.toMillis(1);


The default needs to be much longer than 1 minute. Probably more like 12 hours since it's not currently configurable.

ajsutton · 2019-04-22T23:08:12Z

...h/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactionsTest.java

+  @Test
+  public void shouldEvictMultipleOldTransactions() {
+    final long transactionEvictionIntervalMs = 1L;
+    final TestClock clock = new TestClock();


nit: Make clock a field so it can be reused.

I'd also stick with the default 1 minute timeout for transactions - we can step the clock forward as much as we like so don't need to have a custom value here and can then reuse the PendingTransactions instance in the field.

ajsutton · 2019-04-22T23:12:56Z

...main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/results/TransactionInfoResult.java

@@ -39,7 +37,7 @@ public String getHash() {

  @JsonGetter(value = "addedToPoolAt")
  public String getAddedToPoolAt() {
-    return addedToPoolAt.toString();
+    return Long.toString(addedToPoolAt);


This is wrong - Instant.toString outputs a ISO8601 formatted date time but this outputs a number.

ajsutton

LGTM.

ajsutton · 2019-04-24T22:25:41Z

...m/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactions.java

@@ -92,6 +99,20 @@ public PendingTransactions(
            "operation");
  }

+  public void evictOldTransactions() {
+    synchronized (pendingTransactions) {
+      long now = clock.millis();


nit: final
Even more nitty: :) Personally I would have made this final Instant removeTransactionsBefore = clock.instant().minusMillis(transactionEvictionIntervalMs) and then the filter would be transaction -> transaction.getAddedToPoolAt().isBefore(removeTransactionsBefore). Both ways work fine and it's not a big deal but I find the clarity of calculating the cut-off timepoint helpful.

smatthewenglish changed the title ~~[PAN-1062] Evict old transactions~~ [MINOR / PAN-1062 (A)] Evict old transactions Apr 17, 2019

ajsutton reviewed Apr 17, 2019

View reviewed changes

smatthewenglish changed the title ~~[MINOR / PAN-1062 (A)] Evict old transactions~~ [PAN-1062 (A)] Evict old transactions Apr 18, 2019

smatthewenglish changed the title ~~[PAN-1062 (A)] Evict old transactions~~ [PAN-1062] Evict old transactions (1 of 2) Apr 18, 2019

smatthewenglish force-pushed the robinshultz branch 5 times, most recently from 9cf26bd to fcc06e6 Compare April 20, 2019 01:48

ajsutton reviewed Apr 22, 2019

View reviewed changes

smatthewenglish force-pushed the robinshultz branch 5 times, most recently from eb70468 to 42736e9 Compare April 22, 2019 22:29

ajsutton reviewed Apr 22, 2019

View reviewed changes

smatthewenglish force-pushed the robinshultz branch from 778303b to 94e28b3 Compare April 24, 2019 16:01

ajsutton approved these changes Apr 24, 2019

View reviewed changes

smatthewenglish added 12 commits April 25, 2019 09:55

update

37ba1e5

x

f656399

x

de30ea9

update

e8ac9c5

evictOldTransactions++

3ea0095

test

a327e9a

shouldEvictMultipleOldTransactions

eacea66

x

a1e4d65

shouldEvictExclusivelyOldTransactions

5ad9f79

prblm

97af358

got 'em

ed10a24

gotcha

5348364

smatthewenglish added 26 commits April 25, 2019 09:55

update

cc5a01d

update

0cb16d0

x

545c1ed

update

9addf77

blah

d4f6baf

x

3f119f2

x

6e1193c

x

6282e91

x

3793f9c

x

e3587f6

x

d03b249

update++

86d43c6

x++

d324674

:

03196ff

tst

1377dfa

1 or 8

d2852ec

x

89662a6

clock

0cef577

default

68435bf

Instant

c1d9d2d

1000L

214ab67

Instant

c3616c7

Instant++

502f297

o

dd7f5f8

inline

87fa858

~

16d354a

smatthewenglish force-pushed the robinshultz branch from a8d9806 to 16d354a Compare April 25, 2019 13:59

smatthewenglish merged commit f20687f into PegaSysEng:master Apr 25, 2019

notlesh pushed a commit to notlesh/pantheon that referenced this pull request May 4, 2019

[PAN-1062] Evict old transactions (1 of 2) (PegaSysEng#1299)

c59ea96

notlesh pushed a commit to notlesh/pantheon that referenced this pull request May 14, 2019

[PAN-1062] Evict old transactions (1 of 2) (PegaSysEng#1299)

c0bf672

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PAN-1062] Evict old transactions (1 of 2) #1299

[PAN-1062] Evict old transactions (1 of 2) #1299

smatthewenglish commented Apr 17, 2019 •

edited

Loading

ajsutton Apr 17, 2019

ajsutton Apr 17, 2019

ajsutton Apr 17, 2019

ajsutton Apr 17, 2019

ajsutton Apr 17, 2019

ajsutton left a comment

ajsutton Apr 22, 2019

ajsutton Apr 22, 2019

ajsutton Apr 22, 2019

ajsutton Apr 22, 2019

ajsutton Apr 22, 2019

ajsutton Apr 22, 2019

ajsutton left a comment

ajsutton Apr 22, 2019

ajsutton Apr 22, 2019

ajsutton Apr 22, 2019

ajsutton Apr 22, 2019

ajsutton Apr 22, 2019

ajsutton left a comment

ajsutton Apr 24, 2019


		public class TransactionPoolFactory {

		private static final long TRANSACTION_EVICTION_INTERVAL_MS = TimeUnit.MINUTES.toMillis(1);

[PAN-1062] Evict old transactions (1 of 2) #1299

[PAN-1062] Evict old transactions (1 of 2) #1299

Conversation

smatthewenglish commented Apr 17, 2019 • edited Loading

PR description

Fixed Issue(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajsutton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajsutton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajsutton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smatthewenglish commented Apr 17, 2019 •

edited

Loading