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: skip location for memory store #846

Merged
merged 2 commits into from
Sep 13, 2019

Conversation

tuxcanfly
Copy link
Member

While here, fix docs examples which were failing due to improperly configured blockstore.

Also a minor nit: new bcoin.Chain() instantiates a new Chain without a blockstore, since options is null. This is because we only validate options if they are given, but if given we require a blockstore (except in spv). We could add another check if options is null. Thoughts?

@codecov-io

This comment has been minimized.

@pinheadmz
Copy link
Member

pinheadmz commented Sep 13, 2019

Encountered the same thing writing #832.

Chain doesn't seem to use memory. Meanwhile, creating a Blockstore is different from kinda every other module in bcoin, in that new Blockstore({memory: true}) is not valid, and LevelBlockStore requires a location, even if memory: true

@braydonf
Copy link
Contributor

braydonf commented Sep 13, 2019

Yeah, location isn't necessary in the memory case of blockstore.create.

I still think it would be better to have the examples show usage of persisting data, since that is a common case. This includes use of ensure and similar. That is probably for another PR, this is closer to current usage by a node.

@braydonf
Copy link
Contributor

braydonf commented Sep 13, 2019

@tuxcanfly

new bcoin.Chain()

Should likely remove the if (options) from lib/blockchain/chain:

if (options)
  this.fromOptions(options)

@braydonf
Copy link
Contributor

braydonf commented Sep 13, 2019

@pinheadmz

Chain doesn't seem to use memory.

If you grep for memory in lib/blockchain/chain.js and lib/blockchain/chaindb for usage you won't find it, however memory is still an option used for the database in ChainDB of Chain.

...creating a Blockstore...

That isn't a class, there is AbstractBlockStore, FileBlockStore and LevelBlockStore, and as detailed a blockstore.create function, as similar to the function for creating a bdb database.

@pinheadmz
Copy link
Member

@braydonf

I still think it would be better to have the examples show usage of persisting data, since that is a common case.

One thing I like about the examples is that they can just be run in place and show the user some quick output without writing temp junk to disk. Should we add an example demonstrating bcoin's usage of filesystem (including ensure(), etc)?

As discussed in #832 maybe we should just get rid of the docs/Examples entirely, move it to bcoin.io/guides or something? Issues like #831 are popping up as users try to base production code off the examples...

braydonf added a commit that referenced this pull request Sep 13, 2019
blockstore: skip location for memory store
@braydonf braydonf merged commit a0ba933 into bcoin-org:master Sep 13, 2019
@tuxcanfly
Copy link
Member Author

@pinheadmz Like this PR (and yours) bought to notice - sometimes minor config changes creep without us noticing (especially around larger PRs like blockstore). So the examples are a rather nice way of testing bcoin clients and making sure none of the moving parts break. If we end up retaining them maybe we could have CI run them and check their exit code, just to see that they're working as intended (such a CI task would have caught this issue much earlier).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants