Ibft notifies EthPeer when remote node has a better block #849

rain-on · 2019-02-13T04:08:42Z

There is a failure mode of IBFT whereby a validator fails to import
a block, and also fails to receive the NewBlock message from its
peers. This means said validator is unable to participate in
subsequent rounds, and may cause the network to halt.

To overcome this issue, if an IBFT validator receives messages from
a future height, it will update the "BestEstimatedHeight" of the
corresponding EthPeer object, such that the Synchroniser will
(eventually) download the requisite blocks - thus allowing the
IBFT network to continue to operate.

There is a failure mode of IBFT whereby a validator fails to import a block, and also fails to receive the NewBlock message from its peers. This means said validator is unable to participate in subsequent rounds, and may cause the network to halt. To overcome this issue, if an IBFT validator receives messages from a future height, it will update the "BestEstimatedHeight" of the corresponding EthPeer object, such that the Synchroniser will (eventually) download the requisite blocks - thus allowing the IBFT network to continue to operate.

ajsutton · 2019-02-13T04:13:48Z

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/BaseSynchronizerUpdater.java

+
+import tech.pegasys.pantheon.ethereum.p2p.api.PeerConnection;
+
+public interface BaseSynchronizerUpdater {


How is this a "base" synchronizer updater? Seems like this should be SynchronizerUpdater and the current SynchronizerUpdater should be EthSynchronizerUpdater.

I was really hoping someone would help out with the naming.... :)

ajsutton · 2019-02-13T04:15:06Z

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/SynchronizerUpdater.java

