Storage Deals #117
Storage Deals #117
Conversation
magik6k
force-pushed
the
feat/deals
branch
3 times, most recently
from
e7382dc
to
a96fe81
Compare
chain/deals/client.go
Outdated
| if err := f.Close(); err != nil { | ||
| return cid.Undef, err | ||
| } | ||
| commP, err := sectorbuilder.GeneratePieceCommitment(f.Name(), 6) |
There was a problem hiding this comment.
😬 We need to get better interfaces than this from the proofs team.
chain/deals/handler.go
Outdated
| return | ||
| } | ||
|
|
||
| // TODO: Review: Not signed? |
There was a problem hiding this comment.
mmm, this is a good point. We need a signed deal response from the miner, and we're not getting it right here.
We need to clean up the spec around this.
| return err | ||
| } | ||
| if has { | ||
| // TODO: uncomment after deals work |
There was a problem hiding this comment.
It's basically 'allow duplicates'. I'd leave this in for now, at least until we implement payments, because it makes debugging really annoying
lib/nsbsnet/bsnet.go
Outdated
| @@ -0,0 +1,245 @@ | |||
| package nsbsnet | |||
There was a problem hiding this comment.
this is just a straight-up copy of the bitswap network package with the protocol ID prefix changes integrated... right? I'd say we should upstream this ASAP
node/modules/chain.go
Outdated
| bitswapNetwork := network.NewFromIpfsHost(host, rt) | ||
| // prefix protocol for chain bitswap | ||
| // (so bitswap uses /chain/ipfs/bitswap/1.0.0 internally for chain sync stuff) | ||
| bitswapNetwork := nsbsnet.NewFromIpfsHost(host, rt, "/chain") |
There was a problem hiding this comment.
hrm, why do we need to segregate this at all? It seems to me that what we really want is just to use sessions and pre-seed it with a specific peer ID, but keep that all on the same bitswap (one use case here is for ipfs peers to be able to read filecoin chain data natively)
There was a problem hiding this comment.
Mostly because blockstore for chain and client are separate, which makes implementing gc easier and is nice for badger gc too.
I think We'll never use bitswap for fetching to client blockstore, so we may be able to use tiered blockstore to virtually merge them.. Or just merge them and deal with gc later
There was a problem hiding this comment.
using a tiered blockstore seems fine to me. alternatively, what we're doing here can work for now, and seems not too difficult to change moving forward (entangling things should be easier than disentangling them)
There was a problem hiding this comment.
Overall, this is a good step in the right direction. Before merging, I think we should do a few things:
- break the long functions up a bit more. I commented on most of the places i think this should happen
- Don't use the bytepipebuffer thing. I think its just pulling technical debt into the project, and writing the data to a temp file for now and pushing hard on getting better interfaces from the sector builder is the right approach.
- Ideally we don't have to have separate bitswap instances, I get that its somewhat necessary right now due to datastore separation concerns. At the very least, lets upstream the bitswap network changes and use them here. That should be a pretty uncontroversial change and i think you and steven can get it merged and shipped really quickly.
After that, feel free to merge.
|
oops, i caused merge conflicts by merging #126 |
whyrusleeping
force-pushed
the
feat/deals
branch
from
9d8f090
to
998597f
Compare
* add ipc scripts * resolve validators when returnin subnet actor state * fix scripts. add docs. new export key method
No description provided.