feat(blockstore): implement blockstore #85
Conversation
|
lol, LGTM. i think top level or under util/blockservice both make sense |
| // behavior in tests. Thus, it is left up to the caller to select the msgData | ||
| // that will determine the blocks key. | ||
| func NewBlockOrFail(t *testing.T, msgData string) blocks.Block { | ||
| block, blockCreationErr := blocks.NewBlock([]byte(msgData)) |
There was a problem hiding this comment.
Im curious what input data could lead to a 'bad' block.
There was a problem hiding this comment.
Mainly, creation can fail when multihash.Sum fails.
https://github.com/jbenet/go-ipfs/blob/master/blocks/blocks.go#L16
https://github.com/jbenet/go-multihash/blob/master/sum.go
There was a problem hiding this comment.
Multihash sum only fails when given an invalid hash type. Which, if were calling util.Hash, is guaranteed to be valid.
There was a problem hiding this comment.
Can we prove the code can't regress?
If multihash stipulates in its contract that it can fail, it's something we have to respect. If the belief is that multihash could provide a stronger contract, it's something to inspect within go-multihash. From the looks of it, the mh's error is there to guard against modifications that break that lib.
There was a problem hiding this comment.
hmm... no. but if thats something we are worried about, throwing tests around util.Hash might be a good idea.
There was a problem hiding this comment.
Would tests around util.Hash remove the need to bubble up util.Hash's error return value when the function is used?
There was a problem hiding this comment.
Honestly, I feel like they would. Just my opinion though.
There was a problem hiding this comment.
Also, since we are vendoring multihash, we should do our due dilligence to make sure we arent updating a version of a lib that may break our code.
There was a problem hiding this comment.
What are the benefits of ignoring the error?
If a function can return an error, the caller should explicitly handle that error. It's never a good idea to start making exceptions to this rule. When callers start ignoring the contracts stipulated by the libraries they use, the system becomes a house of cards.
There was a problem hiding this comment.
Yeah, i agree with that. I was just being a proponent of adding more tests, including around util.Hash
|
SGTM @jbenet @whyrusleeping LGTU? |
|
LGTM except for: careful, it's not threadsafe. i think this thing will get used all over the place. I think it may be useful to either add an internal mutex, or just wrap the incoming datastore in a LockDatastore that does this generically. (needs to be written, but trivial) |
|
Oh yeah, this would be the perfect place to add thread safety to the datastores. |
feat(blockstore): implement blockstore
This time I've actually tested this as best I can (without, you know, adding actual unit tests). fixes #85
gx: update go-cid, go-multibase, base32
This PR adds a simple
datastorewrapper so we don't need to repeat boilerplate pack/unpacking in places where the datastore is used.The
blockservicefalls back to querying theexchange(bitswap) when values aren't present in theblockstore.I'm not so sure myself. Either as a top-level package or as as
util/blockstore. Due to circular dependencies there are a number of places where it certainly cannot go (dht,bitswap,blockservice, etc.)