Ibft transmitted packets are logged by gossiper #652

rain-on · 2019-01-24T10:47:20Z

Messages which originate with the current node are logged in the
gossiper such that if a remote peer sends a packet which originated
from the local back to the local node, it should not go back out
again.

.../ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/network/IbftMessageTransmitter.java

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

Messages which originate with the current node are logged in the gossiper such that if a remote peer sends a packet which originated from the local back to the local node, it should not go back out again.

jframe · 2019-01-28T22:09:12Z

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

+  }
+
+  @Override
+  public boolean send(final MessageData messageData, final List<Address> excludeAddressesList) {


I think this method should have gossip in the name as it is doing gossiping

Would rather not have the word gossip littered through the code... or else you end up with:
gossiper.gossip(msg) ...

jframe · 2019-01-28T22:11:41Z

.../ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/network/IbftMessageTransmitter.java

@@ -36,12 +39,11 @@
 public class IbftMessageTransmitter {

  private final MessageFactory messageFactory;
-  private final ValidatorMulticaster multicaster;
+  private final Gossiper network;


Perhaps gossiper? It's not really the network

.../ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/network/IbftMessageTransmitter.java

jframe · 2019-01-28T22:24:22Z

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

+
+  boolean gossipMessage(Message message);
+
+  boolean send(MessageData messageData, List<Address> excludeAddressesList);


This form of gossip where we haven't got the original message is never going to have excluded addresses so perhaps just remove that argument?

jframe · 2019-01-28T22:29:35Z

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

+
+  @Override
+  public boolean send(final MessageData messageData, final List<Address> excludeAddressesList) {
+    Hash uniqueID = Hash.hash(messageData.getData());


Could we just use the hashCode from the messageData instead to uniquely identify the message?

…egaSysEng#655)

* Refactoring for more readable IBFT IT * Renaming Roles to peers * Moving the assert behaviour into the RoundChangePeers * Renmaing prefix of assert to verify, grammar * Reducing usage of getAllPeers() * Dropping the getter for the peer list * Dropping peer from method names, as it's now in the class name * Spotless

fixes NC-2026 adds a `--network` option instead of separated options Networks are now defined using a `--network` option that takes the case insensitive name of an enum values as input : MAINNET, RINKEBY, ROPSTEN, GOERLI, DEV These presets are a set of network-id, genesis file and boonodes list. If the `--genesis-file` is provided with a valid JSON genesis file, Pantheon uses it instead of the default network. An empty bootnodes list is then the default value and network id is the chain id found in the genesis file. `--network-id` and `--bootnodes` options can override these defaults. You can of course also override the `--network-id` and `--bootnodes` for a predefined known network (using `--network`). User have no reason to set `--network` and `--genesis-file` options at the same time that would lead to misunderstandings so we raise an error to prevent both options to be defined at the same time on the command line. Also fixes NC-2189 renaming --private-genesis-file to --genesis-file Updates a lot of doc according to the options changes

…g#653)

fixes NC-2120 --discovery-enabled option refactoring now able to have --discovery-enabled option with true as default and --discovery-enabled=true or --discovery-enabled=false on CLI and discovery-enabled=true or discovery-enabled=false in YAML config file. updates doc

…Eng#677)

…egaSysEng#658)

Separate the management of sync target and actual import from the rest of the Downloader logic in preparation for introducing a fast sync chain downloader.

jframe · 2019-01-29T03:31:09Z

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

+
+  @Override
+  public void send(final MessageData message, final Collection<Address> blackList) {
+    final Object uniqueID = message.hashCode();


be nicer to use Integer instead of Object here

CjHare

I like the separation of the multicaster into single responsibility objects 👍

CjHare · 2019-01-29T06:15:11Z

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

+
+public interface Gossiper {
+
+  void gossipMessage(Message message);


nit; gossipMessage -> send().

From the context (an interface called gossiper), a reader can infer that all the behaviour will be gossiping related. As the method input parameter is a Message does additional value really come from repeating those same details in the method name?

Messages which originate with the current node are logged in the gossiper such that if a remote peer sends a packet which originated from the local back to the local node, it should not go back out again.

rain-on requested review from jframe and CjHare January 24, 2019 10:47

CjHare reviewed Jan 24, 2019

View reviewed changes

.../ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/network/IbftMessageTransmitter.java Outdated Show resolved Hide resolved

CjHare reviewed Jan 24, 2019

View reviewed changes

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/IbftGossip.java Outdated Show resolved Hide resolved

Ibft transmitted packets are logged by gossiper

f1116a0

Messages which originate with the current node are logged in the gossiper such that if a remote peer sends a packet which originated from the local back to the local node, it should not go back out again.

rain-on force-pushed the goss_packets branch from 4188eb7 to f1116a0 Compare January 24, 2019 21:30

touch ups after review

f52c616

jframe reviewed Jan 28, 2019

View reviewed changes

Errorific and others added 22 commits January 29, 2019 12:52

[NC-1970] Added handling in admin_addPeer for when p2p is disabled (P…

cbd5a4b

…egaSysEng#655)

[NC-1970] admin_addPeer acceptance test (PegaSysEng#651)

a0bccde

add description to automatic benchmarks (PegaSysEng#646)

f39db51

Additional logging details for IBFT (PegaSysEng#650)

eb71f3e

Enable CLI config for privacy precompiled contract address (PegaSysEn…

016e002

…g#653)

[NC-1344] Create a simple WorldStateDownloader (PegaSysEng#657)

66ce6ee

Added p2p enabled (PegaSysEng#654)

424c91b

Update WorldStateDownloader run() interface to accept header (PegaSys…

a0f8089

…Eng#677)

Added reference to changelog for CLI changes mapping (PegaSysEng#673)

a68e22e

Removed available from vX.X notes (PegaSysEng#672)

ce7e034

[NC-2137] Add world state and chain download into fast sync workflow (P…

8940854

…egaSysEng#658)

Updated Quickstart docs link (PegaSysEng#681)

05a449f

[NC-2138] Extract out generic parts of Downloader (PegaSysEng#659)

1242930

Separate the management of sync target and actual import from the rest of the Downloader logic in preparation for introducing a fast sync chain downloader.

IBFT available in v1.0 note added (PegaSysEng#667)

9ba5cd0

wip

238b4e4

wip

fa2b1c6

spotless

1c3c240

repairs

3fa447d

post review spotless

ba458d4

tmohay added 5 commits January 29, 2019 13:23

Revert dirty files

0f4d871

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

2fd69bd

revert more files

28ef575

update testcontext to use unique msg multiplexer

c5275dc

spotless

130c6ec

jframe reviewed Jan 29, 2019

View reviewed changes

tmohay added 3 commits January 29, 2019 16:47

change to int uniqueID

4206af1

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

441e97e

repair to Integer

4e0a094

jframe approved these changes Jan 29, 2019

View reviewed changes

Merge branch 'master' into goss_packets

85b6c10

CjHare reviewed Jan 29, 2019

View reviewed changes

rain-on and others added 3 commits January 29, 2019 20:52

Merge branch 'master' into goss_packets

70428c1

rename gossipMessage to send

f11d4a0

Merge branch 'master' into goss_packets

cb103e0

rain-on merged commit 04dd9df into PegaSysEng:master Jan 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ibft transmitted packets are logged by gossiper #652

Ibft transmitted packets are logged by gossiper #652

rain-on commented Jan 24, 2019

jframe Jan 28, 2019

rain-on Jan 28, 2019

jframe Jan 28, 2019

rain-on Jan 29, 2019

jframe Jan 28, 2019

rain-on Jan 28, 2019

jframe Jan 28, 2019

rain-on Jan 29, 2019

jframe Jan 29, 2019

rain-on Jan 29, 2019

CjHare left a comment

CjHare Jan 29, 2019

rain-on Jan 29, 2019


		boolean gossipMessage(Message message);

		boolean send(MessageData messageData, List<Address> excludeAddressesList);


		public interface Gossiper {

		void gossipMessage(Message message);

Ibft transmitted packets are logged by gossiper #652

Ibft transmitted packets are logged by gossiper #652

Conversation

rain-on commented Jan 24, 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

CjHare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment