Events Plugin - Add initial "NewBlock" event message #1463

shemnon · 2019-05-17T22:36:49Z

PR description

It returns for any block we would broadcast to other peers, when
we would broadcast them.
It returns a JSON String containing hask, number, and timestamp
This event data is not set in stone, it may change in type or content.
Acceptance tests and unit tests got a re-work away from the assumption
that there is only one plugin type.

Fixed Issue(s)

* It returns for any block we would broadcast to other peers, when we would broadcast them. * It returns a JSON String containing hask, number, and timestamp * This event data is not set in stone, it may change in type or content. * Acceptance tests and unit tests got a re-work away from the assumption that there is only one plugin type.

make picocli plugin not listen to events.

mbaxter · 2019-05-20T14:47:18Z

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

  public void propagate(final Block block, final UInt256 totalDifficulty) {
+    newBlockSubscribers.forEach(listener -> listener.accept(block));


I don't think you want to add this functionality here. Blockchain.observeBlockAdded already supports observation of new block events.

This event needs to be fired between when we validate the header and when we validate the entire block.

The stack for this code is in part

Breakpoint reached at tech.pegasys.pantheon.ethereum.eth.sync.BlockBroadcaster.propagate(BlockBroadcaster.java:46) at tech.pegasys.pantheon.ethereum.eth.sync.BlockPropagationManager.broadcastBlock(BlockPropagationManager.java:248) at tech.pegasys.pantheon.ethereum.eth.sync.BlockPropagationManager.lambda$validateAndProcessPendingBlock$6(BlockPropagationManager.java:306) at ...

And the stack for using the Blockchain.observeBlockAdded event is

... at tech.pegasys.pantheon.ethereum.eth.sync.BlockPropagationManager.runImportTask(BlockPropagationManager.java:323) at tech.pegasys.pantheon.ethereum.eth.sync.BlockPropagationManager.validateAndProcessPendingBlock(BlockPropagationManager.java:307) at tech.pegasys.pantheon.ethereum.eth.sync.BlockPropagationManager.lambda$importOrSavePendingBlock$5(BlockPropagationManager.java:297) at tech.pegasys.pantheon.ethereum.eth.sync.BlockPropagationManager$$Lambda$1088.740466537.get(Unknown Source:-1) ...

Like 307 for BlockPropagationManager runs the full block validation, which is later than the requirements we have for when this information should be pushed to the events plugin. We don't want full block validation for this event just header (e.g. proof of work) validation.

Ok, that makes sense. I think we just need to change the naming to make it clear what this event is. In the future if we handle NEW_BLOCK_HASHES / NEW_BLOCK messages differently, we may want to expose more events to get all of the new blocks on the network. Most clients don't propagate out NEW_BLOCK messages in response to receiving NEW_BLOCK_HASHES from the network, for example.

mbaxter · 2019-05-20T16:03:34Z

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

 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;

 public class BlockBroadcaster {
  private static final Logger LOG = LogManager.getLogger();

  private final EthContext ethContext;
+  private final Subscribers<Consumer<Block>> newBlockSubscribers = new Subscribers<>();


Suggested change

private final Subscribers<Consumer<Block>> newBlockSubscribers = new Subscribers<>();

private final Subscribers<Consumer<Block>> blockPropagatedSubscribers = new Subscribers<>();

mbaxter · 2019-05-20T16:09:25Z

pantheon/src/main/java/tech/pegasys/pantheon/services/PantheonEventsImpl.java

+  }
+
+  @Override
+  public Object addBlockAddedListener(final Consumer<String> blockJSONListener) {


Suggested change

public Object addBlockAddedListener(final Consumer<String> blockJSONListener) {

public Object observeNewBlockPropagated(final Consumer<String> blockJSONListener) {

Listener is more prevalent in our code base than observer. So I added the Propagated but kept the Listener.

mbaxter · 2019-05-20T16:11:48Z

pantheon/src/main/java/tech/pegasys/pantheon/services/PantheonEventsImpl.java

+  }
+
+  @Override
+  public void removeBlockAddedObserver(final Object listenerIdentifier) {


Suggested change

public void removeBlockAddedObserver(final Object listenerIdentifier) {

public void removeNewBlockPropagatedObserver(final Object listenerIdentifier) {

Listener is more prevalent in our code base than observer. So I added the Propagated but kept the Listener.

mbaxter · 2019-05-20T16:12:04Z

pantheon/src/main/java/tech/pegasys/pantheon/services/PantheonEventsImpl.java

+    }
+  }
+
+  private void dispatchNewBlockMessage(final Block block, final Consumer<String> callback) {


Suggested change

private void dispatchNewBlockMessage(final Block block, final Consumer<String> callback) {

private void dispatchNewBlockPropagatedMessage(final Block block, final Consumer<String> callback) {

Listener is more prevalent in our code base than observer. So I added the Propagated but kept the Listener.

mbaxter · 2019-05-20T16:28:05Z

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

  public void propagate(final Block block, final UInt256 totalDifficulty) {
+    newBlockSubscribers.forEach(listener -> listener.accept(block));


Ok, that makes sense. I think we just need to change the naming to make it clear what this event is. In the future if we handle NEW_BLOCK_HASHES / NEW_BLOCK messages differently, we may want to expose more events to get all of the new blocks on the network. Most clients don't propagate out NEW_BLOCK messages in response to receiving NEW_BLOCK_HASHES from the network, for example.

mbaxter · 2019-05-20T16:30:05Z

pantheon/src/main/java/tech/pegasys/pantheon/services/PantheonEventsImpl.java

+  }
+
+  @Override
+  public Object addBlockAddedListener(final Consumer<String> blockJSONListener) {


(optional) Consider adding a functional interface in place of Consumer<String>.

…ramework

* added a proper listener interface

mbaxter

LGTM pending the typo fix

mbaxter · 2019-05-20T19:05:27Z

pantheon/src/main/java/tech/pegasys/pantheon/services/PantheonEventsImpl.java

+  }
+
+  @Override
+  public Object addNewBlockPropagatedLister(final NewBlockPropagatedListener listener) {


Suggested change

public Object addNewBlockPropagatedLister(final NewBlockPropagatedListener listener) {

public Object addNewBlockPropagatedListener(final NewBlockPropagatedListener listener) {

Typo

mbaxter · 2019-05-20T19:09:48Z

build.gradle

@@ -105,7 +105,7 @@ allprojects {
    java {
      // This path needs to be relative to each project
      target fileTree('.') {
-        include '**/*.java'
+        include '**/src/*/java/**/*.java'


what are these changes for??

meant for another PR. Spotless was catching intellij generated files from .../out/classes/.... By limiting src to having to live in src directories generated and transient java files don't get style checked.

shemnon added 3 commits May 17, 2019 16:35

fix plugin lookup to be relative to pantheon.home

a429e7d

make picocli plugin not listen to events.

spotless

935060f

mbaxter suggested changes May 20, 2019

View reviewed changes

mbaxter reviewed May 20, 2019

View reviewed changes

shemnon added 2 commits May 20, 2019 12:09

Merge branch 'master' of github.com:PegaSysEng/pantheon into plugin_f…

9097514

…ramework

* Change blockAddedListener to newBlockPropagatedListener

fbfb30b

* added a proper listener interface

mbaxter approved these changes May 20, 2019

View reviewed changes

shemnon added 2 commits May 20, 2019 13:14

accidental build change

bff3ce5

speling

3a8a658

shemnon merged commit 9ab506b into PegaSysEng:master May 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Events Plugin - Add initial "NewBlock" event message #1463

Events Plugin - Add initial "NewBlock" event message #1463

shemnon commented May 17, 2019

mbaxter May 20, 2019

shemnon May 20, 2019

mbaxter May 20, 2019

mbaxter May 20, 2019

mbaxter May 20, 2019

shemnon May 20, 2019

mbaxter May 20, 2019

shemnon May 20, 2019

mbaxter May 20, 2019

shemnon May 20, 2019

mbaxter May 20, 2019

mbaxter May 20, 2019

shemnon May 20, 2019

mbaxter left a comment

mbaxter May 20, 2019

mbaxter May 20, 2019

shemnon May 20, 2019

		public void propagate(final Block block, final UInt256 totalDifficulty) {
		newBlockSubscribers.forEach(listener -> listener.accept(block));

	private final Subscribers<Consumer<Block>> newBlockSubscribers = new Subscribers<>();
	private final Subscribers<Consumer<Block>> blockPropagatedSubscribers = new Subscribers<>();

	public Object addBlockAddedListener(final Consumer<String> blockJSONListener) {
	public Object observeNewBlockPropagated(final Consumer<String> blockJSONListener) {

	public void removeBlockAddedObserver(final Object listenerIdentifier) {
	public void removeNewBlockPropagatedObserver(final Object listenerIdentifier) {

	private void dispatchNewBlockMessage(final Block block, final Consumer<String> callback) {
	private void dispatchNewBlockPropagatedMessage(final Block block, final Consumer<String> callback) {

	public Object addNewBlockPropagatedLister(final NewBlockPropagatedListener listener) {
	public Object addNewBlockPropagatedListener(final NewBlockPropagatedListener listener) {

Events Plugin - Add initial "NewBlock" event message #1463

Events Plugin - Add initial "NewBlock" event message #1463

Conversation

shemnon commented May 17, 2019

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

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

mbaxter 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