-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add block quarantine to ForkedChain #3095
base: master
Are you sure you want to change the base?
Conversation
## This only stores blocks that cannot be linked to the | ||
## ForkedChain due to missing ancestor(s). | ||
|
||
orphans: OrderedTable[Hash32, Block] |
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.
minilru
is probably the better choice here, which should simplify the addOrphan logic (and perform better)
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.
a079c86
requires status-im/nim-minilru#3
eth/common/blocks | ||
|
||
const | ||
MaxOrphans = 128 |
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.
we will probably need to introduce an async queue or a similar construct to process quarantine blocks - if we proces 128 blocks in one async time slice, that will likely disconnect everyone due to the long "downtime"..
in the meantime, I think it's enough that we keep 32:ish blocks here, that's already 6 minutes of block time.
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.
Yeah agreed, mostly the sync lags by maximum of 10 blocks
32 seems like a more reasonable number to keep
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.
After I rethink about it, the block quarantine also overlap with importHeader
functionality, especially when importing blocks to FC. They can share the same async worker. And somehow related to #3002
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.
also overlap with
- not necessarily, ie it doesn't need headers to perform its work - when the normal syncer reaches a point here quarantine blocks exist, there's no need to separately download headers since the blocks in the quarantine already have headers too.
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.
ie I think the quarantine, even just as is, is a step forward towards stable sync
Currently the block quarantine implementation has no effect on improving sync since,
Here the arrival of the block means also means So in the current implementation when a nimbus-eth1/execution_chain/beacon/api_handler/api_newpayload.nim Lines 174 to 181 in 44db666
And currently we are only using the quarantine when we are trying to import a block into forked chain, which is fine. But in all the payloads we received with missing parent, are never added to the quarantine so in real life scenario never helps us nimbus-eth1/execution_chain/core/chain/forked_chain.nim Lines 523 to 545 in b3150ec
Ideally when the sync is happening we are getting a lot of |
fix #3056