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.

Sign up for GitHub

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

Jump to bottom

Add file block storage #703

Merged
merged 31 commits into from
Apr 12, 2019
Merged

Add file block storage #703

merged 31 commits into from
Apr 12, 2019

Conversation

braydonf
Copy link
Contributor

@braydonf braydonf commented Feb 20, 2019 •
edited
Loading

This adds a new module blockstore for storing blocks. It adds a file block store that has several advantages:

There can be multiple implementations of the blockstore interface, to give better flexibility to how data is stored. Currently this includes FileBlockStore and LevelBlockStore, this could later be expanded to other methods.

@codecov-io

This comment has been minimized.

Sign in to view
@braydonf braydonf force-pushed the blockstore branch 2 times, most recently from e101f32 to c459753 Compare February 20, 2019 02:06
@braydonf braydonf added the stability / efficiency Denial of service, better resource usage label Feb 20, 2019
@braydonf braydonf force-pushed the blockstore branch 3 times, most recently from 94b8a2b to 1fa7ece Compare February 21, 2019 06:46
@braydonf
Copy link
Contributor Author

Here are some of the initial results with the benchmarks:

FileBlockStore SSD

real	0m6.444s
user	0m4.610s
sys	0m3.884s

LevelBlockStore SSD

real	1m18.667s
user	1m0.571s
sys	0m12.097s

FileBlockStore HDD

real	0m22.239s
user	0m6.652s
sys	0m6.436s

LevelBlockStore HDD

real	3m5.204s
user	1m11.664s
sys	0m11.664s

For the complete breakdown of the performance of each operation see: https://gist.github.com/braydonf/952ace7bcd7bbf529516e16a1d11e67e

@braydonf
Copy link
Contributor Author

bcoin-blockstore-bench-write
bcoin-blockstore-bench-read
bcoin-blockstore-bench-randomread
bcoin-blockstore-bench-prune

Charts were generated via this branch: https://github.com/braydonf/bcoin/tree/blockstore-charts and this commit braydonf@5f1eec7

@braydonf
Copy link
Contributor Author

braydonf commented Mar 5, 2019 •
edited
Loading

This PR does not yet integrate the block store to be used with the chain and node yet, @tuxcanfly has a work-in-progress branch at tuxcanfly@7d53f63 integrating the file block store with chain. And it's showing significant improvements to the initial sync time. From my testing it shaved around 12 hours from the sync for mainnet.

tuxcanfly
tuxcanfly reviewed Mar 5, 2019
View reviewed changes
lib/blockstore/level.js Outdated Show resolved Hide resolved
@braydonf braydonf force-pushed the blockstore branch 2 times, most recently from 1612c66 to f219e56 Compare March 7, 2019 22:36
@braydonf
Copy link
Contributor Author

braydonf commented Mar 7, 2019 •
edited
Loading

Okay, this PR now integrates file blockstore with chain and the fullnode, and also rebased on latest master.

@braydonf
Copy link
Contributor Author

braydonf commented Mar 9, 2019 •
edited
Loading

Here are the results from logging iotop for disk i/o during the IBD and chain sync for mainnet.

fileblockstore-sync-a
This was from the current PR branch at braydonf@823f483 with both the blocks and undo blocks stored with FileBlockStore.

fileblockstore-sync-b
This was from the previously mentioned branch at tuxcanfly@7d53f63 and did not yet include undo blocks in FileBlockStore.

leveldb-sync
This was from the latest master branch at: ea1c6be and stores blocks and undo blocks in LevelDB.

@braydonf braydonf mentioned this pull request Mar 12, 2019
Closed
@braydonf
Copy link
Contributor Author

Tested with pruned and SPV nodes, and is working well.

tuxcanfly
tuxcanfly approved these changes Mar 14, 2019
View reviewed changes
Copy link
Member

@tuxcanfly tuxcanfly left a comment

Choose a reason for hiding this comment

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

As benchmarks above show, there's a marked improvement in performance for writing files which boosts the initial sync time. LGTM with some minor updates 👍

test/blockstore-test.js Outdated Show resolved Hide resolved
test/blockstore-test.js Show resolved Hide resolved
test/blockstore-test.js Show resolved Hide resolved
lib/blockstore/file.js Show resolved Hide resolved
lib/blockstore/file.js Outdated Show resolved Hide resolved
test/blockstore-test.js Show resolved Hide resolved
test/blockstore-test.js Show resolved Hide resolved
lib/blockstore/file.js Outdated Show resolved Hide resolved
test/blockstore-test.js Show resolved Hide resolved
lib/blockstore/file.js Show resolved Hide resolved
@braydonf braydonf mentioned this pull request Mar 21, 2019
Closed
@pinheadmz
Copy link
Member

pinheadmz commented Mar 22, 2019 •
edited
Loading

I wonder if we should have a quick check to make sure all files are present. I renamed a blk file and got an error in the log, but bcoin continued to run:

