Ibft queries #138

rain-on · 2019-10-25T05:49:16Z

Updates to besu to allow for IBFT specific queries to be executed from a plugin

Signed-off-by: Trent Mohay <[email protected]>

consensus/common/src/main/java/org/hyperledger/besu/consensus/common/PoAMetricServiceImpl.java

consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/queries/IbftQueriesImpl.java

Signed-off-by: Trent Mohay <[email protected]>

ajsutton · 2019-10-25T06:07:51Z

besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java

-              besuController.getProtocolContext().getBlockchain());
-      besuPluginContext.addService(PoAMetricsService.class, service);
+    if (consensusState != null) {
+      if (IbftContext.class.isAssignableFrom(consensusState.getClass())) {


I know you're following the existing pattern here, but why are we doing isAssignableFrom checks here? I would have expected this to be pushed into the BesuController class to register these services as it has the correct context types and doesn't need any casting.

Yeah - way nicer

ajsutton · 2019-10-25T06:12:11Z

plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/IbftQueries.java

+
+import java.util.Collection;
+
+public interface IbftQueries extends PoAMetricsService {


Given this is part of the plugin apis it will need comprehensive javadoc.

Agreed - for now just needed to get the code in front of Uzi as he's working on something related.

ajsutton · 2019-10-25T06:14:35Z

plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/IbftQueries.java

+
+public interface IbftQueries extends PoAMetricsService {
+
+  public int getRoundNumberFromCanonicalHead();


I like this as a convenience method but I think it's a bit of an anti-pattern we're developing with only being able to get information about the head block in these PoA APIs. We really should have a variant that accepts a BlockHeader so you can ask for the round number for any block, not just the canonical head. Also applies to PoAMetricsService.getValidatorsForLatestBlock. And the naming between the two should be consistent as well - CanonicalHead is probably the right name so maybe need to make a note to rename getValidatorsForLatestBlock to getValidatorsFromCanonicalHead at the same time as we rename PoAMetricsService to remove "metrics" from the name.

Signed-off-by: Trent Mohay <[email protected]>

usmansaleem

looks good to me

Signed-off-by: Trent Mohay <[email protected]>

usmansaleem

Looks good to me

mark-terry · 2019-10-29T13:08:36Z

plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/PoaQueryService.java

   *
-   * @return Identities of the validators who formed quorum on the latest block.
+   * @return Adresses of all validators in the latest canonical block.


Typo - Addresses

mark-terry · 2019-10-29T13:10:01Z

plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/PoaQueryService.java

@@ -22,12 +22,12 @@
 /**
 * Provides relevant data for producing metrics on the status of a Proof of Authority (PoA) node.


Comment should likely be updated to remove references to metrics.

ajsutton · 2019-10-29T21:08:39Z

plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/PoaQueryService.java

@@ -22,12 +22,12 @@
 /**
 * Provides relevant data for producing metrics on the status of a Proof of Authority (PoA) node.
 */
-public interface PoAMetricsService {
+public interface PoaQueryService {


This is a backwards incompatible change on our public, stable plugin APIs. I think we need to deprecate the old interface but keep it around (the same implementation would implement both old and new). We also need to document a deprecation policy so we are clear about when we can remove the old interface.

ajsutton · 2019-10-30T00:47:59Z

plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/PoAMetricsService.java

@@ -21,7 +21,10 @@

 /**
 * Provides relevant data for producing metrics on the status of a Proof of Authority (PoA) node.
+ *
+ * @deprecated Classes which inherit from this interface should move to using PoaQueryService.


nit: It's not just classes which inherit that need to migrate, also users of the interface.

Suggested change

* @deprecated Classes which inherit from this interface should move to using PoaQueryService.

* @deprecated This interface has been replaced by {@link PoaQueryService}.

reworded as below

ajsutton · 2019-10-30T00:49:01Z

consensus/common/src/main/java/org/hyperledger/besu/consensus/common/PoaQueryServiceImpl.java


 import java.util.ArrayList;
 import java.util.Collection;

-public class PoAMetricServiceImpl implements PoAMetricsService {
+public class PoaQueryServiceImpl implements PoaQueryService {


Should also implement PoAMetricsService.

ajsutton · 2019-10-30T00:50:16Z

besu/src/main/java/org/hyperledger/besu/controller/CliqueQueryFactory.java

+    final BlockInterface blockInterface = new CliqueBlockInterface();
+
+    besuContext.addService(
+        PoaQueryService.class, new PoaQueryServiceImpl(blockInterface, blockchain));


We should register the one implementation instance of PoaQueryServiceImpl under both PoaQueryService and PoaMetricsService to preserve backwards compatibility.

ajsutton · 2019-10-30T00:52:11Z

besu/src/main/java/org/hyperledger/besu/controller/IbftQueryFactory.java

+    final BlockInterface blockInterface = new IbftBlockInterface();
+
+    besuContext.addService(
+        IbftQueryService.class, new IbftQueryServiceImpl(blockInterface, blockchain));


We should register the one implementation instance of PoaQueryServiceImpl under all of PoaQueryService, PoaMetricsService and IbftQueryService to preserve backwards compatibility.

This ensures users who are currently using pluginContext.getService(PoaMetricsService.class) or who want to have a plugin that works for both IBFT & Clique requesting PoaQueryService.

Signed-off-by: Trent Mohay <[email protected]>

ajsutton

I haven't looked at the particular implementation details, just the plugin API changes. I'm happy they're backwards compatible now and the APIs look ok. Probably worth double checking on Poa vs PoA capitalisation and a couple of tweaks to comments suggested.

ajsutton · 2019-10-30T04:33:27Z

besu/src/main/java/org/hyperledger/besu/controller/CliqueQueryPluginServiceFactory.java

+    besuContext.addService(
+        PoaQueryService.class, new PoaQueryServiceImpl(blockInterface, blockchain));
+    besuContext.addService(
+        PoAMetricsService.class, new PoaQueryServiceImpl(blockInterface, blockchain));


nit: we could just use the one instance.

final PoaQueryServiceImpl poaQueryServiceImpl = new PoaQueryServiceImpl(blockInterface, blockchain)); besuContext.addService(PoaQueryService.class, poaQueryServiceImpl); besuContext.addService(PoAMetricsService.class, new PoaQueryServiceImpl(blockInterface, blockchain));

both preserve backwards compatibility correctly though.

oh yeah - nicer.

ajsutton · 2019-10-30T04:33:53Z

besu/src/main/java/org/hyperledger/besu/controller/IbftQueryPluginServiceFactory.java

+    besuContext.addService(
+        IbftQueryService.class, new IbftQueryServiceImpl(blockInterface, blockchain));
+    besuContext.addService(
+        PoaQueryService.class, new IbftQueryServiceImpl(blockInterface, blockchain));
+    besuContext.addService(
+        PoAMetricsService.class, new IbftQueryServiceImpl(blockInterface, blockchain));


Could use a single instance of IbftQueryServiceImpl here too.

ajsutton · 2019-10-30T04:35:01Z

besu/src/main/java/org/hyperledger/besu/services/BesuPluginContextImpl.java

@@ -69,6 +70,10 @@
    serviceRegistry.put(serviceType, service);
  }

+  public void addServicesFrom(final PluginServiceFactory factory) {
+    factory.appendPluginServices(this);


nit: Why we we have this level of indirection? Wouldn't it be easier to remove this method and just call factory.appendPluginServices(besuPluginContext); from the calling site?

that is true in retrospect

ajsutton · 2019-10-30T04:36:27Z

...sus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/queries/IbftQueryServiceImpl.java

+import java.util.Collections;
+
+public class IbftQueryServiceImpl extends PoaQueryServiceImpl
+    implements IbftQueryService, PoAMetricsService {


nit: You don't need to have PoAMetricsService in the implements list here because PoaQueryServiceImpl already implements it.

Suggested change

implements IbftQueryService, PoAMetricsService {

implements IbftQueryService {

ajsutton · 2019-10-30T04:39:53Z

plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/PoAMetricsService.java

 */
+@Deprecated
 public interface PoAMetricsService {


nit: Can we check if we're using PoA or Poa to make sure these new APIs match. I think Poa is strictly right according to the style guide but looks really weird to me - I'd expect PoA. I can live with it either way, just want to minimise inconsistency.

I personally am in favour of Poa, then POA then finally PoA (but it doesn't conform to any recognised capitalisation that I can think of)

ajsutton · 2019-10-30T04:42:00Z

plugin-api/src/main/java/org/hyperledger/besu/plugin/services/query/IbftQueryService.java

+/** Allows for the IBFT specific aspects of the block chain to be queried from plugins. */
+public interface IbftQueryService extends PoaQueryService {
+
+  /**


Missing a comment describing the method. I know the @param and @return cover it but when JavaDoc is generated this will wind up showing as blank in the summary.

ajsutton · 2019-10-30T04:43:18Z

plugin-api/src/main/java/org/hyperledger/besu/plugin/services/query/IbftQueryService.java

+
+import java.util.Collection;
+
+/** Allows for the IBFT specific aspects of the block chain to be queried from plugins. */


No need to mention plugins.

Suggested change

/** Allows for the IBFT specific aspects of the block chain to be queried from plugins. */

/** Provides access to IBFT2-specific information about the blockchain. This service is only available when using IBFT2. */

ajsutton · 2019-10-30T04:43:33Z

plugin-api/src/main/java/org/hyperledger/besu/plugin/services/query/IbftQueryService.java

+   */
+  int getRoundNumberFrom(final BlockHeader header);
+
+  /**


Needs an overview comment.

Backwards compatibility problems have been resolved.

Signed-off-by: Trent Mohay <[email protected]>

jframe · 2019-10-31T01:33:03Z

...sus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/queries/IbftQueryServiceImpl.java

+  @Override
+  public int getRoundNumberFrom(final org.hyperledger.besu.plugin.data.BlockHeader header) {
+    final BlockHeader headerFromChain = getHeaderFromChain(header);
+    final IbftExtraData extraData = IbftExtraData.decode(headerFromChain);


Are we only supporting Ibft 2?

Maybe slack, but as an MVP - only IBFT2 is required.

mark-terry

LGTM.

Signed-off-by: Trent Mohay <[email protected]>

rain-on requested a review from usmansaleem October 25, 2019 05:49

Starting IBFT Queries

a7128a1

Signed-off-by: Trent Mohay <[email protected]>

rain-on force-pushed the ibft_queries branch from 68a5a41 to a7128a1 Compare October 25, 2019 05:55

usmansaleem reviewed Oct 25, 2019

View reviewed changes

consensus/common/src/main/java/org/hyperledger/besu/consensus/common/PoAMetricServiceImpl.java Outdated Show resolved Hide resolved

usmansaleem reviewed Oct 25, 2019

View reviewed changes

consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/queries/IbftQueriesImpl.java Outdated Show resolved Hide resolved

some renames

b3e3ab1

Signed-off-by: Trent Mohay <[email protected]>

ajsutton reviewed Oct 25, 2019

View reviewed changes

minor renames

e631b1c

Signed-off-by: Trent Mohay <[email protected]>

usmansaleem approved these changes Oct 25, 2019

View reviewed changes

Change of tack

c7e89b2

Signed-off-by: Trent Mohay <[email protected]>

usmansaleem approved these changes Oct 28, 2019

View reviewed changes

mark-terry reviewed Oct 29, 2019

View reviewed changes

ajsutton previously requested changes Oct 29, 2019

View reviewed changes

ajsutton reviewed Oct 30, 2019

View reviewed changes

Trent Mohay added 4 commits October 30, 2019 12:53

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

a2d0395

Signed-off-by: Trent Mohay <[email protected]>

added deprecated

1cd1667

Signed-off-by: Trent Mohay <[email protected]>

personal review

f6355e7

Signed-off-by: Trent Mohay <[email protected]>

added unit tests

51cdf98

Signed-off-by: Trent Mohay <[email protected]>

rain-on force-pushed the ibft_queries branch from 7aadfd9 to 51cdf98 Compare October 30, 2019 04:29

ajsutton reviewed Oct 30, 2019

View reviewed changes

fix headers

93aec23

Signed-off-by: Trent Mohay <[email protected]>

rain-on force-pushed the ibft_queries branch from 165baa3 to 93aec23 Compare October 30, 2019 05:07

more cleansing

2b758d2

Signed-off-by: Trent Mohay <[email protected]>

jframe reviewed Oct 31, 2019

View reviewed changes

mark-terry approved these changes Oct 31, 2019

View reviewed changes

rain-on and others added 4 commits October 31, 2019 12:46

Merge branch 'master' into ibft_queries

6532e5f

remove BesuContextImpl::addServicesFrom

9058360

Signed-off-by: Trent Mohay <[email protected]>

fixed spotless

03545e0

Signed-off-by: Trent Mohay <[email protected]>

Added unit tests for GetSigners

27ff144

Signed-off-by: Trent Mohay <[email protected]>

jframe approved these changes Oct 31, 2019

View reviewed changes

rain-on and others added 4 commits October 31, 2019 21:57

Merge branch 'master' into ibft_queries

ee98d56

fix plugin api hash

682e768

Signed-off-by: Trent Mohay <[email protected]>

repair tests

65595f9

Signed-off-by: Trent Mohay <[email protected]>

Merge branch 'master' into ibft_queries

f0acc89

rain-on merged commit 38462bb into hyperledger:master Oct 31, 2019

rain-on deleted the ibft_queries branch October 31, 2019 22:22

gconnect mentioned this pull request Sep 18, 2024

Add ephemery network config #7563

Merged

8 tasks


		import java.util.Collection;

		public interface IbftQueries extends PoAMetricsService {


		public interface IbftQueries extends PoAMetricsService {

		public int getRoundNumberFromCanonicalHead();

		@@ -22,12 +22,12 @@
		/**
		* Provides relevant data for producing metrics on the status of a Proof of Authority (PoA) node.

	* @deprecated Classes which inherit from this interface should move to using PoaQueryService.
	* @deprecated This interface has been replaced by {@link PoaQueryService}.

	implements IbftQueryService, PoAMetricsService {
	implements IbftQueryService {


		import java.util.Collection;

		/** Allows for the IBFT specific aspects of the block chain to be queried from plugins. */

	/** Allows for the IBFT specific aspects of the block chain to be queried from plugins. */
	/** Provides access to IBFT2-specific information about the blockchain. This service is only available when using IBFT2. */

Ibft queries #138

Ibft queries #138

Conversation

rain-on commented Oct 25, 2019

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

usmansaleem left a comment

Choose a reason for hiding this comment

usmansaleem 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

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

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

mark-terry left a comment

Choose a reason for hiding this comment