[PIE-7] Ignore transactions from the network while behind chain head #1228

smatthewenglish · 2019-04-06T21:33:22Z

PR description

Builds on PR #1251, i.e. "migrate TransactionPool (& affiliated test) from 'core' to 'eth'".
Adding SyncState in TransactionPoolFactory.createTransactionPool(), that way we can leave extraneous elements of the system alone.
Utilizing isInSync() of SyncState, i.e.:

  public boolean isInSync() {
    return syncTarget
        .map(
            t -> t.estimatedTargetHeight() - blockchain.getChainHeadBlockNumber() <= SYNC_TOLERANCE)
        .orElse(true);
  }

Fixed Issue(s)

shemnon · 2019-04-08T18:31:37Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionPool.java

    this.pendingTransactions = pendingTransactions;
    this.protocolSchedule = protocolSchedule;
    this.protocolContext = protocolContext;
    this.transactionBatchAddedListener = transactionBatchAddedListener;
+    this.synchronizer = synchronizer;


Can we just pass around a shared SyncStatus object instead of the whole synchronizer?

This is not a constructive review comment. Obviously we can do that -- do you think we ought to do that? If that's what you're suggesting, why do you feel so? Why would that be better than the current paradigm?

That was the implied subtext. Let me try the direct approach.

I don't think the entire synchronizer object should be passed around when all we need is the sync state. Change it and pass the SyncState object instead unless there is a technical reason we cannot.

So to do this right we need to change how we are handling the SyncStatus object. Currently it is an Optional and not returned when we are in sync.

I propose we make it always return SyncStatus, not an Optional. We also add a field that maps the three current states the sync can be in: NOT_STARTED, SYNCING, and UP_TO_DATE. NOT_STARTED corresponds to a highest block of zero, and up to date corresponds to current block equal to highest block. We can also add a method to tell us how many blocks behind we are and use that in the logic where we would drop inbound transactions.

This object is shared between the synchronizer and the transaction pool. Synchronizer writes to it and tx pool reads.

You want to share SyncState the same way the AbstractMiningCoordinator does. SyncStatus is not what you want - it's only for the JSON-RPC side of things (we should, separate to this work, push it to be just in the JSON-RPC realm and not in Synchronizer at all).

Yes, SyncState. We will still need methods to tell us how far out of sync we are. We may also want a means to tell if we are out of sync because we've never started a sync. In that case I don't think we should drop transactions because we may be only a little behind, or a whole lot.

shemnon · 2019-04-09T04:25:54Z

.../java/tech/pegasys/pantheon/tests/acceptance/pubsub/NewPendingTransactionAcceptanceTest.java

@@ -169,6 +169,7 @@ public void subscriptionToMinerNodeMustReceivePublishEvent() {
  }

  @Test
+  @Ignore


I think these tests are showing a genuine failure. The bug says "too far behind" not "behind in any respect." These messages are getting dropped because the full node in a process runner is about 3-4 blocks behind. We don't see this in the thread runner because there is no process and JVM startup overhead and the full node doesn't start behind the miner.

shemnon · 2019-04-09T04:29:25Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionPool.java

@@ -78,6 +81,12 @@ public TransactionPool(
  }

  public void addRemoteTransactions(final Collection<Transaction> transactions) {
+
+    if (synchronizer.getSyncStatus().isPresent()) {


This is too aggressive. If we are behind by as much as 1 block we are going to drop transactions. This can happen from the p2p network's normal operation. We need a threshold, stored in the SynchronizerConfiguration, such as 100. If we are syncing and more than that threshold behind then we can drop results. But if we are not syncing or are within the threshold we should hold onto it.

shemnon · 2019-04-09T04:35:31Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionPool.java

    this.pendingTransactions = pendingTransactions;
    this.protocolSchedule = protocolSchedule;
    this.protocolContext = protocolContext;
    this.transactionBatchAddedListener = transactionBatchAddedListener;
+    this.synchronizer = synchronizer;


So to do this right we need to change how we are handling the SyncStatus object. Currently it is an Optional and not returned when we are in sync.

I propose we make it always return SyncStatus, not an Optional. We also add a field that maps the three current states the sync can be in: NOT_STARTED, SYNCING, and UP_TO_DATE. NOT_STARTED corresponds to a highest block of zero, and up to date corresponds to current block equal to highest block. We can also add a method to tell us how many blocks behind we are and use that in the logic where we would drop inbound transactions.

This object is shared between the synchronizer and the transaction pool. Synchronizer writes to it and tx pool reads.

shemnon · 2019-04-10T15:25:55Z

ethereum/eth/build.gradle

@@ -55,4 +55,7 @@ dependencies {
  testImplementation 'org.mockito:mockito-core'

  jmhImplementation project(path: ':ethereum:core', configuration: 'testSupportArtifacts')
+
+  testImplementation project(path: ':config', configuration: 'testSupportArtifacts')


Alphabetize this with the rest of the testImplementations. At about line 46

shemnon · 2019-04-10T15:39:45Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/transactions/TransactionPool.java

@@ -80,6 +91,9 @@ public TransactionPool(
  public void addRemoteTransactions(final Collection<Transaction> transactions) {
    final Set<Transaction> addedTransactions = new HashSet<>();
    for (final Transaction transaction : sortByNonce(transactions)) {
+      if (!syncState.isInSync()) {


Looks like isInSync has most of the sync tolerance I was looking for already built in. The acceptance criteria listed 100 blocks and the default is 5. We cannot just change the default for isInSync as it is is also used by mining and 5 is their threshold.

Can you update the isInSync to have a version that takes an argument and for this call use 100 (and make the no-args version call the args version with that constant)? Either as a local constant or one of the new unstable options via SynchnizerConfiguration.

Sounds good -- I made it a local constant 'cause I don't know how SynchnizerConfiguration works

shemnon · 2019-04-11T14:26:14Z

I'll give it another pass once #1251 is merged into master, it looks like both PRs are in this PR.

shemnon · 2019-04-15T14:34:55Z

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

@@ -35,13 +37,16 @@ public static TransactionPool createTransactionPool(
    final PeerTransactionTracker transactionTracker = new PeerTransactionTracker();
    final TransactionsMessageSender transactionsMessageSender =
        new TransactionsMessageSender(transactionTracker);
+    final MutableBlockchain blockchain = protocolContext.getBlockchain();
+    final SyncState syncState = new SyncState(blockchain, ethContext.getEthPeers());


We should be using the shared SyncState that the controllers use. So add this as a param to createTransactionPool and have the callers pass their instance. They all should have one by the point they make this call.

yeah -- that makes sense. branch has been updated accordingly

…egaSysEng#1228)

smatthewenglish force-pushed the p7 branch from a1d0d5e to 77d1493 Compare April 8, 2019 18:03

shemnon reviewed Apr 8, 2019

View reviewed changes

smatthewenglish force-pushed the p7 branch from d7e5d5b to 4c0bd4b Compare April 8, 2019 20:10

shemnon suggested changes Apr 9, 2019

View reviewed changes

smatthewenglish force-pushed the p7 branch 10 times, most recently from 886a07f to 08228ff Compare April 10, 2019 07:49

smatthewenglish requested a review from shemnon April 10, 2019 07:58

shemnon reviewed Apr 10, 2019

View reviewed changes

smatthewenglish force-pushed the p7 branch 6 times, most recently from 16d2a60 to 3ea92bb Compare April 11, 2019 11:00

smatthewenglish force-pushed the p7 branch from 3ea92bb to 8475fbf Compare April 15, 2019 14:17

shemnon reviewed Apr 15, 2019

View reviewed changes

smatthewenglish added 5 commits April 15, 2019 13:41

rebase

125192e

x

706a954

x

fc0683f

^‿^

553f840

update

afbb9c6

smatthewenglish force-pushed the p7 branch from 8475fbf to afbb9c6 Compare April 15, 2019 18:01

shemnon approved these changes Apr 15, 2019

View reviewed changes

smatthewenglish merged commit 42ecfb5 into PegaSysEng:master Apr 15, 2019

notlesh pushed a commit to notlesh/pantheon that referenced this pull request Apr 24, 2019

[PIE-7] Ignore transactions from the network while behind chain head (P…

b80c523

…egaSysEng#1228)

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

[PIE-7] Ignore transactions from the network while behind chain head (P…

77f432f

…egaSysEng#1228)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PIE-7] Ignore transactions from the network while behind chain head #1228

[PIE-7] Ignore transactions from the network while behind chain head #1228

smatthewenglish commented Apr 6, 2019 •

edited

Loading

shemnon Apr 8, 2019

smatthewenglish Apr 8, 2019

shemnon Apr 8, 2019

shemnon Apr 9, 2019 •

edited

Loading

ajsutton Apr 9, 2019

shemnon Apr 9, 2019

shemnon Apr 9, 2019 •

edited

Loading

shemnon Apr 9, 2019

shemnon Apr 9, 2019 •

edited

Loading

shemnon Apr 10, 2019

shemnon Apr 10, 2019

smatthewenglish Apr 11, 2019

shemnon commented Apr 11, 2019

shemnon Apr 15, 2019

smatthewenglish Apr 15, 2019

                 }
                 @Test
+                @Ignore

[PIE-7] Ignore transactions from the network while behind chain head #1228

[PIE-7] Ignore transactions from the network while behind chain head #1228

Conversation

smatthewenglish commented Apr 6, 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

shemnon Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shemnon Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shemnon Apr 9, 2019 • edited Loading

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

shemnon commented Apr 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smatthewenglish commented Apr 6, 2019 •

edited

Loading

shemnon Apr 9, 2019 •

edited

Loading

shemnon Apr 9, 2019 •

edited

Loading

shemnon Apr 9, 2019 •

edited

Loading