Limit ibft msg queues #704

jframe · 2019-01-30T02:51:10Z

PR description

Add a hard cap to the number unprocessed messages in the event queue and the seen messages tracked.

Fixed Issue(s)

…ager

CjHare

LGTM

rain-on · 2019-01-31T22:44:16Z

config/src/test/java/tech/pegasys/pantheon/config/IbftConfigOptionsTest.java

@@ -26,6 +26,7 @@
  private static final int EXPECTED_DEFAULT_EPOCH_LENGTH = 30_000;
  private static final int EXPECTED_DEFAULT_BLOCK_PERIOD = 1;
  private static final int EXPECTED_DEFAULT_REQUEST_TIMEOUT = 1;
+  private static final int EXPECTED_DEFAULT_MESSAGE_BUFFER_SIZE = 10_000;


Are these oddly named? I.e. what does "EXPECTED_" indicate?

They are bit oddly named I guess. The "EXPECTED_" indicates this expected values for the assertion. These values are used to assert the default values are correct in the tests. It's existing test code so I just followed the existing pattern.

rain-on · 2019-01-31T22:47:23Z

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

@@ -158,7 +159,8 @@ public TestContext build() {
    // Use a stubbed version of the multicaster, to prevent creating PeerConnections etc.
    final StubValidatorMulticaster multicaster = new StubValidatorMulticaster();

-    final UniqueMessageMulticaster uniqueMulticaster = new UniqueMessageMulticaster(multicaster);
+    final UniqueMessageMulticaster uniqueMulticaster =
+        new UniqueMessageMulticaster(multicaster, MESSAGE_BUFFER_SIZE);


should Multicaster be updated as part of this change? Or is that a separate problem?
Is the size of the multicaster buffer related to the incoming event queue?

The multicaster should be updated to have a limited size buffer. Would be better to have a seperate config option though.

rain-on · 2019-01-31T22:48:29Z

.../src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftBlockHeightManager.java

@@ -64,7 +64,7 @@
  private final BlockTimer blockTimer;
  private final IbftMessageTransmitter transmitter;
  private final MessageFactory messageFactory;
-  private final Map<Integer, RoundState> futureRoundStateBuffer = Maps.newHashMap();
+  private final Map<Integer, RoundState> futureRoundStateBuffer;


is this related to the incoming event queue size?

…new config options

CjHare · 2019-02-01T05:07:38Z

config/src/main/java/tech/pegasys/pantheon/config/IbftConfigOptions.java

@@ -39,4 +41,12 @@ public int getBlockPeriodSeconds() {
  public int getRequestTimeoutSeconds() {
    return ibftConfigRoot.getInteger("requesttimeoutseconds", DEFAULT_ROUND_EXPIRY_SECONDS);
  }
+
+  public int getSeenMessageQueueSize() {


s/getSeenMessageQueueSize/getSeenMessageQueueLimit

QueueSize could be mistaken as being the current queue size

I like limit better. Still don't like these config names though regardless of size or limit suffix. So if have any suggestions...

SeenMessages is less obvious, I'll have to think on it.

CjHare

Please don't use size when you mean limit. Consider common use of size in places such as the Collection classes

PR changed the approved code

jframe · 2019-02-01T05:10:14Z

Please don't use size when you mean limit. Consider common use of size in places such as the Collection classes

Intent was to be the max size

CjHare · 2019-02-01T05:25:10Z

config/src/main/java/tech/pegasys/pantheon/config/IbftConfigOptions.java

+    return ibftConfigRoot.getInteger("seenmessagequeuesize", DEFAULT_SEEN_MESSAGES_QUEUE_SIZE);
+  }
+
+  public int getEventQueueSize() {


The more appropriate name for this variable would be messageQueueLimit, as it is pretty much the standard use for a MQ. This should make sense / easy inference by enterprise users of the product.

I like that

done. changed eventQueueSize -> messageQueueLimit and seenMessageQueueSize -> gossipedHistoryLimit

rain-on · 2019-02-03T22:36:00Z

config/src/main/java/tech/pegasys/pantheon/config/IbftConfigOptions.java

@@ -21,6 +21,8 @@
  private static final long DEFAULT_EPOCH_LENGTH = 30_000;
  private static final int DEFAULT_BLOCK_PERIOD_SECONDS = 1;
  private static final int DEFAULT_ROUND_EXPIRY_SECONDS = 1;
+  private static final int DEFAULT_GOSSIPED_HISTORY_LIMIT = 10_000;


this FEELS like a huge number ... in a good system, should this not be about ... 100?

10,000 is probably large than it needs to be. It will however only take a small amount of memory as we are only storing the hashes. So I don't think we need to be too exact and can be a little generous.
Perhaps 1000 is a more appropriate default? I think this would handle to tracking the message hashes of few rounds. And a size of 100 would only just cover the total number of messages for the current round if we going to use 20 validators.

Now this may be disingenuous - in an ideal situation we only need to buffer enough to prevent re-gossiping gossiped packets.
Which probably means the last 'set' of packets i.e. once we've received a few Prepares, we SHOULDN'T receive any more gossiped proposals (and prior round msgs are discarded out of hand) ... 100 may be plenty....
Happy to go as high as 1000 to handle spurious behaviours from peers .... but I think we should really understand how this works in both normal and byzantine circumstances before we throw in a safe number.

Might be worth thinking about if there's some metrics you could capture to help find the right value for this setting (even if you need to add some expensive ones temporarily). ie every time you find a message you've already gossiped, record how many messages are recorded after it in your history. That will likely have to be a temporary thing because you'll need to use a less efficient structure to hold the history (worst case a list you iterate from most recent to least recent), but it would give you some concrete data to drive decisions from.

We had some further discussion and settled on 1000 for now as this will cover the current height with several rounds.
Would be great to get some metrics on this. And would also allow users to tune this setting if they wanted to.

CjHare · 2019-02-04T01:54:36Z

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

  private static final Logger LOG = LogManager.getLogger();
+  private final int maxQueueSize;


What happened to using queueLimit?

It's still there. I missed updating the existing field name in the IbftEventQueue.

updated to make it consistent with the new config names

jframe added 8 commits January 29, 2019 10:53

move evicting map into it's own class

4fa3284

use evicting map for future msgs in ibftController and roundChangeMan…

fa03a32

…ager

add generics on evict map eviction method

593b7ca

config for message buffer size

bb6532e

test that MaxSizeEvictingMap evicts records when at capacity

968e758

rename map type

cf639a7

Merge branch 'master' into limit_ibft_msg_queues

11be71c

use SizeLimitedMap in the UniqueMessageMulticaster

aa3e2d6

jframe requested review from CjHare and rain-on January 30, 2019 02:51

CjHare previously approved these changes Jan 30, 2019

View reviewed changes

set event queue limit from the ibft message buffer size config

16467b8

rain-on reviewed Jan 31, 2019

View reviewed changes

jframe added 4 commits February 1, 2019 13:42

Merge branch 'master' into limit_ibft_msg_queues

d385a15

revert map and queue changes to future messages and round change and …

3695c51

…new config options

spotless

901a331

fix test context after config changes

13e3388

CjHare reviewed Feb 1, 2019

View reviewed changes

update config names

363f507

rain-on reviewed Feb 3, 2019

View reviewed changes

jframe added 4 commits February 4, 2019 08:41

update config names

b64829e

Update gossiped history limit to 1000

f5b1e2e

Update gossiped history limit to 1000 - test

4afd709

comment on why default gossipped history limit was chosen

08b79fe

CjHare reviewed Feb 4, 2019

View reviewed changes

update field names to match new config names

97af876

Merge branch 'master' into limit_ibft_msg_queues

be0bd99

CjHare approved these changes Feb 5, 2019

View reviewed changes

CjHare and others added 4 commits February 6, 2019 14:31

Merge branch 'master' into limit_ibft_msg_queues

d32c128

Merge branch 'master' into limit_ibft_msg_queues

96a3ab7

Merge branch 'master' into limit_ibft_msg_queues

14ab492

Merge branch 'master' into limit_ibft_msg_queues

66a5279

jframe merged commit e4291e6 into PegaSysEng:master Feb 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit ibft msg queues #704

Limit ibft msg queues #704

jframe commented Jan 30, 2019 •

edited

Loading

CjHare left a comment

rain-on Jan 31, 2019

jframe Feb 1, 2019

rain-on Jan 31, 2019

jframe Jan 31, 2019

rain-on Jan 31, 2019

CjHare Feb 1, 2019

jframe Feb 1, 2019

CjHare Feb 1, 2019

CjHare left a comment

jframe commented Feb 1, 2019

CjHare Feb 1, 2019

jframe Feb 1, 2019

jframe Feb 1, 2019

rain-on Feb 3, 2019

jframe Feb 3, 2019

rain-on Feb 3, 2019

ajsutton Feb 3, 2019

jframe Feb 4, 2019

CjHare Feb 4, 2019

jframe Feb 4, 2019

jframe Feb 4, 2019

		private static final Logger LOG = LogManager.getLogger();
		private final int maxQueueSize;

Limit ibft msg queues #704

Limit ibft msg queues #704

Conversation

jframe commented Jan 30, 2019 • edited Loading

PR description

Fixed Issue(s)

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

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

jframe commented Feb 1, 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

jframe commented Jan 30, 2019 •

edited

Loading