Cache current chain head info #1335
Conversation
…n head has already been imported.
…we wind up with two separate blockchain instances with the same backing store causing them to get out of sync.
| @@ -77,7 +82,7 @@ public static void setupSuite() { | |||
| @Before | |||
| public void setup() { | |||
| blockchainUtil = BlockchainSetupUtil.forTesting(); | |||
| blockchain = spy(blockchainUtil.getBlockchain()); | |||
| blockchain = blockchainUtil.getBlockchain(); | |||
There was a problem hiding this comment.
Using a spy here causes problems because spy doesn't wrap the existing object, it creates a new instance. So we wind up with two instances of DefaultMutableBlockchain - one inside BlockchainSetupUtil which isn't a spy and the one in blockchain which is and they wind up getting out of sync. Sadly this creates some pain in the two tests that wanted to verify how many times a block was imported but it's at least a little more controlled and we consistently use the spies.
There was a problem hiding this comment.
What if you wrap the original object in a spy inside of BlockchainSetupUtil?
There was a problem hiding this comment.
It's possible to do it that way but using spies is a really invasive technique - you don't get the test isolation you do from mocks but pick up the brittleness of testing specific interactions rather than results. So I was really hesitant to embed it in such a commonly used util and encourage it to spread.
| () -> | ||
| bestPeer.hasOutstandingRequests() | ||
| || otherPeers.stream().anyMatch(RespondingEthPeer::hasOutstandingRequests) | ||
| || localBlockchain.getChainHeadBlockNumber() >= bestPeerChainHead); |
There was a problem hiding this comment.
I suspect this test is intermittent in master, but it becomes really intermittent once you make getChainHeadBlockNumber much faster. So we wait for there to be an outstanding request to respond to.
| @@ -171,6 +175,10 @@ private BlockAddedEvent appendBlockHelper( | |||
| final BlockAddedEvent blockAddedEvent = updateCanonicalChainData(updater, block, td); | |||
|
|
|||
| updater.commit(); | |||
There was a problem hiding this comment.
Do we need to synchronize around the commit + chainHeader / totalDifficulty updates?
There was a problem hiding this comment.
They are called from the appendBlock method which is synchronized.
| @@ -77,7 +82,7 @@ public static void setupSuite() { | |||
| @Before | |||
| public void setup() { | |||
| blockchainUtil = BlockchainSetupUtil.forTesting(); | |||
| blockchain = spy(blockchainUtil.getBlockchain()); | |||
| blockchain = blockchainUtil.getBlockchain(); | |||
There was a problem hiding this comment.
What if you wrap the original object in a spy inside of BlockchainSetupUtil?
Store the header for the current chain head and total difficulty to avoid RocksDB lookups when requesting those common values. Also uses that cache to avoid a database lookup when checking if a block has already been imported if the block's parent is the current chain head.
Store the header for the current chain head and total difficulty to avoid RocksDB lookups when requesting those common values. Also uses that cache to avoid a database lookup when checking if a block has already been imported if the block's parent is the current chain head.
Store the header for the current chain head and total difficulty to avoid RocksDB lookups when requesting those common values. Also uses that cache to avoid a database lookup when checking if a block has already been imported if the block's parent is the current chain head.
PR description
Cache the current chain head in
DefaultMutableBlockchainto avoid having to retrieve it's info from RocksDB. For the common case of getting the current chain head number that saves 2 DB reads. When importing blocks we take advantage of having cached the header when checking if the block has already been imported - if it's parent is the chain head it can't have been imported (or it would be the chain head) which saves a DB lookup in the most common case of extending chain head.This primarily improves fast sync times where RocksDB read and write is being hammered by the world state import.