fix(core-blockchain): remove too early check for a chained block #2905
Conversation
In processBlocks() we would check whether the first downloaded block is chained and if not then we would return from processBlocks(). However, if that happens it leads to an "infinite" loop of downloading blocks, not applying any of them until the nodejs process runs out of memory to store all the blocks it has downloaded. A block that would cause this is (on devnet): height=1199883, id=5174570296595098048. Remove this early check. Later all blocks (not only the first one) are verified whether chained or not.
vasild
requested review from
air1one,
faustbrian,
kristjank and
spkjp
as code owners
ghost
added
|
This PR supersedes #2903. If we merge this one, then 2903 can be discarded. |
There was a problem hiding this comment.
I'm not convinced about this one. Each batch contains up to 10k blocks and without this we end up processing them all just to discard them anyway if the first block is unchained which occasionally happens because of how the parallel block download works. An alternative approach would be to just add && !Utils.isException(blocks[0].data) to the condition.
|
Adding "isException()" check would only solve the problem for block "height=1199883, id=5174570296595098048" in particular because we already have it listed as an exception. However there is a bigger problem - due to this check the nodejs process crashes whenever we receive an unchained block for which we don't have an exception. Checking the first of 10k blocks would indeed save us processing time if the first block turns out to be unchained. However that would only work in 1 of 10k cases - if the second or 10th or 100th block is unchained we will still process the remaining ones. So, the pro of this check is that we save processing time in 1 of 10k cases. The con is that it could cause the nodejs process to crash. IMO it hurts more than it helps. |
…ly-chained-check * ArkEcosystem/core/develop: test: mock missing wsServer release: 2.5.24 (#2908) fix(core-p2p): terminate blocked client connections fix(core-p2p): drop connections with malformed messages fix(core-api): return block timestamp for v2 transactions (#2906) fix(core-blockchain): only shift milestoneHeights[] if at that height (#2904) fix(crypto): use `anyOf` for transactions schema (#2894) fix(core-webhooks): cast params in condition checks (#2887) feat(core-p2p): use compression on the p2p level (#2886)
|
Superseded by #2903 |
In processBlocks() we would check whether the first downloaded block is
chained and if not then we would return from processBlocks(). However,
if that happens it leads to an "infinite" loop of downloading blocks,
not applying any of them until the nodejs process runs out of memory to
store all the blocks it has downloaded. A block that would cause this is
(on devnet): height=1199883, id=5174570296595098048.
Remove this early check. Later all blocks (not only the first one) are
verified whether chained or not.
A summary of what changes this PR introduces and why they were made.
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
Does this PR release a new version?
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
developbranch, not themasterbranchIf adding a new feature, the PR's description includes: