[NC-2058] Improve block propagation time #808

smatthewenglish · 2019-02-07T17:54:38Z

PR description

Addresses feedback raised in previous round of review and contains a test that verifies the invocation of the newly added broadcastBlock() method

NOTE:

what do you make of the passage of BlockBroadcaster in the DefaultSynchronizer?

Next Step:

add some tests that verify that the propagated block is in fact being received by peers

ajsutton

Only real issue is the hard coded capability but otherwise looks good. Would be useful to get the PR description updated to indicate what parts of the story this PR doesn't yet cover.

ajsutton · 2019-02-12T21:10:03Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/manager/EthPeer.java

@@ -130,6 +133,15 @@ public ResponseStream send(final MessageData messageData) throws PeerNotConnecte
    }
  }

+  public void propagateBlock(final Block block, final UInt256 totalDifficulty) {
+    final NewBlockMessage newBlockMessage = NewBlockMessage.create(block, totalDifficulty);
+    final Capability capability = Capability.create("eth", 63);


We don't want to create a new capability here and need to use the passed in protocolName. I suspect connection.sendForProtocol(protocolName, newBlockMessage) is what you want.

ajsutton · 2019-02-12T21:13:27Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockPropagationManager.java

@@ -144,12 +144,21 @@ private void onBlockAdded(final BlockAddedEvent blockAddedEvent, final Blockchai
    }
  }

