Add initial support for identity hashes (Part 1) #5117
Conversation
ghost
assigned 
|
After this goes in the next step will be to automatically use identity hashes as a way of inlining tiny blocks. But that change will likely require a go-cid update. |
This will be flood from all connected peers? |
I don't know enough bitswap to answer that. I just tested it using two peers, one with the P.R. and one without. However, I don't think it will be any worse than requesting any very common hash. The older node will store the identity-hash in its blockstore (like any other hash) so it won't have to request it again. |
Yes, but that's an issue with bitswap (and already happens for common blocks). |
Common blocks send only from peers that have it. Identity blocks will be send from all peers that knows identity hash. |
|
Unfortunately, there's really not much we can do here other than wait a few releases between this patch and one that allows users to use inline blocks. That is, users of existing releases of IPFS will always either:
I'd rather have 1. |
|
@Stebalien @whyrusleeping I would like to see this get in ASAP. Someone whose name I forgotten spoke to me during the dev. conf. and apparently is depending on this. Do you see any blockers to getting this reviewed and merged. A migration is not needed for this, it just that for a little some users may have identity hashes stored in the blockstore that can't be removed. Nothing should break. Gx dependency conflicts are starting to get on my nerves |
There was a problem hiding this comment.
Sorry I havent looked at this until now, The change looks really clean. I see no reason not to ship it. cc @magik6k or @schomatis for a quick once-over?
| @@ -196,18 +196,20 @@ func setupNode(ctx context.Context, n *IpfsNode, cfg *BuildCfg) error { | |||
| opts.HasBloomFilterSize = 0 | |||
| } | |||
|
|
|||
| cbs, err := bstore.CachedBlockstore(ctx, bs, opts) | |||
| wbs, err := bstore.CachedBlockstore(ctx, bs, opts) | |||
There was a problem hiding this comment.
Small nit: why is the CachedBlockstore renamed from cbs to wbs? Shouldn't wbs (which I'm not sure what it stands for) be defined later in NewIdStore?
There was a problem hiding this comment.
wbs stands for wrapped-data-store.
I could keep cbs and then defined yet another variable. But that could lead to bugs where cbs was used when wbs was meant to be used.
There was a problem hiding this comment.
Ok, I was just saying because the following self-definition seem a bit weird to me,
wbs = bstore.NewIdStore(wbs)
and I would have expected to create it from another blockstore, not from itself, like
wbs = bstore.NewIdStore(cbs)
There was a problem hiding this comment.
wbs stands for wrapped-data-store.
Shouldn't it be named wds instead? To avoid confusing it with a blockstore (like cbs).
|
I will rebase this on Thursday when I get back. |
|
Sorry I meant wrapped-block-store.
…On July 18, 2018 2:43:46 PM CEST, Lucas Molas ***@***.***> wrote:
schomatis commented on this pull request.
> @@ -196,18 +196,20 @@ func setupNode(ctx context.Context, n
*IpfsNode, cfg *BuildCfg) error {
opts.HasBloomFilterSize = 0
}
- cbs, err := bstore.CachedBlockstore(ctx, bs, opts)
+ wbs, err := bstore.CachedBlockstore(ctx, bs, opts)
> wbs stands for wrapped-data-store.
Shouldn't it be named `wds` instead? To avoid confusing it with a
blockstore (like `cbs`).
--
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub:
#5117 (comment)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
|
Rebased. |
This provides support for identity hashes by using the new blockstore in ipfs/go-ipfs-blockstore#4.
With this identity hashes just work without having to be stored anywhere. It also works with bitswap: if a node that doesn't have id-hash support requests an id-hash from one that does, it will be returned.
Towards #4697.