-
Notifications
You must be signed in to change notification settings - Fork 130
Only import block if it isn't already on the block chain #962
Only import block if it isn't already on the block chain #962
Conversation
@@ -37,6 +37,9 @@ public synchronized boolean importBlock( | |||
final Block block, | |||
final HeaderValidationMode headerValidationMode, | |||
final HeaderValidationMode ommerValidationMode) { | |||
if (context.getBlockchain().contains(block.getHash())) { |
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's the underlying BlockHashFunction
that get's used for IBFT? ...is that hash going to match the hash from the mainnet BlockHashFunction
, or could they be different due to the IBFT header funniness?
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.
This importer is used by only IBFT when we are using IBFT despite the name. It won't be comparing a block hash from mainnet to a block hash from IBFT. The underlying hash function that will be used is different and the hash function that will be used is the IbftBlockHashing::calculateHashOfIbftBlockOnChain which takes cares of header funniness you mentioned.
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.
To be clear, that importer is used by MainNet and IBFT, just Pantheon can't be simultaneously doing Ethash and IBFT. So all blocks will use a consistent block hash function.
By definition, two blocks are equal if and only if their block hash matches. Most important application of that principal is the fact that block headers are stored by their hash in RocksDB.
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.
LGTM.
CPU use has remained low on the testnet since yesterday so removing the draft PR status |
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.
LGTM
Leaving this as draft PR until ibft test nodes have been running for awhile to give confidence this fixes the high cpu issue.
PR description
This changes the block importer so that will only import the block if it is not already imported. This can happen when using an alternative consensus mechanism such as ibft which also imports blocks.
In the case of ibft, a block might already be importing when we get a new_block event and will unnecessarily import the same block twice. This means we will use more resources than necessary to import blocks.
Fixed Issue(s)