-  private void handleNewBlockFromNetwork(final EthMessage message) {
+  void broadcastBlock(final Block block, final UInt256 difficulty) {


Is there a reason this is package private instead of private?

I'd also suggest as you move forward to extract this out into a separate BlockBroadcaster class. It just provides a better separation of concerns and will let us write a more focussed unit test for the approach to broadcasting blocks. Happy enough for it to live here for now though.

CjHare · 2019-02-13T06:06:09Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockPropagationManager.java

+    for (EthPeer ethPeer : availablePeers) {
+      ethPeer.propagateBlock(block, difficulty);
+    }
+  }


If you're feeling adventurous you could merge the collection into a list and forLoop into a single line:
ethContext.getEthPeers().availablePeers().forEach(ethPeer -> ethPeer.propagateBlock(block, difficulty));

shemnon · 2019-02-13T18:13:46Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockBroadcaster.java

+        .forEach(ethPeer -> ethPeer.propagateBlock(block, difficulty));
+  }
+
+  void effectuateBroadcast() {


optional Too many syllables. Perhaps maybeBroadcast or possiblyBroadcast. . Effectuate communicates to me that the block will unconditionally be broadcast, while what we need to communicate is that if it passes some tests it will be broadcast.

shemnon · 2019-02-13T18:16:14Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockPropagationManager.java

@@ -77,12 +78,14 @@
      final EthContext ethContext,
      final SyncState syncState,
      final PendingBlocks pendingBlocks,
-      final LabelledMetric<OperationTimer> ethTasksTimer) {
+      final LabelledMetric<OperationTimer> ethTasksTimer,
+      final BlockBroadcaster<C> blockBroadcaster) {


This field is always overwritten, no need for it in the constructor.

as I wrote in the description, it's easier to test if it's in the constructor. do you suggest making an explicit getter and setter?

I'd say passing it in in the constructor is a good thing - allows you to mock it, but you're also creating a new one on line 115 so it's not using the one passed into the constructor.

The fact it is overwritten and never used prior to that is the real concern. If it's fixed later then it can stay.

yeah- good point about it getting overwritten- mock passed to constructor never getting used 😬- so then we should have a getter and setter then?

No, passing it into the constructor is good, but BlockBroadcaster shouldn't take Bock into its constructor, only pass in the block to broadcast to the method that actual broadcasts it. That way you can reuse the same BlockBroadcaster instance to broadcast multiple blocks and never need to create a new one in BlockPropagationManager.

shemnon · 2019-02-13T18:18:29Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockBroadcaster.java

+import com.google.common.annotations.VisibleForTesting;
+
+class BlockBroadcaster<C> {
+


No real state is kept. Could this be rewitten as a static function? Or perhaps make block a parameter in effectuateBroadcast, but re-use gets tricky when the schedule changes.

I'm not sure how to interpret this comment. it seems that your suggesting to remove these instance variables, and then not

I like the idea of this being a separate class with an instance method to broadcast just for the testing benefits. However you wouldn't pass the block into the constructor and I'd be tempted to limit this just to broadcasting at the moment, not the header validation so you probably only need EthContext.

Not 100% sure about where the validation should live but I think keeping it in BlockPropagationManager will keep the validation concerns together since it will also choose the validation to apply when importing the block (which could be reduced based on what validation has already been done). Making broadcastBlock @VisibleForTesting is also a sign that having something purely responsible for the broadcast, separate from validation, is useful.

And yes I appreciate the original ticket description is based on the validation being done in the separate task which would push it into this class - the fact that we're just applying full validation instead of a new validation class and having seen the number of extra dependencies is pushing me to change that view.

shemnon · 2019-02-13T18:19:06Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockPropagationManager.java

@@ -63,6 +63,7 @@
  private final EthContext ethContext;
  private final SyncState syncState;
  private final LabelledMetric<OperationTimer> ethTasksTimer;
+  private BlockBroadcaster<C> blockBroadcaster;


This field is always overwritten prior to use. Consider making it a variable instead.

shemnon · 2019-02-13T18:20:23Z

...m/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockPropagationManagerTest.java

@@ -75,6 +75,7 @@ public static void setupSuite() {
    fullBlockchain = BlockchainSetupUtil.forTesting().importAllBlocks();
  }

+  @SuppressWarnings("unchecked")


I really don't like disabling the unchecked warning. Consider using <?> when you access no templated methods.

@SuppressWarnings("unchecked") is already used in this class in shouldNotImportBlocksThatAreAlreadyBeingImported

Yeah you have to @SuppressWarnings when creating a mock of a generic class. You can use @Mock and the runner instead but that's a whole other conversation.

ajsutton · 2019-02-13T19:51:46Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockPropagationManager.java

@@ -77,12 +78,14 @@
      final EthContext ethContext,
      final SyncState syncState,
      final PendingBlocks pendingBlocks,
-      final LabelledMetric<OperationTimer> ethTasksTimer) {
+      final LabelledMetric<OperationTimer> ethTasksTimer,
+      final BlockBroadcaster<C> blockBroadcaster) {


I'd say passing it in in the constructor is a good thing - allows you to mock it, but you're also creating a new one on line 115 so it's not using the one passed into the constructor.

ajsutton · 2019-02-13T20:11:47Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockBroadcaster.java

+import com.google.common.annotations.VisibleForTesting;
+
+class BlockBroadcaster<C> {
+


I like the idea of this being a separate class with an instance method to broadcast just for the testing benefits. However you wouldn't pass the block into the constructor and I'd be tempted to limit this just to broadcasting at the moment, not the header validation so you probably only need EthContext.

Not 100% sure about where the validation should live but I think keeping it in BlockPropagationManager will keep the validation concerns together since it will also choose the validation to apply when importing the block (which could be reduced based on what validation has already been done). Making broadcastBlock @VisibleForTesting is also a sign that having something purely responsible for the broadcast, separate from validation, is useful.

ajsutton · 2019-02-13T20:14:40Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockBroadcaster.java

+        protocolSchedule.getByBlockNumber(block.getHeader().getNumber());
+    final BlockHeaderValidator<C> blockHeaderValidator = protocolSpec.getBlockHeaderValidator();
+    final Optional<BlockHeader> maybeParent =
+        protocolContext.getBlockchain().getBlockHeader(block.getHeader().getParentHash());


We should know the parent is available by now so could just use getBlockHeader(...).orElseThrow(() -> new IllegalArgumentException(...)). Better than silently not broadcasting the block.

ajsutton · 2019-02-13T20:56:38Z

...m/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockPropagationManagerTest.java

@@ -75,6 +75,7 @@ public static void setupSuite() {
    fullBlockchain = BlockchainSetupUtil.forTesting().importAllBlocks();
  }

+  @SuppressWarnings("unchecked")


Yeah you have to @SuppressWarnings when creating a mock of a generic class. You can use @Mock and the runner instead but that's a whole other conversation.

ajsutton · 2019-02-13T20:59:13Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockBroadcaster.java

+import com.google.common.annotations.VisibleForTesting;
+
+class BlockBroadcaster<C> {
+


And yes I appreciate the original ticket description is based on the validation being done in the separate task which would push it into this class - the fact that we're just applying full validation instead of a new validation class and having seen the number of extra dependencies is pushing me to change that view.

ajsutton · 2019-02-13T21:02:00Z

...m/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockPropagationManagerTest.java

  @Test
  public void purgesOldBlocks() {
    final int oldBlocksToImport = 3;
    syncConfig =
        SynchronizerConfiguration.builder().blockPropagationRange(-oldBlocksToImport, 5).build();
+    BlockBroadcaster<Void> blockBroadcaster = mock(BlockBroadcaster.class);


We should be able to make the blockBroadcaster from setUp a field and just re-use it here. Which then avoids the need for the @SuppressWarnings("unchecked") on this method.

ajsutton · 2019-02-14T19:31:24Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockPropagationManager.java

@@ -63,6 +66,7 @@
  private final EthContext ethContext;
  private final SyncState syncState;
  private final LabelledMetric<OperationTimer> ethTasksTimer;
+  private BlockBroadcaster blockBroadcaster;


nit: this should be final

ajsutton · 2019-02-14T19:31:54Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockPropagationManager.java

@@ -105,9 +110,34 @@ private void setupListeners() {
        .subscribe(EthPV62.NEW_BLOCK_HASHES, this::handleNewBlockHashesFromNetwork);
  }

+  @VisibleForTesting
+  void broadcastBlock(final Block block) {
+    blockBroadcaster = new BlockBroadcaster(ethContext);


Don't create a new BlockBroadcaster here, just use the one passed into the constructor.

ajsutton · 2019-02-14T19:39:16Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockPropagationManager.java

+                    new IllegalArgumentException(
+                        "Incapable of retrieving header from non-existent parent of "
+                            + block.getHeader().getNumber()
+                            + "."));


This is ok for this PR, but could you follow up with a PR to change importOrSavePendingBlock so that instead of just using protocolContext.getBlockchain().contains(block.getHeader().getParentHash()) it actually gets the parent header and checks if it's present. Then you can pass that header into broadcastBlock and not need this exception. Just be careful that the lookup of the parent header stays inside the synchronized block.

ajsutton · 2019-02-14T19:39:37Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockPropagationManager.java

@@ -105,9 +110,34 @@ private void setupListeners() {
        .subscribe(EthPV62.NEW_BLOCK_HASHES, this::handleNewBlockHashesFromNetwork);
  }

+  @VisibleForTesting
+  void broadcastBlock(final Block block) {


Since the validation is being done in here this should probably be called validateAndBroadcastBlock. And hopefully we don't need it to be @VisibleForTesting - it's a code smell that tests need access to private internals of the class being tested.

ajsutton · 2019-02-14T19:41:08Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockPropagationManager.java

  private void onBlockAdded(final BlockAddedEvent blockAddedEvent, final Blockchain blockchain) {
    // Check to see if any of our pending blocks are now ready for import
    final Block newBlock = blockAddedEvent.getBlock();
+    broadcastBlock(newBlock);


This isn't the right place to trigger block propagation. It will be called after we've processed transactions and imported to the chain - we want to do it before then, so in importOrSavePendingBlock just before the PersistBlockTask.

ajsutton · 2019-02-14T19:42:13Z

...m/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockPropagationManagerTest.java

+  @Test
+  public void verifyBroadcastBlockInvocation() {
+    final BlockPropagationManager<Void> blockPropagationManager =
+        spy(


I'd suggest using a mock here instead of a spy. Spies are generally overly complicated and we don't need the real block propagation - just want to check that the broadcast method was called.

why do you say they're overly complicated? if I change that to mock everything breaks in a confusing way

something related to that generic component

Because a spy is a combination of a mock and a real implementation. So you're simultaneously saying this thing is within the scope of the unit I'm testing (thus using the real one) but also that it's outside of the unit (thus I want to check how it's interacted with).

And the voodoo required to make a spy work leads to a number of weird implications (e.g. that test you and I hit a few weeks back where moving a method reference to the constructor broke the tests because the spy no longer picked it up).

So for this test you don't want to spy on the BlockPropagationManager, you already have a mock BlockBroadcaster that you can assert on:

@Test public void verifyBroadcastBlockInvocation() { blockchainUtil.importFirstBlocks(2); final Block block = blockchainUtil.getBlock(2); blockPropagationManager.start(); // Setup peer and messages final RespondingEthPeer peer = EthProtocolManagerTestUtil.createPeer(ethProtocolManager, 0); final UInt256 totalDifficulty = fullBlockchain.getTotalDifficultyByHash(block.getHash()).get(); final NewBlockMessage newBlockMessage = NewBlockMessage.create(block, totalDifficulty); // Broadcast message EthProtocolManagerTestUtil.broadcastMessage(ethProtocolManager, peer, newBlockMessage); final Responder responder = RespondingEthPeer.blockchainResponder(fullBlockchain); peer.respondWhile(responder, peer::hasOutstandingRequests); verify(blockBroadcaster, times(1)).propagate(block, totalDifficulty); }

And that test will fail because the total difficulty doesn't match. The test is right, the code is wrong. :) Total difficulty is being calculated as:

final UInt256 totalDifficulty = parent.getDifficulty().plus(block.getHeader().getDifficulty());

which is just parent difficulty + child difficulty when you need the sum of the difficulty for every block in the chain to get the total difficulty. final UInt256 totalDifficulty = protocolContext.getBlockchain().getTotalDifficultyByHash(parent.getHash()).get().plus(block.getHeader().getDifficulty()); will get you what you want.

interesting. so that's just the difficulty of that one parent block in isolation, the parent block doesn't incorporate the difficulty of the blocks that came before it. && more interesting still -- the genesis block has an actual difficulty. anyway - thanks for pointing this out.

ajsutton

LGTM.

ajsutton · 2019-02-17T01:41:31Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockBroadcaster.java

+    ethContext
+        .getEthPeers()
+        .availablePeers()
+        .forEach(ethPeer -> ethPeer.propagateBlock(block, difficulty));


We'll need to filter out peers that have already seen this block:
.filter(ethPeer -> ~ethPeer.hasSeenBlock(block.getHash()))
and then should add a test for that as well.

And in EthPeer.propagateBlock we should add the block to the knownBlocks set.

I forgot we needed to filter out peers that have already seen the block.

ajsutton

LGTM. Thanks Matthew.

* o * add test * clean up * scaffolding * update * update * comments * add test * update * update ii * format * update ii * fix * verifyBroadcastBlockInvocation * test * update * update to difficulty calculation * remove BlockBroadcasterTest from this pr * update * update * update II

smatthewenglish added 4 commits February 7, 2019 10:44

o

476da33

add test

93c9aa8

clean up

610834c

scaffolding

d4468a5

smatthewenglish added the work in progress Work on this pull request is ongoing label Feb 7, 2019

update

96a2126

smatthewenglish force-pushed the propaganda branch 4 times, most recently from 107ee40 to a94f0d1 Compare February 12, 2019 19:35

update

2c3b7e4

smatthewenglish force-pushed the propaganda branch from a94f0d1 to 2c3b7e4 Compare February 12, 2019 19:37

comments

1523c18

smatthewenglish removed the work in progress Work on this pull request is ongoing label Feb 12, 2019

smatthewenglish force-pushed the propaganda branch from 55a5465 to aa58cfa Compare February 12, 2019 20:40

add test

e3d12b6

smatthewenglish force-pushed the propaganda branch from aa58cfa to e3d12b6 Compare February 12, 2019 20:58

ajsutton reviewed Feb 12, 2019

View reviewed changes

CjHare reviewed Feb 13, 2019

View reviewed changes

smatthewenglish added 3 commits February 13, 2019 12:38

update

77bf74d

update ii

506b55a

merge

3e0a7b4

shemnon reviewed Feb 13, 2019

View reviewed changes

ajsutton reviewed Feb 13, 2019

View reviewed changes

smatthewenglish added 5 commits February 14, 2019 12:28

update

d734c70

format

e947f14

update ii

88a8835

fix

9911f14

verifyBroadcastBlockInvocation

58ac334

ajsutton reviewed Feb 14, 2019

View reviewed changes

smatthewenglish added 5 commits February 14, 2019 15:32

test

4ce6c83

update

395e9bc

update to difficulty calculation

f88e418

remove BlockBroadcasterTest from this pr

bb29bf8

update

b956425

ajsutton previously approved these changes Feb 16, 2019

View reviewed changes

ajsutton reviewed Feb 17, 2019

View reviewed changes

smatthewenglish added 3 commits February 18, 2019 11:30

update

52aa5ce

update II

cc6dc46

Merge remote-tracking branch 'upstream/master' into propaganda

824308c

ajsutton approved these changes Feb 18, 2019

View reviewed changes

Merge branch 'master' into propaganda

3e99654

smatthewenglish merged commit 7171172 into PegaSysEng:master Feb 18, 2019

		import com.google.common.annotations.VisibleForTesting;

		class BlockBroadcaster<C> {

[NC-2058] Improve block propagation time #808

[NC-2058] Improve block propagation time #808

Conversation

smatthewenglish commented Feb 7, 2019 • edited Loading

PR description

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

Choose a reason for hiding this comment

shemnon Feb 13, 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

smatthewenglish Feb 13, 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

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

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

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

ajsutton left a comment

Choose a reason for hiding this comment

smatthewenglish commented Feb 7, 2019 •

edited

Loading

shemnon Feb 13, 2019 •

edited

Loading

smatthewenglish Feb 13, 2019 •

edited

Loading