Rework chain pull to be concurrency friendly #1346
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can I take this one for a spin or not yet ready? |
|
Yes you can. |
|
Testing now with "legacy", looks normal. |
This was referenced Dec 12, 2019
|
Testing, in another network and yes looks much "cleaner" (granted that the nodes may have changed an is not the same situation, but still). Now, I went after panics related to Attached also the full rel8-chain_pool_root.log |
|
The panic is due to another instance of out of order locking. |
|
I've run tests with some private network topologies. At first glance looks like there is little improvement (pass rate [4/11] is the same like from before the change) in node synchronisation before there were cases in which one out of 7 nodes was not in sync at all. In this run, difference was more subtle (2 or 3 blocks were off) will update more after deeper analysis |
Now that multiple chain pull tasks can pull the same or mostly the same header chain, and in the mean time between arriving headers some blocks in the chain may be received and applied, CandidateForest needs to account for this concurrency. Do this by only retiring candidate entries on GC, setting the state to Applied when the block is committed to storage. When asynchronous ChainAdvance processing sees such an entry at the current block's parent, it knows to drop the hashes it collected to fetch and re-land the chain from the current hash.
NicolasDP
approved these changes
Dec 15, 2019
NicolasDP
reviewed
Dec 15, 2019
| @@ -393,6 +434,9 @@ impl CandidateForest { | |||
| debug_assert!(block.header().hash() == block_hash); | |||
| Ok(()) | |||
| } | |||
| CandidateData::Applied(_) => { | |||
| panic!("caching block {} which is already in storage", block_hash) | |||
There was a problem hiding this comment.
aren't you worried this will blow in production in some cases? can't we just "accept" the state and continue? If the block is already applied then this is not necessarily cause for panic, right?
There was a problem hiding this comment.
This should never happen as the blocks are checked with storage first before being cached.
This removes race conditions in the candidate forest, and handles out-of-order blocks cached due to lacking a parent.
mzabaluev
force-pushed
the
debug-candidate-map
branch
from
7132afa to
ce0af7f
Compare
|
I have added an improvement to remove a cause for panics that @rinor has observed and properly handle out-of-order blocks. |
|
Tested quickly under the same reproducible scenario and one is still there. The fastest way to reproduce this is: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now that multiple chain pull tasks can pull the same or mostly the same header chain, and in the mean time between arriving headers some blocks in the chain may be received and applied,
CandidateForestneeds to account for this concurrency.Do this by only retiring candidate entries on GC, setting the state to
Appliedwhen the block is committed to storage. When asynchronousChainAdvanceprocessing sees such an entry at the current block's parent, it knows to drop the hashes it collected to fetch and re-land the chain from the current hash.Fixes #1327