+      final long knownBlockNumber, final PeerConnection peerConnection) {
+    final EthPeer ethPeer = ethPeers.peer(peerConnection);
+    if (ethPeer == null) {
+      LOG.warn("Received message from a peer with no corresponding EthPeer.");


I wouldn't log this at WARN level. It generally just means the peer disconnected between when the message was received and the processing got to this point. Debug is probably the right level.

ajsutton · 2019-02-13T04:17:29Z

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/SynchronizerUpdater.java

+    }
+
+    if (ethPeer.chainState().getEstimatedHeight() < knownBlockNumber) {
+      ethPeer.chainState().updateHeightEstimate(knownBlockNumber);


You don't need the if condition here, updateHeightEstimate already checks the new height is higher than the previous. And it can do it safely inside a synchronized block.

ajsutton · 2019-02-13T04:18:25Z

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

@@ -88,7 +88,7 @@ public void updateForAnnouncedBlock(
    }
  }

-  private void updateHeightEstimate(final long blockNumber) {
+  public void updateHeightEstimate(final long blockNumber) {
    estimatedHeightKnown = true;
    if (blockNumber > estimatedHeight) {
      estimatedHeight = blockNumber;


This will need a synchronized block around the whole method content. Previously it was only called from other methods which already had synchronized.

CjHare · 2019-02-13T05:53:16Z

...ation-test/java/tech/pegasys/pantheon/consensus/ibft/support/StubbedSynchronizerUpdater.java

+
+public class StubbedSynchronizerUpdater implements BaseSynchronizerUpdater {
+
+  private Map<PeerConnection, ValidatorPeer> validatorNodes;


What am I missing here, doesn't validatorNodes need initialising to avoid NPE?

There's no NPE as the addNetworkPeers creates the map and assigns the created map to this variable.

CjHare · 2019-02-13T05:57:29Z

consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/SynchronizerUpdaterTest.java

+
+    updater.updatePeerChainState(suppliedChainHeight, createAnonymousPeerConnection());
+
+    verify(chainState, times(0)).updateHeightEstimate(anyLong());


Should the anyLong() be knownChainHeight? ...unless any value is really acceptable?

given this is a verify (rather than a "thenReturns") - I'd like to make sure that this function wasn't invoked at all - regardless of the parameter.

jframe · 2019-02-13T21:37:34Z

...c/integration-test/java/tech/pegasys/pantheon/consensus/ibft/support/RoundSpecificPeers.java

@@ -125,6 +126,10 @@ public void commitForNonProposing(final ConsensusRoundIdentifier roundId, final
    nonProposingPeers.forEach(peer -> peer.injectCommit(roundId, hash));
  }

+  public void forNonProposing(final Consumer<ValidatorPeer> assertion) {
+    nonProposingPeers.forEach(peer -> assertion.accept(peer));


could just do nonProposingPeers.forEach(assertion);

way better.nice.

jframe · 2019-02-13T21:40:03Z

...ation-test/java/tech/pegasys/pantheon/consensus/ibft/support/StubbedSynchronizerUpdater.java

+  public void addNetworkPeers(final Collection<ValidatorPeer> nodes) {
+    validatorNodes =
+        nodes.stream()
+            .collect(Collectors.toMap(ValidatorPeer::getPeerConnection, validator -> validator));


instead validator -> validator why just use Function.identity() not really any shorter but it's clearer I think

jframe · 2019-02-13T21:59:17Z

consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/SynchronizerUpdaterTest.java

+
+    updater.updatePeerChainState(suppliedChainHeight, createAnonymousPeerConnection());
+
+    verify(chainState, times(0)).updateHeightEstimate(anyLong());


never() is nicer to use than times(0)

* Moving NodeWhitelistController to permissioning package * Refactoring unit tests * Fix string.format

* check the token if authentication enabled, regardless of user optional status * more tests

…ull. (PegaSysEng#859)

* o * add test * clean up * scaffolding * update * update * comments * add test * update * update * update

…vot (PegaSysEng#862)

…sEng#853)

CjHare · 2019-02-18T00:15:31Z

...ation-test/java/tech/pegasys/pantheon/consensus/ibft/support/StubbedSynchronizerUpdater.java

+
+public class StubbedSynchronizerUpdater implements BaseSynchronizerUpdater {
+
+  private Map<PeerConnection, ValidatorPeer> validatorNodes;


CjHare · 2019-02-18T00:16:36Z

...ation-test/java/tech/pegasys/pantheon/consensus/ibft/support/StubbedSynchronizerUpdater.java

+    validatorNodes.get(peerConnection).updateEstimatedChainHeight(knownBlockNumber);
+  }
+
+  public void addNetworkPeers(final Collection<ValidatorPeer> nodes) {


Is this an add operations or a set operation? ...what happens when its invoked twice?

any thoughts? @rain-on

just saw this - create the map instantiation, then putAll in here. Thus will continue to grow as you call "add" - given its called once in the integration test framework setup, should be acceptable (i.e. doesn't require a "clear()" call atm.

jframe · 2019-02-18T04:41:42Z

...sus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftController.java

@@ -248,5 +254,9 @@ private void addMessageToFutureMessageBuffer(final long chainHeight, final Messa
      futureMessages.put(chainHeight, Lists.newArrayList());
    }
    futureMessages.get(chainHeight).add(rawMsg);
+
+    // Notify the synchroniser the transmitting peer must have the parent block to the received


Think this would make more sense being in processMessage after addMessageToFutureMessageBuffer call instead of being in the addMessageToFutureMessageBuffer method as notifying the synchroniser is not really part of updating the future message buffer.

I had assumed that when the "smart" FutureMessageBuffer came to fruition then this would move across into that class, and it would be the FutureMessageHandler - meaning it would buffer and notify other people, thus the IbftController doesn't need to know about this stuff...

...sus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftController.java

Fix for the _“The input line is too long”_ error on Windows. This fix makes Gradle generate a shortened but operational command in `pantheon.bat`. The modified `pantheon.bat`, instead of explicitly referencing each .jar individually when initializing the classpath, uses the wildcard character `*` instead.

…SysEng#908)

…ally stored. (PegaSysEng#893)

…sEng#906)

…aSysEng#897)

…difficulty, not the highest chain. (PegaSysEng#899)

…mpleted). (PegaSysEng#901)

--> adapt `BlockBroadcaster` to use `send` method on `EthPeer` --> adapt `EthProtocolMamnager` to utilize `BlockBroadcaster` to disseminate newly mined block

…sEng#912) * [PAN-2117] Error when removing bootnodes from nodes whitelist * Fixing AT

Gives approximately a 10% perf improvement in isolation and in real-world tests allows the world state requests to actually reach the concurrent limit.

…local data. (PegaSysEng#914)

* check nodes or accounts permissioning is enabled if config file is set

rain-on requested review from ajsutton, jframe and CjHare February 13, 2019 04:08

rain-on force-pushed the eth_peer_best_block branch from 3bb8136 to 6663f3f Compare February 13, 2019 04:12

ajsutton reviewed Feb 13, 2019

View reviewed changes

CjHare reviewed Feb 13, 2019

View reviewed changes

tmohay added 2 commits February 14, 2019 08:52

post review fixes

8b7eb67

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

d40720f

jframe reviewed Feb 13, 2019

View reviewed changes

tmohay and others added 12 commits February 14, 2019 22:25

repair compile issues

c052f49

Moving NodeWhitelistController to permissioning package (PegaSysEng#855)

4cdf780

* Moving NodeWhitelistController to permissioning package * Refactoring unit tests * Fix string.format

[NC-2046] check for invalid token (PegaSysEng#856)

5ef5b35

* check the token if authentication enabled, regardless of user optional status * more tests

Fix NullPointerException - complete future with fast sync state not n…

df39a22

…ull. (PegaSysEng#859)

[NC-2058] initial scaffolding re block propagation (PegaSysEng#860)

bf92b85

* o * add test * clean up * scaffolding * update * update * comments * add test * update * update * update

IBFT config options (PegaSysEng#861)

5310ffa

Disconnect peers where the common ancestor is before our fast sync pi…

e1cf28c

…vot (PegaSysEng#862)

Renaming authentication config file cli command (PegaSysEng#865)

a9263c3

Fix state download race condition by creating a TaskQueue API (PegaSy…

b277f38

…sEng#853)

fixed jframe comments

fb6b46e

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

154a99b

Merge branch 'master' into eth_peer_best_block

5b1d39b

CjHare reviewed Feb 18, 2019

View reviewed changes

jframe reviewed Feb 18, 2019

View reviewed changes

rain-on and others added 6 commits February 19, 2019 09:24

Merge branch 'master' into eth_peer_best_block

e646f1e

[PAN-2313] Fix authentication header (PegaSysEng#891)

cacbc9a

Fixed MetaMask link

dbdd8c5

gensis typo (PegaSysEng#892)

9d0541a

[Minor] fix auth file validation errors (PegaSysEng#886)

e56915a

Added --permissions-config-file option (PegaSysEng#874)

5d06bb5

Guillaume Lethuillier and others added 23 commits February 20, 2019 11:29

Unit tests of BlockBroadcaster (PegaSysEng#887)

f37cc2d

Limit the number of times we retry peer discovery interactions. (Pega…

ffe38f1

…SysEng#908)

Fix task queue so that the updated failure count for requests is actu…

abd490c

…ally stored. (PegaSysEng#893)

Add metric to expose number of inflight world state requests. (PegaSy…

f929e1f

…sEng#906)

Added Resources page linking to Pantheon blog posts and webinars (Peg…

a6d0d96

…aSysEng#897)

When picking fast sync pivot block, use the peer with the best total …

fda78b7

…difficulty, not the highest chain. (PegaSysEng#899)

Halt the chain download when the future is cancelled (or otherwise co…

92bb480

…mpleted). (PegaSysEng#901)

Process world state download data on a worker thread (PegaSysEng#898)

187c575

Incremental Optimization(s) on BlockBroadcaster (PegaSysEng#911)

fbe197e

--> adapt `BlockBroadcaster` to use `send` method on `EthPeer` --> adapt `EthProtocolMamnager` to utilize `BlockBroadcaster` to disseminate newly mined block

Removed version notes

e77c8bb

[PAN-2117] Error when removing bootnodes from nodes whitelist (PegaSy…

2beb65e

…sEng#912) * [PAN-2117] Error when removing bootnodes from nodes whitelist * Fixing AT

Use an iterator when dequeuing tasks. (PegaSysEng#913)

332f806

Gives approximately a 10% perf improvement in isolation and in real-world tests allows the world state requests to actually reach the concurrent limit.

Increment completed request count when we complete the request using …

4659ecd

…local data. (PegaSysEng#914)

[PAN-2316] check permissions CLI dependencies (PegaSysEng#909)

05a9ec2

* check nodes or accounts permissioning is enabled if config file is set

Expose the Ibft MiningCoordinator (PegaSysEng#916)

a5fab48

repaired StubbedSynchronizerUpdater

a91d6f5

just to kick it off

286d59a

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

6da1588

post merge

330507d

getting there

27bb15d

Merge branch 'master' into eth_peer_best_block

92e85a1

fix ethPeers

dcad364

CjHare approved these changes Feb 20, 2019

View reviewed changes

move ibftcontroller code

69f5c62

jframe approved these changes Feb 20, 2019

View reviewed changes

rain-on and others added 2 commits February 20, 2019 16:26

Merge branch 'master' into eth_peer_best_block

34e073d

fix spotless

75f52f9

rain-on merged commit 8b5fada into PegaSysEng:master Feb 20, 2019

rain-on deleted the eth_peer_best_block branch February 20, 2019 06:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ibft notifies EthPeer when remote node has a better block #849

Ibft notifies EthPeer when remote node has a better block #849

rain-on commented Feb 13, 2019

ajsutton Feb 13, 2019

rain-on Feb 13, 2019

ajsutton Feb 13, 2019

rain-on Feb 14, 2019

ajsutton Feb 13, 2019

rain-on Feb 13, 2019

ajsutton Feb 13, 2019

rain-on Feb 13, 2019

CjHare Feb 13, 2019

rain-on Feb 13, 2019

CjHare Feb 18, 2019

CjHare Feb 13, 2019

rain-on Feb 13, 2019

jframe Feb 13, 2019

rain-on Feb 14, 2019

jframe Feb 13, 2019

rain-on Feb 14, 2019

jframe Feb 13, 2019

rain-on Feb 14, 2019

CjHare Feb 18, 2019

CjHare Feb 18, 2019

CjHare Feb 19, 2019

rain-on Feb 20, 2019

jframe Feb 18, 2019

rain-on Feb 18, 2019


		import tech.pegasys.pantheon.ethereum.p2p.api.PeerConnection;

		public interface BaseSynchronizerUpdater {


		public class StubbedSynchronizerUpdater implements BaseSynchronizerUpdater {

		private Map<PeerConnection, ValidatorPeer> validatorNodes;


		updater.updatePeerChainState(suppliedChainHeight, createAnonymousPeerConnection());

		verify(chainState, times(0)).updateHeightEstimate(anyLong());

Ibft notifies EthPeer when remote node has a better block #849

Ibft notifies EthPeer when remote node has a better block #849

Conversation

rain-on commented Feb 13, 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

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