-
Notifications
You must be signed in to change notification settings - Fork 891
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add debug_getBadBlocks method #1378
Add debug_getBadBlocks method #1378
Conversation
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
…' into feature/add-debug-get-bad-blocks
…' into feature/add-debug-get-bad-blocks Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
15ead17
to
2f8e7a8
Compare
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
final List<BadBlockResult> response = new ArrayList<>(); | ||
final BadBlockManager badBlockManager = | ||
protocolSchedule.getByBlockNumber(blockchain.headBlockNumber()).getBadBlocksManager(); | ||
badBlockManager |
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.
Maybe .stream().map(...).collect(Collectors.toList)
instead of adding to a mutable collection?
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.
Done
import org.apache.tuweni.bytes.Bytes; | ||
|
||
@JsonPropertyOrder({"block", "hash", "rlp"}) | ||
public class BadBlockResult { |
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.
What was your take on the Immutable Framework? Did you like it/not like it? This is another opportunity here.
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.
yes I find it very useful. just forgot to use it :) updated
|
||
public class BadBlockManager { | ||
|
||
private final Map<Hash, Block> badBlocks = new ConcurrentHashMap<>(); |
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.
We want some sort of a limit on the bad blocks we keep around. Maybe via com.google.common.cache.CacheBuilder
?
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.
Geth said they keep on the order of 100 in memory.
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.
Indeed I wanted to use the cacheBuilder but as I understood that we did not want a limit for this cache I did not set it up. But anyway the choice to put a limit is much better
new InvalidBlockException( | ||
"Header failed validation.", child.getNumber(), child.getHash())); | ||
|
||
final BlockHeader invalidBlock = child; |
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.
Add some comments explaining that even though the header is known bad we are downloading the block body for the debug_badBlocks RPC.
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.
Done
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM karim.t2am@gmail.com
PR description
Request
Response
Test Performed on a local testnet :
Changelog