[ETCM-247] Fix Fetcher updates #760
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
eb62350 to
525365d
Compare
- Fetcher will update last block and know top(only if is needed) if a mined or checkpointed block was imported - Fetcher will update know top if detect a block header with better top
525365d to
5fda427
Compare
ntallar
requested changes
Oct 28, 2020
|
|
||
| "given a previously mined blockchain" in customTestCaseResourceM(FakePeer.start2FakePeersRes()) { | ||
| case (peer1, peer2) => | ||
| val blockHeadersPerRequest = 200 |
There was a problem hiding this comment.
Should we use here peer2.syncConfig.blockHeadersPerRequest?
| //keep fetcher state updated in case new checkpoint block or mined block was imported | ||
| case LastBlockChanged(blockNr) => { | ||
| log.debug(s"New last block $blockNr imported from the inside") | ||
| val newState = state.withLastBlock(blockNr).withPossibleNewTopAt(blockNr) |
There was a problem hiding this comment.
Last block seems to also involve the last headers fetched, what if we are already in progress of fetching blocks from the network with block number higher than the one imported (as it's likely the case for checkpoint blocks)? Doesn't this break the consistency of the fetcher?
Maybe the new last block should be blockNr.max(state.lastBlock) or sth like that
kapke
reviewed
Oct 29, 2020
| @@ -232,6 +255,9 @@ class BlockImporter( | |||
| case ChainReorganised(oldBranch, newBranch, totalDifficulties) => | |||
| updateTxPool(newBranch, oldBranch) | |||
| broadcastBlocks(newBranch, totalDifficulties) | |||
| if (informFetcherOnLastBlockChanged) { | |||
There was a problem hiding this comment.
Minor - after changes introduced in ETCM-175 (on develop already), it should be enough to call fetcher from RegularSync actor after receiving update about imported block.
ntallar
approved these changes
Oct 29, 2020
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.
Description
According to https://jira.iohk.io/browse/ETCM-247, the desyncronization between importer and fetcher could cause several issues.
The goal of this PR is to provide an easy fix in order to keep fetcher state updated in case a node is only importing throw new mined blocks, or new checkpoint blocks.
Also, notice that previous to this PR, if a node(node2) wants to sync with other(node1), requires that its peer(node 1) is continuously generating blocks, if is not the case is going to catch up with only the first
nnumber of blocks (according toblock-headers-per-request). By subscribing to new block headers, now fetcher can update its vision about know top and react to this.^ issue: Block downloading doesn't start without any new block received from the network
Proposed Solution
Testing
TODO
Add tests for fetcher once ETCM-211 got merged (use code: ETCM-248)