Limit size of Ibft future message buffer #873
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| /* | ||
| * Copyright 2019 ConsenSys AG. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on | ||
| * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations under the License. | ||
| */ | ||
| package tech.pegasys.pantheon.consensus.ibft.statemachine; | ||
|
|
||
| import tech.pegasys.pantheon.ethereum.p2p.api.Message; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.NavigableMap; | ||
| import java.util.TreeMap; | ||
| import java.util.function.Supplier; | ||
|
|
||
| import com.google.common.annotations.VisibleForTesting; | ||
|
|
||
| /** | ||
| * Buffer which holds future IBFT messages. | ||
| * | ||
| * <p>This buffer only allows messages to be added which have a chain height greater than current | ||
| * height and up to chain futureMessagesMaxDistance from the current chain height. | ||
| * | ||
| * <p>When the total number of messages is greater futureMessagesLimit then messages are evicted. | ||
| * | ||
| * <p>If there is more than one height in the buffer then all messages for the highest chain height | ||
| * are removed. Otherwise if there is only one height the oldest inserted message is removed. | ||
| */ | ||
| public class FutureMessageBuffer { | ||
| private final NavigableMap<Long, List<Message>> buffer; | ||
| private final long futureMessagesMaxDistance; | ||
| private final long futureMessagesLimit; | ||
| private final Supplier<Long> currentChainHeight; | ||
| private long totalMessages; | ||
|
|
||
| public FutureMessageBuffer( | ||
| final long futureMessagesMaxDistance, | ||
| final long futureMessagesLimit, | ||
| final Supplier<Long> currentChainHeight) { | ||
| this(futureMessagesMaxDistance, futureMessagesLimit, currentChainHeight, new TreeMap<>()); | ||
| } | ||
|
|
||
| @VisibleForTesting | ||
| FutureMessageBuffer( | ||
|
There was a problem hiding this comment. Is this constructor really of use? ...looks like when it's called in the tests a There was a problem hiding this comment. 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. There was a problem hiding this comment. That's a tight coupling between the test and the specific implementation of the logic, I'm not convinced that's the best approach. There was a problem hiding this comment. done. updated so don't need special constructor |
||
| final long futureMessagesMaxDistance, | ||
| final long futureMessagesLimit, | ||
| final Supplier<Long> currentChainHeight, | ||
| final NavigableMap<Long, List<Message>> buffer) { | ||
| this.futureMessagesMaxDistance = futureMessagesMaxDistance; | ||
| this.futureMessagesLimit = futureMessagesLimit; | ||
| this.currentChainHeight = currentChainHeight; | ||
| this.buffer = buffer; | ||
| } | ||
|
|
||
| public void addMessage(final long msgChainHeight, final Message rawMsg) { | ||
| final long currentHeight = currentChainHeight.get(); | ||
| if (!validMessageHeight(msgChainHeight, currentHeight)) { | ||
| return; | ||
| } | ||
|
|
||
| if (totalMessages >= futureMessagesLimit) { | ||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. 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. |
||
| evictMessages(); | ||
| } | ||
|
|
||
| if (!buffer.containsKey(msgChainHeight)) { | ||
|
There was a problem hiding this comment. nit: could use computeIfAbsent/putIfAbsent |
||
| buffer.put(msgChainHeight, new ArrayList<>()); | ||
|
There was a problem hiding this comment. what if this msgChainHeight is higher than the height removed in evictMessage? Eg. MsgChainHeight is 5, but we just removed chain-height 4 ... There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. |
||
| } | ||
| buffer.get(msgChainHeight).add(rawMsg); | ||
| totalMessages += 1; | ||
| } | ||
|
|
||
| private boolean validMessageHeight(final long msgChainHeight, final long currentHeight) { | ||
| final boolean isFutureMsg = msgChainHeight > currentHeight; | ||
| final boolean withinMaxChainHeight = | ||
| msgChainHeight <= currentHeight + futureMessagesMaxDistance; | ||
| return isFutureMsg && withinMaxChainHeight; | ||
| } | ||
|
|
||
| private void evictMessages() { | ||
| if (buffer.size() > 1) { | ||
| buffer.remove(buffer.lastKey()); | ||
| } else { | ||
| List<Message> messages = buffer.firstEntry().getValue(); | ||
|
There was a problem hiding this comment. Is it possible that buffer size is 0? I guess not. There was a problem hiding this comment. 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. There was a problem hiding this comment. done. added handling of zero message limit. |
||
| messages.remove(0); | ||
| } | ||
| } | ||
|
|
||
| public List<Message> poll(final long chainHeight) { | ||
| final List<Message> messages = buffer.getOrDefault(chainHeight, Collections.emptyList()); | ||
| buffer.remove(chainHeight); | ||
| return messages; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would probably prefer to see the futureMessageBuffer get created here and passed in (rather than the raw values) - why? Can't answer that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 ^^^
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 futureMessageBufferThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is cleaner and also reduces the need the seperate constructor in the IbftController.
Done.