Prevent duplicate ibft messages being processed by state machine #811
Conversation
...c/integration-test/java/tech/pegasys/pantheon/consensus/ibft/support/TestContextBuilder.java
Show resolved
Hide resolved
|
|
||
| import java.util.Set; | ||
|
|
||
| public class MessageTracker { |
There was a problem hiding this comment.
This could probably do with an interface of "previouslySeen"? Such that the IbftController can only query if a message has been sent, where as the UniqueMulticaster can actually add new messages.
| private final Set<Hash> seenMessages; | ||
|
|
||
| public MessageTracker(final int gossipHistoryLimit) { | ||
| this.seenMessages = newSetFromMap(new SizeLimitedMap<>(gossipHistoryLimit)); |
There was a problem hiding this comment.
Wouldn't it be 'safer' to use a descendant of WeakHashMap instead of HashMap here? GC & all?
There was a problem hiding this comment.
WeakHashMap will drop messages surprisingly often even if the JVM hasn't hit its full memory allocation yet. Anytime it might have to expand the memory currently in use it will drop weak references first, so you can never count on anything being kept. So it's only ever suitable for a cache where values can be recreated on demand and even then I'd avoid it if possible because it makes GC more expensive.
There was a problem hiding this comment.
As AJS said, I want a stronger guarantee. And to ensure memory usage doesn't explode the size of the Map is limited.
| } | ||
|
|
||
| public void start() { | ||
| startNewHeightManager(blockchain.getChainHeadHeader()); | ||
| } | ||
|
|
||
| public void handleMessageEvent(final IbftReceivedMessageEvent msg) { | ||
| handleMessage(msg.getMessage()); | ||
| if (!messageTracker.hasSeenMessage(msg.getMessage().getData())) { |
There was a problem hiding this comment.
Maybe this should be reconsidered - i.e. it should say "If we have SEEN this message before, do not handle it" - whereas it currently says "If I have SENT/GOSSIPED this message before, do not handle it" ... I believe our requirement/concept should be the first of these.
There was a problem hiding this comment.
Thinking about it more your right. I'm preventing sent messages from being processed but it's previously received messages that we need to filter out.
There was a problem hiding this comment.
Added seperate buffer for seen messages in the IbftController
PR description
Prevent duplicate IBFT messages from being processed by the state machine.
Fixed Issue(s)