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

blockstore - separate write and prune batches. #688

Merged
merged 1 commit into from
Mar 7, 2022

Conversation

nodech
Copy link
Contributor

@nodech nodech commented Feb 8, 2022

ChainDB and BlockStore are separate and committing to them is not really atomic,
so we need to make sure the order operations can handle failure at any step.
Previously blockstore writes and prunes would happen after the chaindb was
written, but that could lead to the situation where the blockstore did not
actually have a block but chaindb had the information.
This PR separates write and prune batches for the blockstore, so we can try
writing to blockstore first. In case writing to blockstore fails whole operation
will get aborted. Nothing gets written into the chaindb, so the information is
consistent. In case blockstore writes a block and then chaindb write fails,
we just get extra data in the blockstore. But most of the time, the same block
write will happen again (either main or alt chains).
Pruning always happens after the chaindb was updated to avoid situation, where
chaindb thinks the data is stored, but blockstore has removed it. If blockstore
fails to prune after chaindb removed the information, worst case scenario
(prune mode) we get a maxFileLength(128 MB by default) space wasted.

Issue found here: kyokan/bob-wallet#453

@nodech nodech added bug general - Something isn't working intermediate review difficulty - intermediate blockstore part of the codebase labels Feb 8, 2022
@coveralls
Copy link

coveralls commented Feb 8, 2022

Pull Request Test Coverage Report for Build 1813298077

  • 49 of 53 (92.45%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 62.763%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/blockstore/file.js 23 25 92.0%
lib/blockstore/level.js 21 23 91.3%
Files with Coverage Reduction New Missed Lines %
lib/blockstore/level.js 1 89.41%
Totals Coverage Status
Change from base Build 1679021318: 0.03%
Covered Lines: 21355
Relevant Lines: 31832

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1812837669

  • 46 of 50 (92.0%) changed or added relevant lines in 3 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.03%) to 62.765%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/blockstore/file.js 22 24 91.67%
lib/blockstore/level.js 19 21 90.48%
Files with Coverage Reduction New Missed Lines %
lib/blockstore/level.js 1 89.41%
lib/protocol/consensus.js 1 82.79%
Totals Coverage Status
Change from base Build 1679021318: 0.03%
Covered Lines: 21355
Relevant Lines: 31832

💛 - Coveralls

lib/blockstore/file.js Outdated Show resolved Hide resolved
@nodech nodech force-pushed the blockstore-atomicity branch from 5e45750 to 9cded82 Compare February 8, 2022 15:45
ChainDB and BlockStore are separate and committing to them is not really atomic,
so we need to make sure the order operations can handle failure at any step.
Previously blockstore writes and prunes would happen after the chaindb was
written, but that could lead to the situation where the blockstore did not
actually have a block but chaindb had the information.
  This PR separates write and prune batches for the blockstore, so we can try
writing to blockstore first. In case writing to blockstore fails whole operation
will get aborted. Nothing gets written into the chaindb, so the information is
consistent. In case blockstore writes a block and then chaindb write fails,
we just get extra data in the blockstore. But most of the time, the same block
write will happen again (either main or alt chains).
  Pruning always happens after the chaindb was updated to avoid situation, where
chaindb thinks the data is stored, but blockstore has removed it. If blockstore
fails to prune after chaindb removed the information, worst case scenario
(prune mode) we get a maxFileLength(128 MB by default) space wasted.
@nodech nodech force-pushed the blockstore-atomicity branch 2 times, most recently from 86df238 to 6e62968 Compare February 8, 2022 16:00
@pinheadmz
Copy link
Member

Thanks for the fast work on this, LGTM now after a few nits. Makes sense to split up write/prune and execute them before/after the levelDB index as you've done. I wonder if this is the kind of thing we should add to a patch version and release, maybe Bob Wallet should do the same.

@nodech nodech requested a review from pinheadmz February 15, 2022 11:20
@pinheadmz pinheadmz merged commit a7e937c into handshake-org:master Mar 7, 2022
@nodech nodech added the patch Release version label Mar 9, 2022
@nodech nodech deleted the blockstore-atomicity branch January 6, 2023 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blockstore part of the codebase bug general - Something isn't working intermediate review difficulty - intermediate patch Release version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants