feat(blockstore): implement blockstore by btc · Pull Request #85 · ipfs/kubo

btc · 2014-09-16T10:44:32Z

This PR adds a simple datastore wrapper so we don't need to repeat boilerplate pack/unpacking in places where the datastore is used.

type Blockstore interface {
    Get(u.Key) (*blocks.Block, error)
    Put(blocks.Block) error
}

How is this different from the blockservice?

The blockservice falls back to querying the exchange (bitswap) when values aren't present in the blockstore.

Where does this belong?

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.)

jbenet · 2014-09-16T10:52:08Z

lol, LGTM. i think top level or under util/blockservice both make sense

for use in tests

whyrusleeping · 2014-09-16T17:43:11Z

Im curious what input data could lead to a 'bad' block.

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

Multihash sum only fails when given an invalid hash type. Which, if were calling util.Hash, is guaranteed to be valid.

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.

hmm... no. but if thats something we are worried about, throwing tests around util.Hash might be a good idea.

Would tests around util.Hash remove the need to bubble up util.Hash's error return value when the function is used?

Honestly, I feel like they would. Just my opinion though.

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.

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.

Yeah, i agree with that. I was just being a proponent of adding more tests, including around util.Hash

btc · 2014-09-17T01:19:51Z

SGTM

@jbenet @whyrusleeping LGTU?

jbenet · 2014-09-17T01:25:31Z

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)

whyrusleeping · 2014-09-17T01:30:39Z

Oh yeah, this would be the perfect place to add thread safety to the datastores.

btc · 2014-09-17T04:19:16Z

Created an issue to remind us to address the thread-safety issues.

#87

Still, these changes here are a strict improvment over the existing state of affairs. Let's merge this one in for now. Ready when you are. @jbenet

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

feat(blockstore): implement blockstore

42514f2

btc added the status/in-progress In progress label Sep 16, 2014

Brian Tiger Chow added 2 commits September 16, 2014 03:53

style(blockstore) move to top-level

be83566

feat(util:testutil) add NewBlockOrFail(t, data) factory method

697625a

for use in tests

btc added codereview and removed status/in-progress In progress labels Sep 16, 2014

btc self-assigned this Sep 16, 2014

whyrusleeping reviewed Sep 16, 2014
View reviewed changes

btc mentioned this pull request Sep 17, 2014

Address Datastore thread-safety issues #87

Closed

btc pushed a commit that referenced this pull request Sep 17, 2014

Merge pull request #85 from jbenet/feat/blockstore

2a7dcb0

feat(blockstore): implement blockstore

btc merged commit 2a7dcb0 into master Sep 17, 2014

btc removed the codereview label Sep 17, 2014

btc deleted the feat/blockstore branch September 17, 2014 04:22

btc removed their assignment Mar 30, 2015

ribasushi pushed a commit that referenced this pull request Jul 4, 2021

fix: correctly add the skip option to Warningf

42cabfc

This time I've actually tested this as best I can (without, you know, adding actual unit tests). fixes #85

ariescodescream pushed a commit to ariescodescream/go-ipfs that referenced this pull request Oct 23, 2021

Merge pull request ipfs#85 from libp2p/gx/update-4ruucc

d6637d1

gx: update go-cid, go-multibase, base32

aschmahmann mentioned this pull request Dec 1, 2021

Release v0.11 #8343

Closed

80 tasks

cornwarecjp mentioned this pull request Aug 20, 2024

Download & upload IPNS records through API / CLI #10484

Closed

3 tasks

Uh oh!

Conversation

btc commented Sep 16, 2014

Uh oh!

jbenet commented Sep 16, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

btc commented Sep 17, 2014

Uh oh!

jbenet commented Sep 17, 2014

Uh oh!

whyrusleeping commented Sep 17, 2014

Uh oh!

btc commented Sep 17, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants