Skip to content

Commit

Permalink
blockservice: remove inner fields access methods
Browse files Browse the repository at this point in the history
We shouldn't expose inner implementation details of the blockservice to consumers.

For example by using the blockstore for the .Has call, the gateway skipped verifcid and blocker checks. Probably fine but not we want to do.
  • Loading branch information
Jorropo committed Jan 15, 2024
1 parent 1da7fe3 commit 8de936c
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 38 deletions.
37 changes: 13 additions & 24 deletions blockservice/blockservice.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,8 @@ type BlockService interface {
io.Closer
BlockGetter

// Blockstore returns a reference to the underlying blockstore
Blockstore() blockstore.Blockstore

// Exchange returns a reference to the underlying exchange (usually bitswap)
Exchange() exchange.Interface
// Has indicates if a block is present locally.
Has(ctx context.Context, c cid.Cid) (bool, error)

// AddBlock puts a given block to the underlying datastore
AddBlock(ctx context.Context, o blocks.Block) error
Expand Down Expand Up @@ -132,21 +129,6 @@ func New(bs blockstore.Blockstore, exchange exchange.Interface, opts ...Option)
func NewWriteThrough(bs blockstore.Blockstore, exchange exchange.Interface) BlockService {
return New(bs, exchange, WriteThrough())
}

// Blockstore returns the blockstore behind this blockservice.
func (s *blockService) Blockstore() blockstore.Blockstore {
return s.blockstore
}

// Exchange returns the exchange behind this blockservice.
func (s *blockService) Exchange() exchange.Interface {
return s.exchange
}

func (s *blockService) Allowlist() verifcid.Allowlist {
return s.allowlist
}

func (s *blockService) NewSession(ctx context.Context) BlockGetter {
ses := s.grabSessionFromContext(ctx)
if ses != nil {
Expand Down Expand Up @@ -447,14 +429,13 @@ func (s *session) grabSession() exchange.Fetcher {
s.sesctx = nil // early gc
}()

ex := s.bs.Exchange()
if ex == nil {
if s.bs.exchange == nil {
return
}
s.ses = ex // always fallback to non session fetches

sesEx, ok := ex.(exchange.SessionExchange)
sesEx, ok := s.bs.exchange.(exchange.SessionExchange)
if !ok {
s.ses = s.bs.exchange // always fallback to non session fetches
return
}
s.ses = sesEx.NewSession(s.sesctx)
Expand Down Expand Up @@ -499,3 +480,11 @@ func (s *blockService) grabSessionFromContext(ctx context.Context) *session {

return sss
}

func (s *blockService) Has(ctx context.Context, c cid.Cid) (bool, error) {
if err := verifcid.ValidateCid(s.allowlist, c); err != nil {
return false, err
}

return s.blockstore.Has(ctx, c)
}
5 changes: 1 addition & 4 deletions gateway/blocks_backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"time"

"github.com/ipfs/boxo/blockservice"
blockstore "github.com/ipfs/boxo/blockstore"
"github.com/ipfs/boxo/fetcher"
bsfetcher "github.com/ipfs/boxo/fetcher/impl/blockservice"
"github.com/ipfs/boxo/files"
Expand Down Expand Up @@ -51,7 +50,6 @@ import (

// BlocksBackend is an [IPFSBackend] implementation based on a [blockservice.BlockService].
type BlocksBackend struct {
blockStore blockstore.Blockstore
blockService blockservice.BlockService
dagService format.DAGService
resolver resolver.Resolver
Expand Down Expand Up @@ -143,7 +141,6 @@ func NewBlocksBackend(blockService blockservice.BlockService, opts ...BlocksBack
}

return &BlocksBackend{
blockStore: blockService.Blockstore(),
blockService: blockService,
dagService: dagService,
resolver: r,
Expand Down Expand Up @@ -680,7 +677,7 @@ func (bb *BlocksBackend) IsCached(ctx context.Context, p path.Path) bool {
return false
}

has, _ := bb.blockStore.Has(ctx, rp.RootCid())
has, _ := bb.blockService.Has(ctx, rp.RootCid())
return has
}

Expand Down
28 changes: 18 additions & 10 deletions ipld/merkledag/merkledag_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,13 @@ import (
"testing"
"time"

testinstance "github.com/ipfs/boxo/bitswap/testinstance"
tn "github.com/ipfs/boxo/bitswap/testnet"
. "github.com/ipfs/boxo/ipld/merkledag"
mdpb "github.com/ipfs/boxo/ipld/merkledag/pb"
dstest "github.com/ipfs/boxo/ipld/merkledag/test"
mockrouting "github.com/ipfs/boxo/routing/mock"
delay "github.com/ipfs/go-ipfs-delay"

bserv "github.com/ipfs/boxo/blockservice"
bstest "github.com/ipfs/boxo/blockservice/test"
Expand Down Expand Up @@ -507,10 +511,12 @@ func TestCantGet(t *testing.T) {
}

func TestFetchGraph(t *testing.T) {
var dservs []ipld.DAGService
bsis := bstest.Mocks(2)
for _, bsi := range bsis {
dservs = append(dservs, NewDAGService(bsi))
net := tn.VirtualNetwork(mockrouting.NewServer(), delay.Fixed(0))
sg := testinstance.NewTestInstanceGenerator(net, nil, nil)
instances := sg.Instances(2)
dservs := [2]ipld.DAGService{
NewDAGService(bserv.New(instances[0].Blockstore(), instances[0].Exchange)),
NewDAGService(bserv.New(instances[1].Blockstore(), instances[1].Exchange)),
}

read := io.LimitReader(u.NewTimeSeededRand(), 1024*32)
Expand All @@ -522,7 +528,7 @@ func TestFetchGraph(t *testing.T) {
}

// create an offline dagstore and ensure all blocks were fetched
bs := bserv.New(bsis[1].Blockstore(), offline.Exchange(bsis[1].Blockstore()))
bs := bserv.New(instances[1].Blockstore(), nil)

offlineDS := NewDAGService(bs)

Expand All @@ -547,10 +553,12 @@ func TestFetchGraphWithDepthLimit(t *testing.T) {
}

testF := func(t *testing.T, tc testcase) {
var dservs []ipld.DAGService
bsis := bstest.Mocks(2)
for _, bsi := range bsis {
dservs = append(dservs, NewDAGService(bsi))
net := tn.VirtualNetwork(mockrouting.NewServer(), delay.Fixed(0))
sg := testinstance.NewTestInstanceGenerator(net, nil, nil)
instances := sg.Instances(2)
dservs := [2]ipld.DAGService{
NewDAGService(bserv.New(instances[0].Blockstore(), instances[0].Exchange)),
NewDAGService(bserv.New(instances[1].Blockstore(), instances[1].Exchange)),
}

root := makeDepthTestingGraph(t, dservs[0])
Expand All @@ -561,7 +569,7 @@ func TestFetchGraphWithDepthLimit(t *testing.T) {
}

// create an offline dagstore and ensure all blocks were fetched
bs := bserv.New(bsis[1].Blockstore(), offline.Exchange(bsis[1].Blockstore()))
bs := bserv.New(instances[1].Blockstore(), offline.Exchange(instances[1].Blockstore()))

offlineDS := NewDAGService(bs)

Expand Down

0 comments on commit 8de936c

Please sign in to comment.