[error] (node) ENOENT: no such file or directory, open '/Users/matthewzipkin/.bcoin/testnet/blocks/blk00001.dat'

... though it couldn't add any new blocks to the chain (the file I mangled was the tip).

Downstream in the logs some weird messages:
[warning] (net) Received unrequested block: 0000000000adb71f55f8eb49cd99c899ce6ec294ab2a07ae608c1917e422c8f3 (95.216.35.252:18333).

I guess losing a file screwed up network sync also?

Note Bitcoin Core runs this check at startup:

2019-03-22T21:25:43Z Checking all blk files are present...

@pinheadmz
Copy link
Member

Should we include bcoin.define('Blockstore', './blockstore'); in bcoin.js?
The README has a little guide on creating a blockstore but it's not available from the bcoin object.

@braydonf
Copy link
Contributor Author

There is a scan at the startup for all blk* and blu* files and that there are corresponding index entries for those files, if there is not, as in the case that the ./blocks/index has been removed or has become corrupted, it will regenerate the index from the files.

@braydonf braydonf added the ready for review Ready to be reviewed label Mar 25, 2019
@braydonf
Copy link
Contributor Author

braydonf commented Mar 25, 2019 •
edited
Loading

For clarity there are several follow-up PRs after this one to:

  • De-duplicate the transaction for the txindex and other indexing fixes for addrindex, and separating the indexes from chain as in refactor indexers #424 (updates on this are currently in progress).
  • Generate the chain data in consideration that blocks may already exist, for the case of importing from another local node, and other cases where regenerating the chain is necessary.

braydonf and others added 10 commits April 8, 2019 13:18
@braydonf
blockstore: index after write interrupt and use less memory
8fc605c
@braydonf
blockstore: index undo blocks from files
961f6ed
@tuxcanfly @braydonf
blockstore: add README and docs
6af6353
@braydonf
changelog: add blockstore changes and migration notes
f24f64b
@braydonf
blockstore: fix logger, improve messages
f3e517c
@braydonf
migrate: add time to migrate and compaction log message
8337176
@braydonf
bcoin: expose blockstore module
41ce504
@braydonf
blockstore: optimize block read and index
83824d7 
There is potential for around a 10% to 23% increase to the performance
of block reads by using `allocUnsafe`, however there is already around
a 3 to 6 times increase to the performance. While it's safe to use
`allocUnsafe` as the number of bytes read is checked to be the same
size as the buffer allocation, there is a potential for test cases
to introduce _other_ behavior for `fs.read` that may not have the
same behavior, though this isn't currently the case.
@braydonf
blockchain: remove blocks after commit
3457ccc
@braydonf
blockstore: close file if write or read throws
50fe51c
@braydonf
Copy link
Contributor Author

braydonf commented Apr 8, 2019

Okay, I think this is ready to land.

@braydonf
test: configurable and unique test directory path
5438357 
- Multiple parallel runs of the same test will not conflict
  as a unique identifier is added to the test directory.
- The base test directory can be configured for various
  environments, and can be changed via the environment
  variable `TEMP`, see the implementation of `os.tmpdir()`
  for specific details.
tynes
tynes reviewed Apr 11, 2019
View reviewed changes
Copy link
Member

@tynes tynes left a comment

Choose a reason for hiding this comment

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

Good job with the extensive test cases. I like test/blockstore-test.js.
Overall mostly nits, but this looks good to me. I have synced it a few times without a problem using Node.js v10.

test/blockstore-test.js Show resolved Hide resolved
@braydonf braydonf merged commit 5438357 into bcoin-org:master Apr 12, 2019
braydonf added a commit that referenced this pull request Apr 12, 2019
@braydonf
Merge pull request #703 from braydonf/blockstore
d601b6a 
Add file block storage
@braydonf braydonf mentioned this pull request Apr 12, 2019
Closed
@braydonf braydonf removed the ready for review Ready to be reviewed label Apr 12, 2019
@braydonf braydonf mentioned this pull request Apr 12, 2019
Closed
@braydonf braydonf deleted the blockstore branch April 12, 2019 20:40
@braydonf braydonf mentioned this pull request Sep 5, 2019
Closed
@braydonf braydonf added this to the 2.0.0 milestone Jan 6, 2020
@pinheadmz pinheadmz mentioned this pull request Jul 27, 2020
Closed
@nodech nodech mentioned this pull request Jun 11, 2021
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tynes tynes tynes left review comments

@pinheadmz pinheadmz pinheadmz left review comments

@chjj chjj chjj left review comments

@tuxcanfly tuxcanfly tuxcanfly approved these changes

Assignees
No one assigned
Labels
blockstore stability / efficiency Denial of service, better resource usage
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.
7 participants
@braydonf @codecov-io @pinheadmz @BluSyn @tynes @tuxcanfly @chjj