Limit size of Ibft future message buffer #873

jframe · 2019-02-15T06:46:50Z

PR description

This add a limit to the number of ibft messages that are stored in the future buffer.

There is a total number of messages that are allowed. And also a max distance from the current height that messages are allowed for.

If the total message limit is reached then all the messages in the highest chain height are removed.

Additionally if there is only one height then we will remove the oldest message to make room instead.

Fixed Issue(s)

… rules

…height

rain-on · 2019-02-17T21:33:46Z

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

@@ -307,7 +309,9 @@ private static ControllerAndState createControllerAndFinalState(
                    messageValidatorFactory),
                messageValidatorFactory),
            gossiper,
-            DUPLICATE_MESSAGE_LIMIT);
+            DUPLICATE_MESSAGE_LIMIT,


would probably prefer to see the futureMessageBuffer get created here and passed in (rather than the raw values) - why? Can't answer that.

👍 ^^^
It's cleaner, in terms of IoC and reduces number of constructors on the controller.
I'd suggest instead of passing in values, construct & pass in the objects:
final MessageTracker duplicateMessageTracker, final FutureMessageBuffer futureMessageBuffer

That is cleaner and also reduces the need the seperate constructor in the IbftController.
Done.

rain-on · 2019-02-17T21:36:00Z

...bft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/FutureMessageBuffer.java

+      return;
+    }
+
+    if (totalMessages >= futureMessagesLimit) {


I had assumed evictMessages() MAY need to loop (i.e. what if you need to remove more than 1 height?) - should this conditional become a loop in evict?

I don't think there is any need to remove any more than one height at a time. Each height must contain at least one message so if we remove a height we will have room for the message to be added.

rain-on · 2019-02-17T21:37:22Z

...bft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/FutureMessageBuffer.java

+    }
+
+    if (!buffer.containsKey(msgChainHeight)) {
+      buffer.put(msgChainHeight, new ArrayList<>());


what if this msgChainHeight is higher than the height removed in evictMessage? Eg. MsgChainHeight is 5, but we just removed chain-height 4 ...

Assuming that msgChainHeight is still within the max distance we would add a message for chain height 5. Which is perhaps is a bit odd. Maybe it would be better not to add in that case, essentially evicting itself as the highest chain height? Need to think about this a bit more.

Yeah, I think I'd rather see it evict itself. MAYBE you could do something like add the new message, then run the eviction process.

done. yeah I think it better for it to be evicted. changed the code to run the evict after after adding, avoids needing to special case this add case.

rain-on · 2019-02-17T21:44:23Z

...bft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/FutureMessageBuffer.java

+    if (buffer.size() > 1) {
+      buffer.remove(buffer.lastKey());
+    } else {
+      List<Message> messages = buffer.firstEntry().getValue();


Is it possible that buffer size is 0? I guess not.

I am making an assumption that it isn't 0 at this point but that only holds if futureMessagesLimit isn't 0. Be better if the evict messages handled that case, so i'll update evictMessages to handle that properly.

done. added handling of zero message limit.

… is handled

…ad of config values

CjHare · 2019-02-18T06:46:00Z

...bft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/FutureMessageBuffer.java

+  }
+
+  @VisibleForTesting
+  FutureMessageBuffer(


Is this constructor really of use? ...looks like when it's called in the tests a new TreeMap<> is passed in

It is, I'm using that so I can check the state of the map is correct in the tests without exposing the map to everyone. A new TreeMap<> is passed in, but I'm checking in my tests messages are added to the map correctly.

That's a tight coupling between the test and the specific implementation of the logic, I'm not convinced that's the best approach.

done. updated so don't need special constructor

…d messages or existing buffered messages

rain-on · 2019-02-19T22:10:01Z

...bft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/FutureMessageBuffer.java

+  }
+
+  private void addMessageToBuffer(final long msgChainHeight, final Message rawMsg) {
+    if (!buffer.containsKey(msgChainHeight)) {


nit: could use computeIfAbsent/putIfAbsent

rain-on · 2019-02-19T22:14:25Z

...bft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/FutureMessageBuffer.java

+    this.futureMessagesLimit = futureMessagesLimit;
+  }
+
+  public void addMessage(final long chainHeight, final long msgChainHeight, final Message rawMsg) {


don't LOVE msgChainHeight coming in on this API .... rather this get reset in the Poll somehow.

done.changed so it is using been passed in as a arg.

… futureMessageBuffer

…e message buffer

CjHare · 2019-02-20T07:40:43Z

...bft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/FutureMessageBuffer.java

+  private long chainHeight;
+
+  public FutureMessageBuffer(
+      final long futureMessagesMaxDistance, final long futureMessagesLimit, final int chainHeight) {


Drop the chainHeight and initialise it to zero, as the only place it's getting called in code is the IbftController that is passing in a hardcoded zero.

nice catch. that should not of been initialised to zero. fixed now.

rain-on · 2019-02-20T09:58:24Z

...bft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/FutureMessageBuffer.java

+    this.chainHeight = chainHeight;
+  }
+
+  public void updateChainHeight(final long chainHeight) {


sorry to be difficult, are we worried that both this and poll take "chainHeight" - but they are different numbers?
I.e. this is the number of the most recent block on the chain - whereas 'poll' is really the "next" block ...
Wondering if renaming these functions to (something like):

discardMessagesForHeightsBelow(x)

extractMessagesWithHeight(y)

The height in the poll is really the message chain height, whereas the updateChainHeight is the current chain height we are working so they are different. I think renaming the arg in poll to msgChainHeight might that clearer.

…t the current chain height

…sForHeight

rain-on · 2019-02-20T22:33:04Z

...bft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/FutureMessageBuffer.java

+  }
+
+  public List<Message> retrieveMessagesForHeight(final long height) {
+    this.chainHeight = height;


nit: no need for the 'this'

jframe added 8 commits February 15, 2019 11:17

Limit size of ibft future message buffer and add some simple eviction…

c863d21

… rules

Config options for future messages limit and max distance from chain …

f6c5cf4

…height

improve unit test

e3379da

java doc

6da82cd

test more cases

f8f0eea

var renames

de80a51

replace LinkedList with ArrayList as per error prone suggestions

7b60d63

improve test around boundary cases

fc11dc7

jframe requested review from CjHare and rain-on February 15, 2019 06:46

rain-on reviewed Feb 17, 2019

View reviewed changes

jframe added the work in progress Work on this pull request is ongoing label Feb 18, 2019

jframe added 3 commits February 18, 2019 13:07

ensure that total message size is determined correctly and limit of 0…

4d4978e

… is handled

move current chain height onto addMessage method to simplify constructor

4d1b6eb

pass in the duplicate message tracker and future message buffer inste…

b7a9370

…ad of config values

jframe removed the work in progress Work on this pull request is ongoing label Feb 18, 2019

CjHare reviewed Feb 18, 2019

View reviewed changes

jframe added 3 commits February 19, 2019 14:47

remove testing constructor

4753dd4

ensure that latest height is evicted regardless if it comes from adde…

86b7f0d

…d messages or existing buffered messages

Merge branch 'master' into futureMessageBuffer

1dbe40f

rain-on reviewed Feb 19, 2019

View reviewed changes

jframe added 4 commits February 20, 2019 13:48

use putIfAbsent to simplify adding messages

2c18c4f

Merge branch 'futureMessageBuffer' of github.com:jframe/pantheon into…

74832e2

… futureMessageBuffer

remove chain height from addMessage and move to another method

159b87f

discard messages for old chain heights when updating height for futur…

acc3dea

…e message buffer

CjHare reviewed Feb 20, 2019

View reviewed changes

jframe added 3 commits February 20, 2019 18:08

Merge branch 'master' into futureMessageBuffer

73a83f5

pass through blockchain current chain height

27413a8

shouldn't discard messages higher than chain height

9066aa4

ensure that controller polls future messages before updating height

c9bd2fa

rain-on reviewed Feb 20, 2019

View reviewed changes

jframe added 2 commits February 20, 2019 20:05

renamed poll height arg to make it clear it's the msg chain height no…

3236bad

…t the current chain height

combine poll and updateChainHeight with a single func retrieveMessage…

725aeb4

…sForHeight

rain-on approved these changes Feb 20, 2019

View reviewed changes

CjHare approved these changes Feb 20, 2019

View reviewed changes

jframe added 2 commits February 21, 2019 08:34

remove unneeded this

a4750a3

Merge branch 'master' into futureMessageBuffer

4a08565

jframe merged commit 7efd040 into PegaSysEng:master Feb 20, 2019

jframe deleted the futureMessageBuffer branch February 20, 2019 22:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit size of Ibft future message buffer #873

Limit size of Ibft future message buffer #873

jframe commented Feb 15, 2019

rain-on Feb 17, 2019

CjHare Feb 18, 2019

jframe Feb 18, 2019

rain-on Feb 17, 2019

jframe Feb 17, 2019

rain-on Feb 17, 2019

jframe Feb 17, 2019

rain-on Feb 18, 2019

jframe Feb 19, 2019

rain-on Feb 17, 2019

jframe Feb 17, 2019

jframe Feb 19, 2019

CjHare Feb 18, 2019

jframe Feb 18, 2019

CjHare Feb 18, 2019

jframe Feb 19, 2019

rain-on Feb 19, 2019

jframe Feb 20, 2019

rain-on Feb 19, 2019

jframe Feb 20, 2019

CjHare Feb 20, 2019

jframe Feb 20, 2019

rain-on Feb 20, 2019

jframe Feb 20, 2019

rain-on Feb 20, 2019

jframe Feb 20, 2019

Limit size of Ibft future message buffer #873

Limit size of Ibft future message buffer #873

Conversation

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

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