Skip to content
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

ChainDB: let the BlockFetch client add blocks asynchronously #2721

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mrBliss
Copy link
Contributor

@mrBliss mrBliss commented Nov 2, 2020

Fixes IntersectMBO/ouroboros-consensus#655.

Currently, the effective queue size when adding blocks to the ChainDB is 1 (for
why, see IntersectMBO/ouroboros-consensus#655). In this commit, we let the BlockFetch client add blocks fully
asynchronously to the ChainDB, which restores the effective queue size to the
configured value again, e.g., 10.

The BlockFetch client will no longer wait until the block has been written to
the VolatileDB (and thus also not until the block has been processed by chain
selection). The BlockFetch client can just hand over the block and continue
downloading with minimum delay. To make this possible, we change the behaviour
of getIsFetched and getMaxSlotNo to account for the blocks in the queue,
otherwise the BlockFetch client might try to redownload already-fetched blocks.

This is an alternative to #2489, which let the BlockFetch client write blocks to
the VolatileDB synchronously. The problem with that approach is that multiple
threads are writing to the VolatileDB, instead of a single background thread. We
have relied on the latter to simplify the VolatileDB w.r.t. consistency after
incomplete writes.

@mrBliss mrBliss added the consensus issues related to ouroboros-consensus label Nov 2, 2020
@mrBliss
Copy link
Contributor Author

mrBliss commented Nov 2, 2020

Draft because the impact on bulk sync speed should be measured.

@mrBliss mrBliss force-pushed the mrBliss/add-blocks-fully-async branch 2 times, most recently from 6e98685 to aa1413e Compare November 3, 2020 07:58
return $ \pt ->
case pointToWithOriginRealPoint pt of
Origin -> False
NotOrigin pt' -> checkBlocksToAdd pt' || checkVolDb (realPointHash pt')
Copy link
Contributor Author

@mrBliss mrBliss Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Race condition: when a block has been removed but not yet written to the VolatileDB, this will return False. Possible solution: in the background thread, peek the block, process it, and then remove it from the queue. Add a comment that these two are not disjoint.

cc: @edsko.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thing someone is paying attention 🙄

@mrBliss mrBliss force-pushed the mrBliss/add-blocks-fully-async branch from c5b3a28 to 2defb1f Compare November 3, 2020 09:47
Fixes #2487.

Currently, the effective queue size when adding blocks to the ChainDB is 1 (for
why, see #2487). In this commit, we let the BlockFetch client add blocks fully
asynchronously to the ChainDB, which restores the effective queue size to the
configured value again, e.g., 10.

The BlockFetch client will no longer wait until the block has been written to
the VolatileDB (and thus also not until the block has been processed by chain
selection). The BlockFetch client can just hand over the block and continue
downloading with minimum delay. To make this possible, we change the behaviour
of `getIsFetched` and `getMaxSlotNo` to account for the blocks in the queue,
otherwise the BlockFetch client might try to redownload already-fetched blocks.

This is an alternative to #2489, which let the BlockFetch client write blocks to
the VolatileDB synchronously. The problem with that approach is that multiple
threads are writing to the VolatileDB, instead of a single background thread. We
have relied on the latter to simplify the VolatileDB w.r.t. consistency after
incomplete writes.
See the comment on `cdbBlocksToAdd`.
@mrBliss mrBliss force-pushed the mrBliss/add-blocks-fully-async branch from 2defb1f to 5299069 Compare November 3, 2020 09:49
Niols added a commit to IntersectMBO/ouroboros-consensus that referenced this pull request Jul 11, 2024
This is a port of PR IntersectMBO/ouroboros-network#2721 to the new ChainSelQueue.

Co-authored-by: mrBliss <thomas@well-typed.com>
Niols added a commit to IntersectMBO/ouroboros-consensus that referenced this pull request Jul 11, 2024
This is a port of PR IntersectMBO/ouroboros-network#2721 to the new ChainSelQueue.

Co-authored-by: mrBliss <thomas@well-typed.com>
facundominguez pushed a commit to IntersectMBO/ouroboros-consensus that referenced this pull request Jul 11, 2024
This is a port of PR IntersectMBO/ouroboros-network#2721 to the new ChainSelQueue.

Co-authored-by: mrBliss <thomas@well-typed.com>
facundominguez pushed a commit to IntersectMBO/ouroboros-consensus that referenced this pull request Jul 23, 2024
This is a port of PR IntersectMBO/ouroboros-network#2721 to the new ChainSelQueue.

Co-authored-by: mrBliss <thomas@well-typed.com>
facundominguez added a commit to IntersectMBO/ouroboros-consensus that referenced this pull request Aug 6, 2024
amesgen pushed a commit to IntersectMBO/ouroboros-consensus that referenced this pull request Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chain DB: addBlock queue ineffective
2 participants